SAGA: ITE: Crash when talking to Sist

[dafioram]

ScummVM: Kirben32 2.1.0git-104-g789a38b38c
Game: Inherit the Earth/GOG/Multi-OS/English
OS: Win7-64

Crash when talking to Sist.

1. Load save game
2. Walk to the door where Sist is by following directions at bottom of page
3. Open door with right click, enter door with left click
4. Once you take your robe off right click on sist.

It will crash:

Assertion failed: idx < _size, file ./common/array.h, line 192

This save is before entering the tunnel if you load a saved game that is already
in the tunnel then the crash doesn't happen.

Its also possible that the crash will also happen under simplier circumstances.
I have difficulty reproducing this on linux. I was able to reproduce it once on
linux, but then not after. I can always repro on the windows build.

The directions to Sist's room are given below with respect to the direction your
character is facing and a direction is only given when there is a choice to be made.

-- Spoiler --

Right, Pass door, Straight, Right, through the next 2 doors, Left, Right, Straight

[dafioram]

Before Rat tunnel the first time.

[digitall]

Have tried to replicate with latest Git ie. ScummVM 2.1.0git161-g80dd7b2c0d on Linux x86_64.

Have loaded savegame and followed instructions to reach Sist and talk to him, but have not been able to trigger that array.h exception.

Running under GDB did not yield any useful information, but running under Valgrind showed three bad memory accesses in the SAGA engine code where unallocated/freed memory is accessed. This would likely account for the issue so these should be fixed to see if this corrected the problem.

These three accesses have very similar backtraces as follows:
==1876== Invalid read of size 4
==1876== at 0x18CEF02: Saga::HitZone::getFlags() const (objectmap.h:57)
==1876== by 0x18FB836: Saga::Actor::stepZoneAction(Saga::ActorData*, Saga::HitZone const*, bool, bool) (actor.cpp:552)
==1876== by 0x1904BE5: Saga::Actor::handleActions(int, bool) (actor_walk.cpp:695)
==1876== by 0x1904D46: Saga::Actor::direct(int) (actor_walk.cpp:727)
==1876== by 0x18DB323: Saga::SagaEngine::run() (saga.cpp:380)
==1876== by 0x576FDE: runGame(Plugin const*, OSystem&, Common::String const&) (main.cpp:264)
==1876== by 0x578221: scummvm_main (main.cpp:530)
==1876== by 0x5750EB: main (posix-main.cpp:45)
==1876== Address 0xb1b2ff0 is 80 bytes inside a block of size 120 free'd
==1876== at 0x4C2C13B: free (vg_replace_malloc.c:530)
==1876== by 0x18D2648: Common::Array<Saga::HitZone>::freeStorage(Saga::HitZone*, unsigned int) (array.h:320)
==1876== by 0x18D25AD: Common::Array<Saga::HitZone>::clear() (array.h:218)
==1876== by 0x18D2287: Saga::ObjectMap::clear() (objectmap.cpp:188)
==1876== by 0x18E1F49: Saga::Scene::endScene() (scene.cpp:1169)
==1876== by 0x18DFBEF: Saga::Scene::changeScene(short, int, Saga::SceneTransitionType, int) (scene.cpp:503)
==1876== by 0x18EC02C: Saga::Script::sfScriptGotoScene(Saga::ScriptThread*, int, bool&) (sfuncs.cpp:470)
==1876== by 0x18E5817: Saga::Script::opCcallV(Saga::ScriptThread*, Common::SeekableReadStream*, bool&, bool&) (script.cpp:637)
==1876== by 0x18F4CF8: Saga::Script::runThread(Saga::ScriptThread&) (sthread.cpp:208)
==1876== by 0x18F4951: Saga::Script::executeThreads(unsigned int) (sthread.cpp:156)
==1876== by 0x18DB363: Saga::SagaEngine::run() (saga.cpp:384)
==1876== by 0x576FDE: runGame(Plugin const*, OSystem&, Common::String const&) (main.cpp:264)
==1876== Block was alloc'd at
==1876== at 0x4C2AF0F: malloc (vg_replace_malloc.c:299)
==1876== by 0x18D2944: Common::Array<Saga::HitZone>::allocCapacity(unsigned int) (array.h:309)
==1876== by 0x18D270E: Common::Array<Saga::HitZone>::reserve(unsigned int) (array.h:273)
==1876== by 0x18D24C3: Common::Array<Saga::HitZone>::resize(unsigned int) (array.h:283)
==1876== by 0x18D21CF: Saga::ObjectMap::load(Saga::ByteArray const&) (objectmap.cpp:179)
==1876== by 0x18E165F: Saga::Scene::processSceneResources(Common::Array<Saga::SceneResourceData>&) (scene.cpp:1013)
==1876== by 0x18E04DD: Saga::Scene::loadScene(Saga::LoadSceneParams&) (scene.cpp:682)
==1876== by 0x18DFC08: Saga::Scene::changeScene(short, int, Saga::SceneTransitionType, int) (scene.cpp:506)
==1876== by 0x18DE6EA: Saga::SagaEngine::load(char const*) (saveload.cpp:369)
==1876== by 0x18DB05F: Saga::SagaEngine::run() (saga.cpp:343)
==1876== by 0x576FDE: runGame(Plugin const*, OSystem&, Common::String const&) (main.cpp:264)
==1876== by 0x578221: scummvm_main (main.cpp:530)

