Sviatoslav Razmyslov

Jul 13 2020

Tags:

#Cpp #GameDev

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

Jul 13 2020

Author: Sviatoslav Razmyslov

Introduction
Errors in conditions
Suspicious formatting
Memory release errors
Other Issues
Conclusion

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.

0748_CnC_Remastered_Collection_2/image1.png

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 posted together with the release of the Command & Conquer Remastered collection.

The PVS-Studio 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.

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

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

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

// 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 an incorrect loop, 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 &&. 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.

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

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

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, the intention was to check a certain bit set by the WWKEY_RLS_BIT mask, but the author made a typo. They should have used the & bitwise operator instead of && 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
  }
}

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

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 hasn't seen the above unfinished conditional operator. The remaining else keyword forms the else if construction with the condition below, which most likely changes 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 incomplete refactoring. Now it is unclear whether the ScoresPresent variable should be set to true or 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

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 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 delete[] operator should have been used instead of 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

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 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.

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

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;
  }
  ....
}

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''.

0748_CnC_Remastered_Collection_2/image2.png

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 grey2palette and progresspalett pointers isn't released.

Other Issues

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

int FixedHeapClass::Free(void * pointer);

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

In the Freefunction of the TFixedHeapClass class there is no return operator. What's interesting is that the called FixedHeapClass::Free function also has a return value of the int type. Most likely, the programmer just 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, 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 500 value instead of the function parameter is stored in the local damage variable . Perhaps different behavior was intended.

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

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 detected a potential error in overriding the virtual Occupy_List function. This may cause the wrong functions to be called at runtime.

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

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 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.

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

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. 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. By the way, game developers are currently actively purchasing PVS-Studio. Now the game budgets are quite large, so 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 download and try PVS-Studio on all projects.

#Cpp #GameDev