Как найти работу для фиксиков: анализируем Godot Engine

Разработка игр и их прохождение могут быть невероятно увлекательными и затягивающими занятиями, приносящими огромное удовольствие. Но ничто так не портит впечатление от игрового процесса, как коварно спрятавшийся баг. Поэтому сегодня под нашим пристальным вниманием окажется Open Source движок Godot Engine. Давайте проверим, насколько он хорош, и готов ли он подарить нам незабываемые эмоции от создания и прохождения игр.

Godot — это универсальный 2D и 3D игровой движок, спроектированный для поддержки всех видов проектов. Его можно использовать для создания игр или приложений, которые затем можно выпускать на настольных или мобильных платформах, а также web.

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

На движке были написаны такие игры, как 1000 days to escape, City Game Studio: Your Game Dev Adventure Begins, Precipice.

Версия Godot Engine, на которой производилась проверка — 4.2.2.

Кстати, в 2018 году мы уже проверяли Godot Engine. С прошлой статьёй можно ознакомиться здесь.

И первое, с чего бы хотелось начать после просмотра отчёта — это проблемы с макросами в проекте. Беда с ними в том, что параметры не оборачиваются в круглые скобки. Давайте рассмотрим несколько примеров, когда это может выстрелить в ногу.

Фрагменты N1-N2

Этот макрос нужен, чтобы проверить, выставлен ли определённый флаг предупреждения или нет.

Переменная warning_flags является побитовой маской и имеет тип uint_32t. Это значит, что её значение состоит из 32 битов, где каждому биту соответствует 1, если флаг выставлен, и 0, если нет. Макрос используется в условных операторах, где неявно преобразуется к типу bool. Для понимания работы рассмотрим упрощённый вариант, где вместо 32 бит будем использовать 8.

Предположим, у нас есть какой-нибудь флаг X, который соответствует 4-у биту в маске и в настоящий момент он поднят. Тогда значение переменной warning_flags в двоичной системе будет иметь следующий вид:

Теперь предположим, что мы решили проверить с помощью нашего макроса выставлен ли флаг X.

Мы передаём в макрос переменную flag со значением 00001000 и в результате побитового "И" получаем ненулевое значение, которое преобразуется в bool со значением к true.

Теперь предположим, что мы захотели проверить флаг Y, который соответствует третьему биту, при том же значении переменной warning_flags. Мы передаём в макрос переменную со значением 00000100 и в результате побитового "И" получаем нулевое значение, которое преобразуется в bool со значением false.

Казалось бы, всё отлично, и что же может пойти не так. Но что, если кто-нибудь захочет проверить, выставлен ли один из нескольких флагов? Тогда он может позвать макрос так:

И тогда результат такой операции всегда будет true, даже если ни один из флагов не выставлен. Почему так происходит? Давайте поработаем препроцессором и просто подставим переданное в макрос выражение:

А теперь обратимся к таблице приоритетов операторов:

Операторы перечислены сверху вниз в порядке убывания приоритета. Из этого следует, что выражение будет обработано следующим образом:

Допустим, в warning_flags не установлены интересующие нас флаги X и Y. Тогда первая операция побитового И вернёт значение 0, и затем в него будет установлен флаг Y. Получаем всегда истинную проверку.

Собственно, анализатор выдаёт на этом макросе следующее предупреждение:

V1003 The macro 'HAS_WARNING' is a dangerous expression. The parameter 'flag' must be surrounded by parentheses. shader_language.cpp 40

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

Другой пример опасного макроса:

Предупреждение анализатора:

V1003 The macro 'IS_SAME_ROW' is a dangerous expression. The parameters 'i', 'row' must be surrounded by parentheses. item_list.cpp 643

Если мы передадим в макрос вместо одной переменной какое-нибудь выражение, например, такое:

То в результате подстановки препроцессора получим:

Где порядок вычисления совсем не тот, который ожидали.

Чтобы обезопасить себя от таких ситуаций, достаточно обернуть параметры макроса в скобки:

Фрагмент N3

Теперь рассмотрим следующее условие:

Это условие всегда будет true (не считая случая, когда указатель tex->detect_roughness_callback будет нулевым).

Для того, чтобы разобраться почему так, нужно взглянуть на enum Hint в структуре Uniform:

Под коробкой такого enum'a находится целочисленный тип, и значениям HINT_ROUGHNESS_R и HINT_ROUGHNESS_GRAY соответствуют числа 5 и 9.

