Opened 20 years ago

Closed 20 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
Version: 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 20 years ago.
Use Ring on Porthole to crash

Download all attachments as: .zip

Change History (46)

comment:1 by fingolfin, 20 years ago

Status: newpending

comment:2 by fingolfin, 20 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, 20 years ago

Works perfectly fine for me with latest CVS...

comment:4 by SF/noscript, 20 years ago

Status: pendingnew

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

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

comment:14 by fingolfin, 20 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, 20 years ago

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

comment:16 by fingolfin, 20 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, 20 years ago

Owner: set to Kirben

comment:18 by SF/noscript, 20 years ago

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

comment:19 by Kirben, 20 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, 20 years ago

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

by Kirben, 20 years ago

Attachment: comi.s03 added

Use Ring on Porthole to crash

comment:21 by Kirben, 20 years ago

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

comment:22 by fingolfin, 20 years ago

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

comment:23 by fingolfin, 20 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, 20 years ago

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

comment:25 by Kirben, 20 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, 20 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, 20 years ago

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

comment:28 by fingolfin, 20 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, 20 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, 20 years ago

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

comment:31 by Kirben, 20 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, 20 years ago

No, the subtitles are off

comment:33 by Kirben, 20 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, 20 years ago

Owner: changed from Kirben to fingolfin

comment:35 by Kirben, 20 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, 20 years ago

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

comment:37 by Kirben, 20 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, 20 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, 20 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, 20 years ago

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

comment:41 by SF/vladr, 20 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, 20 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, 20 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, 20 years ago

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

comment:45 by Kirben, 20 years ago

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