Best bugs from games: fellow programmer's mistakes

The presentation shows how the PVS-Studio static analyzer finds errors in the code of well-known games. Possible ways to fix these errors are also demonstrated.
Take a look at an analysis our team made for a few open-source games: https://pvs-studio.com/en/blog/posts/csharp/1056/

Transcript

1. Best Bugs from Games: Fellow Programmers' Mistakes George Gribkov pvs-studio.com
2. 1. How we search for code errors 2. Examples and an overview of bugs found 3. Conclusion Contents 2
3. How We Search for Bugs 3
4. How We Search Bugs in Code 4 An up-to-date list of articles: https://pvs-studio.com/en/blog/inspections/ We check open-source projects regularly.
5. Examples and Overview of Bugs Found 5
6. System Shock (С) 6
7. fix Terrain( fix X, fix Y, int deriv ) { if( deriv == 0 ) return fix_mul( fix_make(0,0x2000), (X - fix_make(20,0) ) ); if( deriv == 1 ) return fix_mul( fix_make(0,0x2000), (X - fix_make(20,0) ) ); if( deriv == 2 ) return 0; return 0; } Example №1 7
8. fix Terrain( fix X, fix Y, int deriv ) { if( deriv == 0 ) return fix_mul( fix_make(0,0x2000), (X - fix_make(20,0) ) ); if( deriv == 1 ) return fix_mul( fix_make(0,0x2000), (X - fix_make(20,0) ) ); if( deriv == 2 ) return 0; return 0; } Example №1 8 V751 Parameter 'Y' is not used inside function body. BTEST.C 67
9. fix Terrain( fix X, fix Y, int deriv ) { if( deriv == 0 ) return fix_mul( fix_make(0,0x2000), (X - fix_make(20,0) ) ); if( deriv == 1 ) return fix_mul( fix_make(0,0x2000), (Y - fix_make(20,0) ) ); if( deriv == 2 ) return 0; return 0; } Example №1 9 V751 Parameter 'Y' is not used inside function body. BTEST.C 67
10. // And here, ladies and gentlemen, // is a celebration of C and C++ and their untamed passion... // ================== TerrainData terrain_info; // Now the actual stuff... // ======================= Funny Comments 10
11. // it's a wonderful world, with a lot of strange men // who are standing around, and they all wearing towels // Returns whether or not in the humble opinion of the // sound system, the sample should be politely // obliterated out of existence Funny Comments 11
12. Space Engineers (C#) 12
13. if (....) MySandboxGame.Log.WriteLine(string.Format( "Could not find any sound for '{0}'", cueName)); else { if (....) string.Format( "Could not find arcade sound for '{0}'", cueName); if (....) string.Format( "Could not find realistic sound for '{0}'", cueName); } Example №1 13
14. if (....) MySandboxGame.Log.WriteLine(string.Format( "Could not find any sound for '{0}'", cueName)); else { if (....) string.Format( "Could not find arcade sound for '{0}'", cueName); if (....) string.Format( "Could not find realistic sound for '{0}'", cueName); } Example №1 14 V3010 The return value of function 'Format' is required to be utilized. Sandbox.Game MyEntity3DSoundEmitter.cs 72, 74
15. if (....) MySandboxGame.Log.WriteLine(string.Format( "Could not find any sound for '{0}'", cueName)); else { if (....) MySandboxGame.Log.WriteLine(string.Format( "Could not find arcade sound for '{0}'", cueName)); if (....) MySandboxGame.Log.WriteLine(string.Format( "Could not find realistic sound for '{0}'", cueName)); } Example №1 15 V3010 The return value of function 'Format' is required to be utilized. Sandbox.Game MyEntity3DSoundEmitter.cs 72, 74
16. var actionsItem = item as MyToolbarItemActions; if (item != null) { if (idx < 0 || idx >= actionsItem .PossibleActions(....) .Count) RemoveToolbarItem(slot); .... } Example №2 16
17. var actionsItem = item as MyToolbarItemActions; if (item != null) { if (idx < 0 || idx >= actionsItem .PossibleActions(....) .Count) RemoveToolbarItem(slot); .... } Example №2 17 V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'item', 'actionsItem'. Sandbox.Game MyGuiControlToolbar.cs 511
18. var actionsItem = item as MyToolbarItemActions; if (item != null) { if (idx < 0 || idx >= actionsItem .PossibleActions(....) .Count) RemoveToolbarItem(slot); .... } Example №2 18 V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'item', 'actionsItem'. Sandbox.Game MyGuiControlToolbar.cs 511
19. var actionsItem = item as MyToolbarItemActions; if (actionsItem != null) { if (idx < 0 || idx >= actionsItem .PossibleActions(....) .Count) RemoveToolbarItem(slot); .... } Example №2 19 V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'item', 'actionsItem'. Sandbox.Game MyGuiControlToolbar.cs 511
20. C&C: Tiberian Dawn и C&C: Red Alert (C++) 20
21. // Maximum number of multi players possible. #define MAX_PLAYERS 8 // max # of players we can have for (int i = 0; i < MAX_PLAYERS && i < 4; i++) { if (GlyphxPlayerIDs[i] == player_id) { MultiplayerStartPositions[i] = XY_Cell(x, y); } } Example №1 21
22. // Maximum number of multi players possible. #define MAX_PLAYERS 8 // max # of players we can have for (int i = 0; i < MAX_PLAYERS && i < 4; i++) { if (GlyphxPlayerIDs[i] == player_id) { MultiplayerStartPositions[i] = XY_Cell(x, y); } } Example №1 22 V590 Consider inspecting the 'i < 8 && i < 4' expression. The expression is excessive or contains a misprint. DLLInterface.cpp 2238
23. // Maximum number of multi players possible. #define MAX_PLAYERS 8 // max # of players we can have for (int i = 0; i < MAX_PLAYERS || i < 4; i++) { if (GlyphxPlayerIDs[i] == player_id) { MultiplayerStartPositions[i] = XY_Cell(x, y); } } Example №1 23 V590 Consider inspecting the 'i < 8 && i < 4' expression. The expression is excessive or contains a misprint. DLLInterface.cpp 2238
24. void * ptr = new char [sizeof(100)]; if (ptr) { sprintf((char *)ptr, "%cTrack %d\t%d:%02d\t%s", ....); listbox.Add_Item((char const *)ptr); } Example №2 24
25. void * ptr = new char [sizeof(100)]; if (ptr) { sprintf((char *)ptr, "%cTrack %d\t%d:%02d\t%s", ....); listbox.Add_Item((char const *)ptr); } Example №2 25 V512 A call of the 'sprintf' function will lead to overflow of the buffer '(char *) ptr'. SOUNDDLG.CPP 250
26. void * ptr = new char [100]; if (ptr) { sprintf((char *)ptr, "%cTrack %d\t%d:%02d\t%s", ....); listbox.Add_Item((char const *)ptr); } Example №2 26 V512 A call of the 'sprintf' function will lead to overflow of the buffer '(char *) ptr'. SOUNDDLG.CPP 250
28. Doom 3 (C++) 28
29. for ( j = 0; j < w.GetNumPoints(); j++ ) { for ( k = 0; k < verts.Num(); j++ ) { if ( verts[k].xyz.Compare(w[j].ToVec3(), POLYTOPE_VERTEX_EPSILON)) { break; } } ... } Example №1 29
30. for ( j = 0; j < w.GetNumPoints(); j++ ) { for ( k = 0; k < verts.Num(); j++ ) { if ( verts[k].xyz.Compare(w[j].ToVec3(), POLYTOPE_VERTEX_EPSILON)) { break; } } ... } Example №1 30 V533 It is likely that a wrong variable is being incremented inside the 'for' operator. Consider reviewing 'j'. idLib surface_polytope.cpp 65
31. for ( j = 0; j < w.GetNumPoints(); j++ ) { for ( k = 0; k < verts.Num(); k++ ) { if ( verts[k].xyz.Compare(w[j].ToVec3(), POLYTOPE_VERTEX_EPSILON)) { break; } } ... } Example №1 31 V533 It is likely that a wrong variable is being incremented inside the 'for' operator. Consider reviewing 'j'. idLib surface_polytope.cpp 65
32. void idBrushBSP::FloodThroughPortals_r (idBrushBSPNode *node, ...) { ... if ( node->occupied ) { common->Error( "Node already occupied\n" ); } if ( !node ) { common->Error( "NULL node\n" ); } ... } Example №2 32
33. void idBrushBSP::FloodThroughPortals_r (idBrushBSPNode *node, ...) { ... if ( node->occupied ) { common->Error( "Node already occupied\n" ); } if ( !node ) { common->Error( "NULL node\n" ); } ... } Example №2 33 V595 The 'node' pointer was utilized before it was verified against nullptr. Check lines: 1421, 1424. DoomDLL brushbsp.cpp 1421
34. Example №2 34
35. void idBrushBSP::FloodThroughPortals_r (idBrushBSPNode *node, ...) { ... if ( node->occupied ) { common->Error( "Node already occupied\n" ); } if ( !node ) { common->Error( "NULL node\n" ); } ... } Example №2 35 V595 The 'node' pointer was utilized before it was verified against nullptr. Check lines: 1421, 1424. DoomDLL brushbsp.cpp 1421
36. void idBrushBSP::FloodThroughPortals_r (idBrushBSPNode *node, ...) { ... if ( !node ) { common->Error( "NULL node\n" ); } if ( node->occupied ) { common->Error( "Node already occupied\n" ); } ... } Example №2 36 V595 The 'node' pointer was utilized before it was verified against nullptr. Check lines: 1421, 1424. DoomDLL brushbsp.cpp 1421
37. osu! (C#) 37
38. public RulesetInfo GetRuleset(int id) => AvailableRulesets.FirstOrDefault(....); .... public ScoreInfo CreateScoreInfo(RulesetStore rulesets) { var ruleset = rulesets.GetRuleset(OnlineRulesetID); var mods = Mods != null ? ruleset.CreateInstance().GetAllMods().Where(....).ToArray() : Array.Empty<Mod>(); .... } Example №1 38
39. public RulesetInfo GetRuleset(int id) => AvailableRulesets.FirstOrDefault(....); .... public ScoreInfo CreateScoreInfo(RulesetStore rulesets) { var ruleset = rulesets.GetRuleset(OnlineRulesetID); var mods = Mods != null ? ruleset.CreateInstance().GetAllMods().Where(....).ToArray() : Array.Empty<Mod>(); .... } Example №1 39 V3146 Possible null dereference of 'ruleset'. The 'FirstOrDefault' can return default null value. APILegacyScoreInfo.cs 24
40. public RulesetInfo GetRuleset(int id) => AvailableRulesets.FirstOrDefault(....); .... public ScoreInfo CreateScoreInfo(RulesetStore rulesets) { var ruleset = rulesets.GetRuleset(OnlineRulesetID); var mods = (Mods != null && ruleset != null) ? ruleset.CreateInstance().GetAllMods().Where(....).ToArray() : Array.Empty<Mod>(); .... } Example №1 40 V3146 Possible null dereference of 'ruleset'. The 'FirstOrDefault' can return default null value. APILegacyScoreInfo.cs 24
41. private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * ((IOsuScreen)next)?.BackgroundParallaxAmount ?? 1.0f; } Example №2 41
42. private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * ((IOsuScreen)next)?.BackgroundParallaxAmount ?? 1.0f; } Example №2 42 V3123 Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. OsuScreenStack.cs 45
43. private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * ((IOsuScreen)next)?.BackgroundParallaxAmount ?? 1.0f; } Example №2 43 V3123 Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. OsuScreenStack.cs 45
44. private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * ((IOsuScreen)next)?.BackgroundParallaxAmount ?? 1.0f; } Example №2 44 V3123 Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. OsuScreenStack.cs 45
45. private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * ((IOsuScreen)next)?.BackgroundParallaxAmount ?? 1.0f; } Example №2 45 V3123 Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. OsuScreenStack.cs 45 x = (c * a) ?? b;
46. Example №2 46 What if ((IOsuScreen)next) is null?
47. private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * ((IOsuScreen)next)?.BackgroundParallaxAmount ?? 1.0f; } Example №2 47 V3123 Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. OsuScreenStack.cs 45 x = (c * a) ?? b;
48. private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * (null)?.BackgroundParallaxAmount ?? 1.0f; } Example №2 48 V3123 Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. OsuScreenStack.cs 45 x = (c * a) ?? b;
49. private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * null ?? 1.0f; } Example №2 49 V3123 Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. OsuScreenStack.cs 45 x = (c * null) ?? b;
50. private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = null ?? 1.0f; } Example №2 50 V3123 Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. OsuScreenStack.cs 45 x = null ?? b;
51. private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = 1.0f; } Example №2 51 V3123 Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. OsuScreenStack.cs 45 x = b;
52. Example №2 52 An error detected!!!
53. private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * ((IOsuScreen)next)?.BackgroundParallaxAmount ?? 1.0f; } Example №2 53 V3123 Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. OsuScreenStack.cs 45
54. private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * (((IOsuScreen)next)?.BackgroundParallaxAmount ?? 1.0f); } Example №2 54 V3123 Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. OsuScreenStack.cs 45 x = c * (a ?? b);
55. private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * (null?.BackgroundParallaxAmount ?? 1.0f); } Example №2 55 V3123 Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. OsuScreenStack.cs 45 x = c * (a ?? b);
56. private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * (null ?? 1.0f); } Example №2 56 V3123 Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. OsuScreenStack.cs 45 x = c * (null ?? b);
57. private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT * 1.0f; } Example №2 57 V3123 Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. OsuScreenStack.cs 45 x = c * b;
58. private void onScreenChange(IScreen prev, IScreen next) { parallaxContainer.ParallaxAmount = ParallaxContainer.DEFAULT_PARALLAX_AMOUNT; } Example №2 58 V3123 Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. OsuScreenStack.cs 45 x = c * b;
59. VVVVVV (C++) 59
60. TiXmlElement *pElem; .... pElem = hDoc.FirstChildElement().Element(); if (!pElem) { printf("No valid root! Corrupt level file?\n"); } pElem->QueryIntAttribute("version", &version); Example №1 60
61. TiXmlElement *pElem; .... pElem = hDoc.FirstChildElement().Element(); if (!pElem) { printf("No valid root! Corrupt level file?\n"); } pElem->QueryIntAttribute("version", &version); Example №1 61 V1004 The 'pElem' pointer was used unsafely after it was verified against nullptr. Check lines: 1739, 1744. editor.cpp 1744
62. TiXmlElement *pElem; .... pElem = hDoc.FirstChildElement().Element(); if (!pElem) { printf("No valid root! Corrupt level file?\n"); return; } pElem->QueryIntAttribute("version", &version); Example №1 62 V1004 The 'pElem' pointer was used unsafely after it was verified against nullptr. Check lines: 1739, 1744. editor.cpp 1744
63. TiXmlElement *pElem; .... pElem = hDoc.FirstChildElement().Element(); if (!pElem) { printf("No valid root! Corrupt level file?\n"); return; // You could also use throw } pElem->QueryIntAttribute("version", &version); Example №1 63 V1004 The 'pElem' pointer was used unsafely after it was verified against nullptr. Check lines: 1739, 1744. editor.cpp 1744
64. Terrible switch 64 ▪ V2008 Cyclomatic complexity: 548. Consider refactoring the 'Game::updatestate' function. Game.cpp 612
65. Terrible switch 65
66. Terrible switch 66
67. Terrible switch 67
68. Terrible switch 68
69. Terrible switch 69
70. Terrible switch 70
71. Terrible switch 71
72. Terrible switch 72 ▪ 3339 lines ▪ Almost 300 case-branches ▪ Not a single enum constant
73. Conclusion 73
74. ▪ Programmers could avoid errors using static analysis ▪ The illustrated examples are just the tip of the iceberg Conclusion 74
75. Q&A George Gribkov pvs-studio.com