Исходя из этого, в условии проверяется, что p_texture_uniforms[i].hint >= 5 или p_texture_uniforms[i].hint <= 9. Это означает, что любое значение p_texture_uniforms[i].hint пройдёт эти проверки, о чём и предупреждает PVS-Studio:

V547 Expression is always true. material_storage.cpp 929

На самом деле программист хотел проверить, что p_texture_uniforms[i].hint находится в диапазоне от 5 до 9. Для этого надо применить логическое "И":

Аналогичное срабатывание:

V547 Expression is always true. material_storage.cpp 1003

Фрагмент N4

Попробуйте найти ошибку здесь самостоятельно:

Предупреждение анализатора:

V778 Two similar code fragments were found. Perhaps, this is a typo and 'y' variable should be used instead of 'x'. font.cpp 1970

Итак, PVS-Studio нашёл ошибку, возникшую при копировании куска кода. Давайте повнимательнее посмотрим на условные блоки. По сути, они идентичны, за исключением того, что в первом случае все операции проводятся над kpk.x, а во втором — над kpk.y.

Но во второе условие в результате copy-paste забралась ошибка. Обратите внимание на вызов WARN_PRINT: если kpk.y > 0xFF, то при формировании предупреждения будет напечатан символ kpk.x, а не kpk.y. Искать ошибку на основе логов будет сложнее :)

P.S.: на самом деле не следовало размножать код таким способом. Явно видно, что два блока кода отличаются только применяемым полем. Лучшим вариантом было бы вынести код в функцию и вызвать её дважды для разных полей:

Фрагмент N5

Ещё условия, но уже вложенные:

Предупреждение анализатора:

V571 Recurring check. The 'mb->is_pressed()' condition was already verified in line 837. grid_map_editor_plugin.cpp 838

В этом фрагменте присутствует лишняя проверка во вложенном операторе if. Выражение mb->is_pressed() уже было проверено уровнем выше. Возможно, это двойная проверка (часто встречается в GUI), но тогда стоило добавить комментарий об этом. А возможно, что должно было быть проверено что-то другое.

Похожие срабатывания:

V571 Recurring check. The '!r_state.floor' condition was already verified in line 1711. physics_body_3d.cpp 1713

V571 Recurring check. The '!wd_window.is_popup' condition was already verified in line 2012. display_server_x11.cpp 2013

V571 Recurring check. The 'member.variable->initializer' condition was already verified in line 946. gdscript_analyzer.cpp 949

Фрагмент N6

И куда же без классики — разыменование указателя до его проверки:

Предупреждение анализатора:

V595 The 'node' pointer was utilized before it was verified against nullptr. Check lines: 246, 251. grid_map_editor_plugin.cpp 246

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

Похожие срабатывания:

V595 The 'p_ternary_op->true_expr' pointer was utilized before it was verified against nullptr. Check lines: 4518, 4525. gdscript_analyzer.cpp 4518

V595 The 'p_parent' pointer was utilized before it was verified against nullptr. Check lines: 4100, 4104. node_3d_editor_plugin.cpp 4100

V595 The 'item' pointer was utilized before it was verified against nullptr. Check lines: 950, 951. project_export.cpp 950

V595 The 'title_bar' pointer was utilized before it was verified against nullptr. Check lines: 1153, 1163. editor_node.cpp 1153

V595 The 'render_target' pointer was utilized before it was verified against nullptr. Check lines: 2121, 2132. rasterizer_canvas_gles3.cpp 2121

V595 The '_p' pointer was utilized before it was verified against nullptr. Check lines: 228, 231. dictionary.cpp 228

V595 The 'class_doc' pointer was utilized before it was verified against nullptr. Check lines: 1215, 1231. extension_api_dump.cpp 1215

Фрагмент N7

Предупреждение анализатора:

V780 Instantiation of LocalVector < AnimationCompressionDataState >: The object 'w' of a non-passive (non-PDS) type cannot be copied using the memcpy function. local_vector.h 280

Интересный фрагмент. У шаблона класса LocalVector сделали оператор конверсии в класс Vector. При таком преобразовании нужно скопировать содержимое текущего вектора в новый. Для этого воспользовались функцией memcpy.

Всё достаточно неплохо, пока шаблонный тип T тривиально копируемый. Однако анализатор обнаружил различные специализации LocalVector, у которого это свойство нарушается. В качестве примера рассмотрим специализацию LocalVector:

Класс AnimationCompressionDataState содержит в себе LocalVector, который сам нетривиально копируемый.

Для этого случая в документации на memcpy есть уточнение: "If the objects are potentially-overlapping or not TriviallyCopyable, the behavior of memcpy is not specified and may be undefined".

