Боремся с хедкрабами в исходниках Source SDK

GameDev тернист и неисповедим. Как и любой проект, он проходит испытания кровью и потом, сражаясь с тварями, тьмой порождёнными: барнаклы, пиявки, муравьиные львы. И это ещё не сказочка, это только смазочка. Больше всего стоит остерегаться жуков хедкрабов. Да, это те самые баги в коде. Если их вовремя не убить монтировкой, то ваша участь — стать отвратительным кадавром. Предлагаем читателю побывать в роли Гордона Фримена, погрузиться в недры Source SDK и побороться с хедкрабами. В качестве монтировки же выступит PVS-Studio.

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

Source SDK — это необходимые зависимости для модов, позволяющие создавать собственный контент для игр на движке Source Engine, разработанный корпорацией Valve. При помощи этого инструментария можно создавать кастомные карты, моды, изменять выражение лица NPC или моделей персонажей.

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

Думаю, важно сказать, что повторная проверка приурочена к обновлению движка, в котором было добавлено немало изменений: прирост производительности игры благодаря возможности использования более чем 4 Гб оперативной памяти, корректное масштабирование элементов VGUI на более высоких разрешениях, таких как 4K. Но с последним крупным обновлением в Source SDK был добавлен код Team Fortress 2. Это позволит моддерам изменять TF2 под свои нужды, начиная от небольших предметов и заканчивая созданием новых игр на основе TF2.

Проект собирался из исходного кода по следующей инструкции. Хочется отметить, что разработчики постарались сделать сборку максимально простой и понятной. Коммит, на котором был собран проект для проверки — 68c8b82.

В статью вошли предупреждения только уровня High (есть ещё Medium и Low).

Выход за границу массива

class DVariant { .... union { .... float m_Vector[3]; .... }; .... }; // This is passed into RecvProxy functions. class CRecvProxyData { public: .... DVariant m_Value; // The value given to you to store. .... }; void RecvProxy_QuaternionToQuaternion( const CRecvProxyData *pData, void *pStruct, void *pOut ) { const float *v = pData->m_Value.m_Vector; Assert( IsFinite( v[0] ) && IsFinite( v[1] ) && IsFinite( v[2] ) && IsFinite( v[3] ) ); ((float*)pOut)[0] = v[0]; ((float*)pOut)[1] = v[1]; ((float*)pOut)[2] = v[2]; ((float*)pOut)[3] = v[3]; }

Предупреждение PVS-Studio: V557 Array overrun is possible. The '3' index is pointing beyond array bound. dt_recv.cpp 369, 373

Поле `m_Value` типа `DVariant` содержит внутри себя массив `m_Vector` с фиксированным размером из 3 элементов. В тот момент, когда в функции `RecvProxy_QuaternionToQuaternion` пытаются получить данные у 4-го элемента массива, поведение будет не определено.

Проблема найдена, можем расходиться. Однако это ещё не всё. На самом деле декларация поля `m_Vector` выглядит несколько хитрее:

