Command and conquer

Американская компания 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 havefor (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);  }  ....}

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

• V648 Priority of the '&&' operation is higher than that of the '||' operation. TEAM.CPP 456
• V648 Priority of the '&&' operation is higher than that of the '||' operation. DISPLAY.CPP 1160
• V648 Priority of the '&&' operation is higher than that of the '||' operation. DISPLAY.CPP 1571
• V648 Priority of the '&&' operation is higher than that of the '||' operation. HOUSE.CPP 2594
• V648 Priority of the '&&' operation is higher than that of the '||' operation. INIT.CPP 2541

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  }}

Когда-то разработчик комментировал код для отладки. С тех пор в коде остался условный оператор с одинаковыми операторами в разных ветках.

Точно таких же мест нашлось ещё два:

• V523 The 'then' statement is equivalent to the 'else' statement. CELL.CPP 1792
• V523 The 'then' statement is equivalent to the 'else' statement. RADAR.CPP 2274

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.

Таких мест нашлось несколько, и все они понемногу вредят работающему приложению (игре):

• 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 416
• 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 [] temp_buffer;'. INIT.CPP 1302
• 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 [] progresspalette;'. MAPSEL.CPP 795
• 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 [] grey2palette;'. MAPSEL.CPP 796
• 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 422
• 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 [] temp_buffer;'. INIT.CPP 1139

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++ поведение компилятора неопределённо.

Такого рода тоже нашёлся целый ряд предупреждений анализатора:

• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. HEAP.CPP 284
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. INIT.CPP 728
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MIXFILE.CPP 134
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MIXFILE.CPP 391
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MSGBOX.CPP 423
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. SOUNDDLG.CPP 407
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. BUFFER.CPP 126
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. BUFF.CPP 162
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. BUFF.CPP 212
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. BFIOFILE.CPP 330
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. EVENT.CPP 934
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. HEAP.CPP 318
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. INIT.CPP 3851
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MIXFILE.CPP 130
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MIXFILE.CPP 430
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MIXFILE.CPP 447
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MIXFILE.CPP 481
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. MSGBOX.CPP 461
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. QUEUE.CPP 2982
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. QUEUE.CPP 3167
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. SOUNDDLG.CPP 406

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 инициализируются собственными значениями. По-моему, бессмысленная операция, но выполняется она много раз:

• V570 The 'hdr->Code' variable is assigned to itself. COMBUF.CPP 807
• V570 The 'hdr->MagicNumber' variable is assigned to itself. COMBUF.CPP 931
• V570 The 'hdr->Code' variable is assigned to itself. COMBUF.CPP 932
• V570 The 'hdr->MagicNumber' variable is assigned to itself. COMBUF.CPP 987
• V570 The 'hdr->Code' variable is assigned to itself. COMBUF.CPP 988
• V570 The 'obj' variable is assigned to itself. MAP.CPP 1132
• V570 The 'hdr->MagicNumber' variable is assigned to itself. COMBUF.CPP 910
• V570 The 'hdr->Code' variable is assigned to itself. COMBUF.CPP 911
• V570 The 'hdr->MagicNumber' variable is assigned to itself. COMBUF.CPP 1040
• V570 The 'hdr->Code' variable is assigned to itself. COMBUF.CPP 1041
• V570 The 'hdr->MagicNumber' variable is assigned to itself. COMBUF.CPP 1104
• V570 The 'hdr->Code' variable is assigned to itself. COMBUF.CPP 1105
• V570 The 'obj' variable is assigned to itself. MAP.CPP 1279

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, а не параметр функции. Возможно, задумывалось другое поведение.

Ещё одно такое место:

• 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: 4031, 4068. TECHNO.CPP 4068

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. Это может приводить к вызову не тех функций в рантайме.

Ещё несколько подозрительных мест:

