Баги, которые удалось найти в движке qdEngine, оказались весьма разнообразны, поэтому не хочется мешать всё в кучу в одной публикации. Читатели могут упустить интересные темы, связанные с написанием качественного кода. Поэтому разбор проекта выйдет в виде серии публикаций, первая из которых посвящается наиболее интересным срабатываниям с точки зрения плагина PVS-Studio.
Кнопка Best
Ассоциация K-D Lab открыла исходный код игрового движка qdEngine, предназначенного для создания квестов (новость на сайте opennet). Движок в основном написан на языке C++ и выглядит интересным объектом для исследования с помощью анализатора кода PVS-Studio.
И действительно, как можно пройти мимо движка, на основе которого в своё время были созданы такие игры, как:
- Братья Пилоты 3D. Дело об Огородных вредителях;
- Братья Пилоты 3D-2. Тайны Клуба Собаководов;
- Братья Пилоты. Обратная сторона Земли;
- Ну, погоди! Выпуск 3. Песня для зайца;
- Похождения бравого солдата Швейка.
Ошибки, которые удалось найти в проекте, разнообразны, и их хочется рассмотреть с разных точек зрения. Поэтому будет несколько публикаций на разные темы, и первая из них посвящается кнопке Best в плагинах PVS-Studio.
Кнопка помогает пользователю при первом знакомстве с инструментом PVS-Studio. Анализатор выбирает 10 предупреждений, которые, скорее всего, должны указывать на реальные ошибки и при этом быть разнообразными и интересными.
Все статические анализаторы кода при первом запуске выдают большое количество предупреждений, многие из которых, скорее всего, окажутся неактуальными или ложными. Это не страшно, так как анализаторы, в том числе и PVS-Studio, имеют гибкие настройки.
Однако программист, который только знакомится с методологией статического анализа, может быть демотивирован множеством предупреждений, которые для него выглядят неактуальными. Хочется сразу увидеть какие-то интересные ошибки. Кнопка Best как раз позволяет это сделать.
При нажатии на неё анализатор по эмпирическому алгоритму выбирает 10 предупреждений. При выборе он руководствуется не только вероятностью, что перед нами именно ошибка, но и старается выбрать предупреждения разного типа.
Стоит объяснить, почему выбрать 10 предупреждений, которые точно являются ошибкой, не самый лучший вариант.
Для примера рассмотрим диагностику V668. Она сообщает о бессмысленной проверке указателя, который вернул оператор new. Пример реального кода из проекта Minetest:
clouds = new Clouds(smgr, -1, time(0));
if (!clouds) {
*error_message = "Memory allocation error (clouds)";
errorstream << *error_message << std::endl;
return false;
}Если анализатор нашёл подобный код, то перед нами ошибка. Конечно, ещё существует nothrow-версия оператора new, но анализатор это тоже учитывает.
Таких ошибок много. На текущий момент при написании статей мы нашли уже 1630 подобных багов в открытых проектах! С большой вероятностью подобные фрагменты будут найдены и в проекте, который используется для знакомства с анализатором.
Однако выдавать много подобных предупреждений не хочется:
- Это неинтересно. Не хочется смотреть, например, на семь одинаковых предупреждений про new.
- Достоверность не означает критичность. Это как раз такой случай. Перед нами фрагмент недостижимого кода. Это не хорошо, но почти наверняка приложение выполняет свои функции. Интересные и критические ошибки обычно выявляются более сложными диагностическими правилами. Чем сложнее ошибка, которую мы ищем, тем труднее гарантировать, что найден именно баг. Даже человеку приходится поломать голову в таких местах.
Для проекта qdEngine эта кнопка отработала хорошо:
- восемь предупреждений указывают на баги или неудачный код;
- одно предупреждение формально правильно, но реальной ошибки нет;
- одно предупреждение ложное.
На мой взгляд, все эти предупреждения хорошо демонстрируют возможности анализатора, и сейчас мы их рассмотрим. Надеюсь, это подтолкнёт тех, кто только читал про PVS-Studio, опробовать его в деле. Скачивайте, устанавливайте, проверяйте проект и нажимайте кнопку Best. Если что-то пойдёт не так, то просто напишите нам в поддержку. Всегда подскажем и поможем.
Предупреждения
Рассмотрим обещанные предупреждения и заодно кое-где поупражняемся в рефакторинге.
Предупреждение N1: дубликат
bool qdGameObject::init()
{
....
drop_flag(QD_OBJ_STATE_CHANGE_FLAG | QD_OBJ_IS_IN_TRIGGER_FLAG |
QD_OBJ_STATE_CHANGE_FLAG | QD_OBJ_IS_IN_INVENTORY_FLAG);
....
}Предупреждение PVS-Studio: V501 There are identical sub-expressions 'QD_OBJ_STATE_CHANGE_FLAG' to the left and to the right of the '|' operator. qd_game_object.cpp 176
Дважды используется константа QD_OBJ_STATE_CHANGE_FLAG, объявленная в файле qd_game_object.h рядом со многими другими:
....
const int QD_OBJ_HAS_BOUND_FLAG = 0x80;
const int QD_OBJ_DISABLE_MOVEMENT_FLAG = 0x100;
const int QD_OBJ_DISABLE_MOUSE_FLAG = 0x200;
const int QD_OBJ_IS_IN_TRIGGER_FLAG = 0x400;
const int QD_OBJ_STATE_CHANGE_FLAG = 0x800;
const int QD_OBJ_IS_IN_INVENTORY_FLAG = 0x1000;
const int QD_OBJ_KEYBOARD_CONTROL_FLAG = 0x2000;
....Скорее всего, забыли использовать какую-то другую константу для составления маски. Возможно, это просто дубликат, и повторяющуюся константу можно удалить из выражения. Точно мне тут сложно сказать, так как я не знаком с кодом проекта.
Предупреждение N2: опечатка (потерянный символ #)
Очень красивая, на мой взгляд, ошибка. Согласен, у меня странный вкус и профессиональная деформация, но, надеюсь, вы тоже оцените этот баг.
const qdGameObjectStateWalk* qdGameObjectMoving::current_walk_state() const
{
const qdGameObjectState* st = get_cur_state();
if(!st || st -> state_type() != qdGameObjectState::STATE_WALK){
#ifndef _QUEST_EDITOR
st = last_walk_state_;
if(!st || st -> state_type() != qdGameObjectState::STATE_WALK)
st = get_default_state();
else
st = get_default_state();
if(!st) st = get_state(0);
#endif
}
....
}Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. qd_game_object_moving.cpp 2781
Анализатор обращает внимание, что, независимо от условия, будет выполнено одно и то же действие:
if(!st || st -> state_type() != qdGameObjectState::STATE_WALK)
st = get_default_state();
else
st = get_default_state();Обратите внимание, что код как-то странно отформатирован. Я имею в виду, что ключевое слово else находится в начале строки.
Если посмотреть на код рядом и немного подумать, то становится ясно, что на самом деле хотели написать не оператор else, а директиву препроцессора #else. Однако программист опечатался и забыл символ решётки (#).
Исправленный код:
const qdGameObjectState* st = get_cur_state();
if(!st || st -> state_type() != qdGameObjectState::STATE_WALK){
#ifndef _QUEST_EDITOR
st = last_walk_state_;
if(!st || st -> state_type() != qdGameObjectState::STATE_WALK)
st = get_default_state();
#else
st = get_default_state();
if(!st) st = get_state(0);
#endifПредупреждение N3: неиспользуемая строка
bool qdGameScene::adjust_files_paths(....)
{
....
for(qdGameObjectList::const_iterator it = object_list().begin();
it != object_list().end(); ++it)
{
if((*it) -> named_object_type() == QD_NAMED_OBJECT_STATIC_OBJ)
{
qdGameObjectStatic* obj = static_cast<qdGameObjectStatic*>(*it);
if(obj -> get_sprite() -> file())
QD_ADJUST_TO_REL_FILE_MEMBER(pack_corr_dir,
obj -> get_sprite() -> file,
obj -> get_sprite() -> set_file,
can_overwrite,
all_ok);
std::string str = obj -> get_sprite() -> file();
str = str;
}
}
....
}Предупреждение PVS-Studio: V570 The 'str' variable is assigned to itself. qd_game_scene.cpp 1799
В цикле происходит какая-то работа со списком файлов. Как я понимаю, в целях отладки был добавлен этот фрагмент кода, который и не нравится анализатору:
std::string str = obj -> get_sprite() -> file();
str = str;В первой строчке берётся имя файла. Вторая строчка, на мой взгляд, служит для двух целей:
- На неё удобно поставить точку останова в отладчике и сразу посмотреть имя файла в переменной str.
- Присваивание объекта самому себе может дополнительно убирать предупреждение какого-то компилятора/анализатора о том, что переменная str не используется.
Ошибки здесь нет. Однако это код с запахом, и его стоит улучшить. Другими словами, это предупреждение анализатора — хороший повод заняться рефакторингом. Для начала я бы изменил интерфейс функции file в классе qdSprite:
const char* file() const { return file_.c_str(); }Странно возвращать содержимое std::string как просто указатель, а затем ещё и проверять его в разных местах на nullptr. На самом деле, функция никогда не вернёт nullptr. Если же где-то в коде нужен именно указатель, то всегда можно будет в этом месте вызвать c_str. Изменённая функция:
const std::string& file() const { return file_; }Естественно, изменение интерфейса функции затронет много мест в коде проекта. Тем более, что для симметричности есть смысл изменить и функцию set_file. Однако у меня есть подозрение, что всё это пошло бы коду на пользу.
Продолжим. Перепишем рассмотренный ранее фрагмент кода.
if((*it) -> named_object_type() == QD_NAMED_OBJECT_STATIC_OBJ)
{
qdGameObjectStatic* obj = static_cast<qdGameObjectStatic*>(*it);
const std::string &file = obj -> get_sprite() -> file();
QD_ADJUST_TO_REL_FILE_MEMBER(pack_corr_dir,
file,
obj -> get_sprite() -> set_file,
can_overwrite,
all_ok);
}Улучшения:
- Код стал короче и проще.
- Исчезла бессмысленная проверка указателя.
- Нет создания временного объекта std::string в цикле только для того, чтобы было проще отлаживаться. Это относительно ресурсоёмкие операции из-за выделения и освобождения памяти. При этом нет гарантии, что компилятор в процессе оптимизации удалит всё лишнее.
- Анализатор PVS-Studio больше не выдаёт предупреждение V570.
- В целях отладки всё так же легко поставить точку останова (на строчку с макросом QD_ADJUST_TO_REL_FILE_MEMBER) и посмотреть имя файла.
Предупреждение N4: потенциальное использование нулевого указателя
bool DDraw_grDispatcher::Finit()
{
....
ddobj_ -> SetCooperativeLevel((HWND)Get_hWnd(),DDSCL_NORMAL);
if(fullscreen_ && ddobj_) ddobj_ -> RestoreDisplayMode();
....
}Предупреждение PVS-Studio: V595 The 'ddobj_' pointer was utilized before it was verified against nullptr. Check lines: 211, 212. ddraw_gr_dispatcher.cpp 211
Указатель ddobj разыменовывается для вызова функции. При этом ниже есть проверка этого указателя. На основании этой проверки анализатор делает вывод, что переменная ddobj может содержать nullptr, о чём и предупреждает нас.
Код можно переписать, например так:
if (ddobj_)
{
ddobj_ -> SetCooperativeLevel((HWND)Get_hWnd(),DDSCL_NORMAL);
if(fullscreen_) ddobj_ -> RestoreDisplayMode();
}Предупреждение N5: деструктор не является виртуальным
template <class T>
class PtrHandle
{
....
~PtrHandle() { delete ptr; }
....
private:
T *ptr;
};Предупреждение PVS-Studio: V599 Instantiation of PtrHandle < ResourceUser >: The virtual destructor is not present, although the 'ResourceUser' class contains virtual functions. Handle.h 14
В коде класса PtrHandle ошибки нет. Однако проблема в том, что в нём хранится объект типа ResourceUser. Посмотрим на объявление этого класса:
class ResourceUser
{
int ID;
static int IDs;
public:
ResourceUser(time_type period) { dtime = period; time = 0; ID = ++IDs; }
virtual int quant() { return 1; }
protected:
time_type time;
time_type dtime;
virtual void init_time(time_type time_) { time = time_ + time_step(); }
virtual time_type time_step() { return dtime; }
friend class ResourceDispatcher;
};В этом классе есть виртуальные функции. Следовательно, предполагается, что от этого класса будут наследоваться другие классы. А раз так, то и деструктор следует объявить как виртуальный. В противном случае деструктор класса PtrHandle будет не полностью разрушать объекты, которые наследуются от ResourceUser. Это приводит к неопределённому поведению и утечкам ресурсов.
Предупреждение N6: неверный оператор delete
char* grDispatcher::temp_buffer(int size)
{
if(size <= 0) size = 1;
if(size > temp_buffer_size_){
delete temp_buffer_;
temp_buffer_ = new char[size];
temp_buffer_size_ = size;
}
return temp_buffer_;
}Предупреждение PVS-Studio: V611 The memory was allocated using the 'operator new[]' but was released using the 'operator delete'. The 'delete[] temp_buffer_;' statement should be used instead. Check lines: 1241, 1242. gr_dispatcher.cpp 1241
В переменной temp_buffer_ хранятся указатели на массивы, созданные с помощью оператора new[]. Следовательно, уничтожаться они должны с помощью оператора delete[].
В рассматриваемом коде для уничтожения массива используется просто delete, предназначенный для одиночных объектов. В результате этот код приводит к неопределённому поведению.
Примечание. Кто-то может возразить, что на практике всё будет работать, так как массив состоит из элементов типа char, и работа операторов new/delete сводится к вызову функций malloc/free. Это не так. Реализация операторов может быть весьма разнообразной. Например, с целью оптимизации одиночные объекты и массивы могут создаваться в различных заранее выделенных (зарезервированных) пулах памяти.
Предупреждение N7: ошибочный тип переменной для синхронизации
static int b_thread_must_stop=0;
void MpegDeinitLibrary()
{
....
if (hThread!=INVALID_HANDLE_VALUE)
{
b_thread_must_stop=1;
while(b_thread_must_stop==1)
Sleep(10);
}
....
}Предупреждение PVS-Studio: V712 Be advised that compiler may delete this cycle or make it infinity. Use volatile variable(s) or synchronization primitives to avoid this. PlayOgg.cpp 293
Для синхронизации потоков используется обыкновенная глобальная переменная. Это плохая идея, так как компилятор в процессе оптимизации может сделать вывод, что она не изменяется, и создаст вечный цикл. Вдобавок изменение переменной может быть не атомарным.
В таком случае следует заменить тип int на atomic_int:
#include <atomic>
static atomic_int b_thread_must_stop { 0 };Предупреждение N8: утечка памяти
bool qdAnimationMaker::insert_frame(....)
{
// IMPORTANT(pabdulin): auto_ptr usage was removed
qdAnimationFrame* fp = new qdAnimationFrame;
fp -> set_file(fname);
fp -> set_length(default_frame_length_);
if (!fp -> load_resources())
return false;
....
delete fp;
return true;
}Предупреждение PVS-Studio: V773 The function was exited without releasing the 'fp' pointer. A memory leak is possible. qd_animation_maker.cpp 40
Если произойдёт ошибка загрузки ресурсов, то функция досрочно завершит работу. При этом не будет удалён объект, указатель на который хранится в переменной fp. Произойдёт утечка памяти.
Правильный вариант:
if (!fp -> load_resources())
{
delete fp;
return false;
}Впрочем, код даже после исправления ошибки нельзя назвать хорошим. Ручное управление памятью провоцирует ошибки, что мы здесь и наблюдаем. Намного лучше использовать умные указатели.
Интересный момент. Обратите внимание на комментарий:
// IMPORTANT(pabdulin): auto_ptr usage was removedВозможно, когда-то этот код был правильным, пока в нём использовался умный указатель типа std::auto_ptr. Затем этот класс устарел. Вместо того, чтобы заменить его на std::unique_ptr, программист решил работать с указателями "в ручном режиме", но не справился с задачей.
Предупреждение N9: ложное срабатывание
class PtrHandle
{
....
PtrHandle& operator=(PtrHandle& p)
{
if (get() != p.get())
{
delete ptr;
ptr = p.release();
}
return *this;
}
....
T* get() const { return ptr; }
....
private:
T *ptr;
};Предупреждение PVS-Studio: V794 The assignment operator should be protected from the case of 'this == &p'. Handle.h 19
Это единственное из 10 предупреждений, выбранных анализатором, которое оказалось ложным. Диагностика V794 предупреждает, что объект должен быть защищён от копирования в самого себя. Для этого анализатор ищет в теле функции конструкцию вида:
if (this != &p)Но здесь написана более сложная проверка через вызов функций get:
if (get() != p.get())Возможно, автор хотел защититься не просто от копирования умного указателя в самого себя, а от копирования двух разных умных указателей, ссылающихся на один объект. Впрочем, в этом нет смысла. Если два умных указателя (без подсчёта ссылок) ссылаются на один объект, то это беда. Возможна ситуация двойного уничтожения объекта.
Так что такая проверка на самом деле ничем не лучше. Она лишь дополнительно запутывает анализатор и людей, читающих код.
Что ж, анализатор не разобрался в ситуации. Чтобы устранить ложное предупреждение, можно поступить одним из следующих способов.
Вариант 1. Добавить комментарий для подавления предупреждения.
PtrHandle& operator=(PtrHandle& p)
{ //-V794
if (get() != p.get())Комментарий явно указывает анализатору PVS-Studio, что код безопасен.
Вариант 2. Переписать проверку.
if (this != &p)Мне, пожалуй, больше нравится этот способ. Вариант с вызовом функций, на мой взгляд, смотрится тяжеловесно.
Если хочется больше контроля, то можно добавить вот такую проверку:
PtrHandle& operator=(PtrHandle& p)
{
if (this != &p)
{
if (get() == p.get())
{
// Всё плохо. Один объект в двух умных указателях.
// Время отлаживаться.
assert(false);
throw std::logic_error("Multiple ownership of an object.");
}
delete ptr;
ptr = p.release();
}
return *this;
}Проверка поможет выявить ситуацию, когда два умных указателя вдруг будут контролировать один и тот же объект.
Предупреждение N10: повторное присваивание
bool qdGameObjectMoving::update_screen_pos()
{
....
if(get_cur_state() -> state_type() == qdGameObjectState::STATE_WALK){
qdGameObjectStateWalk::OffsetType offs_type=
qdGameObjectStateWalk::OFFSET_WALK; // <=
switch(movement_mode_){
case MOVEMENT_MODE_STOP:
offs_type = qdGameObjectStateWalk::OFFSET_STATIC;
break;
case MOVEMENT_MODE_TURN:
offs_type = qdGameObjectStateWalk::OFFSET_STATIC;
break;
case MOVEMENT_MODE_START:
offs_type = qdGameObjectStateWalk::OFFSET_START;
break;
case MOVEMENT_MODE_MOVE:
offs_type = qdGameObjectStateWalk::OFFSET_WALK; // <=
break;
case MOVEMENT_MODE_END:
offs_type = qdGameObjectStateWalk::OFFSET_END;
break;
}
offs += static_cast<qdGameObjectStateWalk*>(
get_cur_state()) -> center_offset(direction_angle_, offs_type);
}
....
}Предупреждение PVS-Studio: V1048 The 'offs_type' variable was assigned the same value. qd_game_object_moving.cpp 1094
Анализатору не нравится, что в ветке MOVEMENT_MODE_MOVE переменной offs_type присваивается значение, которое и так в ней находится.
Формально анализатор прав. Повторное присваивание не имеет смысла и иногда является не просто лишним кодом, а ошибкой.
В этом случае бага нет. Рассмотрим, как можно избавиться от предупреждения.
Вариант 1. Добавить комментарий //-V1048 для подавления предупреждения.
Вариант 2. Немного переписать код, добавив default-ветку:
if(get_cur_state() -> state_type() == qdGameObjectState::STATE_WALK){
qdGameObjectStateWalk::OffsetType offs_type;
switch(movement_mode_){
case MOVEMENT_MODE_STOP:
offs_type = qdGameObjectStateWalk::OFFSET_STATIC;
break;
case MOVEMENT_MODE_TURN:
offs_type = qdGameObjectStateWalk::OFFSET_STATIC;
break;
case MOVEMENT_MODE_START:
offs_type = qdGameObjectStateWalk::OFFSET_START;
break;
case MOVEMENT_MODE_MOVE:
offs_type = qdGameObjectStateWalk::OFFSET_WALK;
break;
case MOVEMENT_MODE_END:
offs_type = qdGameObjectStateWalk::OFFSET_END;
break;
default:
offs_type = qdGameObjectStateWalk::OFFSET_WALK;
}
offs += static_cast<qdGameObjectStateWalk*>(
get_cur_state()) -> center_offset(direction_angle_, offs_type);
}Предупреждение исчезнет, но мне не нравится, что переменная offs_type теперь не инициализируется в месте объявления. Сделаем ещё один шаг рефакторинга и вынесем часть кода в отдельную функцию.
qdGameObjectStateWalk::OffsetType qdGameObjectMoving::GetOffsetType()
{
switch(movement_mode_){
case MOVEMENT_MODE_STOP: return qdGameObjectStateWalk::OFFSET_STATIC;
case MOVEMENT_MODE_TURN: return qdGameObjectStateWalk::OFFSET_STATIC;
case MOVEMENT_MODE_START: return qdGameObjectStateWalk::OFFSET_START;
case MOVEMENT_MODE_MOVE: return qdGameObjectStateWalk::OFFSET_WALK;
case MOVEMENT_MODE_END: return qdGameObjectStateWalk::OFFSET_END;
}
return qdGameObjectStateWalk::OFFSET_WALK;
}
....
if(get_cur_state() -> state_type() == qdGameObjectState::STATE_WALK){
qdGameObjectStateWalk::OffsetType offs_type = GetOffsetType();
offs += static_cast<qdGameObjectStateWalk*>(
get_cur_state()) -> center_offset(direction_angle_, offs_type);}
....Код стал короче и при этом легко читается. Отлично. Более того, теперь переменная offs_type вообще не нужна, и код можно ещё немного сократить.
if(get_cur_state() -> state_type() == qdGameObjectState::STATE_WALK){
offs += static_cast<qdGameObjectStateWalk*>(
get_cur_state()) -> center_offset(direction_angle_, GetOffsetType());
}Хороший пример: предупреждение анализатора подтолкнуло нас к рефакторингу кода, благодаря чему он стал проще и лучше.
Заключение
В статье было показано:
- Как может помочь кнопка Best при первом знакомстве с анализатором PVS-Studio. Предлагаю, не откладывая, получить триал и попробовать проанализировать свои проекты.
- Какие ошибки PVS-Studio помогает найти и устранить ещё на этапе написания кода. Чем раньше ошибка найдена, тем проще и дешевле её исправление.
- Как предупреждения анализатора подталкивают к улучшению кода (рефакторингу).
В последующих статьях о qdEngine я разберу ещё ряд ошибок, найденных в проекте. Спасибо за внимание.
Дополнительные ссылки
- Развитие инструментария С++ программистов: статические анализаторы кода.
- Предупреждения помогают писать лаконичный код.
- Статический анализатор подталкивает писать чистый код.
- Вызов виртуальных функций в конструкторах и деструкторах (C++).
- Как внедрить статический анализатор кода в legacy проект и не демотивировать команду.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Let's check the qdEngine game engine, part one: top 10 warnings issued by PVS-Studio.
