Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#4844 closed defect (fixed)

LURE: Valgrind Memory Errors When Skorl Gatekeeper Drinks

Reported by: digitall Owned by: dreammaster
Priority: normal Component: Engine: Lure
Keywords: Cc:
Game: Lure of the Temptress

Description

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

Ticket imported from: #2980384. Ticket imported from: bugs/4844.

Attachments (4)

lure.007 (11.9 KB) - added by digitall 9 years ago.
Savegame for replication
lure.001 (11.9 KB) - added by digitall 9 years ago.
Easier Savegame for replication - Waiting for Skorl
lure.010 (23.8 KB) - added by digitall 9 years ago.
Savegame in Village, just before leaving for castle - Talk to Ewan
output-fragment.txt (3.6 KB) - added by digitall 9 years ago.
Debugging Log Fragment - Showing deletion of Hotspot 1011

Download all attachments as: .zip

Change History (13)

Changed 9 years ago by digitall

Attachment: lure.007 added

Savegame for replication

Changed 9 years ago by digitall

Attachment: lure.001 added

Easier Savegame for replication - Waiting for Skorl

comment:1 Changed 9 years ago by 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...

comment:2 Changed 9 years ago by dreammaster

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.

Changed 9 years ago by digitall

Attachment: lure.010 added

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

comment:3 Changed 9 years ago by 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.

Changed 9 years ago by digitall

Attachment: output-fragment.txt added

Debugging Log Fragment - Showing deletion of Hotspot 1011

comment:4 Changed 9 years ago by 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.

comment:5 Changed 9 years ago by dreammaster

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.

comment:6 Changed 9 years ago by 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!"

comment:7 Changed 9 years ago by dreammaster

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.

comment:8 Changed 9 years ago by dreammaster

Status: newclosed

comment:9 Changed 9 years ago by dreammaster

Resolution: fixed
Note: See TracTickets for help on using tickets.