Inspection of the Spring RTS engine

Spring RTS is an engine for real-time strategy games. Spring was originally written to repeat the popular Total Annihilation game in the 90th / 00th. In the future, many other beautiful and interesting strategies appeared on this engine, including commercial ones. The games for it are cross-platform and are three-dimensional real-time strategies with huge maps and a large number of combat and construction units. Games have stability issues. Let's try to look at the source (good, open source project).

The official site .

Source code .

As an open source project, Spring RTS includes a number of open libraries, which may also contain errors, and, ultimately, they become part of the engine / games. Some messages will come from libraries that come with the engine. There were especially many warnings in Assimp (Open Asset Import Library).

Code verification was performed using the PVS-Studio tool . The article may not indicate all the errors that can be found using the analyzer. Therefore, do not use the article as a guide to action. Developers can get much more benefit by checking the project on their own.

Typos

V501 There are identical sub-expressions 'aha-> mNumWeights! = Oha-> mNumWeights' to the left and to the right of the '||' operator. assimp findinstancesprocess.cpp 87

struct aiBone
{
  C_STRUCT aiString mName;
  unsigned int mNumWeights;
  C_STRUCT aiVertexWeight* mWeights;
  C_STRUCT aiMatrix4x4 mOffsetMatrix;
  ....
};
bool CompareBones(const aiMesh* orig, const aiMesh* inst)
{
  ....
  aiBone* aha = orig->mBones[i];
  aiBone* oha = inst->mBones[i];
  if (aha->mNumWeights   != oha->mNumWeights   ||  //<==
      aha->mOffsetMatrix != oha->mOffsetMatrix ||
      aha->mNumWeights   != oha->mNumWeights) {    //<==
      return false;
  }
  ....
}

Two identical conditional expressions. Perhaps in one of the cases it is necessary to compare the 'mName' or 'mWeights' field of the aiBone structure.

V501 There are identical sub-expressions to the left and to the right of the '||' operator: 0 == pArchive || 0 == pArchive assimp q3bspfileimporter.cpp 631

bool Q3BSPFileImporter::importTextureFromArchive(
  const Q3BSP::Q3BSPModel *pModel,
  Q3BSP::Q3BSPZipArchive *pArchive, aiScene* /*pScene*/,
  aiMaterial *pMatHelper, int textureId )
{
  ....
  if( NULL == pArchive || NULL == pArchive || NULL == pMatHelper)
  {
    return false;
  }
  if ( textureId < 0 ||
    textureId >= static_cast( pModel->m_Textures.size() ) )
  {
    return false;
  }
  ....
}

Two more identical checks. Most likely, the pModel pointer check is not enough, since the pointers passed to the function are checked here.

V560 A part of conditional expression is always true: 0xFFFF. engine-dedicated% engine-headless% engine-legacy% unitsync cpuid.cpp 144

void CpuId::getMasksIntelLeaf11Enumerate()
{
  ....
  if ((ebx && 0xFFFF) == 0)        //<==
    return;
  if (((ecx >> 8) & 0xFF) == 1) {
    LOG_L(L_DEBUG,"[CpuId] SMT level found");
    shiftCore = eax & 0xf;
  } else {
    LOG_L(L_DEBUG,"[CpuId] No SMT level supported");
  }
  ....
}

Instead of the '&&' operator, the '&' operator should have been used.

V530 The return value of function 'size' is required to be utilized. assimp b3dimporter.cpp 536

void B3DImporter::ReadBB3D( aiScene *scene ){
  _textures.clear();
  _materials.size();     //<==
  _vertices.clear();
  _meshes.clear();
  ....
}

Calling the size () function without using the return value clearly does not make sense. Most likely, here, as elsewhere, you need to call the clear () function.

V592 The expression was enclosed by parentheses twice: ((expression)). One pair of parentheses is unnecessary or misprint is present. engineSim weapon.cpp 597

bool CWeapon::AttackUnit(CUnit* newTargetUnit, bool isUserTarget)
{
  if ((!isUserTarget && weaponDef->noAutoTarget)) {
    return false;
  }
  ....
}

The entire conditional expression is taken in double brackets. Perhaps the negation operator should just apply to the whole expression, and not just to the isUserTarget variable. For example, like this:

