🔧 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". 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.

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.

Here it is

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?

1. The XAudio2AudioSystem class is derived from AudioDriver. So, the pointer to the base driver type is passed to the XAudio2AudioSystem::DestroyDriver function.
2. The assert_not_null macro checks the pointer state. It expands into xenia_assert, which expands into the standard 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.
3. 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.
4. 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 rules.
5. 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 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:

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.

The audio driver hierarchy

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

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

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.

The fix via NVI

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;
};

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 version of the PVS-Studio analyzer to easily spot suspicious code fragments. Let's collaborate to make code more reliable and secure!