The path of GameDev is treacherous and unpredictable. Like any project, it is tested through blood and sweat, battling creatures born from the darkness: barnacles, leeches, and antlions. And this is just the beginning. Above all, one must beware of headcrab bugs. Yes, those are the actual bugs in code.

If you don’t kill them with your trusty crowbar, your fate is to become a repulsive cadaver. We invite the reader to step into the role of Gordon Freeman, delve into the depths of the Source SDK, and fight off the headcrabs. Your crowbar for this mission is PVS-Studio.

About the project

Before crossing the inter-dimensional portal, let’s first brief our dear Gordon on the world he is about to enter.

The Source SDK is a set of essential dependencies for mods, enabling the creation of custom content for games based on the Valve’s Source engine. This toolkit allows developers to build custom maps, mods, and alter the facial expressions of NPCs or character models.

We are truly pleased that the developers fixed the errors from our previous analysis. If you are interested, you can read about it in this article. However, the Source SDK project is not standing still, it is growing and evolving, just like PVS-Studio, which we used to analyze the source code.

Note that this re-check coincides with a major engine update that introduced many changes: improved game performance due to support for more than 4 GB of RAM, proper scaling of VGUI elements at higher resolutions like 4K. Furthermore, the latest major update integrated Team Fortress 2 code into the Source SDK. This enables modders to adapt TF2 to their needs, ranging from small items to creating entirely new games based on TF2.

The project was built from the source code using the following instructions. We must note that the developers made the build process as simple and straightforward as possible. The commit used for the project build and analysis was 68c8b82.

This article includes only warnings of High level (there are also Medium and Low levels).

The check results

Errors with handling arrays

Array index out of bounds

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

The PVS-Studio warning: V557 Array overrun is possible. The ‘3’ index is pointing beyond array bound. dt_recv.cpp 369, 373

The`m_Value` data member of the `DVariant` type contains the `m_Vector` array with the fixed size of 3 elements. Attempting to access data from the 4th element of this array in the `RecvProxy_QuaternionToQuaternion` function, leads to undefined behavior.

The problem is found, we can call it a day. But there is something more. Actually, the declaration of the `m_Vector` data member is a bit more intricate:

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

Judging by the comment, the vector size was once intended to be increased to4, with all dependent code updated. Some time later, the developers realized this would break backward compatibility with mods and rolled back the changes. However, as it turns out, not all instances have been reverted.

An incorrect array memory deallocation

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

The PVS-Studio warning: 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

In the `CCamoMaterialProxy` class constructor, dynamic memory is allocated using `new[]` and then released in the destructor using `delete`. According to the standard, the behavior of the latter operation is undefined. For more details on why arrays must be deleted using `delete[]`, check out our article.

The fixed code:

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

Using an uninitialized array

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

The PVS-Studio warning: V614 [CERT-EXP53-CPP] Uninitialized buffer ‘source’ used. Consider checking the third actual argument of the ‘LoadCmdLineFromFile’ function. vvis.cpp 1095

The analyzer detected using an uninitialized `source` variable, which may lead to unpredictable results. Let’s look through the code and see what’s going on there. The `source` array of the `char[1024]` type is declared without initialization, meaning its cells will contain arbitrary data. Then, `source` is passed to the `LoadCmdLineFromFile` function, which in turn passes it to the `FindKey` function. At this point, the uninitialized buffer is read, and the operation behavior is undefined.

Incredible but true

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

The PVS-Studio analyzer warning: 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

The warning text clearly indicates the issue with an incorrect override of a virtual function. But what could go wrong?

Before delving into the problem, let’s examine the code. There is a `CBaseCombatWeapon` base class that has a `CanHolster` virtual function. We also have a `CWeaponAR2` derived class with a `CanHolster` member function, which was intended to override the virtual function from the base class.

As we can see, these two functions named `CanHolster` have nearly identical signatures. The only difference is the `const` qualifier after the function name.

Recall that when overriding a virtual function, the signature of the derived class function must exactly match the signature of the base class function, namely:

• function names in the base and derived classes must be identical;
• parameter types must be identical;
• qualifiers of non-static functions in the base and derived classes must match;
• return types and exception specifications of the functions must be covariant;
• [additionally] reference qualifiers of the functions must be identical. Since one of the conditions is not met, the derived class will have a new function that shadows the name from the base class. To fix this, authors should add the `const` qualifier after the name of the member function in the derived class. And make sure to add the `override` specifier to verify that there is a base class function we are overriding:

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

