Гадание на пяти строчках: о чем молчит программа

Введение
А вот и он
Итоги

Забудьте о призраках, настоящая угроза кроется в повседневных вещах, таких как static_cast, который может неожиданно лишить вас безопасности, и assert, стремительно исчезающий в релизной сборке. Добро пожаловать в мир ловушек, созданных собственными руками!

Введение

В своей недавней статье "Игровое поле экспериментов: какие ошибки могут подстерегать программиста при создании эмулятора" я разбирала срабатывания анализатора PVS-Studio в проекте Xenia. Было рассмотрено много интересных случаев, и я уже собиралась отложить проект и перейти к другим задачам, однако решила взглянуть ещё раз на срабатывания, которые не попали в статью. Одно из них показалось мне странным: всего пять строчек кода, но я никак не могла разгадать замысел автора. Даже обсудив этот фрагмент с коллегами, мы не смогли его объяснить. Поэтому я решила поделиться размышлениями в этой небольшой заметке.

Коротко о Xenia, если вы не знаете, о чем идёт речь ▼

Xenia — это экспериментальный эмулятор платформы Xbox 360. Он предназначен для воспроизведения игр, изначально разработанных для этой консоли, на современных ПК. Этот проект с открытым исходным кодом активно развивается благодаря поддержке сообщества.

Как уже упоминалось, анализ проводился с использованием статического анализатора PVS-Studio. А для проверки я использовала состояние репозитория на момент коммита 3d30b2e.

Давайте вместе рассмотрим это срабатывание.

А вот и он

Прежде чем перейти к примеру, нужно познакомиться с небольшой иерархией классов:

class AudioDriver
{
public:
  ....
  virtual void DestroyDriver(AudioDriver* driver) = 0;
  ....
};

class XAudio2AudioDriver : public AudioDriver 
{ 
  ....
  void Shutdown();
  virtual void DestroyDriver(AudioDriver* driver);
  ....
};

А теперь непосредственно код:

void XAudio2AudioSystem::DestroyDriver(AudioDriver* driver)
{
  assert_not_null(driver);
  auto xdriver = static_cast<XAudio2AudioDriver*>(driver);
  xdriver->Shutdown();
  assert_not_null(xdriver);
  delete xdriver;
}

Предупреждение PVS-Studio:

V595 The 'xdriver' pointer was utilized before it was verified against nullptr. Check lines: 48, 49. xaudio2_audio_system.cc 48

Этот фрагмент примечателен следующими вещами:

Класс XAudio2AudioSystem является наследником AudioDriver. Значит, в функцию XAudio2AudioSystem::DestroyDriver прилетает указатель на базовый тип (driver).
Макрос assert_not_null проверяет его состояние. Он разворачивается в xenia_assert, а тот в свою очередь в стандартный assert. Да, этот макрос удаляется под релизом, но мы опустим этот момент. В дебаге он помогает нам узнать, что указатель не нулевой.
Далее указатель driver преобразуется к указателю на производный класс (xdriver) через static_cast. При таком способе не происходит проверка, какой именно объект на самом деле лежит под этим указателем. Компилятор всего лишь проверяет, валидно ли такое преобразование согласно стандарту, и оно валидно в этом контексте. При этом результирующий указатель также ненулевой, но не факт, что корректный.
Указатель xdriver разыменовывается, и вызывается нестатическая функция-член XAudio2AudioSystem::Shutdown. Если динамический тип объекта под этим указателем отличен от XAudio2AudioSystem или его наследников, то поведение будет не определено (нарушаются правила strict aliasing).
После этого разработчик задумался, а не нулевой ли случаем этот указатель, и добавил проверку указателя xdriver.

Буквально пять строк функции, а вопросов больше, чем ответов... Тяжело сказать, чего именно пытался добиться разработчик, но я вижу два возможных варианта:

последнюю проверку поставили, потому что разработчик захотел при случае отладить нулевой указатель, который может вернуться после static_cast. К сожалению, указатель всегда будет ненулевым. Даже если представить иную ситуацию, то вместо осмысленного сообщения от макроса assert_not_null разработчик будет иметь дело в отладчике с segfault;
последнюю проверку поставили, потому что далее указатель отдаётся в оператор delete. "Вдруг произойдёт что-то нехорошее, если мы передадим ему нулевой указатель, дай-ка подебажусь при таком случае". К счастью, ничего страшного не произойдёт — оператор delete прекрасно обрабатывает нулевые указатели. И как мы уже поняли, xdriver всегда будет ненулевой.

Если постараться сохранить исходный замысел автора, то лучше привести код к следующему виду:

void XAudio2AudioSystem::DestroyDriver(AudioDriver *driver)
{
  assert_not_null(driver);
  auto xdriver = dynamic_cast<XAudio2AudioDriver*>(driver);
  assert_not_null(xdriver);
  xdriver->Shutdown();
  delete xdriver;
}

Что интересно, переопределение этой функции, но в другом наследнике (SDLAudioDriver::DestroyDriver), реализовано точно таким же образом.

Однако я бы хотела предложить исправление получше, которое позволит избавиться от преобразования к нужному производному типу. Давайте ещё раз взглянем на код аудиосистемы и аудио драйверов.

class AudioDriver
{
public:
  ....
  virtual ~AudioDriver();
  ....
};

class SDLAudioDriver : public AudioDriver
{
public:
  ....
  ~SDLAudioDriver() override;
  ....
  void Shutdown();
  ....
};

class XAudio2AudioDriver : public AudioDriver
{
public:
  ....
  ~XAudio2AudioDriver() override;
  ....
  void Shutdown();
  ....
};

Иерархия аудио драйверов ▼

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

Иерархия аудиосистем ▼

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

Оба производных класса аудио драйверов имеют одинаковый публичный невиртуальный интерфейс Shutdown. Из-за этого и приходится в переопределениях AudioSystem::DestroyDriver производить преобразование к нужному производному классу аудио драйвера и затем вызывать эту функцию.

Можно вынести интерфейс Shutdown в базовый класс в виде чистой виртуальной функции, а AudioSystem::DestroyDriver сделать невиртуальным, убрав из его наследников дублирующийся код.

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

Исправление ▼

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

Оборачивание сырого указателя в std::unique_ptr позволит не переживать за бросок исключения из функции Shutdown: объект под указателем будет удалён оператором delete в любом случае.

Если потребуется, чтобы наследник AudioSystem всё же мог переопределить поведение при удалении аудио драйвера, то можно воспользоваться идиомой NVI (Non-Virtual Interface).

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

Теперь, если наследнику AudioSystem требуется иное поведение при удалении драйвера, достаточно переопределить виртуальную функцию DestroyDriverImpl:

class SomeAudioSystem : public AudioSystem
{
  ....
private:
  void DestroyDriverImpl(AudioDriver* driver) override;
};

Исправление с использованием 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);
}

class SomeAudioSystem : public AudioSystem
{
  ....
private:
  void DestroyDriverImpl(AudioDriver* driver) override;
};

Итоги

Думаю, теперь с проектом я закончила, но путь к совершенству никогда не заканчивается. Мне интересно узнать, что вы думаете об этом фрагменте. Поделитесь своими размышлениями в комментариях :)

А чтобы так же легко находить подозрительные места в коде, предлагаю попробовать пробную версию анализатора PVS-Studio. Давайте вместе делать код более надежным и безопасным!