Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#4581 closed defect (fixed)

ScummVM crashes with Future wars

Reported by: SF/yoannm Owned by: SF/buddha_
Priority: normal Component: Engine: Cine
Keywords: Cc:
Game: Future Wars

Description

Hi,
I am using ScummVM 1.0.0RC1 (but tried also with 0.13.1) on MAC OS X 10.5. The game version is DOS and language English...
The bug occurs always at the same moment and it crashes ScummVM, making the game un-completable...
Right after playing a, kind of, shoot-em-up and killing all the bad guys (happening in the very past of the game-not middle age, not future, but very past- - maybe after 2-3 hours of game), my friend is in critical condition and sent back to the future to be saved... Then our hero goes alone to the next 'screen' and at this moment, the VM crashes...
I could sent the save file if needed (for testing).

Thanks,
Yoann

Ticket imported from: #2848940. Ticket imported from: bugs/4581.

Attachments (2)

fw.1 (1.6 KB) - added by SF/yoannm 10 years ago.
Saved future wars before crash
fw-fix.diff (614 bytes) - added by eriktorbjorn 10 years ago.
Patch against 1.0 branch

Download all attachments as: .zip

Change History (12)

comment:1 Changed 10 years ago by SF/buddha_

Yes, Yoann, please do attach the save file to this bug report.

Changed 10 years ago by SF/yoannm

Attachment: fw.1 added

Saved future wars before crash

comment:2 Changed 10 years ago by SF/yoannm

After loading the file, you need to examine the girl few times until you can get the pendant to send her back to the future. But you probably already know that.

Thanks again,
Yoann

comment:3 Changed 10 years ago by eriktorbjorn

It doesn't crash for me, though I did get some warnings from Valgrind that I don't have the time (and quite possibly not the knowledge) to look into at the moment. From a cursory glance, it looks like it's calling getZoneFromPositionRaw(collisionPage, ...) with y > 200, which seems like a bug since collisionPage is a 320x200 buffer. The rest of the errors may simply be a consequence of that:

