Forget about ghosts! The true threats lurk in everyday things—like static_cast, which can unexpectedly drop all security efforts, and assert, which rapidly vanishes in a release build. Welcome to the world of self-made traps!
I explored the PVS-Studio warnings issued for the Xenia project in my previous article: "Realm of gaming experiments: potential developer errors in emulator creating". I delved into many intriguing cases and was about to put the project on the dusty shelf, moving on to other tasks. Before doing so, I decided to take another look at the warnings that weren't included before. One of them seemed strange to me: just five code lines, yet I couldn't figure out the author's intent. Even after discussing this code fragment with my colleagues, we couldn't explain it. So, I thought I'd share my thoughts on it in this short post.
Xenia is a research emulator for the Xbox 360 platform, enabling games originally developed for this console to run on modern PCs. The development community actively contributes to this open-source project.
As mentioned above, I analyzed the project using the PVS-Studio static analyzer. The checked code matches the 3d30b2e commit.
Let's dive into the warning.
First, let's take a look at a small class hierarchy:
class AudioDriver
{
public:
....
virtual void DestroyDriver(AudioDriver* driver) = 0;
....
};
class XAudio2AudioDriver : public AudioDriver
{
....
void Shutdown();
virtual void DestroyDriver(AudioDriver* driver);
....
};
Here's the code:
void XAudio2AudioSystem::DestroyDriver(AudioDriver* driver)
{
assert_not_null(driver);
auto xdriver = static_cast<XAudio2AudioDriver*>(driver);
xdriver->Shutdown();
assert_not_null(xdriver);
delete xdriver;
}
The PVS-Studio warning:
V595 The 'xdriver' pointer was utilized before it was verified against nullptr. Check lines: 48, 49. xaudio2_audio_system.cc 48
What noteworthy insights can be pointed out in this code snippet?
Just five lines of function, and yet there are more questions than answers... It's hard to determine the developers' intentions, but I can see two options:
To stay true to the author's original intention, it'd be better to bring the code to the following form:
void XAudio2AudioSystem::DestroyDriver(AudioDriver *driver)
{
assert_not_null(driver);
auto xdriver = dynamic_cast<XAudio2AudioDriver*>(driver);
assert_not_null(xdriver);
xdriver->Shutdown();
delete xdriver;
}
Interestingly, the developers overrode this function in SDLAudioDriver::DestroyDriver in exactly the same way.
However, I'd like to propose a better solution that avoids the conversion to the needed derived type. Let's take another look at the audio system and audio driver code.
class AudioDriver
{
public:
....
virtual ~AudioDriver();
....
};
class SDLAudioDriver : public AudioDriver
{
public:
....
~SDLAudioDriver() override;
....
void Shutdown();
....
};
class XAudio2AudioDriver : public AudioDriver
{
public:
....
~XAudio2AudioDriver() override;
....
void Shutdown();
....
};
class AudioSystem
{
public:
....
void UnregisterClient(size_t index);
....
protected:
....
virtual X_STATUS CreateDriver(size_t index,
xe::threading::Semaphore* semaphore,
AudioDriver** out_driver) = 0;
virtual void DestroyDriver(AudioDriver* driver) = 0;
....
static const size_t kMaximumClientCount = 8;
struct {
AudioDriver* driver;
uint32_t callback;
uint32_t callback_arg;
uint32_t wrapped_callback_arg;
bool in_use;
} clients_[kMaximumClientCount];
....
};
void AudioSystem::UnregisterClient(size_t index)
{
....
assert_true(index < kMaximumClientCount);
DestroyDriver(clients_[index].driver);
memory()->SystemHeapFree(clients_[index].wrapped_callback_arg);
clients_[index] = {0};
....
}
Both derived classes of the audio drivers have the same public, non-virtual Shutdown interface. So, the developers need to cast the derived audio driver class to the overridden AudioSystem::DestroyDriver, and then call the function.
They could move the Shutdown interface to the base class as a pure virtual function, and then make AudioSystem::DestroyDriver non-virtual—this would remove the duplicated code from its derivatives.
class AudioDriver
{
public:
....
virtual ~AudioDriver();
virtual void Shutdown() = 0;
....
};
class SDLAudioDriver : public AudioDriver
{
public:
....
~SDLAudioDriver() override;
....
void Shutdown() override;
....
};
class XAudio2AudioDriver : public AudioDriver
{
public:
....
~XAudio2AudioDriver() override;
....
void Shutdown() override;
....
};
class AudioSystem
{
protected:
....
void DestroyDriver(AudioDriver* driver);
....
};
void AudioSystem::DestroyDriver(AudioDriver* driver)
{
assert_not_null(driver);
std::unique_ptr<AudioDriver> tmp { driver };
tmp->Shutdown();
}
Wrapping a raw pointer in a std::unique_ptr will enable the developers not to worry about receiving a Shutdown exception—the operator delete will just remove the object within the pointer anyway.
If the developers need the AudioSystem derivative to override the behavior when the audio driver is removed, they can use the NVI (Non-Virtual Interface) idiom.
class AudioSystem
{
protected:
....
void DestroyDriver(AudioDriver* driver);
....
private:
virtual void DestroyDriverImpl(AudioDriver* driver);
....
};
void AudioSystem::DestroyDriverImpl(AudioDriver* driver)
{
driver->Shutdown();
}
void AudioSystem::DestroyDriver(AudioDriver* driver)
{
assert_not_null(driver);
std::unique_ptr<AudioDriver> _ { driver };
DestroyDriverImpl(driver);
}
Now, if the AudioSystem derivative requires another behavior when removing a driver, the developers just need to override the DestroyDriverImpl virtual function:
class SomeAudioSystem : public AudioSystem
{
....
private:
void DestroyDriverImpl(AudioDriver* driver) override;
};
I can now wrap up the bug inspection for this project, but perfection isn't a destination; it's a continuous journey that never ends. I'd love to hear your thoughts on this code snippet. Share them in the comments :)
I recommend trying the trial version of the PVS-Studio analyzer to easily spot suspicious code fragments. Let's collaborate to make code more reliable and secure!
0