The second and third differ as follows:
3c3
< ==1876== by 0x18FB836: Saga::Actor::stepZoneAction(Saga::ActorData*, Saga::HitZone const*, bool, bool) (actor.cpp:552)
---

==1876== by 0x18FB85A: Saga::Actor::stepZoneAction(Saga::ActorData*, Saga::HitZone const*, bool, bool) (actor.cpp:552)

3c3
< ==1876== by 0x18FB836: Saga::Actor::stepZoneAction(Saga::ActorData*, Saga::HitZone const*, bool, bool) (actor.cpp:552)
---

==1876== by 0x18FB909: Saga::Actor::stepZoneAction(Saga::ActorData*, Saga::HitZone const*, bool, bool) (actor.cpp:563)

35a36

==1876==

This looks to be a likely issue in the Actor stepZoneAction / Hitzone code which should be corrected.

[dafioram]

I tested three different cases with valgrind running to check for uninitialized reads.

case 1: Load saved game right outside of Sist's room, enter itm and then talk to him
Valgrind report: no uninitialized reads

case 2: Load saved game at beginning of rat tunnel then walk to Sist's room, enter it and then talk to him.
Valgrind report: no uninitialized reads

case 3: Load saved game right outside of the rat tunnel, enter tunnel, then walk to Sist's room, enter it and then talk to him.
Valgrind report: The same three uninitialized reads reported above by digitall.

I am confident that fixing these uninitialized reads will prevent the crash on windows since the windows crash only happens in case 3 (only case when valgrind reports uninitialized reads).

At a high level I am not sure where things are going wrong.

At a low level Saga::Actor::handleActions is passing in a HitZone pointer that has not been loaded (I.e., HitZone::load() has not been called on the hitZone object) into Saga::Actor::stepZoneAction.

If the hitZone->load() is not called on a hitZone object then hitZone->_flag will not be initialized and Saga::HitZone::getFlags() (which returns hitZone->_flag) doesn't exist hence the uninitialized read.

I was able to determine this by adding a member variable to hitZone that starts off false and becomes true once hitZone->load() is called. When in the bug case, as I am entering Sist's room it calls stepZoneAction() ( actor_walk.cpp line 694) with a hitZone that has not been loaded. In the bug free cases all hitZones are loaded for each debug stop at actor_walk.cpp line 694.

I added some checks to return from the stepZoneAction() function (by doing a check in stepZoneAction) and returning if hitZone has not been loaded, but even then there are still other uninitialized reads. Namely: two of the conditionals in Actor::stepZoneAction

1. hitZone->isLoaded() (my function for determining if hitZone has been loaded) and
2. actor != _protagonist

I have even seen that my Boolean member variable that is the flag for being loaded or not, is occasionally set to a number!

I think the final fix will involve some conditional changes in Actor::handleActions rather than defensive changes in Actor::stepZoneAction. Since the changes to the latter wouldn't address the root cause.

[eriktorbjorn]

One possible lead: It looks to me as if some ActorData's _lastZone pointer (when you're in Sist's room) is still pointing to a hit zone in the door keeper's room, and that hit zone was deleted when you left the door keeper's room.

That's probably why savegames made on the way towards Sist's room work.

[eriktorbjorn]

One theory, that I didn't have the time to explore further:

I did notice that there seem to be two different actors involved. I don't have my debug log with me, but I seem to remember that one of them had its _lastZone pointer cleared when leaving the room with the door keeper, but another one had it tested (with the now invalid pointer) in Sist's room.

So maybe one of them is regular Rif and one of them is Rif in disguise. If so, there's probably a call to sfSwapActors() when Rif changes clothes. And in that case, maybe they shouldn't just swap location with each other, but also swap _lastZone?

[eriktorbjorn]

It looks like I'm on the right track. I've created a pull request with a proposed fix:

​https://github.com/scummvm/scummvm/pull/1489

[Filippos Karapetis <bluegr@…>]

In 66400e7:

SAGA: Fix potential crash when talking to Sist (bug #10365)

When Rif puts on the rat disguise, he is swapped out for a
different actor. When he takes off the disguise, after reaching
Sist's office, the old actor is swapped back in again.

At that point, original Rif's actor will have a _lastZone pointer
which not only is for the wrong room, it was deleted when Rif in
disguise left the room.

To fix this, we also swap the actors's _lastZone pointers.