Opened 16 years ago

Closed 16 years ago

#1383 closed defect (fixed)

DIG/COMI: Invalid rect values causing crashes

Reported by: SF/noscript Owned by: fingolfin
Priority: normal Component: Engine: SCUMM
Keywords: Cc:
Game: Monkey Island 3

Description

Monkey Island 3 crashes if the language parameter is
set.

-q de crashes. The Sound loops and the pc hangs up.

In the forums I heard that its in the Italian Version, too.

Ticket imported from: #874501. Ticket imported from: bugs/1383.

Attachments (1)

comi.s03 (52.3 KB ) - added by Kirben 16 years ago.
Use Ring on Porthole to crash

Download all attachments as: .zip

Change History (46)

comment:1 by fingolfin, 16 years ago

Status: newpending

comment:2 by fingolfin, 16 years ago

To process your bug report appropriately, we need you to
provide the following additional information:

* ScummVM version (PLEASE test the latest CVS/Daily build)
* Bug details, including instructions on reproducing it
* Language of game (English, German, ...)
* Version of game (talkie, floppy, ...)
* Platform and Compiler (Win32, Linux, MacOS, ...)
* Attach a save game if possible
* If this bug only occurred recently, please note the last
version without the bug, and the first version including
the bug. That way we can fix it quicker by looking at the
changes made.

This should only take you a little time but will make it much easier for
us to process your bug report in a way that satisfies both you and us.

Thank you for your support!

comment:3 by fingolfin, 16 years ago

Works perfectly fine for me with latest CVS...

comment:4 by SF/noscript, 16 years ago

Status: pendingnew

comment:5 by SF/noscript, 16 years ago

ScummVM version: CVS build 1.11.03
for reproducing play the game a while, i Guybrush goes to
the "sumpf (?)" (where the scull is on the island). And speaks
with the woman and aksk her who she is it hangs up.

platform win32 (XP), 2,6Ghz P4

comment:6 by fingolfin, 16 years ago

First of November 2003 ? Uhm, that's from the stone age...

Either, use the last stable release (0.5.1). Or if for some reasons
that is not good enough for you, go for a current CVS build. But
before you report a bug in a CVS build, *always* first check if it's
still there in a current version (we explicitly ask for this on the bug
submission page, BTW).

Note: if you are using CVS builds, you do not just get the
improvements made in CVS, you also get the regressions... which
sometimes are fixed 1 hour, 1 day, or 1 week later.... so you are
taking a risk doing that...

comment:7 by SF/noscript, 16 years ago

Is the CVS same as the daily snapshot? I downloaded the
dayle snapshot but the same effect I uploaded a safegame.

Maybe it is because i run the game with the datafiles on my
harddisk and without CD?

comment:8 by fingolfin, 16 years ago

Daily builds are made each day (or night) from current CVS,
indeed.

I am by default using my (german) COMI from HD. Without any
problems...

comment:9 by SF/noscript, 16 years ago

scumm\scummvm.exe -q de -F -pgame\monkey3 comi

So I load it.

Ich glaub wir knnen auch deutsch sprechen. Hast du ICQ?

comment:10 by fingolfin, 16 years ago

No I am not using ICQ, and our bug tracker is held in english, so
I'd prefer to stay with that language, in the interest of the
community, which may benefit from the solution to your problem.

Anyway, a fact you didn't mention before is that you use "-F".
What happens if you leave that out?
Alsoo, do you have a scummvm.ini, by chance? If so, it would be
interesting to know if it has a 'comi' section, and if so, what stuff
is in that. Also, what settings are set in the 'scummvm' section of
it...

comment:11 by SF/noscript, 16 years ago

There are no settings in the scummvm.ini for comi. But a
savegame path for [scumm] is set. -f and -F have this
problem,too, it crashes.

I could make a SFV File to check if my files are corrupted.

comment:12 by fingolfin, 16 years ago

So, does it actually work if you leave "-qde" away?

I dunno what an "SFV" file is. But here are some MD5 sums of a
couple important files (the last contains the german translation;
everything else should be completely identical to the english
version):
MD5 (COMI.LA0) = fe60d6b5ff51b0553ac59963123b5777
MD5 (COMI.LA1) = 09ae3abb98c43df89119466ae3557c0f
MD5 (COMI.LA2) = 468f5ea61bb622284d57e615718b73cc
MD5 (RESOURCE/LANGUAGE.TAB) =
5d59594b24f3f1332e7d7e17455ed533

comment:13 by SF/noscript, 16 years ago

How can I compare them with the Files on Windows XP?
Which program do I need?

comment:14 by fingolfin, 16 years ago

I don't use Windows, but googling for "windows md5" gave a tons
of sites/toos, e.g.:
http://www.md5summer.org/
http://md5deep.sourceforge.net/
http://winmd5sum.solidblue.biz/
http://www.blisstonia.com/shareware/WinMD5/

