Главная » Хабрахабр » Godot: к вопросу о регулярном использовании статических анализаторов кода

Godot: к вопросу о регулярном использовании статических анализаторов кода

PVS-Studio and GodotАудитория наших читателей растёт, поэтому мы вновь и вновь пишем статьи, в которых объясняем, как правильно использовать методологию статического анализа кода. Мы считаем очень важным объяснить, что инструменты статического анализа должны использоваться не эпизодически, а регулярно. В очередной раз продемонстрируем это на практическом примере, перепроверив проект Godot.

Используйте анализаторы регулярно

Готовясь к выступлению на конференции разработчиков игр, я решил обзавестись новыми примерами интересных ошибок, которые способен выявить инструмент PVS-Studio. С этой целью были проверены несколько игровых движков, одним из которых был Godot. Никаких особенно интересных ошибок для доклада я в нём не нашел, зато мне захотелось написать статью про обыкновенные ошибки. Эти ошибки очень хорошо демонстрируют актуальность регулярного использования инструментов статического анализа кода.

Здесь можно увидеть соответствующий коммит. Следует отметить, что мы уже проверяли этот проект в 2015 году, и автор поработал с выписанными нами ошибками.

Проект изменился. Прошло 3 года. Поэтому нет ничего удивительного, что я легко и быстро смог выписать достаточное количество примеров ошибок для написания этой статьи. Анализатор PVS-Studio тоже изменился, и в нём появились новые диагностики.

Ненайденные ошибки «оседают» в коде надолго, и затем многие из них могут быть выявлены при запуске статического анализа кода. Однако важно другое. При разработке Godot или любого другого проекта постоянно появляются и исправляются новые ошибки. Конечно, так оно и есть, если использовать анализатор неправильно и запускать его только время от времени, например, незадолго до выпуска релиза. Из-за этого иногда возникает ложное ощущение, что статические анализаторы находят только какие-то малоинтересные ошибки в редко используемых участках кода.

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

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

Теперь перейдём к багам, которые так любят читатели наших публикаций.

Ошибки из-за Copy-Paste

Давайте посмотрим, что я заметил, изучая отчёт PVS-Studio. Начну с моей любимой диагностики V501, которая находит ошибки практически в каждом проекте, который мы проверяем :).

Ошибка N1

virtual bool can_export(....)
....
}

Предупреждение PVS-Studio: V501 CWE-570 There are identical sub-expressions '!exists_export_template(«uwp_» + platform_infix + "_debug.zip", & err)' to the left and to the right of the '||' operator. export.cpp 1135

Вызов функции скопирован, но не отредактирован. Классическая Copy-Paste ошибка. Имя второго обрабатываемого файла должно заканчиваться на "_release.zip".

Ошибки N2, N3

