Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some errors found by PVS-Studio tool #24014

Closed
AndreyKarpov opened this issue Nov 27, 2018 · 8 comments
Closed

Some errors found by PVS-Studio tool #24014

AndreyKarpov opened this issue Nov 27, 2018 · 8 comments

Comments

@AndreyKarpov
Copy link

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.

@reduz
Copy link
Member

reduz commented Nov 27, 2018

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

@akien-mga
Copy link
Member

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

@aaronfranke
Copy link
Member

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.

@vesperbot
Copy link

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.

@AndreyKarpov
Copy link
Author

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.

@akien-mga
Copy link
Member

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 2a230d5. 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 akien-mga added this to the 3.1 milestone Nov 28, 2018
@LikeLakers2
Copy link
Contributor

@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)

akien-mga added a commit to akien-mga/godot that referenced this issue Jan 16, 2019
Fixes items 10, 16 and 19 from PVS-Studio blog post
in godotengine#24014.
Jay2645 pushed a commit to Jay2645/godot that referenced this issue Jan 22, 2019
Fixes items 10, 16 and 19 from PVS-Studio blog post
in godotengine#24014.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants