The American company Electronic Arts Inc (EA) has opened the source code of the games Command & Conquer: Tiberian Dawn and Command & Conquer: Red Alert publicly available. Several dozen errors were detected in the source code using the PVS-Studio analyzer, so, please, welcome the continuation of found defects review. Command & Conquer is a series of computer games in the real-time strategy genre. The first game in the series was released in 1995. The source code of the games was together with the release of the Command & Conquer Remastered collection. posted The analyzer was used to find errors in the code. The tool is designed to detect errors and potential vulnerabilities in the source code of programs, written in C, C++, C#, and Java. PVS-Studio Link to the first error overview: " " The Code of the Command & Conquer Game: Bugs from the 90's. Volume one Errors in conditions V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: 3072. STARTUP.CPP 1136 It turns out that users couldn't configure some settings. Or, rather, they did something but due to the fact that the ternary operator always returns a single value, nothing has actually changed. V590 Consider inspecting the 'i < 8 && i < 4' expression. The expression is excessive or contains a misprint. DLLInterface.cpp 2238 Due to an incorrect loop, the position is not set for all players. On the one hand, we see the constant and assume that this is the maximum number of players. On the other hand, we see the condition and the operator . So the loop never makes 8 iterations. Most likely, at the initial stage of development, the programmer hadn't used constants. When he started, he forgot to delete the old numbers from the code. MAX_PLAYERS 8 i < 4 && V648 Priority of the '&&' operation is higher than that of the '||' operation. INFANTRY.CPP 1003 You can make the code non-obvious (and most likely erroneous) simply by not specifying the priority of operations for the and operators. Here I can't really get if it's an error or not. Given the overall quality of the code for these projects, we can assume that here and in several other places, we will find errors related to operations priority: || && • V648 Priority of the '&&' operation is higher than that of the '||' operation. TEAM.CPP 456 • V648 Priority of the '&&' operation is higher than that of the '||' operation. DISPLAY.CPP 1160 • V648 Priority of the '&&' operation is higher than that of the '||' operation. DISPLAY.CPP 1571 • V648 Priority of the '&&' operation is higher than that of the '||' operation. HOUSE.CPP 2594 • V648 Priority of the '&&' operation is higher than that of the '||' operation. INIT.CPP 2541 V617 Consider inspecting the condition. The '((1L << STRUCT_CHRONOSPHERE))' argument of the '|' bitwise operation contains a non-zero value. HOUSE.CPP 5089 To check whether certain bits are set in a variable, use the & operator, not |. Due a typo in this code snippet, we have a condition that is always true here. V768 The enumeration constant 'WWKEY_RLS_BIT' is used as a variable of a Boolean-type. KEYBOARD.CPP 286 I think, in the parameter, the intention was to check a certain bit set by the mask, but the author made a typo. They should have used the & bitwise operator instead of && to check the key code. key WWKEY_RLS_BIT Suspicious formatting V523 The 'then' statement is equivalent to the 'else' statement. RADAR.CPP 1827 A developer once commented on code for debugging. Since then, a conditional operator with the same operators in different branches has remained in the code. Exactly the same two places were found: • V523 The 'then' statement is equivalent to the 'else' statement. CELL.CPP 1792 • V523 The 'then' statement is equivalent to the 'else' statement. RADAR.CPP 2274 V705 It is possible that 'else' block was forgotten or commented out, thus altering the program's operation logics. NETDLG.CPP 1506 Due to a large comment, the developer hasn't seen the above unfinished conditional operator. The remaining keyword forms the construction with the condition below, which most likely changes the original logic. else else if V519 The 'ScoresPresent' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 539, 541. INIT.CPP 541 Another potential defect due to incomplete refactoring. Now it is unclear whether the variable should be set to or . ScoresPresent true false Memory release errors V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] poke_data;'. CCDDE.CPP 410 The analyzer found an error related to the fact that memory can be allocated and released in incompatible ways. To free up memory allocated for an array, the operator should have been used instead of . delete[] delete There were several such places, and all of them gradually harm the running application (game): • V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] poke_data;'. CCDDE.CPP 416 • V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] temp_buffer;'. INIT.CPP 1302 • V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] progresspalette;'. MAPSEL.CPP 795 • V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] grey2palette;'. MAPSEL.CPP 796 • V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] poke_data;'. CCDDE.CPP 422 • V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] temp_buffer;'. INIT.CPP 1139 V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. ENDING.CPP 254 The and operators are separated for a reason. They perform different tasks to clear memory. When using an untyped pointer, the compiler doesn't know which data type the pointer is pointing to. In the C++ standard, the behavior of the compiler is uncertain. delete delete[] There was also a number of analyzer warnings of this kind: • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. HEAP.CPP 284 • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. INIT.CPP 728 • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MIXFILE.CPP 134 • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MIXFILE.CPP 391 • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MSGBOX.CPP 423 • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. SOUNDDLG.CPP 407 • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. BUFFER.CPP 126 • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. BUFF.CPP 162 • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. BUFF.CPP 212 • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. BFIOFILE.CPP 330 • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. EVENT.CPP 934 • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. HEAP.CPP 318 • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. INIT.CPP 3851 • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MIXFILE.CPP 130 • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MIXFILE.CPP 430 • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MIXFILE.CPP 447 • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MIXFILE.CPP 481 • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MSGBOX.CPP 461 • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. QUEUE.CPP 2982 • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. QUEUE.CPP 3167 • V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. SOUNDDLG.CPP 406 V773 The function was exited without releasing the 'progresspalette' pointer. A memory leak is possible. MAPSEL.CPP 258 The developer might have thought: ''If I don't free memory at all, I will definitely not make a mistake and will choose the correct operator''. But it results in a memory leak, which is also an error. Somewhere at the end of the function, memory gets released. Before that, there are many places with a conditional exit of the function, and memory by the and pointers isn't released. grey2palette progresspalett Other Issues V570 The 'hdr->MagicNumber' variable is assigned to itself. COMBUF.CPP 806 Two fields in the CommHdr structure are initialized with their own values. In my opinion, it's a meaningless operation, but it is executed many times: • V570 The 'hdr->Code' variable is assigned to itself. COMBUF.CPP 807 • V570 The 'hdr->MagicNumber' variable is assigned to itself. COMBUF.CPP 931 • V570 The 'hdr->Code' variable is assigned to itself. COMBUF.CPP 932 • V570 The 'hdr->MagicNumber' variable is assigned to itself. COMBUF.CPP 987 • V570 The 'hdr->Code' variable is assigned to itself. COMBUF.CPP 988 • V570 The 'obj' variable is assigned to itself. MAP.CPP 1132 • V570 The 'hdr->MagicNumber' variable is assigned to itself. COMBUF.CPP 910 • V570 The 'hdr->Code' variable is assigned to itself. COMBUF.CPP 911 • V570 The 'hdr->MagicNumber' variable is assigned to itself. COMBUF.CPP 1040 • V570 The 'hdr->Code' variable is assigned to itself. COMBUF.CPP 1041 • V570 The 'hdr->MagicNumber' variable is assigned to itself. COMBUF.CPP 1104 • V570 The 'hdr->Code' variable is assigned to itself. COMBUF.CPP 1105 • V570 The 'obj' variable is assigned to itself. MAP.CPP 1279 V591 Non-void function should return a value. HEAP.H 123 In the function of the class there is no operator. What's interesting is that the called function also has a return value of the type. Most likely, the programmer just forgot to write the statement and now the function returns an incomprehensible value. Free TFixedHeapClass return FixedHeapClass::Free int return V672 There is probably no need in creating the new 'damage' variable here. One of the function's arguments possesses the same name and this argument is a reference. Check lines: 1219, 1278. BUILDING.CPP 1278 The damage parameter is passed by reference. Therefore, the function body is expected to change the value of this variable. But at one point, the developer declared a variable with the same name. Because of this, the value instead of the function parameter is stored in the local damage variable . Perhaps different behavior was intended. 500 One more similar fragment: • V672 There is probably no need in creating the new 'damage' variable here. One of the function's arguments possesses the same name and this argument is a reference. Check lines: 4031, 4068. TECHNO.CPP 4068 V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'Occupy_List' in derived class 'BulletClass' and base class 'ObjectClass'. BULLET.H 90 The analyzer detected a potential error in overriding the virtual function. This may cause the wrong functions to be called at runtime. Occupy_List Some other suspicious fragments: • V762 It is possible a virtual function was overridden incorrectly. See qualifiers of function 'Ok_To_Move' in derived class 'TurretClass' and base class 'DriveClass'. TURRET.H 76 • V762 It is possible a virtual function was overridden incorrectly. See fourth argument of function 'Help_Text' in derived class 'HelpClass' and base class 'DisplayClass'. HELP.H 55 • V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'Draw_It' in derived class 'MapEditClass' and base class 'HelpClass'. MAPEDIT.H 187 • V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'Occupy_List' in derived class 'AnimClass' and base class 'ObjectClass'. ANIM.H 80 • V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'Overlap_List' in derived class 'BulletClass' and base class 'ObjectClass'. BULLET.H 102 • V762 It is possible a virtual function was overridden incorrectly. See qualifiers of function 'Remap_Table' in derived class 'BuildingClass' and base class 'TechnoClass'. BUILDING.H 281 • V762 It is possible a virtual function was overridden incorrectly. See fourth argument of function 'Help_Text' in derived class 'HelpClass' and base class 'DisplayClass'. HELP.H 58 • V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'Overlap_List' in derived class 'AnimClass' and base class 'ObjectClass'. ANIM.H 90 V763 Parameter 'coord' is always rewritten in function body before being used. DISPLAY.CPP 4031 The parameter is immediately overwritten in the function body. The old value was not used. This is very suspicious when a function has arguments and it doesn't depend on them. In addition, some coordinates are passed as well. coord So this fragment is worth checking out: • V763 Parameter 'coord' is always rewritten in function body before being used. DISPLAY.CPP 4251 V507 Pointer to local array 'localpalette' is stored outside the scope of this array. Such a pointer will become invalid. MAPSEL.CPP 757 There are a lot of global variables in the game code. Perhaps, it used to be a common approach to writing code back then. However, now it is considered bad and even dangerous. The InterpolationPalette pointer is stored in the local array localpalette, which will become invalid after exiting the function. A couple more dangerous places: • V507 Pointer to local array 'localpalette' is stored outside the scope of this array. Such a pointer will become invalid. MAPSEL.CPP 769 • V507 Pointer to local array 'buffer' is stored outside the scope of this array. Such a pointer will become invalid. WINDOWS.CPP 458 Conclusion As I wrote in the first report, let's hope that new Electronic Arts projects are of better quality. Now that game budgets are quite large, no one needs extra expenses to fix bugs in production. Speaking of which, fixing an error at an early stage of code writing does not take up much time and other resources. You are welcome to visit our site to and try PVS-Studio on all projects. download Source - https://www.viva64.com/en/b/0748/

Bugs from the 90's: The Code of Command and Conquer [Volume 2]

About Author

Comments

TOPICS

THIS ARTICLE WAS FEATURED IN

Related Stories

Bugs From the 90's: The Code of the Command & Conquer Game

#10 Rules of Bug Bounty

The Noonification: Do Leaders Really Need Emotional Intelligence? (10/14/2022)

3 Web Design Factors to Ponder Before Starting an E-commerce Venture

5 Common Web Development Mistakes and How to Avoid Them

51 Stories To Learn About Bugs

Bugs From the 90's: The Code of the Command & Conquer Game

#10 Rules of Bug Bounty

The Noonification: Do Leaders Really Need Emotional Intelligence? (10/14/2022)

3 Web Design Factors to Ponder Before Starting an E-commerce Venture

5 Common Web Development Mistakes and How to Avoid Them

51 Stories To Learn About Bugs

Light-Mode

Classic

Newspaper

Minty

Dark-Mode

Neon Noir

Minty

HN StartUps