SIGABRT: free(): invalid next size (fast)

[gurka]

I'm currently working on a tool that can aid in identifying the Tibia version of recordings, when it cannot be determined by inspecting the "message of the day" message in the login packet (e.g. "Your last visit in Tibia: 31 Dec 2004"). The tool basically iterates over the user's available data files and then attempts to processes the whole recording using tibiarc.

This works good in most cases, but some combinations of Tibia version and recording file results in a crash (see title). The one I've investigated so far seem to crash because the player's position is parsed incorrectly which later on causes map_GetTile to return an invalid tile (because the index is negative). That it turn causes it to write out-of-bounds and (in at least one case) breaks the trc_packet list. Finally an invalid free() is done which results in SIGABRT.

This seems to fix it:

diff --git a/lib/parser.c b/lib/parser.c
index 5780712..fc5fca1 100644
--- a/lib/parser.c
+++ b/lib/parser.c
@@ -329,6 +329,9 @@ static bool parser_ParseMapDescription(struct trc_data_reader *reader,
     int zIdx, endZ, zStep;
     uint16_t tileSkip;

+    ParseAssert(gamestate->Map.Position.X + xOffset >= 0);
+    ParseAssert(gamestate->Map.Position.Y + yOffset >= 0);
+
     if (gamestate->Map.Position.Z > 7) {
         zIdx = gamestate->Map.Position.Z - 2;
         endZ = MIN(15, gamestate->Map.Position.Z + 2);

Now, I guess the question is; should tibiarc be able to handle processing recordings using the wrong Tibia version (and wrong data files), and then error out gracefully, or is it OK to just crash (or assert)? Imo it would be nice for it to be able to error out gracefully, instead of just crashing, but it will require quite a lot of "defensive programming"... however, based on how good this approach of identifying Tibia version has worked so far it already seem to be handle unexpected things very good. :)

[jhogberg]

Thanks! Yes, we should handle it gracefully, one of the goals I set for myself with the cleanup of the old code was to weed out all UB including uncontrolled crashes. Given that it’s an ancient C code base written back when I was horrible at it (now I’m merely bad at it), this is not going to be the last crash we see 😅

The refactor of parsing I mentioned in #13 is limping along by the way, though it’s becoming apparent that I’m reaching the limit of what can sanely be expressed in C — the abstract packets come with a lot of boilerplate that I’m having a hard time getting rid of, and which is also having knock-on effects on other parts of the code — so it will likely be a while before I’m happy enough with the result.

[jhogberg]

I've merged a fix with a slightly different take, rejecting all positions that look funny rather than only those on map updates. Thanks again for reporting :-)