Checking the source code of the game engine Serious Engine v.1.10 for the anniversary of the shooter Serious Sam

On the anniversary of the release of the first-person shooter Serious Sam, which was held in March 2016, the game developers from the Croatian company Croteam decided to open the source code of the game engine Serious Engine 1 v.1.10. He interested a lot of developers who wanted to explore and improve the engine. I also decided to participate in improving the code and prepared an article with an overview of the errors found using the PVS-Studio static analyzer.

Introduction

Serious Engine is a game engine developed by Croatian company Croteam . Version v.1.10 was used in the games Serious Sam Classic: The First Encounter and Serious Sam Classic: The Second Encounter. Subsequently, Croteam developed more advanced game engines - Serious Engine 2, Serious Engine 3 and Serious Engine 4, and the source code for Serious Engine version 1.10 was officially opened and available under the GNU General Public License v.2 .

I want to note that the project is easily assembled in Visual Studio 2013 and is easily verified using the PVS-Studio 6.02 static analyzer.

Typos!

')
V501 operator: 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; }; .... }; 

For clarity, I changed the formatting of this code fragment. So the defect that the analyzer found is much more noticeable. The variable is compared to itself. The object with the name 'tp' has the field 'tp_iAnisotropy', therefore, by analogy with the neighboring code, part of the condition should look like "tp_iAnisotropy == tp.tp_iAnisotropy".

V501 GetShadingMapWidth () <32 'There are identical sub-expressions operator. 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(); .... } 

Here the analyzer detected a suspicious code to check the width and height of a certain card. Only widths are more exact, because the code contains two identical checks “GetShadingMapWidth () <32”. Most likely, the condition should be like this:

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

V501 There are identical sub-expressions '(vfp_ptPrimitiveType == vfpToCompare.vfp_ptPrimitiveType)' operator. 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) && .... ); 

The condition in the overloaded comparison operator takes 35 lines. It is not surprising that, for convenience, the author used the copying of lines to speed up the writing of code. But with this approach it is easy to make a mistake. Perhaps there is an extra check here, but you may have forgotten to rename the copied line, and the comparison operator now does not always return the correct result.

Many strange comparisons

V559 Suspicious assignment: the operator of the condition: 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(); } 

There are many suspicious comparisons in the engine code. For example, in this fragment, the pointer “pwndView” is obtained, and then it is assigned a NULL, which is why the condition is always false.

Most likely, here they wanted to write the inequality operator '! =' And the code should be like this:

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

There is another similar code snippet:

• V559 Suspicious assignment of the operator of the condition: pwndView = 0. mainfrm.cpp 710

V547 Expression is always false. Probably the '||' operator should be used here. 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 { .... } .... } 

One variable named "en_RenderType" is compared with two different constants. The error is that the logical multiplication operator '&&' is used. A variable cannot be equal to two constants at the same time, therefore the condition is always false. At this point, use the operator '||'.

V559 Suspicious assignment of the operator: _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"; } .... } 

An interesting mistake. In this function, a certain query is executed and the result is written into the buffer with the name “strModURL” (url-link to “mod”). Further, this result is stored in an object with the name "_strModURLSelected" of the class "CTString". This is an own implementation of the class for working with strings. Due to a typo, in the 'if (_strModURLSelected = "")' condition, the previously received url link is triturated with an empty string instead of a comparison with it. Next comes the string casting operator type const char *. As a result, the condition will make a comparison with a pointer zero, which stores a reference to an empty string. Such a pointer is always non-zero. Therefore, the condition will always be true.

The result of this code will always be the use of a link that is strictly prescribed in the code. Although planned to use this link as the default.

V547 Expression is always true. Probably the '&&' operator should be used here. 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; } .... } 

Here the analyzer detected an error opposite to the previous one. Two checks on the "pid_eptType" variable for inequality always give the truth due to the use of the operator '||'. Therefore, the exit from the function is always performed, regardless of the value of the ppidProperty pointer and the variable ppidProperty-> pid_eptType.

V547 Expression 'ulUsedShadowMemory> = 0' is always true. Unsigned type value is always> = 0. gfxlibrary.cpp 1693

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

In this code fragment, unsafe decrement of an unsigned variable is performed, since an overflow of the “ulUsedShadowMemory” variable may occur. At the same time there is Assert (), which will never issue a warning. A very suspicious place that developers should check out.

V704 'this! = 0' it should be compiled, because it can be never. NULL. entity.h 697

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

The engine code contains 28 comparisons of 'this' with zero. The code was written a long time ago, but according to the modern standard of the C ++ language, the 'this' pointer cannot be zero, therefore, the compiler can perform optimization and remove the check. In more difficult conditions, this can lead to unexpected errors. Examples can be found in the diagnostic documentation .

Specifically, Visual C ++ does not behave this way. But this is a matter of time. Such a code is more outlawed.

V547 Expression 'achrLine! = ""' Is always true. To compare strings you should use strcmp () function. 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(); } .... } 