Исправить код не составит труда, достаточно заменить вызов memcpy на std::uninitialized_copy:

PVS-Studio обнаружил ещё 38 опасных специализаций, но их полный список я приводить, конечно же, не буду:

V780 Instantiation of LocalVector < AnimationCompressionDataState >: The object 'w' of a non-passive (non-PDS) type cannot be copied using the memcpy function. local_vector.h 280

V780 Instantiation of LocalVector < LocalVector >: The object 'w' of a non-passive (non-PDS) type cannot be copied using the memcpy function. local_vector.h 280

V780 Instantiation of LocalVector < Mapping, uint32_t, bool, bool >: The object 'w' of a non-passive (non-PDS) type cannot be copied using the memcpy function. local_vector.h 280

V780 Instantiation of LocalVector < OAHashMap < uint64_t, Specialization > >: The object 'w' of a non-passive (non-PDS) type cannot be copied using the memcpy function. local_vector.h 280

V780 Instantiation of LocalVector < Pair < StringName, StringName >, uint32_t, bool, bool >: The object 'w' of a non-passive (non-PDS) type cannot be copied using the memcpy function. local_vector.h 280

...

Фрагмент N8

Возможное нарушение программной логики:

Предупреждение анализатора:

V781 The value of the 'non_op' index is checked after it was used. Perhaps there is a mistake in program logic. gdscript_highlighter.cpp 370

Значение non_op сначала используется в качестве индекса при доступе к символам строки, и только потом проверяется на то, что оно меньше длины.

Обратите внимание на доступ к строке после проверки. Если non_op < line_length, то это ещё не означает, что (non_op + 1) < line_length. Поэтому в str[non_op + 1] может произойти выход за границу строки. Особенно с учётом того, что под коробкой String не лежат нуль-терминированные строки.

Корректная проверка должна выглядеть так:

Фрагмент N9

Предупреждение анализатора:

V555 The expression 'particles->amount - lifetime_split > 0' will work as 'particles->amount != lifetime_split'. particles_storage.cpp 959

Этот пример интересен тем, что, несмотря на не совсем корректное срабатывание анализатора, он указал нам на место, куда разработчикам стоит обратить внимание.

Если разница двух беззнаковых переменных больше нуля, то на самом деле это выражение семантически равно particles->amount != lifetime_split. Условие посчитается как false только в случае, когда эти переменные равны. Если левый операнд меньше правого, то произойдёт переполнение с оборачиванием, и результирующее выражение будет больше нуля. Если левый операнд больше правого, то разность будет больше нуля.

Однако примечательно тут другое: обе переменные имеют одинаковый ранг, но разную знаковость. Компилятор по стандарту обязан провести неявные преобразования, прежде чем выполнить вычитание. И в этой ситуации общим типом будет беззнаковый 32-битный int. И это тоже может добавить сюрпризов, если в левом операнде будет отрицательное число.

Наиболее корректная проверка в случае участия выражений знаковых и беззнаковых типов одного ранга будет выглядеть следующим образом:

На самом деле мы с вами переизобрели std::cmp_greater, введённый в C++20, и, начиная с этой версии стандарта, можно написать лаконичный код:

Фрагмент N10

Предупреждение анализатора:

V1044 Loop break conditions do not depend on the number of iterations. animation_state_machine_editor.cpp 693

Цикл while длится ровно одну итерацию. Очень похоже на паттерн, когда из контейнера нужно взять только первый элемент, и это делается с помощью цикла for:

Таким образом, не нужно проверять, не содержит ли контейнер в себе первый элемент. ИМХО, такой код скорее запутывает, т.к. ожидаешь от циклов заведомо неизвестного конечного числа итераций.

Во фрагменте же выше цикл while абсолютно не имеет смысла. Достаточно было бы простой конструкции if:

Фрагмент N11

Читатель может задаться вопросом: "И что же здесь не так?" Мы бы и сами не поняли, если бы не срабатывание диагностического правила V1076. Что интересно, это первое выписанное нами срабатывание. Диагностическое правило проверяет текст программы на наличие невидимых символов. Такие символы — это своего рода закладки, которые программист может не видеть из-за настроек отображения текста в среде разработки, зато компилятор прекрасно их видит и обрабатывает.

Предупреждение анализатора:

V1076 Code contains invisible characters that may alter its logic. Consider enabling the display of invisible characters in the code editor. locales.h 1114

Давайте внимательно посмотрим на следующую строку:

Именно в ней содержится закладка с невидимым символом. Если воспользоваться hex-редактором, то можно заметить следующее:

