Вебинар: Использование статических анализаторов кода при разработке безопасного ПО - 19.12
Забудьте о призраках, настоящая угроза кроется в повседневных вещах, таких как static_cast, который может неожиданно лишить вас безопасности, и assert, стремительно исчезающий в релизной сборке. Добро пожаловать в мир ловушек, созданных собственными руками!
В своей недавней статье "Игровое поле экспериментов: какие ошибки могут подстерегать программиста при создании эмулятора" я разбирала срабатывания анализатора PVS-Studio в проекте 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
Этот фрагмент примечателен следующими вещами:
Буквально пять строк функции, а вопросов больше, чем ответов... Тяжело сказать, чего именно пытался добиться разработчик, но я вижу два возможных варианта:
Если постараться сохранить исходный замысел автора, то лучше привести код к следующему виду:
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);
}
Теперь, если наследнику AudioSystem требуется иное поведение при удалении драйвера, достаточно переопределить виртуальную функцию DestroyDriverImpl:
class SomeAudioSystem : public AudioSystem
{
....
private:
void DestroyDriverImpl(AudioDriver* driver) override;
};
Думаю, теперь с проектом я закончила, но путь к совершенству никогда не заканчивается. Мне интересно узнать, что вы думаете об этом фрагменте. Поделитесь своими размышлениями в комментариях :)
А чтобы так же легко находить подозрительные места в коде, предлагаю попробовать пробную версию анализатора PVS-Studio. Давайте вместе делать код более надежным и безопасным!
0