Game Code Command & Conquer: bugs from the 90s. Volume One

image1.png

The American company Electronic Arts Inc (EA) has posted the open source code for the games Command & Conquer: Tiberian Dawn and Command & 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 & Conquer – 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. Game source code published along with the release of the collection Command & Conquer Remastered.

To find errors in the code, an analyzer was used. PVS-Studio. 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 check dest == NULL 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-added != Credits)

but inattention won.

Exactly the same code fragment is 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, here you will not notice that of the function Mono_Y called a function Get_xinstead Get_y. At class Monoclass there really are 2 functions that differ in 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]));
    ....
  }
  ....
}

This seems to be a debugging method, but how much harm he could do to the psyche of the developer, the story is silent. Here is an array Path consists of 9 elements, and everything is printed thirteen.

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 is easy to figure it out after a little analysis.

To two-dimensional array _MoveSpillage are accessed by an index taken from an 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 about that.

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 %dt%d:%02dt%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) will return something more (at least 100) But the operator sizeof returns the size of the type and never even counts any expressions. It was necessary to write just a constant 100, and even better, use named constants, or another type for strings or a 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);
  ....
}

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

You can still fix 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 array Bquantity 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 the loop with the final value is executed 86. Therefore, at this point, 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 function arguments memset.

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, in type char 1-byte value is placed, and numbers 4109 there will never be. So this operator switch just contains an unreachable code branch.

There were several such places:

  • 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 type bool. 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 such 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 warning list 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 '<<'. 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 type int, then 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 '<<'. 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 goes:

int bits_to_shift = bits % UNITSIZE;

Constant UNITSIZE has the meaning 32:

int bits_to_shift = bits % 32;

So, the value of the variable bits_to_shift will be zero for all values bitsmultiples of 32.

Therefore, in this piece of code:

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

there will be a shift of 32 bits, if from a constant 32 will be deducted 0.

List of all warnings from PVS-Studio 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 the contemporary Electronic Arts projects are of higher quality. If not, then we invite you to our site download and try PVS-Studio on all projects.

Someone may argue 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. Therefore, 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.

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 & Conquer Game: Bugs From the 90's. Volume one.

Similar Posts

Leave a Reply