В мире антропоморфных животных

Мы не смогли пройти мимо открытого исходного кода Overgrowth и проверили его качество с помощью PVS-Studio. Давайте же вместе посмотрим, где больше интересного экшена: в игре или в её исходном коде!

Overgrowth – игра от компании Wolfire Games. Это экшен с видом от 3-го лица, действие которого происходит в мрачном средневековом мире животных с повадками людей. В игре очень интересная система управления и довольно продвинутый ИИ. В ходе прохождения игроку даётся полная свобода передвижений и организации своих действий. Присутствует многопользовательский режим.

Overgrowth построен на движке Phoenix. Он содержит улучшенную модель движения. Бег, прыжки, перекаты, повороты происходят плавно, а также все позы и анимация изменяются в зависимости от окружения, настроения и даже индивидуальности каждого персонажа. На окружающую среду игры действует погода, даже деревья будут расти быстрее под солнечными лучами.

Анонс Overgrowth произошёл 17 сентября 2008 года, окончательная версия игры вышла 16 октября 2017 года.

Так как с момента публикации исходного кода в проект непрерывно коммитят, я зафиксировал его версией: f2a67f7.

Итак, давайте разберём самые интересные предупреждения, которые удалось найти при помощи проверки проекта PVS-Studio.

Начнём, пожалуй, с функции, в которой PVS-Studio выдал два предупреждения на соседние строки кода.

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

Начнём с предупреждения V773, как наиболее тривиального. Память под указатель worldImporter была выделена при помощи оператора new и по выходу из функции не была освобождена. Это плохая практика, которая приводит к утечке ресурсов. Поправить данный фрагмент кода можно было бы, позвав оператор delete по завершению работы с этим указателем.

Перейдём к предупреждению V611 и буферу heightfieldData. Тут разработчик очистил динамически выделенную память, однако сделал это неправильно. Вместо того, чтоб позвать оператор delete[] для освобождения аллоцированной ранее памяти при помощи оператора new[], он позвал простой delete. Согласно стандарту, такой код явно приведёт к неопределённому поведению, вот ссылка на соответствующий пункт.

Поправленный фрагмент кода:

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

V772 [CERT-MSC15-C] Calling a «delete» operator for a void pointer will cause undefined behavior. OVR_CAPI_Util.cpp 380

Применение операторов delete и delete[] для указателя на void ведёт к неопределённому поведению. Чтобы избежать ошибки, нужно явно привести указатель к его фактическому типу при очистке памяти.

Чтобы понять, как глубока проблема, было проведено небольшое исследование. Поле Samples инициализируется только в одном месте и типом uint8_t*. Вот пруф:

Это говорит об архитектурной ошибке при проектировании класса. Возможно, раньше оно инициализировалось разными типами и это убрали вследствие рефакторинга, а изменить тип поля Samples с void* на uint8_t* забыли.

Разработчикам в любом случае стоит обратить внимание на этот участок кода и поправить его, он странный и ведёт к UB.

V595 [CERT-EXP12-C] The «ctx» pointer was utilized before it was verified against nullptr. Check lines: 130, 131. ascontext.cpp 130

В данном фрагменте кода указатель ctx сначала разыменовывается, а потом проверяется на 0. Это выглядит довольно подозрительно. Если программист опасается, что ctx может быть равен nullptr, то, возможно, его стоит разыменовывать уже после проверки:

V547 Expression 'connect_id_ == — 1' is always true. placeholderobject.cpp 342

В данном фрагменте кода анализатор заметил избыточную проверку connect_id_ == -1. Она уже была выполнена в условии верхнего уровня, и переменная connect_id_ с этого момента не менялась.

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

V791 The initial value of the index in the nested loop equals «i'. Perhaps, 'i + 1» should be used instead. navmeshhintobject.cpp 65

