Our website uses cookies to enhance your browsing experience.
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 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.
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"
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:
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.
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:
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.
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):
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:
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''.
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.
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:
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:
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:
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:
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:
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.
0
0
0
0