image1.png

The American company Electronic Arts Inc (EA) has published in open access the source code of the games Command & amp; Conquer: Tiberian Dawn and Command & amp; Conquer: Red Alert. This code should help gaming communities develop mods and maps, create custom units, and customize gameplay logic. We all have a unique opportunity to plunge into the history of development, which is very different from the modern one. Then there was no StackOverflow site, convenient code editors and powerful compilers. And then there were no static analyzers, and the first thing the community will encounter is hundreds of errors in the code. But the PVS-Studio team will help with this, pointing out the places of these errors.

Introduction


Command & amp; Conquer is a series of computer games in the genre of real-time strategy. The first game in the series was released in 1995. Electronic Arts acquired the development studio for this game only in 1998.

Since then, several games and many mods have been released. The source code for the games was published along with the release of the collection Command & amp; Conquer Remastered .

To find errors in the code, the analyzer PVS-Studio was used. This is a tool for detecting errors and potential vulnerabilities in the source code of programs written in C, C++, C # and Java.

Due to the large volume of problems found in the code, all examples of errors will be given in a cycle 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

void List_Copy(short const * source, int len, short * dest) { if (dest == NULL || dest == NULL) { return; } .... } 

I want to start the review with the eternal copy-paste. In the function for copying lists, we never checked the source pointer and checked the destination pointer 2 times, because we copied the dest == NULL check and forgot to replace the variable name.

V584 The 'Current' value is present on both sides of the '! =' Operator. The expression is incorrect or it can be simplified. CREDITS.CPP 173

