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).
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 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.
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;
}
....
}There are two identical conditional expressions. In one of them, the 'mName' or 'mWeights' field of the aiBone structure should probably be compared.
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<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 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");
}
....
}The '&' operator should be used instead of '&&'.
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 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 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 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:
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;
}
....
}A string and its length, which is obviously different in one place, are passed into the 'TokenMatch' function.
Other two places:
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 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 zeroed. We even have a special article on such bugs: The Last Line Effect.
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 don't think anyone uses conditional constructs when there is no choice. Looks like the programmer forgot to fix 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);
}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 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;}Comparison operators opposed in their meaning look even stranger when implemented in the same way.
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 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;
}
....
}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 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 although it 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 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 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(....);
}
....
}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.
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);
....
}
}
....
}Repeating checks are not errors, but there are a lot of fragments in the project where checks are really missing.
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());
}
....
}In this code fragment, for instance, the programmer should have checked the pointer before dereferencing it.
Other similar fragments:
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 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 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;
}
....
}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:
std::string test = std::string("..") + io->getOsSeparator() + tmp;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 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:
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<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:
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;
}Under the C++11 language standard, shifting a negative number causes undefined behavior.
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.