Two more classes nearby contain the same error:

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

A null pointer check after the pointer using

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

The PVS-Studio warning: V595 The ‘pRenderable’ pointer was utilized before it was verified against nullptr. Check lines: 3535, 3553. clientshadowmgr.cpp 3535

The `RemoveAllShadowsFromReceiver` function begins with the `pRenderable` pointer dereference. However, later in the `SHADOW_RECEIVER_STUDIO_MODEL` case of the `switch` statement, there is a condition that checks whether this pointer is valid. The developer assumes it might be null. Given that this pointer is a function parameter, it would be logical to check it and perform an early return from the function:

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

If this function has an implicit contract that “the first parameter must always be non-null,” then the check in `switch` should be removed.

There are quite a few such warnings, and they deserve attention:

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

A suspicious conversion

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

The PVS-Studio warning: 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

The `SetRenderColor` function sets the RGBA color values, where each parameter is of the `unsigned char` type with a possible value range of `[0 .. 255]`. When attempting to pass arguments of the `float` type, the fractional part will be truncated. Therefore, function parameters `r`, `g`, `b`, and `a` will have values equal to `0`.

Unfortunately, the repository lacks blame information, so I have two scenarios on how this error appeared in the code.

1. The function once processed colors in a floating-point representation. The processing was later changed to integers, but not all call sites were updated.
2. The developer mistakenly believed that `SetRenderColor` handled floating-point numbers and set them accordingly. Here are similar warnings:

• 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

An incorrect comparison

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

The PVS-Studio warning: 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

The `stricmp` function can return any negative value if `node->m_Name` is lexicographically less than `head->m_Name` (case-insensitive). Therefore, comparing the result of `stricmp` to `-1` is incorrect.

The fixed code:

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

Anomalies

The true and false branches of the ‘if’ statement are identical

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

The PVS-Studio warning: V523 The ‘then’ statement is equivalent to the ‘else’ statement. weapon_crossbow.cpp 542

The analyzer detected a situation where the `then` and `else` branches of the `if` statement are identical. Based on the code comment, it seems that targeted fire in multiplayer was once possible, but this functionality was later removed. However, the check for the fire mode remained in the implementation. The report also listed five similar instances:

Similar warnings

• 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

Regardless of the condition, the same action is performed

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

The PVS-Studio warning: V583 The ‘?:’ operator, regardless of its conditional expression, always returns one and the same value: 0.0. in_main.cpp 605

This is most likely a typo, as returning the same value in the condition looks suspicious.

Assigning different values to the same variable

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; // <=
....
}

The PVS-Studio analyzer warning: V519 The ‘val[3]’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 124, 125. BaseVSShader.cpp 125

The analyzer detected that the `val[3]` variable is assigned different values several times in a row. But the variable is not used between these assignments. This might be a typo, and the intended expression was something like `val[3] += fWValue`.

A variable assigned to itself

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

The PVS-Studio analyzer warning: V570 The same value is assigned twice to the ‘m_flFieldOfView’ variable. npc_monk.cpp 274

The `m_flFieldOfView` data member is assigned the same `-0.707` value twice—the cosine of a 135-degree field of view. This is either a typo where another data member should have been used, or one assignment is redundant and can be removed.

A parameter became redundant

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

The PVS-Studio analyzer warning: V763 Parameter ‘iElement’ is always rewritten in function body before being used. dt_utlvector_send.cpp 49

The `iElement` parameter, which the function receives by copy, is always overwritten before using, which is strange. Perhaps, the parameter is no longer needed, but there hasn’t been time to refactor the code. In that case, authors should make the parameter unnamed, reducing questions from other developers about why it is always overwritten:

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

Warnings

• 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

Conclusion

We thank you, dear readers, for setting aside your tasks and spending this time with the seasoned ronin—the Source SDK. We hope you found this article interesting and informative.

If you would like to try PVS-Studio analyzer, we offer a free license for open source projects.

Well, it’s time to say goodbye. We wish you all the best, see you!

Wait, do you hear that? It seems we’ve found the headcrab nest. I suggest we patch up our wounds and reload our magazines to finally defeat these creatures. For now, let’s make camp before our next journey.

To be continued.