Opened 9 years ago

Closed 9 years ago

#5650 closed defect (fixed)

QFG3: assertion failure when making thief sign to rope maker

Reported by: SF/vcappe Owned by: wjp
Priority: normal Component: Engine: SCI
Keywords: Cc:
Game: Quest for Glory 3

Description

the game crashes in asserting isValidRect() when making the thief sign to the rope maker. To be more precise it happens when the thief start to make his jump.

This bug apparently appeared in r52207 (git aa3cefa) which was supposed to fix bug #3039768 (in addition to r52207, i tested v1.2.0 v1.2.1 and 1.3.0git4514-g489d4d6; all of them have the bug).

v1.1.1 and r52204 (git 2bd5077) do not crash (since they have bug #3039768 instead).
______
platform: Arch Linux
compiler: gcc 4.5.2
i tried both -O2 and -O0
______
stderr says:
WARNING: kCan(t)BeHere - invalid rect -32430, 32767 -> -32415, -32767!
WARNING: kCan(t)BeHere - invalid rect -32516, 32767 -> -32501, -32767!
WARNING: kCan(t)BeHere - invalid rect -32602, 32767 -> -32587, -32767!
WARNING: kCan(t)BeHere - invalid rect -32429, 32766 -> -32414, -32768!
scummvm: /usr/local/src/devel/scummvm/common/rect.h:103: Common::Rect::Rect(int16, int16, int16, int16): Assertion `isValidRect()' failed.

[3]+ Aborted (core dumped) ./build_debug/scummvm --savepath=saves/ -x 003 -F qfg3

______
compiling assertions out (-DNDEBUG=1) doesn't help either: it sends the game in a seemingly infinite loop that only SIGKILL can break, while spewing lots of 'kCan(t)BeHere - invalid rect' warnings at stderr.

______
To reproduce: load attached savegame; click on the hero; select "give the thief sign"; then click until the rope maker finish his speak.

Ticket imported from: #3289799. Ticket imported from: bugs/5650.

Attachments (2)

qfg3.001 (37.5 KB ) - added by SF/vcappe 9 years ago.
save at the rope maker
bt_full.txt (9.3 KB ) - added by SF/vcappe 9 years ago.
output of gdb `backtrace full' command

Download all attachments as: .zip

Change History (14)

by SF/vcappe, 9 years ago

Attachment: qfg3.001 added

save at the rope maker

comment:1 by SF/vcappe, 9 years ago

> To be more precise it happens when the thief start to make his jump.

s/thief/rope maker/

comment:2 by bluegr, 9 years ago

I can't reproduce this with the latest daily version under Windows 7 64-bit. The scene works correctly for me. Which version of ScummVM are you using? Which OS?

comment:3 by bluegr, 9 years ago

Owner: set to bluegr

comment:4 by SF/vcappe, 9 years ago

OS: Arch Linux i686 (uptodate as of 2011-04-16)

i self-compiled a selection of version from git: v1.2.0, v1.2.1, r52207 (git v0.3.0-37833-gaa3cefa) and 1.3.0git4514-g489d4d6 (more or less: current on 2011-04-19).

the compiler is gcc 4.5.2 as shipped by Arch Linux (4.5.2-6).

i tried both with and without --enable-release; and with -O0 (optimization disabled) and -O2 (default optimization level) (passing them in the CXXFLAGS environment variable and not giving --enable-release to ./configure so they are not overriden).

could this be architecture and/or compiler dependent?

note how the coordinates given in the warnings are close/equals to (-)2^15-1 (iow the min/maximum value in a signed 16bit integer).

comment:5 by SF/vcappe, 9 years ago

backtrace from gdb (more complete version attached):
______

[New Thread 18212]
[New Thread 18220]
[New Thread 18213]
Core was generated by `./build_debug/scummvm --savepath=saves/ -x 003 -F qfg3'.
Program terminated with signal 6, Aborted.
#0 0xb76fe424 in __kernel_vsyscall ()
#0 0xb76fe424 in __kernel_vsyscall ()
#1 0xb70fadc1 in raise () from /lib/libc.so.6
#2 0xb70fc64e in abort () from /lib/libc.so.6
#3 0xb70f37f8 in __assert_fail () from /lib/libc.so.6
#4 0x0807caf3 in Common::Rect::Rect (this=0xbfbcb4e6, x1=-32429, y1=32766, x2=-32414, y2=-32768) at /usr/local/src/devel/scummvm/common/rect.h:103
#5 0x080aefc0 in Sci::GfxCoordAdjuster16::onControl (this=0x89a92a0, rect={top = 32766, left = -32429, bottom = -32768, right = -32414}) at /usr/local/src/devel/scummvm/engines/sci/graphics/coordadjuster.cpp:62
#6 0x080ac9ad in Sci::GfxCompare::kernelOnControl (this=0x89a2840, screenMask=4 '\004', rect=@0xbfbcb51e) at /usr/local/src/devel/scummvm/engines/sci/graphics/compare.cpp:101
#7 0x08079b10 in Sci::kOnControl (s=0x8983498, argc=5, argv=0x89966f8) at /usr/local/src/devel/scummvm/engines/sci/engine/kgraphics.cpp:501
#8 0x080a19b9 in Sci::callKernelFunc (s=0x8983498, kernelCallNr=78, argc=5) at /usr/local/src/devel/scummvm/engines/sci/engine/vm.cpp:392
#9 0x080a3441 in Sci::run_vm (s=0x8983498) at /usr/local/src/devel/scummvm/engines/sci/engine/vm.cpp:869
#10 0x08098fce in Sci::invokeSelector (s=0x8983498, object={segment = 39, offset = 8567}, selectorId=57, k_argc=2, k_argp=0x89966a0, argc=0, argv=0x0) at /usr/local/src/devel/scummvm/engines/sci/engine/selector.cpp:250
#11 0x080a63e2 in Sci::GfxAnimate::invoke (this=0x89a6420, list=0x8995e70, argc=2, argv=0x89966a0) at /usr/local/src/devel/scummvm/engines/sci/graphics/animate.cpp:97
#12 0x080a93bf in Sci::GfxAnimate::kernelAnimate (this=0x89a6420, listReference={segment = 27, offset = 0}, cycle=true, argc=2, argv=0x89966a0) at /usr/local/src/devel/scummvm/engines/sci/graphics/animate.cpp:617
#13 0x0807c492 in Sci::kAnimate (s=0x8983498, argc=2, argv=0x89966a0) at /usr/local/src/devel/scummvm/engines/sci/engine/kgraphics.cpp:1097
#14 0x080a19b9 in Sci::callKernelFunc (s=0x8983498, kernelCallNr=11, argc=2) at /usr/local/src/devel/scummvm/engines/sci/engine/vm.cpp:392
#15 0x080a3441 in Sci::run_vm (s=0x8983498) at /usr/local/src/devel/scummvm/engines/sci/engine/vm.cpp:869
#16 0x0806df1a in Sci::SciEngine::runGame (this=0x890aa48) at /usr/local/src/devel/scummvm/engines/sci/sci.cpp:691
#17 0x0806cccc in Sci::SciEngine::run (this=0x890aa48) at /usr/local/src/devel/scummvm/engines/sci/sci.cpp:351
#18 0x0804eef4 in runGame (plugin=0x8812388, system=@0x8807008, edebuglevels=@0xbfbcc71c) at /usr/local/src/devel/scummvm/base/main.cpp:208
#19 0x0804fb46 in scummvm_main (argc=6, argv=0xbfbccba4) at /usr/local/src/devel/scummvm/base/main.cpp:421
#20 0x0804df3b in main (argc=6, argv=0xbfbccba4) at /usr/local/src/devel/scummvm/backends/platform/sdl/posix/posix-main.cpp:48

by SF/vcappe, 9 years ago

Attachment: bt_full.txt added

output of gdb `backtrace full' command

comment:6 by wjp, 9 years ago

I can't reproduce this either (64 bit linux)

comment:7 by SF/vcappe, 9 years ago

i think i found the problem.

in kSetJump() we do:
vx = (int16)((float)(dx * sqrt(gy / (2.0 * tmp))));

but tmp can be negative here (and gy is always >=0), so we can end up passing a negative number to sqrt() which is not a good idea. This is what happen in this case and what ultimately cause the crash.

Instead, it should be:
vx = (int16)((float)(dx * sqrt(gy / ABS(2.0 * tmp))));

comment:8 by wjp, 9 years ago

Thank you for the analysis. However, the solution you suggest doesn't match the comments above the function.

Could you test if replacing the test 'if (tmp != 0)' by 'if (tmp != && dx != 0)' works? (Right above the sqrt line.)

comment:9 by wjp, 9 years ago

'if (tmp != 0 && dx != 0)', sorry

comment:10 by SF/vcappe, 9 years ago

yes, it does work.

comment:11 by wjp, 9 years ago

Owner: changed from bluegr to wjp
Resolution: fixed
Status: newclosed

comment:12 by wjp, 9 years ago

Thanks. Committed as dda6df6

Note: See TracTickets for help on using tickets.