Opened 13 years ago

Closed 13 years ago

#5764 closed defect (fixed)

IHNM: Crash to desktop in Ellen's Adventure

Reported by: SF/m4xpower Owned by: digitall
Priority: normal Component: Engine: SAGA
Version: Keywords:
Cc: Game: I Have No Mouth

Description

SCUMMVM will crash to desktop 100% of the time for me when I enter a room. In the provided savegame, enter the room on the far right. If it crashes to desktop, you're experiencing what I experience.

Ticket imported from: #3323722. Ticket imported from: bugs/5764.

Attachments (1)

ihnm.s21​ (18.0 KB ) - added by SF/m4xpower 13 years ago.
IHNM Savegame

Download all attachments as: .zip

Change History (17)

by SF/m4xpower, 13 years ago

Attachment: ihnm.s21​ added

IHNM Savegame

comment:1 by wjp, 13 years ago

Summary: Crash to desktop in Ellen's Adventure → IHNM: Crash to desktop in Ellen's Adventure

comment:2 by eriktorbjorn, 13 years ago

It crashes for me too with that savegame. Valgrind warns about invalid reads like this:

==1644== Invalid read of size 4 ==1644== at 0x892EAFA: Common::Array<Saga::ActorFrameSequence>::empty() const (array.h:199) ==1644== by 0x892BBAB: Saga::Actor::getActorFrameRange(unsigned short, int) (actor.cpp:703) ==1644== by 0x8932441: Saga::Actor::handleActions(int, bool) (actor_walk.cpp:364) ==1644== by 0x89321E1: Saga::Actor::updateActorsScene(int) (actor_walk.cpp:310) ==1644== by 0x89162AB: Saga::Scene::loadScene(Saga::LoadSceneParams&) (scene.cpp:846) ==1644== by 0x89154F9: Saga::Scene::changeScene(short, int, Saga::SceneTransitionType, int) (scene.cpp:505) ==1644== by 0x892B612: Saga::Actor::takeExit(unsigned short, Saga::HitZone const*) (actor.cpp:539) ==1644== by 0x892B77C: Saga::Actor::stepZoneAction(Saga::ActorData*, Saga::HitZone const*, bool, bool) (actor.cpp:566) ==1644== by 0x893366D: Saga::Actor::handleActions(int, bool) (actor_walk.cpp:695) ==1644== by 0x89337BE: Saga::Actor::direct(int) (actor_walk.cpp:727) ==1644== by 0x8911C5C: Saga::SagaEngine::run() (saga.cpp:377) ==1644== by 0x8050175: runGame(PluginSubclass<MetaEngine> const*, OSystem&, Common::String const&) (main.cpp:207) ==1644== Address 0xa317e80 is 1,056 bytes inside a block of size 1,964 free'd ==1644== at 0x4023A8C: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) ==1644== by 0x892ED67: Common::Array<Saga::ActorData>::reserve(unsigned int) (array.h:244) ==1644== by 0x892E43A: Common::Array<Saga::ActorData>::resize(unsigned int) (array.h:249) ==1644== by 0x892AD02: Saga::Actor::loadActorList(int, int, int, int, int) (actor.cpp:388) ==1644== by 0x8927642: Saga::Resource_RES::loadGlobalResources(int, int) (resource_res.cpp:99) ==1644== by 0x89159F0: Saga::Scene::loadScene(Saga::LoadSceneParams&) (scene.cpp:614) ==1644== by 0x89154F9: Saga::Scene::changeScene(short, int, Saga::SceneTransitionType, int) (scene.cpp:505) ==1644== by 0x8913DD6: Saga::SagaEngine::load(char const*) (saveload.cpp:321) ==1644== by 0x8911A4F: Saga::SagaEngine::run() (saga.cpp:339) ==1644== by 0x8050175: runGame(PluginSubclass<MetaEngine> const*, OSystem&, Common::String const&) (main.cpp:207) ==1644== by 0x8050D8C: scummvm_main (main.cpp:420) ==1644== by 0x804F05A: main (posix-main.cpp:45) ==1644==

comment:3 by eriktorbjorn, 13 years ago

Regression testing points to this as being the commit that introduced the bug:

commit fa7e8a8eb382c37d35d9166dcd87646811e4a377 Author: Andrew Kurushin <h00ligan@users.sourceforge.net> Date: Sun Oct 24 17:42:45 2010 +0000

SAGA: replace Actor::_actors and _objs malloc base arrays with Common::Array

svn-id: r53766

(Yesterday's discussion on #scummvm pointed to it as the probable cause, but I don't know if anyone actually verified. And anyway, it should be documented here in any case.)

The bug is present both in the development version and in 1.3.0, and I think we really should try to get it fixed for the planned 1.3.1.

comment:4 by fuzzie, 13 years ago

Again repeating conversation from IRC: I think simply adding an _actors.clear() before the _actors.resize() on line 388 should resolve this, since it will force the re-construction of the ActorData objects and hence reset (among other things) the _frames field. Someone with IHNM should test this. The other allocation changes in the code history don't look like they should cause problems, but I only glanced through.

comment:5 by digitall, 13 years ago

fuzzie: I have IHNM so can look at this problem and fix.

comment:6 by digitall, 13 years ago

With savegame and latest git master, I get the following: scummvm: ./common/array.h:166: T& Common::Array<T>::operator[](int) [with T = Saga::ActorFrameSequence]: Assertion `idx >= 0 && (uint)idx < _size' failed.

comment:7 by digitall, 13 years ago

Hmm, Git bisection indicates the following commit as being the regression point: 0970cdf5e55a7dc52f8f2a3fd76421bdd33d6164 SAGA: reduce memory usage

svn-id: r53782

But I suspect this is just the one where the memory issue causes a segfault. I could repeat with Valgrind, but I suspect eriktorbjorn is correct.

comment:8 by wjp, 13 years ago

Do you mean you tested with latest git master or with latest git master + fuzzie's suggested fix on top of it?

comment:9 by digitall, 13 years ago

wjpalenstijn: Sorry, should have been clear: Latest Git Master, no patch.

comment:10 by digitall, 13 years ago

OK.. Have confirned by Git bisect with Valgrind that fa7e8a8eb382c37d35d9166dcd87646811e4a377 is the first bad commit.

comment:11 by digitall, 13 years ago

Tested fuzzie's fix with Valgrind. No issues. Pushed fix to master as 8e42ee4c983167f0e2589b1401ba90a5df99fcc6. Will need to playtest IHNM and ITE to check that this hasn't caused any issues.

comment:12 by digitall, 13 years ago

Resolution: → fixed

comment:13 by digitall, 13 years ago

Owner: set to digitall

comment:14 by SF/m4xpower, 13 years ago

Thanks, guys! I'm glad I could contribute to your excellent project!

comment:15 by digitall, 13 years ago

Status: new → closed

comment:16 by digitall, 13 years ago

Completed full playtest successfully. Closing.

Note: See TracTickets for help on using tickets.