Код игры 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

Начать обзор хочется с вечного 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

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

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

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

• 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

Более объёмный фрагмент кода, который скопировали с последствиями. Согласитесь, кроме как с помощью анализатора, тут не заметишь, что из функции 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

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

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

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

V512 A call of the 'sprintf' function will lead to overflow of the buffer '(char *) ptr'. SOUNDDLG.CPP 250

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

V512 A call of the 'memset' function will lead to underflow of the buffer 'Buffer'. KEYBOARD.CPP 96

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

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

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

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

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

По-моему, я многократно видел эту ошибку и в современных проектах. Программисты до сих пор путают 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

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

• 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

Указатель 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

В этой функции происходит обработка вводимых символов. Как известно, в тип 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

В этом фрагменте кода анализатор нашёл применение операции инкремента к переменной типа 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

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

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

• 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

Тоже очень старая проблема, актуальная и в наши дни. Для работы с типом 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

Здесь происходит сдвиг отрицательного числа влево, что является неопределённым поведением. Отрицательное число получается из нуля при применении оператора инверсии. Т.к. результат операции помещается в тип 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

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

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

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

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

будет происходить сдвиг на 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-ю часть обзора к этой серии игр.

Первоисточник: Блог компании PVS-Studio.