Разработка игр и их прохождение могут быть невероятно увлекательными и затягивающими занятиями, приносящими огромное удовольствие. Но ничто так не портит впечатление от игрового процесса, как коварно спрятавшийся баг. Поэтому сегодня под нашим пристальным вниманием окажется Open Source движок Godot Engine. Давайте проверим, насколько он хорош, и готов ли он подарить нам незабываемые эмоции от создания и прохождения игр.
Godot
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. С прошлой статьёй можно ознакомиться здесь.
Результаты проверки с помощью PVS-Studio
И первое, с чего бы хотелось начать после просмотра отчёта — это проблемы с макросами в проекте. Беда с ними в том, что параметры не оборачиваются в круглые скобки. Давайте рассмотрим несколько примеров, когда это может выстрелить в ногу.
Фрагменты N1-N2
#define HAS_WARNING(flag) (warning_flags & flag)Этот макрос нужен, чтобы проверить, выставлен ли определённый флаг предупреждения или нет.
Переменная warning_flags является побитовой маской и имеет тип uint_32t. Это значит, что её значение состоит из 32 битов, где каждому биту соответствует 1, если флаг выставлен, и 0, если нет. Макрос используется в условных операторах, где неявно преобразуется к типу bool. Для понимания работы рассмотрим упрощённый вариант, где вместо 32 бит будем использовать 8.
Предположим, у нас есть какой-нибудь флаг X, который соответствует 4-у биту в маске и в настоящий момент он поднят. Тогда значение переменной warning_flags в двоичной системе будет иметь следующий вид:
00001000Теперь предположим, что мы решили проверить с помощью нашего макроса выставлен ли флаг X.
Мы передаём в макрос переменную flag со значением 00001000 и в результате побитового "И" получаем ненулевое значение, которое преобразуется в bool со значением к true.
Теперь предположим, что мы захотели проверить флаг Y, который соответствует третьему биту, при том же значении переменной *warning_flags. *Мы передаём в макрос переменную со значением 00000100 и в результате побитового "И" получаем нулевое значение, которое преобразуется в bool со значением false.
Казалось бы, всё отлично, и что же может пойти не так. Но что, если кто-нибудь захочет проверить, выставлен ли один из нескольких флагов? Тогда он может позвать макрос так:
if (HAS_WARNING(flags::X | flags::Y)) ....И тогда результат такой операции всегда будет true, даже если ни один из флагов не выставлен. Почему так происходит? Давайте поработаем препроцессором и просто подставим переданное в макрос выражение:
if (warning_flags & flags::X | flags::Y) ....А теперь обратимся к таблице приоритетов операторов:
Приоритет | Оператор | Описание | Ассоциативность |
|---|---|---|---|
.... | .... | .... | .... |
11 | a & b | Побитовое И | Слева направо |
.... | .... | .... | .... |
13 | | | Побитовое ИЛИ | Слева направо |
Операторы перечислены сверху вниз в порядке убывания приоритета. Из этого следует, что выражение будет обработано следующим образом:
if (( warning_flags & flags::X ) | flags::Y) ....Допустим, в 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
И, как указано в сообщении, для исправления нужно всего лишь обернуть параметр макроса в скобки:
#define HAS_WARNING(flag) (warning_flags & (flag))Другой пример опасного макроса:
#define IS_SAME_ROW(i, row) (i / current_columns == row)Предупреждение анализатора:
V1003 The macro 'IS_SAME_ROW' is a dangerous expression. The parameters 'i', 'row' must be surrounded by parentheses. item_list.cpp 643
Если мы передадим в макрос вместо одной переменной какое-нибудь выражение, например, такое:
IS_SAME_ROW(current + 1, row)То в результате подстановки препроцессора получим:
(current + 1 / current_columns == row)Где порядок вычисления совсем не тот, который ожидали.
Чтобы обезопасить себя от таких ситуаций, достаточно обернуть параметры макроса в скобки:
#define IS_SAME_ROW(i, row) ((i) / current_columns == (row))Фрагмент N3
Теперь рассмотрим следующее условие:
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:
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 Expression is always true. material_storage.cpp 929
На самом деле программист хотел проверить, что p_texture_uniforms[i].hint находится в диапазоне от 5 до 9. Для этого надо применить логическое "И":
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
Попробуйте найти ошибку здесь самостоятельно:
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;
}
....
}
Предупреждение анализатора:
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.: на самом деле не следовало размножать код таким способом. Явно видно, что два блока кода отличаются только применяемым полем. Лучшим вариантом было бы вынести код в функцию и вызвать её дважды для разных полей:
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
Ещё условия, но уже вложенные:
void GridMapEditor::_mesh_library_palette_input(const Ref<InputEvent> &p_ie)
{
const Ref<InputEventMouseButton> 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 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
И куда же без классики — разыменование указателя до его проверки:
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 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
template <class T, class U = uint32_t,
bool force_trivial = false, bool tight = false>
class LocalVector
{
....
public:
operator Vector<T>() const
{
Vector<T> ret;
ret.resize(size());
T *w = ret.ptrw();
memcpy(w, data, sizeof(T) * count);
return ret;
}
....
};Предупреждение анализатора:
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>:
struct AnimationCompressionDataState
{
uint32_t components = 3;
LocalVector<uint8_t> data; // Committed packets.
struct PacketData
{
int32_t data[3] = { 0, 0, 0 };
uint32_t frame = 0;
};
float split_tolerance = 1.5;
LocalVector<PacketData> temp_packets;
// used for rollback if the new frame does not fit
int32_t validated_packet_count = -1;
....
};Класс 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:
operator Vector<T>() const
{
Vector<T> 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 <int> >: 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
Возможное нарушение программной логики:
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 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 не лежат нуль-терминированные строки.
Корректная проверка должна выглядеть так:
if ( is_digit(str[non_op])
|| ( str[non_op] == '.'
&& non_op + 1 < line_length
&& is_digit(str[non_op + 1]) ) )
{
in_number = true;
}Фрагмент N9
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 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. И это тоже может добавить сюрпризов, если в левом операнде будет отрицательное число.
Наиболее корректная проверка в случае участия выражений знаковых и беззнаковых типов одного ранга будет выглядеть следующим образом:
if (particles->amount >= 0 && particles->amount > lifetime_split)На самом деле мы с вами переизобрели std::cmp_greater, введённый в C++20, и, начиная с этой версии стандарта, можно написать лаконичный код:
if (std::cmp_greater(particles->amount, lifetime_split))Фрагмент N10
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 Loop break conditions do not depend on the number of iterations. animation_state_machine_editor.cpp 693
Цикл while длится ровно одну итерацию. Очень похоже на паттерн, когда из контейнера нужно взять только первый элемент, и это делается с помощью цикла for:
for (auto &&item : items)
{
DoSomething(item);
break;
}Таким образом, не нужно проверять, не содержит ли контейнер в себе первый элемент. ИМХО, такой код скорее запутывает, т.к. ожидаешь от циклов заведомо неизвестного конечного числа итераций.
Во фрагменте же выше цикл while абсолютно не имеет смысла. Достаточно было бы простой конструкции if:
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
static const char *script_list[][2] = {
....
{ "Myanmar / Burmese", "Mymr" },
{ "Nag Mundari", "Nagm" },
{ "Nandinagari", "Nand" },
....
}Читатель может задаться вопросом: "И что же здесь не так?" Мы бы и сами не поняли, если бы не срабатывание диагностического правила 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
Давайте внимательно посмотрим на следующую строку:
{ "Nag Mundari", "Nagm" },Именно в ней содержится закладка с невидимым символом. Если воспользоваться hex-редактором, то можно заметить следующее:
Между двойной кавычкой и символом N затесались 3 байта: E2, 80 и 8B. Они соответствуют Unicode-символу ZERO WIDTH SPACE (U+200B).
К счастью, наличие этого символа в строковом литерале не повлияет на логику программы.
Строки из массива script_list, в котором содержится "заражённый" строковый литерал, попадают в хэш-таблицу TranslationServer::script_map. Ключом такой хэш-таблицы будет второй строковый литерал из пары, а значением — первый. Значит, строковый литерал с закладкой попадёт в хэш-таблицу как значение, и поиск по хэш-таблице не нарушится.
Далее можно изучить, а куда потенциально может попасть это значение из хэш-таблицы. Я нашёл несколько мест:
Значение попадёт в строку, возвращаемую функцией TranslationServer::get_locale_name. Проанализировав вызывающие функции, видно, что эта строка так или иначе попадёт в GUI ([1], [2], [3], [4]).
Значение возвращается из функции TranslationServer::get_script_name. Проанализировав вызывающие функции, также можно сделать вывод, что строка попадёт в GUI ([1], [2]).
Вероятнее всего, закладка внеслась случайно в результате копирования названия с какого-нибудь сайта. Достаточно просто удалить этот символ из строкового литерала.
Фрагмент N12
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 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
void Image::initialize_data(int p_width, int p_height, bool p_use_mipmaps,
Format p_format, const Vector<uint8_t> &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 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++ это ведёт к неопределённому поведению.
Если нет никаких вспомогательных функций, которые проверяют переполнение, то самый простой вариант исправления может выглядеть так:
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
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 The function exited without calling the 'mutex.unlock' function. Check lines: 556, 438. remote_debugger.cpp 556
Крайне интересный фрагмент с многопоточным исполнением. Анализатор PVS-Studio обнаружил, что на некоторых путях исполнения мьютекс может быть не разблокирован. Давайте разбираться.
Начать нужно с того, какой тип мьютекса используется:
class RemoteDebugger : public EngineDebugger
{
....
private:
// Make handlers and send_message thread safe.
Mutex mutex;
....
};Копнём чуть глубже, что же это за Mutex:
template <class StdMutexT>
class MutexImpl
{
friend class MutexLock<MutexImpl<StdMutexT>>;
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<THREADING_NAMESPACE::recursive_mutex>;Итак, на самом деле перед нами не обычный мьютекс, а рекурсивный. Используют его совместно с кастомной RAII-обёрткой:
template <class MutexT>
class MutexLock
{
friend class ConditionVariable;
std::unique_lock<typename MutexT::StdMutexType> lock;
public:
_ALWAYS_INLINE_ explicit MutexLock(const MutexT &p_mutex)
: lock(p_mutex.mutex) {}
};Почти повсеместно мьютекс RemoteDebugger::mutex используется совместно с RAII-обёртками, покажу лишь пару мест: [1], [2], [3], ....
Однако в одном месте что-то пошло не так. Анализатор указал на место, где с мьютексом работают вручную. Из-за этого имеем несколько различных вариантов исполнения кода:
Мьютекс блокируется, цикл не выполняется ни разу (N == 0). В итоге поток управления покинет функцию RemoteDebugger::debug со счётчиком захвата, увеличенным на 1.
Мьютекс блокируется, цикл выполняется N == 1 раз. В этом случае всё будет хорошо — счётчик захвата рекурсивного мьютекса увеличивается и уменьшается на одинаковое число.
Мьютекс блокируется, цикл выполняется N > 1 раз. В итоге у рекурсивного мьютекса счётчик захвата уменьшится на N – 1 относительно момента до его ручной блокировки, что может привести к неопределённому поведению.
Если изучить вызовы функции is_peer_connected по кодовой базе ([1], [2], [3], ....), то во всех случаях они происходят под блокировкой RemoteDebugger::mutex. Судя по всему, программисту и в этом случае требовалась блокировка, но реализовал её он вручную.
На основе таких предположений можно поправить код следующим способом:
void RemoteDebugger::debug(....)
{
....
const auto is_peer_connected_sync = [this]
{
MutexLock _ { mutex };
return is_peer_connected();
};
while (is_peer_connected_sync())
{
....
}
....
}Не гарантирую, что исправление абсолютно корректно, т.к. только разработчики Godot знают, что здесь должно происходить. Но теперь мы хотя бы избавились от потенциального неопределённого поведения, связанного с разблокировкой мьютекса на каждой итерации цикла.
Заключение
Ошибки в коде встречаются самые разнообразные: от простых до сложных, от заметных до незаметных. Для того, чтобы не портить удовольствие и впечатление от продукта, его нужно постоянно очищать от багов и ошибок. С такой задачей хорошо справляются статические и динамические анализаторы.
Начать использовать такие решения проще, чем может показаться. Например, получить пробную версию анализатора PVS-Studio можно здесь. Также существуют различные сценарии бесплатного его использования.
Всем спасибо за чтение и хорошего дня!
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Alexey Smolskas. How to find job for Rescue Rangers: analyzing Godot Engine.