union { .... #if 0 // We can't ship this since it changes the size of DTVariant // to be 20 bytes instead of 16 and that breaks MODs!!! float m_Vector[4]; #else float m_Vector[3]; #endif .... };

Судя из комментария, когда-то размер вектора хотели увеличить до 4, при этом изменив весь сопровождающий код. Спустя какое-то время разработчики осознали, что это сломает обратную совместимость с модами, и откатили правки. Но, как оказалось, не во всех местах.

class CCamoMaterialProxy : public CEntityMaterialProxy { public: CCamoMaterialProxy(); virtual ~CCamoMaterialProxy(); .... private: .... void GenerateRandomPointsInNormalizedCube( void ); .... private: .... Vector *m_pointsInNormalizedBox; // [m_CamoPatternNumColors] .... }; void CCamoMaterialProxy::GenerateRandomPointsInNormalizedCube( void ) { m_pointsInNormalizedBox = new Vector[m_CamoPatternNumColors]; .... } CCamoMaterialProxy::~CCamoMaterialProxy() { .... delete m_pointsInNormalizedBox; }

Предупреждение PVS-Studio: V611 The memory was allocated using the 'operator new[]' but was released using the 'operator delete'. The 'delete[] m_pointsInNormalizedBox;' statement should be used instead. Check lines: 164, 561. camomaterialproxy.cpp 164

В конструкторе класса `CCamoMaterialProxy` динамическая память выделяется при помощи выражения `new[]` и затем удаляется в деструкторе при помощи выражения `delete`. По стандарту поведение последней операции будет не определено. Для более подробной информации о том, почему массивы необходимо удалять через `delete[]`, лучше ознакомиться с нашей статьёй.

Исправленный код:

CCamoMaterialProxy::~CCamoMaterialProxy() { .... delete[] m_pointsInNormalizedBox; }

KeyValues *KeyValues::FindKey(const char *keyName, bool bCreate) { // return the current key if a NULL subkey is asked for if (!keyName || !keyName[0]) // <= return this; // look for '/' characters deliminating sub fields char szBuf[256] = { 0 }; const char *subStr = strchr(keyName, '/'); const char *searchStr = keyName; // pull out the substring if it exists if (subStr) { int size = Min( (int)(subStr - keyName + 1), (int)V_ARRAYSIZE( szBuf ) ); V_strncpy( szBuf, keyName, size ); .... } .... } void LoadCmdLineFromFile( int &argc, char **&argv, const char *keyname, const char *appname ) { .... if ( kv->LoadFromFile( g_pFileSystem, filename ) ) { // Load the commandline arguments for this app KeyValues *appKey = kv->FindKey( keyname ); .... } .... } int RunVVis( int argc, char **argv ) { char portalfile[1024]; char source[1024]; char mapFile[1024]; double start, end; .... LoadCmdLineFromFile( argc, argv, source, "vvis" ); .... }

Предупреждение PVS-Studio: V614 [CERT-EXP53-CPP] Uninitialized buffer 'source' used. Consider checking the third actual argument of the 'LoadCmdLineFromFile' function. vvis.cpp 1095

Анализатор обнаружил использование неинициализированной переменной `source`, что может привести к непредсказуемым результатам. Пройдёмся по коду и посмотрим, что происходит. Объявляется массив `source` типа `char[1024]` без инициализации, из-за этого в ячейках массива будут хранится произвольные данные. Далее `source` передаётся в функцию `LoadCmdLineFromFile`, а та в свою очередь передаёт его в функцию `FindKey`. И уже здесь неинициализированный буфер будет прочтён, и поведение такой операции не определено.

Same, same but different

class CBaseCombatWeapon : public BASECOMBATWEAPON_DERIVED_FROM { public: .... virtual bool CanHolster( void ) const { return TRUE; }; .... }; class CWeaponAR2 : public CHL2MPMachineGun { public: .... bool CanHolster( void ); .... }; bool CWeaponAR2::CanHolster( void ) { if ( m_bShotDelayed ) return false; return BaseClass::CanHolster(); }

Предупреждение анализатора PVS-Studio: V762 It is possible a virtual function was overridden incorrectly. See qualifiers of function 'CanHolster' in derived class 'C_WeaponAR2' and base class 'C_BaseCombatWeapon'. weapon_ar2.h 48

Из текста предупреждения заметно, что проблема в ошибочном переопределении виртуальной функции. Но что могло пойти не так?

Прежде чем разобраться в проблеме, немного вникнем в код. Есть базовый класс `CBaseCombatWeapon`, у которого есть виртуальная функция `CanHolster`. Также у нас есть дочерний класс `CWeaponAR2` с функцией-членом `CanHolster`, который по задумке должен был переопределить виртуальную функцию из базового класса.

Как видим, эти две функции с именем `CanHolster` имеют почти что одинаковые сигнатуры. Единственное различие — квалификатор `const` после имени функции.

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

имена функций в базовом и производном классах должны быть одинаковы;
типы параметров функций должны быть одинаковы;
квалификаторы нестатических функций в базовом и производном классах должны совпадать;
возвращаемые типы и спецификации исключений функций в базовом и производном классах должны быть коварианты;
[дополнительно] ссылочные квалификаторы функций должны быть идентичными.

Так как одно из условий не выполняется, то в производный класс будет добавлена новая функция, которая перекроет имя из базового класса. Чтобы исправить возникшую ситуацию, необходимо добавить квалификатор `const` после имени функции-члена из дочернего класса. И не забудем добавить спецификатор `override`, проверяющий, что есть функция из базового, которую переопределяем:

class CWeaponAR2 : public CHL2MPMachineGun { public: .... bool CanHolster( void ) const override; .... };

Неподалёку разместились ещё два класса, в которых содержится та же ошибка:

V762 It is possible a virtual function was overridden incorrectly. See qualifiers of function 'CanHolster' in derived class 'C_WeaponPhysCannon' and base class 'C_BaseCombatWeapon'. weapon_physcannon.h 289

void CClientShadowMgr::RemoveAllShadowsFromReceiver( IClientRenderable* pRenderable, ShadowReceiver_t type ) { // Don't bother if this renderable doesn't receive shadows if ( !pRenderable->ShouldReceiveProjectedTextures( // <= SHADOW_FLAGS_PROJECTED_TEXTURE_TYPE_MASK ) ) return; // Do different things depending on the receiver type switch( type ) { case SHADOW_RECEIVER_BRUSH_MODEL: { model_t* pModel = const_cast<model_t*>(pRenderable->GetModel()); shadowmgr->RemoveAllShadowsFromBrushModel( pModel ); } break; case SHADOW_RECEIVER_STATIC_PROP: staticpropmgr->RemoveAllShadowsFromStaticProp(pRenderable); break; case SHADOW_RECEIVER_STUDIO_MODEL: if( pRenderable && pRenderable->GetModelInstance() != // <= MODEL_INSTANCE_INVALID ) { shadowmgr->RemoveAllShadowsFromModel(pRenderable->GetModelInstance()); } break; ... }

Предупреждение PVS-Studio: V595 The 'pRenderable' pointer was utilized before it was verified against nullptr. Check lines: 3535, 3553. clientshadowmgr.cpp 3535

В начале функции `RemoveAllShadowsFromReceiver` разыменовывается указатель `pRenderable`. Но далее в кейсе `SHADOW_RECEIVER_STUDIO_MODEL` конструкции `switch` есть условие, которое проверяет этот указатель на валидность. Разработчик предполагает, что он может быть нулевым. Исходя из того, что этот указатель — параметр функции, логичным кажется всё же проверить его и сделать ранний возврат из функции:

// Don't bother if this renderable doesn't receive shadows if ( !pRenderable || !pRenderable->ShouldReceiveProjectedTextures( SHADOW_FLAGS_PROJECTED_TEXTURE_TYPE_MASK ) ) return;

Если же эта функция имеет неявный контракт "первый параметр всегда должен быть ненулевым", то проверку в `switch` стоит убрать.

Таких предупреждений довольно много, и на них следует обратить внимание:

V595 The 'pPlayer' pointer was utilized before it was verified against nullptr. Check lines: 426, 442. weapon_frag.cpp 426
V595 The 'pPlayer' pointer was utilized before it was verified against nullptr. Check lines: 146, 152. weapon_hl2mpbasebasebludgeon.cpp 146
V595 The 'pOwner' pointer was utilized before it was verified against nullptr. Check lines: 1364, 1380. weapon_physcannon.cpp 1364
V595 The 'node' pointer was utilized before it was verified against nullptr. Check lines: 2338, 2341. nav_generate.cpp 2338
V595 The 'adjNode' pointer was utilized before it was verified against nullptr. Check lines: 2339, 2341. nav_generate.cpp 2339
V595 The 'm_goal' pointer was utilized before it was verified against nullptr. Check lines: 730, 744. NextBotPathFollow.cpp 730
V595 The 'body' pointer was utilized before it was verified against nullptr. Check lines: 1748, 1762. NextBotPathFollow.cpp 1748
V595 The 'm_pszString' pointer was utilized before it was verified against nullptr. Check lines: 819, 827. convar.cpp 819
V595 The 'pPortal->nodes[nOtherSideIndex]' pointer was utilized before it was verified against nullptr. Check lines: 712, 715. vbsp.cpp 712
V595 The 'front' pointer was utilized before it was verified against nullptr. Check lines: 397, 475, 477. polylib.cpp 475
V595 The 'm_pCommand' pointer was utilized before it was verified against nullptr. Check lines: 251, 252. consoledialog.cpp 251
V595 The '_title' pointer was utilized before it was verified against nullptr. Check lines: 1594, 1603. Frame.cpp 1594
V595 The 'pActionSignalTarget' pointer was utilized before it was verified against nullptr. Check lines: 636, 642. perforcefilelistframe.cpp 636
V595 The '_activeTab' pointer was utilized before it was verified against nullptr. Check lines: 1063, 1087. PropertySheet.cpp 1063
V595 The 'm_pTree' pointer was utilized before it was verified against nullptr. Check lines: 132, 144. TreeViewListControl.cpp 132
V595 The 'pShaderShadow' pointer was utilized before it was verified against nullptr. Check lines: 103, 334, 335. example_model_dx9_helper.cpp 334

typedef unsigned char byte; inline void CBaseEntity::SetRenderColor( byte r, byte g, byte b, byte a ) { m_clrRender.Init( r, g, b, a ); } void CGrenadeBugBait::BugBaitTouch( CBaseEntity *pOther ) { .... if ( pSporeExplosion ) { .... pSporeExplosion->SetRenderColor( 0.0f, 0.5f, 0.25f, 0.15f ); // <= .... } .... }

Предупреждение PVS-Studio: V674 The literal '0.5f' of 'float' type is being implicitly cast to 'unsigned char' type while calling the 'SetRenderColor' function. Inspect the second argument. grenade_bugbait.cpp 168

Функция `SetRenderColor` устанавливает значение цвета по RGBA, где каждый параметр имеет тип `unsigned char` с возможным диапазоном значений `[0 .. 255]`. При попытке передать аргументы с типом `float` дробная часть будет отброшена. Это приведёт к тому, что параметры функции `r`, `g`, `b`, `a` будут иметь значения, равные `0`.

К сожалению, в репозитории отсутствует информация по blame, поэтому у меня есть два предположения, как ошибка появилась в кодовой базе:

1. Когда-то функция обрабатывала цвета в вещественном представлении. Затем обработку сменили на целые числа, но забыли поправить все точки вызова.

2. Разработчик ошибочно посчитал, что функция `SetRenderColor` обрабатывает вещественные числа, и задал их таким способом.

Дополнительно несколько аналогичных предупреждений:

V674 The literal '0.5f' of 'float' type is being implicitly cast to 'unsigned char' type while calling the 'SetRenderColor' function. Inspect the second argument. weapon_bugbait.cpp 171
V674 The literal '25.6' of 'double' type is being implicitly cast to 'int' type while calling the 'SetScrollRate' function. Inspect the first argument. grenade_tripmine.cpp 179

template <class T> void AddSortedByName(T * & head, T * node) { if ( (! head) || (stricmp(node->m_Name, head->m_Name) == -1)) { .... } .... }

Предупреждение PVS-Studio: V698 Expression 'stricmp(....) == -1' is incorrect. This function can return not only the value '-1', but any negative value. Consider using 'stricmp(....) < 0' instead. utlintrusivelist.h 509

Функция `stricmp` может вернуть любое отрицательное значение, если `node->m_Name` лексикографически меньше, чем `head->m_Name` (без учёта регистра). Поэтому сравнивать результат функции `stricmp` со значением `-1` некорректно.

Исправленный код:

if ( (! head) || (stricmp(node->m_Name, head->m_Name) < 0)) { .... }

Истинная и ложная ветки оператора 'if' полностью совпадают

void CWeaponCrossbow::PrimaryAttack( void ) { if ( m_bInZoom && g_pGameRules->IsMultiplayer() ) { //FireSniperBolt(); FireBolt(); } else { FireBolt(); } .... }

Аналогичные срабатывания.

V523 The 'then' statement is equivalent to the 'else' statement. c_basehlplayer.cpp 634
V523 The 'then' statement is equivalent to the 'else' statement. hl2_triggers.cpp 752
V523 The 'then' statement is equivalent to the 'else' statement. npc_combine.cpp 2528
V523 The 'then' statement is equivalent to the 'else' statement. npc_ichthyosaur.cpp 724
V523 The 'then' statement is equivalent to the 'else' statement. Menu.cpp 1185

float CInput::KeyState ( kbutton_t *key ) { .... if ( impulseup && !impulsedown ) { // released this frame? val = down ? 0.0 : 0.0; // <= } .... return val; }

Предупреждение PVS-Studio: V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: 0.0. in_main.cpp 605

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

void CBaseVSShader::SetPixelShaderConstant_W(int pixelReg, int constantVar, float fWValue ) { .... float val[4]; if (pPixelVar->GetType() == MATERIAL_VAR_TYPE_VECTOR) pPixelVar->GetVecValue( val, 4 ); else val[0] = val[1] = val[2] = val[3] = pPixelVar->GetFloatValue(); val[3]=fWValue; // <= .... }

Предупреждение анализатора PVS-Studio: V519 The 'val[3]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 124, 125. BaseVSShader.cpp 125

Анализатор обнаружил, что в переменную `val[3]` несколько раз подряд присваиваются разные значения. Причём между этими присваиваниями сама переменная не используется. Возможно, это опечатка, и планировалось написать что-то вроде такого выражения: `val[3] += fWValue`.

void CNPC_Monk::Spawn() { .... m_flFieldOfView = m_flFieldOfView = -0.707; // 270` .... }

Предупреждение анализатора PVS-Studio: V570 The same value is assigned twice to the 'm_flFieldOfView' variable. npc_monk.cpp 274

Полю `m_flFieldOfView` дважды присваивается одно и то же значение `-0.707` (косинус угла обзора в 135 градусов). Либо это опечатка, и должно использоваться также другое поле, либо одно присваивание лишнее, и его можно убрать.

void SendProxy_UtlVectorElement(...., int iElement, ....) { .... iElement = pProp->GetElementStride(); .... if ( iElement >= pUtlVec->Count() ) { .... } else { pExtra->m_ProxyFn( pProp, pData, (char*)pUtlVec->Base() + iElement*pExtra->m_ElementStride, pOut, 0, objectID ); } }

Предупреждение анализатора PVS-Studio: V763 Parameter 'iElement' is always rewritten in function body before being used. dt_utlvector_send.cpp 49

Параметр `iElement`, который функция принимает по копии, всегда перезаписывается и лишь затем используется. Это странное поведение. Возможно, надобность в параметре пропала, но на рефакторинг кода не было времени. Тогда логично сделать параметр неименованным, и у других разработчиков будет меньше вопросов, почему он всегда перезаписывается:

void SendProxy_UtlVectorElement(...., int /* iElement */, ....) { .... auto iElement = pProp->GetElementStride(); .... }

Предупреждения.

V763 Parameter 'flDist' is always rewritten in function body before being used. npc_cranedriver.cpp 180
V763 Parameter 'flDist' is always rewritten in function body before being used. npc_strider.cpp 2561

Хочется поблагодарить вас, читателей, что отложили свои дела и провели это время со старейшим ронином Source SDK, сражаясь с чужеродными формами жизни. Надеюсь, статья была интересной и познавательной.

Если вы захотите попробовать анализатор PVS-Studio, для open source проектов у нас предоставляется бесплатная лицензия.

Что ж, нам пора прощаться. Всех благ вам, и до новых встреч!

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

Продолжение следует.

Боремся с хедкрабами в исходниках Source SDK

О проекте

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

Ошибки при работе с массивами

Неверное освобождение памяти массива

Использование неинициализированного массива

Невероятно, но факт

Проверка на nullptr производится после использования указателя

Подозрительное преобразование

Некорректное сравнение

Аномалии

Независимо от условия, будет выполнено одно и то же действие

Присваивания разных значений одной переменной

Переменная присваивается сама себе

Параметр стал ненужным

Заключение