Andrey Karpov

Nov 27 2018

Tags:

#Cpp #StaticAnalysis

Godot: On Regular Use of Static Analyzers

Nov 27 2018

Author: Andrey Karpov

Use analyzers on a regular basis
Bugs resulting from copy-paste
Typos
Miscellaneous
Conclusion

Because of our ever-expanding audience, we have to write new articles so that the new readers could learn about the correct way of using static code analysis. We believe that it is extremely important to get across the idea that static analysis tools must be used regularly, not sporadically. With this article, we'll try to demonstrate this just one more time by rechecking the Godot project as an example.

Use analyzers on a regular basis

When preparing for the game developers conference, I thought it would be a good idea to get new examples of some interesting bugs detected by PVS-Studio. For that purpose, I checked a number of game engines including Godot. I failed to find any particularly interesting cases for my lecture, but I did feel the urge to write an article about ordinary defects because they make a perfect example to illustrate the importance of regular use of static analysis tools.

As you may know, we already checked this project in 2015, and the authors fixed the bugs we had reported. Here's the commit.

It's been three years since then. The project has changed. PVS-Studio has changed too; it's got plenty of new diagnostics - no wonder it didn't take me long to collect a bunch of example bugs for this article.

But what matters is this. When developing Godot or any other project, developers are constantly adding new bugs and fixing them. Those that haven't been noticed "settle in" and stay in the code for a long time until they get uncovered through static analysis. This could create a false impression that static analyzers find only trivial defects in rarely used parts of the code. Yes, that's exactly what happens if you use the analyzer in a wrong way, that is run it only on single occasions, say, before releasing.

Sure, we, too, do one-time checks of open-source projects when writing our articles. But we have a different goal. We do this to showcase our tool's bug-detecting capabilities, and it has little to do with improving the overall quality of a given project and reducing the bug-fixing costs.

So, once again, the purpose of static analysis is not to detect long-standing bugs. After all, those are usually minor flaws since they would have otherwise showed up on the user side and got noticed and fixed. Static analysis is about promptly eliminating bugs in freshly written or modified code thus reducing the debugging time, the amount of user complaints, and eventually the development costs.

Now let's get to the bugs, which is what you like the most about our articles.

Bugs resulting from copy-paste

Let's see what I've managed to pick from the PVS-Studio report. I'll start with my favorite diagnostic, V501, which finds bugs in almost every project that we check :).

Error 1

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 diagnostic message: 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

This is a classic copy-paste defect: the programmer copied a function call but forgot to change it. The name of the second file to be processed should end with "_release.zip".

Errors 2, 3

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 diagnostic message: 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 diagnostic message: 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

This case is straightforward, and I don't think you need any comments from me. It's just the same classic type of copy-paste as in the previous case.

Error 4

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 diagnostic message: V501 CWE-570 There are identical sub-expressions to the left and to the right of the '||' operator. soft_body.cpp 399

The programmer copied the first line and pasted it twice but changed the axis number only in the second line while leaving the third one unchanged. That's "The Last Line Effect" in action.

Note. In addition to "the last line effect", I've also discovered a few other notable error patterns which I talk about in the following articles: "The most dangerous function in the C/C++ world", "The Evil within the Comparison Functions". And now I'm announcing a new article, which I plan to start writing shortly. Its working title is "0, 1, 2" and it promises to be both entertaining and enlightening. Stay tuned by subscribing to one of our channels: twitter, telegram, or "old-school" rss.

Error 5

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 diagnostic message: 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

I'm not sure if this snippet is faulty. But I do agree with the analyzer that the second block of code doesn't look right. And I'm almost sure this snippet was written using copy-paste and the programmer forgot to change h_scroll to v_scroll in the second block.

If so, the correct version should look like this:

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;

Error 6

This is yet another case where a fairly large code fragment was cloned with some of the lines left unchanged. I marked the flawed line with the "// <=" comment.

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 diagnostic message: V522 CWE-476 Dereferencing of the null pointer 'E' might take place. shader_gles2.cpp 102

The bug was detected in an indirect way: by employing dataflow analysis, PVS-Studio found that the E pointer could be null at the moment of its dereferencing.

The problem here is that the author of the code forgot to change one of the Es to a C in the copied fragment. This results in the function's weird behavior.

Typos

Error 7

Non-C/C++ programmers might find it amazing how one could accidentally write a comma (',') instead of an asterisk ('*') and still have the code compile. Yet that's how things are.

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 diagnostic message: V521 CWE-480 Such expressions using the ',' operator are dangerous. Make sure the expression is correct. os_windows.cpp 776

The bmi.bmiHeader.biSizeImage variable is assigned the value of the dib_size.x variable. After that, the comma operator (',') is executed, whose precedence is lower than that of the '=' operator. Finally, the dib_size.y * 4 expression is evaluated but its result is not used anywhere.

What the programmer really meant to use was the multiplication operator ('*'), not the comma. First, it makes the expression meaningful. Second, if you look further down the code, you'll see a similar initialization of the same variable but with the correct operator:

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

