Opened 15 years ago

Closed 15 years ago

#4582 closed defect (fixed)

AGI: Crash when saving fallback-matched game

Reported by: SF/rinco Owned by: lordhoto
Priority: high Component: Engine: AGI
Version: Keywords:
Cc: Game: AGI Fanmade


If you're running a AGI game that has used fallback matching (that is, during loading you get a message like "Your game version has been detected using fallback matching as a variant of agi-fanmade (Unknown v3 Game).") then attempting to save the game will crash scummvm.

This is due to the unchecked use of the return from getGameMD5() in AgiEngine::saveGame() (engines/agi/saveload.cpp:101 in the SVN version as I write this):

const char *tmp = getGameMD5(); for (i = 0; i < 32; i++) out->writeByte(tmp[i]);

For a fallback-matched game, the md5 field, and thus the return value of getGameMD5(), is null and so the above code causes a segfault. I'm not sure what the intended behaviour for this case is, but I can't imagine a segfault is it ;)

Ticket imported from: #2849084. Ticket imported from: bugs/4582.

Attachments (2)

detection.patch (7.0 KB ) - added by bluegr 15 years ago.
Beginning of conversion of gameid and extras fields to Common::String
nullmd5fix.patch (1000 bytes ) - added by SF/rinco 15 years ago.
Fix for AGI savegames where game md5 string is null.

Download all attachments as: .zip

Change History (10)

by bluegr, 15 years ago

Attachment: detection.patch added

Beginning of conversion of gameid and extras fields to Common::String

comment:1 by bluegr, 15 years ago

Meh, ate my comment...

I was saying that both gameid and extras are const char * in ADGameDescription (engines/advancedDetector.h), so they're deleted once the fallback detector function exits. I had the exact same issue in the SCI fallback detector, and I used the quick'n'dirty fix: strdup for both gameid and extras (which leaks memory, but doesn't crash).

The proper way would probably be to make both fields Common::String - I've added a quick patch for this, but it crashes for me - I don't have the time to look into this more right now, but I'm uploading the patch in case anyone wants to investigate.

In the meantime, we could use strdup() to avoid the crash...

comment:2 by SF/rinco, 15 years ago

The issue here is slightly different - gameid and extras are filled in (albeit with empty strings), but the char* for md5 is set to NULL. According to comments in the ADFileDescription struct, that's legitimate so it seems like it's incumbent upon on the caller (AgiEngine::saveGame() in this case) to deal with that situation.

I've attached a patch that does this by just setting the savegame md5 string to all zeros in the case of a valid md5 string not being available. Similarly, the warning for mismatched md5 strings is only generated if one is available.

comment:3 by fingolfin, 15 years ago

@thebluegr: Making those fields Common::String is not possible, because then the structs wouldn't be POD types, and thus we couldn't declare tables anymore. No go.

Anyway, what you write seems to have no connection to the fact that the md5 string is 0 -- i.e., I don't see what you write has to do with the issue reported here :).

by SF/rinco, 15 years ago

Attachment: nullmd5fix.patch added

Fix for AGI savegames where game md5 string is null.

comment:4 by sev-, 15 years ago

This bug is nice to get fixed before the release. Raising priority for keeping the track.

comment:5 by sev-, 15 years ago

Priority: normalhigh

comment:6 by lordhoto, 15 years ago

Summary: Crash when saving fallback-matched AGI gameAGI: Crash when saving fallback-matched game

comment:7 by lordhoto, 15 years ago

I did use the patch attached by rinco as a base for my commit fixing this bug. Thus I'm closing it. Thanks for reporting and the patch.

As for thebluegr's problem: It seems the AGI fallback detector saves its gameid and extra in Common::String instances of the AgiMetaEngine, which isn't destroyed before running the game, thus the gameid and extra fields should be no problems here at all.

comment:8 by lordhoto, 15 years ago

Owner: set to lordhoto
Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.