HimeraSearchDB
Carding_EbayThief
triada
CrackerTuch
BMIX
issn24

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

Alvaros
Онлайн

Alvaros

.
Регистрация
14.05.16
Сообщения
21.452
Реакции
101
Репутация
204
45c686581b6cddb3473e6f0f7912deec.png


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

Введение


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

С тех пор вышло несколько игр и множество модов. Исходный код игр
You must be registered for see links
вместе с выпуском коллекции
You must be registered for see links
.

Для поиска ошибок в коде использовался анализатор
You must be registered for see links
. Это инструмент для выявления ошибок и потенциальных уязвимостей в исходном коде программ, написанных на языках С, 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) { // 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(); // View();
}
return(short)mono->Get_X(); // code>

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

В коде очень много глобальных переменных и очевидно, что в них легко запутаться. Предупреждение анализатора о выходе за границу массива выдаётся в месте обращения к массиву 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: // / code>

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

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

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

  • 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 '

void XMP_Randomize(digit * result, Straw & rng, int total_bits, int precision)
{
....
((unsigned char *)result)[nbytes-1] &=
(unsigned char)(~((~0) code>

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

#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)) code>

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


int bits_to_shift = bits % UNITSIZE;

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


int bits_to_shift = bits % 32;

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

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


.... code>

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

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

  • V610 Undefined behavior. Check the shift operator 'li>
    • V610 Undefined behavior. Check the shift operator ' 24)' is negative. ANIM.CPP 160
    • V610 Undefined behavior. Check the shift operator ' 24)' is negative. BUILDING.CPP 4037
    • V610 Undefined behavior. Check the shift operator ' 24)' is negative. DRIVE.CPP 2160
    • V610 Undefined behavior. Check the shift operator ' 24)' is negative. DRIVE.CPP 2161
    • V610 Undefined behavior. Check the shift operator ' 24)' is negative. DRIVE.CPP 2162
    • V610 Undefined behavior. Check the shift operator ' 24)' is negative. DRIVE.CPP 2163
    • V610 Undefined behavior. Check the shift operator ' 24)' is negative. DRIVE.CPP 2164
    • V610 Undefined behavior. Check the shift operator ' 24)' is negative. DRIVE.CPP 2165
    • V610 Undefined behavior. Check the shift operator ' 24)' is negative. DRIVE.CPP 2166
    • V610 Undefined behavior. Check the shift operator ' 24)' is negative. DRIVE.CPP 2167
    • V610 Undefined behavior. Check the shift operator ' 24)' is negative. DRIVE.CPP 2168
    • V610 Undefined behavior. Check the shift operator ' 24)' is negative. DRIVE.CPP 2169
    • V610 Undefined behavior. Check the shift operator ' 24)' is negative. DRIVE.CPP 2170
    • V610 Undefined behavior. Check the shift operator ' 24)' is negative. DRIVE.CPP 2171
    • V610 Undefined behavior. Check the shift operator ' 24)' is negative. DRIVE.CPP 2172
    • V610 Undefined behavior. Check the shift operator ' 24)' is negative. DRIVE.CPP 2173
    • V610 Undefined behavior. Check the shift operator ' 24)' is negative. DRIVE.CPP 2174
    • V610 Undefined behavior. Check the shift operator ' 24)' is negative. DRIVE.CPP 2175
    • V610 Undefined behavior. Check the shift operator ' 24)' is negative. DRIVE.CPP 2176
    • V610 Undefined behavior. Check the shift operator ' 24)' is negative. DRIVE.CPP 2177
    • V610 Undefined behavior. Check the shift operator ' 24)' is negative. DRIVE.CPP 2178
    • V610 Undefined behavior. Check the shift operator ' 24)' is negative. DRIVE.CPP 2179
    • V610 Undefined behavior. Check the shift operator ' 24)' is negative. DRIVE.CPP 2180
    • V610 Undefined behavior. Check the shift operator ' 24)' is negative. DRIVE.CPP 2181
    • V610 Undefined behavior. Check the shift operator ' 24)' is negative. DRIVE.CPP 2182
    • V610 Undefined behavior. Check the shift operator ' 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 'li>
      • V610 Undefined behavior. Check the shift operator 'li>

      • Заключение


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

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

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

        You must be registered for see links


        Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov.
        You must be registered for see links
        .
 
Войдите или зарегистрируйтесь для ответа.
Сверху Снизу