The American company Electronic Arts Inc (EA) has made the source code of the games Command & Conquer: Tiberian Dawn and Command & Conquer: Red Alert publicly available. This code should help the game community to develop mods and maps, create custom units, and customize the gameplay logic. We all now have a unique opportunity to plunge into the history of development, which is very different from the modern one. Back then, there was no StackOverflow site, convenient code editors, or powerful compilers. Moreover, at that time, there were no static analyzers, and the first thing the community will face is hundreds of errors in the code. This is what the PVS-Studio team will help you with by pointing out the erroneous places. Introduction 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 Electronic Arts company acquired this game's development studio only in 1998. Since then, several games and many mods have been released. The source code of the games was together with the release of the collection. posted Command & Conquer Remastered 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 Due to the large amount of problems found in the code, all error examples will be given in a series of two articles. Typos and copy-paste V501 There are identical sub-expressions to the left and to the right of the '||' operator: dest == 0 || dest == 0 CONQUER.CPP 5576 I'd like to start the review with the never ending copy-paste. The author hasn't checked the pointer for the source and checked the destination pointer twice, because they had copied the check and had forgotten to change the variable name. dest == NULL V584 The 'Current' value is present on both sides of the '!=' operator. The expression is incorrect or it can be simplified. CREDITS.CPP 173 The analyzer found a meaningless comparison. I suppose there must have been something as follows: but inattention has won. The exact same code fragment was copied to another function: • V584 The 'Current' value is present on both sides of the '!=' operator. The expression is incorrect or it can be simplified. CREDITS.CPP 246 V524 It is odd that the body of 'Mono_Y' function is fully equivalent to the body of 'Mono_X' function. MONOC.CPP 753 A larger piece of code that was copied with the consequences. You have to admit, that except using the analyzer, you won't be able notice that the function instead of was called from the function. The class does have 2 functions that differ by one symbol. Most likely, we found a real error. Get_X Get_Y Mono_Y MonoClass I found the identical piece of code below: • V524 It is odd that the body of 'Mono_Y' function is equivalent to the body of 'Mono_X' function. MONOC.CPP 1083 Errors with arrays V557 Array overrun is possible. The '9' index is pointing beyond array bound. FOOT.CPP 232 It seems that this is a debugging method, but the history doesn't record to what extent it could be detrimental for the developer's mental health. Here, the array consists of elements, and all of them are printed. Path 9 13 In total, 4 memory accesses outside the array boundary: • V557 Array overrun is possible. The '9' index is pointing beyond array bound. FOOT.CPP 232 • V557 Array overrun is possible. The '10' index is pointing beyond array bound. FOOT.CPP 233 • V557 Array overrun is possible. The '11' index is pointing beyond array bound. FOOT.CPP 234 • V557 Array overrun is possible. The '12' index is pointing beyond array bound. FOOT.CPP 235 V557 Array underrun is possible. The value of '_SpillTable[index]' index could reach -1. COORD.CPP 149 At first glance, the example is complex, but it is easy to puzzle it out after a brief analysis. The two-dimensional array is accessed by an index that is taken from the array. The array happens to contain negative values. Perhaps, access to data is organized according to a special formula and this is what the developer intended. Nonetheless, I'm not sure about that. _MoveSpillage _SpillTable V512 A call of the 'sprintf' function will lead to overflow of the buffer '(char *) ptr'. SOUNDDLG.CPP 250 An attentive reader will wonder - why such a long string is saved in a buffer of 4 bytes? This is because the programmer thought that would return something more (at least ). However, the operator returns the size of the type and even never evaluates any expressions. The author should have just written the constant , or better yet, used named constants, or a different type for strings or a pointer. sizeof(100) 100 sizeof 100 V512 A call of the 'memset' function will lead to underflow of the buffer 'Buffer'. KEYBOARD.CPP 96 A buffer is cleared by 256 bytes, although the full size of the original buffer is . Oops... goofed this one. 256*sizeof(unsigned short) It can be also fixed as follows: V557 Array overrun is possible. The 'QuantityB' function processes value '[0..86]'. Inspect the first argument. Check lines: 'HOUSE.H:928', 'CELL.CPP:2337'. HOUSE.H 928 There are a lot of global variables in the code and it is obvious that they are easy to get confused. The analyzer's warning about an array index out of bounds is issued at the point of accessing the array by index. The array size is 84 elements. Algorithms for analyzing the data flow in the analyzer helped to find out that the index value comes from another function – . There, a loop is executed with a final value of . Therefore, 12 bytes of "someone's" memory (3 elements) are constantly being read in this place. BQuantity Goodie_Check 86 int V575 The 'memset' function processes '0' elements. Inspect the third argument. DLLInterface.cpp 1103 In my opinion, I have repeatedly seen this error in modern projects. Programmers still confuse the 2nd and 3rd arguments of the function. memset One more similar fragment: V575 The 'memset' function processes '0' elements. Inspect the third argument. DLLInterface.cpp 1404 About null pointers V522 Dereferencing of the null pointer 'list' might take place. DISPLAY.CPP 1062 An explicit access to a null pointer looks very strange. This place looks like the one with a typo and there are a few other places worth checking out: • V522 Dereferencing of the null pointer 'list' might take place. DISPLAY.CPP 951 • V522 Dereferencing of the null pointer 'unitsptr' might take place. QUEUE.CPP 2362 • V522 Dereferencing of the null pointer 'unitsptr' might take place. QUEUE.CPP 2699 V595 The 'enemy' pointer was utilized before it was verified against nullptr. Check lines: 3689, 3695. TECHNO.CPP 3689 The pointer is dereferenced, and then checked to make sure that it is nonnull. It is still a vital problem, dare I say it, for every open source project. I'm sure that in projects with closed code the situation is about the same, unless, of course, PVS-Studio is used ;-) enemy Incorrect casts V551 The code under this 'case' label is unreachable. The '4109' value of the 'char' type is not in the range [-128; 127]. WINDOWS.CPP 547 This function handles the characters you enter. As you know, a 1-byte value is placed in the type, and the number will never be there. So, this statement just contains an unreachable code branch. char 4109 switch Several such places were found: • V551 The code under this 'case' label is unreachable. The '4105' value of the 'char' type is not in the range [-128; 127]. WINDOWS.CPP 584 • V551 The code under this 'case' label is unreachable. The '4123' value of the 'char' type is not in the range [-128; 127]. WINDOWS.CPP 628 V552 A bool type variable is being incremented: printedtext ++. Perhaps another variable should be incremented instead. ENDING.CPP 170 In this code fragment, the analyzer found the application of the increment operation to a variable of the type. This is correct code from the point of view of the language, but it looks very strange now. This operation is also marked as deprecated, starting from the C++17 standard. bool In total, 2 such places were detected: • V552 A bool type variable is being incremented: done ++. Perhaps another variable should be incremented instead. ENDING.CPP 187 V556 The values of different enum types are compared. Types: ImpactType, ResultType. AIRCRAFT.CPP 742 The programmer tied some logic to comparing the values of different enumerations. Technically, this works because numerical representations are compared. But such code often leads to logical errors. It is worth tweaking the code (of course, if this project is to be supported). The entire list of warnings for this diagnostic looks like this: • V556 The values of different enum types are compared: SoundEffectName[voc].Where == IN_JUV. DLLInterface.cpp 402 • V556 The values of different enum types are compared: SoundEffectName[voc].Where == IN_VAR. DLLInterface.cpp 405 • V556 The values of different enum types are compared: Map.Theater == CNC_THEATER_DESERT. Types: TheaterType, CnCTheaterType. DLLInterface.cpp 2805 • V556 The values of different enum types are compared. Types: ImpactType, ResultType. AIRCRAFT.CPP 4269 • V556 The values of different enum types are compared: SoundEffectName[voc].Where == IN_VAR. DLLInterface.cpp 429 V716 Suspicious type conversion in assign expression: 'HRESULT = BOOL'. GBUFFER.H 780. This is a very old problem that is still relevant today. There are special macros for working with the HRESULT type. Casting to BOOL and visa versa isn't used for this type. These two data types are extremely similar to each other from the point of view of the language, but logically they are still incompatible. The implicit type casting operation that exists in the code doesn't make sense. This and a few other places would be worth refactoring: • V716 Suspicious type conversion in assign expression: 'HRESULT = BOOL'. GBUFFER.H 817 • V716 Suspicious type conversion in assign expression: 'HRESULT = BOOL'. GBUFFER.H 857 • V716 Suspicious type conversion in assign expression: 'HRESULT = BOOL'. GBUFFER.H 773 • V716 Suspicious type conversion in assign expression: 'HRESULT = BOOL'. GBUFFER.H 810 • V716 Suspicious type conversion in assign expression: 'HRESULT = BOOL'. GBUFFER.H 850 V610 Undefined behavior. Check the shift operator '<<'. The left operand '(~0)' is negative. MP.CPP 2410 Here, the negative number is shifted to the left, which is undefined behavior. A negative number is obtained from zero when using the inversion operator. Since the result of the operation is placed in the type, the compiler uses it to store the value, whereas it is of the signed type. int In 2020, the compiler already finds this error as well: Warning C26453: Arithmetic overflow: Left shift of a negative signed number is undefined behavior. But compilers are not full-fledged static analyzers, because they solve other problems. So here is another example of undefined behavior that is only detected by PVS-Studio: V610 Undefined behavior. Check the shift operator '<<'. The right operand ('(32 - bits_to_shift)' = [1..32]) is greater than or equal to the length in bits of the promoted left operand. MP.CPP 659. The analyzer has found an unusual situation. The 32-bit number can be potentially shifted to the right for the number of bits, exceeding the available number. Here's how it works: The constant has the value : UNITIZE 32 Thus, the value of the variable will be zero for all values that are multiples of . bits_to_shift bits 32 Therefore, in this code fragment: 32 digits will be shifted if is subtracted from the constant . 0 32 List of all PVS-Studio warnings about shifts with undefined behavior: • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(~0)' is negative. TARGET.H 66 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 24) * 256) / 24)' is negative. ANIM.CPP 160 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 12) * 256) / 24)' is negative. BUILDING.CPP 4037 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 21) * 256) / 24)' is negative. DRIVE.CPP 2160 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 21) * 256) / 24)' is negative. DRIVE.CPP 2161 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 20) * 256) / 24)' is negative. DRIVE.CPP 2162 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 20) * 256) / 24)' is negative. DRIVE.CPP 2163 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 18) * 256) / 24)' is negative. DRIVE.CPP 2164 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 18) * 256) / 24)' is negative. DRIVE.CPP 2165 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 17) * 256) / 24)' is negative. DRIVE.CPP 2166 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 16) * 256) / 24)' is negative. DRIVE.CPP 2167 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 15) * 256) / 24)' is negative. DRIVE.CPP 2168 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 14) * 256) / 24)' is negative. DRIVE.CPP 2169 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 13) * 256) / 24)' is negative. DRIVE.CPP 2170 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 12) * 256) / 24)' is negative. DRIVE.CPP 2171 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 11) * 256) / 24)' is negative. DRIVE.CPP 2172 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 10) * 256) / 24)' is negative. DRIVE.CPP 2173 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 9) * 256) / 24)' is negative. DRIVE.CPP 2174 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 8) * 256) / 24)' is negative. DRIVE.CPP 2175 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 7) * 256) / 24)' is negative. DRIVE.CPP 2176 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 6) * 256) / 24)' is negative. DRIVE.CPP 2177 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 5) * 256) / 24)' is negative. DRIVE.CPP 2178 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 4) * 256) / 24)' is negative. DRIVE.CPP 2179 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 3) * 256) / 24)' is negative. DRIVE.CPP 2180 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 2) * 256) / 24)' is negative. DRIVE.CPP 2181 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 1) * 256) / 24)' is negative. DRIVE.CPP 2182 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 5) * 256) / 24)' is negative. INFANTRY.CPP 2730 • V610 Undefined behavior. Check the shift operator '>>'. The right operand ('(32 - bits_to_shift)' = [1..32]) is greater than or equal to the length in bits of the promoted left operand. MP.CPP 743 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(~0)' is negative. RANDOM.CPP 102 • V610 Undefined behavior. Check the shift operator '<<'. The left operand '(~0L)' is negative. RANDOM.CPP 164 Conclusion Let's hope that modern projects of Electronic Arts are of better quality. If not, we invite you to our site to and try PVS-Studio on all projects. download Someone may object that cool successful games used to be made with this quality, and we can partially agree with this. On the other hand, we mustn't forget that competition in the development of programs and games has grown many times over the years. Expenses on development, support, and advertising have also increased. Consequently, fixing errors at later stages of development can lead to significant financial and reputational losses. Follow our blog and don't miss the 2nd part of the review of this games series. Source - https://www.viva64.com/en/b/0741/

Flow

Target

Visa

Too Long; Didn't Read

In your car, at home, or at work — Bosch technology shapes many areas of life.

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

Too Long; Didn't Read

Companies Mentioned

Coin Mentioned

PVS-Studio

About Author

TOPICS

THIS ARTICLE WAS FEATURED IN...

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

Too Long; Didn't Read

Companies Mentioned

Coin Mentioned

PVS-Studio

About Author

TOPICS

THIS ARTICLE WAS FEATURED IN...

RELATED STORIES