To get a trial key
fill out the form below
Team License (standard version)
Enterprise License (extended version)
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Request our prices
New License
License Renewal
--Select currency--
USD
EUR
GBP
RUB
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
I am interested to try it on the platforms:
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Message submitted.

Your message has been sent. We will email you at


If you haven't received our response, please do the following:
check your Spam/Junk folder and click the "Not Spam" button for our message.
This way, you won't miss messages from our team in the future.

>
>
>
The Code of the Command & Conquer Game:…

The Code of the Command & Conquer Game: Bugs from the 90's. Volume two

Jul 13 2020

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.

Popular related articles
Static analysis as part of the development process in Unreal Engine

Date: Jun 27 2017

Author: Andrey Karpov

Unreal Engine continues to develop as new code is added and previously written code is changed. What is the inevitable consequence of ongoing development in a project? The emergence of new bugs in th…
PVS-Studio for Java

Date: Jan 17 2019

Author: Andrey Karpov

In the seventh version of the PVS-Studio static analyzer, we added support of the Java language. It's time for a brief story of how we've started making support of the Java language, how far we've co…
The Evil within the Comparison Functions

Date: May 19 2017

Author: Andrey Karpov

Perhaps, readers remember my article titled "Last line effect". It describes a pattern I've once noticed: in most cases programmers make an error in the last line of similar text blocks. Now I want t…
How PVS-Studio Proved to Be More Attentive Than Three and a Half Programmers

Date: Oct 22 2018

Author: Andrey Karpov

Just like other static analyzers, PVS-Studio often produces false positives. What you are about to read is a short story where I'll tell you how PVS-Studio proved, just one more time, to be more atte…
The way static analyzers fight against false positives, and why they do it

Date: Mar 20 2017

Author: Andrey Karpov

In my previous article I wrote that I don't like the approach of evaluating the efficiency of static analyzers with the help of synthetic tests. In that article, I give the example of a code fragment…
The Ultimate Question of Programming, Refactoring, and Everything

Date: Apr 14 2016

Author: Andrey Karpov

Yes, you've guessed correctly - the answer is "42". In this article you will find 42 recommendations about coding in C++ that can help a programmer avoid a lot of errors, save time and effort. The au…
Appreciate Static Code Analysis!

Date: Oct 16 2017

Author: Andrey Karpov

I am really astonished by the capabilities of static code analysis even though I am one of the developers of PVS-Studio analyzer myself. The tool surprised me the other day as it turned out to be sma…
Characteristics of PVS-Studio Analyzer by the Example of EFL Core Libraries, 10-15% of False Positives

Date: Jul 31 2017

Author: Andrey Karpov

After I wrote quite a big article about the analysis of the Tizen OS code, I received a large number of questions concerning the percentage of false positives and the density of errors (how many erro…
The Last Line Effect

Date: May 31 2014

Author: Andrey Karpov

I have studied many errors caused by the use of the Copy-Paste method, and can assure you that programmers most often tend to make mistakes in the last fragment of a homogeneous code block. I have ne…
Technologies used in the PVS-Studio code analyzer for finding bugs and potential vulnerabilities

Date: Nov 21 2018

Author: Andrey Karpov

A brief description of technologies used in the PVS-Studio tool, which let us effectively detect a large number of error patterns and potential vulnerabilities. The article describes the implementati…

Comments (0)

Next comments

This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
This website uses cookies and other technology to provide you a more personalized experience. By continuing the view of our web-pages you accept the terms of using these files. If you don't want your personal data to be processed, please, leave this site.
Learn More →
Accept