Serious Sam shooter anniversary - finding bugs in the code of the Serious Engine v.1.10

Author: Svyatoslav Razmyslov

The first-person shooter 'Serious Sam' celebrated its release anniversary on March, 2016. In honor of this, the game developers form the Croatian company Croteam decided to open the source code for the game engine, Serious Engine 1 v.1.10. It provoked the interest of a large number of developers, who got an opportunity to have a look at the code and improve it. I have also decided to participate in the code improvement, and wrote an article reviewing the bugs that were found by PVS-Studio analyzer.

Introduction

Serious Engine is a game engine developed by a Croatian company Croteam. V 1.1o, and was used in the games 'Serious Sam Classic: The First Encounter', and 'Serious Sam Classic: The Second Encounter'. Later on, the Croteam Company released more advanced game engines - Serious Engine 2, Serious Engine 3, and Serious Engine 4; the source code of Serious Engine version 1.10 was officially made open and available under the license GNU General Public License v.2

The project is easily built in Visual Studio 2013, and checked by PVS-Studio 6.02 static analyzer.

Typos!

V501 There are identical sub-expressions to the left and to the right of the '==' 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; };
  ....
};

I have changed the formatting of this code fragment to make it more visual. The defect, found by the analyzer became more evident - the variable is compared with itself. The object with the name 'tp' has a field 'tp_iAnisotropy', so, by the analogy with the neighboring part of the code, a part of the condition should be 'tp_iAnisotropy'.

V501 There are identical sub-expressions 'GetShadingMapWidth() < 32' to the left and to the right of the '||' 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();
  ....
}

The analyzer found a suspicious code fragment which checks the width and height of a map, of the width, to be more exact, because we can see two similar checks "GetShadingMapWidth()<32" in the code. Most probably, the conditions should be:

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

V501 There are identical sub-expressions '(vfp_ptPrimitiveType == vfpToCompare.vfp_ptPrimitiveType)' to the left and to the right of the '&&' 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. No wonder the author was copying the strings to write faster, but it's very easy to make an error coding in such a way. Perhaps there is an extra check here, or the copied string was not renamed, and the comparison operator doesn't always return a correct result.

Strange comparisons

V559 Suspicious assignment inside the condition expression of 'if' operator: 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 is quite a number of strange comparisons in the code of the engine. For example, in this code fragment we get a pointer "pwndView", which is then assigned with NULL, making the condition always false.

Most likely the programmer meant to write the inequality operator '!=' and the code should have been like this:

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

Two more similar code fragments:

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 with the name "en_RenderType" is compared with two different constants. The error is in the usage of '&&' logical and operator. A variable can never be equal to two constants at the same time, that's why the condition is always false. The '||' operator should be used in this fragment.

V559 Suspicious assignment inside the condition expression of 'if' 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 bug. A request is performed in this function, and the result with the name "strModURL" is written in the buffer (url to "mod"). Later this result is saved in the object under the name "_strModURLSelected". This is its own class implementation that works with strings. Because of a typo, in the condition "if (_strModURLSelected="")" the url that was received earlier will be replaced with an empty string, instead of comparison. Then the operator, casting the string to the 'const char*' type takes action. As a result we'll have verification against null of the pointer which contains a link to the empty string. Such a pointer can never be equal to zero. Therefore, the condition will always be true. So, the program will always use the link that is hard coded, although it was meant to be used as a default value.

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;
 }
 ....
}

The analyzer detected a bug that is totally different from the previous one. Two checks of the "pid_eptType" variable are always true because of the '||' operator. Thus, the function always returns, regardless of the value of the "ppidProperty" pointer value and "ppidProperty->pid_eptType" variable.

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);      // <=
  ....
}

An unsafe decrement of an unsigned variable is executed in this code fragment, as the variable "ulUsedShadowMemory" may overflow, at the same time there is Assert() that never issues a warning. It is a very suspicious code fragment, the developers should recheck it.

V704 'this != 0' expression should be avoided - this expression is always true on newer compilers, because 'this' pointer can never be NULL. entity.h 697

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

There are 28 comparisons of 'this' with null in the code of the engine. The code was written a long time ago, but according to the latest standard of C++ language, 'this' pointer can never be null, and therefore the compiler can do the optimization and delete the check. This can lead to unexpected errors in the case of more complicated conditions. Examples can be found in the documentation for this diagnostic.

At this point Visual C++ doesn't work like that, but it's just a matter of time. This code is outlawed from now on.

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 wrong comparison of a string with an empty string. The error is that the (achrLine != "") check is always true, and the increment of the "ctLines" is always executed, although the comments say that it should execute only for non-empty strings.

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

Correct code, using the strcmp() function:

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

Two more wrong comparisons:

Miscellaneous errors

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);       // <=
  ....
}

A string is formed in the buffer, then the programmer wants to get a new string, saving the previous string value and add two more words. It seems really simple.

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

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

As a result we would want to have a string:

N = 123, S = test

But in practice, we will have the following string in the buffer:

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

In similar situations, the same code can lead not only to incorrect text, but also to program abortion. The code can be fixed if you use a new buffer to store the result. A safe option:

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

The same should be done in the Serious Engine code. Due to pure luck, the code may work correctly, but it would be much safer to use an additional buffer to form the string.

V579 The qsort function receives the pointer and its size as arguments. 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 function qsort() takes the size of the element of array to be sorted as the third argument. It is very suspicious that the pointer size is always passed there. Perhaps the programmer copied the first argument of the function to the third one, and forgot to delete the ampersand.

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's unclear, what the highlighted string does. Well, it's clear that it does nothing. The class field is not used in any way, perhaps this error got here after refactoring or the string was left unchanged 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 body of the function, and is used 28 times in a row. Negative numbers are passed to this macro, where they are shifted. According to the latest standards of the C++ language, the shift of a negative number results in 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 code formatting, we may assume that the keyword 'else' is missing in the cascade of conditions.

One more similar fragment:

V595 The 'pAD' pointer was utilized 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();
}

In the end I would like to give an example of an error with potential dereference of a null pointer. If you read the analyzer warning, you will see how dangerous the pointer "pAD" is in this small function. Almost immediately after the call of "pAD->AddReference()", the check "pAD != NULL"is executed, which denotes a possible passing of a pointer to this function.

Here is a full list of dangerous fragments that contain pointers:

Conclusion

The analysis of Serious Engine 1 v.1.10 showed that bugs can live in the program for a very long time, and even celebrate anniversaries! This article contains only some of the most interesting examples from the analyzer report. Several warnings were given as a list. But the whole report has quite a good number of warnings, taking into account that the project is not very large. The Croteam Company have more advanced game engines - Serious Engine 2, Serious Engine 3 and Serious Engine 4. I hate to think, how much of the unsafe code could get into the new versions of the engine. I hope that the developers will use a static code analyzer, and make the users happy, producing high-quality games. Especially knowing that the analyzer is easy to download, easy to run in Visual Studio, and for other systems there is a Standalone utility.