# Проверка игрового движка qdEngine, часть третья: дополнительная десятка багов В первой статье про qdEngine было рассмотрено 10 ошибок, выбранных плагином PVS\-Studio\. Однако есть ещё 10 багов, заслуживающих внимания\. Как говорится, лучше учиться на чужих ошибках\. Заодно они хорошо демонстрируют возможности PVS\-Studio в выявлении разнообразнейших ошибочных паттернов\. ![1123_qdEngine_check_3_ru/image1.png](https://import.viva64.com/docx/blog/1123_qdEngine_check_3_ru/image1.png) Предыдущие статьи о проверке игрового движка qdEngine: 1. [Топ 10 предупреждений PVS\-Studio](https://pvs-studio.ru/ru/blog/posts/cpp/1119/) 1. [Упрощение C\+\+ кода](https://pvs-studio.ru/ru/blog/posts/cpp/1121/) Без долгих предисловий приступим к рассмотрению ассорти из багов :\) ## Предупреждение N1: ошибка в обработчике ошибок Обработчики ошибочных ситуаций, как правило, не тестируются\. Для них сложно и неинтересно создавать юнит\-тесты\. В режиме ручного тестирования до них сложно добраться \(т\.е\. создать ситуацию, когда в приложении возникнет соответствующая ошибка\)\. В результате обработчики ошибок сами часто содержат ошибки\. Как\-нибудь напишу про это отдельную статью\. Здесь на помощь приходят инструменты статического анализа кода, такие как [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/)\. Анализаторы проверяют код независимо от того, как часто \(с какой вероятностью\) он вызывается в процессе работы приложения\. Поэтому анализаторы могут найти ошибки в редко используемом коде\. Рассмотрим как раз такой случай: ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v510/) The 'printf' function is not expected to receive class\-type variable as third actual argument\. AVIGenerator\.cpp 132 Дело в том, что _m\_sFile_ – это класс типа [_\_bstr\_t_](https://learn.microsoft.com/en-us/cpp/cpp/bstr-t-class?view=msvc-170)\. Попытка распечатать содержимое класса как обыкновенную строчку приведёт к неопределённому поведению\. На практике, скорее всего, вместо имени файла будут распечатаны мусорные символы\. Если мусорных символов будет слишком много, то переполнится буфер _szBuffer\[1024\]_\. Переполнение буфера, в свою очередь, можно рассматривать как потенциальную уязвимость\. В общем, используйте PVS\-Studio для профилактики подобных классических багов в обработчиках ошибок\. Код можно исправить, используя перегруженные операторы класса _\_bstr\_t_: ```cpp // Extract the pointers to the encapsulated Unicode or multibyte BSTR object. operator wchar_t* operator char* ``` Исправленный код: ```cpp _tprintf(szBuffer, _T("AVI Engine failed to initialize. Check filename %s."), (LPCSTR)m_sFile); ``` Тип _LPCSTR_ развернётся в зависимости от режима компиляции \(UNICODE приложение или нет\) в _const wchar\_t\*_ или _const_ _char\*_\. ## Предупреждение N2: недостижимый код ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v779/) Unreachable code detected\. It is possible that an error is present\. qd\_game\_scene\.cpp 1245 Код после оператора _return_ не выполняется\. Блок недостижимого кода точно такой же, как и перед оператором _return_\. Скорее всего, повторяющийся код получился случайно в процессе неаккуратного редактирования кода или слияния веток\. Думаю, нижнюю часть кода можно просто удалить\. ## Предупреждение N3: new \[\] / delete В движке qdEngine очень много ручного управления памятью\. Как следствие, много ошибок, связанных с выделением и освобождением памяти\. Рекомендую тем, кто будет развивать этот движок, особое внимание уделить переработке кода, связанного с работой с памятью\. ```cpp bool grFont::load_index(XStream& fh) { int buf_sz = fh.size(); char* buf = new char[buf_sz]; .... delete buf; return true; } ``` Предупреждение PVS\-Studio: [V611](https://pvs-studio.ru/ru/docs/warnings/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\[\]_\. Правильный код: ```cpp bool grFont::load_index(XStream& fh) { int buf_sz = fh.size(); char* buf = new char[buf_sz]; .... delete[] buf; return true; } ``` Хотя нет\. Это тоже неправильный код\. Правильно — это использовать умные указатели: ```cpp bool grFont::load_index(XStream& fh) { int buf_sz = fh.size(); auto buf = std::make_unique(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: функция не возвращает значение ```cpp class qdSound : public qdNamedObject, public qdResource { .... //! Возвращает длительность звука в секундах. float length() const { #ifndef __QD_SYSLIB__ return sound_.length(); #endif } .... } ``` Предупреждение PVS\-Studio: [V591](https://pvs-studio.ru/ru/docs/warnings/v591/) Non\-void function should return a value\. qd\_sound\.h 70 Если определён макрос _\_\_QD\_SYSLIB\_\__, то функция _length_ будет устроена так: ```cpp float length() const { } ``` Вызов такой функции приводит к неопределённому поведению\. **Примечание\.** Некоторые программисты считают, что в таком случае неопределённое поведение сводится к тому, что функция возвращает случайное значение\. Нет\. Неопределённое поведение — это всё, что угодно\. Посмотрите интересный пример в этой [статье](https://pvs-studio.ru/ru/blog/posts/cpp/0917/)\. Предлагаю исправить код следующим образом: ```cpp #ifndef __QD_SYSLIB__ float length() const { return sound_.length(); } #endif ``` Это приведёт к тому, что часть внешнего кода перестанет компилироваться\. Вот и отлично\. Этот код тоже надо переписать, так как он не имеет смысла, раз вызывает "мёртвую" функцию _length_\. Если такой радикальный подход по каким\-то причинам не подходит, то можно пойти на следующий компромисс: ```cpp #ifndef __QD_SYSLIB__ float length() const { return sound_.length(); } #else [[noreturn]] float length() const { throw std::logic_error { "Function not implemented" }; } #endif ``` ## Предупреждение N5: утечка памяти ```cpp template LineContribType* C2PassScale::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](https://pvs-studio.ru/ru/docs/warnings/v773/) The function was exited without releasing the 'res' pointer\. A memory leak is possible\. 2PassScale\.h 107 Вот эта строчка в коде лишняя: ```cpp LineContribType *res = new LineContribType; ``` Созданный объект нигде не используется и не удаляется\. Единственное, что происходит — утечка памяти\. ## Предупреждение N6: ошибка в логике ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v783/) Dereferencing of the invalid iterator 'it1' might take place\. qd\_condition\_group\.cpp 58 Происходит попытка удаления несуществующего элемента: ```cpp conditions_.erase(conditions_.end()); ``` Скорее всего, в условии допущена логическая ошибка, и на самом деле должно быть написано: ```cpp if (it1 == conditions_.end()) return false; conditions_.erase(it1); ``` ## Предупреждение N7: опасная работа с типом HRESULT ```cpp 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_](https://en.wikipedia.org/wiki/HRESULT): ```cpp HRESULT RenderFile( [in] LPCWSTR lpcwstrFile, [in] LPCWSTR lpcwstrPlayList ); ``` Поэтому проверка является ненадёжной\. Предупреждение PVS\-Studio: [V545](https://pvs-studio.ru/ru/docs/warnings/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_, приведёт к багу\. Правильная проверка: ```cpp if (FAILED(graph_builder_ -> RenderFile(wPath,NULL))) { close_file(); return false; } ``` ## Предупреждение N8: false вместо nullptr Этот фрагмент кода, как и рассмотренный выше, работает\. Но назвать правильным его нельзя\. ```cpp const char* qdInterfaceDispatcher::get_save_title() const { if (!cur_screen_) return false; .... return false; } ``` Предупреждения PVS\-Studio на строчки "return false": * [V601](https://pvs-studio.ru/ru/docs/warnings/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_: ```cpp const char* qdInterfaceDispatcher::get_save_title() const { if (!cur_screen_) return nullptr; .... return nullptr; } ``` ## Предупреждение N9: что\-то не так, но не знаю что Прошу прощения за длинный фрагмент кода\. Я не придумал, как его красиво сократить, не сделав ещё более непонятным\. ```cpp 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_\. Соответственно, этот код не имеет смысла: ```cpp // Если шаг так и не был сделан if (fir_step) return trg; ``` Об этом и предупреждает анализатор PVS\-Studio: [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'fir\_step' is always false\. qd\_game\_object\_moving\.cpp 1724 Строчка с проверкой является лишней, или в циклах имеется логическая ошибка\. К сожалению, я не могу сказать точнее, так как не знаком с кодом проекта\. ## Предупреждение N10: strictly aligned ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v1032/) The pointer 'buf' is cast to a more strictly aligned pointer type\. zip\_container\.cpp 237 В байтовом буфере происходит поиск сигнатуры _0x06054b50L_\. Это сделано непереносимым, опасным образом\. Начнём с того, что непонятно, какая именно сигнатура ищется\. Дело в том, что тип _long_ имеет [разный размер](https://pvs-studio.ru/ru/blog/terms/0012/) на различных платформах\. Чаще всего _long_ бывает 32\-битным и 64\-битным\. В зависимости от размера в памяти будет искаться: * Сигнатура из 4 байт 0x06054b50; * Или сигнатура из 8 байт 0x0000000006054b50\. Вряд ли предполагается, что сигнатура может быть разного размера\. Скорее всего, она всегда 4\-байтовая\. Поэтому правильнее использовать тип фиксированного размера _uint32\_t_\. Однако предупреждение анализатора про иное\. Дело в том, что для поиска сигнатуры используется указатель типа _char\*\._ Чтобы произвести проверку, на каждой итерации адрес указателя \(_char \*_\) интерпретируется как блок из 4/8 байт \(_unsigned long \*_\)\. Картинка для пояснения: ![1123_qdEngine_check_3_ru/image2.png](https://import.viva64.com/docx/blog/1123_qdEngine_check_3_ru/image2.png) Проблема в том, что не учитывается выравнивание\. Значения типа _unsigned long_ должны лежать по адресам, кратным 4 или 8 байтам \(или другому значению, в зависимости от архитектуры\)\. Доступ по невыровненному адресу приводит к неопределённому поведению\. Как оно себя проявит, предсказать невозможно, и зависит это от архитектуры микропроцессора и компилятора\. Наиболее вероятные сценарии: 1. Произойдёт аварийное завершение программы\. 1. Будет прочитано неверное значение\. Например, могут не учитываться два младших бита указателя и чтение всё равно будет выполнятся по границам, кратным 4 байтам\. 1. Чтение данных будет выполняться медленно, так как процессору придётся выполнить дополнительные операции\. 1. Всё работает корректно, по крайней мере пока :\) Исправить проблему можно разными вариантами\. Самый простой вариант — использовать функцию _memcmp_, которая работает на уровне массива байт\. ```cpp if (memcmp(buf + i, &idx_signature, sizeof(idx_signature)) == 0) ``` А можно ли написать код на современном C\+\+, не используя функцию _memcmp_ из мира C? Можно, но, если честно, получается немного длиннее\. Поэтому приведу такой вариант скорее из спортивного интереса: ```cpp 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>(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; } ``` ## Ещё что\-то осталось? Есть ещё одна ошибка, заслуживающая рассмотрения\. Она связанна с объектно\-ориентированным программированием и потребует подробного рассмотрения, поэтому я вынесу её в отдельную, последнюю статью\. Спасибо за внимание\. Подписывайтесь на ежемесячный [дайджест статей](https://pvs-studio.ru/ru/subscribe/), чтобы не пропустить что\-то интересное\.