Opened 10 years ago

Closed 10 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
Keywords: Cc:
Game: AGI Fanmade

Description

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 10 years ago.
Beginning of conversion of gameid and extras fields to Common::String
nullmd5fix.patch (1000 bytes) - added by SF/rinco 10 years ago.
Fix for AGI savegames where game md5 string is null.

Download all attachments as: .zip

Change History (10)

Changed 10 years ago by bluegr

Attachment: detection.patch added

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

comment:1 Changed 10 years ago by bluegr

Meh, sf.net 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 Changed 10 years ago by SF/rinco

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 Changed 10 years ago by fingolfin

@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 :).

Changed 10 years ago by SF/rinco

Attachment: nullmd5fix.patch added

Fix for AGI savegames where game md5 string is null.

comment:4 Changed 10 years ago by sev-

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

comment:5 Changed 10 years ago by sev-

Priority: normalhigh

comment:6 Changed 10 years ago by lordhoto

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

comment:7 Changed 10 years ago by lordhoto

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 Changed 10 years ago by lordhoto

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