Godot: Regular use of static code analyzers

Published on November 27, 2018

Godot: Regular use of static code analyzers

    PVS-Studio and GodotThe audience of our readers is growing, so we again and again write articles in which we explain how to correctly use the methodology of static code analysis. We consider it very important to explain that static analysis tools should be used not sporadically, but regularly. Once again, we will demonstrate this with a practical example, having rechecked the Godot project.

    Use analyzers regularly


    Preparing for a speech at a game developer conference, I decided to get new examples of interesting errors that the PVS-Studio tool can reveal . To this end, several game engines were tested, one of which was Godot. I did not find any particularly interesting mistakes for the report, but I wanted to write an article about ordinary mistakes. These errors very well demonstrate the relevance of regular use of static code analysis tools.

    It should be noted that we already checked this project in 2015, and the author worked with the errors we wrote out. Here you can see the corresponding commit.

    3 years have passed. The project has changed. The PVS-Studio analyzer has also changed, and new diagnostics have appeared in it. Therefore, it is not surprising that I easily and quickly managed to write out a sufficient number of error examples for writing this article.

    However, another is important. When developing Godot or any other project, new errors constantly appear and are corrected. Undetected errors "settle" in the code for a long time, and then many of them can be detected when running static code analysis. Because of this, sometimes there is a false feeling that static analyzers find only some uninteresting errors in rarely used parts of the code. Of course, the way it is, if you use the analyzer incorrectly and run it only from time to time, for example, shortly before the release of the release.

    Yes, when writing articles, we do one-time checks of open projects. But we have another goal. We want to demonstrate the capabilities of the code analyzer to detect defects. This task has little to do with improving the quality of the project code as a whole and reducing the costs associated with editing errors.

    Once again about the main thing. The meaning of static code analysis is not to find outdated errors. These old errors are usually minor, otherwise they would interfere with the users and they would have already been found and corrected. The meaning of static analysis in quickly eliminating errors in newly written or modified code. This reduces debugging time, the number of user complaints and, ultimately, reduces the budget of the developed project.

    We now turn to the bugs that readers of our publications are so fond of.

    Copy-Paste errors


    Let's see what I noticed while studying the PVS-Studio report. I’ll start with my favorite V501 diagnostics, which finds errors in almost every project we check :).

    Error 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 warning: V501 CWE-570 There are identical sub-expressions! Exists_export_template ("uwp_" + platform_infix + "_debug.zip", & err) 'to the left and right operator. export.cpp 1135

    Classic Copy-Paste Error. The function call is copied but not edited. The name of the second file being processed must end with "_release.zip".

    Errors 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 warning: V501 CWE-570 There are identical sub-expressions 'bnode-> statements [i] -> type == SL :: Node :: TYPE_CONTROL_FLOW' || 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 Warning: V501 CWE-570 There are identical sub-expressions' p_what == MainLoop :: NOTIFICATION_WM_FOCUS_OUT '||' operator. editor_spin_slider.cpp 157

    I think the errors are clearly visible and do not require any explanation. Exactly the same classic Copy-Paste, as in the first case.

    Error 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 Warning: V501 CWE-570 ||| operator. soft_body.cpp 399

    Here the first line was copied twice. But the number of the axis of coordinates was changed only in the second line. And they forgot to edit the third line. This is nothing but the “ Last Line Effect ”.

    Note. At the moment, in addition to the “Last Line Effect”, I have made the following interesting observations: “ The most dangerous function in the C / C ++ world ”, “ Evil lives in comparison functions". And now I will announce the new article, the writing of which I plan to do in the near future. The working title is" 0, 1, 2 ". It should be interesting and instructive. I invite you to subscribe to one of the channels not to miss: twitter , vk. com , instagram , telegram and old school rss .

    Error 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 Warning: V778 CWE-682 Two code fragments were found. Perhaps this is a typo and 'v_scroll' variable should be used instead of 'h_scroll'. scroll_container.cpp 249

    Regarding this code snippet, I do not have complete confidence that there is an error here. However, I agree with the analyzer that the second block looks very suspicious. Most likely, the code was written using Copy-Paste and in the second block of text they forgot to replace h_scroll with v_scroll .

    The code should probably be 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 N6

    Another case when a sufficiently large piece of code was copied and unsuccessfully changed. The line with the error is marked by me 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 warning: V522 CWE-476 Dereferencing of the null pointer 'E' might take place. shader_gles2.cpp 102

    Error detected indirectly. By analyzing the data flow, PVS-Studio has revealed that the E pointer may be zero at the time of its dereference.

    The error lies in the fact that in the copied code snippet forgot to replace in one place E on the C . Because of this error, the function works in a very strange way and does incomprehensible things.

    Typos


    Error N7 It is

    difficult to imagine for non-C or C ++ programmers that a typo can be made by writing the comma, * instead of the asterisk, and the code will be compiled. However it is.

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

    variable bmi.bmiHeader.biSizeImage is assigned the value of the variable dib_size.x . Next, the comma operator ',' is executed, whose priority is lower than that of the '=' operator. The result of the expression dib_size.y * 4 is not used at all.

    Instead of a comma in the expression should be used multiplication operator '*'. First, such an expression would make sense. Secondly, below there is a similar, but already correct version of the initialization of the same variable:

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

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

    Any index will be considered valid. To correct the error, you need to replace the operator || on && :

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

    This logical error was most likely due to inattention, so I tend to attribute it to typos.

    Exactly the same error can be observed in the same file below. The fault of reproduction errors, apparently, is Copy-Paste.

    A propagated error: V547 CWE-571 Expression 'idx> = 0 || idx <4 'is always true. variant_op.cpp 2527

    Error N10

    WTF?

    An example of an error from which one would like to exclaim: 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 warning: V621 CWE-835 Consider inspecting the 'for' operator. It is possible that you will not be executed at all. animation_blend_space_1d.cpp 113

    Note the condition for stopping the loop: i> p_at_index . It is always true, since the variable i is initialized with the value blend_points_used - 1 . In this case, from the two earlier checks it follows that blend_points_used> p_at_index .

    A condition can become false only when there is an overflow of the sign variable i , which is an indefinite behavior. Moreover, the overflow will not come, as it will happen much earlier to go beyond the array boundary.

    Before us, in my opinion, is a beautiful typo, when instead of '<' they wrote '>'. Yes, I have a perverted idea of ​​the beauty of mistakes :).

    Correct cycle:

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

    Error N11

    Another no less striking case of typos in the cycle 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 warning: 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 may be overstepping of the array because the value of i increases uncontrollably. Secure Code:

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

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

    Accidentally forgot to write a break statement . Therefore, when hit in a case of GDScriptParser :: DataType :: SCRIPT, variables will be assigned values ​​as if it were a case of GDScriptParser :: DataType :: GDSCRIPT .

    Error N13 The

    following error can be classified as Copy-Paste. However, I am not sure that such a short line was copied. So we will consider it a simple typo when typing.

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

    PVS-Studio warning: 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

    Assigning the same variable twice. Below you can see the following code snippet:

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

    Most likely, for the first case it should have been written the same way.

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

    Fragment from the V751 diagnostic description :

    The analyzer detected a suspicious function, one of the parameters of which is never used. However, its other parameter is used several times, which may indicate an error.

    As you can see, this is true, and it is very suspicious. The variable p_x is used twice and p_y is not used. Most likely, it should be written:

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

    By the way, in the source code the function call is written in one line. Because of this, the error is more difficult to notice. If the author of the code wrote the actual arguments in the column, as I did in the article, the error would immediately be evident. Do not forget that table formatting is very useful and allows you to avoid many typos. See the chapter “Align the code of the same type with a table” in the article “ The main issue of programming, refactoring and all that ”.

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

    In the loop, the same file is processed in all iterations of the loop. A typo here:

    String file = files[0];

    It should be:

    String file = files[i];

    Other errors


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

    The PVS-Studio analyzer immediately generates two responses to this code:

    • 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

    And indeed, this ternary operator in both expressions looks very strange:

    i >= 3 ? -1 : 1 

    In one case, the condition is always true, and in the other, false. It’s hard to say how this code should look right. Perhaps it is just redundant and you can write this:

    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 may be wrong, and the code must be corrected in a completely different way.

    Error N17

    Almost no V595 errors were found, although usually they are abundantly found in any project . Apparently, these errors were corrected after the previous check, and then errors of this type almost did not appear. I saw only a few false positives and one error.

    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 warning: V595 CWE-476 The 'from_node' 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 checked for equality to nullptr . Checks should be swapped:

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

    Error 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 warning: 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. In this case, the JOY_AXIS_MAX constant , which specifies the number of iterations of the loop, is 10. It turns out that the loop goes beyond the edge of the array.

    Error N19

    And the last one is a very strange function, which, apparently, is intended for testing something. The function is long, so I will give it in the form of a picture (click on the image to enlarge).

    Very strange function



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

    There are several unconditional return statements in the function . In the picture I marked them with red ovals. It seems that several different unit tests were collected into this function, but forgot to remove the extra return NULL . As a result, the function does not check what it should check. Almost the whole body of a function consists of unreachable code.

    Perhaps, of course, this is some tricky idea. But it seems to me that this happened randomly and the code should be fixed.

    On it let's finish. Surely, if you look closely at the analyzer report, you can find other errors. However, even written out with more than enough to write an article. Then it will become boring for me and the reader :).

    Conclusion


    The article describes errors that would not exist if the code was regularly analyzed using PVS-Studio. However, more importantly, using the analysis regularly, one would immediately find and fix many other errors. My colleague described this idea in more detail in a note: “The philosophy of static code analysis: we have 100 programmers, the analyzer found few errors, is it useless? “. I highly recommend spending 10 minutes reading this short, but very important article.

    Thanks for attention. I invite everyone to download and try the PVS-Studio static analysis to check your own projects.



    If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. Godot: On Regular Use of Static Analyzers .