[Scummvm-tracker] [ScummVM] #9640: SCI: GK1: Mime crash in Jackson Square

[sluicebox]

#9640: SCI: GK1: Mime crash in Jackson Square
----------------------+------------------------------
Reporter:  sluicebox  |      Owner:
    Type:  defect     |     Status:  new
Priority:  normal     |  Component:  Engine: SCI
Keywords:  sci32      |       Game:  Gabriel Knight 1
----------------------+------------------------------
 There is a concurrency bug in the scripts for the mime in Jackson Square
 that can crash the game in ScummVM. This is a script bug in the original
 game, though I don't know if it crashes the original interpreter. It is
 difficult, maybe impossible, to reproduce on purpose due to the large
 timeouts and small timing windows involved, but it's easy to simulate in
 the ScummVM debugger. Although it is unlikely to happen, it did happen to
 me, and I was able to keep the process open and use the debugger to
 inspect the state and verify these findings.

 On Day 1, if you take the mime to the southeast corner of the park and
 walk near the blues band then the mime will stand in front of them and
 annoy them. Where he's standing is in the path of the grandma and if she
 walks near him during this then he will break away and start annoying her.
 Depending on the timing, this will crash the game with:
 '''
 [CelObjView::getNumCels]: loop number 4 is equal to loop count in view
 4201, method xMime::lastCel (room 430, script 64998, localCall
 ffffffff)!'''

 This error gets thrown by ScummVM SCI32 code that's trying to trap places
 in games that depend on a bug in the original interpreter that returned
 random memory when asking for the number of cells of one past the last
 loop index. In this case, the game isn't relying on that, this trap just
 happens to have caught two game scripts colliding and setting an invalid
 loop.

 The script bug technically exists on all four Jackson Square rooms but as
 far as I can tell the conditions to trigger it are only in the southeast
 room, 430. In other rooms the pedestrian paths aren't located where they
 interrupt the mime when annoying a performer.

 If you get near the mime, he follows you. If he gets close to a
 pedestrian, he will annoy them and be shooed away and return to his
 original location. If he gets close to a stationary performer then he will
 annoy them for 20 seconds before being shooed away. If you walk back to
 the mime while he's annoying a performer then he will stop and follow you
 again. So, you lose the mime to pedestrians and performers but you have 20
 seconds to reclaim him from performers.

 Pedestrian scripts and mime scripts are in script 401 and are used by all
 four Jackson Square rooms.
 Stationary performer scripts are independently implemented in their own
 room scripts, 430 for southeast.
 It's probably not a good sign that the room-specific Code instances are
 named "specialMimeCode"...

 * When walking by the blues band, sAnnoyBlues (430) is run and sets
 mimeTimer4 for 20 seconds. mimeTimer4 in turn runs sMimeLeaves.
 * When the mime is near a pedestrian, sMimeFollowsPed (401) is run,
 causing the mime to annoy, be shooed, and leave.
 * When reclaiming the mime from the blues bands, mimeTimer4 is stopped.
 * When a pedestrian gets near the mime while he's annoying the blues band,
 mimeTimer4 *isn't* stopped!

 mimeTimer4 not being stopped means it can fire while the mime is annoying
 the pedestrian, causing sMimeLeaves and sMimeFollowsPed to run at the same
 time and step on each other as they both manipulate xMime (401). Usually
 this results in harmless graphical glitching (the mime is shooed away by
 grandma, then teleports back to be shooed away by the band), but if the
 timing is right then sMimeLeaves changes xMime:loop in between
 sMimeFollowsPed states and sMimeFollowsPed increments the loop number out
 of bounds.

 Egregious oversimplification of scripts:

 {{{
 sMimeFollowsPed (401)
 State 0:
         local7 = (pedestrian == grandma or woman) ? 0 : 1
         cycles = 1
 State 2:
         xMime:view = 4201
         xMime:loop = local7
 State 4:
         grandma dialog: "I'll call the police!"
         ticks = 120
 State 5
         xMime:cel = 0
         xMime:loop = xMime:loop + 2 // uh-oh
         xMime:setCycle(End self) // kaboom if xMime:loop is 4
 ...

 sMimeLeaves (430)
 State 0:
         ticks = 60
 State 1
         xMime:view = 4201
         xMime:cel = 0
         xMime:loop = 2 // UH-OH!
         xMime:setCycle(End self)
 ...
 }}}

 That's a 120-ish tick window in between sMimeFollowsPed setting the loop
 to zero and incrementing it by two. If sMimeLeaves:changeState(1) occurs
 during that then the loop is set to two before being incremented to four
 and is then out of bounds.

 To reproduce the crash with the debugger, get the mime to annoy the
 grandma. Break into the debugger as soon as she speaks,
 sMimeFollowsPed:state should be 4 and xMime:loop should be 0. Simulate
 sMimeLeaves' interference with "send xMime loop 2". Leave the debugger and
 the game will quickly crash. This is the same backtrace that I got from
 the real crash. I confirmed in the real crash that sMimeLeaves was in
 state 1.

 To attempt to reproduce the real crash, get the mime to annoy the band
 soon before the grandma appears on the screen. Even if it doesn't crash,
 you'll probably see the mime glitch out and teleport around.

 Ideally these scripts wouldn't run at the same time, but I doubt that's
 possible with script patching since these scripts are spread across two
 resources, half is common code and the other is room-specific, and I don't
 imagine backporting synchronization to be easy, even without bytecode
 constraints.

 I think the bug can be mitigated by patching
 sMimeFollowsPed:changeState(5) to set xMime:loop to local7 + 2 instead of
 xMime:loop + 2. local7 has the same value and isn't set anywhere else so
 it's safe to use instead of the mutable shared state. You still have the
 two scripts stepping on each other and bouncing the mime around but as far
 as I can tell there's nothing that crashes. I omitted a lot of stuff from
 my script summary though so someone who knows what they're doing should
 take a look.

 Attached is a save game in southeast Jackson Square right before the
 grandma shows up, and a screenshot of the backtrace from the real crash.

--
Ticket URL: <https://bugs.scummvm.org/ticket/9640>
ScummVM <https://bugs.scummvm.org>
ScummVM