• V762 It is possible a virtual function was overridden incorrectly. See qualifiers of function 'Ok_To_Move' in derived class 'TurretClass' and base class 'DriveClass'. TURRET.H 76
• V762 It is possible a virtual function was overridden incorrectly. See fourth argument of function 'Help_Text' in derived class 'HelpClass' and base class 'DisplayClass'. HELP.H 55
• V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'Draw_It' in derived class 'MapEditClass' and base class 'HelpClass'. MAPEDIT.H 187
• V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'Occupy_List' in derived class 'AnimClass' and base class 'ObjectClass'. ANIM.H 80
• V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'Overlap_List' in derived class 'BulletClass' and base class 'ObjectClass'. BULLET.H 102
• V762 It is possible a virtual function was overridden incorrectly. See qualifiers of function 'Remap_Table' in derived class 'BuildingClass' and base class 'TechnoClass'. BUILDING.H 281
• V762 It is possible a virtual function was overridden incorrectly. See fourth argument of function 'Help_Text' in derived class 'HelpClass' and base class 'DisplayClass'. HELP.H 58
• V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'Overlap_List' in derived class 'AnimClass' and base class 'ObjectClass'. ANIM.H 90

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

И это место стоит проверить:

• V763 Parameter 'coord' is always rewritten in function body before being used. DISPLAY.CPP 4251

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

Ещё парочка опасных мест:

• V507 Pointer to local array 'localpalette' is stored outside the scope of this array. Such a pointer will become invalid. MAPSEL.CPP 769
• V507 Pointer to local array 'buffer' is stored outside the scope of this array. Such a pointer will become invalid. WINDOWS.CPP 458

Заключение

Как я уже писал в первом отчёте, будем надеяться, что новые проекты Electronic Arts более качественные. Вообще, разработчики игр активно приобретают PVS-Studio. Сейчас бюджеты игр достаточно велики, поэтому лишние расходы на исправление багов в продакшене никому не нужны. А исправление ошибки на раннем этапе написания кода практически не отнимает время и другие ресурсы.

Приглашаем на наш сайт скачать и попробовать PVS-Studio на всех проектах.


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. The Code of the Command & Conquer Game: Bugs from the 90's. Volume two.

Данная статья посвящена проверке проекта OpenRA с помощью статического анализатора PVS-Studio. Что такое OpenRA? Это игровой движок с открытым исходным кодом, предназначенный для создания стратегий в реальном времени. В статье рассказывается о том, как проводился анализ, какие особенности самого проекта были обнаружены и какие интересные срабатывания выдал PVS-Studio. Ну и, конечно же, здесь будут рассмотрены некоторые особенности анализатора, которые позволили сделать процесс проверки проекта более комфортным.

OpenRA

Проект, выбранный для проверки, представляет собой игровой движок для RTS в стиле таких игр, как Command & Conquer: Red Alert. Более подробную информацию можно найти на сайте. Исходный код написан на C# и доступен для просмотра и использования в репозитории.

OpenRA был выбран для проверки по 3 причинам. Во-первых, он, судя по всему, представляет интерес для многих людей. Во всяком случае, это касается обитателей GitHub, так как репозиторий собрал более 8 тысяч звёзд. Во-вторых, кодовая база OpenRA насчитывает 1285 файлов. Обычно такого количества вполне достаточно, чтобы надеяться найти в них интересные срабатывания. Ну и в-третьих Игровые движки это круто :)

Лишние срабатывания

Я анализировал OpenRA с помощью PVS-Studio и поначалу был воодушевлён результатами:


Я решил, что среди такого количества High-предупреждений уж точно смогу найти целую уйму самых разных срабатываний. И, конечно же, на их основе напишу самую крутую и интересную статью :) Но не тут-то было!

Стоило лишь взглянуть на сами предупреждения, и всё сразу встало на свои места. 1277 из 1306 предупреждений уровня High были связаны с диагностикой V3144. Она выдаёт сообщения вида "This file is marked with copyleft license, which requires you to open the derived source code". Более подробно данная диагностика описана здесь.

Очевидно, что срабатывания подобного плана меня совершенно не интересовали, ведь OpenRA и так является open-source проектом. Поэтому их необходимо было скрыть, чтобы они не мешали просмотру остальной части лога. Так как я пользовался плагином для Visual Studio, то сделать это было легко. Нужно было просто кликнуть правой кнопкой по одному из срабатываний V3144 и в открывшемся меню выбрать "Hide all V3144 errors".


Также можно выбрать, какие предупреждения будут отображены в логе, перейдя в раздел "Detectable Errors (C#)" в опциях анализатора.


Для того, чтобы перейти к ним, используя плагин для Visual Studio 2019, нужно кликнуть в верхнем меню Extensions->PVS-Studio->Options.