comment:15 by SF/noscript, 16 years ago

MD5 are the same. I heard from a lucas arts sound driver.
Maybe this is the problem?

comment:16 by fingolfin, 16 years ago

Hu? What do you mean with "lucas arts sound driver" ?

If you were on Unix, I'd now ask you for a "stack trace", however I
have no clue how to get one on Windows.

Assigning temporarily to Kirben - he's Windows user, and maybe
can try to reproduce the problem, or has some other idea...

comment:17 by fingolfin, 16 years ago

Owner: set to Kirben

comment:18 by SF/noscript, 16 years ago

OK, there is no driver at all.
But the bug is still there ;(

comment:19 by Kirben, 16 years ago

I have not been able to reproduce any sound lock ups for
awhile, so they might have been fixed.
I managed to get another graphics related crash though, gdb
backtrace:
Program received signal SIGSEGV, Segmentation fault.
0x77c42f43 in msvcrt!memcpy () from C:\WINDOWS\system32
\msvcrt.dll
(gdb) bt
#0 0x77c42f43 in msvcrt!memcpy () from
C:\WINDOWS\system32\msvcrt.dll
#1 0x00433e15 in Scumm::ScummEngine::blit(unsigned
char*, unsigned char const*, int, int) (
this=0x13e94f0,
dst=0x299b874 "NuNu666A\232\232
AAAAA^^AAAAAAAAA^f\223\210\207
\207\207\207\207
\207p\237l\212T\177\177R\222\222Xf9\205SNSNSS", '='
<repeats 12 times>, "\224\205
\035AAAAAA^AAAAAAAAA^AA^A^", 'A' <repeats 24
times>, "^AAA^AAA^AAA^AAAAAAAA^AAAAA"...,
src=0x28cf3f4 "NuNu666A\232\232
AAAAA^^AAAAAAAAA^f\223\210\207
\207\207\207\207
\207p\237l\212T\177\177R\222\222Xf9\205SNSNSS", '='
<repeats 12 times>, "\224\205
\035AAAAAA^AAAAAAAAA^AA^A^", 'A' <repeats 24
times>, "^AAA^AAA^AAA^AAAAAAAA^AAAAA"..., w=-69,
h=30) at scumm/gfx.cpp:663
#2 0x00433ae7 in Scumm::ScummEngine::restoreBG
(Common::Rect, unsigned char) (this=0x13e94f0, rect=
{top = 182, left = 0, bottom = 212, right = -69},
backColor=0 '\0') at scumm/gfx.cpp:577
#3 0x00433be0 in
Scumm::CharsetRenderer::restoreCharsetBg()
(this=0x256b390) at scumm/gfx.cpp:611
#4 0x00426da2 in Scumm::ScummEngine::stopTalk()
(this=0x13e94f0) at scumm/actor.cpp:1201
#5 0x0045dcbd in Scumm::Sound::processSfxQueues()
(this=0x1450480) at scumm/sound.cpp:446
#6 0x0045cf3b in Scumm::Sound::processSoundQues()
(this=0x1450480) at scumm/sound.cpp:81
#7 0x0041b1fd in Scumm::ScummEngine::scummLoop(int)
(this=0x13e94f0, delta=5)
at scumm/scummvm.cpp:1561
#8 0x00419a99 in Scumm::ScummEngine::mainRun()
(this=0x13e94f0) at scumm/scummvm.cpp:1253
#9 0x004180cc in Scumm::ScummEngine::go()
(this=0x13e94f0) at scumm/scummvm.cpp:898
#10 0x004069ac in main (argc=3, argv=0x3f25c8) at
base/main.cpp:292

comment:20 by Kirben, 16 years ago

Summary: crashes if language parameter is setCOMIL Crashes if language parameter is set

by Kirben, 16 years ago

Attachment: comi.s03 added

Use Ring on Porthole to crash

comment:21 by Kirben, 16 years ago

A similar crash also occurs in Dig, see #877478, gdb outputs
show causes is the same.

comment:22 by fingolfin, 16 years ago

Summary: COMIL Crashes if language parameter is setCOMI: Crashes if language parameter is set

comment:23 by fingolfin, 16 years ago

Kirben, so that only occurs if you use -qde, but not otherwise?

In any case, the backtrace looks wrong. Can you recompile
ScummVM without optimizations and try again? In particular, it
says now:
#2 0x00433ae7 in Scumm::ScummEngine::restoreBG
(Common::Rect, unsigned char) (this=0x13e94f0, rect=
{top = 182, left = 0, bottom = 212, right = -69},
but restoreBG() will just abort if rect.left >= rect.right (which is
the case here), so I don't see how it could ever crash... unless the
backtrace is wrong :-)

comment:24 by Kirben, 16 years ago

The choosen language doesn't matter, I will try and get a
better back trace.

