SvyatoslavMC 18 июн 2020 в 09:23

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

13 мин

12K

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

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

Введение

Command & Conquer — серия компьютерных игр в жанре стратегии в реальном времени. Первая игра серии была выпущена в 1995 году. Компания Electronic Arts приобрела студию разработки этой игры только в 1998 год.

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

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

Из-за большого объёма найденных проблем в коде, все примеры ошибок будут приведены в цикле из двух статей.

Опечатки и copy-paste

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

Начать обзор хочется с вечного copy-paste. В функции для копирования списков ни разу не проверили указатель на источник и 2 раза проверили указатель-назначение, потому что скопировали проверку dest == NULL и забыли заменить имя переменной.

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

Анализатор обнаружил бессмысленное сравнение. Я полагаю, что там должно было быть что-то вроде:

if (Current-adder != Credits)

но невнимательность победила.

Точно такой же фрагмент кода скопирован в другую функцию:

V584 The 'Current' value is present on both sides of the '!=' operator. The expression is incorrect or it can be simplified. CREDITS.CPP 246

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

Более объёмный фрагмент кода, который скопировали с последствиями. Согласитесь, кроме как с помощью анализатора, тут не заметишь, что из функции Mono_Y позвали функцию Get_X, вместо Get_Y. У класса MonoClass действительно есть 2 функции, отличающиеся одним символов. Скорее всего, мы нашли настоящую ошибку.

И ниже по файлу нашёлся идентичный фрагмент кода:

V524 It is odd that the body of 'Mono_Y' function is fully equivalent to the body of 'Mono_X' function. MONOC.CPP 1083

Ошибки с массивами

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

Похоже, это отладочный метод, но сколько вреда он мог нанести психике разработчика история умалчивает. Здесь массив Path состоит из 9 элементов, а печатаются все 13.

Итого 4 обращения к памяти за границу массива:

V557 Array overrun is possible. The '9' index is pointing beyond array bound. FOOT.CPP 232
V557 Array overrun is possible. The '10' index is pointing beyond array bound. FOOT.CPP 233
V557 Array overrun is possible. The '11' index is pointing beyond array bound. FOOT.CPP 234
V557 Array overrun is possible. The '12' index is pointing beyond array bound. FOOT.CPP 235

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

На первый взгляд, пример сложный, но в нём легко разобраться после небольшого анализа.

К двумерному массиву _MoveSpillage обращаются по индексу, который берётся из массива _SpillTable. А он внезапно содержит отрицательные значения. Возможно, доступ к данным организован по особой формуле и это то, что задумывал разработчик. Но я не уверен в этом.

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

Внимательный читатель задастся вопросом, почему такая длинная строка сохранится в буфер из 4-х байт? А потому, что программист думал, что sizeof(100) вернёт что-то больше (как минимум 100). Но оператор sizeof возвращает размер типа и даже никогда не считает никакие выражения. Надо было написать просто константу 100, а ещё лучше использовать именованные константы, или другой тип для строк или указателя.

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

Некий буфер очищается на 256 байт, хотя полный размер оригинального буфера 256*sizeof(unsigned short). Ошибочка.

Можно так ещё исправить:

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

В коде очень много глобальных переменных и очевидно, что в них легко запутаться. Предупреждение анализатора о выходе за границу массива выдаётся в месте обращения к массиву BQuantity по индексу. Размер массива – 84 элемента. Алгоритмы анализа потока данных в анализаторе помогли установить, что значение индекса приходит из другой функции – Goodie_Check. Там выполняется цикл с конечным значением 86. Следовательно, в этом месте постоянно происходит чтение 12 байт "чужой" памяти (3 элемента типа int).

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

По-моему, я многократно видел эту ошибку и в современных проектах. Программисты до сих пор путают 2-й и 3-й аргументы функции memset.

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

V575 The 'memset' function processes '0' elements. Inspect the third argument. DLLInterface.cpp 1404

Про нулевые указатели

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

Явное обращение к нулевому указателю выглядит очень странно. Это место выглядит как опечатка и есть ещё несколько мест, которые стоит проверить:

V522 Dereferencing of the null pointer 'list' might take place. DISPLAY.CPP 951
V522 Dereferencing of the null pointer 'unitsptr' might take place. QUEUE.CPP 2362
V522 Dereferencing of the null pointer 'unitsptr' might take place. QUEUE.CPP 2699

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

Указатель enemy разыменовывают, а потом проверяют, что он ненулевой. До сих пор актуальная проблема, не побоюсь этого слова, каждого проекта с открытым исходным кодом. Уверен, что в проектах с закрытым кодом ситуация примерно такая же, если, конечно, не используется PVS-Studio ;-)

Неправильные касты

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

В этой функции происходит обработка вводимых символов. Как известно, в тип char помещается 1-байтовое значение, и числа 4109 там никогда не будет. Так, этот оператор switch просто содержит недостижимую ветку кода.

Таких мест было найдено несколько:

V551 The code under this 'case' label is unreachable. The '4105' value of the 'char' type is not in the range [-128; 127]. WINDOWS.CPP 584
V551 The code under this 'case' label is unreachable. The '4123' value of the 'char' type is not in the range [-128; 127]. WINDOWS.CPP 628

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

В этом фрагменте кода анализатор нашёл применение операции инкремента к переменной типа bool. Это корректный код с точки зрения языка, но очень странно выглядит сейчас. Также такая операция помечена как deprecated, начиная cо стандарта C++17.

Всего 2 таких места было выявлено:

