QFG4: Crash casting Trigger on squid monolith after using grapnel

[Vhati]

ScummVM 2.1.0git4278-gd31e37683c (Dec 13 2018 04:20:25)
Windows 7 64bit
QFG4 CD (English)

Using the grapnel on the ledge beside the squid monolith, followed by casting the Trigger spell on the monolith, crashes the game.

lookupSelector: Attempt to send to non-object or invalid script.
Address 0000:0000, method triggerEffect::setScaler
(room 800, script 64998, localCall ffffffff)!

• A new character only acquires the Trigger spell by starting as a mage.
• The grapnel item (from the Adventurers' Guild) is not naturally available to mages.
• This bug is possible for a non-mage imported character who learned the Trigger spell in a previous game.

The original interpreter also does this.

File - 5kb MD5 - Full MD5
RESOURCE.000 - 263dce4aa34c49d3ad29bec889007b1c - 1364ba69e3c0abb68cc0170650a56692
RESOURCE.AUD - c39521bffb1d8b19a57394866184a0ca - 71098b9e97e20c8941c0e4812d5f906f
RESOURCE.MAP - aba367f2102e81782d961b14fbe3d630 - 801a04cc6aa5d437681a2dd0b6545248
RESOURCE.SFX - 3cf95e09dab8b11d675e0537e18b499a - 7c858d7253f86dab4cc6066013c5ecec

[Vhati]

Occurs in the floppy edition under ScummVM, too.


QFG4 Floppy 1.1a + note patch (English)

File - 5kb MD5 - Full MD5
RESOURCE.000 - f64fd6aa3977939a86ff30783dd677e1 - ff42260a665995a85aeb277ad80aac8a
RESOURCE.MAP - d10a4cc177d2091d744e2ad8c049b0ae - 3695b1b0a1d15f3d324ea9f0cc325245
RESOURCE.SFX - 3cf95e09dab8b11d675e0537e18b499a - 7c858d7253f86dab4cc6066013c5ecec

[Vhati]

Quick and dirty way to reproduce...

• Create a new mage character.
• Get the grapnel item.
  • send hero get 16
• Teleport to the squid monolith.
  • room 800
• Click the grapnel on the east ledge.
• Hero will have insufficient climbing skill and walk back to the monolith.
• Bring up the spell list (star icon), and cast "Trigger" on the monolith (1st row, 3rd icon).

Sidenote:
A non-mage with points in Magic can acquire Trigger with "send hero learn 22".
Climbing skill can be adjusted to succeed with "vv g 258 250".

[Vhati]

I had to decompile this first quote by hand because SCI Companion got confused.


script 11 - castTriggerScript::changeState(2)

(triggerEffect
	x: g441_myX
	y: g442_myY
	setScaler: g0_hero
	cycleSpeed: 0
	setPri: 180
	init:
	setCycle: Fwd
)
#...

The Trigger spell is calling triggerEffect::setScaler(g0_hero).


script 64998 - Prop::setScaler()

(method (setScaler param1)
	(if scaler (scaler dispose:) (= scaler 0))
	(cond 
		((or (not argc) (not param1)) (return))
		((param1 isKindOf: Scaler)
			(= scaler
				(if (& (param1 -info-?) $8000)
					(param1 new:)
				else
					param1
				)
			)
			(= scaleSignal (| scaleSignal $0001))
			(= scaleSignal (& scaleSignal $fffd))
			(scaler init: self &rest)
		)
# An arg was given, hero, which is not itself a Scaler. We land here.
		(else
			(= scaler ((param1 scaler?) new:))
			(= scaleSignal (param1 scaleSignal?))
			(= maxScale (param1 maxScale?))
			(scaler client: self doit:)
		)
	)
)

There's an attempt to clone hero's scaler by calling new() on it.
Problem: hero's scaler is null.


script 800 - sUseTheGrapnel::changeState(5)

(5
	(g0_hero
		view: 7
		setLoop: 0 1
		setCel: 0
		setScale:
		cycleSpeed: 8
		setCycle: End self
	)
)

hero::setScale() is given no arg.


script 64998 - Prop::setScale()

(method (setScale param1)
	(if scaler (scaler dispose:) (= scaler 0))
	(if (not argc)
		(super setScale:)
	else
		(super setScale: param1)
	)
)

The superclass method, View::setScale(), without args, just twiddles some bit flags on the "scaleSignal" property.

Using the grapnel nulls out hero's scaler.
The Trigger spell can't handle the null.

• Throughout the game, "setScale: [blank]" and "setScale: 0" are frequent.
  • For hero, that's usually soon followed by assigning a new scaler, but not always.

• "setScaler: global0" appears frequently too. Mostly for the torch halo and Levitate sparkles.
  • Any of those could crash on a null too. Mitigated by rooms commonly setting a scaler on hero in init(). One example was cautious enough to check first if hero HAD a scaler to clone.

[Vhati]

The superclass method, View::setScale(), without args, just twiddles some bit flags on the "scaleSignal" property.

I think it's locking the scale to the last value set by the scaler.

Scalers react to changes in Y position to shrink hero far into the horizon. It'd make sense to remove scalers when vertical movement on the rope is about to happen.

With that in mind, the rope should be putting a scaler back on after climbing is finished.

[Vhati]

Eh, leaving the scaler on during the climb isn't so bad.
Does noticeably grow a little on the way down.
If all else fails, I can go with that.


I see.

• sUseTheGrapnel::changeState() climbs up.
  • State 4 moves hero to 242 y_145.
  • State 5 freezes hero's scale.
• sGetDown::changeState() climbs down.
  • State 6 moves back to x:242 y:145.
  • State 7 would be the time to put a scaler on.
    • Same y pos: no sudden scale change.

As luck would have it, local19 is never used.
We can stash the scaler there while it's not on hero!


I experimented with the debugger.

• I nulled hero's "scaler" property to see what would happen.
• Recklessly stashed its address in a global - in case there was garbage collection going on.
• hero didn't shrink during the climb up, didn't grow during the climb down.
• I reattached the scaler. Dynamic shrinking resumed, and Trigger worked.

Should be able to assign hero's scaler to local19 in the room's init(), then put it back on hero in sGetDown.

[Vhati]

Dang it. The Trigger spell can be cast while hero is standing up on the ledge.

If I remove and reattach the scaler, I'd also need to prevent casting in between.

Or... create a frozen-value scaler in the room's init(), where there's space... local6 is available... attach that in between... and dispose it with the rm800::dispose().

Annnnd gotta keep the original scaler if the climb up was aborted.


So much effort. Leaving the original scaler on is sounding better.


What about having Prop::setScaler() NOT clone when null?
Dunno if there's room, or if that's all it'd need.

[Vhati]

What about having Prop::setScaler() NOT clone when null?
Dunno if there's room, or if that's all it'd need.

Nah. Would've prevented other potential crashes, but I could only shave off 1 byte. :/

May need to investigate all the other hero scaler nulling scripts for potential interactions with setScaler(hero) scripts, fixing them separately.

[Vhati]

​Pull Request: SCI32: QFG4 Fix null scaler, climbing by monolith

[Vhati]

Another idea, though I don't see room for it.

On the climb up, re-init() the original scaler with args to make it INTO a dummy scaler. Then on the climb down, re-init() with the original args.

[Vhati]

Jackpot!!!

Script 800 has two long seemingly unused procedures, proc800_3() and proc800_4().
And they're exported for running via calle!

The content of both is identical to the room's init().


Besides providing plenty of room for fixing this bug, they could be used to fix bugs anywhere!

Their signatures don't include parameters. Maybe args don't need to be formally declared. If they can accept args, they can perform multiple fixes.

[Vhati]

Script 800 has two long seemingly unused procedures
[...]
The content of both is identical to the room's init().

*sigh* It was a mirage. Those were not real procedures.

The content of all three had the same addresses.

Either rm800's init() was being exported multiple times, or SCI Companion was glitching.

ScummVM's patcher only sees one copy of rm800::init(), even if I tell it to look for a line 3 times and modify hits as it goes.

[bluegr]

Thanks for your work! The pull request has been merged, so this can be closed now