Результаты проверки

После того как срабатывания V3144 были отфильтрованы, предупреждений в логе стало значительно меньше:


Тем не менее среди них удалось найти интересные моменты.

Бессмысленные условия

Достаточно многие срабатывания указывали на лишние проверки. Подобное может свидетельствовать об ошибке, ведь намеренно такой код обычно люди не пишут. Однако в OpenRA довольно часто всё выглядит так, будто эти ненужные условия добавлены специально. Например:

public virtual void Tick(){  ....  Active = !Disabled && Instances.Any(i => !i.IsTraitPaused);  if (!Active)    return;  if (Active)  {    ....  }}

Предупреждение анализатора: V3022 Expression 'Active' is always true. SupportPowerManager.cs 206

PVS-Studio вполне справедливо подмечает, что вторая проверка бессмысленна, ведь если Active будет false, то до неё выполнение не дойдёт. Может быть тут и правда ошибка, но я думаю, что это написано намеренно. Зачем? Ну а почему бы и нет?

Возможно, перед нами некое временное решение, доработка которого оставлена "на потом". В таких случаях достаточно удобно, что анализатор будет напоминать разработчику о подобных недоделках.

Давайте рассмотрим ещё одну проверку "на всякий случай":

Pair<string, bool>[] MakeComponents(string text){  ....  if (highlightStart > 0 && highlightEnd > highlightStart)  // <=  {    if (highlightStart > 0)                                 // <=    {      // Normal line segment before highlight      var lineNormal = line.Substring(0, highlightStart);      components.Add(Pair.New(lineNormal, false));    }      // Highlight line segment    var lineHighlight = line.Substring(      highlightStart + 1,       highlightEnd - highlightStart  1    );    components.Add(Pair.New(lineHighlight, true));    line = line.Substring(highlightEnd + 1);  }  else  {    // Final normal line segment    components.Add(Pair.New(line, false));    break;  }  ....}

Предупреждение анализатора: V3022 Expression 'highlightStart > 0' is always true. LabelWithHighlightWidget.cs 54

Опять же, очевидно, что повторная проверка полностью лишена смысла. Значение highlightStart проверяется дважды, причём в соседних строчках. Ошибка? Возможно, в одном из условий выбраны не те переменные для проверки. Так или иначе, сложно сказать наверняка, в чём тут дело. Ясно одно код надо изучить и поправить или оставить пояснение, если дополнительная проверка всё-таки зачем-то нужна.

Вот ещё один подобный момент:

public static void ButtonPrompt(....){  ....  var cancelButton = prompt.GetOrNull<ButtonWidget>(    "CANCEL_BUTTON"  );  ....  if (onCancel != null && cancelButton != null)  {    cancelButton.Visible = true;    cancelButton.Bounds.Y += headerHeight;    cancelButton.OnClick = () =>    {      Ui.CloseWindow();      if (onCancel != null)        onCancel();    };    if (!string.IsNullOrEmpty(cancelText) && cancelButton != null)      cancelButton.GetText = () => cancelText;  }  ....}

Предупреждение анализатора: V3063 A part of conditional expression is always true if it is evaluated: cancelButton != null. ConfirmationDialogs.cs 78

cancelButton действительно может быть null, ведь в эту переменную записывается значение, возвращаемое методом GetOrNull. Однако логично посчитать, что в теле условного оператора cancelButton никаким образом не превратится в null. Тем не менее проверка всё равно есть. Если не обратить внимание на внешнее условие, то вообще получается странная ситуация: сначала производится обращение к свойствам переменной, а потом разработчик решил убедиться всё-таки null там или нет? :)

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

В игровом движке Unity, например, оператор "==" переопределён для класса UnityEngine.Object. В официальной документации, доступной по ссылке, показано, что сравнение экземпляров этого класса с null работает не так, как обычно. Что ж, наверняка у разработчиков были причины для реализации подобной необычной логики.

В OpenRA я чего-то такого не нашёл :). Так что если в рассмотренных ранее проверках на null и есть какой-то смысл, то состоит он в чём-то другом.

PVS-Studio смог обнаружить ещё несколько аналогичных моментов, но ни к чему перечислять здесь их все. Всё же скучновато смотреть одни и те же срабатывания. К счастью (или нет) анализатор смог отыскать и другие странности.

