Американская компания Electronic Arts Inc (EA) выложила в открытый доступ исходный код игр Command & Conquer: Tiberian Dawn и Command & Conquer: Red Alert. Этот код должен помочь игровым сообществам разрабатывать моды и карты, создавать пользовательские юниты и настраивать логику игрового процесса. У всех нас появилась уникальная возможность окунуться в историю разработки, которая очень сильно отличается от современной. Тогда не было сайта Stack Overflow, удобных редакторов кода и мощных компиляторов. А ещё тогда не было статических анализаторов, и первое, с чем столкнётся сообщество, - это сотни ошибок в коде. Но с этим-то и поможет команда PVS-Studio, указав на места этих ошибок.
Command & Conquer - серия компьютерных игр в жанре стратегии в реальном времени. Первая игра серии была выпущена в 1995 году. Компания Electronic Arts приобрела студию разработки этой игры только в 1998 год.
С тех пор вышло несколько игр и множество модов. Исходный код игр опубликовали вместе с выпуском коллекции Command & Conquer Remastered.
Для поиска ошибок в коде использовался анализатор PVS-Studio. Это инструмент для выявления ошибок и потенциальных уязвимостей в исходном коде программ, написанных на языках С, C++, C# и Java.
Из-за большого объёма найденных проблем в коде, все примеры ошибок будут приведены в цикле из двух статей.
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)но невнимательность победила.
Точно такой же фрагмент кода скопирован в другую функцию:
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 функции, отличающиеся одним символов. Скорее всего, мы нашли настоящую ошибку.
И ниже по файлу нашёлся идентичный фрагмент кода:
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 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.
Ещё одно такое место:
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) {
....
}
....
}
....
}Явное обращение к нулевому указателю выглядит очень странно. Это место выглядит как опечатка и есть ещё несколько мест, которые стоит проверить:
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 просто содержит недостижимую ветку кода.
Таких мест было найдено несколько:
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 таких места было выявлено:
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();
}
....
}Программист завязал некую логику на сравнение значений разных перечислений. Технически это работает, т.к. сравниваются численные представления. Но такой код часто приводит к логическим ошибкам. Стоит поправить код (если этот проект ещё будут поддерживать, конечно).
Весь список предупреждений этой диагностики выглядит так:
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 и обратно. Эти два типа данных крайне похожи друг на друга с точки зрения языка, но логически несовместимы. Существующая в коде операция неявного приведения типов лишена смысла.
Это и ещё несколько мест стоило бы отрефакторить:
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 про сдвиги с неопределённым поведением:
Будем надеяться, что современные проекты Electronic Arts более качественные. Если нет, то приглашаем на наш сайт скачать и попробовать PVS-Studio на всех проектах.
Кто-то может возразить, что и с таким качеством раньше делали крутые успешные игры, и отчасти с этим можно согласиться. Но не надо забывать, что конкуренция в разработке программ и игр многократно выросла за столько лет. Выросли и расходы на разработку, поддержку и рекламу. Следовательно, исправление ошибок на поздних этапах разработки влечёт за собой значительные финансовые и репутационные убытки.
Следите за нашим блогом, чтобы не пропустить 2-ю часть обзора к этой серии игр.