Американская компания Electronic Arts Inc (EA) выложила в открытый доступ исходный код игр Command & Conquer: Tiberian Dawn и Command & Conquer: Red Alert. В исходном коде было обнаружено несколько десятков ошибок с помощью анализатора PVS-Studio, поэтому встречайте продолжение описания найденных дефектов.
Command & Conquer - серия компьютерных игр в жанре стратегии в реальном времени. Первая игра серии была выпущена в 1995 году. Исходный код игр опубликовали вместе с выпуском коллекции Command & Conquer Remastered.
Для поиска ошибок в коде использовался анализатор PVS-Studio. Это инструмент для выявления ошибок и потенциальных уязвимостей в исходном коде программ, написанных на языках С, C++, C# и Java.
Ссылка на первый обзор ошибок: "Игра Command & Conquer: баги из 90-х. Том первый".
V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: 3072. STARTUP.CPP 1136
void Read_Setup_Options( RawFileClass *config_file )
{
....
ScreenHeight = ini.Get_Bool("Options", "Resolution", false) ? 3072 : 3072;
....
}Оказывается, на некоторые настройки пользователи не могли повлиять. Точнее они что-то делали, но из-за того, что тернарный оператор всегда возвращает одно значение, по факту ничего не менялось.
V590 Consider inspecting the 'i < 8 && i < 4' expression. The expression is excessive or contains a misprint. DLLInterface.cpp 2238
// Maximum number of multi players possible.
#define MAX_PLAYERS 8 // max # of players we can have
for (int i = 0; i < MAX_PLAYERS && i < 4; i++) {
if (GlyphxPlayerIDs[i] == player_id) {
MultiplayerStartPositions[i] = XY_Cell(x, y);
}
}Из-за неправильного цикла не задаётся позиция для всех игроков. С одной стороны, мы видим константу MAX_PLAYERS 8 и предполагаем, что это максимальное количество игроков. С другой – мы видим условие i < 4 и оператор &&. Таким образом, цикл никогда не делает 8 итераций. Скорее всего, на начальном этапе разработки программист не использовал константы, а когда начал – забыл удалить старые числа из кода.
V648 Priority of the '&&' operation is higher than that of the '||' operation. INFANTRY.CPP 1003
void InfantryClass::Assign_Target(TARGET target)
{
....
if (building && building->Class->IsCaptureable &&
(GameToPlay != GAME_NORMAL || *building != STRUCT_EYE && Scenario < 13)) {
Assign_Destination(target);
}
....
}Сделать код неочевидным (и, скорее всего, ошибочным) можно просто не указав приоритет операций для операторов || и &&. Здесь совсем непонятно, это ошибка или нет. Но учитывая общее качество кода этих проектов, предположим, что здесь и ещё в нескольких местах допущены ошибки с приоритетом операций:
V617 Consider inspecting the condition. The '((1L << STRUCT_CHRONOSPHERE))' argument of the '|' bitwise operation contains a non-zero value. HOUSE.CPP 5089
typedef enum StructType : char {
STRUCT_NONE=-1,
STRUCT_ADVANCED_TECH,
STRUCT_IRON_CURTAIN,
STRUCT_WEAP,
STRUCT_CHRONOSPHERE, // 3
....
}
#define STRUCTF_CHRONOSPHERE (1L << STRUCT_CHRONOSPHERE)
UrgencyType HouseClass::Check_Build_Power(void) const
{
....
if (State == STATE_THREATENED || State == STATE_ATTACKED) {
if (BScan | (STRUCTF_CHRONOSPHERE)) { // <=
urgency = URGENCY_HIGH;
}
}
....
}Чтобы проверить, выставлены ли определённые биты в переменной, следует использовать оператор &, а не |. Из-за опечатки в этом фрагменте кода получилось всегда истинное условие.
V768 The enumeration constant 'WWKEY_RLS_BIT' is used as a variable of a Boolean-type. KEYBOARD.CPP 286
typedef enum {
WWKEY_SHIFT_BIT = 0x100,
WWKEY_CTRL_BIT = 0x200,
WWKEY_ALT_BIT = 0x400,
WWKEY_RLS_BIT = 0x800,
WWKEY_VK_BIT = 0x1000,
WWKEY_DBL_BIT = 0x2000,
WWKEY_BTN_BIT = 0x8000,
} WWKey_Type;
int WWKeyboardClass::To_ASCII(int key)
{
if ( key && WWKEY_RLS_BIT)
return(KN_NONE);
return(key);
}Я думаю, в параметре key хотели проверить определённый бит, заданный маской WWKEY_RLS_BIT, но сделали опечатку. Следовало использовать побитовый оператор &, а не &&, чтобы проверить код клавиши.
V523 The 'then' statement is equivalent to the 'else' statement. RADAR.CPP 1827
void RadarClass::Player_Names(bool on)
{
IsPlayerNames = on;
IsToRedraw = true;
if (on) {
Flag_To_Redraw(true);
// Flag_To_Redraw(false);
} else {
Flag_To_Redraw(true); // force drawing of the plate
}
}Когда-то разработчик комментировал код для отладки. С тех пор в коде остался условный оператор с одинаковыми операторами в разных ветках.
Точно таких же мест нашлось ещё два:
V705 It is possible that 'else' block was forgotten or commented out, thus altering the program's operation logics. NETDLG.CPP 1506
static int Net_Join_Dialog(void)
{
....
/*...............................................................
F4/SEND/'M' = edit a message
...............................................................*/
if (Messages.Get_Edit_Buf()==NULL) {
....
} else
/*...............................................................
If we're already editing a message and the user clicks on
'Send', translate our input to a Return so Messages.Input() will
work properly.
...............................................................*/
if (input==(BUTTON_SEND | KN_BUTTON)) {
input = KN_RETURN;
}
....
}Из-за большого комментария разработчик не увидел выше недописанный условный оператор. Оставшееся ключевое слово else образует с условием ниже конструкцию else if, что, скорее всего, является изменением изначальной логики.
V519 The 'ScoresPresent' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 539, 541. INIT.CPP 541
bool Init_Game(int , char *[])
{
....
ScoresPresent = false;
//if (CCFileClass("SCORES.MIX").Is_Available()) {
ScoresPresent = true;
if (!ScoreMix) {
ScoreMix = new MixFileClass("SCORES.MIX");
ThemeClass::Scan();
}
//}Ещё один потенциальный дефект из-за незаконченного рефакторинга. Теперь непонятно, переменная ScoresPresent должна иметь значение true, или всё-таки false.
V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] poke_data;'. CCDDE.CPP 410
BOOL Send_Data_To_DDE_Server (char *data, int length, int packet_type)
{
....
char *poke_data = new char [length + 2*sizeof(int)]; // <=
....
if(DDE_Class->Poke_Server( .... ) == FALSE) {
CCDebugString("C&C95 - POKE failed!\n");
DDE_Class->Close_Poke_Connection();
delete poke_data; // <=
return (FALSE);
}
DDE_Class->Close_Poke_Connection();
delete poke_data; // <=
return (TRUE);
}Анализатор обнаружил ошибку, связанную с тем, что память может выделяется и освобождаться несовместимыми между собой способами. Для освобождения памяти, выделенной под массив, следовало использовать оператор delete[], а не delete.
Таких мест нашлось несколько, и все они понемногу вредят работающему приложению (игре):
V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. ENDING.CPP 254
void GDI_Ending(void)
{
....
void * localpal = Load_Alloc_Data(CCFileClass("SATSEL.PAL"));
....
delete [] localpal;
....
}Операторы delete и delete[] разделены неслучайно. Они выполняют разную работу по очистке памяти. А при использовании нетипизированного указателя компилятор не знает, на какой тип данных ведёт указатель. В стандарте языка C++ поведение компилятора неопределённо.
Такого рода тоже нашёлся целый ряд предупреждений анализатора:
V773 The function was exited without releasing the 'progresspalette' pointer. A memory leak is possible. MAPSEL.CPP 258
void Map_Selection(void)
{
....
unsigned char *grey2palette = new unsigned char[768];
unsigned char *progresspalette = new unsigned char[768];
....
scenario = Scenario + ((house == HOUSE_GOOD) ? 0 : 14);
if (house == HOUSE_GOOD) {
lastscenario = (Scenario == 14);
if (Scenario == 15) return;
} else {
lastscenario = (Scenario == 12);
if (Scenario == 13) return;
}
....
}"Если вообще не освобождать память, то точно не ошибусь в выборе оператора!" - возможно, подумал программист.
Но тогда происходит утечка памяти, что тоже является ошибкой. Где-то в конце функции выполняется освобождение памяти, но до этого много мест, где происходит условный выход из функции, и память по указателям grey2palette и progresspalett не освобождается.
V570 The 'hdr->MagicNumber' variable is assigned to itself. COMBUF.CPP 806
struct CommHdr {
unsigned short MagicNumber;
unsigned char Code;
unsigned long PacketID;
} *hdr;
void CommBufferClass::Mono_Debug_Print(int refresh)
{
....
hdr = (CommHdr *)SendQueue[i].Buffer;
hdr->MagicNumber = hdr->MagicNumber;
hdr->Code = hdr->Code;
....
}Два поля структуры CommHdr инициализируются собственными значениями. По-моему, бессмысленная операция, но выполняется она много раз:
V591 Non-void function should return a value. HEAP.H 123
int FixedHeapClass::Free(void * pointer);
template<class T>
class TFixedHeapClass : public FixedHeapClass
{
....
virtual int Free(T * pointer) {FixedHeapClass::Free(pointer);};
};В функции Free класса TFixedHeapClass нет оператора return. Самое интересное, что у вызываемой функции FixedHeapClass::Free возвращаемое значение тоже типа int. Скорее всего, программист просто забыл написать оператор return и теперь функция возвращает непонятное значение.
V672 There is probably no need in creating the new 'damage' variable here. One of the function's arguments possesses the same name and this argument is a reference. Check lines: 1219, 1278. BUILDING.CPP 1278
ResultType BuildingClass::Take_Damage(int & damage, ....)
{
....
if (tech && tech->IsActive && ....) {
int damage = 500;
tech->Take_Damage(damage, 0, WARHEAD_AP, source, forced);
}
....
}Параметр damage передаётся по ссылке. Следовательно, в теле функции ожидается изменение значений этой переменной. Но в одном месте разработчик объявил переменную с таким же именем. Из-за этого значение 500 сохранится в локальную переменную damage, а не параметр функции. Возможно, задумывалось другое поведение.
Ещё одно такое место:
V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'Occupy_List' in derived class 'BulletClass' and base class 'ObjectClass'. BULLET.H 90
class ObjectClass : public AbstractClass
{
....
virtual short const * Occupy_List(bool placement=false) const; // <=
virtual short const * Overlap_List(void) const;
....
};
class BulletClass : public ObjectClass,
public FlyClass,
public FuseClass
{
....
virtual short const * Occupy_List(void) const; // <=
virtual short const * Overlap_List(void) const {return Occupy_List();};
....
};Анализатор обнаружил потенциальную ошибку при переопределении виртуальной функции Occupy_List. Это может приводить к вызову не тех функций в рантайме.
Ещё несколько подозрительных мест:
V763 Parameter 'coord' is always rewritten in function body before being used. DISPLAY.CPP 4031
void DisplayClass::Set_Tactical_Position(COORDINATE coord)
{
int xx = 0;
int yy = 0;
Confine_Rect(&xx, &yy, TacLeptonWidth, TacLeptonHeight,
Cell_To_Lepton(MapCellWidth) + GlyphXClientSidebarWidthInLeptons,
Cell_To_Lepton(MapCellHeight));
coord = XY_Coord(xx + Cell_To_Lepton(MapCellX), yy + Cell_To_Lepton(....));
if (ScenarioInit) {
TacticalCoord = coord;
}
DesiredTacticalCoord = coord;
IsToRedraw = true;
Flag_To_Redraw(false);
}Параметр coord сразу перезаписывается в теле функции. Старое значение не использовалось. Это очень подозрительно, когда у функции есть аргументы, а она от них не зависит. А тут ещё координаты какие-то передают.
И это место стоит проверить:
V507 Pointer to local array 'localpalette' is stored outside the scope of this array. Such a pointer will become invalid. MAPSEL.CPP 757
extern "C" unsigned char *InterpolationPalette;
void Map_Selection(void)
{
unsigned char localpalette[768];
....
InterpolationPalette = localpalette;
....
}В коде игр присутствует очень много глобальных переменных. Вероятно, в те времена это был распространённый подход к написанию кода. Но сейчас он считается плохим и даже опасным.
В указатель InterpolationPalette сохраняется локальный массив localpalette, который станет невалидным после выхода из функции.
Ещё парочка опасных мест:
Как я уже писал в первом отчёте, будем надеяться, что новые проекты Electronic Arts более качественные. Вообще, разработчики игр активно приобретают PVS-Studio. Сейчас бюджеты игр достаточно велики, поэтому лишние расходы на исправление багов в продакшене никому не нужны. А исправление ошибки на раннем этапе написания кода практически не отнимает время и другие ресурсы.
Приглашаем на наш сайт скачать и попробовать PVS-Studio на всех проектах.