# Spring RTS Engine Checkup

Spring RTS is a game engine for real\-time strategy \(RTS\) video games\. Spring was originally created to reproduce the Total Annihilation game popular in the 90\\00\-s\. During the later years, a lot of other nice and interesting strategy games, including commercial ones, were developed based on this engine\. Spring RTS based games are cross\-platform 3D real\-time strategies with huge maps and numbers of combat and building units\. However, they face certain stability issues\. Let's take a look at the source codes \(thanks god, this project is open\-source\)\.

![0293_SpringRTS/image1.png](https://import.viva64.com/docx/blog/0293_SpringRTS/image1.png)

[Official site](https://springrts.com/)\.

[Source code](https://github.com/spring/spring)\.

Being an open\-source project, Spring RTS includes a number of open\-source third\-party libraries that may also contain bugs which ultimately become part of the engine or games\. Some diagnostic messages cited in this article are related to the libraries that come with the engine\. Especially many warnings were triggered by Assimp \(Open Asset Import Library\)\.

Code analysis was done with the [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/) tool\. The article covers far not all of the bugs the analyzer has found in the code\. That's why you shouldn't treat it as a guide on bug fixing\. For analysis to be much more efficient, the developers should check the project themselves\.

## Typos

[V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions 'aha\-\>mNumWeights \!\= oha\-\>mNumWeights' to the left and to the right of the '\|\|' operator\. assimp findinstancesprocess\.cpp 87

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

There are two identical conditional expressions\. In one of them, the 'mName' or 'mWeights' field of the aiBone structure should probably be compared\.

[V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions to the left and to the right of the '\|\|' operator: 0 \=\= pArchive \|\| 0 \=\= pArchive assimp q3bspfileimporter\.cpp 631

```cpp
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<int>( pModel->m_Textures.size() ) )
  {
    return false;
  }
  ....
}
```

Two more identical checks\. A check for the 'pModel' pointer is most likely missing as it is pointers passed into the function that are checked in this fragment\.

[V560](https://pvs-studio.com/en/docs/warnings/v560/) A part of conditional expression is always true: 0xFFFF\. engine\-dedicated%engine\-headless%engine\-legacy%unitsync cpuid\.cpp 144

```cpp
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");
  }
  ....
}
```

The '&' operator should be used instead of '&&'\.

[V530](https://pvs-studio.com/en/docs/warnings/v530/) The return value of function 'size' is required to be utilized\. assimp b3dimporter\.cpp 536

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

Calling the size\(\) function without using its return value doesn't make any sense\. Most likely, it is necessary to call the clear\(\) function here, like in the other lines\.

[V592](https://pvs-studio.com/en/docs/warnings/v592/) The expression was enclosed by parentheses twice: \(\(expression\)\)\. One pair of parentheses is unnecessary or misprint is present\. engineSim weapon\.cpp 597

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

The entire conditional expression is embraced in double parentheses\. But it is probably the whole expression that the complementary operator should actually be applied to, not just the 'isUserTarget' variable\. For example:

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

[V666](https://pvs-studio.com/en/docs/warnings/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

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

A string and its length, which is obviously different in one place, are passed into the 'TokenMatch' function\.

Other two 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

Apart from plain typos that occur when typing text, I singled out certain suspicious fragments cited below\. The following examples show "successfully" edited code written through the copy\-paste technique\.

[V519](https://pvs-studio.com/en/docs/warnings/v519/) The 'pTexture\-\>achFormatHint\[2\]' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 663, 664\. assimp q3bspfileimporter\.cpp 664

```cpp
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 zeroed\. We even have a special article on such bugs: [The Last Line Effect](https://pvs-studio.com/en/blog/posts/cpp/0260/)\.

[V583](https://pvs-studio.com/en/docs/warnings/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

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

I don't think anyone uses conditional constructs when there is no choice\. Looks like the programmer forgot to fix one variable here\.

[V524](https://pvs-studio.com/en/docs/warnings/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

```cpp
/** 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);
}
```

Addition and subtraction functions are implemented in a suspiciously similar fashion\. It must be that the programmer forgot to change the sign in the subtraction function\.

[V524](https://pvs-studio.com/en/docs/warnings/v524/) It is odd that the body of '\>' function is fully equivalent to the body of '<' function\. assimp 3dshelper\.h 470

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

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

Comparison operators opposed in their meaning look even stranger when implemented in the same way\.

## Formatting

In this section, we are going to discuss suspicious fragments related to code formatting\. If the issues described here are genuine errors or not is up to the authors to decide, but the programming style in these fragments is obviously far from perfect\.

[V628](https://pvs-studio.com/en/docs/warnings/v628/) It's possible that the line was commented out improperly, thus altering the program's operation logics\. assimp colladaparser\.cpp 2281

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

    break;
  }
  ....
}
```

It was 'break' that used to be called all the time in this code originally, but now the loop is terminated only by condition\. Perhaps the condition itself should have been commented out too\.

[V640](https://pvs-studio.com/en/docs/warnings/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

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

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

[V646](https://pvs-studio.com/en/docs/warnings/v646/) Consider inspecting the application's logic\. It's possible that 'else' keyword is missing\. streflop s\_atanf\.cpp 90

```cpp
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 operator is in the same line as the closing brace of the previous if\. There may be the keyword 'else' missing in this place and then the program works quite differently than the programmer expected\.

[V640](https://pvs-studio.com/en/docs/warnings/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

```cpp
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(....);
  }
  ....
}
```

Two operators in conditions are shifted here at once\. It wouldn't look that strange but for another similar condition with correctly arranged braces earlier in the code\.

## Pointers

[V571](https://pvs-studio.com/en/docs/warnings/v571/) Recurring check\. The 'if \(0 \=\= MatFilePtr\)' condition was already verified in line 140\. assimp ogrematerial\.cpp 143

```cpp
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);
      ....
    }
  }
  ....
}
```

Repeating checks are not errors, but there are a lot of fragments in the project where checks are really missing\.

[V595](https://pvs-studio.com/en/docs/warnings/v595/) The 'model\-\>GetRootPiece\(\)' pointer was utilized before it was verified against nullptr\. Check lines: 236, 238\. engine\-headless%engine\-legacy imodelparser\.cpp 236

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

  if (model->GetRootPiece() != NULL) {                      // <=
    CreateLists(model->GetRootPiece());
  }
  ....
}
```

In this code fragment, for instance, the programmer should have checked the pointer before dereferencing it\.

Other similar fragments:

* 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](https://pvs-studio.com/en/docs/warnings/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

```cpp
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 a pointer, the %p specifier should be used\. The current code will work correctly as long as the pointer size coincides with that of the 'int' type\.

[V643](https://pvs-studio.com/en/docs/warnings/v643/) Unusual pointer arithmetic: "\.\." \+ io\-\>getOsSeparator\(\)\. The value of the 'char' type is being added to the string pointer\. assimp lwsloader\.cpp 467

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

The programmer expected that the "\.\.\\tmp" string would be received, but in this case an integer value will be added to the pointer to the "\.\." string instead\. It will surely cause a string literal overflow\. To prevent issues like that, one should avoid using such arithmetic operations over string and character variables\.

The correct code:

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

## Memory handling

[V512](https://pvs-studio.com/en/docs/warnings/v512/) A call of the 'memset' function will lead to underflow of the buffer 'area'\. RAI gterrainmap\.h 84

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

Incomplete memory zeroing\. An array of 50 pointers is declared but only 50 bytes are zeroed, the size of the array being 50\*sizeof\(pointer\) bytes\.

Other similar issues:

* 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](https://pvs-studio.com/en/docs/warnings/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

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

If the size of a memory block can't be changed, the realloc\(\) function will return a null pointer, while the pointer to the previous memory area will be lost\. It is necessary to save the pointer into a buffer variable and make corresponding checks\.

Another issue of that kind:

* 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](https://pvs-studio.com/en/docs/warnings/v610/) Undefined behavior\. Check the shift operator '<<\. The left operand '\(\- 1\)' is negative\. engine\-dedicated%engine\-headless%engine\-legacy%unitsync cpuid\.cpp 176

```cpp
void CpuId::getMasksIntelLeaf11()
{
  getMasksIntelLeaf11Enumerate();

  // We determined the shifts now compute the masks
  maskVirtual = ~((-1) << shiftCore);
  maskCore = (~((-1) << shiftPackage)) ^ maskVirtual;
  maskPackage = (-1) << shiftPackage;
}
```

Under the C\+\+11 language standard, shifting a negative number causes undefined behavior\.

## Conclusion

I hope improving this project's quality will also stimulate improvement of all the products based on it\. It's quite a nice project for beginner game developers and ordinary gamers, followers of the RTS genre\.

Using static analysis regularly will help you save plenty of time to solve more serious tasks\.