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

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