Opened 22 years ago

Closed 22 years ago

Last modified 6 years ago

#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)

guimaps.diff (273 bytes ) - added by (none) 22 years ago.

Download all attachments as: .zip

Change History (15)

by (none), 22 years ago

Attachment: guimaps.diff added

comment:1 by SF/painelf, 22 years ago

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.

comment:2 by SF/ender, 22 years ago

Summary: Fix for MI2 saveload dialogMI2 saveload dialog [msvc debug]

comment:3 by SF/ender, 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 fingolfin, 22 years ago

Could somebody tell me what the actual error is? The bug report somehow doesn't mention it...

comment:5 by SF/painelf, 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 SF/painelf, 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 wjp, 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 SF/cccp_99, 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 SF/cccp_99, 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 SF/cccp_99, 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 Kirben, 22 years ago

Owner: set to fingolfin

comment:12 by SF/ender, 22 years ago

Fixed with CCCPs latest patch.

comment:13 by SF/ender, 22 years ago

Resolution: fixed
Status: newclosed

comment:14 by digitall, 6 years ago

Component: Engine: SCUMM
Game: Monkey Island 2
Note: See TracTickets for help on using tickets.