В первой статье про 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.
