SHERLOCK: ROSETATTOO: Crash when clicking during flashback scene at Cleopatra's Needle

[PushmePullyu]

Tested with master 645230091f29ddcb73474cd820581e5f739afa15 on Linux x86_64 using the English CD version of Rose Tattoo.

The flashback scene with Needhem at Cleopatra's Needle uses the following talk files via talkTo():

from SherlockEngine::sceneLoop():    Need18a
from SherlockEngine::sceneLoop()
    Tattoo::TattooScene::doBgAnim()
        BaseObject::checkObject():   movi68s // This disables the mouse via TattooTalk::cmdMouseOnOff(), see below
                                     Boom68d
                                     Kick68d
                                     Prus68d
                                     Fade68d // This enables the mouse via TattooTalk::cmdMouseOnOff(), see below
from SherlockEngine::sceneLoop():    Fade68d // after changing scene, with _scriptMoreFlag set to 1 and _scriptSaveIndex set to 380
from SherlockEngine::sceneLoop()
    Tattoo::TattooScene::doBgAnim()
        Sprite::checkSprite():       Strt18s.tlk // This should now be called with _scriptMoreFlag unset, but see below

Starting a walk action by left clicking on a valid target position right before the first talkTo() call for "Fade68d" will hit this code:
engines/sherlock/talk.cpp:176 in talkTo():

if (people[HOLMES]._walkCount || (!people[HOLMES]._walkTo.empty() &&
			(IS_SERRATED_SCALPEL || people._allowWalkAbort))) {
		// Only interrupt if trying to do an action, and not just if player is walking around the scene
		if (people._allowWalkAbort) {
			abortFlag = true;
			// an arrow zone might have been clicked before the interrupt, cancel the scene transition
			ui._exitZone = -1;
		}

		people[HOLMES].gotoStand();
	}

In addition to stopping the walk action via gotoStand(), this also sets abortFlag (_allowWalkAbort is always true in Rose Tattoo), which in turn causes _talkToAbort to be set:
engines/sherlock/talk.cpp:462 in talkTo():

_talkToAbort = abortFlag;

The next talkTo() call for "Fade68d" will now return early due to _talkToAbort, and the _scriptMoreFlag intended for this call will remain set.
When calling talkTo() for "Strt18s.tlk", this flag causes doScript() to incorrectly use the index in _scriptSaveIndex, which leads to interpreting out-of-bounds data as a script, possibly crashing the game. The earlier talkTo() calls are not affected because doBgAnim() resets _talkToAbortFlag.

The original game does not exhibit the problem: it completely disables player control and hides the cursor in "movi68s" via opcode 0xAE (TattooTalk::cmdMouseOnOff()) and undoes this in "Fade68d". In ScummVM however, the opcode only hides the cursor (which is immediately shown again).

To reproduce:

1. Load this save: https://bugs.scummvm.org/attachment/ticket/14580/rosetattoo.015
2. Talk to Needhem and select option 1: "You witnessed..."
3. Near the end of the flashback scene, when the two men are leaving: keep spam-clicking somewhere Sherlock could walk to

If you are unlucky in causing a crash, setting a breakpoint in Sherlock::Talk::talkTo(Common::String) with the condition

filename._str[0] == 'S' && _scriptMoreFlag

will show if the bug was triggered.

[PushmePullyu]

PR for this is here: ​https://github.com/scummvm/scummvm/pull/5318

[PushmePullyu]

Forgot to link the DOS save (so original behavior can be verified): https://bugs.scummvm.org/attachment/ticket/14580/HOLMES2.SAV