Недостижимый код

void IResolveOrder.ResolveOrder(Actor self, Order order){  ....  if (!order.Queued || currentTransform == null)    return;    if (!order.Queued && currentTransform.NextActivity != null)    currentTransform.NextActivity.Cancel(self);  ....}

Предупреждение анализатора: V3022 Expression '!order.Queued && currentTransform.NextActivity != null' is always false. TransformsIntoTransforms.cs 44

И вновь перед нами бессмысленная проверка. Правда, в отличие от предыдущих, здесь представлено уже не просто лишнее условие, а самый настоящий недостижимый код. Рассмотренные ранее always true проверки по сути не влияли на работу программы. Их можно убрать из кода, а можно оставить ничего не изменится.

Здесь же странная проверка приводит к тому, что часть кода не выполняется. При этом мне сложно предположить, какие изменения здесь должны быть внесены в качестве поправки. В самом простом и приятном случае недостижимый код попросту и не должен выполняться. Тогда никакой ошибки тут нет. Однако я сомневаюсь, что программист сознательно написал строку просто для красоты.

Неинициализированная переменная в конструкторе

public class CursorSequence{  ....  public readonly ISpriteFrame[] Frames;  public CursorSequence(    FrameCache cache,     string name,     string cursorSrc,     string palette,     MiniYaml info  )  {    var d = info.ToDictionary();    Start = Exts.ParseIntegerInvariant(d["Start"].Value);    Palette = palette;    Name = name;    if (      (d.ContainsKey("Length") && d["Length"].Value == "*") ||       (d.ContainsKey("End") && d["End"].Value == "*")    )       Length = Frames.Length - Start;    else if (d.ContainsKey("Length"))      Length = Exts.ParseIntegerInvariant(d["Length"].Value);    else if (d.ContainsKey("End"))      Length = Exts.ParseIntegerInvariant(d["End"].Value) - Start;    else      Length = 1;    Frames = cache[cursorSrc]      .Skip(Start)      .Take(Length)      .ToArray();    ....  }}

Предупреждение анализатора: V3128 The 'Frames' field is used before it is initialized in constructor. CursorSequence.cs 35

Весьма неприятный момент. Попытка получения значения свойства Length у неинициализированной переменной неизбежно приведёт к выбрасыванию NullReferenceException. В обычной ситуации вряд ли такая ошибка осталась бы незамеченной всё же невозможность создания экземпляра класса легко себя обнаруживает. Но здесь исключение будет выброшено только в том случае, если условие

(d.ContainsKey("Length") && d["Length"].Value == "*") || (d.ContainsKey("End") && d["End"].Value == "*")

будет истинным.

Трудно судить о том, как нужно поправить код, чтобы всё было хорошо. Я могу лишь предположить, что функция должна выглядеть как-то так:

public CursorSequence(....){  var d = info.ToDictionary();  Start = Exts.ParseIntegerInvariant(d["Start"].Value);  Palette = palette;  Name = name;  ISpriteFrame[] currentCache = cache[cursorSrc];      if (    (d.ContainsKey("Length") && d["Length"].Value == "*") ||     (d.ContainsKey("End") && d["End"].Value == "*")  )     Length = currentCache.Length - Start;  else if (d.ContainsKey("Length"))    Length = Exts.ParseIntegerInvariant(d["Length"].Value);  else if (d.ContainsKey("End"))    Length = Exts.ParseIntegerInvariant(d["End"].Value) - Start;  else    Length = 1;  Frames = currentCache    .Skip(Start)    .Take(Length)    .ToArray();  ....}

В данном варианте указанная проблема отсутствует, однако сказать, насколько он соответствует изначальной задумке, сможет только разработчик.

Потенциальная опечатка

public void Resize(int width, int height){  var oldMapTiles = Tiles;  var oldMapResources = Resources;  var oldMapHeight = Height;  var oldMapRamp = Ramp;  var newSize = new Size(width, height);  ....  Tiles = CellLayer.Resize(oldMapTiles, newSize, oldMapTiles[MPos.Zero]);  Resources = CellLayer.Resize(    oldMapResources,    newSize,    oldMapResources[MPos.Zero]  );  Height = CellLayer.Resize(oldMapHeight, newSize, oldMapHeight[MPos.Zero]);  Ramp = CellLayer.Resize(oldMapRamp, newSize, oldMapHeight[MPos.Zero]);    ....}

