📦 ♏️ 🐈 Command & Conquer game code: bugs from the 90s. Volume two 😼 👩🏿‍🤝‍👨🏾 📚

The American company Electronic Arts Inc (EA) has released the open source code of the games Command & Conquer: Tiberian Dawn and Command & Conquer: Red Alert. Several dozen bugs were found in the source code using the PVS-Studio analyzer, so please welcome the continuation of the description of the found defects.

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 source code of the games was published with the release of the Command & Conquer Remastered collection .

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

Link to the first bug summary: " Command & Conquer Game : Bugs from the 90s. Volume One ".

Errors in conditions

V583 The '?:' Operator, regardless of its conditional expression, always returns one and the same value: 3072. STARTUP.CPP 1136

void Read_Setup_Options( RawFileClass *config_file )
{
  ....
  ScreenHeight = ini.Get_Bool("Options", "Resolution", false) ? 3072 : 3072;
  ....
}

It turns out that users could not influence some settings. More precisely, they did something, but due to the fact that the ternary operator always returns one value, in fact nothing changed.

V590 Consider inspecting the 'i <8 && i <4' expression. The expression is excessive or contains a misprint. DLLInterface.cpp 2238

// 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);
  }
}

Due to the wrong cycle, the position is not set for all players. On the one hand, we see the constant MAX_PLAYERS 8 and assume that this is the maximum number of players. On the other hand, we see the condition i <4 and the && operator . Thus, the loop never makes 8 iterations. Most likely, at the initial stage of development, the programmer did not use constants, and when he started, he forgot to remove the old numbers from the code.

V648 Priority of the '&&' operation is higher than that of the '||' operation. INFANTRY.CPP 1003

void InfantryClass::Assign_Target(TARGET target)
{
  ....
  if (building && building->Class->IsCaptureable &&
    (GameToPlay != GAME_NORMAL || *building != STRUCT_EYE && Scenario < 13)) {
    Assign_Destination(target);
  }
  ....
}

You can make the code unobvious (and most likely erroneous) simply by not specifying the priority of operations for the || operators. and && . It is completely unclear here whether this is a mistake or not. But given the overall quality of the code of these projects, let's assume that here and in several other places there are mistakes with the priority of operations:

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

typedef enum StructType : char {
  STRUCT_NONE=-1,
  STRUCT_ADVANCED_TECH,
  STRUCT_IRON_CURTAIN,
  STRUCT_WEAP,
  STRUCT_CHRONOSPHERE, // 3
  ....
}

#define  STRUCTF_CHRONOSPHERE (1L << STRUCT_CHRONOSPHERE)

UrgencyType HouseClass::Check_Build_Power(void) const
{
  ....
  if (State == STATE_THREATENED || State == STATE_ATTACKED) {
    if (BScan | (STRUCTF_CHRONOSPHERE)) {  // <=
      urgency = URGENCY_HIGH;
    }
  }
  ....
}

To check if certain bits in a variable are set, use the & operator, not |. Due to a typo in this piece of code, the condition is always true.

V768 The enumeration constant 'WWKEY_RLS_BIT' is used as a variable of a Boolean-type. KEYBOARD.CPP 286

typedef enum {
  WWKEY_SHIFT_BIT = 0x100,
  WWKEY_CTRL_BIT  = 0x200,
  WWKEY_ALT_BIT   = 0x400,
  WWKEY_RLS_BIT   = 0x800,
  WWKEY_VK_BIT    = 0x1000,
  WWKEY_DBL_BIT   = 0x2000,
  WWKEY_BTN_BIT   = 0x8000,
} WWKey_Type;

int WWKeyboardClass::To_ASCII(int key)
{
  if ( key && WWKEY_RLS_BIT)
    return(KN_NONE);
  return(key);
}

I think, in the key parameter, they wanted to check a certain bit specified by the WWKEY_RLS_BIT mask , but made a typo. The bitwise operator &, rather than &&, should have been used to check the key code.

Suspicious formatting

V523 The 'then' statement is equivalent to the 'else' statement. RADAR.CPP 1827

void RadarClass::Player_Names(bool on)
{
  IsPlayerNames = on;
  IsToRedraw = true;
  if (on) {
    Flag_To_Redraw(true);
//    Flag_To_Redraw(false);
  } else {
    Flag_To_Redraw(true);   // force drawing of the plate
  }
}

Once upon a time, a developer commented on code for debugging. Since then, the code has remained a conditional operator with the same operators in different branches.

Exactly the same places were found two more:

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

static int Net_Join_Dialog(void)
{
  ....
  /*...............................................................
  F4/SEND/'M' = edit a message
  ...............................................................*/
  if (Messages.Get_Edit_Buf()==NULL) {
    ....
  } else

  /*...............................................................
  If we're already editing a message and the user clicks on
  'Send', translate our input to a Return so Messages.Input() will
  work properly.
  ...............................................................*/
  if (input==(BUTTON_SEND | KN_BUTTON)) {
    input = KN_RETURN;
  }
  ....
}

Due to a large comment, the developer did not see the above undefined conditional operator. The rest of the else keyword forms with the condition below the else if construction , which is most likely a change to the original logic.

V519 The 'ScoresPresent' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 539, 541. INIT.CPP 541

