Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#5704 closed defect (fixed)

Tinsel: DW 1 & 2 Segfault on start

Reported by: SF/ezekiel000 Owned by: fingolfin
Priority: normal Component: Engine: Tinsel
Keywords: Cc:
Game: Discworld

Description

Both Discworld 1 & 2 crash ScummVM with this on the terminal:
User picked target 'dw' (gameid 'tinsel')...
Looking for a plugin supporting this gameid... Tinsel
Starting 'Tinsel engine game'
Connected to Alsa sequencer client [14:0]
ALSA client initialised [128:0]
Segmentation fault

And Discworld 2:
User picked target 'dw2-gb' (gameid 'tinsel')...
Looking for a plugin supporting this gameid... Tinsel
Starting 'Tinsel engine game'
Segmentation fault

Running a my own compiled ScummVM built 17/05 on Ubuntu 11.04 amd64:
ScummVM 1.4.0git (May 17 2011 22:58:32)
Features compiled in: Vorbis ALSA SEQ TiMidity RGB zLib Theora

With these compile options:
./configure --enable-myst --enable-riven --enable-sword25 --disable-he --disable-agi --disable-agos --disable-agos2 --disable-cine --disable-cruise --disable-draci --disable-drascula --disable-gob --disable-groovie --disable-hugo --disable-kyra --disable-lure --disable-parallaction --disable-queen --disable-saga --disable-ihnm --disable-sci --disable-sky --disable-teenagent --disable-toon --disable-touche --disable-tucker --disable-debug --disable-mt32emu --disable-mad --disable-flac --disable-fluidsynth --disable-readline --enable-release

Ticket imported from: #3303799. Ticket imported from: bugs/5704.

Change History (8)

comment:1 by dreammaster, 8 years ago

Owner: set to fingolfin

comment:2 by dreammaster, 8 years ago

Hi Fingolfin,

I think the problem was caused by your commit 'Remove unnecessary casts'. This has removed pointer to object operations which are necessary. In background.cpp:165, it's meant to return a pointer to the pDispList field, rather than the pDispList value itself.

I'm not sure if there's some cool way with git to undo that single commit, so I'll leave it up to you to decide how to proceed.

comment:3 by digitall, 8 years ago

Can't replicate this crash with the latest Git master.

Could you:
a) try to compile with just "./configure && make clean && make" and see if the crash still occurs.
b) confirm the git revision you are using i.e. ./scummvm --version from a build without --enable-release.

If the crash still occurs even with the options in a), then please try updating to the latest Git master and repeating a) and b).

P.S. If you are trying to disable all engines but a small selection, "./configure --disable-all-engines --enable-myst --enable-riven --enable-sword25" would make for a far more readable command...

comment:4 by SF/ezekiel000, 8 years ago

I updated to the latest git master and did just:
./configure && make clean && make

Version:
ezekiel@alice:~/scummvm-scummvm-f830d68$ ./scummvm -v
ScummVM 1.4.0git (May 18 2011 12:31:02)
Features compiled in: Vorbis ALSA SEQ TiMidity RGB zLib Theora

And still got segfaults on both discworld games.

P.S. Thanks for pointing out the disable all engines flag, it makes my command a bit shorter:
./configure --disable-all-engines --enable-made --enable-mohawk --enable-myst --enable-riven --enable-scumm --enable-scumm-7-8 --enable-sword1 --enable-sword2 --enable-sword25 --enable-tinsel --disable-debug --disable-mt32emu --disable-mad --disable-flac --disable-fluidsynth --disable-readline --enable-release

comment:5 by fingolfin, 8 years ago

dreammaster, you are right about deducing the commit which is the cause of this crash, and I reverted it. However, the code it touches is most definitely *not* correct: It casts an OBJECT** to OBJECT*. This is *very* fishy, and should be analyzed on its own.

comment:6 by fingolfin, 8 years ago

Resolution: fixed
Status: newclosed

comment:7 by fingolfin, 8 years ago

Ah, I see: The PLAYFIELD struct has as first entry an OBJECT pointer, just like the OBJECT itself. So in fact, the dirty trick used by tinsel here is to pretend the PLAYFIELD is an OBJECT, using it as the imaginary HEAD of the linked list of objects PLAYFIELD::pDispList points to.

I'll document this more explicitly in the code, and we probably still should rewrite the code to avoid that, as it could cause problem with aliasing analysis in compilers.

comment:8 by dreammaster, 8 years ago

Thanks for looking into further. That makes sense, and explains why the cast was being done, which definitely looked weird.

Note: See TracTickets for help on using tickets.