ソースSDKプロジェクトの分析

Source SDK-Valve Corporationによって開発された、ソースエンジンの変更を作成するためのユーティリティのセット。 プロジェクトのソースコードは、2013年の終わりにダウンロードおよび検証されました。 年末年始に、私は検査の結果に関する記事を書くつもりでした。 しかし、怠は創造性に勝ち、仕事に戻ったときだけ記事を書き始めました。 ただし、この期間中にソースコードが変更された可能性は低いと思います。 PVS-Studioコードアナライザーを使用して見つけた疑わしい場所をよく理解することをお勧めします。

ソースSDKとは

ウィキペディアからプロジェクトの説明を借りる：



Source SDK （ソフトウェア開発キット）は、ソースエンジンで変更を作成するためのユーティリティセットです。Valveは、ValveからSourceゲームを購入するすべてのプレイヤーにSteamネットワークを介して無料で配布します。 このセットを使用すると、15バージョンと更新された7バージョンのエンジンの2つのバージョンでマップを編集できます（Half-Life 2で使用されたエンジンの古いバージョンは、新しいバージョンとの互換性のため使用されません）。



プロジェクトサイト： http : //source.valvesoftware.com/sourcesdk.php



このプロジェクトは十分に大きいため、プロジェクトの欠点を常に特定できることは驚くことではありません。 分析はPVS-Studioツールを使用して実行されました。

疑わしい表現

変数を自分自身に分割する

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は、警告V501を次のファイルに発行します：viewpostprocess.cpp 1888



これに注意してください：

• （フロート）nScreenWidth /（フロート）nScreenWidth
• （フロート）nScreenHeight /（フロート）nScreenHeight

これらは非常に疑わしい表現です。 ここに何を書くべきかを言うのは難しいと思いますが、ほとんどの場合、他の何かを言います。

IsJoystickPOVCode（）関数の二重呼び出し

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studioは、警告V501を次のファイルに発行します：textentry.cpp 1639



関数 'IsJoystickPOVCode（code）'が2回呼び出されます。 2番目の呼び出しは冗長であるか、別の関数が呼び出されている必要があります。

常に偽の状態

 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は、次のファイルにV547警告を発行します：vrad.cpp 2412。



条件「numbounce <0」が満たされることはありません。 符号なし変数はゼロより小さくすることはできません。

奇妙な文字列比較

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studioは、ファイルmultiplay_gamerules.cpp 860に対してV666警告を発行します。



私が理解しているように、彼らは武器の名前が文字「NPC_」で始まることを確認したかったのです。 しかし、このコードにはタイプミスが含まれています。 おそらく正しいオプションはこれです：

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

配列を操作するときのエラー

配列のサイズが正しくありません

 #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は、 V511警告を次のファイルに発行します：disp_vrad.cpp 60



正式な引数「int iNeighbors [512]」は配列ではありません。 これは単なるポインタです。 数値「512」は、おそらくポインターが512要素の配列を指していることをプログラマーに伝えます。 しかし、それ以上ではありません。 「sizeof（iNeighbors）」という表現は違法です。 配列のサイズではなく、ポインターのサイズを返します。 つまり、式「sizeof（iNeighbors）」は「sizeof（void *）」に等しくなります。



このエラーは、より安全なマクロを使用することで回避できました。 たとえば、これ：

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

ポインターのサイズを計算しようとすると、コンパイル段階でエラーが発生します。 このようなマクロは、Chromiumプロジェクトで使用されます。 この魔法の構造の詳細については、「 PVS-Studio vs Chromium 」の記事をご覧ください。

文字列の長さの誤った定義

 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は次のファイルに警告V579を発行します：vgui_messagechars.cpp 240



式「sizeof（msg-> text）」は、文字列の長さではなく、ポインターのサイズを計算します。 ほとんどの場合、ここに書き込む必要があります。Q_strcpy（msg-> text、data）;

破壊された配列を操作する

 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は次のファイルに警告V507を発行します：baseflex.cpp 1326



一時配列のアドレスは変数「pszAct」に配置されます。 この配列は破棄されるため、「pszAct」に含まれるアドレスを使用することはできません。 ただし、実際には、このコードは機能する可能性があり、コードの正確性の誤った外観を作成します。 一時配列 'szActName'によって占有されているメモリ領域はそれ以上使用されない可能性が非常に高くなります。 その結果、プログラムはプログラマが期待するとおりに動作します。 しかし、これは運であり、それ以上のものではありません。

海外に行く

 #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は次のファイルに警告V557を発行します：hud_weaponselection.cpp 632、633。



ループカウンターは0〜6の値を取ります。ただし、xModifiersおよびyModifiers配列には4つの要素しか含まれていません。 その結果、配列のオーバーフローが発生します。

新しいオペレーターの危険な使用。

無意味なチェック

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studioは、次のファイルに対して警告V668を発行します：disp_vbsp.cpp 532。



「CCoreDispInfo」タイプのオブジェクトを作成できない場合、g_CoreDispInfos.Purge（）関数を呼び出す必要があります。 ただし、この関数は呼び出されません。 メモリ割り当てエラーが発生すると、例外std :: bad_allocがスローされます。 このコードは非推奨であり、「new」演算子の動作の変更を反映するために変更する必要があります。



「new」演算子が返されることが確認される他の場所は、付録の記事の最後に記載されています。

デストラクタの新しい演算子

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studioは次のファイルにV509警告を発行します：particles_new.cpp 92。