bool Init_Game(int , char *[])
{
  ....
  ScoresPresent = false;
//if (CCFileClass("SCORES.MIX").Is_Available()) {
    ScoresPresent = true;
    if (!ScoreMix) {
      ScoreMix = new MixFileClass("SCORES.MIX");
      ThemeClass::Scan();
    }
//}

Another potential defect due to unfinished refactoring. Now it is not clear whether the ScoresPresent variable must be true , or still false .

Memory deallocation 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

BOOL Send_Data_To_DDE_Server (char *data, int length, int packet_type)
{
  ....
  char *poke_data = new char [length + 2*sizeof(int)]; // <=
  ....
  if(DDE_Class->Poke_Server( .... ) == FALSE) {
    CCDebugString("C&C95 - POKE failed!\n");
    DDE_Class->Close_Poke_Connection();
    delete poke_data;                                  // <=
    return (FALSE);
  }

  DDE_Class->Close_Poke_Connection();

  delete poke_data;                                    // <=

  return (TRUE);
}

The analyzer has detected an error related to the fact that memory can be allocated and released in incompatible ways. To free the memory allocated for the array, you should have used the delete [] operator , not delete .

There were several such places, and they all 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

void GDI_Ending(void)
{
  ....
  void * localpal = Load_Alloc_Data(CCFileClass("SATSEL.PAL"));
  ....
  delete [] localpal;
  ....
}

The delete and delete [] operators are separated for a reason. They do a different job of cleaning up memory. And when using an untyped pointer, the compiler does not know what data type the pointer is pointing to. In the C ++ language standard, the behavior of the compiler is undefined.

A number of analyzer warnings were also found 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

void Map_Selection(void)
{
  ....
  unsigned char *grey2palette    = new unsigned char[768];
  unsigned char *progresspalette = new unsigned char[768];
  ....
  scenario = Scenario + ((house == HOUSE_GOOD) ? 0 : 14);
  if (house == HOUSE_GOOD) {
    lastscenario = (Scenario == 14);
    if (Scenario == 15) return;
  } else {
    lastscenario = (Scenario == 12);
    if (Scenario == 13) return;
  }
  ....
}

"If you do not free memory at all, then I will definitely not be mistaken in choosing an operator!" - perhaps the programmer thought.

But then a memory leak occurs, which is also an error. Somewhere at the end of the function, memory is freed, but before that there are many places where the conditional exit from the function occurs, and the memory by the pointers grey2palette and progresspalett is not freed.

miscellanea

V570 The 'hdr-> MagicNumber' variable is assigned to itself. COMBUF.CPP 806

struct CommHdr {
  unsigned short MagicNumber;
  unsigned char Code;
  unsigned long PacketID;
} *hdr;

void CommBufferClass::Mono_Debug_Print(int refresh)
{
  ....
  hdr = (CommHdr *)SendQueue[i].Buffer;
  hdr->MagicNumber = hdr->MagicNumber;
  hdr->Code = hdr->Code;
  ....
}

Two fields of the CommHdr structure are initialized with their own values. In my opinion, a pointless operation, but it is performed 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

int FixedHeapClass::Free(void * pointer);

template<class T>
class TFixedHeapClass : public FixedHeapClass
{
  ....
  virtual int Free(T * pointer) {FixedHeapClass::Free(pointer);};
};

The functions of the Free class TFixedHeapClass no operator's return statement . The most interesting thing is that the called function FixedHeapClass :: Free has a return value of type int . Most likely, the programmer simply forgot to write the return statement and now the function returns an incomprehensible value.

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

ResultType BuildingClass::Take_Damage(int & damage, ....)
{
  ....
  if (tech && tech->IsActive && ....) {
    int damage = 500;
    tech->Take_Damage(damage, 0, WARHEAD_AP, source, forced);
  }
  ....
}

The damage parameter is passed by reference. Therefore, changes in the values of this variable are expected in the body of the function. But in one place, the developer declared a variable with the same name. Because of this, the value 500 will be stored in the local variable damage, rather than a function parameter. Perhaps a different behavior was intended.

Another such place:

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

class ObjectClass : public AbstractClass
{
  ....
  virtual short const * Occupy_List(bool placement=false) const; // <=
  virtual short const * Overlap_List(void) const;
  ....
};

class BulletClass : public ObjectClass,
                    public FlyClass,
                    public FuseClass
{
  ....
  virtual short const * Occupy_List(void) const;                 // <=
  virtual short const * Overlap_List(void) const {return Occupy_List();};
  ....
};

The analyzer has detected a potential error while overriding the Occupy_List virtual function . This can lead to the wrong functions being called at runtime.

A few more suspicious places:

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

void DisplayClass::Set_Tactical_Position(COORDINATE coord)
{
  int xx = 0;
  int yy = 0;

  Confine_Rect(&xx, &yy, TacLeptonWidth, TacLeptonHeight,
    Cell_To_Lepton(MapCellWidth) + GlyphXClientSidebarWidthInLeptons,
    Cell_To_Lepton(MapCellHeight));

  coord = XY_Coord(xx + Cell_To_Lepton(MapCellX), yy + Cell_To_Lepton(....));

  if (ScenarioInit) {
    TacticalCoord = coord;
  }
  DesiredTacticalCoord = coord;
  IsToRedraw = true;
  Flag_To_Redraw(false);
}

The coord parameter is immediately overwritten in the body of the function. The old value was not used. It is very suspicious when a function has arguments and it does not depend on them. And then there are some coordinates.

And this place 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

extern "C" unsigned char *InterpolationPalette;

void Map_Selection(void)
{
  unsigned char localpalette[768];
  ....
  InterpolationPalette = localpalette;
  ....
}

There are a lot of global variables in the game code. This was probably a common approach to coding back then. But now it is considered bad and even dangerous.

The local array localpalette is stored in the InterpolationPalette pointer, 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 the new projects of Electronic Arts are of better quality. In general, game developers are actively purchasing PVS-Studio. Now the budgets of games are quite large, so no one needs the extra costs of fixing bugs in production. And fixing an error at an early stage of coding practically does not take time and other resources.

We invite you to download and try PVS-Studio on all projects on our website .

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

Command & Conquer game code: bugs from the 90s. Volume two