# 5 lines of fortune: what program keeps under wraps
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\!
## Intro
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](https://pvs-studio.com/en/blog/posts/cpp/1177/)"\. 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 [open\-source project](https://github.com/xenia-project/xenia)\.
As mentioned above, I analyzed the project using the [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/) static analyzer\. The checked code matches the [3d30b2e](https://github.com/xenia-project/xenia/tree/3d30b2eec3ab1f83140b09745bee881fb5d5dde2) commit\.
Let's dive into the warning\.
## Here it is
First, let's take a look at a small class hierarchy:
```cpp
class AudioDriver
{
public:
....
virtual void DestroyDriver(AudioDriver* driver) = 0;
....
};
class XAudio2AudioDriver : public AudioDriver
{
....
void Shutdown();
virtual void DestroyDriver(AudioDriver* driver);
....
};
```
Here's the code:
```cpp
void XAudio2AudioSystem::DestroyDriver(AudioDriver* driver)
{
assert_not_null(driver);
auto xdriver = static_cast(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](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/apu/xaudio2/xaudio2_audio_system.cc#L48-L49)
What noteworthy insights can be pointed out in this code snippet?
1. The _XAudio2AudioSystem_ class is derived from _AudioDriver_\. So, the pointer to the base _driver_ type is passed to the _XAudio2AudioSystem::DestroyDriver_ function\.
1. The [_assert\_not\_null_](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/base/assert.h#L66) macro checks the pointer state\. It expands into [_xenia\_assert_](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/base/assert.h#L24), which expands into the standard [_assert_](https://en.cppreference.com/w/cpp/error/assert)\. Yes, this macro is removed in release builds, but I'll set that point aside\. In debug builds, it helps check if the pointer is non\-_null_\.
1. Next, the _driver_ pointer is cast to the pointer to the derived _xdriver_ class via _static\_cast_\. Then there's no check to ensure which object the pointer refers to\. The compiler simply checks whether such a casting is valid according to the standard, and whether it's valid within the context\. In this case, the resulting pointer is also non\-_null_, but maybe incorrect\.
1. The _xdriver_ pointer is dereferenced and the non\-static _XAudio2AudioSystem::Shutdown_ member function is called\. If the specified dynamic object type differs from _XAudio2AudioSystem_ or its derivatives, the behavior will be undefined, as it violates the [strict aliasing](https://en.cppreference.com/w/cpp/language/reinterpret_cast#Type_aliasing) rules\.
1. 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 [segfault](https://pvs-studio.com/en/blog/terms/0063/) in the debugger\.
* The developers created the check because the pointer is further passed to the operator _delete_\. Perhaps the reasoning was, "Something bad can happen if we pass it a _null_ pointer, let's debug it in this case"\. Fortunately, nothing will happen—the operator _delete_ handles _null_ pointers perfectly well\. As we've already realized, _xdriver_ will always be non\-_null_\.
To stay true to the author's original intention, it'd be better to bring the code to the following form:
```cpp
void XAudio2AudioSystem::DestroyDriver(AudioDriver *driver)
{
assert_not_null(driver);
auto xdriver = dynamic_cast(driver);
assert_not_null(xdriver);
xdriver->Shutdown();
delete xdriver;
}
```
Interestingly, the developers overrode this function in [_SDLAudioDriver::DestroyDriver_](https://github.com/xenia-project/xenia/blob/3d30b2eec3ab1f83140b09745bee881fb5d5dde2/src/xenia/apu/sdl/sdl_audio_system.cc#L44-L50) 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
```cpp
class 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
```cpp
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\.
The fixed code
```cpp
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 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](https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Non-Virtual_Interface) \(Non\-Virtual Interface\) idiom\.
The fix via NVI
```cpp
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 _ { 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:
```cpp
class SomeAudioSystem : public AudioSystem
{
....
private:
void DestroyDriverImpl(AudioDriver* driver) override;
};
```
## Outro
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](https://pvs-studio.com/en/pvs-studio/try-free/) version of the PVS\-Studio analyzer to easily spot suspicious code fragments\. Let's collaborate to make code more reliable and secure\!