#400 Makebin: Buffer overrun and truncated output on .noi to .sym conversion

[bbbbbr]

This was found in SDCC r12539, but should hold true for any current version since there have been no changes to makebin aside from r12748 (no-nintendo logo option).

This patch fixes a small buffer overrun in the function noi2sym() that was causing the MinGW Windows build to prematurely close and truncate the .converted .sym file output (from .noi). After which it would dump the remaining symbol conversion data to the console. In some cases this resulted in a small ~6k file .sym file which instead should havebeen ~50k.

The issue did not visibly manifest on Linux, but the overrun does still occur.

The problem was as follows.

In some cases the code label[i+1] = '\0' was reached with "i" already pointing to the max item in the array, so when it writes to i+1 it stomps other memory and apparently forces the output file closed.

Since "i" still increments once more after the loop exit is reached, it should be "< 31" not "< 32" (for an array size of 33).
char label[33]; ... // read label for (i = 0; i < 32; ++i) { // Stuff } ... label[i+1] = '\0'; //<- buffer overrun when i == 32
Changes:
* Converted the string length to a constant for easier code maintenance.
* Removes a filter that was stripping symbol length records, which may be useful in some cases. I can remove that if you'd prefer the patch doesn't include it.

This function might be more compact if it used fgets() and strtok(), but I wanted to keep the code changes for the fix minimal. If there is interest in further changes I can propose those in a separate patch.

Attached:
* Example input .noi file
* Truncated output .sym file
* Non-truncated correct output .sym file generated once the patch was applied
* Patch for makebin

[bbbbbr]

Sorry about the bad formatting of the code snippet. Here is a second try on that.

char label[33];
...
// read label
for (i = 0; i < 32; ++i) {
    // Stuff
}
...
label[i+1] = '\0'; //<- buffer overrun when i == 32

[Philipp Klaus Krause]

Thanks. Applied in [r12757].

Feel free to submit a patch using fgets() and strtok() to improve makebin further.

I guess the Gameboy is the main use case for makebin these days (personally I use objcopy from GNU binutils to convert ihx to bin for e.g. the Z80-based ColecoVision), but I'm not sure.

Philipp

[bbbbbr]

Thank you!

OK, I'll look into a simpler version of the function.

Yes, Gameboy and clones may be it's primary use, with SMS/Game Gear to a lesser extent (GBDK and perhaps also Z88DK)