Здесь анализатор выявил неоптимальный цикл. Используется паттерн кода с выполнением некоторых операций для пар элементов массива. При этом анализатор понял, что нет смысла выполнять операцию для пары, состоящей из одного и того же элемента i == j. Данный фрагмент кода можно упростить:

V561 [CERT-DCL01-C] It«s probably better to assign value to 'other_radius_sq» variable than to declare it anew. Previous declaration: scenegraph.cpp, line 2006. scenegraph.cpp 2010

Анализатор обнаружил подозрительный фрагмент кода. Здесь происходит переопределение переменной other_radius_sq. Зачастую появление сущностей с одинаковыми именами — следствие неудачной копипасты.

Значение переменной imageBits не меняется между инициализацией и проверками. Скорее всего, это не ошибка, просто анализатор выявил странный недописанный или избыточный фрагмент кода. Разработчикам стоит обратить на него внимание!

V769 [CERT-EXP08-C] The «idx_buffer_offset' pointer in the 'idx_buffer_offset += pcmd->ElemCount» expression equals nullptr. The resulting value is senseless and it should not be used. imgui_impl_sdl_gl3.cpp 138

Анализатор обнаружил подозрительную операцию сложения, применяемую к нулевому указателю. Указатель далее не используется. Да и нельзя его использовать. В общем, это какой-то бессмысленный код.

Ещё одно похожее срабатывание:

V769 [CERT-EXP08-C] The «cp' pointer in the »cp ++' expression equals nullptr. The resulting value is senseless and it should not be used. crn_file_utils.cpp 547

Данный фрагмент также похож на последствия рефакторинга или криво реализованный алгоритм. Что имели в виду разработчики — остаётся только гадать…

V523 The «then' statement is equivalent to the 'else» statement. skeleton.cpp 152

В продолжение обзора странного кода. Анализатор обнаружил if с одинаковыми then и else. Скорее всего, разработчик просто не дописал второй фрагмент кода. Об этом можно судить по разным закомментированным фрагментам в ветках кода.

V728 An excessive check can be simplified. The '(A && !B) || (!A && B)« expression is equivalent to the »bool(A) != bool(B)' expression. as_compiler.cpp 4317

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

PVS-Studio обнаружил паттерн в проверяемом условии, который можно упростить — и сделать код существенно читабельнее. Программист пытается понять, что произошёл вызов одного или другого конструктора. Операция, которую он применяет очень похожа на XOR. Однако в C++ нет исключающего «ИЛИ» для типа bool, и порой это выливается в гораздо более громоздкий код. Переписать его можно, например, таким образом:

V610 [CERT-INT34-C] Undefined behavior. Check the shift operator '<<«. The right operand ('i» = [0..63]) is greater than or equal to the length in bits of the promoted left operand. bitarray.cpp 77

PVS-Studio обнаружил опасный код со сдвигом влево беззнакового числа. Согласно стандарту, это неопределённое поведение, если правый операнд больше или равен разрядности левого операнда. Литерал 1UL под MSVC представлен 32 битами, правый операнд же лежит в диапазоне от 0 до 63.

Так как код рассчитан в том числе и для сборки под Windows, разработчикам стоит обратить внимание на него. Вот ещё несколько предупреждений с такой же проблемой:

V751 [CERT-MSC13-C] Parameter «rayTo» is not used inside function body. btSoftBody.cpp 2148

Здесь анализатор говорит о формальном параметре rayTo, который не используется в теле функции. При этом параметр rayFrom используется несколько раз, что может свидетельствовать об ошибке или не очень удачном рефакторинге кода.

В результате проверки анализатор нашёл в проекте ошибки разного рода: традиционные опечатки, довольно много неправильной работы с памятью, логические ошибки. Мы надеемся, что благодаря данной статье, разработчики Overgrowth смогут поправить некоторые недочёты, а может быть, захотят самостоятельно перепроверить свою кодовую базу PVS-Studio. Это безусловно поможет замечательной игре и дальше радовать своих фанатов в уже новых, менее подверженных багам сборках: )