V552 A bool type variable is being incremented: done ++. Perhaps another variable should be incremented instead. ENDING.CPP 187

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

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

Весь список предупреждений этой диагностики выглядит так:

V556 The values of different enum types are compared: SoundEffectName[voc].Where == IN_JUV. DLLInterface.cpp 402
V556 The values of different enum types are compared: SoundEffectName[voc].Where == IN_VAR. DLLInterface.cpp 405
V556 The values of different enum types are compared: Map.Theater == CNC_THEATER_DESERT. Types: TheaterType, CnCTheaterType. DLLInterface.cpp 2805
V556 The values of different enum types are compared. Types: ImpactType, ResultType. AIRCRAFT.CPP 4269
V556 The values of different enum types are compared: SoundEffectName[voc].Where == IN_VAR. DLLInterface.cpp 429

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

Тоже очень старая проблема, актуальная и в наши дни. Для работы с типом HRESULT существуют специальные макросы, а не используется приведение в BOOL и обратно. Эти два типа данных крайне похожи друг на друга с точки зрения языка, но логически несовместимы. Существующая в коде операция неявного приведения типов лишена смысла.

Это и ещё несколько мест стоило бы отрефакторить:

V716 Suspicious type conversion in assign expression: 'HRESULT = BOOL'. GBUFFER.H 817
V716 Suspicious type conversion in assign expression: 'HRESULT = BOOL'. GBUFFER.H 857
V716 Suspicious type conversion in assign expression: 'HRESULT = BOOL'. GBUFFER.H 773
V716 Suspicious type conversion in assign expression: 'HRESULT = BOOL'. GBUFFER.H 810
V716 Suspicious type conversion in assign expression: 'HRESULT = BOOL'. GBUFFER.H 850

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

Здесь происходит сдвиг отрицательного числа влево, что является неопределённым поведением. Отрицательное число получается из нуля при применении оператора инверсии. Т.к. результат операции помещается в тип int, то компилятор использует его для хранения значения, а он знаковый.

В 2020 году такую ошибку находит уже и компилятор:

Warning C26453: Arithmetic overflow: Left shift of a negative signed number is undefined behavior.

Но компиляторы не являются полноценными статическими анализаторами, т.к. решают другие задачи. Поэтому вот ещё один пример неопределённого поведения, который выявляется только с помощью 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++;
  }
  ....
}

Анализатор обнаружил ситуацию, где потенциально возможен сдвиг вправо 32-х битного числа на большее количество разрядов, чем там есть. Вот, как это получается:

int bits_to_shift = bits % UNITSIZE;

Константа UNITSIZE имеет значение 32:

int bits_to_shift = bits % 32;

Так, значение переменной bits_to_shift будет равно нулю для всех значений bits, кратных числу 32.

Следовательно, в этом фрагменте кода:

.... << (UNITSIZE - bits_to_shift) ....

будет происходить сдвиг на 32 разряда, если из константы 32 будет вычитаться 0.

Список всех предупреждений PVS-Studio про сдвиги с неопределённым поведением:

V610 Undefined behavior. Check the shift operator '<<'. The left operand '(~0)' is negative. TARGET.H 66
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 24) * 256) / 24)' is negative. ANIM.CPP 160
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 12) * 256) / 24)' is negative. BUILDING.CPP 4037
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 21) * 256) / 24)' is negative. DRIVE.CPP 2160
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 21) * 256) / 24)' is negative. DRIVE.CPP 2161
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 20) * 256) / 24)' is negative. DRIVE.CPP 2162
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 20) * 256) / 24)' is negative. DRIVE.CPP 2163
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 18) * 256) / 24)' is negative. DRIVE.CPP 2164
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 18) * 256) / 24)' is negative. DRIVE.CPP 2165
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 17) * 256) / 24)' is negative. DRIVE.CPP 2166
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 16) * 256) / 24)' is negative. DRIVE.CPP 2167
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 15) * 256) / 24)' is negative. DRIVE.CPP 2168
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 14) * 256) / 24)' is negative. DRIVE.CPP 2169
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 13) * 256) / 24)' is negative. DRIVE.CPP 2170
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 12) * 256) / 24)' is negative. DRIVE.CPP 2171
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 11) * 256) / 24)' is negative. DRIVE.CPP 2172
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 10) * 256) / 24)' is negative. DRIVE.CPP 2173
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 9) * 256) / 24)' is negative. DRIVE.CPP 2174
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 8) * 256) / 24)' is negative. DRIVE.CPP 2175
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 7) * 256) / 24)' is negative. DRIVE.CPP 2176
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 6) * 256) / 24)' is negative. DRIVE.CPP 2177
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 5) * 256) / 24)' is negative. DRIVE.CPP 2178
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 4) * 256) / 24)' is negative. DRIVE.CPP 2179
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 3) * 256) / 24)' is negative. DRIVE.CPP 2180
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 2) * 256) / 24)' is negative. DRIVE.CPP 2181
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 1) * 256) / 24)' is negative. DRIVE.CPP 2182
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(((- 5) * 256) / 24)' is negative. INFANTRY.CPP 2730
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 743
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(~0)' is negative. RANDOM.CPP 102
V610 Undefined behavior. Check the shift operator '<<'. The left operand '(~0L)' is negative. RANDOM.CPP 164

Заключение

Будем надеяться, что современные проекты Electronic Arts более качественные. Если нет, то приглашаем на наш сайт скачать и попробовать PVS-Studio на всех проектах.

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

Следите за нашим блогом, чтобы не пропустить 2-ю часть обзора к этой серии игр.

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

Теги:

Хабы: