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 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.
A quick note about Xenia
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 static analyzer. The checked code matches the
What noteworthy insights can be pointed out in this code snippet?
- The XAudio2AudioSystem class is derived from AudioDriver. So, the pointer to the base driver type is passed to the XAudio2AudioSystem::DestroyDriver function.
- The , which expands into the standard rules.
- Afterward, the developers wonder whether the pointer is null and checks the xdriver pointer.
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:
- The developers may have added the last check to debug a potential null pointer that could return after static_cast. However, the pointer will always be non-null. Even in an alternative scenario, instead of receiving a meaningful message from the assert_not_null macro, the developer would encounter a 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.
The audio driver hierarchy
CODEclass AudioDriver
{
public:
....
virtual ~AudioDriver();
....
};
class SDLAudioDriver : public AudioDriver
{
public:
....
~SDLAudioDriver() override;
....
void Shutdown();
....
};
class XAudio2AudioDriver : public AudioDriver
{
public:
....
~XAudio2AudioDriver() override;
....
void Shutdown();
....
};
The audio system hierarchy
CODEclass 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.
The fixed code
CODEclass 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 version of the PVS-Studio analyzer to easily spot suspicious code fragments. Let's collaborate to make code more reliable and secure!
↗ Original-Artikel auf dev.to lesenVollständiger Original-BerichtAusführliche Details, Code-Beispiele & Hersteller-Stellungnahme auf dev.to.
SOCIAL SHARE CARD GENERATOR