シューティングゲームSerious Samの記念日のためにゲームエンジンSerious Engine v.1.10のソースコードを確認する

2016年3月に行われた一人称シューティングゲームSerious Samのリリースの記念日までに、クロアチアの会社Croteamのゲーム開発者は、ゲームエンジンSerious Engine 1 v.1.10のソースコードを開くことにしました。 彼は、エンジンを学び、改善したい多くの開発者に興味を持ちました。 また、コードの改善に参加することを決め、PVS-Studio静的アナライザーを使用して検出されたエラーの概要を含む記事を作成しました。

はじめに

Serious Engineは、クロアチアの会社Croteamによって開発されたゲームエンジンです 。 バージョンv.1.10はSerious Sam Classic：The First EncounterおよびSerious Sam Classic：The Second Encounterゲームで使用されていました。 その後、Croteamはより高度なゲームエンジンであるSerious Engine 2、Serious Engine 3、およびSerious Engine 4を開発し、Serious Engineバージョン1.10のソースコードが公式に公開され、 GNU General Public License v.2で利用可能になりました。



プロジェクトはVisual Studio 2013で簡単に組み立てられ、 PVS-Studio 6.02静的アナライザーを使用して簡単に検証されることに注意してください。

タイプミス！

V501 「==」演算子の左右に同一の副次式があります。tp_iAnisotropy == tp_iAnisotropy gfx_wrapper.h 180

class CTexParams { public: inline BOOL IsEqual( CTexParams tp) { return tp_iFilter == tp.tp_iFilter && tp_iAnisotropy == tp_iAnisotropy && // <= tp_eWrapU == tp.tp_eWrapU && tp_eWrapV == tp.tp_eWrapV; }; .... };
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

わかりやすくするために、このコードの書式を変更しました。 そのため、アナライザーが検出した欠陥ははるかに顕著です。 変数はそれ自体と比較されます。 「tp」という名前のオブジェクトには「tp_iAnisotropy」というフィールドがあるため、隣接するコードとの類推により、条件の一部は「tp_iAnisotropy == tp.tp_iAnisotropy」のようになります。



V501 「||」の左と右に同一のサブ式「GetShadingMapWidth（）<32」があります 演算子。 terrain.cpp 561

