Checking the Source SDK Project

The Source SDK is a software development kit compiled by Valve Corporation that is used to create games or mods for the Source engine. I downloaded and checked the project's source codes at the end of 2013 already and intended to write an article about it during the New Year holidays. But laziness prevailed over the craving for creativity, and I sat down to writing the article only on getting back to work. However, I doubt that the source codes have changed during this time. Now you are welcome to have a look at the suspicious code fragments found in the project code by the PVS-Studio code analyzer.

About Source SDK

Here's the project's description from Wikipedia:

The Source SDK is a software development kit compiled by Valve Corporation that is used to create games or mods for the Source engine.

Games like Left 4 Dead, Left 4 Dead 2, Counter Strike: Global Offensive, and Portal 2 are packaged with their own set of 'Authoring Tools' that are tailored for their specific engine builds and come packaged with the game on Steam. These games use most of the same programs below, but the source code can not be dumped.

In 2013, an update was released for Source SDK that allowed users to build OS X and Linux versions of their mods. Additionally, support was added for Valve's new Steampipe content distribution system as well as the Oculus Rift.

The project website: http://source.valvesoftware.com/sourcesdk.php

Since the Source SDK project is rather large, it's no wonder there are always some bugs to be found in it. Analysis was performed with the PVS-Studio analyzer.

Suspicious expressions

Variables divided by themselves

static void DrawPyroVignette(....)
{
  ....
  Vector2D vMaxSize(
   ( float )nScreenWidth / ( float )nScreenWidth /
     NUM_PYRO_SEGMENTS * 2.0f,
   ( float )nScreenHeight / ( float )nScreenHeight /
     NUM_PYRO_SEGMENTS * 2.0f );
  ....
}

PVS-Studio generates the V501 warning on the following file: viewpostprocess.cpp 1888

Note the following expressions:

They are very strange. I cannot say for sure what exactly should be written here, but I suspect there must be something else.

Double call of the IsJoystickPOVCode() function

void TextEntry::OnKeyCodePressed(KeyCode code)
{
  ....
  if ( IsMouseCode(code) || IsNovintButtonCode(code) ||
       IsJoystickCode(code) || IsJoystickButtonCode(code) ||
       IsJoystickPOVCode(code) || IsJoystickPOVCode(code) ||
       IsJoystickAxisCode(code) )
  ....
}

PVS-Studio generates the V501 warning on the following file: textentry.cpp 1639

The 'IsJoystickPOVCode(code)' function is called twice. The second call is excessive or some other function should have been called.

Condition always false

unsigned  numbounce = 100;
int ParseCommandLine( int argc, char **argv, bool *onlydetail )
{
  ....
  numbounce = atoi (argv[i]);
  if ( numbounce < 0 )
  {
    Warning(
      "Error: expected non-negative value after '-bounce'\n");
    return 1;
  }
  ....
}

PVS-Studio generates the V547 warning on the following file: vrad.cpp 2412.

The "numbounce < 0" condition will never be executed. An unsigned variable cannot be less than zero.

Strange string comparison

void CMultiplayRules::DeathNotice( .... )
{
  ....
  else if ( strncmp( killer_weapon_name, "NPC_", 8 ) == 0 )
  ....
}

PVS-Studio generates the V666 warning on the following file: multiplay_gamerules.cpp 860.

As far as I understand, the programmer wanted to check that the weapon name starts with "NPC_". If so, there is a typo in the code. I guess a correct check must look like this:

else if ( strncmp( killer_weapon_name, "NPC_", 4 ) == 0 )

Array handing errors

Incorrect array size calculation

#define RTL_NUMBER_OF_V1(A) (sizeof(A)/sizeof((A)[0]))
#define _ARRAYSIZE(A)   RTL_NUMBER_OF_V1(A)

int GetAllNeighbors( const CCoreDispInfo *pDisp,
                     int iNeighbors[512] )
{
  ....
  if ( nNeighbors < _ARRAYSIZE( iNeighbors ) )
    iNeighbors[nNeighbors++] = pCorner->m_Neighbors[i];
  ....
}

PVS-Studio generates the V511 warning on the following file: disp_vrad.cpp 60

The actual argument "int iNeighbors[512]" is not an array - it's just a pointer. The number '512' reminds the programmer that the pointer is most likely to refer to an array consisting of 512 items - but no more than that. The 'sizeof(iNeighbors)' expression is illegal as it returns the pointer size, not the array size. That is, the 'sizeof(iNeighbors)' expression will equal 'sizeof(void *).