Предупреждение анализатора: V3127 Two similar code fragments were found. Perhaps, this is a typo and 'oldMapRamp' variable should be used instead of 'oldMapHeight' Map.cs 964

Анализатор обнаружил подозрительный момент, связанный с передачей аргументов в функции. Взглянем на вызовы отдельно:

CellLayer.Resize(oldMapTiles,     newSize, oldMapTiles[MPos.Zero]);CellLayer.Resize(oldMapResources, newSize, oldMapResources[MPos.Zero]);CellLayer.Resize(oldMapHeight,    newSize, oldMapHeight[MPos.Zero]);CellLayer.Resize(oldMapRamp,      newSize, oldMapHeight[MPos.Zero]);

Достаточно странно, что в последнем вызове производится передача oldMapHeight, а не oldMapRamp. Конечно, далеко не все подобные случаи действительно являются ошибками. Вполне возможно, что тут всё написано правильно. Но согласитесь, что выглядит это место необычно. Я склоняюсь к тому, что здесь действительно допущена ошибка.

Примечание коллеги Андрея Карпова. А я не вижу в данном коде ничего странного :). Это же классическая ошибка последней строки!

Если же ошибки тут всё-таки нет, то стоит добавить какое-нибудь пояснение. Ведь если момент похож на ошибку, то его обязательно кому-нибудь захочется исправить.

True, true and nothing but true

В проекте нашлись весьма своеобразные методы, возвращаемое значение которых имеет тип bool. Их своеобразность заключается в том, что при любых условиях они возвращают true. Например:

static bool State(  S server,   Connection conn,   Session.Client client,   string s){  var state = Session.ClientState.Invalid;  if (!Enum<Session.ClientState>.TryParse(s, false, out state))  {    server.SendOrderTo(conn, "Message", "Malformed state command");    return true;  }  client.State = state;  Log.Write(    "server",     "Player @{0} is {1}",    conn.Socket.RemoteEndPoint,     client.State  );  server.SyncLobbyClients();  CheckAutoStart(server);  return true;}

Предупреждение анализатора: V3009 It's odd that this method always returns one and the same value of 'true'. LobbyCommands.cs 123

Всё ли в порядке в этом коде? Закралась ли тут ошибка? Выглядит крайне странно. Почему разработчик не использовал void?

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

Я решил посмотреть, где вызывается этот метод и используется ли его возвращаемое always true значение. Оказалось, что на него присутствует лишь одна-единственная ссылка в том же классе в словаре commandHandlers, который имеет тип

IDictionary<string, Func<S, Connection, Session.Client, string, bool>>

При инициализации в него добавляются значения

{"state", State},{"startgame", StartGame},{"slot", Slot},{"allow_spectators", AllowSpectators}

и т.д.

Перед нами представлен редкий (мне хочется в это верить) случай, когда статическая типизация создаёт нам проблемы. Ведь сделать словарь, в котором значениями будут функции с различными сигнатурами как минимум проблематично. commandHandlers используется лишь в методе InterpretCommand:

public bool InterpretCommand(  S server, Connection conn, Session.Client client, string cmd){  if (    server == null ||     conn == null ||     client == null ||     !ValidateCommand(server, conn, client, cmd)  )  return false;  var cmdName = cmd.Split(' ').First();  var cmdValue = cmd.Split(' ').Skip(1).JoinWith(" ");  Func<S, Connection, Session.Client, string, bool> a;  if (!commandHandlers.TryGetValue(cmdName, out a))    return false;  return a(server, conn, client, cmdValue);}

Судя по всему, целью разработчика была универсальная возможность сопоставления строкам тех или иных выполняемых операций. Я думаю, что выбранный им способ далеко не единственный, однако предложить что-то более удобное/правильное в такой ситуации не так уж просто. Особенно, если не использовать какой-нибудь dynamic или что-то подобное. Если у вас есть идеи на этот счёт, оставляйте комментарии. Мне было бы интересно посмотреть на различные варианты решения данной задачи :).

Получается, что предупреждения, связанные с always true методами в этом классе, скорее всего ложные. И всё же Пугает вот это вот "скорее всего" :) Нужно быть осторожным с такими штуками, ведь среди них и правда может оказаться ошибка.

