SvyatoslavMC 13 июл 2020 в 09:34

Код игры Command & Conquer: баги из 90-х. Том второй

13 мин

11K

Блог компании PVS-StudioC*C++*Open source*Разработка игр*

Американская компания 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 have

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

Теги:

Хабы: