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

• V762 It is possible a virtual function was overridden incorrectly. See qualifiers of function 'CanHolster' in derived class 'C_WeaponRPG' and base class 'C_BaseCombatWeapon'. weapon_rpg.h 193

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

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();
  }
  ....
}

Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. weapon_crossbow.cpp 542

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

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

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();
  ....
}

Заключение

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

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

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

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

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

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Denis Tukupov. Combating headcrabs in the Source SDK codebase.