#4844 LURE: Valgrind Memory Errors When Skorl Gatekeeper Drinks

[digitall]

When the Skorl Gatekeeper lays down to drink from the cask, Valgrind indicates a number of memory errors associated with hotspots.
The console also issues the following warnings:
WARNING: Hotspot 2725h is not currently active!
WARNING: Hotspot 2726h is not currently active!

This is repeatable and is triggered by the following procedure:
1. Load Savegame
2. (Quickly) Use Tongs on Bung (in Cask)
3. Talk to Minnow (before he leaves room)
- Am here for Selena
- Tell Skorl that Cask is Leaking
4. Hide behind back right Pillar
5. Wait. Skorl Guard comes in, sees leaking cask and drinks. Error occurs.

Valgrind output is as follows :
==9090== Thread 1:
==9090== Invalid read of size 2
==9090== at 0x850847E: Lure::Hotspot::doAction(Lure::Action, Lure::HotspotData*) (hotspots.cpp:1368)
==9090== by 0x85093FC: Lure::Hotspot::doAction() (hotspots.cpp:1239)
==9090== by 0x8509CF8: Lure::HotspotTickHandlers::standardCharacterAnimHandler(Lure::Hotspot&) (hotspots.cpp:2645)
==9090== by 0x850A5DF: Lure::HotspotTickHandlers::jailorAnimHandler(Lure::Hotspot&) (hotspots.cpp:3208)
==9090== by 0x84FDE11: Lure::Hotspot::tick() (hotspots.cpp:477)
==9090== by 0x84FC1E4: Lure::Game::tick() (game.cpp:92)
==9090== by 0x84FC28B: Lure::Game::nextFrame() (game.cpp:125)
==9090== by 0x84FC536: Lure::Game::execute() (game.cpp:182)
==9090== by 0x84CF21F: Lure::LureEngine::go() (lure.cpp:160)
==9090== by 0x84D03B4: Lure::LureEngine::run() (lure.h:102)
==9090== by 0x80565C0: runGame(PluginSubclass<MetaEngine> const*, OSystem&, Common::String const&) (main.cpp:216)
==9090== by 0x8056F69: scummvm_main (main.cpp:389)
==9090== Address 0x611748c is 36 bytes inside a block of size 2,492 free'd
==9090== at 0x4024D9A: operator delete(void*) (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
==9090== by 0x84DAA45: Common::SharedPtrDeletionImpl<Lure::Hotspot>::~SharedPtrDeletionImpl() (ptr.h:48)
==9090== by 0x84D02B8: Common::SharedPtr<Lure::Hotspot>::decRef() (ptr.h:206)
==9090== by 0x84D02E8: Common::SharedPtr<Lure::Hotspot>::~SharedPtr() (ptr.h:122)
==9090== by 0x84D02FF: Common::ListInternal::Node<Common::SharedPtr<Lure::Hotspot> >::~Node() (list_intern.h:42)
==9090== by 0x84D739B: Common::List<Common::SharedPtr<Lure::Hotspot> >::erase(Common::ListInternal::NodeBase*) (list.h:241)
==9090== by 0x84D741E: Common::List<Common::SharedPtr<Lure::Hotspot> >::erase(Common::ListInternal::Iterator<Common::SharedPtr<Lure::Hotspot> >) (list.h:88)
==9090== by 0x84D3411: Lure::Resources::deactivateHotspot(unsigned short, bool) (res.cpp:680)
==9090== by 0x84E9539: Lure::Script::deactivateHotspot(unsigned short, unsigned short, unsigned short) (scripts.cpp:136)
==9090== by 0x84EA306: Lure::Script::execute(unsigned short) (scripts.cpp:1110)
==9090== by 0x8500EE1: Lure::Hotspot::npcExecScript(Lure::HotspotData*) (hotspots.cpp:2056)
==9090== by 0x8508366: Lure::Hotspot::doAction(Lure::Action, Lure::HotspotData*) (hotspots.cpp:1330)

ScummVM 1.2.0svn48453 (Apr 1 2010 03:32:24)
Features compiled in: Vorbis FLAC MP3 ALSA RGB zLib FluidSynth
built on Linux x86_32 2.6.31 by GCC 4.3.4

[digitall]

Savegame for replication

[digitall]

Easier Savegame for replication - Waiting for Skorl

[digitall]

Tested with :
ScummVM 1.2.0svn48657 (Apr 14 2010 11:07:00)
Features compiled in: Vorbis FLAC MP3 ALSA RGB zLib FluidSynth

Problem still present.

Added savegame for easier replication. Just wait for Skorl to arrive and drink, though this misses the
Hotspot warnings which occur while going to hide...

[Paul Gilbert]

Thanks for the savegame. Initial analysis indicated that the second problem (the warnings) were almost certainly due to at least one town NPC not being correctly disabled when you go to the castle. Not a major problem, and indeed the original (from memory) had the village NPCs still moving around even when you're in the castle. It's just the doorway hotspots are no longer active at that point, and I explicitly added warnings for these kind of things to the ScummVM module.

I need to progress to a point just before going to the castle (or if you have a handy savegame, that would be appreciated too), and I'll see about adding some extra code to disable all the town characters when you enter the castle.

[digitall]

Savegame in Village, just before leaving for castle - Talk to Ewan

[digitall]

I didn't have a savegame at this point, but since Lure is fairly quick to play through, I have produced one which has been attached to this bug.

Thanks for attention so far, but though the warnings should be fixed, I'm more interested in fixing the valgrind memory issues associated with the Skorl Gatekeeper.

I'll try to work out what the exact call to hotspots.cpp:1368 is causing the invalid read.

[digitall]

Debugging Log Fragment - Showing deletion of Hotspot 1011

[digitall]

The error is triggered by the Animation of the Skorl Gatekeeper lying down to start drinking.

This is called by Hotspot 1011 tick() which calls a NPC_EXEC_SCRIPT action.
The script called, 1e49h then proceeds to call a number of opcodes including
an opcode to deactivate Hotspot 1011 i.e. the calling hotspot.
The deactivation function also triggers the resource manager to deallocate
the hotspot object, which then triggers the issue when the debug call around
hotspots.cpp:1368 attempts to access the _hotspotId data element of a
deallocated object.

This is based on the attached debugging log fragment.

dreammaster : I'm not familiar enough with the engine structure, to determine exactly why the issue is happening and how to fix it e.g. Is this a script bug present in the original, an issue with the resource manager (destId?), a problem
with script opcode implementation. But I hope this helps you work out a fix. Thanks.

[Paul Gilbert]

That helped enormously, thanks. I've committed a change to hotspots.cpp - r48741. If you could verify it fixes the Valgrind warning, I'll then backport it to the 1.1 branch and close this ticket.

[digitall]

Tested with r48741 and savegame.
No valgrind issues found and visually looks correct.
Thanks for this fix.

P.S. The following warnings are still seen. Is this expected? :
"WARNING: Hotspot 2725h is not currently active!
WARNING: Hotspot 2726h is not currently active!"

[Paul Gilbert]

An earlier fix I added should resolve this problem, by de-activating all the active NPC characters in the village when you enter the castle (previously the game was still moving them around in the background). This means of course, that it won't automatically get applied on any existing savegames from when the player is already inside the castle. Since it's only a warning message, though, I don't consider this any issue for existing savegames.