Using a safer macro could have helped avoid this error, for example:

template <typename T, size_t N>
char (&ArraySizeHelper(T (&array)[N]))[N];
#define arraysize(array) (sizeof(ArraySizeHelper(array)))

When trying to calculate the pointer size, a compilation error will occur. Such a macro is used in the Chromium project. To learn more about this magic construct, see the article "PVS-Studio vs Chromium".

Incorrect string length calculation

typedef struct message_s
{
  ....
  char    *text;
  ....
} message_t;

int CMessageCharsPanel::AddText(....)
{
  ....
  msg->text = new char[ Q_strlen( data ) + 1 ];
  Assert( msg->text );
  Q_strncpy( msg->text, data, sizeof( msg->text ) );
  ....
}

PVS-Studio generates the V579 warning on the following file: vgui_messagechars.cpp 240

The "sizeof(msg->text)" expression calculates the pointer size, not the string length. Most likely, the following code should be written instead: Q_strcpy( msg->text, data);

Handling a destroyed array

static Activity DetermineExpressionMoveActivity(
  CChoreoEvent *event, CAI_BaseNPC *pNPC )
{
  ....
  const char *pszAct = Q_strstr( sParam2, " " );
  if ( pszAct )
  {
    char szActName[256];
    Q_strncpy( szActName, sParam2, sizeof(szActName) );
    szActName[ (pszAct-sParam2) ] = '\0';
    pszAct = szActName;
  }
  ....
}

PVS-Studio generates the V507 warning on the following file: baseflex.cpp 1326

The address of the temporary array is stored in the 'pszAct' variable. Since this array will be destroyed, one can't use the address stored in that variable. However, this code may work successfully thus creating an illusion of being flawless. It's highly probable that the memory area occupied by the temporary array 'szActName' is never used after that, which results in the program behaving just the way the programmer wants it to. But it's just pure luck.

Array index out of bounds

#define MAX_WEAPON_SLOTS    6  // hud item selection slots

