Checkup engine Spring RTS

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

Official site .

Source code

As an open source project, Spring RTS includes a number of open libraries that may also contain bugs, and ultimately they become part of the engine / games. Some messages will be from the libraries that come with the engine. Especially many warnings were in Assimp (Open Asset Import Library).
')
The verification of the code 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 themselves.

Typos

V501 There are identical sub-expressions "aha-> mNumWeights! = Oha-> mNumWeights" 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. It is possible that in one of the cases it is necessary to compare the 'mName' or 'mWeights' field of the aiBone structure.

V501 There are identical to the left and 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. Most likely, there is not enough verification of the 'pModel' pointer, since the pointers passed to the function are checked here.

V560 A part of the 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 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 is clearly meaningless. Most likely, here, as elsewhere, you need to call the function clear ().

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

All conditional expression is in double brackets. Perhaps the negation operator should apply to the entire expression, and not just to the 'isUserTarget' variable. For example:

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

V666 Consider inspecting the third argument of the function 'TokenMatch'. This is not the case. 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 is passed to the 'TokenMatch' function and its length, which in one place is clearly not the same.

Two more such places:

• V666 Consider inspecting the third argument of the function 'TokenMatch'. This is not the case. assimp aseparser.cpp 1561
• V666 Consider inspecting the third argument of the function 'TokenMatch'. This is not the case. assimp aseparser.cpp 1527

Copy paste

The following suspicious places I have separated from the simple typos that arise when typing. Further, it is clearly seen how the duplicated code was “successfully” corrected.

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 nullified. There is even a separate article about such places: 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 think that conditional constructions are not used if there is no choice. It can be assumed that here one variable was forgotten to be corrected.

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+cr,g+cg,b+cb); } /** Component-wise subtraction */ aiColor3D operator-(const aiColor3D& c) const { return aiColor3D(r+cr,g+cg,b+cb); } 

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

V524 It is odd that the body of '>' function is fully equivalent. 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 comparison operators are even more suspicious when implemented in the same way.

Formatting

In this section, we will talk about suspicious places related to code formatting. Are the real errors here - more visible to the authors, but the programming style is clearly not the best.

V628 It was possible that the line was commented out, 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, this place was always called 'break', now the exit from the cycle is performed only by condition. Perhaps it should comment out the condition itself.

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 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 statement is located on the same line as the closing bracket of the previous if. Probably, in this place the 'else' keyword is missing, 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. 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 are shifted in the condition. Perhaps it would not look so suspicious if there were no similar condition with properly placed braces above.

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

Duplicate checks are not a mistake, but there are many places in the project where there are really not enough checks.

V595 The 'model-> GetRootPiece ()' pointer was used 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 its dereference.

Similar places:

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

V576 Incorrect format. Consider checking the sprintf function. To print the value of 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 pointer is the same size as the '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, but in this case an integer value would be added to the pointer to the string "..". This is guaranteed to lead the string literal out of bounds. To prevent such a situation, such arithmetic operations with string and character variables should be avoided.

The correct code is:

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

Memory operations

V512 A call-out 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 reset. 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 the underflow of the buffer BQ. RAI builder.cpp 67
• V512 A call of the memset function will lead to the underflow of the buffer SL. RAI unitmanager.cpp 28
• V512 A call of the memset function will lead to the underflow of the buffer group. RAI unitmanager.cpp 29
• V512 A call of the memset function will lead to the event flow of event buffer. RAI rai.cpp 77

V701 realloc () possible leak: when realloc () fails in allocating memory, the 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); .... } 

In case the size of the memory block cannot be changed, the realloc () function will return a null pointer, and the pointer to the old memory area will be lost. You must save the pointer to the buffer variable and do the appropriate checks.

Similar place:

• V701 realloc () possible leak: when realloc () fails in allocating memory, the 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 standard, a negative number shift leads to undefined behavior.

Conclusion

I hope the improvement of the quality of this project will lead to the improvement of all related products. This is a good project for novice game developers and gamers, fans of the genre.

Using static analysis regularly, you can save a lot of time on 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 .