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

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

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

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

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

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

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

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

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

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

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

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

Ошибка N1

virtual bool can_export(....)
{
  ....
  if (!exists_export_template("uwp_" + platform_infix + "_debug.zip", &err) ||
      !exists_export_template("uwp_" + platform_infix + "_debug.zip", &err)) {
    valid = false;
    r_missing_templates = true;
  }
  ....
}

Предупреждение 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 < 4' is always true. variant_op.cpp 2152

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

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

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

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

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

Ошибка N10

Пример ошибки, от которой так и хочется воскликнуть: 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 > p_at_index. Оно всегда истинно, так как переменная i инициализируется значением blend_points_used — 1. При этом из двух более ранних проверок следует, что 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 < X.size()' should be used instead of 'X.size()'. animation_state_machine_editor.cpp 852

Может произойти выход за границу массива, так как значение 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

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

Ошибка 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

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

Ошибка N19

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

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

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

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

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

Заключение

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

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

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