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

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

Введение

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

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

А вот и он

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

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

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

1. Класс XAudio2AudioSystem является наследником AudioDriver. Значит, в функцию XAudio2AudioSystem::DestroyDriver прилетает указатель на базовый тип (driver).

2. Макрос assert_not_null проверяет его состояние. Он разворачивается в xenia_assert, а тот в свою очередь в стандартный assert. Да, этот макрос удаляется под релизом, но мы опустим этот момент. В дебаге он помогает нам узнать, что указатель не нулевой.

3. Далее указатель driver преобразуется к указателю на производный класс (xdriver) через static_cast. При таком способе не происходит проверка, какой именно объект на самом деле лежит под этим указателем. Компилятор всего лишь проверяет, валидно ли такое преобразование согласно стандарту, и оно валидно в этом контексте. При этом результирующий указатель также ненулевой, но не факт, что корректный.

4. Указатель xdriver разыменовывается, и вызывается нестатическая функция-член XAudio2AudioSystem::Shutdown. Если динамический тип объекта под этим указателем отличен от XAudio2AudioSystem или его наследников, то поведение будет не определено (нарушаются правила strict aliasing).

5. После этого разработчик задумался, а не нулевой ли случаем этот указатель, и добавил проверку указателя 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 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(); } 

Оборачивание сырого указателя в 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); } 

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

Итоги

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

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

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Aleksandra Uvarova. 5 lines of fortune: what program keeps under wraps.