Opened 8 years ago
Closed 3 years ago
#9757 closed defect (outdated)
SCI: Access uninitialised memory in games with digital sound effects
Reported by: | criezy | Owned by: | |
---|---|---|---|
Priority: | normal | Component: | Engine: SCI |
Version: | Keywords: | midi | |
Cc: | Game: | King's Quest 6 |
Description
I ran ScummVM with valgrind today and it reports access to uninitialised memory when starting King Quest VI. Here is the valgrind report:
==79542== Conditional jump or move depends on uninitialised value(s) ==79542== at 0x10153A401: Sci::MusicEntry** Common::sortPartition<Sci::MusicEntry**, bool (*)(Sci::MusicEntry const*, Sci::MusicEntry const*)>(Sci::MusicEntry**, Sci::MusicEntry**, Sci::MusicEntry**, bool (*&)(Sci::MusicEntry const*, Sci::MusicEntry const*)) (algorithm.h:185) ==79542== by 0x101539BAC: void Common::sort<Sci::MusicEntry**, bool (*)(Sci::MusicEntry const*, Sci::MusicEntry const*)>(Sci::MusicEntry**, Sci::MusicEntry**, bool (*)(Sci::MusicEntry const*, Sci::MusicEntry const*)) (algorithm.h:222) ==79542== by 0x101536A1B: Sci::SciMusic::sortPlayList() (music.cpp:316) ==79542== by 0x10153743E: Sci::SciMusic::soundPlay(Sci::MusicEntry*) (music.cpp:461) ==79542== by 0x10153B8D4: Sci::SoundCommandParser::processPlaySound(Sci::reg_t, bool) (soundcmd.cpp:222) ==79542== by 0x10153B264: Sci::SoundCommandParser::kDoSoundPlay(Sci::EngineState*, int, Sci::reg_t*) (soundcmd.cpp:162) ==79542== by 0x1014A57D3: Sci::kDoSoundPlay(Sci::EngineState*, int, Sci::reg_t*) (ksound.cpp:52) ==79542== by 0x1014E83C3: Sci::callKernelFunc(Sci::EngineState*, int, int) (vm.cpp:462) ==79542== by 0x1014E5E09: Sci::run_vm(Sci::EngineState*) (vm.cpp:920) ==79542== by 0x10146EE21: Sci::SciEngine::runGame() (sci.cpp:654) ==79542== by 0x10146DBBC: Sci::SciEngine::run() (sci.cpp:425) ==79542== by 0x10000AFFB: runGame(PluginSubclass<MetaEngine> const*, OSystem&, Common::String const&) (main.cpp:263) ==79542== ==79542== Conditional jump or move depends on uninitialised value(s) ==79542== at 0x10153A401: Sci::MusicEntry** Common::sortPartition<Sci::MusicEntry**, bool (*)(Sci::MusicEntry const*, Sci::MusicEntry const*)>(Sci::MusicEntry**, Sci::MusicEntry**, Sci::MusicEntry**, bool (*&)(Sci::MusicEntry const*, Sci::MusicEntry const*)) (algorithm.h:185) ==79542== by 0x101539BAC: void Common::sort<Sci::MusicEntry**, bool (*)(Sci::MusicEntry const*, Sci::MusicEntry const*)>(Sci::MusicEntry**, Sci::MusicEntry**, bool (*)(Sci::MusicEntry const*, Sci::MusicEntry const*)) (algorithm.h:222) ==79542== by 0x101539BDF: void Common::sort<Sci::MusicEntry**, bool (*)(Sci::MusicEntry const*, Sci::MusicEntry const*)>(Sci::MusicEntry**, Sci::MusicEntry**, bool (*)(Sci::MusicEntry const*, Sci::MusicEntry const*)) (algorithm.h:224) ==79542== by 0x101536A1B: Sci::SciMusic::sortPlayList() (music.cpp:316) ==79542== by 0x10153743E: Sci::SciMusic::soundPlay(Sci::MusicEntry*) (music.cpp:461) ==79542== by 0x10153B8D4: Sci::SoundCommandParser::processPlaySound(Sci::reg_t, bool) (soundcmd.cpp:222) ==79542== by 0x10153B264: Sci::SoundCommandParser::kDoSoundPlay(Sci::EngineState*, int, Sci::reg_t*) (soundcmd.cpp:162) ==79542== by 0x1014A57D3: Sci::kDoSoundPlay(Sci::EngineState*, int, Sci::reg_t*) (ksound.cpp:52) ==79542== by 0x1014E83C3: Sci::callKernelFunc(Sci::EngineState*, int, int) (vm.cpp:462) ==79542== by 0x1014E5E09: Sci::run_vm(Sci::EngineState*) (vm.cpp:920) ==79542== by 0x10146EE21: Sci::SciEngine::runGame() (sci.cpp:654) ==79542== by 0x10146DBBC: Sci::SciEngine::run() (sci.cpp:425) ==79542== ==79542== Conditional jump or move depends on uninitialised value(s) ==79542== at 0x10153A401: Sci::MusicEntry** Common::sortPartition<Sci::MusicEntry**, bool (*)(Sci::MusicEntry const*, Sci::MusicEntry const*)>(Sci::MusicEntry**, Sci::MusicEntry**, Sci::MusicEntry**, bool (*&)(Sci::MusicEntry const*, Sci::MusicEntry const*)) (algorithm.h:185) ==79542== by 0x101539BAC: void Common::sort<Sci::MusicEntry**, bool (*)(Sci::MusicEntry const*, Sci::MusicEntry const*)>(Sci::MusicEntry**, Sci::MusicEntry**, bool (*)(Sci::MusicEntry const*, Sci::MusicEntry const*)) (algorithm.h:222) ==79542== by 0x101539BC1: void Common::sort<Sci::MusicEntry**, bool (*)(Sci::MusicEntry const*, Sci::MusicEntry const*)>(Sci::MusicEntry**, Sci::MusicEntry**, bool (*)(Sci::MusicEntry const*, Sci::MusicEntry const*)) (algorithm.h:223) ==79542== by 0x101536A1B: Sci::SciMusic::sortPlayList() (music.cpp:316) ==79542== by 0x10153743E: Sci::SciMusic::soundPlay(Sci::MusicEntry*) (music.cpp:461) ==79542== by 0x10153B8D4: Sci::SoundCommandParser::processPlaySound(Sci::reg_t, bool) (soundcmd.cpp:222) ==79542== by 0x10153B264: Sci::SoundCommandParser::kDoSoundPlay(Sci::EngineState*, int, Sci::reg_t*) (soundcmd.cpp:162) ==79542== by 0x1014A57D3: Sci::kDoSoundPlay(Sci::EngineState*, int, Sci::reg_t*) (ksound.cpp:52) ==79542== by 0x1014E83C3: Sci::callKernelFunc(Sci::EngineState*, int, int) (vm.cpp:462) ==79542== by 0x1014E5E09: Sci::run_vm(Sci::EngineState*) (vm.cpp:920) ==79542== by 0x10146EE21: Sci::SciEngine::runGame() (sci.cpp:654) ==79542== by 0x10146DBBC: Sci::SciEngine::run() (sci.cpp:425) ==79542==
Note: the reason I ran ScummVM with valgrind is because I got a crash once today when starting King Quest VI in ScummVM, but was not able to reproduce it afterward. The crash was due to hitting the assert on line 417 in resource_adio.cpp:
assert(offset + syncSize <= srcSize);
I don't know if the two are related, but since this is a possibility I am mentioning it here.
This is on macOS X 10.9.5 compiled by myself with the latest changes (commit f30f34cb).
Change History (6)
comment:1 by , 8 years ago
Summary: | Access uninitialised memory when starting KQ6 → SCI: Access uninitialised memory in games with digital sound effects |
---|
comment:2 by , 8 years ago
(Oh, and, this is unrelated to a resource_audio.cpp assertion; a non-reproducible assertion failure there is kind of scary to me since there should be no way for that to only assert some of the times you run a game.)
comment:3 by , 7 years ago
Keywords: | midi added |
---|---|
Owner: | set to |
comment:4 by , 7 years ago
Owner: | removed |
---|
comment:5 by , 6 years ago
Game: | → King's Quest 6 |
---|
comment:6 by , 3 years ago
Resolution: | → outdated |
---|---|
Status: | new → closed |
This has been fixed at some point in the last five years. All codepaths that reach SciMusic::sortPlayList()
now initialize _time first.
As for the assertion failure, csnover re-worked that and removed the assert() a few days after he posted his response.
The
MusicEntry::time
member is not initialized for sound effects that use sampled digital audio (MusicEntry::isSample == true
), so any game that plays sampled sounds will raise this warning when (1) the list of sounds is sorted, and (2) there are two sounds with identical priorities, and (3) one or more of the sounds is a digital sample.A possible solution to this would be to change the condition at the end of
SoundCommandParser::initSoundResource
to allow sampledMusicEntry
s to be initialised bySciMusic::soundInitSnd
, and then changesoundInitSnd
to just set the sampled MusicEntry’stime
property and return. I don’t know this code well enough to say whether or not this is a ‘correct’ fix though.