Buffer Overflow in `Options -> Controls ->Control Mapping` (debug x64 windows)

[GermanAizek]

@hrydgard, I just started debug build and wanted to reassign buttons, but my curCat turned out to be number 3 (Extended PSP controls), and at + 1 it went beyond array boundary, as I understood.

[GermanAizek]

@hrydgard, correct way is to make std::vector with fixed size and do not use direct acess by index + 1.

My fast cringe patch:

1. Append fourth category controls nullable.
2. Check on NULL catName, firstKey or open and access compare

// Category name, first input from psp_button_names.
static const Cat cats[] = {
    {"Standard PSP controls", CTRL_UP, &categoryToggles_[0]},
    {"Control modifiers", VIRTKEY_ANALOG_ROTATE_CW, &categoryToggles_[1]},
    {"Emulator controls", VIRTKEY_FASTFORWARD, &categoryToggles_[2]},
    {"Extended PSP controls", VIRTKEY_AXIS_RIGHT_Y_MAX, &categoryToggles_[3]},
    {NULL, NULL, NULL}
};

int curCat = -1;
CollapsibleSection *curSection = nullptr;
for (size_t i = 0; i < numMappableKeys; i++) {
    if (curCat < (int)ARRAY_SIZE(cats) && (cats[curCat + 1].catName != NULL && mappableKeys[i].key == cats[curCat + 1].firstKey)) {
        if (curCat >= 0) {
            curSection->SetOpenPtr(cats[curCat].open);
        }
        curCat++;

                ...

[hrydgard]

Hm, you're right, it's a bug. I don't think a vector is necessary here, I prefer the extra line with NULLs.

By the way, I removed the "Vulnerability" tag because I can't see a way to use this for anything nefarious. It could lead to crashes though indeed.

[hrydgard]

This has been fixed for a while, closing.