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 |
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)
Change History (10)
by , 15 years ago
| Attachment: | detection.patch added |
|---|
comment:1 by , 15 years ago
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 by , 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 , 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 , 15 years ago
| Attachment: | nullmd5fix.patch added |
|---|
Fix for AGI savegames where game md5 string is null.
comment:4 by , 15 years ago
This bug is nice to get fixed before the release. Raising priority for keeping the track.
comment:5 by , 15 years ago
| Priority: | normal → high |
|---|
comment:6 by , 15 years ago
| Summary: | Crash when saving fallback-matched AGI game → AGI: Crash when saving fallback-matched game |
|---|
comment:7 by , 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 , 15 years ago
| Owner: | set to |
|---|---|
| Resolution: | → fixed |
| Status: | new → closed |
Beginning of conversion of gameid and extras fields to Common::String