Skip to content

Commit

Permalink
KYRA: Silence analysis tools about possible memory leaks
Browse files Browse the repository at this point in the history
  • Loading branch information
Johannes Schickel committed May 15, 2016
1 parent 109c54c commit 3664caa
Showing 1 changed file with 8 additions and 0 deletions.
8 changes: 8 additions & 0 deletions engines/kyra/staticres.cpp
Expand Up @@ -838,6 +838,14 @@ void KyraEngine_LoK::initStaticResource() {
SoundResourceInfo_PC98 resInfoIngame("KYRAM%d.DAT");
_sound->initAudioResourceInfo(kMusicIntro, &resInfoIntro);
_sound->initAudioResourceInfo(kMusicIngame, &resInfoIngame);

// This should never happen, but we add this to silence static
// analysis tools which complain about memory leaks.
delete[] soundFiles;
} else {
// This should never happen, but we add this to silence static
// analysis tools which complain about memory leaks.
delete[] soundFiles;
}
}

Expand Down

2 comments on commit 3664caa

@sev-
Copy link
Member

@sev- sev- commented on 3664caa May 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean to "silence"? There was a memory leak, you never release it, and copy in the object constructors the file names, leaving the original allocated memory intact.

Or am I missing something?

@lordhoto
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I looked at your changes I assumed this was based on some static analysis tool output because the change looked blindly done and untested. Doing a test run with valgrind showed that it was now indeed suffering from use-after-free.

The object constructors kept the pointer around, they never did any copying. This is why this caused invalid reads when running the games: you released the memory even though it was still in use.
There still was the problem that the allocated memory did not get freed after use. The structures did handle both unmanaged pointers and managed pointers. There was no special handling to release unmanaged memory. I already fixed this leak case as a side effect in 2317e3f where now only managed memory is passed, which is what the code assumed.

Please sign in to comment.