==27808== Invalid read of size 1
==27808== at 0x826811C: Cine::getZoneFromPositionRaw(unsigned char*, short, short, short) (script_fw.cpp:1847)
==27808== by 0x82689A6: Cine::checkCollision(short, short, short, short, short) (script_fw.cpp:1866)
==27808== by 0x8269454: Cine::FWScript::o1_checkCollision() (script_fw.cpp:885)
==27808== by 0x82684A8: Cine::FWScript::execute() (script_fw.cpp:678)
==27808== by 0x8268512: Cine::executeGlobalScripts() (script_fw.cpp:1919)
==27808== by 0x827FF41: Cine::CineEngine::mainLoop(int) (main_loop.cpp:352)
==27808== by 0x8278E81: Cine::CineEngine::run() (cine.cpp:92)
==27808== by 0x8054828: runGame(PluginSubclass<MetaEngine> const*, OSystem&, Common::String const&) (main.cpp:212)
==27808== by 0x8055139: scummvm_main (main.cpp:377)
==27808== by 0x8051B69: main (main.cpp:108)
==27808== Address 0x647b3f9 is 369 bytes inside a block of size 4,120 free'd
==27808== at 0x4024E3A: free (vg_replace_malloc.c:323)
==27808== by 0x45222F7: closedir (in /lib/i686/cmov/libc-2.9.so)
==27808== by 0x8793BD7: POSIXFilesystemNode::getChildren(Common::Array<AbstractFSNode*>&, Common::FSNode::ListMode, bool) const (posix-fs.cpp:206)
==27808== by 0x87A2553: Common::FSNode::getChildren(Common::FSList&, Common::FSNode::ListMode, bool) const (fs.cpp:80)
==27808== by 0x87A261C: Common::FSDirectory::cacheDirectoryRecursive(Common::FSNode, int, Common::String const&) const (fs.cpp:271)
==27808== by 0x87A2923: Common::FSDirectory::ensureCached() const (fs.cpp:306)
==27808== by 0x87A2BE2: Common::FSDirectory::lookupCache(Common::HashMap<Common::String, Common::FSNode, Common::IgnoreCase_Hash, Common::IgnoreCase_EqualTo>&, Common::String const&) const (fs.cpp:203)
==27808== by 0x87A2C7C: Common::FSDirectory::createReadStreamForMember(Common::String const&) const (fs.cpp:241)
==27808== by 0x87995B5: Common::SearchSet::createReadStreamForMember(Common::String const&) const (archive.cpp:217)
==27808== by 0x87A1C1D: Common::File::open(Common::String const&, Common::Archive&) (file.cpp:63)
==27808== by 0x87A1D9F: Common::File::open(Common::String const&) (file.cpp:54)
==27808== by 0x8270594: Cine::loadTextData(char const*) (texte.cpp:51)
==27808==
==27808== Conditional jump or move depends on uninitialised value(s)
==27808== at 0x82689B0: Cine::checkCollision(short, short, short, short, short) (script_fw.cpp:1868)
==27808== by 0x8269454: Cine::FWScript::o1_checkCollision() (script_fw.cpp:885)
==27808== by 0x82684A8: Cine::FWScript::execute() (script_fw.cpp:678)
==27808== by 0x8268512: Cine::executeGlobalScripts() (script_fw.cpp:1919)
==27808== by 0x827FF41: Cine::CineEngine::mainLoop(int) (main_loop.cpp:352)
==27808== by 0x8278E81: Cine::CineEngine::run() (cine.cpp:92)
==27808== by 0x8054828: runGame(PluginSubclass<MetaEngine> const*, OSystem&, Common::String const&) (main.cpp:212)
==27808== by 0x8055139: scummvm_main (main.cpp:377)
==27808== by 0x8051B69: main (main.cpp:108)
==27808==
==27808== Conditional jump or move depends on uninitialised value(s)
==27808== at 0x82689B7: Cine::checkCollision(short, short, short, short, short) (script_fw.cpp:1868)
==27808== by 0x8269454: Cine::FWScript::o1_checkCollision() (script_fw.cpp:885)
==27808== by 0x82684A8: Cine::FWScript::execute() (script_fw.cpp:678)
==27808== by 0x8268512: Cine::executeGlobalScripts() (script_fw.cpp:1919)
==27808== by 0x827FF41: Cine::CineEngine::mainLoop(int) (main_loop.cpp:352)
==27808== by 0x8278E81: Cine::CineEngine::run() (cine.cpp:92)
==27808== by 0x8054828: runGame(PluginSubclass<MetaEngine> const*, OSystem&, Common::String const&) (main.cpp:212)
==27808== by 0x8055139: scummvm_main (main.cpp:377)
==27808== by 0x8051B69: main (main.cpp:108)
==27808==
==27808== Conditional jump or move depends on uninitialised value(s)
==27808== at 0x82675A6: Common::Array<unsigned short>::operator[](int) (array.h:164)
==27808== by 0x8268A5E: Cine::checkCollision(short, short, short, short, short) (script_fw.cpp:1877)
==27808== by 0x8269454: Cine::FWScript::o1_checkCollision() (script_fw.cpp:885)
==27808== by 0x82684A8: Cine::FWScript::execute() (script_fw.cpp:678)
==27808== by 0x8268512: Cine::executeGlobalScripts() (script_fw.cpp:1919)
==27808== by 0x827FF41: Cine::CineEngine::mainLoop(int) (main_loop.cpp:352)
==27808== by 0x8278E81: Cine::CineEngine::run() (cine.cpp:92)
==27808== by 0x8054828: runGame(PluginSubclass<MetaEngine> const*, OSystem&, Common::String const&) (main.cpp:212)
==27808== by 0x8055139: scummvm_main (main.cpp:377)
==27808== by 0x8051B69: main (main.cpp:108)
==27808==
==27808== Conditional jump or move depends on uninitialised value(s)
==27808== at 0x82675B3: Common::Array<unsigned short>::operator[](int) (array.h:164)
==27808== by 0x8268A5E: Cine::checkCollision(short, short, short, short, short) (script_fw.cpp:1877)
==27808== by 0x8269454: Cine::FWScript::o1_checkCollision() (script_fw.cpp:885)
==27808== by 0x82684A8: Cine::FWScript::execute() (script_fw.cpp:678)
==27808== by 0x8268512: Cine::executeGlobalScripts() (script_fw.cpp:1919)
==27808== by 0x827FF41: Cine::CineEngine::mainLoop(int) (main_loop.cpp:352)
==27808== by 0x8278E81: Cine::CineEngine::run() (cine.cpp:92)
==27808== by 0x8054828: runGame(PluginSubclass<MetaEngine> const*, OSystem&, Common::String const&) (main.cpp:212)
==27808== by 0x8055139: scummvm_main (main.cpp:377)
==27808== by 0x8051B69: main (main.cpp:108)
==27808==
==27808== Use of uninitialised value of size 4
==27808== at 0x8268A5F: Cine::checkCollision(short, short, short, short, short) (script_fw.cpp:1877)
==27808== by 0x8269454: Cine::FWScript::o1_checkCollision() (script_fw.cpp:885)
==27808== by 0x82684A8: Cine::FWScript::execute() (script_fw.cpp:678)
==27808== by 0x8268512: Cine::executeGlobalScripts() (script_fw.cpp:1919)
==27808== by 0x827FF41: Cine::CineEngine::mainLoop(int) (main_loop.cpp:352)
==27808== by 0x8278E81: Cine::CineEngine::run() (cine.cpp:92)
==27808== by 0x8054828: runGame(PluginSubclass<MetaEngine> const*, OSystem&, Common::String const&) (main.cpp:212)
==27808== by 0x8055139: scummvm_main (main.cpp:377)
==27808== by 0x8051B69: main (main.cpp:108)

