Our website uses cookies to enhance your browsing experience.
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.
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).
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
Them_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.
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;
}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.
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:
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:
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:
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.
SetRenderColor handled floating-point numbers and set them accordingly.Here are similar warnings:
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))
{
....
}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:
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.
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.
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.
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();
....
}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.
0
0
0
107
0
33
0
37
0
0