if (!(isUserTarget && weaponDef->noAutoTarget)) {
  return false;
}

V666 Consider inspecting third argument of the function 'TokenMatch'. It is possible that the value does not correspond with the length of a string which was passed with the second argument. assimp plyparser.cpp 185

PLY::ESemantic PLY::Property::ParseSemantic(....)
{
  ....
  else if (TokenMatch(pCur,"specular_alpha",14))
  {
    eOut = PLY::EST_SpecularAlpha;
  }
  else if (TokenMatch(pCur,"opacity",7))
  {
    eOut = PLY::EST_Opacity;
  }
  else if (TokenMatch(pCur,"specular_power",6))
  {
    eOut = PLY::EST_PhongPower;
  }
  ....
}

The string and its length are passed to the 'TokenMatch' function, which obviously does not match in one place.

Two more such places:

• V666 Consider inspecting third argument of the function 'TokenMatch'. It is possible that the value does not correspond with the length of a string which was passed with the second argument. assimp aseparser.cpp 1561
• V666 Consider inspecting third argument of the function 'TokenMatch'. It is possible that the value does not correspond with the length of a string which was passed with the second argument. assimp aseparser.cpp 1527

Copy paste

I have separated the following suspicious places from simple typos that occur when typing. Further it is clearly visible how the “successfully” duplicated code was fixed.

V519 The 'pTexture-> achFormatHint [2]' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 663, 664. assimp q3bspfileimporter.cpp 664

bool Q3BSPFileImporter::importTextureFromArchive(....)
{
  ....
  pTexture->achFormatHint[ 0 ] = ext[ 0 ];
  pTexture->achFormatHint[ 1 ] = ext[ 1 ];
  pTexture->achFormatHint[ 2 ] = ext[ 2 ];
  pTexture->achFormatHint[ 2 ] = '\0';
  ....
}

The last significant character was accidentally reset to zero. There is even a separate article about such places: The effect of the last line .

V583 The '?:' Operator, regardless of its conditional expression, always returns one and the same value: player.cpuUsage. engine-dedicated% engine-headless% engine-legacy gameserver.cpp 902

void CGameServer::LagProtection()
{
  ....
  const float playerCpuUsage =
    player.isLocal ? player.cpuUsage : player.cpuUsage; //<==
  ....
}

I think conditional constructs are not used if there is no choice. We can assume that they forgot to correct one variable here.

V524 It is odd that the body of '-' function is fully equivalent to the body of '+' function. assimp% engine-headless% engine-legacy types.h 183

/** Component-wise addition */
aiColor3D operator+(const aiColor3D& c) const {
  return aiColor3D(r+c.r,g+c.g,b+c.b);
}
/** Component-wise subtraction */
aiColor3D operator-(const aiColor3D& c) const {
  return aiColor3D(r+c.r,g+c.g,b+c.b);
}

The functions of addition and subtraction are suspiciously equally implemented. Most likely, for subtraction they forgot to correct the sign.

V524 It is odd that the body of '>' function is fully equivalent to the body of '<' function. assimp 3dshelper.h 470

bool operator < (const aiFloatKey& o) const
  {return mTime < o.mTime;}
bool operator > (const aiFloatKey& o) const
  {return mTime < o.mTime;}

Opposite in meaning comparison operators look even more suspicious when implemented in the same way.

Formatting

This section will talk about suspicious places related to code formatting. Whether the real errors here are more visible to the authors, but the programming style is clearly not the best.

V628 It's possible that the line was commented out improperly, thus altering the program's operation logics. assimp colladaparser.cpp 2281

void ColladaParser::ReadSceneLibrary()
{
  ....
  else if( mReader->getNodeType() == irr::io::EXN_ELEMENT_END)
  {
    if( strcmp( mReader->getNodeName(), "....") == 0)
      //ThrowException( "Expected end of \"....\" element.");
    break;
  }
  ....
}

Previously, 'break' was always called in this place, now exit from the loop is performed only by condition. Perhaps the condition itself should be commented out.

V640 The code's operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. sound oggstream.cpp 256

