[Scummvm-tracker] [ScummVM] #10365: SAGA: ITE: Crash when talking to Sist

[dafioram]

#10365: SAGA: ITE: Crash when talking to Sist
--------------------------------+--------------------------
  Reporter:  dafioram           |      Owner:  (none)
      Type:  defect             |     Status:  new
  Priority:  blocker            |  Component:  Engine: SAGA
Resolution:                     |   Keywords:
      Game:  Inherit the Earth  |
--------------------------------+--------------------------

Comment (by 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.

--
Ticket URL: <https://bugs.scummvm.org/ticket/10365#comment:3>
ScummVM <https://bugs.scummvm.org>
ScummVM