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