Geekly Articles each Day
📜 ⬆️ ⬇️

Serious errors in the code CryEngine V


In May 2016, the German company Crytek decided to publish the source code for the CryEngine V game engine on GitHub. The project is under active development, which entails the appearance of many errors in the code. We have already checked the project with PVS-Studio for Windows, and now we were able to test the project with PVS-Studio for Linux. The material again scored on a large article describing only very serious errors.

Introduction


CryEngine is a game engine created by the German company Crytek in 2002, and originally used in the first-person shooter Far Cry. On CryEngine of different versions, many excellent games were made from several game studios that licensed the engine: Far Cry, Crysis, Entropia Universe, Blue Mars, Warface, Homefront: The Revolution, Sniper: Ghost Warrior, Armored Warfare, Evolve and many others . In March 2016, Crytek announced the release of its new engine, the CryEngine V, and soon published the source code on GitHub .

To check the open source code, we used the PVS-Studio static analyzer version 6.14 for Linux. Now, developers of cross-platform projects have become even more convenient to monitor the quality of the code with a single code analysis tool. Download version for Linux can be in the form of an archive or package for the package manager. For most distributions, you can customize the installation and upgrade using our repository.

The article includes general-purpose warnings and only the “High” confidence level (there is also Medium and Low). Honestly, I did not master to watch carefully all the “High” warnings, because almost immediately picked up errors for the article with a quick review. I took up the project several times and for a long time could not find the time to write a review article, so I can say about the given bugs that they have been living in the code for more than one month. Also, some errors were not corrected from the previous article on the verification of the project.
')
Downloading and testing source code in Linux was very easy. Here is a list of all the necessary commands:

mkdir ~/projects && cd ~/projects git clone https://github.com/CRYTEK/CRYENGINE.git cd CRYENGINE/ git checkout main chmod +x ./download_sdks.py ./download_sdks.py pvs-studio-analyzer trace -- \ sh ./cry_waf.sh build_linux_x64_clang_profile -p gamesdk pvs-studio-analyzer analyze \ -l /path/to/PVS-Studio.lic \ -o ~/projects/CRYENGINE/cryengine.log \ -r ~/projects/CRYENGINE/ \ -C clang++-3.8 -C clang-3.8 \ -e ~/projects/CRYENGINE/Code/SDKs \ -j4 plog-converter -a GA:1,2 -t tasklist \ -o ~/projects/CRYENGINE/cryengine_ga.tasks \ ~/projects/CRYENGINE/cryengine.log

The cryengine_ga.tasks report file can be opened and viewed in QtCreator. What did you find in the source code of CryEngine V?

Unhappy Active () Function


V501 operator: bActive == bActive LightEntity.h 124

 void SetActive(bool bActive) { if (bActive == bActive) return; m_bActive = bActive; OnResetState(); }

Because of the typo, the function does nothing. It seems to me that if there was a Miss Misprint contest, then this piece of code would definitely take first place. I think this error has every chance of getting into the " C / C ++ bugs of the month " section.

But this is not all, here is a function from another class:

V501 There are identical sub-expressions 'm_staticObjects' operator. FeatureCollision.h 66

 class CFeatureCollision : public CParticleFeature { public: CRY_PFX2_DECLARE_FEATURE public: CFeatureCollision(); .... bool IsActive() const { return m_terrain || m_staticObjects || m_staticObjects; } .... bool m_terrain; bool m_staticObjects; bool m_dynamicObjects; };

Here, in the IsActive () function, the m_staticObjects variable is used twice , although there is an unused variable m_dynamicObjects next to it . Perhaps in the code they wanted to use it.

Code above has no bugs


V547 Expression 'outArrIndices [i] <0' is always false. Unsigned type value is never <0. CGFLoader.cpp 881

 static bool CompactBoneVertices(...., DynArray<uint16>& outArrIndices, ....) // <= uint16 { .... outArrIndices.resize(3 * inFaceCount, -1); int outVertexCount = 0; for (int i = 0; i < verts.size(); ++i) { .... outArrIndices[....] = outVertexCount - 1; } // Making sure that the code above has no bugs // <= LOL for (int i = 0; i < outArrIndices.size(); ++i) { if (outArrIndices[i] < 0) // <= LOL { return false; } } return true; }