Changed 10 years ago by eriktorbjorn

Attachment: fw-fix.diff added

Patch against 1.0 branch

comment:4 Changed 10 years ago by eriktorbjorn

I've attached a possible fix. I don't know if it works for you, and even if it does I'm not sure if it's the right one. (Though it makes sense to me.) All it does is to restrict x and y to valid coordinates in getZoneFromPositionRaw().

If it's the right fix, maybe getZoneFromPosition() should have the same kind of sanity checking, but it doesn't appear to be used. It looks like it's intended for a packed version of collisionPage.

comment:5 Changed 10 years ago by SF/yoannm

Should I build the VM from the SVN sources to test it?

comment:6 Changed 10 years ago by SF/yoannm

Hi,
Ok, I just built ScummVM from the SVN sources and it worked. The VM doesn't crash anymore at this moment.
As far as it concerns me, the fix worked perfectly.

Thanks a lot,
Yoann

comment:7 Changed 10 years ago by SF/buddha_

Owner: set to SF/buddha_
Resolution: fixed
Status: newclosed

comment:8 Changed 10 years ago by SF/buddha_

I committed a workaround for this bug to the trunk (r43920) and to the branch-1-0-0 (r43921) by returning zero in Future Wars when trying to read outside the 320x200 buffer in getZoneFromPositionRaw (Y positions 200-232 were actually being accessed).

So at least this bug is gone now. BUT!
It is possible that this workaround causes other bugs and thus:

Yoann or anyone else reading this, please, if you could play through Future Wars using a daily build of ScummVM (of the branch-1-0-0) which is of revision 43921 or later and report any problems you find (See http://www.scummvm.org/news/20090710b/ for more info about testing) that would be much appreciated. Thanks!

comment:9 Changed 10 years ago by SF/yoannm

Ok, I will restart the game using the last branch-1-0-0 revision (>= r43921) and see if it still works.

Thanks again for the fix.
Yoann

comment:10 Changed 10 years ago by SF/yoannm

Hi,

I just completed Future Wars (on the same configuration previously described) with the revision r43921.
No bugs found.

Cheers,
Yoann

Note: See TracTickets for help on using tickets.