Godot: Some errors found by PVS-Studio tool

Created on 27 Nov 2018  路  8Comments  路  Source: godotengine/godot

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: https://www.viva64.com/en/b/0594/

I hope it will help to fix some bugs.

bug confirmed hero wanted! junior job

Most helpful comment

Error 7 looked familiar, it was already fixed by me: dc2e734#diff-33050a8c8ed21f4806d591c06f251d48R776

What version of Godot was this analysis run on? The date says today, but the code it's looking at must be at least 40 days out of date.

Yes, it has been a while since the code check. I've been checking projects to find interesting material for a conference, then I was getting ready with a report and presented it. Only after that I took up the article. However, the outcome is quite favorable :). This confirms once again the idea that the analyzer should be applied regularly.

P.S. I suggest to go beyond the article and perform the analysis yourself. We're willing to provide a license for a month.

All 8 comments

Hi Andrey! Thanks a ton for your work, I am sure community will have fun squashing these bugs!

As noted in #24009, error 5 has been fixed already by #23735.

Error 7 looked familiar, it was already fixed by me: https://github.com/godotengine/godot/commit/dc2e73499a3264c6c77eb4ddadb6961d684c2941#diff-33050a8c8ed21f4806d591c06f251d48R776

What version of Godot was this analysis run on? The date says today, but the code it's looking at must be at least 40 days out of date.

Out of these errors, #10 is in some code that shifts array elements to the right, thus the for cycle should most likely be like this:

for (int i = blend_points_used; i > p_at_index; i--) {
  blend_points[i] = blend_points[i - 1];
}

Please verify.

Error 7 looked familiar, it was already fixed by me: dc2e734#diff-33050a8c8ed21f4806d591c06f251d48R776

What version of Godot was this analysis run on? The date says today, but the code it's looking at must be at least 40 days out of date.

Yes, it has been a while since the code check. I've been checking projects to find interesting material for a conference, then I was getting ready with a report and presented it. Only after that I took up the article. However, the outcome is quite favorable :). This confirms once again the idea that the analyzer should be applied regularly.

P.S. I suggest to go beyond the article and perform the analysis yourself. We're willing to provide a license for a month.

Regarding the remaining errors:

  • Error 10: What @vesperbot suggests seems good to me. Maybe @karroffel can confirm as he implemented BlendSpace1D.
  • Error 16: The solution proposed in the article seems correct, I assume that (i >= 3 ? -1 : 1) was used initially before the if (i < 3) conditional was added, and the code was not simplified.
  • Error 17: No need to be afraid of pointers, the current code is obviously wrong to check if the pointer is NULL after a check that deferences it. Swapping the tests (or chaining them in swapped order with ||) is correct.
  • Error 18: Bug introduced by @BastiaanOlij in 2a230d571d. The two extra axes should be named, probably with an empty string.
  • Error 19: All those return NULL; are indeed bogus, they should be removed. Then the math test should be run with godot --test math to see if everything breaks :)

@akien-mga In regards to error 19 (here's the patch I used, if you need it: error-19-testing.txt):

E:\Godot\Source Code>bin\godot.windows.tools.32.exe --test math
OpenGL ES 3.0 Renderer: GeForce GTX 1060 6GB/PCIe/SSE2
R: 256 G: 128 B: 26 EXP: 16
RGBE: 1, 0.5, 0.101563, 1
Dvectors: 0
Mem used: 0
MAx mem used: 1920000
0 : 0
1 : 1
2 : 2
3 : 3
4 : 4
5 : 5
6 : 6
7 : 7
8 : 8
9 : 9
10 : 10
11 : 11
12 : 12
13 : 13
14 : 14
15 : 15
16 : 16
17 : 17
18 : 18
19 : 19
later Dvectors: 1
later Mem used: 80
Mlater Ax mem used: 1920000
ERROR: Could not open file: math
   At: main\tests\test_math.cpp:500

(64-bit version generates the same output)

Was this page helpful?
0 / 5 - 0 ratings

Related issues

RebelliousX picture RebelliousX  路  3Comments

n-pigeon picture n-pigeon  路  3Comments

Zylann picture Zylann  路  3Comments

Spooner picture Spooner  路  3Comments

EdwardAngeles picture EdwardAngeles  路  3Comments