The American company Electronic Arts Inc (EA) has made the source code of the games Command & Conquer: Tiberian Dawn and Command & Conquer: Red Alert publicly available. This code should help the game community to develop mods and maps, create custom units, and customize the gameplay logic. We all now have a unique opportunity to plunge into the history of development, which is very different from the modern one. Back then, there was no Stack Overflow site, convenient code editors, or powerful compilers. Moreover, at that time, there were no static analyzers, and the first thing the community will face is hundreds of errors in the code. This is what the PVS-Studio team will help you with by pointing out the erroneous places.
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 Electronic Arts company acquired this game's development studio only in 1998.
Since then, several games and many mods have been released. 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.
Due to the large amount of problems found in the code, all error examples will be given in a series of two articles.
V501 There are identical sub-expressions to the left and to the right of the '||' operator: dest == 0 || dest == 0 CONQUER.CPP 5576
void List_Copy(short const * source, int len, short * dest)
{
if (dest == NULL || dest == NULL) {
return;
}
....
}I'd like to start the review with the never ending copy-paste. The author hasn't checked the pointer for the source and checked the destination pointer twice, because they had copied the dest == NULLcheck and had forgotten to change the variable name.
V584 The 'Current' value is present on both sides of the '!=' operator. The expression is incorrect or it can be simplified. CREDITS.CPP 173
void CreditClass::AI(bool forced, HouseClass *player_ptr, bool logic_only)
{
....
long adder = Credits - Current;
adder = ABS(adder);
adder >>= 5;
adder = Bound(adder, 1L, 71+72);
if (Current > Credits) adder = -adder;
Current += adder;
Countdown = 1;
if (Current-adder != Current) { // <=
IsAudible = true;
IsUp = (adder > 0);
}
....
}The analyzer found a meaningless comparison. I suppose there must have been something as follows:
if (Current-adder != Credits)but inattention has won.
The exact same code fragment was copied to another function:
V524 It is odd that the body of 'Mono_Y' function is fully equivalent to the body of 'Mono_X' function. MONOC.CPP 753
class MonoClass {
....
int Get_X(void) const {return X;};
int Get_Y(void) const {return Y;};
....
}
int Mono_X(void)
{
if (MonoClass::Is_Enabled()) {
MonoClass *mono = MonoClass::Get_Current();
if (!mono) {
mono = new MonoClass();
mono->View();
}
return(short)mono->Get_X(); // <=
}
return(0);
}
int Mono_Y(void)
{
if (MonoClass::Is_Enabled()) {
MonoClass *mono = MonoClass::Get_Current();
if (!mono) {
mono = new MonoClass();
mono->View();
}
return(short)mono->Get_X(); // <= Get_Y() ?
}
return(0);
}A larger piece of code that was copied with the consequences. You have to admit, that except using the analyzer, you won't be able notice that the Get_X function instead of Get_Ywas called from the Mono_Y function. The MonoClass class does have 2 functions that differ by one symbol. Most likely, we found a real error.
I found the identical piece of code below:
V557 Array overrun is possible. The '9' index is pointing beyond array bound. FOOT.CPP 232
#define CONQUER_PATH_MAX 9 // Number of cells to look ahead for movement.
FacingType Path[CONQUER_PATH_MAX];
void FootClass::Debug_Dump(MonoClass *mono) const
{
....
if (What_Am_I() != RTTI_AIRCRAFT) {
mono->Set_Cursor(50, 3);
mono->Printf("%s%s%s%s%s%s%s%s%s%s%s%s",
Path_To_String(Path[0]),
Path_To_String(Path[1]),
Path_To_String(Path[2]),
Path_To_String(Path[3]),
Path_To_String(Path[4]),
Path_To_String(Path[5]),
Path_To_String(Path[6]),
Path_To_String(Path[7]),
Path_To_String(Path[8]),
Path_To_String(Path[9]),
Path_To_String(Path[10]),
Path_To_String(Path[11]),
Path_To_String(Path[12]));
....
}
....
}It seems that this is a debugging method, but the history doesn't record to what extent it could be detrimental for the developer's mental health. Here, the Path array consists of 9 elements, and all 13 of them are printed.
In total, 4 memory accesses outside the array boundary:
V557 Array underrun is possible. The value of '_SpillTable[index]' index could reach -1. COORD.CPP 149
typedef enum FacingType : char {
....
FACING_COUNT, // 8
FACING_FIRST=0
} FacingType;
short const * Coord_Spillage_List(COORDINATE coord, int maxsize)
{
static short const _MoveSpillage[(int)FACING_COUNT+1][5] = {
....
};
static char const _SpillTable[16] = {8,6,2,-1,0,7,1,-1,4,5,3,-1,-1,-1,-1,-1};
....
return(&_MoveSpillage[_SpillTable[index]][0]);
....
}At first glance, the example is complex, but it is easy to puzzle it out after a brief analysis.
The two-dimensional _MoveSpillage array is accessed by an index that is taken from the _SpillTable array. The array happens to contain negative values. Perhaps, access to data is organized according to a special formula and this is what the developer intended. Nonetheless, I'm not sure about that.
V512 A call of the 'sprintf' function will lead to overflow of the buffer '(char *) ptr'. SOUNDDLG.CPP 250
void SoundControlsClass::Process(void)
{
....
void * ptr = new char [sizeof(100)]; // <=
if (ptr) {
sprintf((char *)ptr, "%cTrack %d\t%d:%02d\t%s", // <=
index, listbox.Count()+1, length / 60, length % 60, fullname);
listbox.Add_Item((char const *)ptr);
}
....
}An attentive reader will wonder - why such a long string is saved in a buffer of 4 bytes? This is because the programmer thought that sizeof(100) would return something more (at least 100). However, the sizeof operator returns the size of the type and even never evaluates any expressions. The author should have just written the constant 100, or better yet, used named constants, or a different type for strings or a pointer.
V512 A call of the 'memset' function will lead to underflow of the buffer 'Buffer'. KEYBOARD.CPP 96
unsigned short Buffer[256];
WWKeyboardClass::WWKeyboardClass(void)
{
....
memset(Buffer, 0, 256);
....
}A buffer is cleared by 256 bytes, although the full size of the original buffer is 256*sizeof(unsigned short). Oops... goofed this one.
It can be also fixed as follows:
memset(Buffer, 0, sizeof(Buffer));V557 Array overrun is possible. The 'QuantityB' function processes value '[0..86]'. Inspect the first argument. Check lines: 'HOUSE.H:928', 'CELL.CPP:2337'. HOUSE.H 928
typedef enum StructType : char {
STRUCT_NONE=-1,
....
STRUCT_COUNT, // <= 87
STRUCT_FIRST=0
} StructType;
int BQuantity[STRUCT_COUNT-3]; // <= [0..83]
int QuantityB(int index) {return(BQuantity[index]);} // <= [0..86]
bool CellClass::Goodie_Check(FootClass * object)
{
....
int bcount = 0;
for( j=0; j < STRUCT_COUNT; j++) {
bcount += hptr->QuantityB(j); // <= [0..86]
}
....
}There are a lot of global variables in the code and it is obvious that they are easy to get confused. The analyzer's warning about an array index out of bounds is issued at the point of accessing the BQuantity array by index. The array size is 84 elements. Algorithms for analyzing the data flow in the analyzer helped to find out that the index value comes from another function – Goodie_Check. There, a loop is executed with a final value of 86. Therefore, 12 bytes of "someone's" memory (3 int elements) are constantly being read in this place.
V575 The 'memset' function processes '0' elements. Inspect the third argument. DLLInterface.cpp 1103
void* __cdecl memset(
_Out_writes_bytes_all_(_Size) void* _Dst,
_In_ int _Val,
_In_ size_t _Size
);
extern "C" __declspec(dllexport) bool __cdecl CNC_Read_INI(....)
{
....
memset(ini_buffer, _ini_buffer_size, 0);
....
}In my opinion, I have repeatedly seen this error in modern projects. Programmers still confuse the 2nd and 3rd arguments of the memset function.
One more similar fragment:
V522 Dereferencing of the null pointer 'list' might take place. DISPLAY.CPP 1062
void DisplayClass::Get_Occupy_Dimensions(int & w, int & h, short const *list)
{
....
if (!list) {
/*
** Loop through all cell offsets, accumulating max & min x- & y-coords
*/
while (*list != REFRESH_EOL) {
....
}
....
}
....
}An explicit access to a null pointer looks very strange. This place looks like the one with a typo and there are a few other places worth checking out:
V595 The 'enemy' pointer was utilized before it was verified against nullptr. Check lines: 3689, 3695. TECHNO.CPP 3689
void TechnoClass::Base_Is_Attacked(TechnoClass const *enemy)
{
FootClass *defender[6];
int value[6];
int count = 0;
int weakest = 0;
int desired = enemy->Risk() * 2;
int risktotal = 0;
/*
** Humans have to deal with their own base is attacked problems.
*/
if (!enemy || House->Is_Ally(enemy) || House->IsHuman) {
return;
}
....
}The enemy pointer is dereferenced, and then checked to make sure that it is nonnull. It is still a vital problem, dare I say it, for every open source project. I'm sure that in projects with closed code the situation is about the same, unless, of course, PVS-Studio is used ;-)
V551 The code under this 'case' label is unreachable. The '4109' value of the 'char' type is not in the range [-128; 127]. WINDOWS.CPP 547
#define VK_RETURN 0x0D
typedef enum {
....
WWKEY_VK_BIT = 0x1000,
....
}
enum {
....
KA_RETURN = VK_RETURN | WWKEY_VK_BIT,
....
}
void Window_Print(char const string[], ...)
{
char c; // Current character.
....
switch(c) {
....
case KA_FORMFEED: // <= 12
New_Window();
break;
case KA_RETURN: // <= 4109
Flush_Line();
ScrollCounter++;
WinCx = 0;
WinCy++;
break;
....
}
....
}This function handles the characters you enter. As you know, a 1-byte value is placed in the char type, and the number 4109 will never be there. So, this switch statement just contains an unreachable code branch.
Several such places were found:
V552 A bool type variable is being incremented: printedtext ++. Perhaps another variable should be incremented instead. ENDING.CPP 170
void Nod_Ending(void)
{
....
bool printedtext = false;
while (!done) {
if (!printedtext && !Is_Sample_Playing(kanefinl)) {
printedtext++;
Alloc_Object(....);
mouseshown = true;
Show_Mouse();
}
....
}
....
}In this code fragment, the analyzer found the application of the increment operation to a variable of the bool type. This is correct code from the point of view of the language, but it looks very strange now. This operation is also marked as deprecated, starting from the C++17 standard.
In total, 2 such places were detected:
V556 The values of different enum types are compared. Types: ImpactType, ResultType. AIRCRAFT.CPP 742
ImpactType FlyClass::Physics(COORDINATE & coord, DirType facing);
typedef enum ImpactType : unsigned char { // <=
IMPACT_NONE,
IMPACT_NORMAL,
IMPACT_EDGE
} ImpactType;
typedef enum ResultType : unsigned char { // <=
RESULT_NONE,
....
} ResultType;
void AircraftClass::AI(void)
{
....
if (Physics(Coord, PrimaryFacing) != RESULT_NONE) { // <=
Mark();
}
....
}The programmer tied some logic to comparing the values of different enumerations. Technically, this works because numerical representations are compared. But such code often leads to logical errors. It is worth tweaking the code (of course, if this project is to be supported).
The entire list of warnings for this diagnostic looks like this:
V716 Suspicious type conversion in assign expression: 'HRESULT = BOOL'. GBUFFER.H 780
BOOL __cdecl Linear_Blit_To_Linear(...);
inline HRESULT GraphicViewPortClass::Blit(....)
{
HRESULT return_code=0;
....
return_code=(Linear_Blit_To_Linear(this, &dest, x_pixel, y_pixel
, dx_pixel, dy_pixel
, pixel_width, pixel_height, trans));
....
return ( return_code );
}This is a very old problem that is still relevant today. There are special macros for working with the HRESULT type. Casting to BOOL and visa versa isn't used for this type. These two data types are extremely similar to each other from the point of view of the language, but logically they are still incompatible. The implicit type casting operation that exists in the code doesn't make sense.
This and a few other places would be worth refactoring:
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(~0)' is negative. MP.CPP 2410
void XMP_Randomize(digit * result, Straw & rng, int total_bits, int precision)
{
....
((unsigned char *)result)[nbytes-1] &=
(unsigned char)(~((~0) << (total_bits % 8)));
....
}Here, the negative number is shifted to the left, which is undefined behavior. A negative number is obtained from zero when using the inversion operator. Since the result of the operation is placed in the int type, the compiler uses it to store the value, whereas it is of the signed type.
In 2020, the compiler already finds this error as well:
Warning C26453: Arithmetic overflow: Left shift of a negative signed number is undefined behavior.
But compilers are not full-fledged static analyzers, because they solve other problems. So here is another example of undefined behavior that is only detected by PVS-Studio:
V610 Undefined behavior. Check the shift operator '<<'. The right operand ('(32 - bits_to_shift)' = [1..32]) is greater than or equal to the length in bits of the promoted left operand. MP.CPP 659
#define UNITSIZE 32
void XMP_Shift_Right_Bits(digit * number, int bits, int precision)
{
....
int digits_to_shift = bits / UNITSIZE;
int bits_to_shift = bits % UNITSIZE;
int index;
for (index = digits_to_shift; index < (precision-1); index++) {
*number = (*(number + digits_to_shift) >> bits_to_shift) |
(*(number + (digits_to_shift + 1)) << (UNITSIZE - bits_to_shift));
number++;
}
....
}The analyzer has found an unusual situation. The 32-bit number can be potentially shifted to the right for the number of bits, exceeding the available number. Here's how it works:
int bits_to_shift = bits % UNITSIZE;The UNITIZE constant has the value 32:
int bits_to_shift = bits % 32;Thus, the value of the bits_to_shift variable will be zero for all bits values that are multiples of 32.
Therefore, in this code fragment:
.... << (UNITSIZE - bits_to_shift) ....32 digits will be shifted if 0 is subtracted from the constant 32.
List of all PVS-Studio warnings about shifts with undefined behavior:
Let's hope that modern projects of Electronic Arts are of better quality. If not, we invite you to our site to download and try PVS-Studio on all projects.
Someone may object that cool successful games used to be made with this quality, and we can partially agree with this. On the other hand, we mustn't forget that competition in the development of programs and games has grown many times over the years. Expenses on development, support, and advertising have also increased. Consequently, fixing errors at later stages of development can lead to significant financial and reputational losses.
Follow our blog and don't miss the 2nd part of the review of this games series.