void CHudWeaponSelection::Paint()
{
  ....
  int xModifiers[] = { 0, 1, 0, -1 };
  int yModifiers[] = { -1, 0, 1, 0 };

  for ( int i = 0; i < MAX_WEAPON_SLOTS; ++i )
  {
    ....
    xPos += ( m_flMediumBoxWide + 5 ) * xModifiers[ i ];
    yPos += ( m_flMediumBoxTall + 5 ) * yModifiers[ i ];
  ....
}

PVS-Studio generates the V557 warning on the following file: hud_weaponselection.cpp 632, 633.

The loop counter takes values from 0 to 6. But the arrays xModifiers and yModifiers contain only 4 items each. It will result in an array overrun.

Dangerous use of the new operator

Pointless checks

void EmitDispLMAlphaAndNeighbors()
{
  ....
  CCoreDispInfo *pDisp = new CCoreDispInfo;
  if ( !pDisp )
  {
    g_CoreDispInfos.Purge();
    return;
  }
  ....
}

PVS-Studio generates the V668 warning on the following file: disp_vbsp.cpp 532.

If an object of the 'CCoreDispInfo' type cannot be created, the function g_CoreDispInfos.Purge() must be called. But it won't happen: if a memory allocation error occurs, the std::bad_alloc exception will be thrown. This code fragment is obsolete and must be revised according to the changes in the 'new' operator's behavior.

See the appendix at the end of the article for other fragments with checks of the values returned by the 'new' operator.

The new operator in a destructor

CNewParticleEffect::~CNewParticleEffect(void)
{
  ....
  KeyValues *msg = new KeyValues( "ParticleSystem_Destroy" );
  ....
}

PVS-Studio generates the V509 warning on the following file: particles_new.cpp 92.

It's not safe to use constructs that may cause an exception inside a destructor. The 'new' operator is just such a construct: it throws an exception in case of a memory allocation error.

Let me explain why it is dangerous. If an exception is generated inside a program, the stack gets collapsed, which results in calling destructors to destroy the objects. If the destructor of an object being destroyed during stack collapse throws one more exception, it will leave the destructor and the C++ library will at once crash calling the terminate() function.

Typos

A typo in a nested loop

void DrawTeslaSegs(....)
{
  int i;
  ....
  for ( i = 0; i < segments; i++ )
  {
    ....
    for ( int j = 0; i < iBranches; j++ )
    {
      curSeg.m_flWidth *= 0.5;
    }
    ....
  }
  ....
}

PVS-Studio generates the V534 warning on the following file: beamdraw.cpp 592.

Note the second loop:

for ( int j = 0; i < iBranches; j++ )

The termination condition for the nested loop contains the 'i' variable related to the external loop. I have a strong suspicion that it's a typo.

Incorrect initialization

inline void SetX( float val );
inline void SetY( float val );
inline void SetZ( float val );
inline void SetW( float val );

inline void Init( float ix=0, float iy=0,
                  float iz=0, float iw = 0 ) 
{
  SetX( ix );
  SetY( iy );
  SetZ( iz );
  SetZ( iw );
}

PVS-Studio generates the V525 warning on the following file: networkvar.h 455.

I guess the function should look like this:

{
  SetX( ix );
  SetY( iy );
  SetZ( iz );
  SetW( iw );
}

Note the last function call.

Consequences of Copy-Paste

class ALIGN16 FourVectors
{
public:
  fltx4 x, y, z;
  ....
};

FourVectors BackgroundColor;

void RayTracingEnvironment::RenderScene(....)
{
  ....
  intens.x=OrSIMD(AndSIMD(BackgroundColor.x,no_hit_mask),
                  AndNotSIMD(no_hit_mask,intens.x));
  intens.y=OrSIMD(AndSIMD(BackgroundColor.y,no_hit_mask),
                  AndNotSIMD(no_hit_mask,intens.y));
  intens.z=OrSIMD(AndSIMD(BackgroundColor.y,no_hit_mask),
                  AndNotSIMD(no_hit_mask,intens.z));

  ....
}

PVS-Studio generates the V537 warning on the following file: trace2.cpp 189.

This code must have been written through the Copy-Paste technique. In the first line, there are members of the 'x' class; in the second, of the 'y' class; and in the third, there are both 'z' and 'y'. I guess the code should look like this:

intens.z=OrSIMD(AndSIMD(BackgroundColor.z,no_hit_mask),
                AndNotSIMD(no_hit_mask,intens.z));

Assigning different values to one and the same variable

void GetFPSColor( int nFps, unsigned char ucColor[3] )
{
  ....
  int nFPSThreshold1 = 20;
  int nFPSThreshold2 = 15;
  
  if (IsPC() &&
      g_pMaterialSystemHardwareConfig->GetDXSupportLevel() >= 95)
  {
    nFPSThreshold1 = 60;
    nFPSThreshold1 = 50;
  }
  ....
}

PVS-Studio generates the V519 warning on the following file: vgui_fpspanel.cpp 192.

I guess the following should have been written here:

nFPSThreshold1 = 60;
nFPSThreshold2 = 50;

Bad constructor

CAI_ShotRegulator::CAI_ShotRegulator() :
  m_nMinBurstShots(1), m_nMaxBurstShots(1)
{
  m_flMinRestInterval = 0.0f;
  m_flMinRestInterval = 0.0f;
  m_flMinBurstInterval = 0.0f;
  m_flMaxBurstInterval = 0.0f;
  m_flNextShotTime = -1;
  m_nBurstShotsRemaining = 1;
  m_bInRestInterval = false;
  m_bDisabled = false;
}

PVS-Studio generates the V519 warning on the following file: ai_utils.cpp 49.

Another typo which has the following consequences:

Similar troubles can be found in the constructors of the classes CEnvTonemapController and CBasePlayerAnimState. But it's too boring to describe similar cases, so see the appendix for other samples.

Undefined behavior

Complex expressions

int m_nNewSequenceParity;
int m_nResetEventsParity;

void C_BaseAnimating::ResetSequenceInfo( void )
{
  ....
  m_nNewSequenceParity = 
    ( ++m_nNewSequenceParity ) & EF_PARITY_MASK;
  m_nResetEventsParity =
    ( ++m_nResetEventsParity ) & EF_PARITY_MASK;
  ....
}

PVS-Studio generates the V567 warning on the following file: c_baseanimating.cpp 5301, 5302.

See a nice detailed explanation in the documentation to find out why undefined behavior occurs here and why it's impossible to predict the 'm_nResetEventsParity' variable's value. You will find a very similar code sample there.

Shifts

inline void SetStyleType( int w, int h, int type )
{
  Assert( type < NUM_EDGE_STYLES );
  Assert( type >= 0 );
  // Clear old value
  m_nPanelBits[ w ][ h ] &= ( ~0x03 << 2 );
  // Insert new value
  m_nPanelBits[ w ][ h ] |= ( type << 2 );
}

PVS-Studio generates the V610 warning on the following file: c_func_breakablesurf.cpp 157.

Shifting negative numbers leads to undefined behavior. In this code fragment, the '~0x03' number is negative. For details on negative number shifts see the article "Wade not in unknown waters. Part three".

A virtual destructor missing

class CFlashlightEffect
{
  ....
  ~CFlashlightEffect();
  ....
};

class CHeadlightEffect : public CFlashlightEffect { .... };

CFlashlightEffect *m_pFlashlight;

C_BasePlayer::~C_BasePlayer()
{
  ....
  delete m_pFlashlight;
}

PVS-Studio generates the V599 warning on the following file: c_baseplayer.cpp 454.

There's the CFlashlightEffect class here which has a non-virtual destructor. But there's also the class CHeadlightEffect inherited from it. I guess you understand what follows from that.

Suspicious arithmetic

There are quite many fragments in the project where integer types and floating-point types are used together. I suspect that some calculations are not accurate enough or even don't make any sense at all. I'll show you only 3 examples; for the rest, see the appendix.

The first suspicious fragment

void TE_BloodStream(....)
{
  ....
  int      speedCopy = amount;
  ....
  speedCopy -= 0.00001; // so last few will drip
  ....
}

PVS-Studio generates the V674 warning on the following file: c_te_bloodstream.cpp 141.

It's too strange to subtract 0.00001 from an 'int' variable.

The second suspicious fragment

#define  ON_EPSILON    0.1      
void CPrediction::SetIdealPitch (....)
{
  int    step;
  ....
  step = floor_height[j] - floor_height[j-1];
  if (step > -ON_EPSILON && step < ON_EPSILON)
    continue;
  ....
}

PVS-Studio generates the V674 warning on the following file: prediction.cpp 977.

The type chosen for the 'step' variable is not quite appropriate.

The third suspicious fragment

virtual int GetMappingWidth( ) = 0;
virtual int GetMappingHeight( ) = 0;

void CDetailObjectSystem::LevelInitPreEntity()
{
  ....
  float flRatio = pMat->GetMappingWidth() /
                  pMat->GetMappingHeight();
  ....
}

PVS-Studio generates the V636 warning on the following file: detailobjectsystem.cpp 1480.

I would suggest calculating the value of the 'flRatio' variable with a higher accuracy as integer division doesn't provide enough of it. To enhance accuracy, the code can be rewritten in the following way:

float flRatio = static_cast<float>(pMat->GetMappingWidth()) /
                pMat->GetMappingHeight();

Miscellaneous

Types confused

enum PhysGunPickup_t
{
  PICKED_UP_BY_CANNON,
  PUNTED_BY_CANNON,
  PICKED_UP_BY_PLAYER,
};

enum PhysGunDrop_t
{
  DROPPED_BY_PLAYER,
  THROWN_BY_PLAYER,
  DROPPED_BY_CANNON,
  LAUNCHED_BY_CANNON,
};

void CBreakableProp::OnPhysGunDrop(...., PhysGunDrop_t Reason)
{
  ....
  if( Reason == PUNTED_BY_CANNON )
  {
    PlayPuntSound(); 
  }
  ....
}

PVS-Studio generates the V556 warning on the following file: props.cpp 1520.

The 'Reason' variable is of the PhysGunDrop_t type, while 'PUNTED_BY_CANNON' is of the 'PhysGunPickup_t' type.

Potentially dangerous fprintf

static void Exit(const char *msg)
{
  fprintf( stderr, msg );
  Pause();
  exit( -1 );
}

PVS-Studio generates the V618 warning on the following file: vice.cpp 52.

The 'fprintf()' function can work quite well, but it is potentially dangerous. If control characters appear - either by accident or consciously - inside the 'msg' string, it will have unpredictable consequences.

See an interesting post on this subject: "Wade not in unknown waters. Part two".

This is the safe version of this code:

fprintf( stderr, "%s", msg );

Appendix

This file contains all the other warnings by PVS-Studio which I found worth attention. But don't rely solely on this list, as I just scanned through the report and could have missed many issues. Besides, static analysis can be truly useful only when being used regularly, not just once for a project.

This is the list of other issues: source-sdk-addition-log.txt

Conclusion

Hope you liked this article and the Source SDK developers found it useful.

Taking the opportunity, let me remind you that we have just recently released a new software product, CppCat. It is a light, cheap and fast static analyzer for C/C++ code. Its diagnostic capabilities are very much close to those of PVS-Studio. We believe this tool will suit most developers. Welcome to try it!