Проверка игрового движка qdEngine, часть третья: дополнительная десятка багов

В первой статье про qdEngine было рассмотрено 10 ошибок, выбранных плагином PVS-Studio. Однако есть ещё 10 багов, заслуживающих внимания. Как говорится, лучше учиться на чужих ошибках. Заодно они хорошо демонстрируют возможности PVS-Studio в выявлении разнообразнейших ошибочных паттернов.

Предыдущие статьи о проверке игрового движка qdEngine:

1. Топ 10 предупреждений PVS-Studio
2. Упрощение C++ кода

Без долгих предисловий приступим к рассмотрению ассорти из багов 🙂

Предупреждение N1: ошибка в обработчике ошибок

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

В результате обработчики ошибок сами часто содержат ошибки. Как-нибудь напишу про это отдельную статью.

Здесь на помощь приходят инструменты статического анализа кода, такие как PVS-Studio. Анализаторы проверяют код независимо от того, как часто (с какой вероятностью) он вызывается в процессе работы приложения. Поэтому анализаторы могут найти ошибки в редко используемом коде. Рассмотрим как раз такой случай:

class CAVIGenerator   {   ....   _bstr_t m_sFile;   .... };  HRESULT CAVIGenerator::InitEngine() {   ....   TCHAR szBuffer[1024];   ....   if (hr != AVIERR_OK)   {     _tprintf(szBuffer,              _T("AVI Engine failed to initialize. Check filename %s."),m_sFile);     m_sError=szBuffer;   .... };

Предупреждение PVS-Studio: V510 The ‘printf’ function is not expected to receive class-type variable as third actual argument. AVIGenerator.cpp 132

Дело в том, что m_sFile – это класс типа _bstr_t. Попытка распечатать содержимое класса как обыкновенную строчку приведёт к неопределённому поведению. На практике, скорее всего, вместо имени файла будут распечатаны мусорные символы. Если мусорных символов будет слишком много, то переполнится буфер szBuffer[1024]. Переполнение буфера, в свою очередь, можно рассматривать как потенциальную уязвимость.

В общем, используйте PVS-Studio для профилактики подобных классических багов в обработчиках ошибок.

Код можно исправить, используя перегруженные операторы класса _bstr_t:

// Extract the pointers to the encapsulated Unicode or multibyte BSTR object. operator wchar_t* operator char*

Исправленный код:

_tprintf(szBuffer,          _T("AVI Engine failed to initialize. Check filename %s."),          (LPCSTR)m_sFile);

Тип LPCSTR развернётся в зависимости от режима компиляции (UNICODE приложение или нет) в const wchar_t* или const char*.

Предупреждение N2: недостижимый код

bool qdGameScene::follow_path_seek(qdGameObjectMoving* pObj,                                    bool lock_target) {   if (qdGameObjectMoving::FOLLOW_UPDATE_PATH == pObj->follow_condition())     selected_object_->set_grid_zone_attributes(sGridCell::CELL_SELECTED);    return pObj->move(selected_object_->last_move_order(), lock_target);    if (qdGameObjectMoving::FOLLOW_UPDATE_PATH == pObj->follow_condition())     selected_object_->drop_grid_zone_attributes(sGridCell::CELL_SELECTED); }

Предупреждение PVS-Studio: V779 Unreachable code detected. It is possible that an error is present. qd_game_scene.cpp 1245

Код после оператора return не выполняется.

Блок недостижимого кода точно такой же, как и перед оператором return. Скорее всего, повторяющийся код получился случайно в процессе неаккуратного редактирования кода или слияния веток. Думаю, нижнюю часть кода можно просто удалить.

Предупреждение N3: new [] / delete

В движке qdEngine очень много ручного управления памятью. Как следствие, много ошибок, связанных с выделением и освобождением памяти. Рекомендую тем, кто будет развивать этот движок, особое внимание уделить переработке кода, связанного с работой с памятью.

bool grFont::load_index(XStream& fh) {   int buf_sz = fh.size();   char* buf = new char[buf_sz];   ....   delete buf;   return true; }

Предупреждение PVS-Studio: V611 The memory was allocated using the ‘operator new[]’ but was released using the ‘operator delete’. The ‘delete[] buf;’ statement should be used instead. gr_font.cpp 72

Выделяется массив, поэтому для освобождения должен использоваться оператор delete[]. Правильный код:

bool grFont::load_index(XStream& fh) {   int buf_sz = fh.size();   char* buf = new char[buf_sz];   ....   delete[] buf;   return true; }

Хотя нет. Это тоже неправильный код. Правильно — это использовать умные указатели:

bool grFont::load_index(XStream& fh) {   int buf_sz = fh.size();   auto buf = std::make_unique<char[]>(buf_sz);   ....   return true; }

P.S.: Ещё несколько мест с точно такой же ошибкой:

• V611 The memory was allocated using the ‘operator new[]’ but was released using the ‘operator delete’. The ‘delete[] buf;’ statement should be used instead. gr_font.cpp 101
• V611 The memory was allocated using the ‘operator new[]’ but was released using the ‘operator delete’. The ‘delete[] alpha_buffer_;’ statement should be used instead. Check lines: 25, 168. gr_font.cpp 25
• V611 The memory was allocated using the ‘operator new[]’ but was released using the ‘operator delete’. The ‘delete[] buffer;’ statement should be used instead. qda_editor.cpp 1012

Предупреждение N4: функция не возвращает значение

class qdSound : public qdNamedObject, public qdResource {   ....   //! Возвращает длительность звука в секундах.   float length() const {  #ifndef __QD_SYSLIB__     return sound_.length();  #endif   }   .... }

Предупреждение PVS-Studio: V591 Non-void function should return a value. qd_sound.h 70

Если определён макрос __QD_SYSLIB__, то функция length будет устроена так:

float length() const {  }

Вызов такой функции приводит к неопределённому поведению.

Примечание. Некоторые программисты считают, что в таком случае неопределённое поведение сводится к тому, что функция возвращает случайное значение. Нет. Неопределённое поведение — это всё, что угодно. Посмотрите интересный пример в этой статье.

Предлагаю исправить код следующим образом:

#ifndef __QD_SYSLIB__   float length() const {      return sound_.length();    } #endif

Это приведёт к тому, что часть внешнего кода перестанет компилироваться. Вот и отлично. Этот код тоже надо переписать, так как он не имеет смысла, раз вызывает «мёртвую» функцию length.

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

#ifndef __QD_SYSLIB__   float length() const {      return sound_.length();    } #else   [[noreturn]] float length() const {      throw std::logic_error { "Function not implemented" };   } #endif

Предупреждение N5: утечка памяти

template<class FilterClass> LineContribType* C2PassScale<FilterClass>::AllocContributions(UINT uLineLength,                                                                UINT uWindowSize) {   static LineContribType line_ct;    LineContribType *res = new LineContribType;     line_ct.WindowSize = uWindowSize;    line_ct.LineLength = uLineLength;     if (contribution_buffer_.size() < uLineLength)     contribution_buffer_.resize(uLineLength);    line_ct.ContribRow = &*contribution_buffer_.begin();    if (weights_buffer_.size() < uLineLength * uWindowSize)     weights_buffer_.resize(uLineLength * uWindowSize);    double* p = &*weights_buffer_.begin();    for (UINT u = 0; u < uLineLength; u++) {     line_ct.ContribRow[u].Weights = p;     p += uWindowSize;   }   return &line_ct; }

Предупреждение PVS-Studio: V773 The function was exited without releasing the ‘res’ pointer. A memory leak is possible. 2PassScale.h 107

Вот эта строчка в коде лишняя:

LineContribType *res = new LineContribType;

Созданный объект нигде не используется и не удаляется. Единственное, что происходит — утечка памяти.

Предупреждение N6: ошибка в логике

bool qdConditionGroup::remove_condition(int condition_id) {   ....   conditions_container_t::iterator it1 = std::find(conditions_.begin(),                                                    conditions_.end(),                                                    condition_id);   if (it1 != conditions_.end())     return false;    conditions_.erase(it1);   return true; }

Предупреждение PVS-Studio: V783 Dereferencing of the invalid iterator ‘it1’ might take place. qd_condition_group.cpp 58

Происходит попытка удаления несуществующего элемента:

conditions_.erase(conditions_.end());

Скорее всего, в условии допущена логическая ошибка, и на самом деле должно быть написано:

if (it1 == conditions_.end())   return false;  conditions_.erase(it1);

Предупреждение N7: опасная работа с типом HRESULT

class winVideo {   ....   struct IGraphBuilder*  graph_builder_;   .... };  bool winVideo::open_file(const char *fname) {   ....   if (graph_builder_ -> RenderFile(wPath,NULL))   {     close_file(); return false;   }   .... }

На первый взгляд кажется, что с кодом всё хорошо. Но дело в том, что функция RenderFile возвращает тип HRESULT:

HRESULT RenderFile(   [in] LPCWSTR lpcwstrFile,   [in] LPCWSTR lpcwstrPlayList );

Поэтому проверка является ненадёжной.

Предупреждение PVS-Studio: V545 Such conditional expression of ‘if’ statement is incorrect for the HRESULT type value ‘graph_builder_->RenderFile(wPath, 0)’. The SUCCEEDED or FAILED macro should be used instead. WinVideo.cpp 139

HRESULT — это 32-разрядное значение, разделённое на три различных поля: код серьёзности ошибки, код устройства и код ошибки. Для проверки значений типа HRESULT предназначены такие макросы, как SUCCEEDED, FAILED.

Тип HRESULT имеет множество состояний. Это может быть 0L (S_OK), 0x80000002L (Ran out of memory), 0x80004005L (unspecified failure) и т. д. Обратите внимание, что состояние S_OK кодируется как 0.

Соответственно, любой статус, кроме S_OK, считается ошибочным, и функция досрочно завершает работу. В целом, видимо, этот код сейчас работает правильно, но написан небезопасно. Небольшой рефакторинг в случае, если кто-то подумает, что функция RenderFile возвращает тип bool, приведёт к багу.

Правильная проверка:

if (FAILED(graph_builder_ -> RenderFile(wPath,NULL))) {   close_file(); return false; }

Предупреждение N8: false вместо nullptr

Этот фрагмент кода, как и рассмотренный выше, работает. Но назвать правильным его нельзя.

const char* qdInterfaceDispatcher::get_save_title() const {   if (!cur_screen_)     return false;   ....   return false; }

Предупреждения PVS-Studio на строчки «return false»:

• V601 The ‘false’ value is implicitly cast to the pointer. qd_interface_dispatcher.cpp 772
• V601 The ‘false’ value is implicitly cast to the pointer. qd_interface_dispatcher.cpp 783

Значение false неявно преобразуется к нулевому указателю, и поэтому код работает, как и задумано. Тем не менее хотя бы во имя красоты и приличия надо использовать nullptr:

const char* qdInterfaceDispatcher::get_save_title() const {   if (!cur_screen_)     return nullptr;   ....   return nullptr; }

Предупреждение N9: что-то не так, но не знаю что

Прошу прощения за длинный фрагмент кода. Я не придумал, как его красиво сократить, не сделав ещё более непонятным.

Vect2s qdGameObjectMoving::get_nearest_walkable_point(const Vect2s& target) const {   ....   // Идем с конца. Если натыкаемся на проходимую точку, отличную от начальной   bool fir_step = true;   if (abs(x2 - x1) > abs(y2 - y1)) {     int dx = int(float(x2 - x1)/dr.x);     do {       if (false == is_walkable(Vect2s(r.xi(),r.yi())))       {         // Если только первый шаг, то неудача         if (fir_step) return Vect2s(-1, -1);         r -= dr;         return Vect2s(r.xi(),r.yi());       }        fir_step = false;       r += dr;     } while (--dx >= 0);   }   else {     int dy = int(float(y2 - y1)/dr.y);     do {       if (false == is_walkable(Vect2s(r.xi(),r.yi())))       {         if (fir_step) return Vect2s(-1, -1);         r -= dr;         return Vect2s(r.xi(),r.yi());       }        fir_step = false;       r += dr;     } while (--dy >= 0);   }    // Если шаг так и не был сделан   if (fir_step) return trg;   .... }

Прошу изучить циклы. Если они не заканчиваются выходом из функции, то переменная fir_step по завершению циклов всегда равна false.

Соответственно, этот код не имеет смысла:

// Если шаг так и не был сделан if (fir_step) return trg;

Об этом и предупреждает анализатор PVS-Studio: V547 Expression ‘fir_step’ is always false. qd_game_object_moving.cpp 1724

Строчка с проверкой является лишней, или в циклах имеется логическая ошибка. К сожалению, я не могу сказать точнее, так как не знаком с кодом проекта.

Предупреждение N10: strictly aligned

bool zipContainer::find_index() {   const int buf_sz = 1024;   char buf[buf_sz];    stream_.seek(0,XS_END);    while (stream_.tell() >= buf_sz) {     stream_.seek(-buf_sz,XS_CUR);     stream_.read(buf,buf_sz);      const unsigned long idx_signature = 0x06054b50L;      for (int i = 0; i < buf_sz - sizeof(long) * 3; i++) {       if (*((unsigned long*)(buf + i)) == idx_signature) {         XBuffer xbuf(buf + i + sizeof(long) + sizeof(unsigned short) * 4,                      sizeof(long) * 2);         xbuf > index_size_ > index_offset_;         return true;       }     }   }    return false; }

Предупреждение PVS-Studio: V1032 The pointer ‘buf’ is cast to a more strictly aligned pointer type. zip_container.cpp 237

В байтовом буфере происходит поиск сигнатуры 0x06054b50L. Это сделано непереносимым, опасным образом.

Начнём с того, что непонятно, какая именно сигнатура ищется. Дело в том, что тип long имеет разный размер на различных платформах. Чаще всего long бывает 32-битным и 64-битным. В зависимости от размера в памяти будет искаться:

• Сигнатура из 4 байт 0x06054b50;
• Или сигнатура из 8 байт 0x0000000006054b50.

Вряд ли предполагается, что сигнатура может быть разного размера. Скорее всего, она всегда 4-байтовая. Поэтому правильнее использовать тип фиксированного размера uint32_t.

Однако предупреждение анализатора про иное. Дело в том, что для поиска сигнатуры используется указатель типа char*. Чтобы произвести проверку, на каждой итерации адрес указателя (char *) интерпретируется как блок из 4/8 байт (unsigned long *). Картинка для пояснения:

Проблема в том, что не учитывается выравнивание. Значения типа unsigned long должны лежать по адресам, кратным 4 или 8 байтам (или другому значению, в зависимости от архитектуры).

Доступ по невыровненному адресу приводит к неопределённому поведению. Как оно себя проявит, предсказать невозможно, и зависит это от архитектуры микропроцессора и компилятора. Наиболее вероятные сценарии:

1. Произойдёт аварийное завершение программы.
2. Будет прочитано неверное значение. Например, могут не учитываться два младших бита указателя и чтение всё равно будет выполнятся по границам, кратным 4 байтам.
3. Чтение данных будет выполняться медленно, так как процессору придётся выполнить дополнительные операции.
4. Всё работает корректно, по крайней мере пока 🙂

Исправить проблему можно разными вариантами. Самый простой вариант — использовать функцию memcmp, которая работает на уровне массива байт.

if (memcmp(buf + i, &idx_signature, sizeof(idx_signature)) == 0)

А можно ли написать код на современном C++, не используя функцию memcmp из мира C?

Можно, но, если честно, получается немного длиннее. Поэтому приведу такой вариант скорее из спортивного интереса:

const uint32_t idx_sig = 0x06054b50L;  const std::ranges::subrange   haystack { buf, buf + buf_sz - sizeof(idx_sig) * 3 }; const auto needle = std::bit_cast<std::array<char, sizeof(idx_sig)>>(idx_sig);  if (auto res = std::ranges::search(haystack, needle); !std::empty(res)) {   XBuffer xbuf(it + sizeof(uint32_t) + sizeof(uint16_t) * 4,                sizeof(uint32_t) * 2);   xbuf > index_size_ > index_offset_;   return true; }

Ещё что-то осталось?

Есть ещё одна ошибка, заслуживающая рассмотрения. Она связанна с объектно-ориентированным программированием и потребует подробного рассмотрения, поэтому я вынесу её в отдельную, последнюю статью.

Спасибо за внимание. Подписывайтесь на ежемесячный дайджест статей, чтобы не пропустить что-то интересное.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Let’s check the qdEngine game engine, part three: 10 more bugs.