Project Analysis Source SDK

Source SDK is a set of utilities for creating modifications on the Source engine, developed by Valve Corporation. The source codes of the project were downloaded and tested at the end of 2013. On New Year's holidays, I planned to write an article about the results of inspections. But laziness triumphed over creativity, and I started writing an article only when I returned to work. However, I think it is unlikely that during this period something has changed in the source codes. I offer you to familiarize yourself with the suspicious places that I found using the PVS-Studio code analyzer.

What is the Source SDK

I borrow a project description from Wikipedia:

Source SDK (English Software Development Kit - “application developer kit”) is a set of utilities for creating modifications on the Source engine, distributed free of charge by Valve through the Steam network to all players who have purchased any Source game from Valve. This set allows you to edit maps on two versions of the engine - the 15th and the updated 7th (the old version of the engine used in Half-Life 2 is not used due to compatibility with the new version).
')
Project website: http://source.valvesoftware.com/sourcesdk.php

The project is quite large, so it is not surprising that it can always reveal shortcomings. The analysis was performed using the PVS-Studio tool.

Suspicious Expressions

Dividing the variables into 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 issues a warning V501 to the following file: viewpostprocess.cpp 1888

Pay attention to this:

• (float) nScreenWidth / (float) nScreenWidth
• (float) nScreenHeight / (float) nScreenHeight

These are very suspicious expressions. I find it difficult to say what should be written here, but most likely something else.

Double call of function IsJoystickPOVCode ()

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

PVS-Studio issues a warning V501 to the following file: textentry.cpp 1639

The function 'IsJoystickPOVCode (code)' is called twice. The second call is redundant or should have called another function.

Always a false condition

 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 issues a V547 warning to the following file: vrad.cpp 2412.

The “numbounce <0” condition will never be met. 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 issues a V666 warning to the following file: multiplay_gamerules.cpp 860.

As I understand it, they wanted to check that the name of the weapon begins with the characters "NPC_". But then this code contains a typo. Probably the correct option is this:

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

Errors when working with arrays

Incorrect array size determination

 #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 issues a V511 warning to the following file: disp_vrad.cpp 60

The formal argument “int iNeighbors [512]” is not an array. This is just a pointer. The number '512' tells the programmer that, most likely, the pointer refers to an array of 512 elements. But no more than that. The expression 'sizeof (iNeighbors)' is illegal. It returns not the size of the array, but the size of the pointer. That is, the expression 'sizeof (iNeighbors)' will be equal to 'sizeof (void *).

This error could have been avoided if a more secure macro was used. For example, such:

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

When trying to calculate the size of a pointer, an error occurs at the compilation stage. Such a macro is used in the Chromium project. You can read more about this magical design in the article " PVS-Studio vs Chromium ".

Wrong definition of string length

 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 issues a V579 warning message to the following file: vgui_messagechars.cpp 240

The expression “sizeof (msg-> text)” computes the size of the pointer, and not at all the length of the string. Most likely, you should write here: Q_strcpy (msg-> text, data);

Work with 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 issues a warning V507 to the following file: baseflex.cpp 1326

The address of the temporary array is placed in the 'pszAct' variable. Since this array will be destroyed, the address contained in 'pszAct' cannot be used. However, in practice, this code can work, which creates a false appearance of correct code. It is very likely that the memory area occupied by the 'szActName' temporary array is not used further. As a result, the program behaves as the programmer expects. However, this is luck and nothing more.

Overrun array

 #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 issues a warning V557 to the following file: hud_weaponselection.cpp 632, 633.

The loop counter takes values ​​from 0 to 6. However, the xModifiers and yModifiers arrays contain only 4 elements. As a result, an overrun of the array will occur

Dangerous use of the new operator.

Checks without meaning

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

PVS-Studio issues a V668 warning message to the following file: disp_vbsp.cpp 532.

If it is not possible to create an object of the type 'CCoreDispInfo', then the g_CoreDispInfos.Purge () function should be called. But this function will not be called. If a memory allocation error occurs, the std :: bad_alloc exception will be thrown. This code is outdated and should be modified to reflect changes in the behavior of the 'new' operator.

Other places where it is checked that the returned operator 'new' will be listed at the end of the article in the appendix.

New operator in destructor

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

PVS-Studio issues a V509 warning message to the following file: particles_new.cpp 92.

It is dangerous to use constructs in a destructor that can lead to an exception. This is exactly the construction of the 'new' operator. In case of a memory allocation error, it generates an exception.

