# Как найти работу для фиксиков: анализируем Godot Engine Разработка игр и их прохождение могут быть невероятно увлекательными и затягивающими занятиями, приносящими огромное удовольствие\. Но ничто так не портит впечатление от игрового процесса, как коварно спрятавшийся баг\. Поэтому сегодня под нашим пристальным вниманием окажется Open Source движок Godot Engine\. Давайте проверим, насколько он хорош, и готов ли он подарить нам незабываемые эмоции от создания и прохождения игр\. ![1150_Godot_ru/image1.png](https://import.viva64.com/docx/blog/1150_Godot_ru/image1.png) ## Godot Godot — это универсальный 2D и 3D игровой движок, спроектированный для поддержки всех видов проектов\. Его можно использовать для создания игр или приложений, которые затем можно выпускать на настольных или мобильных платформах, а также web\. Движок достаточно молодой, но уже набирает обороты и пользуется популярностью у тех, кто ценит открытость исходного кода, бесплатность и хорошую расширяемость\. Godot весьма дружелюбен к новичкам, и поэтому популярен у начинающих разработчиков\. На движке были написаны такие игры, как 1000 days to escape, City Game Studio: Your Game Dev Adventure Begins, Precipice\. Версия Godot Engine, на которой производилась проверка — [4\.2\.2](https://github.com/godotengine/godot/tree/4.2.2-stable)\. Кстати, в 2018 году мы уже проверяли Godot Engine\. С прошлой статьёй можно ознакомиться [здесь](https://pvs-studio.ru/ru/blog/posts/cpp/0594/)\. ## Результаты проверки с помощью PVS\-Studio И первое, с чего бы хотелось начать после просмотра отчёта — это проблемы с макросами в проекте\. Беда с ними в том, что параметры не оборачиваются в круглые скобки\. Давайте рассмотрим несколько примеров, когда это может выстрелить в ногу\. **Фрагменты N1\-N2** ```cpp #define HAS_WARNING(flag) (warning_flags & flag) ``` Этот макрос нужен, чтобы проверить, выставлен ли определённый флаг предупреждения или нет\. Переменная _warning\_flags_ является побитовой маской и имеет тип _uint\_32t_\. Это значит, что её значение состоит из 32 битов, где каждому биту соответствует 1, если флаг выставлен, и 0, если нет\. Макрос используется в условных операторах, где неявно преобразуется к типу _bool_\. Для понимания работы рассмотрим упрощённый вариант, где вместо 32 бит будем использовать 8\. Предположим, у нас есть какой\-нибудь флаг X, который соответствует 4\-у биту в маске и в настоящий момент он поднят\. Тогда значение переменной _warning\_flags_ в двоичной системе будет иметь следующий вид: ```cpp 00001000 ``` Теперь предположим, что мы решили проверить с помощью нашего макроса выставлен ли флаг _X_\. Мы передаём в макрос переменную _flag_ со значением _00001000_ и в результате побитового "И" получаем ненулевое значение, которое преобразуется в _bool_ со значением к _true_\. Теперь предположим, что мы захотели проверить флаг _Y_, который соответствует третьему биту, при том же значении переменной _warning\_flags\. _Мы передаём в макрос переменную со значением _00000100_ и в результате побитового "И" получаем нулевое значение, которое преобразуется в _bool_ со значением _false_\. Казалось бы, всё отлично, и что же может пойти не так\. Но что, если кто\-нибудь захочет проверить, выставлен ли один из нескольких флагов? Тогда он может позвать макрос так: ```cpp if (HAS_WARNING(flags::X | flags::Y)) .... ``` И тогда результат такой операции всегда будет _true_, даже если ни один из флагов не выставлен\. Почему так происходит? Давайте поработаем препроцессором и просто подставим переданное в макрос выражение: ```cpp if (warning_flags & flags::X | flags::Y) .... ``` А теперь обратимся к [таблице](https://en.cppreference.com/w/cpp/language/operator_precedence) приоритетов операторов: |Приоритет|Оператор|Описание|Ассоциативность| |---|---|---|---| |\\\.\\\.\\\.\\\.|\\\.\\\.\\\.\\\.|\\\.\\\.\\\.\\\.|\\\.\\\.\\\.\\\.| |11|a & b |Побитовое И|Слева направо| |\\\.\\\.\\\.\\\.|\\\.\\\.\\\.\\\.|\\\.\\\.\\\.\\\.|\\\.\\\.\\\.\\\.| |13|\\\| |Побитовое ИЛИ|Слева направо| Операторы перечислены сверху вниз в порядке убывания приоритета\. Из этого следует, что выражение будет обработано следующим образом: ```cpp if (( warning_flags & flags::X ) | flags::Y) .... ``` Допустим, в _warning\_flags_ не установлены интересующие нас флаги _X_ и _Y_\. Тогда первая операция побитового И вернёт значение 0, и затем в него будет установлен флаг _Y_\. Получаем всегда истинную проверку\. Собственно, анализатор выдаёт на этом макросе следующее предупреждение: [V1003](https://pvs-studio.ru/ru/docs/warnings/v1003/) The macro 'HAS\_WARNING' is a dangerous expression\. The parameter 'flag' must be surrounded by parentheses\. [shader\_language\.cpp](https://github.com/godotengine/godot/blob/15073afe3856abd2aa1622492fe50026c7d63dc1/servers/rendering/shader_language.cpp#L40) 40 И, как указано в сообщении, для исправления нужно всего лишь обернуть параметр макроса в скобки: ```cpp #define HAS_WARNING(flag) (warning_flags & (flag)) ``` Другой пример опасного макроса: ```cpp #define IS_SAME_ROW(i, row) (i / current_columns == row) ``` Предупреждение анализатора: [V1003](https://pvs-studio.ru/ru/docs/warnings/v1003/) The macro 'IS\_SAME\_ROW' is a dangerous expression\. The parameters 'i', 'row' must be surrounded by parentheses\. [item\_list\.cpp](https://github.com/godotengine/godot/blob/15073afe3856abd2aa1622492fe50026c7d63dc1/scene/gui/item_list.cpp#L643) 643 Если мы передадим в макрос вместо одной переменной какое\-нибудь выражение, например, такое: ```cpp IS_SAME_ROW(current + 1, row) ``` То в результате подстановки препроцессора получим: ```cpp (current + 1 / current_columns == row) ``` Где порядок вычисления совсем не тот, который ожидали\. Чтобы обезопасить себя от таких ситуаций, достаточно обернуть параметры макроса в скобки: ```cpp #define IS_SAME_ROW(i, row) ((i) / current_columns == (row)) ``` **Фрагмент N3** Теперь рассмотрим следующее условие: ```cpp const auto hint_r = ShaderLanguage::ShaderNode::Uniform::HINT_ROUGHNESS_R; const auto hint_gray = ShaderLanguage::ShaderNode::Uniform::HINT_ROUGHNESS_GRAY; if (tex->detect_roughness_callback && ( p_texture_uniforms[i].hint >= hint_r || p_texture_uniforms[i].hint <= hint_gray)) { .... } ``` Это условие всегда будет _true_ \(не считая случая, когда указатель _tex\-\>detect\_roughness\_callback_ будет нулевым\)\. Для того, чтобы разобраться почему так, нужно взглянуть на _enum Hint_ в структуре _Uniform_: ```cpp struct Uniform { .... enum Hint { HINT_NONE, HINT_RANGE, HINT_SOURCE_COLOR, HINT_NORMAL, HINT_ROUGHNESS_NORMAL, HINT_ROUGHNESS_R, HINT_ROUGHNESS_G, HINT_ROUGHNESS_B, HINT_ROUGHNESS_A, HINT_ROUGHNESS_GRAY, HINT_DEFAULT_BLACK, HINT_DEFAULT_WHITE, HINT_DEFAULT_TRANSPARENT, HINT_ANISOTROPY, HINT_SCREEN_TEXTURE, HINT_NORMAL_ROUGHNESS_TEXTURE, HINT_DEPTH_TEXTURE, HINT_MAX }; .... }; ``` Под коробкой такого 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](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression is always true\. [material\_storage\.cpp](https://github.com/godotengine/godot/blob/15073afe3856abd2aa1622492fe50026c7d63dc1/servers/rendering/renderer_rd/storage_rd/material_storage.cpp#L929) 929 На самом деле программист хотел проверить, что _p\_texture\_uniforms\[i\]\.hint_ находится в диапазоне от 5 до 9\. Для этого надо применить логическое "И": ```cpp if (tex->detect_roughness_callback && ( p_texture_uniforms[i].hint >= hint_r && p_texture_uniforms[i].hint <= hint_gray)) { .... } ``` Аналогичное срабатывание: * V547 Expression is always true\. material\_storage\.cpp 1003 **Фрагмент N4** Попробуйте найти ошибку здесь самостоятельно: ```cpp Error FontFile::load_bitmap_font(const String &p_path) { if (kpk.x >= 0x80 && kpk.x <= 0xFF) { kpk.x = _oem_to_unicode[encoding][kpk.x - 0x80]; } else if (kpk.x > 0xFF){ WARN_PRINT(vformat("Invalid BMFont OEM character %x (should be 0x00-0xFF).", kpk.x)); kpk.x = 0x00; } if (kpk.y >= 0x80 && kpk.y <= 0xFF) { kpk.y = _oem_to_unicode[encoding][kpk.y - 0x80]; } else if (kpk.y > 0xFF){ WARN_PRINT(vformat("Invalid BMFont OEM character %x (should be 0x00-0xFF).", kpk.x)); kpk.y = 0x00; } .... } ``` ![1150_Godot_ru/image2.png](https://import.viva64.com/docx/blog/1150_Godot_ru/image2.png) Предупреждение анализатора: [V778](https://pvs-studio.ru/ru/docs/warnings/v778/) Two similar code fragments were found\. Perhaps, this is a typo and 'y' variable should be used instead of 'x'\. [font\.cpp](https://github.com/godotengine/godot/blob/15073afe3856abd2aa1622492fe50026c7d63dc1/scene/resources/font.cpp#L1970) 1970 Итак, PVS\-Studio нашёл ошибку, возникшую при копировании куска кода\. Давайте повнимательнее посмотрим на условные блоки\. По сути, они идентичны, за исключением того, что в первом случае все операции проводятся над _kpk\.x_, а во втором — над _kpk\.y_\. Но во второе условие в результате copy\-paste забралась ошибка\. Обратите внимание на вызов _WARN\_PRINT_: если _kpk\.y \> 0xFF_, то при формировании предупреждения будет напечатан символ _kpk\.x_, а не _kpk\.y_\. Искать ошибку на основе логов будет сложнее :\) P\.S\.: на самом деле не следовало размножать код таким способом\. Явно видно, что два блока кода отличаются только применяемым полем\. Лучшим вариантом было бы вынести код в функцию и вызвать её дважды для разных полей: ```cpp Error FontFile::load_bitmap_font(const String &p_path) { constexpr auto check = [](auto &ch) { if (ch >= 0x80 && ch <= 0xFF) { auto res = _oem_to_unicode[encoding][ch - 0x80]; ch = res; } else if (ch > 0xFF) { WARN_PRINT(vformat("Invalid BMFont OEM character %x (should be 0x00-0xFF).",ch)); ch = 0x00; } }; check(kpk.x); check(kpk.y); .... } ``` **Фрагмент N5** Ещё условия, но уже вложенные: ```cpp void GridMapEditor::_mesh_library_palette_input(const Ref &p_ie) { const Ref mb = p_ie; // Zoom in/out using Ctrl + mouse wheel if (mb.is_valid() && mb->is_pressed() && mb->is_command_or_control_pressed()) { if (mb->is_pressed() && mb->get_button_index() == MouseButton::WHEEL_UP) { size_slider->set_value(size_slider->get_value() + 0.2); } .... } } ``` Предупреждение анализатора: [V571](https://pvs-studio.ru/ru/docs/warnings/v571/) Recurring check\. The 'mb\-\>is\_pressed\(\)' condition was already verified in line 837\. [grid\_map\_editor\_plugin\.cpp](https://github.com/godotengine/godot/blob/15073afe3856abd2aa1622492fe50026c7d63dc1/modules/gridmap/editor/grid_map_editor_plugin.cpp#L838) 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** И куда же без классики — разыменование указателя до его проверки: ```cpp void GridMapEditor::_update_cursor_transform() { cursor_transform = Transform3D(); cursor_transform.origin = cursor_origin; cursor_transform.basis = node->get_basis_with_orthogonal_index(cursor_rot); cursor_transform.basis *= node->get_cell_scale(); cursor_transform = node->get_global_transform() * cursor_transform; if (selected_palette >= 0) { if (node && !node->get_mesh_library().is_null()) { cursor_transform *= node->get_mesh_library() ->get_item_mesh_transform(selected_palette); } } .... } ``` Предупреждение анализатора: [V595](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'node' pointer was utilized before it was verified against nullptr\. Check lines: 246, 251\. [grid\_map\_editor\_plugin\.cpp](https://github.com/godotengine/godot/blob/15073afe3856abd2aa1622492fe50026c7d63dc1/modules/gridmap/editor/grid_map_editor_plugin.cpp#L246) 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** ```cpp template class LocalVector { .... public: operator Vector() const { Vector ret; ret.resize(size()); T *w = ret.ptrw(); memcpy(w, data, sizeof(T) * count); return ret; } .... }; ``` Предупреждение анализатора: [V780](https://pvs-studio.ru/ru/docs/warnings/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](https://github.com/godotengine/godot/blob/15073afe3856abd2aa1622492fe50026c7d63dc1/core/templates/local_vector.h#L280) 280 Интересный фрагмент\. У шаблона класса _LocalVector_ сделали оператор конверсии в класс _Vector_\. При таком преобразовании нужно скопировать содержимое текущего вектора в новый\. Для этого воспользовались функцией [_memcpy_](https://en.cppreference.com/w/cpp/string/byte/memcpy)\. Всё достаточно неплохо, пока шаблонный тип _T_ [тривиально копируемый](https://en.cppreference.com/w/cpp/language/classes#Trivially_copyable_class)\. Однако анализатор обнаружил различные специализации _LocalVector_, у которого это свойство нарушается\. В качестве примера рассмотрим специализацию _LocalVector_: ```cpp struct AnimationCompressionDataState { uint32_t components = 3; LocalVector data; // Committed packets. struct PacketData { int32_t data[3] = { 0, 0, 0 }; uint32_t frame = 0; }; float split_tolerance = 1.5; LocalVector temp_packets; // used for rollback if the new frame does not fit int32_t validated_packet_count = -1; .... }; ``` Класс _AnimationCompressionDataState_ содержит в себе _LocalVector_, который сам [нетривиально копируемый](https://github.com/godotengine/godot/blob/15073afe3856abd2aa1622492fe50026c7d63dc1/core/templates/local_vector.h#L299)\. ![1150_Godot_ru/image3.png](https://import.viva64.com/docx/blog/1150_Godot_ru/image3.png) Для этого случая в документации на _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_](https://en.cppreference.com/w/cpp/memory/uninitialized_copy): ```cpp operator Vector() const { Vector ret; ret.resize(size()); T *w = ret.ptrw(); std::uninitialized_copy(data, data + count, w); return ret; } ``` 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** Возможное нарушение программной логики: ```cpp Dictionary GDScriptSyntaxHighlighter::_get_line_syntax_highlighting_impl (int p_line) { const String &str = text_edit->get_line(p_line); .... if ( is_digit(str[non_op]) || ( str[non_op] == '.' && non_op < line_length && is_digit(str[non_op + 1]) ) ) { in_number = true; } .... } ``` Предупреждение анализатора: [V781](https://pvs-studio.ru/ru/docs/warnings/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](https://github.com/godotengine/godot/blob/15073afe3856abd2aa1622492fe50026c7d63dc1/modules/gdscript/editor/gdscript_highlighter.cpp#L370) 370 Значение _non\_op_ сначала используется в качестве индекса при доступе к символам строки, и только потом проверяется на то, что оно меньше длины\. Обратите внимание на доступ к строке после проверки\. Если _non\_op < line\_length_, то это ещё не означает, что _\(non\_op \+ 1\) < line\_length_\. Поэтому в _str\[non\_op \+ 1\]_ может произойти выход за границу строки\. Особенно с учётом того, что под коробкой [_String_](https://github.com/godotengine/godot/blob/4.2.2-stable/core/string/ustring.h#L183) не лежат нуль\-терминированные строки\. Корректная проверка должна выглядеть так: ```cpp if ( is_digit(str[non_op]) || ( str[non_op] == '.' && non_op + 1 < line_length && is_digit(str[non_op + 1]) ) ) { in_number = true; } ``` **Фрагмент N9** ```cpp struct Particles { .... int amount = 0; .... }; void ParticlesStorage::_particles_update_instance_buffer( Particles *particles, const Vector3 &p_axis, const Vector3 &p_up_axis) { .... uint32_t lifetime_split = ....; // Offset VBO so you render starting at the newest particle. if (particles->amount - lifetime_split > 0) { .... } .... } ``` Предупреждение анализатора: [V555](https://pvs-studio.ru/ru/docs/warnings/v555/) The expression 'particles\-\>amount \- lifetime\_split \> 0' will work as 'particles\-\>amount \!\= lifetime\_split'\. [particles\_storage\.cpp](https://github.com/godotengine/godot/blob/15073afe3856abd2aa1622492fe50026c7d63dc1/drivers/gles3/storage/particles_storage.cpp#L959) 959 Этот пример интересен тем, что, несмотря на не совсем корректное срабатывание анализатора, он указал нам на место, куда разработчикам стоит обратить внимание\. Если разница двух беззнаковых переменных больше нуля, то на самом деле это выражение семантически равно _particles\-\>amount \!\= lifetime\_split_\. Условие посчитается как _false_ только в случае, когда эти переменные равны\. Если левый операнд меньше правого, то произойдёт переполнение с оборачиванием, и результирующее выражение будет больше нуля\. Если левый операнд больше правого, то разность будет больше нуля\. Однако примечательно тут другое: обе переменные имеют одинаковый ранг, но разную знаковость\. Компилятор по стандарту обязан провести [неявные преобразования](https://en.cppreference.com/w/c/language/conversion#Integer_promotions), прежде чем выполнить вычитание\. И в этой ситуации общим типом будет беззнаковый 32\-битный _int_\. И это тоже может добавить сюрпризов, если в левом операнде будет отрицательное число\. Наиболее корректная проверка в случае участия выражений знаковых и беззнаковых типов одного ранга будет выглядеть следующим образом: ```cpp if (particles->amount >= 0 && particles->amount > lifetime_split) ``` На самом деле мы с вами переизобрели _std::cmp\_greater_, введённый в C\+\+20, и, начиная с этой версии стандарта, можно написать лаконичный код: ```cpp if (std::cmp_greater(particles->amount, lifetime_split)) ``` **Фрагмент N10** ```cpp void AnimationNodeStateMachineEditor::_delete_tree_draw() { TreeItem *item = delete_tree->get_next_selected(nullptr); while (item) { delete_window->get_cancel_button()->set_disabled(false); return; } delete_window->get_cancel_button()->set_disabled(true); } ``` Предупреждение анализатора: [V1044](https://pvs-studio.ru/ru/docs/warnings/v1044/) Loop break conditions do not depend on the number of iterations\. [animation\_state\_machine\_editor\.cpp](https://github.com/godotengine/godot/blob/15073afe3856abd2aa1622492fe50026c7d63dc1/editor/plugins/animation_state_machine_editor.cpp#L693) 693 Цикл _while_ длится ровно одну итерацию\. Очень похоже на паттерн, когда из контейнера нужно взять только первый элемент, и это делается с помощью цикла _for_: ```cpp for (auto &&item : items) { DoSomething(item); break; } ``` Таким образом, не нужно проверять, не содержит ли контейнер в себе первый элемент\. ИМХО, такой код скорее запутывает, т\.к\. ожидаешь от циклов заведомо неизвестного **конечного** числа итераций\. Во фрагменте же выше цикл _while_ абсолютно не имеет смысла\. Достаточно было бы простой конструкции _if_: ```cpp void AnimationNodeStateMachineEditor::_delete_tree_draw() { TreeItem *item = delete_tree->get_next_selected(nullptr); if (item) { delete_window->get_cancel_button()->set_disabled(false); return; } delete_window->get_cancel_button()->set_disabled(true); } ``` **Фрагмент N11** ```cpp static const char *script_list[][2] = { .... { "Myanmar / Burmese", "Mymr" }, { "​Nag Mundari", "Nagm" }, { "Nandinagari", "Nand" }, .... } ``` Читатель может задаться вопросом: "И что же здесь не так?" Мы бы и сами не поняли, если бы не срабатывание диагностического правила [V1076](https://pvs-studio.ru/ru/docs/warnings/v1076/)\. Что интересно, это первое выписанное нами срабатывание\. Диагностическое правило проверяет текст программы на наличие невидимых символов\. Такие символы — это своего рода закладки, которые программист может не видеть из\-за настроек отображения текста в среде разработки, зато компилятор прекрасно их видит и обрабатывает\. Предупреждение анализатора: [V1076](https://pvs-studio.ru/ru/docs/warnings/v1076/) Code contains invisible characters that may alter its logic\. Consider enabling the display of invisible characters in the code editor\. [locales\.h](https://github.com/godotengine/godot/blob/15073afe3856abd2aa1622492fe50026c7d63dc1/core/string/locales.h#L1114) 1114 Давайте внимательно посмотрим на следующую строку: ```cpp { "​Nag Mundari", "Nagm" }, ``` Именно в ней содержится закладка с невидимым символом\. Если воспользоваться hex\-редактором, то можно заметить следующее: ![1150_Godot_ru/image4.png](https://import.viva64.com/docx/blog/1150_Godot_ru/image4.png) Между двойной кавычкой и символом _N_ затесались 3 байта: _E2_, _80_ и _8B_\. Они соответствуют Unicode\-символу [**ZERO WIDTH SPACE**](https://www.utf8-chartable.de/unicode-utf8-table.pl?start=8192&number=128) \(U\+200B\)\.
К счастью, наличие этого символа в строковом литерале не повлияет на логику программы\\\. Строки из массива _script\_list_, в котором содержится "заражённый" строковый литерал, [попадают](https://github.com/godotengine/godot/blob/4.2.2-stable/core/string/translation.cpp#L276) в хэш\-таблицу _TranslationServer::script\_map_\. Ключом такой хэш\-таблицы будет второй строковый литерал из пары, а значением — первый\. Значит, строковый литерал с закладкой попадёт в хэш\-таблицу как значение, и поиск по хэш\-таблице не нарушится\. Далее можно изучить, а куда потенциально может попасть это значение из хэш\-таблицы\. Я нашёл несколько мест: 1. Значение попадёт в строку, возвращаемую функцией [_TranslationServer::get\_locale\_name_](https://github.com/godotengine/godot/blob/4.2.2-stable/core/string/translation.cpp#L470)\. Проанализировав вызывающие функции, видно, что эта строка так или иначе попадёт в GUI \([\[1\]](https://github.com/godotengine/godot/blob/4.2.2-stable/editor/localization_editor.cpp#L207), [\[2\]](https://github.com/godotengine/godot/blob/4.2.2-stable/editor/localization_editor.cpp#L560), [\[3\]](https://github.com/godotengine/godot/blob/4.2.2-stable/editor/plugins/font_config_plugin.cpp#L292), [\[4\]](https://github.com/godotengine/godot/blob/4.2.2-stable/editor/project_manager.cpp#L3074)\)\. 1. Значение возвращается из функции [_TranslationServer::get\_script\_name_](https://github.com/godotengine/godot/blob/4.2.2-stable/core/string/translation.cpp#L503)\. Проанализировав вызывающие функции, также можно сделать вывод, что строка попадёт в GUI \([\[1\]](https://github.com/godotengine/godot/blob/4.2.2-stable/editor/plugins/font_config_plugin.cpp#L290), [\[2\]](https://github.com/godotengine/godot/blob/4.2.2-stable/editor/plugins/font_config_plugin.cpp#L370)\)\.
Вероятнее всего, закладка [внеслась](https://github.com/godotengine/godot/commit/ec8084d87f273266c5d79d06c421b5167dd97f94) случайно в результате копирования названия с какого\-нибудь сайта\. Достаточно просто удалить этот символ из строкового литерала\. **Фрагмент N12** ```cpp void MeshStorage::update_mesh_instances() { .... uint64_t mask = RS::ARRAY_FORMAT_VERTEX | RS::ARRAY_FORMAT_NORMAL | 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](https://github.com/godotengine/godot/blob/15073afe3856abd2aa1622492fe50026c7d63dc1/drivers/gles3/storage/mesh_storage.cpp#L1414) 1414 * V578 An odd bitwise operation detected\. Consider verifying it\. [mesh\_storage\.cpp](https://github.com/godotengine/godot/blob/15073afe3856abd2aa1622492fe50026c7d63dc1/drivers/gles3/storage/mesh_storage.cpp#L1414) 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** ```cpp void Image::initialize_data(int p_width, int p_height, bool p_use_mipmaps, Format p_format, const Vector &p_data) { .... ERR_FAIL_COND_MSG(p_width > MAX_WIDTH, "The Image width specified (" + itos(p_width) + " pixels) cannot be greater than " + itos(MAX_WIDTH) + " pixels."); ERR_FAIL_COND_MSG(p_height > MAX_HEIGHT, "The Image height specified (" + itos(p_height) + " pixels) cannot be greater than " + itos(MAX_HEIGHT) + " pixels."); ERR_FAIL_COND_MSG(p_width * p_height > MAX_PIXELS, "Too many pixels for image, maximum is " + itos(MAX_PIXELS)); .... } ``` Предупреждение анализатора: [V1083](https://pvs-studio.ru/ru/docs/warnings/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](https://github.com/godotengine/godot/blob/15073afe3856abd2aa1622492fe50026c7d63dc1/core/io/image.cpp#L2200) 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\+\+ это ведёт к неопределённому поведению\. Если нет никаких вспомогательных функций, которые проверяют переполнение, то самый простой вариант исправления может выглядеть так: ```cpp ERR_FAIL_COND_MSG((int64_t) p_width * (int64_t) p_height > (int64_t) MAX_PIXELS, "Too many pixels for image, maximum is " + itos(MAX_PIXELS)); ``` **Фрагмент N14** ```cpp void RemoteDebugger::debug(....) { .... mutex.lock(); while (is_peer_connected()) { mutex.unlock(); .... } send_message("debug_exit", Array()); if (Thread::get_caller_id() == Thread::get_main_id()) { if (mouse_mode != Input::MOUSE_MODE_VISIBLE) { Input::get_singleton()->set_mouse_mode(mouse_mode); } } else { MutexLock mutex_lock(mutex); messages.erase(Thread::get_caller_id()); } } ``` [V1020](https://pvs-studio.ru/ru/docs/warnings/v1020/) The function exited without calling the 'mutex\.unlock' function\. Check lines: 556, 438\. [remote\_debugger\.cpp](https://github.com/godotengine/godot/blob/15073afe3856abd2aa1622492fe50026c7d63dc1/core/debugger/remote_debugger.cpp#L556) 556 Крайне интересный фрагмент с многопоточным исполнением\. Анализатор PVS\-Studio обнаружил, что на некоторых путях исполнения мьютекс может быть не разблокирован\. Давайте разбираться\. Начать нужно с того, какой тип мьютекса используется: ```cpp class RemoteDebugger : public EngineDebugger { .... private: // Make handlers and send_message thread safe. Mutex mutex; .... }; ``` Копнём чуть глубже, что же это за _Mutex_: ```cpp template class MutexImpl { friend class MutexLock>; using StdMutexType = StdMutexT; mutable StdMutexT mutex; public: _ALWAYS_INLINE_ void lock() const { mutex.lock(); } _ALWAYS_INLINE_ void unlock() const { mutex.unlock(); } _ALWAYS_INLINE_ bool try_lock() const { return mutex.try_lock(); } }; // Recursive, for general use using Mutex = MutexImpl; ``` Итак, на самом деле перед нами не обычный мьютекс, а рекурсивный\. Используют его совместно с кастомной RAII\-обёрткой: ```cpp template class MutexLock { friend class ConditionVariable; std::unique_lock lock; public: _ALWAYS_INLINE_ explicit MutexLock(const MutexT &p_mutex) : lock(p_mutex.mutex) {} }; ``` Почти повсеместно мьютекс _RemoteDebugger::mutex_ используется совместно с RAII\-обёртками, покажу лишь пару мест: [\[1\]](https://github.com/godotengine/godot/blob/4.2.2-stable/core/debugger/remote_debugger.cpp#L147), [\[2\]](https://github.com/godotengine/godot/blob/4.2.2-stable/core/debugger/remote_debugger.cpp#L189), [\[3\]](https://github.com/godotengine/godot/blob/4.2.2-stable/core/debugger/remote_debugger.cpp#L264), \.\.\.\. Однако в одном месте что\-то пошло не так\. Анализатор указал на место, где с мьютексом работают вручную\. Из\-за этого имеем несколько различных вариантов исполнения кода: 1. Мьютекс блокируется, цикл не выполняется ни разу \(_N \=\= 0_\)\. В итоге поток управления покинет функцию _RemoteDebugger::debug_ со счётчиком захвата, увеличенным на 1\. 1. Мьютекс блокируется, цикл выполняется _N \=\= 1_ раз\. В этом случае всё будет хорошо — счётчик захвата рекурсивного мьютекса увеличивается и уменьшается на одинаковое число\. 1. Мьютекс блокируется, цикл выполняется _N \> 1_ раз\. В итоге у рекурсивного мьютекса счётчик захвата уменьшится на _N – 1_ относительно момента до его ручной блокировки, что может привести к неопределённому поведению\. Если изучить вызовы функции _is\_peer\_connected_ по кодовой базе \([\[1\]](https://github.com/godotengine/godot/blob/4.2.2-stable/core/debugger/remote_debugger.cpp#L147-L151), [\[2\]](https://github.com/godotengine/godot/blob/4.2.2-stable/core/debugger/remote_debugger.cpp#L189-L192), [\[3\]](https://github.com/godotengine/godot/blob/4.2.2-stable/core/debugger/remote_debugger.cpp#L264-L265), \.\.\.\.\), то во всех случаях они происходят под блокировкой _RemoteDebugger::mutex_\. Судя по всему, программисту и в этом случае требовалась блокировка, но реализовал её он вручную\. На основе таких предположений можно поправить код следующим способом: ```cpp void RemoteDebugger::debug(....) { .... const auto is_peer_connected_sync = [this] { MutexLock _ { mutex }; return is_peer_connected(); }; while (is_peer_connected_sync()) { .... } .... } ``` Не гарантирую, что исправление абсолютно корректно, т\.к\. только разработчики Godot знают, что здесь должно происходить\. Но теперь мы хотя бы избавились от потенциального неопределённого поведения, связанного с разблокировкой мьютекса на каждой итерации цикла\. ## Заключение Ошибки в коде встречаются самые разнообразные: от простых до сложных, от заметных до незаметных\. Для того, чтобы не портить удовольствие и впечатление от продукта, его нужно постоянно очищать от багов и ошибок\. С такой задачей хорошо справляются статические и динамические анализаторы\. Начать использовать такие решения проще, чем может показаться\. Например, получить пробную версию анализатора PVS\-Studio можно [здесь](https://pvs-studio.ru/ru/pvs-studio/try-free/)\. Также существуют различные [сценарии бесплатного](https://pvs-studio.ru/ru/blog/posts/0614/) его использования\. Всем спасибо за чтение и хорошего дня\!