例外につながる可能性のあるデストラクタで構造を使用することは危険です。 これはまさに「新しい」演算子の構築です。 メモリ割り当てエラーが発生した場合、例外をスローします。



危険について説明します。 プログラムで例外が発生すると、スタックの折りたたみが開始され、その間、デストラクタを呼び出すことによりオブジェクトが破棄されます。 スタックが崩壊したときに破壊されるオブジェクトのデストラクタが別の例外をスローし、デストラクタがこの例外を残すと、C ++ライブラリはterminate（）関数を呼び出してプログラムをすぐにクラッシュさせます。

タイプミス

ネストされたループのエラー

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studioは、ファイルbeamdraw.cpp 592に対してV534警告を発行します。



2番目のサイクルに注意してください。

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

ネストされたループを完了するための条件では、変数「i」が使用されます。これは外側のループを指します。 私たちはタイプミスを扱っているという強い疑いがあります。

誤った初期化

 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は次のファイルにV525警告を発行します：networkvar.h 455。



私には、関数は次のように書かれていたはずです：

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

最後の関数呼び出しに注意してください。

コピーペーストの結果

 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は、次のファイルに対して警告V537を発行します：trace2.cpp 189。



ほとんどの場合、このコードはコピーペーストを使用して記述されています。 最初の行では、「x」という名前のクラスのメンバーを使用しています。 2番目の名前は「y」です。 そして、3番目には「z」と「y」の両方があります。 ほとんどの場合、コードは次のようになります。

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

1つの変数に異なる値を割り当てる

 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は、vgui_fpspanel.cpp 192ファイルに対してV519警告を発行します。



どうやら、書かれている必要があります：

 nFPSThreshold1 = 60; nFPSThreshold2 = 50;
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

悪いコンストラクタ

 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は、次のファイルにV519警告を発行します：ai_utils.cpp 49。



再びタイプミス、これにより：

1. 変数m_flMinRestIntervalは2回ゼロに設定されます。
2. 変数m_flMaxRestIntervalは初期化されません。

クラスコンストラクターCEnvTonemapController、CBasePlayerAnimStateにも同様の問題があります。 しかし、同様の例を説明するのは退屈なので、アプリケーションに対応するものを入れます（記事の最後を参照）。

未定義の動作

複合式

 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は、次のファイルに対して警告V567を発行します：c_baseanimating.cpp 5301、5302。



ここで未定義の動作が発生し、変数「m_nResetEventsParity」の値を予測できない理由は、ドキュメントに詳しく説明されています。 説明には非常によく似たコードの例があります。

シフト

 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は、次のファイルにV610警告を発行します：c_func_breakablesurf.cpp 157。



負の数をシフトすると、未定義の動作が発生します。 このコードでは、負の数は「〜0x03」です。 より詳しくは、「 フォードを知らないで、水に入らないでください。パート3 」 という記事ですでに検討した負の数のシフトの問題です。

仮想デストラクタがありません

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studioは、ファイルc_baseplayer.cpp 454に対してV599警告を発行します。



クラスCFlashlightEffectがあります。 仮想デストラクタはありません。 ただし、CHeadlightEffectクラスはこのクラスから継承されます。 これから生じる結果は理解できると思います。

疑わしい算術

このプロジェクトには、整数型と浮動小数点型が奇妙に組み合わされた場所がたくさんあります。 一部の計算は十分に正確ではないか、まったく意味をなさないことが疑われています。 以下に3つの例を示します。 残りの不審な場所はアプリにリストされます。

最初の不審なフラグメント

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studioは、ファイルc_te_bloodstream.cpp 141に対してV674警告を発行します。



'int'型の変数から0.00001を減算することは非常に疑わしいです。

2番目の不審なフラグメント

 #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は、次のファイルに対して警告V674を発行します： predict.cpp 977。



変数タイプ「step」はあまり適切に選択されていません。

3番目の疑わしいフラグメント

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studioは、次のファイルにV636警告を発行します：detailobjectsystem.cpp 1480。



おそらく、変数 'flRatio'の値をより正確に計算するのが理にかなっています。 精度が低い理由は整数除算です。 精度を高めるために、これを書くことができます：

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

雑多

型の混乱

 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は、props.cpp 1520ファイルに対して警告V556を発行します。



変数「Reason」のタイプはPhysGunDrop_tです。 また、「PUNTED_BY_CANNON」は「PhysGunPickup_t」を指します。

潜在的に危険なfprintf

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studioは、次のファイルにV618警告を発行します：vice.cpp 52。



関数 'fprintf（）'は非常に正しく動作する可能性があります。 ただし、潜在的に危険です。 制御文字が誤ってまたは意図的に文字列「msg」に含まれている場合、結果は予測できません。



このテーマに関する興味深いメモ：「 フォードを知らないで、水に入らないでください。パート2。 」



コードの安全なバージョン：

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

アプリ

私の意見では、注目に値するその他のPVS-Studio警告は、ファイルにリストされます。 ただし、このリストに完全に依存しないでください。 私は表面的にプロジェクトを見て、あまり注意を払うことができませんでした。 さらに、静的解析の本当の利点は、プロジェクトの1回限りの検証ではなく、定期的に使用することです。



他の不審な場所のリスト： source-sdk-addition-log.txt

おわりに

この記事が読者にとって興味深いものであり、Source SDKプロジェクトの開発者にとって有益であることを願っています。