This error is worth a separate section. In general, in the CryEngine code, there are a lot of places where unsigned variables are meaninglessly compared to zero. There are hundreds of such places, but I would like to pay special attention to this fragment, since The code was written intentionally.

So, there is an array of unsigned numbers - outArrIndices . Next, the array is filled by some algorithm. After that, a brilliant check of each element of the array is performed, so that among them there are no negative numbers. The array elements are of type uint16 .

Errors when working with memory


V512 A call of the memcpy function will lead to hashableData. GeomCacheRenderNode.cpp 285

 void CGeomCacheRenderNode::Render(....) { .... CREGeomCache* pCREGeomCache = iter->second.m_pRenderElement; .... uint8 hashableData[] = { 0, 0, 0, 0, 0, 0, 0, 0, (uint8)std::distance(pCREGeomCache->....->begin(), &meshData), (uint8)std::distance(meshData....->....begin(), &chunk), (uint8)std::distance(meshData.m_instances.begin(), &instance) }; memcpy(hashableData, pCREGeomCache, sizeof(pCREGeomCache)); .... }

Pay attention to the memcpy () function arguments. The pCREGeomCache object is planned to be copied into the hashableData array, but by chance the size of the pointer was not calculated using the sizeof operator, but the size of the object. Due to an error, the object is not completely copied, but only 4 or 8 bytes.

If you’re a sizeof () ’ ClipVolumeManager.cpp 145

 void CClipVolumeManager::GetMemoryUsage(class ICrySizer* pSizer) const { pSizer->AddObject(this, sizeof(this)); for (size_t i = 0; i < m_ClipVolumes.size(); ++i) pSizer->AddObject(m_ClipVolumes[i].m_pVolume); }

A similar error was made by calculating instead of the class size, the size of the pointer this . The correct option is sizeof (* this) .

V530 ClipVolumes.cpp 492

 vector<unique_ptr<CFullscreenPass>> m_jitteredDepthPassArray; void CClipVolumesStage::PrepareVolumetricFog() { .... for (int32 i = 0; i < m_jitteredDepthPassArray.size(); ++i) { m_jitteredDepthPassArray[i].release(); } m_jitteredDepthPassArray.resize(depth); for (int32 i = 0; i < depth; ++i) { m_jitteredDepthPassArray[i] = CryMakeUnique<....>(); m_jitteredDepthPassArray[i]->SetViewport(viewport); m_jitteredDepthPassArray[i]->SetFlags(....); } .... }

If you refer to the documentation for the class std :: unique_ptr , then the release () function should be used approximately as follows:

 std::unique_ptr<Foo> up(new Foo()); Foo* fp = up.release(); delete fp;

Most likely, in this code snippet, we wanted to use the reset () function instead of release () .

V549 memcpy function is equal to the second argument. ObjectsTree_Serialize.cpp 1135

 void COctreeNode::LoadSingleObject(....) { .... float* pAuxDataDst = pObj->GetAuxSerializationDataPtr(....); const float* pAuxDataSrc = StepData<float>(....); memcpy(pAuxDataDst, pAuxDataDst, min(....) * sizeof(float)); .... }

Forgot to pass pAuxDataSrc to the memcpy () function. Instead, the same pAuxDataDst variable is used as the source and destination . No one is immune from typos. By the way, those who wish can check their attentiveness by passing our test to identify such errors: q.viva64.com .

Strange code


