Opened 17 years ago

Closed 17 years ago

Last modified 12 months ago

#338 closed defect (fixed)

MI2 saveload dialog [msvc debug]

Reported by: (none) Owned by: fingolfin
Priority: normal Component: Engine: SCUMM
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) 17 years ago.

Download all attachments as: .zip

Change History (15)

by (none), 17 years ago

Attachment: guimaps.diff added

comment:1 by SF/painelf, 17 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, 17 years ago

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

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

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

comment:5 by SF/painelf, 17 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, 17 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, 17 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, 17 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, 17 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, 17 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, 17 years ago

Owner: set to fingolfin

comment:12 by SF/ender, 17 years ago

Fixed with CCCPs latest patch.

comment:13 by SF/ender, 17 years ago

Resolution: fixed
Status: newclosed

comment:14 by digitall, 12 months ago

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