bool COggStream::UpdateBuffers()
{
  ....
  active = DecodeStream(buffer);
  if (active)
    alSourceQueueBuffers(source, 1, &buffer); CheckError("....");
  ....
}

The CheckError () function is not part of the condition, but is written as if it were.

V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. streflop s_atanf.cpp 90

Simple __atanf(Simple x)
{
  ....
  ix = hx&0x7fffffff;
  if(ix>=0x50800000) {  /* if |x| >= 2^34 */
      if(ix>0x7f800000)
    return x+x;    /* NaN */
      if(hx>0) return  atanhi[3]+atanlo[3];
      else     return -atanhi[3]-atanlo[3];
  } if (ix < 0x3ee00000) {  /* |x| < 0.4375f */            //<==
      if (ix < 0x31000000) {  /* |x| < 2^-29 */
    if(huge+x>one) return x;  /* raise inexact */
      }
      id = -1;
  } else {
    ....
  }
  ....
}

The if statement is located on the same line as the closing bracket from the previous if. Perhaps the 'else' keyword is missing at this point, and the program does not work as the programmer expected.

V640 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. AAI aaibrain.cpp 1138

void AAIBrain::BuildUnitOfMovementType(....)
{
  ....
  if(ai->Getbt()->units_static[unit].cost < ....)
  {
    if(ai->Getexecute()->AddUnitToBuildqueue(unit, 3, urgent))
    {
      ai->Getbt()->units_dynamic[unit].requested += 3;
      ai->Getut()->UnitRequested(....);
    }
  }
  else if(ai->Getbt()->units_static[unit].cost < ....)
  {
    if(ai->Getexecute()->AddUnitToBuildqueue(unit, 2, urgent))
      ai->Getbt()->units_dynamic[unit].requested += 2;
      ai->Getut()->UnitRequested(....);
  }
  else
  {
    if(ai->Getexecute()->AddUnitToBuildqueue(unit, 1, urgent))
      ai->Getbt()->units_dynamic[unit].requested += 1;
      ai->Getut()->UnitRequested(....);
  }
  ....
}

Here at once in two places the operators in the condition are shifted. Perhaps this would not have looked so suspicious if there had not been a similar condition above with the curly braces placed correctly.

Pointers

V571 Recurring check. The 'if (0 == MatFilePtr)' condition was already verified in line 140. assimp ogrematerial.cpp 143

aiMaterial* OgreImporter::LoadMaterial(const std::string MaterialName) const
{
  ....
  MatFilePtr=m_CurrentIOHandler->Open(MaterialFileName);
  if(NULL==MatFilePtr)
  {
    //try the default mat Library
    if(NULL==MatFilePtr)
    {
      MatFilePtr=m_CurrentIOHandler->Open(m_MaterialLibFilename);
      ....
    }
  }
  ....
}

Repeated checks are not an error, but there are many places in the project where there are really not enough checks.

V595 The 'model-> GetRootPiece ()' pointer was utilized before it was verified against nullptr. Check lines: 236, 238. engine-headless% engine-legacy imodelparser.cpp 236

S3DModel* C3DModelLoader::Load3DModel(std::string modelName)
{
  ....
  model->GetRootPiece()->SetCollisionVolume(                //<==
    new CollisionVolume("box", -UpVector, ZeroVector));
  if (model->GetRootPiece() != NULL) {                      //<==
    CreateLists(model->GetRootPiece());
  }
  ....
}

For example, in this place it would be worth checking the correctness of the pointer before dereferencing it.

Similar places:

• V595 The 'szComment' pointer was utilized before it was verified against nullptr. Check lines: 1559, 1564. assimp unzip.c 1559
• V595 The 'facCAI' pointer was utilized before it was verified against nullptr. Check lines: 1059, 1064. engineSim commandai.cpp 1059
• V595 The 'projectileDrawer' pointer was utilized before it was verified against nullptr. Check lines: 170, 176. engineSim shieldprojectile.cpp 170
• V595 The 'szComment' pointer was utilized before it was verified against nullptr. Check lines: 2068, 2073. minizip unzip.c 2068

V576 Incorrect format. Consider checking the fifth actual argument of the 'sprintf' function. To print the value of pointer the '% p' ​​should be used. engine-dedicated% engine-headless% engine-legacy seh.cpp 45