Между двойной кавычкой и символом N затесались 3 байта: E2, 80 и 8B. Они соответствуют Unicode-символу ZERO WIDTH SPACE (U+200B).

К счастью, наличие этого символа в строковом литерале не повлияет на логику программы.

Вероятнее всего, закладка внеслась случайно в результате копирования названия с какого-нибудь сайта. Достаточно просто удалить этот символ из строкового литерала.

Фрагмент N12

Предупреждения анализатора:

V501 There are identical sub-expressions 'RenderingServer::ARRAY_FORMAT_VERTEX' to the left and to the right of the '|' operator. mesh_storage.cpp 1414

V578 An odd bitwise operation detected. Consider verifying it. mesh_storage.cpp 1414

Странная инициализация битовой маски. В неё два раза записывается RS::ARRAY_FORMAT_VERTEX, хотя, возможно, хотели записать какой-то другой флаг.

Такое же срабатывание:

V501 There are identical sub-expressions 'RenderingServer::ARRAY_FORMAT_VERTEX' to the left and to the right of the '|' operator. mesh_storage.cpp 1300

V578 An odd bitwise operation detected. Consider verifying it. mesh_storage.cpp 1300

Фрагмент N13

Предупреждение анализатора:

V1083 Signed integer overflow is possible in 'p_width * p_height' arithmetic expression. This leads to undefined behavior. Left operand is in range '[0x1..0x1000000]', right operand is in range '[0x1..0x1000000]'. image.cpp 2200

Итак, мы имеем две переменные p_width и p_height типа int. Максимальное значение, которое может хранить 4-байтовый int, равно 2'147'483'647.

Сначала в коде проверяется, что p_width <= MAX_WIDTH, где MAX_WIDTH == 16'777'216. Затем проверяется, что p_height <= MAX_HEIGHT, где MAX_HEIGHT == 16 777 216. В третьей проверке мы сравниваем, что произведение p_width * p_height <= MAX_PIXELS.

Разберём ситуацию, когда p_width == p_height && p_width == 16'777'216. Результат перемножения этих двух чисел равен 281'474'976'710'656. Для того, чтобы отобразить такой результат, требуется уже 8-байтовое число, т.е. налицо знаковое переполнение. А, как известно, в языках C и C++ это ведёт к неопределённому поведению.

Если нет никаких вспомогательных функций, которые проверяют переполнение, то самый простой вариант исправления может выглядеть так:

Фрагмент N14

V1020 The function exited without calling the 'mutex.unlock' function. Check lines: 556, 438. remote_debugger.cpp 556

Крайне интересный фрагмент с многопоточным исполнением. Анализатор PVS-Studio обнаружил, что на некоторых путях исполнения мьютекс может быть не разблокирован. Давайте разбираться.

Начать нужно с того, какой тип мьютекса используется:

Копнём чуть глубже, что же это за Mutex:

Итак, на самом деле перед нами не обычный мьютекс, а рекурсивный. Используют его совместно с кастомной RAII-обёрткой:

Почти повсеместно мьютекс RemoteDebugger::mutex используется совместно с RAII-обёртками, покажу лишь пару мест: [1], [2], [3], ....

Однако в одном месте что-то пошло не так. Анализатор указал на место, где с мьютексом работают вручную. Из-за этого имеем несколько различных вариантов исполнения кода:

1. Мьютекс блокируется, цикл не выполняется ни разу (N == 0). В итоге поток управления покинет функцию RemoteDebugger::debug со счётчиком захвата, увеличенным на 1.

2. Мьютекс блокируется, цикл выполняется N == 1 раз. В этом случае всё будет хорошо — счётчик захвата рекурсивного мьютекса увеличивается и уменьшается на одинаковое число.

3. Мьютекс блокируется, цикл выполняется N > 1 раз. В итоге у рекурсивного мьютекса счётчик захвата уменьшится на N – 1 относительно момента до его ручной блокировки, что может привести к неопределённому поведению.

Если изучить вызовы функции is_peer_connected по кодовой базе ([1], [2], [3], ....), то во всех случаях они происходят под блокировкой RemoteDebugger::mutex. Судя по всему, программисту и в этом случае требовалась блокировка, но реализовал её он вручную.

На основе таких предположений можно поправить код следующим способом:

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

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

Начать использовать такие решения проще, чем может показаться. Например, получить пробную версию анализатора PVS-Studio можно здесь. Также существуют различные сценарии бесплатного его использования.

Всем спасибо за чтение и хорошего дня!

#bugs #pvsstudio #godot #godotengine #staticanalysis