 void CTerrain::SetShadowMapsSize(....) { .... if(GetShadowMapWidth()<32 || GetShadingMapHeight()<32) { .... } if(GetShadingMapWidth()<32 || GetShadingMapWidth()<32) { // <= tr_iShadingMapSizeAspect = 0; } .... PIX pixShadingMapWidth = GetShadingMapWidth(); PIX pixShadingMapHeight = GetShadingMapHeight(); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

ここで、アナライザは特定のカードの幅と高さを確認するために疑わしいコードを検出しました。 より正確には、コードには2つの同一のチェック "GetShadingMapWidth（）<32"が含まれているため、幅のみです。 ほとんどの場合、条件は次のようになります。

 if(GetShadingMapWidth()<32 || GetShadingMapHeight()<32) { tr_iShadingMapSizeAspect = 0; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

V501 「&&」演算子の左右には、同一のサブ式「（vfp_ptPrimitiveType == vfpToCompare.vfp_ptPrimitiveType）」があります。 worldeditor.h 580

 inline BOOL CValuesForPrimitive::operator==(....) { return ( (....) && (vfp_ptPrimitiveType == vfpToCompare.vfp_ptPrimitiveType) &&//<= (vfp_plPrimitive == vfpToCompare.vfp_plPrimitive) && .... (vfp_bDummy == vfpToCompare.vfp_bDummy) && (vfp_ptPrimitiveType == vfpToCompare.vfp_ptPrimitiveType) &&//<= .... (vfp_fXMin == vfpToCompare.vfp_fXMin) && (vfp_fXMax == vfpToCompare.vfp_fXMax) && (vfp_fYMin == vfpToCompare.vfp_fYMin) && (vfp_fYMax == vfpToCompare.vfp_fYMax) && (vfp_fZMin == vfpToCompare.vfp_fZMin) && (vfp_fZMax == vfpToCompare.vfp_fZMax) && .... );
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

多重定義された比較演算子の条件には35行が必要です。 驚くことではないが、便宜上、著者は文字列のコピーを使用してコードの記述を高速化した。 しかし、このアプローチを使用すると、間違いを犯しやすくなります。 おそらくここに余分なチェックがありますが、コピーされた行の名前を変更するのを忘れてしまい、比較演算子が常に正しい結果を返すとは限りません。

たくさんの奇妙な比較

V559 「if」演算子の条件式内の疑わしい割り当て：pwndView =0。mainfrm.cpp 697

 void CMainFrame::OnCancelMode() { // switches out of eventual direct screen mode CWorldEditorView *pwndView = (....)GetActiveView(); if (pwndView = NULL) { // <= // get the MDIChildFrame of active window CChildFrame *pfrChild = (....)pwndView->GetParentFrame(); ASSERT(pfrChild!=NULL); } CMDIFrameWnd::OnCancelMode(); }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

エンジンコードには、多くの疑わしい比較が含まれています。 たとえば、このフラグメントでpwndViewポインターを受信すると、NULLが割り当てられます。このため、条件は常にfalseです。



おそらく、彼らは不等式演算子「！=」を書きたかったのです。ここで、コードは次のようになります。

 if (pwndView != NULL) { // get the MDIChildFrame of active window CChildFrame *pfrChild = (....)pwndView->GetParentFrame(); ASSERT(pfrChild!=NULL); }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

同様のコードスニペットもあります。

• V559「if」演算子の条件式内の疑わしい割り当て：pwndView =0。mainfrm.cpp 710

V547式は常に偽です。 おそらく '||' ここで演算子を使用する必要があります。 entity.cpp 3537

 enum RenderType { .... RT_BRUSH = 4, RT_FIELDBRUSH = 8, .... }; void CEntity::DumpSync_t(CTStream &strm, INDEX iExtensiveSyncCheck) { .... if( en_pciCollisionInfo == NULL) { strm.FPrintF_t("Collision info NULL\n"); } else if (en_RenderType==RT_BRUSH && // <= en_RenderType==RT_FIELDBRUSH) { // <= strm.FPrintF_t("Collision info: Brush entity\n"); } else { .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

「en_RenderType」という名前の1つの変数が2つの異なる定数と比較されます。 エラーは、論理乗算演算子「&&」が使用されていることです。 変数を同時に2つの定数に等しくすることはできないため、条件は常にfalseです。 この時点で、「||」演算子を使用します。



V559 「if」演算子の条件式内の疑わしい割り当て：_strModURLSelected = ""。 menu.cpp 1188

 CTString _strModURLSelected; void JoinNetworkGame(void) { .... char strModURL[256] = {0}; _pNetwork->ga_strRequiredMod.ScanF(...., &strModURL); _fnmModSelected = CTString(strModName); _strModURLSelected = strModURL; // <= if (_strModURLSelected="") { // <= _strModURLSelected = "http://www.croteam.com/mods/Old"; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

興味深い間違い。 この関数では、特定の要求が実行され、結果（「mod」へのURLリンク）が「strModURL」という名前でバッファに書き込まれます。 さらに、この結果は、「CTString」クラスの「_strModURLSelected」という名前のオブジェクトに格納されます。 これは、文字列を操作するためのクラスのネイティブ実装です。 タイプミスのため、条件 'if（_strModURLSelected = ""）'で、以前に受信したURLリンクは、比較する代わりに空の文字列によって擦り切れます。 次に、文字列をconst char *型にキャストする演算子が作用します。 その結果、条件は、空の文字列へのリンクを格納するポインターゼロと比較されます。 このようなポインターは常にゼロ以外です。 したがって、条件は常に真になります。



このコードの結果は、常にコードにハードコードされたリンクの使用になります。 ただし、このリンクをデフォルト値として使用する予定でした。



V547式は常に真です。 ここでは、おそらく「&&」演算子を使用する必要があります。 propertycombobar.cpp 1853

 CEntity *CPropertyComboBar::GetSelectedEntityPtr(void) { // obtain selected property ID ptr CPropertyID *ppidProperty = GetSelectedProperty(); // if there is valid property selected if( (ppidProperty == NULL) || (ppidProperty->pid_eptType != CEntityProperty::EPT_ENTITYPTR) || (ppidProperty->pid_eptType != CEntityProperty::EPT_PARENT) ) { return NULL; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

ここで、アナライザーは前のエラーと反対のエラーを検出しました。 不平等の変数 "pid_eptType"の2つのチェックは、演算子 '||'の使用により常にtrueになります。 したがって、ポインター「ppidProperty」および変数「ppidProperty-> pid_eptType」の値に関係なく、関数からの出口は常に実行されます。



V547式「ulUsedShadowMemory> = 0」は常に真です。 符号なしの型の値は常に> = 0です。gfxlibrary.cpp1693

 void CGfxLibrary::ReduceShadows(void) { ULONG ulUsedShadowMemory = ....; .... ulUsedShadowMemory -= sm.Uncache(); // <= ASSERT( ulUsedShadowMemory>=0); // <= .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

このコードフラグメントでは、unsigned型の変数の安全でないデクリメントが実行されます。 ulUsedShadowMemory変数のオーバーフローが発生する可能性があります。 同時に、近くにAssert（）があり、これは警告を生成しません。 開発者が確認すべき非常に疑わしい場所。



V704 'this！= 0'式は避ける必要があります-'this 'ポインターがNULLになることはないため、この式は新しいコンパイラーでは常にtrueです。 entity.h 697

 inline void CEntity::AddReference(void) { if (this!=NULL) { // <= ASSERT(en_ctReferences>=0); en_ctReferences++; } };
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

エンジンコードには、「this」とゼロの28個の比較が含まれています。 コードは長い間書かれていましたが、最新のC ++言語標準によれば、「this」ポインターをnullにすることはできないため、コンパイラーは最適化を実行してチェックを削除できます。 より複雑な条件では、これにより予期しないエラーが発生する可能性があります。 例は診断資料に記載されています 。



具体的には、Visual C ++はこのように動作しません。 しかし、これは時間の問題です。 そのようなコードはより違法です。



V547式 'achrLine！= ""'は常に真です。 文字列を比較するには、strcmp（）関数を使用する必要があります。 worldeditor.cpp 2254

 void CWorldEditorApp::OnConvertWorlds() { .... char achrLine[256]; // <= CTFileStream fsFileList; // count lines in list file try { fsFileList.Open_t( fnFileList); while( !fsFileList.AtEOF()) { fsFileList.GetLine_t( achrLine, 256); // increase counter only for lines that are not blank if( achrLine != "") ctLines++; // <= } fsFileList.Close(); } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

アナライザーは、空のストリングとの誤ったストリング比較を検出しました。 エラーは、チェック（achrLine！= ""）が常にtrueであり、ctLines変数の増分が常に実行されることです。 コメントでは、これは空でない行に対してのみ行われるべきであると述べていますが。



この動作は、この条件で2つのポインターが比較されるという事実によって引き起こされます。「achrLine」と一時的な空の文字列へのポインターです。 このようなポインターが等しくなることはありません。



strcmp（）関数を使用した正しいコード：

 if(strcmp(achrLine, "") != 0) ctLines++;
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

このような誤った比較がさらに2つあります。

• V547式は常に真です。 文字列を比較するには、strcmp（）関数を使用する必要があります。 propertycombobar.cpp 965
• V547式 'achrLine == ""'は常にfalseです。 文字列を比較するには、strcmp（）関数を使用する必要があります。 worldeditor.cpp 2293

その他のエラー

V541文字列「achrDefaultScript」をそれ自体に印刷することは危険です。 dlgcreateanimatedtexture.cpp 359

 BOOL CDlgCreateAnimatedTexture::OnInitDialog() { .... // allocate 16k for script char achrDefaultScript[ 16384]; // default script into edit control sprintf( achrDefaultScript, ....); // <= .... // add finishing part of script sprintf( achrDefaultScript, // <= "%sANIM_END\r\nEND\r\n", // <= achrDefaultScript); // <= .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

ここでは、バッファ内に特定の行を形成します。 次に、新しい行を取得し、その行の以前の値を保持し、さらに2つの単語を追加したいと考えています。 すべてがシンプルなようです。



ここで予期しない結果が得られる理由を説明するために、この診断のドキュメントから簡単な例を引用します。

 char s[100] = "test"; sprintf(s, "N = %d, S = %s", 123, s);
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

このコードの結果として、次の行を取得します。

 N = 123, S = test
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

しかし、実際には、バッファー内に行が形成されます。

 N = 123, S = N = 123, S =
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

そのような状況では、同様のコードは誤ったテキストを表示するだけでなく、プログラムをクラッシュさせる可能性があります。 新しいバッファを使用して結果を保存する場合、コードを修正できます。 安全なオプション：

 char s1[100] = "test"; char s2[100]; sprintf(s2, "N = %d, S = %s", 123, s1);
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

同様に、シリアスエンジンコードで行う価値があります。 運がコードで動作する場合もありますが、追加のバッファを使用して文字列を形成する方が安全です。



V579 qsort関数は、ポインターとそのサイズを引数として受け取ります。 間違いかもしれません。 3番目の引数を調べます。 mesh.cpp 224

 // optimize lod of mesh void CMesh::OptimizeLod(MeshLOD &mLod) { .... // sort array qsort(&_aiSortedIndex[0] // <= ctVertices sizeof(&_aiSortedIndex[0]), // <= qsort_CompareArray); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

qsort（）関数は、ソートされた配列の要素のサイズの3番目の引数を取ります。 ポインタのサイズが常にそこに渡されるのは非常に疑わしいです。 おそらく、誰かが関数の最初の引数を3番目の引数にコピーしましたが、アドレスを取得する文字を消去するのを忘れていました。



V607所有者なしの式「pdecDLLClass-> dec_ctProperties」。 entityproperties.cpp 107

 void CEntity::ReadProperties_t(CTStream &istrm) // throw char * { .... CDLLEntityClass *pdecDLLClass = en_pecClass->ec_pdecDLLClass; .... // for all saved properties for(INDEX iProperty=0; iProperty<ctProperties; iProperty++) { pdecDLLClass->dec_ctProperties; // <= .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

選択したコード行が何をするかは明確ではありません。 むしろ、何もないことは明らかです。 クラスフィールドはいかなる場合にも使用されません。 おそらく、このエラーはリファクタリング後に発生したか、デバッグ後に行だけが残った可能性があります。



V610未定義の動作。 シフト演算子「<<」を確認してください。 左のオペランド '（-2）'は負です。 layermaker.cpp 363

 void CLayerMaker::SpreadShadowMaskOutwards(void) { #define ADDNEIGHBOUR(du, dv) \ if ((pixLayerU+(du)>=0) \ &&(pixLayerU+(du)<pixLayerSizeU) \ &&(pixLayerV+(dv)>=0) \ &&(pixLayerV+(dv)<pixLayerSizeV) \ &&(pubPolygonMask[slOffsetMap+(du)+((dv)<<pixSizeULog2)])) {\ .... \ } ADDNEIGHBOUR(-2, -2); // <= ADDNEIGHBOUR(-1, -2); // <= .... // <= }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

マクロ「ADDNEIGHBOUR」は関数本体で宣言され、28回連続で使用されます。 負の数はこのマクロに渡され、そこでシフトされます。 最新のCおよびC ++言語標準によれば、負の数をシフトすると未定義の動作が発生します。



V646アプリケーションのロジックの検査を検討してください。 「else」キーワードが欠落している可能性があります。 sessionstate.cpp 1191

 void CSessionState::ProcessGameStream(void) { .... if (res==CNetworkStream::R_OK) { .... } if (res==CNetworkStream::R_BLOCKNOTRECEIVEDYET) { // <= .... } else if (res==CNetworkStream::R_BLOCKMISSING) { .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

コードの設計を見ると、「else」キーワードが条件カスケードに実際に欠落していると仮定できます。



このような別の場所：

• V646アプリケーションのロジックの検査を検討してください。 「else」キーワードが欠落している可能性があります。 terrain.cpp 759

V595 nullptrに対して検証される前に、「pAD」ポインターが使用されました。 行をチェック：791、796。anim.cpp 791

 void CAnimObject::SetData(CAnimData *pAD) { // mark new data as referenced once more pAD->AddReference(); // <= // mark old data as referenced once less ao_AnimData->RemReference(); // remember new data ao_AnimData = pAD; if( pAD != NULL) StartAnim( 0); // <= // mark that something has changed MarkChanged(); }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

記事の最後に、NULLポインターの逆参照の可能性があるエラーの例を示します。 アナライザーの警告を読んだ後、この小さな関数では、pADポインターがどの程度危険であるかが簡単にわかります。 「pAD-> AddReference（）」を呼び出した直後に、チェック「pAD！= NULL」が実行され、この関数へのNULLポインターの転送の可能性が示されます。



ポインターで見つかった危険な場所のリスト全体：

• V595 nullptrに対して検証される前に、「_ ppenPlayer」ポインターが使用されました。 行を確認してください：851、854。computer.cpp 851
• V595 nullptrに対して検証される前に、 '_ meshEditOperations'ポインターが使用されました。 行を確認してください：416、418。modelermeshexporter.cpp 416
• V595 nullptrに対して検証される前に、「_ fpOutput」ポインターが使用されました。 行を確認してください：654、664。modelermeshexporter.cpp 654
• V595 nullptrに対して検証される前に、 '_ appPolPnts'ポインターが使用されました。 行を確認してください：647、676。modelermeshexporter.cpp 647
• V595 nullptrに対して検証される前に、「pModelerView」ポインターが使用されました。 行を確認してください：60、63。dlginfopgglobal.cpp 60
• V595 nullptrに対して検証される前に、「pNewWT」ポインターが使用されました。 行を確認してください：736、744。modeler.cpp 736
• V595 nullptrに対して検証される前に、「pvpViewPort」ポインターが使用されました。 行を確認してください：1327、1353。serioussam.cpp 1327
• V595 nullptrに対して検証される前に、「pDC」ポインターが使用されました。 行を確認：138、139。tooltipwnd.cpp 138
• V595 nullptrに対して検証される前に、「m_pDrawPort」ポインターが使用されました。 行を確認：94、97。wndanimationframes.cpp 94
• V595 nullpenrに対して検証される前に、「penBrush」ポインターが使用されました。 行を確認：9033、9035。worldeditorview.cpp 9033

おわりに

ゲームエンジンSerious Engine 1 v.1.10を確認すると、プロジェクトのコードのエラーが非常に長く続くことがあり、記念日を祝うことさえあることがわかりました。 この記事には、アナライザーレポートの最も興味深い例の一部のみが含まれています。 私は多くの警告をリストに挙げました。 しかし、レポート全体には、このような小規模なプロジェクトに関するかなりの数の警告が含まれています。 Croteamには、より高度なゲームエンジン-深刻なエンジン2、深刻なエンジン3、深刻なエンジン4があります。どれだけ危険なコードが新しいシリーズのエンジンに移行できるか想像できません。 この記事を読んだ後、開発者がプロ​​ジェクトでPVS-Studio静的アナライザーを使用し、高品質のゲームでユーザーを喜ばせることを願っています。 結局のところ、アナライザーは簡単にダウンロードでき 、Visual Studioで簡単に実行 でき 、他のビルドシステムにはスタンドアロンユーティリティがあります。





英語を話す聴衆とこの記事を共有したい場合は、翻訳へのリンクを使用してください：Svyatoslav Razmyslov。 シリアスサムシューターアニバーサリー-シリアスエンジンv。1.10のコードのバグを見つけます。