Godot: Regular use of static code analyzers
The 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
virtualboolcan_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
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
);
}
returntrue;
}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)
returnfalse;
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")) {
returnfalse;
}
}
....
}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 : 1In 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())
returnfalse; //may have been removedif (!from_node)
returnfalse;
....
}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)
returnfalse;
if (!from_node->is_inside_tree())
returnfalse; //may have been removedError n18
enum JoystickList {
....
JOY_AXIS_MAX = 10,
....
};
staticconstchar *_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).
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 .