void CreditClass::AI(bool forced, HouseClass *player_ptr, bool logic_only) { .... long adder=Credits - Current; adder=ABS(adder); adder >>= 5; adder=Bound(adder, 1L, 71+72); if (Current > Credits) adder=-adder; Current += adder; Countdown=1; if (Current-adder != Current) {//<= IsAudible=true; IsUp=(adder > 0); } .... } 

The analyzer found a meaningless comparison. I suppose there should have been something like:

if (Current-adder != Credits) 

but inattention won.

Exactly the 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

class MonoClass { .... int Get_X(void) const {return X;}; int Get_Y(void) const {return Y;}; .... } int Mono_X(void) { if (MonoClass::Is_Enabled()) { MonoClass *mono=MonoClass::Get_Current(); if (!mono) { mono=new MonoClass(); mono->View(); } return(short)mono->Get_X();//<= } return(0); } int Mono_Y(void) { if (MonoClass::Is_Enabled()) { MonoClass *mono=MonoClass::Get_Current(); if (!mono) { mono=new MonoClass(); mono->View(); } return(short)mono->Get_X();//<= Get_Y() ? } return(0); } 

A more voluminous piece of code that was copied with the consequences. Agree, except with the help of the analyzer, you will not notice that from the function Mono_Y they called the function Get_X , instead of Get_Y . The MonoClass class does have 2 functions that differ by one character. Most likely, we found a real mistake.

And below the file there was an identical code fragment:

  • V524 It is odd that the body of 'Mono_Y' function is fully 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

#define CONQUER_PATH_MAX 9//Number of cells to look ahead for movement. FacingType Path[CONQUER_PATH_MAX]; void FootClass::Debug_Dump(MonoClass *mono) const { .... if (What_Am_I() != RTTI_AIRCRAFT) { mono->Set_Cursor(50, 3); mono->Printf("%s%s%s%s%s%s%s%s%s%s%s%s", Path_To_String(Path[0]), Path_To_String(Path[1]), Path_To_String(Path[2]), Path_To_String(Path[3]), Path_To_String(Path[4]), Path_To_String(Path[5]), Path_To_String(Path[6]), Path_To_String(Path[7]), Path_To_String(Path[8]), Path_To_String(Path[9]), Path_To_String(Path[10]), Path_To_String(Path[11]), Path_To_String(Path[12])); .... } .... } 

It seems that this is a debugging method, but how much harm he could inflict on the psyche of the developer, the story is silent. Here, the Path array consists of 9 elements, and all 13 are printed.

Total 4 memory accesses abroad array:

  • 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

typedef enum FacingType : char { .... FACING_COUNT,//8 FACING_FIRST=0 } FacingType; short const * Coord_Spillage_List(COORDINATE coord, int maxsize) { static short const _MoveSpillage[(int)FACING_COUNT+1][5]={ .... }; static char const _SpillTable[16]={8,6,2,-1,0,7,1,-1,4,5,3,-1,-1,-1,-1,-1}; .... return(&_MoveSpillage[_SpillTable[index]][0]); .... } 

At first glance, the example is complex, but it’s easy to figure it out after a little analysis.

The two-dimensional array _MoveSpillage is accessed by the index taken from the array _SpillTable . And it suddenly contains negative values. Perhaps access to data is organized according to a special formula, and this is what the developer intended. But I'm not so sure.

V512 A call of the 'sprintf' function will lead to overflow of the buffer '(char *) ptr'. SOUNDDLG.CPP 250

void SoundControlsClass::Process(void) { .... void * ptr=new char [sizeof(100)];//<= if (ptr) { sprintf((char *)ptr, "%cTrack %d\t%d:%02d\t%s",//<= index, listbox.Count()+1, length/60, length % 60, fullname); listbox.Add_Item((char const *)ptr); } .... } 

An attentive reader will wonder why such a long line is stored in a buffer of 4 bytes? But because the programmer thought that sizeof (100) would return something more (at least 100 ). But the sizeof operator returns the type size and never even counts any expressions. You just had to write the constant 100 , and even better, use named constants, or another type for strings or pointer.

V512 A call of the 'memset' function will lead to underflow of the buffer 'Buffer'. KEYBOARD.CPP 96

unsigned short Buffer[256]; WWKeyboardClass::WWKeyboardClass(void) { .... memset(Buffer, 0, 256); .... } 

Some buffer is cleared for 256 bytes, although the full size of the original buffer is 256 * sizeof (unsigned short) . Mistake.

You can still fix it like this:

memset(Buffer, 0, sizeof(Buffer)); 

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

typedef enum StructType : char { STRUCT_NONE=-1, .... STRUCT_COUNT,//<= 87 STRUCT_FIRST=0 } StructType; int BQuantity[STRUCT_COUNT-3];//<= [0..83] int QuantityB(int index) {return(BQuantity[index]);}//<= [0..86] bool CellClass::Goodie_Check(FootClass * object) { .... int bcount=0; for( j=0; j < STRUCT_COUNT; j++) { bcount += hptr->QuantityB(j);//<= [0..86] } .... } 

There are a lot of global variables in the code and it is obvious that they are easily confused. The analyzer warning about leaving the array border is issued at the place of access to the BQuantity array by index. The size of the array is 84 elements. The data flow analysis algorithms in the analyzer helped to establish that the index value comes from another function - Goodie_Check . There, a loop is executed with the final value 86 . Therefore, in this place 12 bytes of “alien” memory are constantly read (3 elements of type int ).

V575 The 'memset' function processes '0' elements. Inspect the third argument. DLLInterface.cpp 1103

void* __cdecl memset( _Out_writes_bytes_all_(_Size) void* _Dst, _In_ int _Val, _In_ size_t _Size ); extern "C" __declspec(dllexport) bool __cdecl CNC_Read_INI(....) { .... memset(ini_buffer, _ini_buffer_size, 0); .... } 

In my opinion, I have repeatedly seen this error in modern projects. Programmers are still confusing the 2nd and 3rd arguments to the memset function.

Another such place:

  • 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

void DisplayClass::Get_Occupy_Dimensions(int & w, int & h, short const *list) { .... if (!list) {/* ** Loop through all cell offsets, accumulating max & min x- & y-coords */while (*list != REFRESH_EOL) { .... } .... } .... } 

The explicit reference to the null pointer looks very strange. This place looks like a typo and there are some more places worth checking:

  • 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

void TechnoClass::Base_Is_Attacked(TechnoClass const *enemy) { FootClass *defender[6]; int value[6]; int count=0; int weakest=0; int desired=enemy->Risk() * 2; int risktotal=0;/* ** Humans have to deal with their own base is attacked problems. */if (!enemy || House->Is_Ally(enemy) || House->IsHuman) { return; } .... } 

Pointer enemy dereference, and then check that it is non-zero. There is still an urgent problem, I’m not afraid of the word, of every open source project. I am sure that in closed-source projects the situation is about the same, unless, of course, PVS-Studio is used ;-)

Wrong castes


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

#define VK_RETURN 0x0D typedef enum { .... WWKEY_VK_BIT=0x1000, .... } enum { .... KA_RETURN=VK_RETURN | WWKEY_VK_BIT, .... } void Window_Print(char const string[],...) { char c;//Current character. .... switch(c) { .... case KA_FORMFEED://<= 12 New_Window(); break; case KA_RETURN://<= 4109 Flush_Line(); ScrollCounter++; WinCx=0; WinCy++; break; .... } .... } 

This function processes input characters. As you know, a 1-byte value is placed in the char type, and the number 4109 will never be there. So, this switch statement simply contains an unreachable code branch.

Several 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

void Nod_Ending(void) { .... bool printedtext=false; while (!done) { if (!printedtext && !Is_Sample_Playing(kanefinl)) { printedtext++; Alloc_Object(....); mouseshown=true; Show_Mouse(); } .... } .... } 

In this code snippet, the analyzer found the application of the increment operation to a variable of the bool type. This is the correct code in terms of language, but it looks very strange now. Also, such an operation is marked as deprecated, starting with the C++ 17 standard.

Only 2 of these places were identified:

  • 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

ImpactType FlyClass::Physics(COORDINATE & coord, DirType facing); typedef enum ImpactType : unsigned char {//<= IMPACT_NONE, IMPACT_NORMAL, IMPACT_EDGE } ImpactType; typedef enum ResultType : unsigned char {//<= RESULT_NONE, .... } ResultType; void AircraftClass::AI(void) { .... if (Physics(Coord, PrimaryFacing) != RESULT_NONE) {//<= Mark(); } .... } 

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’s worth correcting the code (if this project is still supported, of course).

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

BOOL __cdecl Linear_Blit_To_Linear(...); inline HRESULT GraphicViewPortClass::Blit(....) { HRESULT return_code=0; .... return_code=(Linear_Blit_To_Linear(this, &dest, x_pixel, y_pixel , dx_pixel, dy_pixel , pixel_width, pixel_height, trans)); .... return ( return_code ); } 

Also a very old problem, relevant today. There are special macros for working with the HRESULT type, and casts to BOOL and vice versa are not used. These two types of data are extremely similar to each other in terms of language, but are logically incompatible. The operation of implicit type conversion existing in the code is meaningless.

This and a few more 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 '& lt; & lt;'. The left operand '(~ 0)' is negative. MP.CPP 2410

void XMP_Randomize(digit * result, Straw & rng, int total_bits, int precision) { .... ((unsigned char *)result)[nbytes-1] &= (unsigned char)(~((~0) << (total_bits % 8))); .... } 

Here a negative number shifts to the left, which is an undefined behavior. A negative number is obtained from zero when applying the inversion operator. Because the result of the operation is placed in the int type, the compiler uses it to store the value, and it is signed.

In 2020, the compiler also finds such an error:

Warning C26453: Arithmetic overflow: Left shift of a negative signed number is undefined behavior.

But compilers are not full-fledged static analyzers, as solve other problems. Therefore, here is another example of undefined behavior that is detected only using PVS-Studio:

V610 Undefined behavior. Check the shift operator '& lt; & lt;'. 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

#define UNITSIZE 32 void XMP_Shift_Right_Bits(digit * number, int bits, int precision) { .... int digits_to_shift=bits/UNITSIZE; int bits_to_shift=bits % UNITSIZE; int index; for (index=digits_to_shift; index < (precision-1); index++) { *number=(*(number + digits_to_shift) >> bits_to_shift) | (*(number + (digits_to_shift + 1)) << (UNITSIZE - bits_to_shift)); number++; } .... } 

The analyzer detected a situation where a 32-bit number shift to the right by a larger number of bits is potentially possible. Here's how it works:

int bits_to_shift=bits % UNITSIZE; 

The constant UNITSIZE is 32 :

int bits_to_shift=bits % 32; 

So, the value of the variable bits_to_shift will be zero for all values ​​of bits that are multiples of the number 32 .

Therefore, in this code snippet:

.... << (UNITSIZE - bits_to_shift).... 

a shift of 32 bits will occur if 32 is subtracted from the 32 constant.

List of all warnings from PVS-Studio about shifts with undefined behavior:

  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '(~ 0)' is negative. TARGET.H 66
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '((((- 24) * 256)/24)' is negative. ANIM.CPP 160
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '((((- 12) * 256)/24)' is negative. BUILDING.CPP 4037
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '((((- 21) * 256)/24)' is negative. DRIVE.CPP 2160
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '((((- 21) * 256)/24)' is negative. DRIVE.CPP 2161
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '((((- 20) * 256)/24)' is negative. DRIVE.CPP 2162
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '((((- 20) * 256)/24)' is negative. DRIVE.CPP 2163
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '((((- 18) * 256)/24)' is negative. DRIVE.CPP 2164
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '((((- 18) * 256)/24)' is negative. DRIVE.CPP 2165
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '((((- 17) * 256)/24)' is negative. DRIVE.CPP 2166
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '((((- 16) * 256)/24)' is negative. DRIVE.CPP 2167
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '(((- - 15) * 256)/24)' is negative. DRIVE.CPP 2168
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '((((- 14) * 256)/24)' is negative. DRIVE.CPP 2169
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '((((- 13) * 256)/24)' is negative. DRIVE.CPP 2170
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '((((- 12) * 256)/24)' is negative. DRIVE.CPP 2171
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '((((- 11) * 256)/24)' is negative. DRIVE.CPP 2172
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '((((- 10) * 256)/24)' is negative. DRIVE.CPP 2173
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '((((- 9) * 256)/24)' is negative. DRIVE.CPP 2174
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '((((- 8) * 256)/24)' is negative. DRIVE.CPP 2175
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '((((- 7) * 256)/24)' is negative. DRIVE.CPP 2176
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '((((- 6) * 256)/24)' is negative. DRIVE.CPP 2177
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '((((- 5) * 256)/24)' is negative. DRIVE.CPP 2178
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '((((- 4) * 256)/24)' is negative. DRIVE.CPP 2179
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '((((- 3) * 256)/24)' is negative. DRIVE.CPP 2180
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '((((- 2) * 256)/24)' is negative. DRIVE.CPP 2181
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. The left operand '((((- 1) * 256)/24)' is negative. DRIVE.CPP 2182
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'. 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 '& lt; & lt;'. The left operand '(~ 0)' is negative. RANDOM.CPP 102
  • V610 Undefined behavior. Check the shift operator '& lt; & lt;'.The left operand '(~ 0L)' is negative. RANDOM.CPP 164

Conclusion


Let's hope that the contemporary Electronic Arts projects are of higher quality. If not, then we invite you to download and try PVS-Studio on all projects.

Someone may object that cool quality games were made with such quality before, and partly we can agree with that. But do not forget that competition in the development of programs and games has grown many times over so many years. The costs of development, support and advertising also increased. Consequently, the correction of errors in the later stages of development entails significant financial and reputational losses.

Follow our blog to not miss the 2nd part of the review for this series of games.

ITKarma picture

If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. The Code of the Command & amp; Conquer Game: Bugs From the 90's. Volume one .

Source