V501 There are identical to the left and the right of the | || operator: val == 0 || val == - 0 XMLCPB_AttrWriter.cpp 363

 void CAttrWriter::PackFloatInSemiConstType(float val, ....) { uint32 type = PFSC_VAL; if (val == 0 || val == -0) // <= type = PFSC_0; else if (val == 1) type = PFSC_1; else if (val == -1) type = PFSC_N1; .... }

The developers planned to compare the real variable val with zero and negative zero (signed zero / negative zero), but they did it wrong. By declaring integer constants, the values ​​of the zeros are the same.

Most likely, the code should be corrected as follows, declaring constants of real type:

 if (val == 0.0f || val == -0.0f) type = PFSC_0;

On the other hand, the conditional expression is redundant, since it is enough to compare a variable with a normal zero. For this reason, the original code is executed as the programmer expects.

But if it is necessary to identify exactly negative zero, then correctly do it with the help of the function std :: signbit .

V501 There are identical sub-expressions' m_joints [i] .limits [1] [j] articulatedentity.cpp 1326

 int CArticulatedEntity::Step(float time_interval) { .... for (j=0;j<3;j++) if (!(m_joints[i].flags & angle0_locked<<j)&& isneg(m_joints[i].limits[0][j]-m_joints[i].qext[j]) + isneg(m_joints[i].qext[j]-m_joints[i].limits[1][j]) + isneg(m_joints[i].limits[1][j]-m_joints[i].limits[1][j]) < 2) { .... }

The last part of the conditional expression contains subtraction from the m_joints [i] .limits [1] [j] variable. The code looks suspicious. There are many indexes in the expression, perhaps one of them has an error.

Another such place:


V590 Consider inspecting this expression. The expression is misprint. GoalOp_Crycd2.cpp 3779

 void COPCrysis2FlightFireWeapons::ParseParam(....) { .... bool paused; value.GetValue(paused); if (paused && (m_State != eFP_PAUSED) && (m_State != eFP_PAUSED_OVERRIDE)) { m_NextState = m_State; m_State = eFP_PAUSED; m_PausedTime = 0.0f; m_PauseOverrideTime = 0.0f; } else if (!paused && (m_State == eFP_PAUSED) && // <= (m_State != eFP_PAUSED_OVERRIDE)) // <= { m_State = m_NextState; m_NextState = eFP_STOP; m_PausedTime = 0.0f; m_PauseOverrideTime = 0.0f; } .... }

The conditional expression is written in such a way that the result does not depend on the m_State subexpression ! = EFP_PAUSED_OVERRIDE . Although I’m telling someone, the developers haven’t fixed this code snippet after the first article.

If anyone is interested, then I described this kind of errors earlier in a separate article " Logical expressions in C / C ++. How professionals are mistaken ."

V529 Odd semicolon ';' after 'for' operator. boolean3d.cpp 1077

 int CTriMesh::Slice(...) { .... pmd->pMesh[0]=pmd->pMesh[1] = this; AddRef();AddRef(); for(pmd0=m_pMeshUpdate; pmd0->next; pmd0=pmd0->next); // <= pmd0->next = pmd; .... }

Another piece of code, uncorrected since the previous check. The code is still not clear: is there an erroneous formatting or a logical error?

About pointers


V522 Dereferencing of the null pointer 'pCEntity' might take place. BreakableManager.cpp 2396

 int CBreakableManager::HandlePhysics_UpdateMeshEvent(....) { CEntity* pCEntity = 0; .... if (pmu && pSrcStatObj && GetSurfaceType(pSrcStatObj)) { .... if (pEffect) { .... if (normal.len2() > 0) pEffect->Spawn(true, pCEntity->GetSlotWorldTM(...); // <= } } .... if (iForeignData == PHYS_FOREIGN_ID_ENTITY) { pCEntity = (CEntity*)pForeignData; if (!pCEntity || !pCEntity->GetPhysicalProxy()) return 1; } .... }

The analyzer detected null pointer dereferencing. The function code is written or refactored in such a way that a code branch is formed in which the pCEntity pointer initialized by zero will be used.

Now consider a potential null pointer dereference.

V595 The 'pTrack' pointer was used before it was counted against nullptr. Check lines: 60, 61. AudioNode.cpp 60

 void CAudioNode::Animate(SAnimContext& animContext) { .... const bool bMuted = gEnv->IsEditor() && (pTrack->GetFlags() & IAnimTrack::eAnimTrackFlags_Muted); if (!pTrack || pTrack->GetNumKeys() == 0 || pTrack->GetFlags() & IAnimTrack::eAnimTrackFlags_Disabled) { continue; } .... }

The author of this code fragment first used the pTrack pointer, and on the very next line of the code checks its validity before dereferencing. Most likely, this is a potential place for incorrect operation of the program.

There are a lot of warnings for the V595 , and they won't fit in the article. Very often such code is a real mistake, but thanks to luck, it works correctly.

V571 Recurring check. The 'if (rLightInfo.m_pDynTexture)' condition was already verified in line 69. ObjMan.cpp 70

 // Safe memory helpers #define SAFE_RELEASE(p){ if (p) { (p)->Release(); (p) = NULL; } } void CObjManager::UnloadVegetationModels(bool bDeleteAll) { .... SVegetationSpriteLightInfo& rLightInfo = ....; if (rLightInfo.m_pDynTexture) SAFE_RELEASE(rLightInfo.m_pDynTexture); .... }

There is no serious error in this code snippet, but you should not write an extra code if the corresponding checks are already included in the special macro.

Another place with extra code:


V575 The memcpy function doesn't copy the whole string. Use 'strcpy / strcpy_s' function to preserve terminal null. SystemInit.cpp 4045

 class CLvlRes_finalstep : public CLvlRes_base { .... for (;; ) { if (*p == '/' || *p == '\\' || *p == 0) { char cOldChar = *p; *p = 0; // create zero termination _finddata_t fd; bool bOk = FindFile(szFilePath, szFile, fd); if (bOk) assert(strlen(szFile) == strlen(fd.name)); *p = cOldChar; // get back the old separator if (!bOk) return; memcpy((void*)szFile, fd.name, strlen(fd.name)); // <= if (*p == 0) break; ++p; szFile = p; } else ++p; } .... }

This code may have an error. When copying a string, the last terminal zero is lost. In this case, you must copy strlen () + 1 character, or use special functions to copy strings: strcpy or strcpy_s .

Comma problems


V521 Such expressions using the ',' operator are dangerous. Make sure the expression! SWords [iWord] .empty (), iWord ++ 'is correct. TacticalPointSystem.cpp 3243

 bool CTacticalPointSystem::Parse(....) const { string sInput(sSpec); const int MAXWORDS = 8; string sWords[MAXWORDS]; int iC = 0, iWord = 0; for (; iWord < MAXWORDS; !sWords[iWord].empty(), iWord++) // <= { sWords[iWord] = sInput.Tokenize("_", iC); } .... }

Note the section of the for loop with counters. What does a logical expression do there? Most likely, it should be moved to another place and write like this:

 for (; iWord < MAXWORDS && !sWords[iWord].empty(); iWord++) {...}

V521 Such expressions using the ',' operator are dangerous. Make sure the expression is correct. HommingSwarmProjectile.cpp 187

 void CHommingSwarmProjectile::HandleEvent(....) { .... explodeDesc.normal = -pCollision->n,pCollision->vloc[0]; .... }

Another strange code snippet with the operator ','.

Suspicious conditions


V571 Recurring check. The 'if (pos == npos)' condition was already verified in line 1530. CryString.h 1539

 //! Find last single character. // \return -1 if not found, distance from beginning otherwise. template<class T> inline typename CryStringT<T>::....::rfind(....) const { const_str str; if (pos == npos) { // find last single character str = _strrchr(m_str, ch); // return -1 if not found, distance from beginning otherwise return (str == NULL) ? (size_type) - 1 : (size_type)(str - m_str); } else { if (pos == npos) { pos = length(); } if (pos > length()) { return npos; } value_type tmp = m_str[pos + 1]; m_str[pos + 1] = 0; str = _strrchr(m_str, ch); m_str[pos + 1] = tmp; } return (str == NULL) ? (size_type) - 1 : (size_type)(str - m_str); }

The analyzer detected a repeated check of the pos variable. Because of this error, part of the code is never executed. There is also duplicate code in the function, so the function should be rewritten better.

This code was successfully duplicated in another place:


V523 The 'then' statement is equivalent to the 'else' statement. ScriptTable.cpp 789

 bool CScriptTable::AddFunction(const SUserFunctionDesc& fd) { .... char sFuncSignature[256]; if (fd.sGlobalName[0] != 0) cry_sprintf(sFuncSignature, "%s.%s(%s)", fd.sGlobalName, fd.sFunctionName, fd.sFunctionParams); else cry_sprintf(sFuncSignature, "%s.%s(%s)", fd.sGlobalName, fd.sFunctionName, fd.sFunctionParams); .... }

Regardless of the contents of the line, it is still trying to print the same way. This code is also very much in the project, here are some warnings:


Undefined behavior


V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. physicalplaceholder.h 25

 class CPhysicalEntity; const int NO_GRID_REG = -1<<14; const int GRID_REG_PENDING = NO_GRID_REG+1; const int GRID_REG_LAST = NO_GRID_REG+2;

The analyzer is able to find several types of errors leading to undefined behavior. According to the modern standard of language, a shift of a negative number to the left leads to undefined behavior.

Here are some more questionable places:


Another type of indefinite behavior is associated with repeated changes in the variable between two points of repetition:

V567 Undefined behavior. The variable "m_current" variable OperatorQueue.cpp 101

 boolCOperatorQueue::Prepare(....) { ++m_current &= 1; m_ops[m_current].clear(); return true; }

Unfortunately, this place is not the only one in the code:


Questions to the developers


In the code of CryEngine V, I found an interesting way for developers to communicate through comments in the code.

Here is the funniest comment I found with the warning:

V763 Parameter 'enable' is always rewritten in function body before being used.

 void CNetContext::EnableBackgroundPassthrough(bool enable) { SCOPED_GLOBAL_LOCK; // THIS IS A TEMPORARY HACK TO MAKE THE GAME PLAY NICELY, // ASK peter@crytek WHY IT'S STILL HERE enable = false; .... }

Then I decided to search for similar texts with the help of searching through the contents of files and wrote out several places:

 .... // please ask me when you want to change [tetsuji] .... // please ask me when you want to change [dejan] .... //if there are problems with this function, ask Ivo uint32 numAnims = pCharacter->GetISkeletonAnim()->GetNumAnimsInFIFO(layer); if (numAnims) return pH->EndFunction(true); .... //ask Ivo for details //if (pCharacter->GetCurAnimation() && // pCharacter->GetCurAnimation()[0] != '\0') // return pH->EndFunction(pCharacter->GetCurAnimation()); .... ///////////////////////////////////////////////////////////////// // Strange, !do not remove... ask Timur for the meaning of this. ///////////////////////////////////////////////////////////////// if (m_nStrangeRatio > 32767) { gEnv->pScriptSystem->SetGCFrequency(-1); // lets get nasty. } ///////////////////////////////////////////////////////////////// // Strange, !do not remove... ask Timur for the meaning of this. ///////////////////////////////////////////////////////////////// if (m_nStrangeRatio > 1000) { if (m_pProcess && (m_pProcess->GetFlags() & PROC_3DENGINE)) m_nStrangeRatio += cry_random(1, 11); } ///////////////////////////////////////////////////////////////// .... // tank specific: // avoid steering input around 0.5 (ask Anton) .... CryWarning(VALIDATOR_MODULE_EDITOR, VALIDATOR_WARNING, "....: Wrong edited item. Ask AlexL to fix this."); .... // If this renders black ask McJohn what's wrong. glGenerateMipmap(GL_TEXTURE_2D); ....

Well and the most important question to developers: why they do not use specialized tools to improve the quality of the code? Of course, I mean PVS-Studio. :)

Once again I want to note that the article contains only a fraction of the errors. I have not watched the end of even High level warnings. So the project is still waiting for those who carefully check it. I, unfortunately, can not find so much time, dozens of other projects are waiting for me.

Conclusion


For many years of developing a static code analyzer, I came to the conclusion that in development teams with a certain number of people it is already impossible to write code without errors. No, I am not against Code Review, but calculate for yourself how long it will take a manager to review the code of 10 people? And the next day? And more people? In such circumstances, Code Review is required when only key product components are changed. In large volumes, this approach is extremely inefficient. Automated code checking with static analyzers will help to significantly improve the situation for the better. This is not a replacement for existing tests, but a completely different approach to code quality control (by the way, the tests also contain errors using static code analysis). Correction of errors at the time of writing the code is almost worthless, in contrast to those that are at the stage of the testers, and errors in the released product already incur enormous costs.

Download and try PVS-Studio at this link.

Discuss licensing options, prices and options for discounts you can write to us in support .

Picture 3



Do not grieve the unicorn with your code ...



If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Critical errors in CryEngine V code

Read the article and have a question?
Often our articles are asked the same questions. We collected answers to them here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please review the list.

Source: https://habr.com/ru/post/325600/

More articles:


All Articles