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. Dozens of errors were detected in the source code using the PVS-Studio analyzer, so meet the continuation of the description of the defects found.

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

Link to the first review of errors: " Game Command & amp; Conquer: 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 a single value, nothing has changed in fact.

V590 Consider inspecting the 'i & lt; 8 & amp; & amp; i & lt; 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 for all players is not set. 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 & lt; 4 and the operator & amp; & amp; . So the loop never does 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 '& amp; & amp;' 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 the operations for the || and & amp; & amp; operators. It is completely unclear here whether this is a mistake or not. But taking into account the overall quality of the code for these projects, suppose that here and in several places errors were made with the priority of operations:

  • V648 Priority of the '& amp; & amp;' operation is higher than that of the '||' operation. TEAM.CPP 456
  • V648 Priority of the '& amp; & amp;' operation is higher than that of the '||' operation. DISPLAY.CPP 1160
  • V648 Priority of the '& amp; & amp;' operation is higher than that of the '||' operation. DISPLAY.CPP 1571
  • V648 Priority of the '& amp; & amp;' operation is higher than that of the '||' operation. HOUSE.CPP 2594
  • V648 Priority of the '& amp; & amp;' operation is higher than that of the '||' operation. INIT.CPP 2541

V617 Consider inspecting the condition. The '((1L & lt; & lt; 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 & amp; operator, not |. Because of a typo in this code fragment, a true condition is always obtained.

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 specific bit specified by the WWKEY_RLS_BIT mask, but they made a typo. You should use the bitwise operator & amp; rather than & amp; & amp; 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 a developer commented on code for debugging.Since then, the conditional operator with the same operators in different branches has remained in the code.

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 the big comment, the developer did not see the above unfinished conditional operator. The remaining else keyword forms the else if construct with the condition below, 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 flaw due to incomplete refactoring. Now it’s not clear whether the variable ScoresPresent should be set to true , or still false .

Free memory 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 detected an error related to the fact that the memory can be allocated and freed in incompatible ways. To free the memory allocated for the array, the delete [] operator should be used, not the delete .

There were several such places, and all of them gradually harm the working 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 [] gray2palette;'. 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 operators delete and delete [] are not randomly separated. They do a different job of clearing memory. And when using an untyped pointer, the compiler does not know what type of data the pointer leads to. In the C++ language standard, compiler behavior is undefined.

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

"Если вообще не освобождать память, то точно не ошибусь в выборе оператора!" — возможно, подумал программист.

image2.png

Но тогда происходит утечка памяти, что тоже является ошибкой. Где-то в конце функции выполняется освобождение памяти, но до этого много мест, где происходит условный выход из функции, и память по указателям grey2palette и progresspalett не освобождается.

Разное


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

Два поля структуры CommHdr инициализируются собственными значениями. По-моему, бессмысленная операция, но выполняется она много раз:

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

В функции Free класса TFixedHeapClass нет оператора return . Самое интересное, что у вызываемой функции FixedHeapClass::Free возвращаемое значение тоже типа int . Скорее всего, программист просто забыл написать оператор return и теперь функция возвращает непонятное значение.

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

Параметр damage передаётся по ссылке. Следовательно, в теле функции ожидается изменение значений этой переменной. Но в одном месте разработчик объявил переменную с таким же именем. Из-за этого значение 500 сохранится в локальную переменную damage, а не параметр функции. Возможно, задумывалось другое поведение.

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 detected a potential error while redefining the virtual function Occupy_List . This may result in calling the wrong functions in 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 function body. The old value was not used. This is very suspicious when a function has arguments, and it does not depend on them. And then some coordinates are being transmitted.

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. It was probably in those days a common approach to writing code. But now it is considered bad and even dangerous.

The localpalette local array is saved 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, we hope that the new Electronic Arts projects are of higher quality. In general, game developers are actively purchasing PVS-Studio. Now the budgets of games are quite large, so no one needs the extra expenses for fixing bugs in production. And fixing the error at the early stage of writing code practically does not take time and other resources.

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

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

Source