Let me explain the danger. If an exception occurs in the program, the stack collapses, during which objects are destroyed by calling destructors. If the destructor of the object being destroyed when the stack collapses throws another exception, and this exception leaves the destructor, the C ++ library immediately crashes the program by calling the terminate () function.

Typos

Error in 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 issues a V534 warning to the following file: beamdraw.cpp 592.

Pay attention to the second cycle:

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

The condition for completing a nested loop uses the variable 'i', which refers to the outer loop. I have a strong suspicion that we are dealing with 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 issues a V525 warning message to the following file: networkvar.h 455.

I think the function should have been written like this:

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

Pay attention to the last function call.

The aftermath 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 issues a V537 warning message to the following file: trace2.cpp 189.

Most likely, this code was written using Copy-Paste. See, the first line uses class members with the name 'x'. In the second, with the name 'y'. And in the third there is both 'z' and 'y'. Most likely, the code should be like this:

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

Assigning different values ​​to one 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 issues a warning V519 to the following file: vgui_fpspanel.cpp 192.

Apparently, it should have been written:

 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 issues a V519 warning message to the following file: ai_utils.cpp 49.

Again a typo, due to which:

1. The variable m_flMinRestInterval is assigned a zero value two times.
2. The variable m_flMaxRestInterval remains uninitialized.

Similar troubles are in the class constructors CEnvTonemapController, CBasePlayerAnimState. But it’s boring to describe similar examples, so I put the relevant ones in the appendix (see the end of the article).

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 issues a V567 warning to the following file: c_baseanimating.cpp 5301, 5302.

Why an undefined behavior occurs here and why the value of the variable 'm_nResetEventsParity' cannot be predicted well described in the documentation. In the description there is an example of very similar code.

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 issues a V610 warning to the following file: c_func_breakablesurf.cpp 157.

Shifting negative numbers leads to undefined behavior. In this code, the negative number is '~ 0x03'. I have already discussed the question of the shift of negative numbers in more detail in the article “ Without knowing the ford, do not climb into the water. Part Three ”.

There is no virtual destructor

 class CFlashlightEffect { .... ~CFlashlightEffect(); .... }; class CHeadlightEffect : public CFlashlightEffect { .... }; CFlashlightEffect *m_pFlashlight; C_BasePlayer::~C_BasePlayer() { .... delete m_pFlashlight; } 

PVS-Studio issues a V599 warning message to the following file: c_baseplayer.cpp 454.

There is a class CFlashlightEffect. It has no virtual destructor. However, the CHeadlightEffect class is inherited from this class. The resulting consequences, I think, are clear.

Suspicious arithmetic

There are a lot of places in the project where integer types and floating point types are strangely combined. There is a suspicion that some calculations are not accurate enough or have no meaning at all. I will demonstrate further only 3 examples. The remaining suspicious places are listed in the app.

First suspicious fragment

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

PVS-Studio issues a warning V674 to the following file: c_te_bloodstream.cpp 141.

It is very suspicious to subtract the value 0.00001 from a variable of the type 'int'.

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 issues a warning V674 to the following file: prediction.cpp 977.

The type of the variable 'step' is not very well chosen.

Third suspicious fragment

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

PVS-Studio issues a warning V636 to the following file: detailobjectsystem.cpp 1480.

Perhaps it makes sense to calculate the value of the 'flRatio' variable more accurately. The reason for the low accuracy is integer division. To improve accuracy, you can write this:

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

miscellanea

Type confusion

 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 issues a warning V556 to the following file: props.cpp 1520.

The variable 'Reason' is of type PhysGunDrop_t. And 'PUNTED_BY_CANNON' refers to 'PhysGunPickup_t'.

Potentially dangerous fprintf

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

PVS-Studio issues a V618 warning to the following file: vice.cpp 52.

The 'fprintf ()' function can work quite correctly. However, it is potentially dangerous. If accidentally or consciously in the string 'msg' control characters are contained, the consequences will be unpredictable.

An interesting note on this topic: " Without knowing the ford, do not climb into the water. Part two ."

Safe code option:

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

application

The file will list other PVS-Studio warnings that, in my opinion, deserve attention. But you should not completely rely on this list. I looked at the project superficially and could not pay attention to many things. Plus, the real benefits of static analysis are its regular use, not one-time verification of the project.

List of other suspicious places: source-sdk-addition-log.txt

Conclusion

I hope this article was interesting to readers and useful to the developers of the project Source SDK.