Errors 8, 9

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 diagnostic message: V547 CWE-571 Expression 'idx >= 0 || idx < 4' is always true. variant_op.cpp 2152

Any index will be considered correct. To fix this, the || operator should be replaced with &&:

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

This logical error must be stemming from inattention, so I'd call it a typo.

The same flaw can be found a bit further in the same file. Again, it looks like this bug was duplicated using copy-paste.

The bug is this: V547 CWE-571 Expression 'idx >= 0 || idx < 4' is always true. variant_op.cpp 2527

Error 10

There are bugs that almost make you cry out "WTF?!" This is one of those.

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 diagnostic message: 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

Note the loop termination condition: i > p_at_index. It's always true since the i variable is initialized to the value blend_points_used - 1. On the other hand, it follows from the two earlier checks that blend_points_used > p_at_index.

The condition can become false only if the signed variable i overflows, which is undefined behavior. But it wouldn't even get that far because there'll be an array overrun long before that.

Error 11

Here's another - no less cool - typo in a loop condition.

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 diagnostic message: 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

There's a risk of array overrun as the value of i is increasing uncontrollably. This is the safe version:

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

Error 12

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 diagnostic message: V796 CWE-484 It is possible that 'break' statement is missing in switch statement. gdscript_compiler.cpp 135

The programmer accidentally left out a break statement. As a result, when execution enters case GDScriptParser::DataType::SCRIPT, the variables would be assigned values as if it were case GDScriptParser::DataType::GDSCRIPT.

Error 13

This one could be classified as a copy-paste bug too, but I'm not sure the programmer would copy such a short line rather than write it from scratch, so let's treat it as a regular typo.

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

PVS-Studio diagnostic message: 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

One and the same variable is assigned values twice. The snippet above is followed by this code:

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

This snippet suggests that the previous one should look the same.

Error 14

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 diagnostic message: V751 Parameter 'p_y' is not used inside function body. texture.cpp 1085

Here's a quote from the description of the V751 diagnostic:

The analyzer detected a suspicious function where one of the parameters is never used while another parameter is used several times. It may indicate an error in the code.

As you can see, this is exactly what happens here and it does look suspicious: the p_x variable is used twice, while p_y is not used at all. The correct version should probably look like this:

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

By the way, the function call is originally written in one line, which makes the error harder to notice. If the author had arranged the arguments in a column, like I did here, they would have surely noticed the problem right away. Table-style formatting is a useful technique, which can prevent lots of typos - keep this in mind. For details, see the section "Table-style formatting" of the article "The Ultimate Question of Programming, Refactoring, and Everything".

Error 15

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 diagnostic message: V767 Suspicious access to element of 'files' array by a constant index inside a loop. sprite_frames_editor_plugin.cpp 602

The same file is processed at each loop iteration. The problem is in this line:

String file = files[0];

It should look like this:

String file = files[i];

Miscellaneous

Error 16

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);
    ....
  }
  ....
}

This code triggers two warnings at once:

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

Indeed, the ternary operator does looks strange in both expressions:

i >= 3 ? -1 : 1

The condition is always true in one case and always false in the other. I'm not certain about what exactly this code should look like. Perhaps it's just redundant and could be simplified as follows:

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];
  ....
}

I might be wrong about the way of fixing it though.

Error 17

I got almost zero V595 messages this time, though there are typically plenty of them in any project. Perhaps the developers fixed all such errors after the previous check and didn't make them anymore. I got only one genuine bug and a few false positives.

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 diagnostic message: 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

The from_node pointer is first dereferenced to call the is_inside_tree function and only then is checked for nullptr. The checks should be swapped:

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

Error 18

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 diagnostic message: V557 CWE-125 Array overrun is possible. The value of 'i' index could reach 9. input_default.cpp 1119

The _axes array consists of eight elements, while the value of the JOY_AXIS_MAX constant, which defines the number of loop iterations, is 10. So, this is an array overrun.

Error 19

The last bug for today. It has to do with a particularly strange function, which, I guess, is used for some check. It's long, so I'll attach a screenshot of it (click to enlarge).

PVS-Studio diagnostic message: V779 CWE-561 Unreachable code detected. It is possible that an error is present. test_math.cpp 457

There are several unconditional return statements - I circled them in red. It looks as if the authors composed this function of several different unit tests but forgot to delete the extra return NULL statements. As a result, the function doesn't check what it should. Most of its body is unreachable code.

This could be some deliberate trick, of course. But I have a strong suspicion that it's a mistake and it needs fixing.

Let's finish here. Perhaps I could've picked more examples to share with you, but we've already had more than enough for today. If we went on, it would start to get boring both for you and me :)

Conclusion

The defects described above wouldn't have ever existed if the code had been regularly checked with PVS-Studio. What's more important, though, is that regular use would have helped catch and fix tons of other defects right away. My colleague elaborates on this topic in his article "Philosophy of Static Code Analysis: We Have 100 Developers, the Analyzer Found Few Bugs, Is Analyzer Useless?". I highly recommend spending 10 minutes on reading this short yet extremely important piece.

Thanks for reading. Drop by our website to download PVS-Studio and try it with your own projects.

#Cpp #StaticAnalysis