Все подобные срабатывания стоит тщательно проверить, а затем уже пометить при необходимости как ложные. Делается это достаточно просто. Нужно оставить в том месте, на которое указывает анализатор, специальный комментарий:

static bool State(....) //-V3009

Есть и другой способ: можно выделить срабатывания, которые необходимо пометить как ложные, и в контекстном меню кликнуть на "Mark selected messages as False Alarms".


Подробнее эту тему можно изучить в документации.

Лишняя проверка на null?

static bool SyncLobby(....){  if (!client.IsAdmin)  {    server.SendOrderTo(conn, "Message", "Only the host can set lobby info");    return true;  }  var lobbyInfo = Session.Deserialize(s);   if (lobbyInfo == null)                    // <=  {    server.SendOrderTo(conn, "Message", "Invalid Lobby Info Sent");    return true;  }  server.LobbyInfo = lobbyInfo;  server.SyncLobbyInfo();  return true;}

Предупреждение анализатора: V3022 Expression 'lobbyInfo == null' is always false. LobbyCommands.cs 851

Ещё один метод, всегда возвращающий true. Однако на этот раз мы разбираем срабатывание другого типа. Изучать такие штуки нужно достаточно внимательно, потому что далеко не факт, что это просто избыточный код. Но обо всём по порядку.

Метод Deserialize никогда не возвращает null в этом можно легко убедиться, взглянув на его код:

public static Session Deserialize(string data){  try  {    var session = new Session();    ....    return session;  }  catch (YamlException)  {    throw new YamlException(....);  }  catch (InvalidOperationException)  {    throw new YamlException(....);  }}

Для удобства чтения я сократил исходный код метода. Полностью его можно увидеть, перейдя по ссылке. Ну или поверьте мне на слово, что переменная session здесь ни при каких обстоятельствах не превращается в null.

Что же мы видим в нижней части? Deserialize не возвращает null, если что-то пошло не так, он бросает исключения. Разработчик, написавший после вызова проверку на null, думал иначе, судя по всему. Скорее всего в исключительной ситуации метод SyncLobby должен выполнять код, который сейчас выполняется да никогда он не выполняется, ведь lobbyInfo никогда не null:

if (lobbyInfo == null){  server.SendOrderTo(conn, "Message", "Invalid Lobby Info Sent");  return true;}

Полагаю, что вместо этой "лишней" проверки всё-таки нужно использовать try-catch. Ну или зайти с другой стороны и написать какой-нибудь TryDeserialize, который в случае исключительной ситуации будет возвращать null.

Possible NullReferenceException

public ConnectionSwitchModLogic(....){  ....  var logo = panel.GetOrNull<RGBASpriteWidget>("MOD_ICON");  if (logo != null)  {    logo.GetSprite = () =>    {      ....    };  }  if (logo != null && mod.Icon == null)                    // <=  {    // Hide the logo and center just the text    if (title != null)    title.Bounds.X = logo.Bounds.Left;    if (version != null)      version.Bounds.X = logo.Bounds.X;    width -= logo.Bounds.Width;  }  else  {    // Add an equal logo margin on the right of the text    width += logo.Bounds.Width;                           // <=  }  ....}

Предупреждение анализатора: V3125 The 'logo' object was used after it was verified against null. Check lines: 236, 222. ConnectionLogic.cs 236

Что-то мне подсказывает, что здесь стопроцентно допущена ошибка. Перед нами уже точно не "лишние" проверки, ведь метод GetOrNull, скорее всего, действительно может вернуть нулевую ссылку. Что же будет, если logo будет null? Обращение к свойству Bounds приведёт к выбрасыванию исключения, что явно не входило в планы разработчика.

Возможно, фрагмент нужно переписать как-то так:

if (logo != null){  if (mod.Icon == null)  {    // Hide the logo and center just the text    if (title != null)    title.Bounds.X = logo.Bounds.Left;    if (version != null)      version.Bounds.X = logo.Bounds.X;    width -= logo.Bounds.Width;  }  else  {    // Add an equal logo margin on the right of the text    width += logo.Bounds.Width;  }}

Данный вариант достаточно прост для восприятия, хотя дополнительная вложенность выглядит не слишком здорово. В качестве более ёмкого решения можно использовать null-conditional operator:

// Add an equal logo margin on the right of the textwidth += logo?.Bounds.Width ?? 0; // <=

Отмечу, что верхний вариант исправления мне нравится больше. Читать его приятно и никаких вопросов при этом не возникает. Но некоторые разработчики достаточно высоко ценят краткость, поэтому второй вариант я тоже решил привести :).

Быть может, всё-таки OrDefault?

public MapEditorLogic(....){  var editorViewport = widget.Get<EditorViewportControllerWidget>("MAP_EDITOR");  var gridButton = widget.GetOrNull<ButtonWidget>("GRID_BUTTON");  var terrainGeometryTrait = world.WorldActor.Trait<TerrainGeometryOverlay>();  if (gridButton != null && terrainGeometryTrait != null) // <=  {    ....  }  var copypasteButton = widget.GetOrNull<ButtonWidget>("COPYPASTE_BUTTON");  if (copypasteButton != null)  {    ....  }  var copyFilterDropdown = widget.Get<DropDownButtonWidget>(....);  copyFilterDropdown.OnMouseDown = _ =>  {    copyFilterDropdown.RemovePanel();    copyFilterDropdown.AttachPanel(CreateCategoriesPanel());  };  var coordinateLabel = widget.GetOrNull<LabelWidget>("COORDINATE_LABEL");  if (coordinateLabel != null)  {    ....  }  ....}

Предупреждение анализатора: V3063 A part of conditional expression is always true if it is evaluated: terrainGeometryTrait != null. MapEditorLogic.cs 35

Давайте проанализируем данный фрагмент. Можно обратить внимание, что каждый раз, когда используется метод GetOrNull класса Widget, производится проверка на равенство null. В то же время, если используется Get, то проверки нет. Это логично метод Get не возвращает null:

public T Get<T>(string id) where T : Widget{  var t = GetOrNull<T>(id);  if (t == null)    throw new InvalidOperationException(....);  return t;}

Если элемент не найден, то выбрасывается исключение это разумное поведение. И в то же время логичным вариантом будет проверять значения, возвращаемые методом GetOrNull, на равенство нулевой ссылке.

В коде, приведённом выше, на равенство null проверяется значение, возвращённое методом Trait. На самом деле внутри метода Trait и вызывается Get класса TraitDictionary:

public T Trait<T>(){  return World.TraitDict.Get<T>(this);}

Может ли быть такое, что этот Get ведёт себя не так, как рассмотренный ранее? Всё же классы разные. Давайте же проверим:

public T Get<T>(Actor actor){  CheckDestroyed(actor);  return InnerGet<T>().Get(actor);}

Метод InnerGet возвращает экземпляр TraitContainer<T>. Реализация Get в этом классе очень напоминает Get класса Widget:

public T Get(Actor actor){  var result = GetOrDefault(actor);  if (result == null)    throw new InvalidOperationException(....);  return result;}

Главное сходство состоит именно в том, что и здесь никогда не возвращается null. В случае, если что-то пошло не так, аналогично выбрасывается InvalidOperationException. Следовательно, метод Trait ведёт себя так же.

Да, здесь может быть просто лишняя проверка, которая ни на что не влияет. Разве что выглядит странно, но нельзя сказать, что такой код сильно запутает читающего. А вот если проверка тут как раз нужна, то в некоторых случаях будет неожиданно выбрасываться исключение. Это печально.

В этом месте кажется более подходящим вызов какого-нибудь TraitOrNull. Однако такого метода нет :). Зато есть TraitOrDefault, который и является аналогом GetOrNull для данного случая.

Есть ещё один подобный момент, связанный уже методом Get:

public AssetBrowserLogic(....){  ....  frameSlider = panel.Get<SliderWidget>("FRAME_SLIDER");  if (frameSlider != null)  {    ....  }  ....}

Предупреждение анализатора: V3022 Expression 'frameSlider != null' is always true. AssetBrowserLogic.cs 128

Как и в коде, рассмотренном ранее, здесь что-то не в порядке. Либо проверка действительно лишняя, либо вместо Get всё-таки нужно вызывать GetOrNull.

Потерянное присваивание

public SpawnSelectorTooltipLogic(....){  ....  var textWidth = ownerFont.Measure(labelText).X;  if (textWidth != cachedWidth)  {    label.Bounds.Width = textWidth;    widget.Bounds.Width = 2 * label.Bounds.X + textWidth; // <=  }  widget.Bounds.Width = Math.Max(                         // <=    teamWidth + 2 * labelMargin,     label.Bounds.Right + labelMargin  );  team.Bounds.Width = widget.Bounds.Width;  ....}

Предупреждение анализатора: V3008 The 'widget.Bounds.Width' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 78, 75. SpawnSelectorTooltipLogic.cs 78

Похоже, что в случае истинности условия textWidth != cachedWidth в widget.Bounds.Width должно записываться некоторое специфичное для данного случая значение. Однако присваивание, производимое ниже вне зависимости от истинности данного условия, лишает строку

widget.Bounds.Width = 2 * label.Bounds.X + textWidth;

всякого смысла. Вполне вероятно, что здесь просто забыли поставить else:

if (textWidth != cachedWidth){  label.Bounds.Width = textWidth;  widget.Bounds.Width = 2 * label.Bounds.X + textWidth;}else{  widget.Bounds.Width = Math.Max(    teamWidth + 2 * labelMargin,    label.Bounds.Right + labelMargin  );}

Проверка default-значения

public void DisguiseAs(Actor target){  ....  var tooltip = target.TraitsImplementing<ITooltip>().FirstOrDefault();  AsPlayer = tooltip.Owner;  AsActor = target.Info;  AsTooltipInfo = tooltip.TooltipInfo;  ....}

Предупреждение анализатора: V3146 Possible null dereference of 'tooltip'. The 'FirstOrDefault' can return default null value. Disguise.cs 192

В каких случаях обычно используется FirstOrDefault вместо First? Если выборка пуста, то First выбросит InvalidOperationException. FirstOrDefault же не выбросит исключение, а вернёт null для ссылочного типа.

В проекте интерфейс ITooltip реализуют различные классы. Таким образом, если target.TraitsImplementing<ITooltip>() вернёт пустую выборку, в tooltip будет записан null. Обращение к свойствам этого объекта, которое производится далее, приведёт к NullReferenceException.

В случаях, когда разработчик уверен, что выборка не будет пустой, правильнее будет использовать First. Если же такой уверенности нет, то стоит проверять значение, возвращаемое FirstOrDefault. Довольно странно, что здесь этого нет. Ведь значения, возвращаемые рассмотренным ранее методом GetOrNull, всегда проверялись. Отчего же тут этого не сделали?

Да кто его знает А, точно! Наверняка на эти вопросы может ответить разработчик. В конце концов ему этот код и править :)

Заключение

OpenRA так или иначе оказался проектом, который было приятно и интересно проверять. Разработчики проделали большую работу и при этом не забывали и о том, что исходник должен быть удобен для изучения. Конечно, и тут найдутся разные спорные моменты, но куда ж без них :)

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

Очевидно, что куда лучше обнаружить ошибку, чем её последствия. Можно для этого тратить часы на перепроверку огромного количества новых исходников вручную. Ну и старые заодно глянуть вдруг ранее не заметили какую-нибудь оплошность? Да, ревью действительно полезны, но если просматривать приходится большое количество кода, то со временем перестаёшь замечать некоторые вещи. Да и тратится на это много времени и сил.


Статический анализ как раз и является удобным дополнением к другим способам проверки качества исходного кода, таких как code-review. PVS-Studio найдёт "простые" (а иногда и не только) ошибки вместо разработчика, позволяя людям сосредоточиться на более серьёзных вопросах.

Да, анализатор иногда выдаёт ложные срабатывания и не способен найти вообще все ошибки. Но его использование сэкономит кучу времени и нервов. Да, он не идеален и иногда ошибается и сам. Однако, в общем и целом PVS-Studio делает процесс разработки намного проще, приятнее и даже (неожиданно!) дешевле :).

На самом деле не нужно верить мне на слово куда лучше будет убедиться в правдивости вышесказанного самостоятельно. По ссылке можно загрузить анализатор и получить триальный ключ. Куда уж проще? :)

Ну а на этом всё. Спасибо за внимание! Желаю вам чистого кода и такого же чистого лога ошибок!


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Nikita Lipilin. Unicorns break into RTS: analyzing the OpenRA source code.