В первой статье про qdEngine было рассмотрено 10 ошибок, выбранных плагином PVS-Studio. Однако есть ещё 10 багов, заслуживающих внимания. Как говорится, лучше учиться на чужих ошибках. Заодно они хорошо демонстрируют возможности PVS-Studio в выявлении разнообразнейших ошибочных паттернов.
Предыдущие статьи о проверке игрового движка qdEngine:
Без долгих предисловий приступим к рассмотрению ассорти из багов :)
Предупреждение 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 байтам (или другому значению, в зависимости от архитектуры).
Доступ по невыровненному адресу приводит к неопределённому поведению. Как оно себя проявит, предсказать невозможно, и зависит это от архитектуры микропроцессора и компилятора. Наиболее вероятные сценарии:
- Произойдёт аварийное завершение программы.
- Будет прочитано неверное значение. Например, могут не учитываться два младших бита указателя и чтение всё равно будет выполнятся по границам, кратным 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.
