#338 closed defect (fixed)
MI2 saveload dialog [msvc debug]
| Reported by: | (none) | Owned by: | fingolfin |
|---|---|---|---|
| Priority: | normal | Component: | Engine: SCUMM |
| Version: | Keywords: | ||
| Cc: | Game: | Monkey Island 2 |
Description
Changes a value in the string_map_table_v5 that seems to be corrupt. This error must have been around for a while. I narrowed the time of change down to between 18:00 and 21:00 on May 14th (by trial and error). I have not tested this with anything but MI2. I have no clue as to why this bug was introduced initially. /Painelf
Ticket imported from: #580350. Ticket imported from: bugs/338.
Attachments (1)
Change History (15)
by , 22 years ago
| Attachment: | guimaps.diff added |
|---|
comment:1 by , 22 years ago
comment:2 by , 22 years ago
| Summary: | Fix for MI2 saveload dialog → MI2 saveload dialog [msvc debug] |
|---|
comment:3 by , 22 years ago
Well, actually, this patch isn't the real fix.
The problem was introduced by a change by Fingolfin in guimaps:
fixed #555567 (savegame menu head line) by moving & enlarging the headline item in the save dialog, and using string 28 for the title
Will look into it. Thanks tho :)
comment:4 by , 22 years ago
Could somebody tell me what the actual error is? The bug report somehow doesn't mention it...
comment:5 by , 22 years ago
Sorry. In MI2 the game exits with an "Message stack overflow" when opening saveload dialog. It tries to prints the first string in the dialog ("How may I serve you?") but the string pointer seems corrupt - it points to a non-zero terminated string, which results in a string longer than 500 chars, hence stack overflow.
comment:6 by , 22 years ago
The string adress is okay, perhaps this is an issue of termination?
The string terminates with 29 20 FF 04 44 00, '29' and '20' being the ') ' in 'Monkey 2(v1.0 11/5/92) '.
1) There are no additional zeroes following this string. Immideately following is (on my machine) uninitialized memory (0xcccccc).
2) Stepping through addMessageToStack() reveals that it does indeed read one byte past the last 00, and checks if that byte is zero.
comment:7 by , 22 years ago
Valgrind gives a couple of pages of errors at this point. The first two are below. The rest are probably caused by these. If you need them anyway, let me know. (Line numbers are from CVS of 9 sept. 2002, btw)
==2436== Conditional jump or move depends on uninitialised value(s) ==2436== at 0x8078914: Scumm::addMessageToStack(unsigned char *) (scumm/string.cpp:639) ==2436== by 0x8078555: Scumm::drawString(int) (scumm/string.cpp:507) ==2436== by 0x80543D0: Gui::drawString(char const *, int, int, int, unsigned char, bool) (gui/gui.cpp:381) ==2436== by 0x8054597: Gui::drawWidget(GuiWidget const *) (gui/gui.cpp:451) ==2436== ==2436== Conditional jump or move depends on uninitialised value(s) ==2436== at 0x80788D7: Scumm::addMessageToStack(unsigned char *) (scumm/string.cpp:634) ==2436== by 0x8078555: Scumm::drawString(int) (scumm/string.cpp:507) ==2436== by 0x80543D0: Gui::drawString(char const *, int, int, int, unsigned char, bool) (gui/gui.cpp:381) ==2436== by 0x8054597: Gui::drawWidget(GuiWidget const *) (gui/gui.cpp:451)
comment:8 by , 22 years ago
The source of this problem is the game data file itself. The string ends with : FF 04 44 00. It should probably be: FF 04 01 44 00 (other strings have it) FF 04 uses next two bytes for a variable, and it ends up skipping the null terminator in this case (string.cpp, line 658), so the string isn't terminated properly. Probably another problem is on line 639 of the same file where it also skips two bytes.
Endy's suggested fixme is in patch # 607713
CCCP
comment:9 by , 22 years ago
After looking at it again this morning, I see that my original conclusion was wrong. The Fixme should be taken away and the problem correctly fixed.
Here's a full description:
First, end of the data that's currently going in the dialog box: "91) + some garbage" Hex View: 39 31 29 20 FF 04 44 00
The actual string doesn't end with that 00, because 00 is a part of the variable number (44 00 becomes 0x0044). Looking at the resource file, you can see there's a total of 3 special opcodes(separated for easier viewing): 39 31 29 20 [FF 04 44 00] 2C [FF 04 45 00] 20 [FF 07 1A 00] 00
The last 00 is actually the null terminator for the full string. The string should become something like this after addMessagetoStack(): "91) 0,0 Regular Mode"
Two 0's are taken from variables 0x44 and 0x45, and 'Regular Mode' (or 'Easy Mode') comes from string 0x1A.
The problem results from an incomplete string copy from the resource on line 423 of gui.cpp. As we all know, strcpy() only goes to the first null, but in this case it's a part of the special opcode that should be skipped.
The proper solution would be to either copy with memcpy (if the length of the string is known), or to make a new string copy function for resources that would take care of the special characters after FF.
CCCP
comment:10 by , 22 years ago
Another thing: NewGui has a similar problem too (though it doesn't crash). It's also copying the string for the dialog box when it creates the StaticTextWidget on line 289 of dialog.cpp. That calls String::String() to convert str. It uses memcpy(), but still strlen() to determine the length of the string, which is again incorrect.
Both gui and newgui should probably be fixed together to use the same function to determine the length of the string and copy it.
CCCP
comment:11 by , 22 years ago
| Owner: | set to |
|---|
comment:13 by , 22 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
comment:14 by , 6 years ago
| Component: | → Engine: SCUMM |
|---|---|
| Game: | → Monkey Island 2 |
Btw, I realize this is a trivial patch that should probably not have been submitted this way. It was merely an excersize in cvs, since I've never really used it before.