comment:25 by Kirben, 16 years ago

I tried compiling without the -O flag but gdb back trace is
exactly the same.
Isn't rect unsigned though ? so right would be more than left

comment:26 by fingolfin, 16 years ago

No, it's not unsigned (would've been a pretty bad designe choice,
that :-). Feel free to look at it yourself, in common/rect.h :-)

Anyway, noscript, what about you, does it crash for you without "-
qde", too, or only with "-qde" ?

comment:27 by Kirben, 16 years ago

Already answered in forums check
http://sourceforge.net/forum/forum.php?
thread_id=1002189&forum_id=115757

comment:28 by fingolfin, 16 years ago

I just see that you attached a savegame, Kirben. However, no
instructions as to how that savegame might be helpful... I loaded it
but didn't manage to crash with it, either.

comment:29 by Kirben, 16 years ago

The save game description is all you should need to do to get
ScummVM to crash, crashes right after cutscene of Guybrush
giving ring to Elaine for me.

comment:30 by SF/noscript, 16 years ago

Same for me!! Crashes and the voice stutters when guybrush
speaks.

comment:31 by Kirben, 16 years ago

Just noticed crash only seems to occur if subtitles are
enabled.
So bug #877611 could be related too.

comment:32 by SF/noscript, 16 years ago

No, the subtitles are off

comment:33 by Kirben, 16 years ago

I think you are getting another issue confused with this one.
An 'Assertion failed: num != -1, file
scumm/imuse_digi/dimuse_music.cpp, line 41' will occur
shortly after too, if subtitles are disabled.
Try starting ScummVM from command line to check.

comment:34 by Kirben, 16 years ago

Owner: changed from Kirben to fingolfin

comment:35 by Kirben, 16 years ago

The problem is caused by fingolfin's changes on 'Wed, 07 Jan
2004 19:24:03 -0800' of moving clipping to lines 114 - 120 of
common/rect.h.
The new clipping function includes less checks, not checking
if values are below 0.

comment:36 by Kirben, 16 years ago

Summary: COMI: Crashes if language parameter is setDIG/COMI: Invalid rect values causing crashes

comment:37 by Kirben, 16 years ago

Adding back checks for rect.left and rect.right values below
zero in restoreBG() of gfx.cpp has fixes the problems for now.
Not sure f that is right fix though.

comment:38 by Kirben, 16 years ago

Owner: changed from fingolfin to Kirben
Summary: DIG/COMI: Invalid rect values causing crashesCOMI: Crashes if language parameter is set

comment:39 by Kirben, 16 years ago

Owner: changed from Kirben to fingolfin
Summary: COMI: Crashes if language parameter is setDIG/COMI: Invalid rect values causing crashes

comment:40 by Kirben, 16 years ago

Ooops, didn't mean to revert assign/subject.

comment:41 by SF/vladr, 16 years ago

Fingolfin,

> In any case, the backtrace looks wrong.

No, the backtrace is absolutely right. What happens is that
restoreBG() is called with a rect with bot X coordinates < 0,
but that does not trigger "if (rect.left >= rect.right ..."
because, say, rect.left = -850 and rect.right = -845 (actual
numbers, it's sitting on the breakpoint as I write). Then clip
(), I guess, sets rect.left() to 0, and rect.right() (and the
width) remain negative.

So I'd say, fix clip() in rect.h because right now clip() wrongly
assumes lots of things, i.e. it should assume less and look
something like:

// these (existing code) work for overlapping
// rects only:
if (top < r.top) top = r.top;
if (left < r.left) left = r.left;
if (bottom > r.bottom) bottom = r.bottom;
if (right > r.right) right = r.right;

// extra checks for non-overlapping rects:
if (top > r.bottom) top = r.bottom;
if (left > r.right) left = r.right;
if (bottom < r.top) bottom = r.top;
if (right < r.left) right = r.left;

Cheers,
V.

comment:42 by fingolfin, 16 years ago

vladr, thanks, the clipping code is indeed incomplete. I'll look into
it.

Anyway, loading the savegame, using the ring on the window, then
skipping over the cutscene till the part II screen "only" causes
ScummVM to hang in an infinite loop - i.e. I never ever get to any
"crash". Both with "old" CVS, with Kirben's latest CVS mods, nor
with an improved Rect::clip(). It hangs in script 88 with an endless
loop (I haven't yet analyzed that, though)

comment:43 by fingolfin, 16 years ago

Fixed clipping code is in CVS.

The lockup I was experiencing turned out to be a serious bug in
our array code (essentially, it caused savegames to not be endian
safe anymore). I fixed this, and have a workaround which still
allows old savegames to work properly. Alas I can't check this in
before the SF.net CVS server upgrade is complete.

comment:44 by fingolfin, 16 years ago

Can you please verify that the issue is gone with latest CVS?

comment:45 by Kirben, 16 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.