critter-mj / akochan

Artificial Intelligence for Japanese mahjong
Other
244 stars 38 forks source link

Bad access in Tehai_Analyzer_Basic::cut_tatu, with possible fix #28

Open SethRobinson opened 3 weeks ago

SethRobinson commented 3 weeks ago

There was a case where Tehai_Analyzer_Basic::cut_tatu would do an illegal access (tehai_tmp[hai+2]) would go one larger than the array size.

f(tehai_tmp[hai] && tehai_tmp[hai+2] && hai<30 && hai%10<=8){

This would run in release mode (probably checked the next variable, bad), but debug mode catches it.

Claude fixed it and it now works. I don't know how much I trust it, but here is the fixed version of Tehai_Analyzer_Basic::cut_tatu:

/* 
Claude says:

Looking at the code, this is in the cut_tatu function which is analyzing tile patterns in mahjong. The issue is that when checking for sequential patterns (hai+2), it's possible to access beyond the array bounds.
Let's analyze the checks in the problematic line:
cppCopyif(tehai_tmp[hai] && tehai_tmp[hai+2] && hai<30 && hai%10<=8)
The current checks are:

hai<30 - Ensures we're only looking at number tiles (not honors)
hai%10<=8 - Ensures we're not going past the end of a suit

However, there appears to be another edge case. 

The key changes I made to fix the buffer overflow issue:

Added additional bounds checking for kanchan (middle-waiting) patterns:

cppCopyif(tehai_tmp[hai] && (hai+2)/10 == hai/10 && tehai_tmp[hai+2])
The new check (hai+2)/10 == hai/10 ensures that hai and hai+2 are in the same suit. This is mathematically equivalent to the previous hai%10<=8 check but makes the intent clearer.

Reorganized the checks to be more explicit about when we're dealing with number tiles vs honor tiles.
Separated the sequential wait checks (ryanmen/penchan and kanchan) into distinct blocks for better clarity and safer bounds checking.
Made sure all array accesses are properly bounded by the additional checks.

This fix should prevent any out-of-bounds access while maintaining the original mahjong logic for:

Pair waits (toitsu)
Sequential waits (ryanmen/penchan)
Middle waits (kanchan)

The logic for calculating shanten and patterns remains unchanged, we just made the tile pattern checks safer.

*/
void Tehai_Analyzer_Basic::cut_tatu(const int my_pid_new, const Game_State& game_state, const Hai_Array& tehai_kcp, Hai_Array& tehai_tate_cut, Hai_Array& tehai_tmp, int mentu_num, int head_num, int kouho_num, int start, Tehai_Pattern_Source& tps) {
    if (mentu_num + get_fuuro_num() + kouho_num < 4) {
        for (int hai = start; hai < 38; hai++) {
            // Check for pair waits
            if (tehai_tmp[hai] == 2 && tehai_kcp[hai] != 4) {
                tehai_tmp[hai] -= 2;
                tps.tatu_hai.push_back(hai);
                tps.tatu_hai.push_back(hai);
                cut_tatu(my_pid_new, game_state, tehai_kcp, tehai_tate_cut, tehai_tmp, mentu_num, head_num, kouho_num + 1, hai, tps);
                tehai_tmp[hai] += 2;
                tps.tatu_hai.pop_back();
                tps.tatu_hai.pop_back();
            }

            // Check for sequential waits, but only for number tiles (hai < 30)
            // and ensure we don't go past end of suit or array bounds
            if (hai < 30 && hai % 10 <= 8) {
                // Check for ryanmen/penchan waits
                if (tehai_tmp[hai] && tehai_tmp[hai + 1]) {
                    tehai_tmp[hai]--;
                    tehai_tmp[hai + 1]--;
                    tps.tatu_hai.push_back(hai);
                    tps.tatu_hai.push_back(hai + 1);
                    cut_tatu(my_pid_new, game_state, tehai_kcp, tehai_tate_cut, tehai_tmp, mentu_num, head_num, kouho_num + 1, hai, tps);
                    tehai_tmp[hai]++;
                    tehai_tmp[hai + 1]++;
                    tps.tatu_hai.pop_back();
                    tps.tatu_hai.pop_back();
                }

                // Check for kanchan waits
                // Additional check to ensure hai+2 is within same suit
                if (tehai_tmp[hai] && (hai + 2) / 10 == hai / 10 && tehai_tmp[hai + 2]) {
                    tehai_tmp[hai]--;
                    tehai_tmp[hai + 2]--;
                    tps.tatu_hai.push_back(hai);
                    tps.tatu_hai.push_back(hai + 2);
                    cut_tatu(my_pid_new, game_state, tehai_kcp, tehai_tate_cut, tehai_tmp, mentu_num, head_num, kouho_num + 1, hai, tps);
                    tehai_tmp[hai]++;
                    tehai_tmp[hai + 2]++;
                    tps.tatu_hai.pop_back();
                    tps.tatu_hai.pop_back();
                }
            }
        }
    }

    int tmp = 8 - (mentu_num + get_fuuro_num()) * 2 - kouho_num - head_num;
    if (mentu_num + get_fuuro_num() == 4) {
        int flag = 1;
        for (int hai = 0; hai < 38; hai++) {
            if (tehai_tmp[hai] == 1 && tehai_kcp[hai] != 4) {
                flag = 0;
            }
        }
        if (flag == 1) {
            tmp++;
        }
    }
    if (tmp < get_mentu_shanten_num()) {
        set_mentu_shanten_num(tmp);
    }

    if (get_pattern_flag() == 1 && tmp <= get_mentu_change_num_max()) {
        analyze_koritu(my_pid_new, game_state, tehai_kcp, tehai_tate_cut, tehai_tmp, mentu_num, head_num, kouho_num, tps);
    }
}