static String dump_node_code(SL::Node *p_node, int p_level) { .... if (bnode->statements[i]->type == SL::Node::TYPE_CONTROL_FLOW || bnode->statements[i]->type == SL::Node::TYPE_CONTROL_FLOW) { code += scode; //use directly } else { code += _mktab(p_level) + scode + ";\n"; } ....
}

Предупреждение PVS-Studio: V501 CWE-570 There are identical sub-expressions 'bnode->statements[i]->type == SL::Node::TYPE_CONTROL_FLOW' to the left and to the right of the '||' operator. test_shader_lang.cpp 183

void EditorSpinSlider::_notification(int p_what) { if (p_what == MainLoop::NOTIFICATION_WM_FOCUS_OUT || p_what == MainLoop::NOTIFICATION_WM_FOCUS_OUT) { if (grabbing_spinner) { Input::get_singleton()->set_mouse_mode(Input::MOUSE_MODE_VISIBLE); grabbing_spinner = false; grabbing_spinner_attempt = false; } } ....
}

Предупреждение PVS-Studio: V501 CWE-570 There are identical sub-expressions 'p_what == MainLoop::NOTIFICATION_WM_FOCUS_OUT' to the left and to the right of the '||' operator. editor_spin_slider.cpp 157

Точно такой же классический Copy-Paste, как и в первом случае. Думаю, ошибки хорошо видны и не требуют каких-либо пояснений.

Ошибка N4

String SoftBody::get_configuration_warning() const { .... Transform t = get_transform(); if ((ABS(t.basis.get_axis(0).length() - 1.0) > 0.05 || ABS(t.basis.get_axis(1).length() - 1.0) > 0.05 || ABS(t.basis.get_axis(0).length() - 1.0) > 0.05)) { if (!warning.empty()) ....
}

Предупреждение PVS-Studio: V501 CWE-570 There are identical sub-expressions to the left and to the right of the '||' operator. soft_body.cpp 399

Но номер оси координат был изменён только во второй строке. Здесь первая строка была скопирована дважды. Это не что иное, как "Эффект последней строки". А третью строчку отредактировать забыли.

И сейчас я сделаю анонс новой статьи, написанием которой я планирую заняться в ближайшее время. Примечание. На данный момент, помимо «Эффекта последней строки», мною сделаны следующие интересные наблюдения: "Самая опасная функция в мире С/С++", "Зло живёт в функциях сравнения". Должно получиться интересно и поучительно. Рабочее название «0, 1, 2». Приглашаю подписываться на один из каналов, чтобы не пропустить: twitter, vk.com, Instagram, telegram и «олдскульный» rss.

Ошибка N5

void ScrollContainer::_notification(int p_what) { .... if (h_scroll->is_visible_in_tree() && h_scroll->get_parent() == this) size.y -= h_scroll->get_minimum_size().y; if (v_scroll->is_visible_in_tree() && v_scroll->get_parent() == this) size.x -= h_scroll->get_minimum_size().x; ....
}

Предупреждение PVS-Studio: V778 CWE-682 Two similar code fragments were found. Perhaps, this is a typo and 'v_scroll' variable should be used instead of 'h_scroll'. scroll_container.cpp 249

Однако я согласен с анализатором, что второй блок выглядит очень подозрительно. По поводу этого фрагмента кода у меня нет полной уверенности, что здесь есть ошибка. Скорее всего, код писался с помощью Copy-Paste и во втором блоке текста забыли заменить h_scroll на v_scroll.

Вероятно, код должен быть таким:

if (h_scroll->is_visible_in_tree() && h_scroll->get_parent() == this) size.y -= h_scroll->get_minimum_size().y; if (v_scroll->is_visible_in_tree() && v_scroll->get_parent() == this) size.x -= v_scroll->get_minimum_size().x;

Ошибка N6

Строчка с ошибкой помечена мною комментарием "// <=". Ещё один случай, когда был скопирован и неудачно изменён достаточно большой фрагмент кода.

void ShaderGLES2::bind_uniforms() { .... const Map<uint32_t, Variant>::Element *E = uniform_defaults.front(); while (E) { int idx = E->key(); int location = version->uniform_location[idx]; if (location < 0) { E = E->next(); continue; } Variant v; v = E->value(); _set_uniform_variant(location, v); E = E->next(); } const Map<uint32_t, CameraMatrix>::Element *C = uniform_cameras.front(); while (C) { int idx = E->key(); // <= int location = version->uniform_location[idx]; if (location < 0) { C = C->next(); continue; } glUniformMatrix4fv(location, 1, GL_FALSE, &(C->get().matrix[0][0])); C = C->next(); } uniforms_dirty = false;
}

Предупреждение PVS-Studio: V522 CWE-476 Dereferencing of the null pointer 'E' might take place. shader_gles2.cpp 102

Благодаря анализу потока данных PVS-Studio выявил, что указатель E может быть нулевым в момент его разыменования. Ошибка выявлена косвенным образом.

Из-за этой ошибки функция работает очень странным образом и делает непонятные вещи. Ошибка заключается в том, что в скопированном фрагменте кода забыли заменить в одном месте E на C.

Опечатки

Ошибка N7

Однако это так. Программистам, пишущим не на языке C или C++, сложно представить, что можно допустить опечатку, написав вместо звёздочки '*' запятую ',', и при этом код будет компилироваться.

LRESULT OS_Windows::WndProc(....) { .... BITMAPINFO bmi; ZeroMemory(&bmi, sizeof(BITMAPINFO)); bmi.bmiHeader.biSize = sizeof(BITMAPINFOHEADER); bmi.bmiHeader.biWidth = dib_size.x; bmi.bmiHeader.biHeight = dib_size.y; bmi.bmiHeader.biPlanes = 1; bmi.bmiHeader.biBitCount = 32; bmi.bmiHeader.biCompression = BI_RGB; bmi.bmiHeader.biSizeImage = dib_size.x, dib_size.y * 4; ....
}

Предупреждение PVS-Studio: V521 CWE-480 Such expressions using the ',' operator are dangerous. Make sure the expression is correct. os_windows.cpp 776

Далее выполняется оператор запятая ',', чей приоритет ниже, чем у оператора '='. Переменной bmi.bmiHeader.biSizeImage присваивается значение переменной dib_size.x. Результат же выражения dib_size.y * 4 никак не используется.

Во-первых, такое выражение будет иметь смысл. Вместо запятой в выражении должен использоваться оператор умножения '*'. Во-вторых, ниже есть похожий, но уже корректный вариант инициализации такой же переменной:

bmi.bmiHeader.biSizeImage = dib_size.x * dib_size.y * 4;

Ошибки N8, N9

void Variant::set(....) { .... int idx = p_index; if (idx < 0) idx += 4; if (idx >= 0 || idx < 4) { Color *v = reinterpret_cast<Color *>(_data._mem); (*v)[idx] = p_value; valid = true; } ....
}

Предупреждение PVS-Studio: V547 CWE-571 Expression 'idx >= 0 || idx

Чтобы исправить ошибку, надо заменить оператор || на &&: Любой индекс будет считаться корректным.

if (idx >= 0 && idx < 4) {

Эта логическая ошибка возникла, скорее всего, по невнимательности, поэтому я склонен отнести её к опечаткам.

Виной размножения ошибки, по всей видимости, является Copy-Paste. Точно такую же ошибку можно наблюдать в этом же файле чуть ниже.

variant_op.cpp 2527 Размноженная ошибка: V547 CWE-571 Expression 'idx >= 0 || idx < 4' is always true.

Ошибка N10

WTF?

Пример ошибки, от которой так и хочется воскликнуть: WTF?!

void AnimationNodeBlendSpace1D::add_blend_point( const Ref<AnimationRootNode> &p_node, float p_position, int p_at_index)
{ ERR_FAIL_COND(blend_points_used >= MAX_BLEND_POINTS); ERR_FAIL_COND(p_node.is_null()); ERR_FAIL_COND(p_at_index < -1 || p_at_index > blend_points_used); if (p_at_index == -1 || p_at_index == blend_points_used) { p_at_index = blend_points_used; } else { for (int i = blend_points_used - 1; i > p_at_index; i++) { blend_points[i] = blend_points[i - 1]; } } ....
}

Предупреждение PVS-Studio: V621 CWE-835 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. animation_blend_space_1d.cpp 113

Оно всегда истинно, так как переменная i инициализируется значением blend_points_used — 1. Обратите внимание на условие остановки цикла: i > p_at_index. При этом из двух более ранних проверок следует, что blend_points_used > p_at_index.

Более того, до переполнения не дойдёт, так как намного раньше произойдёт выход за границу массива. Условие может стать ложным, только когда возникнет переполнение знаковой переменной i, что является неопределённым поведением.

Да, у меня извращённое представление о красоте ошибок :). Перед нами, на мой взгляд, красивейшая опечатка, когда вместо '<' написали '>'.

Корректный цикл:

for (int i = blend_points_used - 1; i < p_at_index; i++) {

Ошибка N11

Ещё один не менее яркий случай опечатки в условии цикла.

void AnimationNodeStateMachineEditor::_state_machine_pos_draw() { .... int idx = -1; for (int i = 0; node_rects.size(); i++) { if (node_rects[i].node_name == playback->get_current_node()) { idx = i; break; } } ....
}

Предупреждение PVS-Studio: V693 CWE-835 Consider inspecting conditional expression of the loop. It is possible that 'i

Безопасный код: Может произойти выход за границу массива, так как значение i увеличивается бесконтрольно.

for (int i = 0; i < node_rects.size(); i++) {

Ошибка N12

GDScriptDataType GDScriptCompiler::_gdtype_from_datatype( const GDScriptParser::DataType &p_datatype) const
{ .... switch (p_datatype.kind) { .... case GDScriptParser::DataType::NATIVE: { result.kind = GDScriptDataType::NATIVE; result.native_type = p_datatype.native_type; } break; case GDScriptParser::DataType::SCRIPT: { result.kind = GDScriptDataType::SCRIPT; result.script_type = p_datatype.script_type; result.native_type = result.script_type->get_instance_base_type(); } case GDScriptParser::DataType::GDSCRIPT: { result.kind = GDScriptDataType::GDSCRIPT; result.script_type = p_datatype.script_type; result.native_type = result.script_type->get_instance_base_type(); } break; ....
}

Предупреждение PVS-Studio: V796 CWE-484 It is possible that 'break' statement is missing in switch statement. gdscript_compiler.cpp 135

Поэтому при попадании в case GDScriptParser::DataType::SCRIPT переменным будут присвоены значения, как будто это case GDScriptParser::DataType::GDSCRIPT. Случайно забыли написать оператор break.

Ошибка N13

Однако я не уверен, что столь короткая строка была скопирована. Следующую ошибку можно классифицировать как Copy-Paste. Так что будем считать это простой опечаткой при наборе текста.

void CPUParticles::_particles_process(float p_delta) { .... if (flags[FLAG_DISABLE_Z]) { p.velocity.z = 0.0; p.velocity.z = 0.0; } ....
}

Предупреждение PVS-Studio: V519 CWE-563 The 'p.velocity.z' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 664, 665. cpu_particles.cpp 665

Ниже можно увидеть вот такой фрагмент кода: Два раза происходит присваивание одной и той же переменной.

if (flags[FLAG_DISABLE_Z]) { p.velocity.z = 0.0; p.transform.origin.z = 0.0;
}

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

Ошибка N14

bool AtlasTexture::is_pixel_opaque(int p_x, int p_y) const { if (atlas.is_valid()) { return atlas->is_pixel_opaque( p_x + region.position.x + margin.position.x, p_x + region.position.y + margin.position.y ); } return true;
}

Предупреждение PVS-Studio: V751 Parameter 'p_y' is not used inside function body. texture.cpp 1085

Фрагмент из описания диагностики V751:

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

Переменная p_x используется дважды, а p_y не используется. Как видите, это действительно так, и это очень подозрительно. Скорее всего, должно быть написано:

return atlas->is_pixel_opaque( p_x + region.position.x + margin.position.x, p_y + region.position.y + margin.position.y
);

Кстати, в исходном коде вызов функции написан в одну строчку. Из-за этого ошибку сложнее заметить. Если бы автор кода написал фактические аргументы в столбик, как я сделал это в статье, то ошибка сразу бросилась бы в глаза. Не забывайте, что табличное форматирование весьма полезно и позволяет избежать множества опечаток. См. главу «Выравнивайте однотипный код „таблицей“ в статье „Главный вопрос программирования, рефакторинга и всего такого“.

Ошибка N15

bool SpriteFramesEditor::can_drop_data_fw(....) const { .... Vector<String> files = d["files"]; if (files.size() == 0) return false; for (int i = 0; i < files.size(); i++) { String file = files[0]; String ftype = EditorFileSystem::get_singleton()->get_file_type(file); if (!ClassDB::is_parent_class(ftype, "Texture")) { return false; } } ....
}

Предупреждение PVS-Studio: V767 Suspicious access to element of 'files' array by a constant index inside a loop. sprite_frames_editor_plugin.cpp 602

Опечатка здесь: В цикле обрабатывается один и тот же файл на всех итерациях цикла.

String file = files[0];

Должно быть:

String file = files[i];

Прочие ошибки

Ошибка N16

CSGBrush *CSGBox::_build_brush() { .... for (int i = 0; i < 6; i++) { .... if (i < 3) face_points[j][(i + k) % 3] = v[k] * (i >= 3 ? -1 : 1); else face_points[3 - j][(i + k) % 3] = v[k] * (i >= 3 ? -1 : 1); .... } ....
}

Анализатор PVS-Studio выдаёт сразу два срабатывания на этот код:

  • V547 CWE-570 Expression 'i >= 3' is always false. csg_shape.cpp 939
  • V547 CWE-571 Expression 'i >= 3' is always true. csg_shape.cpp 941

И действительно, вот этот тернарный оператор в обоих выражениях выглядит очень странно:

i >= 3 ? -1 : 1

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

for (int i = 0; i < 6; i++) { .... if (i < 3) face_points[j][(i + k) % 3] = v[k]; else face_points[3 - j][(i + k) % 3] = -v[k]; ....
}

Я могу быть неправ, и код надо исправить совсем другим способом.

Ошибка N17

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

bool CanvasItemEditor::_get_bone_shape(....) { .... Node2D *from_node = Object::cast_to<Node2D>( ObjectDB::get_instance(bone->key().from)); .... if (!from_node->is_inside_tree()) return false; //may have been removed if (!from_node) return false; ....
}

Предупреждение PVS-Studio: V595 CWE-476 The 'from_node' pointer was utilized before it was verified against nullptr. Check lines: 565, 567. canvas_item_editor_plugin.cpp 565

Проверки следует поменять местами: Указатель from_node вначале разыменовывается для вызова функции is_inside_tree и только затем проверятся на равенство nullptr.

if (!from_node) return false;
if (!from_node->is_inside_tree()) return false; //may have been removed

Ошибка N18

enum JoystickList { .... JOY_AXIS_MAX = 10, ....
}; static const char *_axes[] = { "Left Stick X", "Left Stick Y", "Right Stick X", "Right Stick Y", "", "", "L2", "R2"
}; int InputDefault::get_joy_axis_index_from_string(String p_axis) { for (int i = 0; i < JOY_AXIS_MAX; i++) { if (p_axis == _axes[i]) { return i; } } ERR_FAIL_V(-1);
}

Предупреждение PVS-Studio: V557 CWE-125 Array overrun is possible. The value of 'i' index could reach 9. input_default.cpp 1119

При этом константа JOY_AXIS_MAX, задающая количество итераций цикла, равна 10. Массив _axes состоит из восьми элементов. Получается, что в цикле происходит выход за границу массива.

Ошибка N19

Функция длинная, поэтому я приведу её в виде картинки (нажмите на картинку для увеличения). И последняя очень странная функция, которая, видимо, предназначена для тестирования чего-то.

Очень странная функция

It is possible that an error is present. Предупреждение PVS-Studio: V779 CWE-561 Unreachable code detected. test_math.cpp 457

На картинке я отметил их красными овалами. В функции встречается несколько безусловных операторов return. В результате функция не проверяет то, что должна проверять. Такое впечатление, что в эту функцию собрали несколько разных юнит-тестов, но забыли удалить лишние return NULL. Почти всё тело функции состоит из недостижимого кода.

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

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

Заключение

В статье описаны ошибки, которых бы не существовало, если бы код регулярно анализировался с помощью PVS-Studio. Однако, что ещё более важно, используя анализ регулярно, можно было бы сразу найти и устранить множество других ошибок. Более развернуто эту мысль описал мой коллега в заметке: „Философия статического анализа кода: у нас 100 программистов, анализатор нашел мало ошибок, он бесполезен?“. Очень рекомендую потратить 10 минут на прочтение этой короткой, но очень важной статьи.

Приглашаю всех скачать и попробовать статический анализ PVS-Studio для проверки ваших собственных проектов. Спасибо за внимание.

Godot: On Regular Use of Static Analyzers. Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov.


Оставить комментарий

Ваш email нигде не будет показан
Обязательные для заполнения поля помечены *

*

x

Ещё Hi-Tech Интересное!

Иди-ка ты сам на… или правила общения в команде

Пост-ответ на статью "Иди-ка ты на !@# со своей "токсичностью"". Если бы я последовал советам из этой статьи, мне достаточно было бы проявить эмоцию и сказать автору "Иди-ка ты сам на на ..., ты ничего не понимаешь!". Поэтому давайте разберем ...

[Перевод] Сделал редизайн — потерял миллиард

Исследуем эпичные провалы редизайна и мотаем на ус. Менеджер по продукту заходит в отдел дизайна и заказывает редизайн сайта. «Наш сайт выглядит таким старым! У всех наших конкурентов есть более яркие сайты. Давайте перепроектируем его. Кнопки с разноцветными тенями — ...