The analyzer detected an incorrect string comparison with an empty string. The error is that the test (achrLine! = "") Is always true and the increment of the variable "ctLines" is always executed. Although the commentary says that this should only be done for non-empty strings.

This behavior is caused by the fact that in this condition two pointers are compared: “achrLine” and a pointer to a temporary empty line. Such pointers will never be equal.

Correct code using the strcmp () function:

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

There are two more such wrong comparisons:

• V547 Expression is always true. To compare strings you should use strcmp () function. propertycombobar.cpp 965
• V547 Expression 'achrLine == "" is always false. To compare strings you should use strcmp () function. worldeditor.cpp 2293

Mistakes

V541 It is dangerous to print the string 'achrDefaultScript' into itself. 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); // <= .... } 

Here form a certain line in the buffer. Then they want to get a new line, keeping the previous value of the line, and add two more words to it. It seems simple.

To explain why an unexpected result can be obtained here, I will quote a simple example from the documentation for this diagnostic:

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

As a result of this code, I want to get the line:

 N = 123, S = test 

But in practice, the string will be formed in the buffer:

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

In such situations, a similar code can lead not only to the output of incorrect text, but also to the emergency termination of the program. The code can be corrected by using a new buffer to save the result. Safe option:

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

Similarly, it is worth doing in the code Serious Engine. Although code may work due to luck, it will be safer to use an additional buffer to form a string.

It is a V579 . It is possibly a mistake. Inspect the third argument. mesh.cpp 224

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

The qsort () function takes as its third argument the size of the element of the array being sorted. It is very suspicious that the size of the pointer is always passed there. Most likely someone copied the first argument of the function to the third, but forgot to erase the character of taking the address.

V607 Ownerless expression '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; // <= .... } .... } 

It is not clear what the highlighted line of code does. Rather, it is clear that just nothing. The class field is not used at all. Perhaps this error occurred after refactoring, or just a line remained after debugging.

V610 Undefined behavior. Check the shift operator '<<'. The left operand '(- 2)' is negative. 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); // <= .... // <= } 

The macro "ADDNEIGHBOUR" is declared in the function body and is used 28 times in a row. Negative numbers are passed to this macro where they are shifted. According to modern C and C ++ standards, a negative number shift leads to undefined behavior.

V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. sessionstate.cpp 1191

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

Looking at the design of the code, it can be assumed that in the cascade of conditions the 'else' keyword is indeed missing.

Another place:

• V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. terrain.cpp 759

V595 The 'pAD' pointer was used before it was verified against nullptr. Check lines: 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(); } 

At the end of the article I want to give an example of an error with a potential dereference of a null pointer. After reading the analyzer warning, in this small function it is easy to see how dangerous the "pAD" pointer is used. Almost immediately after calling “pAD-> AddReference ()”, a “pAD! = NULL” check is performed, indicating that a null pointer can be passed to this function.

The entire list of found dangerous places with signs:

• V595 The '_ppenPlayer' pointer was used before it was verified against nullptr. Check lines: 851, 854. computer.cpp 851
• V595 The '_meshEditOperations' pointer was used before it was verified against nullptr. Check lines: 416, 418. modelermeshexporter.cpp 416
• V595 The '_fpOutput' pointer was used before it was verified against nullptr. Check lines: 654, 664. modelermeshexporter.cpp 654
• V595 The '_appPolPnts' pointer was used before it was verified against nullptr. Check lines: 647, 676. modelermeshexporter.cpp 647
• V595 The 'pModelerView' pointer was used before it was verified against nullptr. Check lines: 60, 63. dlginfopgglobal.cpp 60
• V595 The 'pNewWT' pointer was used before it was verified against nullptr. Check lines: 736, 744. modeler.cpp 736
• V595 The 'pvpViewPort' pointer was used before it was verified against nullptr. Check lines: 1327, 1353. serioussam.cpp 1327
• V595 The 'pDC' pointer was used before it was verified against nullptr. Check lines: 138, 139. tooltipwnd.cpp 138
• V595 The 'm_pDrawPort' pointer was used before it was verified against nullptr. Check lines: 94, 97. wndanimationframes.cpp 94
• V595 The 'penBrush' pointer was used before it was verified against nullptr. Check lines: 9033, 9035. worldeditorview.cpp 9033

Conclusion

Checking the game engine Serious Engine 1 v.1.10 showed that errors in the code of projects can live for a very long time and even celebrate the anniversary! The article includes only some of the most interesting examples from the analyzer report. I gave a lot of warnings just a list. But the entire report contains quite a few warnings for such a small project. Croteam has more advanced game engines - Serious Engine 2, Serious Engine 3 and Serious Engine 4. I am afraid to imagine how much dangerous code could migrate to the new series of the engine. I hope that after reading the article, the developers will use the PVS-Studio static analyzer in their projects and will delight users with high-quality games. After all, the analyzer is easy to download , easy to run in Visual Studio, and for any other build systems there is a Standalone utility.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Serious Sam Shooter anniversary - finding bugs in the code of the Serious Engine v.1.10 .