Electronic arts

Американская компания Electronic Arts Inc (EA) выложила в открытый доступ исходный код игр Command & Conquer: Tiberian Dawn и Command & Conquer: Red Alert. В исходном коде было обнаружено несколько десятков ошибок с помощью анализатора PVS-Studio, поэтому встречайте продолжение описания найденных дефектов.

Введение

Command & Conquer серия компьютерных игр в жанре стратегии в реальном времени. Первая игра серии была выпущена в 1995 году. Исходный код игр опубликовали вместе с выпуском коллекции Command & Conquer Remastered.

Для поиска ошибок в коде использовался анализатор PVS-Studio. Это инструмент для выявления ошибок и потенциальных уязвимостей в исходном коде программ, написанных на языках С, C++, C# и Java.

Ссылка на первый обзор ошибок: "Игра Command & Conquer: баги из 90-х. Том первый".

Ошибки в условиях

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

Оказывается, на некоторые настройки пользователи не могли повлиять. Точнее они что-то делали, но из-за того, что тернарный оператор всегда возвращает одно значение, по факту ничего не менялось.

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 havefor (int i = 0; i < MAX_PLAYERS && i < 4; i++) {  if (GlyphxPlayerIDs[i] == player_id) {    MultiplayerStartPositions[i] = XY_Cell(x, y);  }}

Из-за неправильного цикла не задаётся позиция для всех игроков. С одной стороны, мы видим константу MAX_PLAYERS 8 и предполагаем, что это максимальное количество игроков. С другой мы видим условие i < 4 и оператор &&. Таким образом, цикл никогда не делает 8 итераций. Скорее всего, на начальном этапе разработки программист не использовал константы, а когда начал забыл удалить старые числа из кода.

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

Сделать код неочевидным (и, скорее всего, ошибочным) можно просто не указав приоритет операций для операторов || и &&. Здесь совсем непонятно, это ошибка или нет. Но учитывая общее качество кода этих проектов, предположим, что здесь и ещё в нескольких местах допущены ошибки с приоритетом операций:

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

Чтобы проверить, выставлены ли определённые биты в переменной, следует использовать оператор &, а не |. Из-за опечатки в этом фрагменте кода получилось всегда истинное условие.

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

Я думаю, в параметре key хотели проверить определённый бит, заданный маской WWKEY_RLS_BIT, но сделали опечатку. Следовало использовать побитовый оператор &, а не &&, чтобы проверить код клавиши.

Подозрительное форматирование

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

Когда-то разработчик комментировал код для отладки. С тех пор в коде остался условный оператор с одинаковыми операторами в разных ветках.

Точно таких же мест нашлось ещё два:

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

Из-за большого комментария разработчик не увидел выше недописанный условный оператор. Оставшееся ключевое слово else образует с условием ниже конструкцию else if, что, скорее всего, является изменением изначальной логики.

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();    }//}

Ещё один потенциальный дефект из-за незаконченного рефакторинга. Теперь непонятно, переменная ScoresPresent должна иметь значение true, или всё-таки 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);}

Анализатор обнаружил ошибку, связанную с тем, что память может выделяется и освобождаться несовместимыми между собой способами. Для освобождения памяти, выделенной под массив, следовало использовать оператор delete[], а не delete.

Таких мест нашлось несколько, и все они понемногу вредят работающему приложению (игре):

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

Операторы delete и delete[] разделены неслучайно. Они выполняют разную работу по очистке памяти. А при использовании нетипизированного указателя компилятор не знает, на какой тип данных ведёт указатель. В стандарте языка C++ поведение компилятора неопределённо.

Такого рода тоже нашёлся целый ряд предупреждений анализатора:

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

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


Но тогда происходит утечка памяти, что тоже является ошибкой. Где-то в конце функции выполняется освобождение памяти, но до этого много мест, где происходит условный выход из функции, и память по указателям 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, а не параметр функции. Возможно, задумывалось другое поведение.

Ещё одно такое место:

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

Анализатор обнаружил потенциальную ошибку при переопределении виртуальной функции Occupy_List. Это может приводить к вызову не тех функций в рантайме.

Ещё несколько подозрительных мест:

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

Параметр coord сразу перезаписывается в теле функции. Старое значение не использовалось. Это очень подозрительно, когда у функции есть аргументы, а она от них не зависит. А тут ещё координаты какие-то передают.

И это место стоит проверить:

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

В коде игр присутствует очень много глобальных переменных. Вероятно, в те времена это был распространённый подход к написанию кода. Но сейчас он считается плохим и даже опасным.

В указатель InterpolationPalette сохраняется локальный массив localpalette, который станет невалидным после выхода из функции.

Ещё парочка опасных мест:

• 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

Заключение

Как я уже писал в первом отчёте, будем надеяться, что новые проекты Electronic Arts более качественные. Вообще, разработчики игр активно приобретают PVS-Studio. Сейчас бюджеты игр достаточно велики, поэтому лишние расходы на исправление багов в продакшене никому не нужны. А исправление ошибки на раннем этапе написания кода практически не отнимает время и другие ресурсы.

Приглашаем на наш сайт скачать и попробовать PVS-Studio на всех проектах.


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. The Code of the Command & Conquer Game: Bugs from the 90's. Volume two.