riksweeney / edgar

A 2D platform game with a persistent world. When Edgar's father fails to return home after venturing out one dark and stormy night, Edgar fears the worst: he has been captured by the evil sorcerer who lives in a fortress beyond the forbidden swamp.
https://www.parallelrealities.co.uk/games/edgar
116 stars 28 forks source link

Occasional crash when loading saved game due to invalid index #52

Closed lumidify closed 3 years ago

lumidify commented 3 years ago

Sometimes when a saved game is loaded, the program crashes while trying to load the resources, like so:

#0  loadResources (buffer=0x7ce819a75a "PLAYER_DATA") at src/system/resources.c:395
395                             e = addEntityFromResource(value[type], value[name], startX == -1 ? 0 : atoi(value[startX]), startY == -1 ? 0 : atoi(value[startY]));
(gdb) bt
#0  loadResources (buffer=0x7ce819a75a "PLAYER_DATA") at src/system/resources.c:395
#1  0x0000007a3ff1517d in loadGame (slot=6) at src/system/load_save.c:349
#2  0x0000007a3ff210e4 in continueGame () at src/menu/main_menu.c:439
#3  0x0000007a3ff20487 in doMenu () at src/menu/main_menu.c:174
#4  0x0000007a3ff21eb0 in doMenu () at src/menu/menu.c:40
#5  0x0000007a3feda48b in main (argc=1, argv=0x7f7ffffeaf28) at src/main.c:346
Current language:  auto; currently minimal
(gdb) p name
$1 = -1

I printed the buffer that's being parsed here and noticed that some blocks (surrounded by {}) at the end don't have any key called "NAME", which is being looked for here, leading to name being -1. Often, this does not cause an issue because indexing value[-1] just returns a null byte, which is subsequently ignored because the functions used to handle these blocks, such as addTriggerFromResource(), don't actually use that value. Other times, however, the program crashes since it isn't actually legal to index that position.

Modifying it like so fixed it for me (where name_value is declared at the top of the function):

if (name == -1)
{
    name_value = ""; /* or anything, really, since it's just ignored */
}
else
{
    name_value = value[name];
}
e = addEntityFromResource(value[type], name_value, startX == -1 ? 0 : atoi(value[startX]), startY == -1 ? 0 : atoi(value[startY]));

Does this seem okay or did I miss something?

polluks commented 3 years ago

Or how about e = addEntityFromResource(value[type], name == -1 ? "" : value[name], startX == -1 ? 0 : atoi(value[startX]), startY == -1 ? 0 : atoi(value[startY]));?

lumidify commented 3 years ago

Or how about e = addEntityFromResource(value[type], name == -1 ? "" : value[name], startX == -1 ? 0 : atoi(value[startX]), startY == -1 ? 0 : atoi(value[startY]));?

Right, that's much better, of course. No idea why I wanted to overcomplicate it so much...

riksweeney commented 3 years ago

If this fixes the issue for you then I can update the code base.

lumidify commented 3 years ago

Closing this as it has been fixed now. Thanks!