void __cdecl
se_translator_function(unsigned int err,
                       struct _EXCEPTION_POINTERS* ep)
{
  char buf[128];
  sprintf(buf,"%s(0x%08x) at 0x%08x",ExceptionName(err), //<==
    errep->ExceptionRecord->ExceptionAddress);           //<==
  CrashHandler::ExceptionHandler(ep);
  throw std::exception(buf);
}

To print the pointer, you must use the% p specifier. The current code will work correctly as long as the size of the pointer matches the size of 'int'.

V643 Unusual pointer arithmetic: ".." + io-> getOsSeparator (). The value of the 'char' type is being added to the string pointer. assimp lwsloader.cpp 467

std::string LWSImporter::FindLWOFile(const std::string& in)
{
  ....
  std::string test = ".." + io->getOsSeparator() + tmp;   //<==
  if (io->Exists(test))
    return test;
  test = ".." + io->getOsSeparator() + test;              //<==
  if (io->Exists(test)) {
    return test;
  }
  ....
}

It was expected that the string ".. \ tmp" would be received, however, in this case, an integer value will be added to the pointer to the string "..". This is guaranteed to lead to the exit of the string literal. To prevent such a situation, similar arithmetic operations with string and character variables should be avoided.

The correct version of the code:

std::string test = std::string("..") + io->getOsSeparator() + tmp;

Memory operations

V512 A call of the 'memset' function will lead to underflow of the buffer 'area'. RAI gterrainmap.h 84

#define MAP_AREA_LIST_SIZE 50
struct TerrainMapMobileType
{
  TerrainMapMobileType()
  {
    ....
    memset(area,0,MAP_AREA_LIST_SIZE);       //<==
  };
  TerrainMapArea *area[MAP_AREA_LIST_SIZE];  //<==
  ....
};

Incomplete zeroing of memory. An array of 50 pointers is declared, and 50 bytes are reset. The size of the array is 50 * sizeof (pointer) bytes.

Similar places:

• V512 A call of the 'memset' function will lead to underflow of the buffer 'BQ'. RAI builder.cpp 67
• V512 A call of the 'memset' function will lead to underflow of the buffer 'SL'. RAI unitmanager.cpp 28
• V512 A call of the 'memset' function will lead to underflow of the buffer 'Group'. RAI unitmanager.cpp 29
• V512 A call of the 'memset' function will lead to underflow of the buffer 'eventList'. RAI rai.cpp 77

V701 realloc () possible leak: when realloc () fails in allocating memory, original pointer 'dest' is lost. Consider assigning realloc () to a temporary pointer. assimp blenderloader.cpp 217

void BlenderImporter::InternReadFile( const std::string& pFile, 
  aiScene* pScene, IOSystem* pIOHandler)
{
  ....
  dest = reinterpret_cast( realloc(dest,total) );
  memcpy(dest + total - have,block,have);
  ....
}

If it is not possible to change the size of the memory block, the realloc () function will return a null pointer, and the pointer to the old memory area will be lost. It is necessary to save the pointer to the buffer variable and do the corresponding checks.

Similar place:

• V701 realloc () possible leak: when realloc () fails in allocating memory, original pointer 'dest' is lost. Consider assigning realloc () to a temporary pointer. assimp xglloader.cpp 181

Undefined behavior

V610 Undefined behavior. Check the shift operator '<<. The left operand '(- 1)' is negative. engine-dedicated% engine-headless% engine-legacy% unitsync cpuid.cpp 176

void CpuId::getMasksIntelLeaf11()
{
  getMasksIntelLeaf11Enumerate();
  // We determined the shifts now compute the masks
  maskVirtual = ~((-1) << shiftCore);
  maskCore = (~((-1) << shiftPackage)) ^ maskVirtual;
  maskPackage = (-1) << shiftPackage;
}

According to the C ++ 11 language standard, shifting a negative number leads to undefined behavior.

Conclusion

I hope that improving the quality of this project will lead to an improvement in all products related to it. This is a good project for beginner game developers and just gamers, fans of the genre.

Using static analysis regularly, you can save a lot of time in solving more useful tasks.

This article is in English.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Spring RTS Engine Checkup .