Geekly Articles each Day
📜 ⬆️ ⬇️

The long-awaited check CryEngine V

In May 2016, the German company Crytek decided to publish the source code for the CryEngine V game engine on Github. The game engine is written in C ++ and immediately attracted the attention of both the open-source developer community and the PVS-Studio static analyzer development team that checks the quality of the open code projects. Many different games from different game studios have been made on CryEngine of different versions, and now the engine has become available to even more developers. The article contains an overview of errors detected using a static code analyzer.

Introduction


CryEngine is a game engine created by the German company Crytek in 2002 and originally used in the first-person shooter Far Cry. At CryEngine of different versions, many excellent games were made from different 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, the PVS-Studio static analyzer version 6.05 was used. It is a tool for detecting errors in the source code of programs written in C, C ++ and C # languages. The only correct way to use the static analysis methodology is to regularly check the code on the developers' computers and the build server. But to demonstrate the capabilities of the PVS-Studio analyzer, we perform one-time checks of open-source projects and write review articles describing the errors found. However, if the project seemed interesting to us, after the coming of one or two years, we can check it again. In essence, such repeated checks are no different from one-time checks, since many changes accumulate in the code.

Both well-known and popular projects, as well as those proposed by readers by e-mail, are selected for verification. Therefore, among the game engines CryEngine V is not the first. The following engines were tested:
Also once was tested CryEngine 3 SDK .
')
I want to pay special attention to checking the Unreal Engine 4 game engine. Using this project as an example, we were able to tell in detail about the correct application of the static analysis methodology on a real project: from introducing an analyzer into a project to reducing the number of warnings to zero, followed by monitoring for errors in the new code. Our work with the Unreal Engine 4 project resulted in cooperation with Epic Games, in which PVS-Studio team fixed all the warnings of the analyzer in the engine source code, and a joint article was written about the work done, which was published in the Unreal Engine Blog ( link to Russian translation ). Epic Games also acquired a PVS-Studio license for independent control of the quality of the code. We are ready for a similar cooperation with Crytek.

Analyzer report structure


In this article I want to answer several frequently asked questions related to the number of warnings and false positives. For example, - “How many percent are false alarms?” - or - “Why are there so few errors in such a large project?”.

To begin with, all the warnings of the PVS-Studio analyzer are divided into three levels of importance: High , Medium and Low . For example, the High level contains high-critical warnings, which are most likely to be real errors, and the Low level contains warnings with low criticality, or warnings with a very high probability of a false positive response. It should be remembered that a specific error code does not necessarily bind it to a certain level of importance, and the distribution of messages into groups strongly depends on the context in which they were generated.

In the CryEngine V project, general analyzer warnings (General Analysis) are distributed by severity levels as follows:
Figure 1 shows the distribution of warnings by severity level in a pie chart.



Figure 1 - Distribution of warnings by level of importance in percentage

It is impossible to fit the description of all warnings into the article and to show the corresponding code fragments. Usually the article contains 10-40 examples of errors with the description. Some suspicious code points are simply a list. Most warnings remain pending. At best, after notifying the developers, they ask for a full analyzer report for detailed study. The bitter truth is that in most of the projects we check, the article is more than enough after viewing only High level alerts. And the CryEngine V game engine is no exception. Figure 2 shows the structure of High level alerts.



Figure 2 - Description of High Level Warnings

Now in more detail about the sectors of the presented pie chart:

Test results


Offensive copy-paste




V501 There are identical sub-expressions to the left and to the right of the operator: q2.vz - q2.vz entitynode.cpp 93
bool CompareRotation(const Quat& q1, const Quat& q2, float epsilon) { return (fabs_tpl(q1.vx - q2.vx) <= epsilon) && (fabs_tpl(q1.vy - q2.vy) <= epsilon) && (fabs_tpl(q2.vz - q2.vz) <= epsilon) // <= && (fabs_tpl(q1.w - q2.w) <= epsilon); }

A typo in one digit is perhaps one of the most offensive typos a programmer can make. In this function, the analyzer detected a suspicious expression (q2.vz - q2.vz) , in which the variables q1 and q2 are likely to be confused.

V501 There are identical sub-expressions '(m_eTFSrc == eTF_BC6UH)' to the left and to the right of the '||' operator. texturestreaming.cpp 919
 //! Texture formats. enum ETEX_Format : uint8 { .... eTF_BC4U, //!< 3Dc+. eTF_BC4S, eTF_BC5U, //!< 3Dc. eTF_BC5S, eTF_BC6UH, eTF_BC6SH, eTF_BC7, eTF_R9G9B9E5, .... }; bool CTexture::StreamPrepare(CImageFile* pIM) { .... if ((m_eTFSrc == eTF_R9G9B9E5) || (m_eTFSrc == eTF_BC6UH) || // <= (m_eTFSrc == eTF_BC6UH)) // <= { m_cMinColor /= m_cMaxColor.a; m_cMaxColor /= m_cMaxColor.a; } .... }

Another type of typos associated with copying constants. In this case, the m_eTFSrc variable is compared twice with the eTF_BC6UH constant, in the place of which any other should be, for example, distinguished by just one letter - the eTF_BC6SH constant.

Two more similar places in the project:
V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 266, 268. d3dhwshader.cpp 266
 int SD3DShader::Release(EHWShaderClass eSHClass, int nSize) { .... if (eSHClass == eHWSC_Pixel) return ((ID3D11PixelShader*)pHandle)->Release(); else if (eSHClass == eHWSC_Vertex) return ((ID3D11VertexShader*)pHandle)->Release(); else if (eSHClass == eHWSC_Geometry) // <= return ((ID3D11GeometryShader*)pHandle)->Release(); // <= else if (eSHClass == eHWSC_Geometry) // <= return ((ID3D11GeometryShader*)pHandle)->Release(); // <= else if (eSHClass == eHWSC_Hull) return ((ID3D11HullShader*)pHandle)->Release(); else if (eSHClass == eHWSC_Compute) return ((ID3D11ComputeShader*)pHandle)->Release(); else if (eSHClass == eHWSC_Domain) return ((ID3D11DomainShader*)pHandle)->Release() .... }

Here is an example of a lazy copy of a cascade of conditional statements, in one of the cases of which they forgot to make changes to the code.

V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 970, 974. environmentalweapon.cpp 970
 void CEnvironmentalWeapon::UpdateDebugOutput() const { .... const char* attackStateName = "None"; if(m_currentAttackState & // <= EAttackStateType_EnactingPrimaryAttack) // <= { attackStateName = "Primary Attack"; } else if(m_currentAttackState & // <= EAttackStateType_EnactingPrimaryAttack) // <= { attackStateName = "Charged Throw"; } .... }

In the previous code, there was at least some chance that the extra condition was the result of too many copies of the code and one check was simply superfluous. In this code snippet, due to identical conditional expressions, the variable attackStateName will never take the value “Charged Throw”.

V519 The 'BlendFactor [2]' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 1265, 1266. ccrydxgldevicecontext.cpp 1266
 void CCryDXGLDeviceContext:: OMGetBlendState(...., FLOAT BlendFactor[4], ....) { CCryDXGLBlendState::ToInterface(ppBlendState, m_spBlendState); if ((*ppBlendState) != NULL) (*ppBlendState)->AddRef(); BlendFactor[0] = m_auBlendFactor[0]; BlendFactor[1] = m_auBlendFactor[1]; BlendFactor[2] = m_auBlendFactor[2]; // <= BlendFactor[2] = m_auBlendFactor[3]; // <= *pSampleMask = m_uSampleMask; }

In the project code, there was such a function, in which, because of a typo in the index, the filling of the element with the index three is missing : BlendFactor [3] . This place would be just one of the interesting places with a typo, if the analyzer did not find another 2 fragments, where they copied the code with a typo:

V519 The 'm_auBlendFactor [2]' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 904, 905. ccrydxgldevicecontext.cpp 905
 void CCryDXGLDeviceContext:: OMSetBlendState(....const FLOAT BlendFactor[4], ....) { .... m_uSampleMask = SampleMask; if (BlendFactor == NULL) { m_auBlendFactor[0] = 1.0f; m_auBlendFactor[1] = 1.0f; m_auBlendFactor[2] = 1.0f; // <= m_auBlendFactor[2] = 1.0f; // <= } else { m_auBlendFactor[0] = BlendFactor[0]; m_auBlendFactor[1] = BlendFactor[1]; m_auBlendFactor[2] = BlendFactor[2]; // <= m_auBlendFactor[2] = BlendFactor[3]; // <= } m_pContext->SetBlendColor(m_auBlendFactor[0], m_auBlendFactor[1], m_auBlendFactor[2], m_auBlendFactor[3]); m_pContext->SetSampleMask(m_uSampleMask); .... }

This is the place where they continue to skip filling the element with the index '3'. For a moment, I even thought that this made some sense, but this thought quickly left me, when at the end of the function I saw an appeal to all four elements of the m_auBlendFactor array. It looks like in the file ccrydxgldevicecontext.cpp just made a few copies of the code with a typo.

V523 The 'then' statement is equivalent to the 'else' statement. d3dshadows.cpp 1410
 void CD3D9Renderer::ConfigShadowTexgen(....) { .... if ((pFr->m_Flags & DLF_DIRECTIONAL) || (!(pFr->bUseHWShadowMap) && !(pFr->bHWPCFCompare))) { //linearized shadows are used for any kind of directional //lights and for non-hw point lights m_cEF.m_TempVecs[2][Num] = 1.f / (pFr->fFarDist); } else { //hw point lights sources have non-linear depth for now m_cEF.m_TempVecs[2][Num] = 1.f / (pFr->fFarDist); } .... }

At the end of the section on copy-paste I provide a description of another interesting error. Regardless of the result of the conditional expression, the value of m_cEF.m_TempVecs [2] [Num] is always calculated using a single formula. Judging by the adjacent code of this fragment, there is no typo in the index, in this condition they want to fill in exactly the element with the index '2'. But the calculation formula most likely wanted to use a different one, but after copying the string, they forgot to change the code.

Initialization issues




V546 Member of a class is initialized by itself: 'eConfigMax (eConfigMax)'. particleparams.h 1013
 ParticleParams() : .... fSphericalApproximation(1.f), fVolumeThickness(1.0f), fSoundFXParam(1.f), eConfigMax(eConfigMax.VeryHigh), // <= fFadeAtViewCosAngle(0.f) {}

The analyzer detected a possible typo when the class field is initialized using its own value.

V603 The object was not used. If you wish to call the constructor, 'this-> SRenderingPassInfo :: SRenderingPassInfo (....)' should be used. i3dengine.h 2589
 SRenderingPassInfo() : pShadowGenMask(NULL) , nShadowSide(0) , nShadowLod(0) , nShadowFrustumId(0) , m_bAuxWindow(0) , m_nRenderStackLevel(0) , m_eShadowMapRendering(static_cast<uint8>(SHADOW_MAP_NONE)) , m_bCameraUnderWater(0) , m_nRenderingFlags(0) , m_fZoomFactor(0.0f) , m_pCamera(NULL) , m_nZoomInProgress(0) , m_nZoomMode(0) , m_pJobState(nullptr) { threadID nThreadID = 0; gEnv->pRenderer->EF_Query(EFQ_MainThreadList, nThreadID); m_nThreadID = static_cast<uint8>(nThreadID); m_nRenderFrameID = gEnv->pRenderer->GetFrameID(); m_nRenderMainFrameID = gEnv->pRenderer->GetFrameID(false); } SRenderingPassInfo(threadID id) { SRenderingPassInfo(); // <= SetThreadID(id); }

Here the analyzer has detected a misuse of the constructor The programmer probably thought that calling the constructor in such a way without parameters inside another constructor would initialize the class fields, but this is not so.

In this code, the following will occur: a new unnamed object of type SRenderingPassInfo will be created and immediately destroyed. As a result, the class fields remain uninitialized. One way to correct an error would be to write a separate initialization function and call it from different constructors.

V688 The c m_cNewGeomMML variable variable variable variable conf conf conf conf conf conf confusion. terrain_node.cpp 344
 void CTerrainNode::Init(....) { .... m_nOriginX = m_nOriginY = 0; // sector origin m_nLastTimeUsed = 0; // basically last time rendered uint8 m_cNewGeomMML = m_cCurrGeomMML = m_cNewGeomMML_Min .... m_pLeafData = 0; m_nTreeLevel = 0; .... }

The analyzer found the local variable name cNewGeomMML matches the class field. Often such code is not an error, but here it looks very suspicious against the background of initialization of other fields of the class.

V575 The 'memset' function processes '0' elements. Inspect the third argument. crythreadutil_win32.h 294
 void EnableFloatExceptions(....) { .... CONTEXT ctx; memset(&ctx, sizeof(ctx), 0); // <= .... }

With the help of the analyzer a very interesting error was found. When the memset () function was called, the passed arguments were confused, and as a result, the function was called to fill 0 bytes of memory. Here is the function prototype:
 void * memset ( void * ptr, int value, size_t num );

The third argument should be the size of the buffer, and the second is the value that needs to be filled.

Corrected version:
 void EnableFloatExceptions(....) { .... CONTEXT ctx; memset(&ctx, 0, sizeof(ctx)); .... }

V630 The '_alloca' function is used to allocate memory constructors. command_buffer.cpp 62
 void CBuffer::Execute() { .... QuatT * pJointsTemp = static_cast<QuatT*>( alloca(m_state.m_jointCount * sizeof(QuatT))); .... }

In the project code, there are places where, using the alloca () function, allocate memory for an array of objects. In the above code, with this method of allocating memory to objects of the QuatT class, neither the constructor nor the destructor will call. Such code can lead to working with uninitialized variables and other errors.

The entire list of suspicious places:
V583 The '?:' Operator, regardless of its conditional expression, always returns one and the same value: -1.8f. posealignerc3.cpp 330
 ILINE bool InitializePoseAlignerPinger(....) { .... chainDesc.offsetMin = Vec3(0.0f, 0.0f, bIsMP ? -1.8f : -1.8f); chainDesc.offsetMax = Vec3(0.0f, 0.0f, bIsMP ? +0.75f : +1.f); .... }

In the project code there are cases when the ternary operator ?: Returns the same value. Perhaps, in the previous case, so written for the beauty of the code, but why did they do it in this fragment?
 float predictDelta = inputSpeed < 0.0f ? 0.1f : 0.1f; // <= float dict = angle + predictDelta * ( angle - m_prevAngle) / dt ;

All such places found in the project:
V570 The 'runtimeData.entityId' variable is assigned to itself. behaviortreenodes_ai.cpp 1771
 void ExecuteEnterScript(RuntimeData& runtimeData) { ExecuteScript(m_enterScriptFunction, runtimeData.entityId); runtimeData.entityId = runtimeData.entityId; // <= runtimeData.executeExitScriptIfDestructed = true; }

Suspicious assignment of a variable to itself. Developers should check out this place.

Priorities for operations




V502 Perhaps the '?:' Operator was different. The '?:' Operator has a lower limit than the '+' operator. gpuparticlefeaturespawn.cpp 79
 bool HasDuration() { return m_useDuration; } void CFeatureSpawnRate::SpawnParticles(....) { .... SSpawnData& spawn = pRuntime->GetSpawnData(i); const float amount = spawn.amount; const int spawnedBefore = int(spawn.spawned); const float endTime = spawn.delay + HasDuration() ? spawn.duration : fHUGE; .... }

It seems that in this function time is incorrectly measured. The priority of the addition operator is higher than that of the ternary ?: Operator, so the value 0 or 1 is first added to spawn.delay , and the value spawn.duration or fHUGE is always written to the endTime variable . This is a fairly common mistake. I described the interesting error patterns in the priorities of operations found in the PVS-Studio error database in the article: Logical expressions in C / C ++. How wrong are the professionals .

V634 The operation of the operation is higher than that of the operation. It's possible that parentheses should not be used in the expression. model.cpp 336
 enum joint_flags { angle0_locked = 1, .... }; bool CDefaultSkeleton::SetupPhysicalProxies(....) { .... for (int j = 0; .... ; j++) { // lock axes with 0 limits range m_arrModelJoints[i]....flags |= (....) * angle0_locked << j; } .... }

Another very interesting error related to the priority of multiply and bit-shift operations. The latter has lower execution priority, so the entire expression at each iteration is multiplied by one (the constant angle0_locked is equal to one), which looks very strange.

Most likely they wanted to write this:
 m_arrModelJoints[i]....flags |= (....) * (angle0_locked << j);

A list of 35 suspicious places with the priority of shift operators is given in the CryEngine5_V634.txt file.

Undefined behavior


Undefined behavior is a property of some programming languages ​​in certain situations to produce a result that depends on random factors, such as a memory state or an interrupt. In other words, the specification does not define the behavior of the language in any possible situations. It is considered an error to allow such a situation in the program, even if the program is successfully executed on some compiler, it will not be cross-platform and may fail on another machine, in another OS and even on other compiler settings.



V610 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. physicalplaceholder.h 25
 #ifndef physicalplaceholder_h #define physicalplaceholder_h #pragma once .... const int NO_GRID_REG = -1<<14; const int GRID_REG_PENDING = NO_GRID_REG+1; ....

According to the current C ++ standard, a left-shift of a negative number leads to undefined behavior. In addition, there are several more such places in the CryEngine V code:
V567 Undefined behavior. The variable "m_current" variable operatorqueue.cpp 105
 bool COperatorQueue::Prepare(....) { ++m_current &= 1; m_ops[m_current].clear(); return true; }

The analyzer detected an expression that leads to undefined program behavior. A variable is repeatedly used between two points of sequence, and its value changes. As a result, it is impossible to predict the result of such an expression.

More suspicious places:

Different errors in conditions




V579 It is possibly a mistake. Inspect the third argument. graphicspipelinestateset.h 58
 bool operator==(const SComputePipelineStateDescription& other) const { return 0 == memcmp(this, &other, sizeof(this)); // <= }

The equality operator made a mistake in calling the memcmp () function, passing the pointer size to it, and not the size of the object. Now objects are compared by the first few bytes.

Corrected version:
 memcmp(this, &other, sizeof(*this));

Unfortunately, there are three more such places in the project that are worth checking 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. livingentity.cpp 181
 CLivingEntity::~CLivingEntity() { for(int i=0;i<m_nParts;i++) { if (!m_parts[i].pPhysGeom || ....) delete[] m_parts[i].pMatMapping; m_parts[i].pMatMapping=0; } .... }

In the game engine code, I noticed a lot of places where developers write several operators per line. Often, these are not even ordinary assignments, but cycles, conditions, function calls, and sometimes all this mixed up (Figure 3).



Figure 3 - Bad code formatting

With such a programming style, it is almost impossible to avoid errors on a large scale. Thus, in the case under consideration, it was planned, under certain conditions, to free up memory from under an array of objects and to reset the pointer. But due to incorrect formatting, the m_parts [i] .pMatMapping pointer is reset to each iteration of the loop. What negative consequences this may have, I cannot predict, but the code is unequivocally alarming.

A few more places with suspicious formatting:
V695 Range intersections are possible within conditional expressions. Example: if (A <5) {...} else if (A <2) {...}. Check lines: 538, 540. statobjrend.cpp 540
 bool CStatObj::RenderDebugInfo(....) { .... ColorB clr(0, 0, 0, 0); if (nRenderMats == 1) clr = ColorB(0, 0, 255, 255); else if (nRenderMats == 2) clr = ColorB(0, 255, 255, 255); else if (nRenderMats == 3) clr = ColorB(0, 255, 0, 255); else if (nRenderMats == 4) clr = ColorB(255, 0, 255, 255); else if (nRenderMats == 5) clr = ColorB(255, 255, 0, 255); else if (nRenderMats >= 6) // <= clr = ColorB(255, 0, 0, 255); else if (nRenderMats >= 11) // <= clr = ColorB(255, 255, 255, 255); .... }

In this code, the programmer made a mistake due to which the color ColorB (255, 255, 255, 255) will never be selected. First, the values ​​of nRenderMats are compared in order with numbers from 1 to 5, but when the developer switched to working with a range of values, he did not take into account that values ​​greater than 11 already fall into a range greater than 6, therefore, the latter condition is never satisfied.

This cascade of conditions was completely copied to one more place:
V695 Range intersections are possible within conditional expressions. Example: if (A <5) {...} else if (A <2) {...}. Check lines: 393, 399. xmlcpb_nodelivewriter.cpp 399
 enum eNodeConstants { .... CHILDBLOCKS_MAX_DIST_FOR_8BITS = BIT(7) - 1, // 127 CHILDBLOCKS_MAX_DIST_FOR_16BITS = BIT(6) - 1, // 63 .... }; void CNodeLiveWriter::Compact() { .... if (dist <= CHILDBLOCKS_MAX_DIST_FOR_8BITS) // dist <= 127 { uint8 byteDist = dist; writeBuffer.AddData(&byteDist, sizeof(byteDist)); isChildBlockSaved = true; } else if (dist <= CHILDBLOCKS_MAX_DIST_FOR_16BITS) // dist <= 63 { uint8 byteHigh = CHILDBLOCKS_USING_MORE_THAN_8BITS | ....); uint8 byteLow = dist & 255; writeBuffer.AddData(&byteHigh, sizeof(byteHigh)); writeBuffer.AddData(&byteLow, sizeof(byteLow)); isChildBlockSaved = true; } .... }

A similar error in the condition was made in this code fragment, only now a rather large code fragment does not receive control. The values ​​of the CHILDBLOCKS_MAX_DIST_FOR_8BITS and CHILDBLOCKS_MAX_DIST_FOR_16BITS constants have values ​​such that the second condition will never be true.

V547 Expression 'pszScript [iSrcBufPos]! =' == '' is always true. The value range of char type: [-128, 127]. luadbg.cpp 716
 bool CLUADbg::LoadFile(const char* pszFile, bool bForceReload) { FILE* hFile = NULL; char* pszScript = NULL, * pszFormattedScript = NULL; .... while (pszScript[iSrcBufPos] != ' ' && .... pszScript[iSrcBufPos] != '=' && pszScript[iSrcBufPos] != '==' && // <= pszScript[iSrcBufPos] != '*' && pszScript[iSrcBufPos] != '+' && pszScript[iSrcBufPos] != '/' && pszScript[iSrcBufPos] != '~' && pszScript[iSrcBufPos] != '"') {} .... }

In a large conditional expression, there is a subexpression that is always true. The literal '==' will be of type int and will be equal to 15677. The pszScript array consists of elements of type char . A value of type char cannot be 15677. That is why the expression pszScript [iSrcBufPos]! = '==' is always true.

V734 An excessive expression. Examine the substrings "_ddn" and "_ddna". texture.cpp 4212
 void CTexture::PrepareLowResSystemCopy(byte* pTexData, ....) { .... // make sure we skip non diffuse textures if (strstr(GetName(), "_ddn") // <= || strstr(GetName(), "_ddna") // <= || strstr(GetName(), "_mask") || strstr(GetName(), "_spec.") || strstr(GetName(), "_gloss") || strstr(GetName(), "_displ") || strstr(GetName(), "characters") || strstr(GetName(), "$") ) return; .... }

The strstr () function searches for the first occurrence of the specified substring in another string and returns a pointer to the first occurrence of the substring or a null pointer. The first one looks for the substring "_ddn", and then "_ddna", which means that the condition will be met if a shorter substring is found. Perhaps the code does not work as planned by the programmer. Well, or at least the expression is redundant and can be simplified by removing the extra check.

V590 Consider inspecting this expression. The expression is misprint. goalop_crysis2.cpp 3779
 void COPCrysis2FlightFireWeapons::ParseParam(....) { .... else if (!paused && (m_State == eFP_PAUSED) && // <= (m_State != eFP_PAUSED_OVERRIDE)) // <= .... }

The conditional expression in the ParseParam () function is written in such a way that the result does not depend on the subexpression ( m_State! = EFP_PAUSED_OVERRIDE ).

Consider a simplified example:
 if ( err == code1 && err != code2) { .... }

The result of the entire conditional expression does not depend on the result of the subexpression (err! = Code2) , which is clearly seen when building the truth table for this example (Figure 4)



Figure 4 - Truth Table for Boolean Expression

Comparing unsigned numbers with zero




In checked projects, there are often comparisons of unsigned numbers with zero, the result of which is always either true or false. This code does not always contain a serious error. Often this is the result of excessive caution, or the test remains after changing the type of variable from signed to unsigned. In any case, such comparisons should be carefully checked.

V547 Expression 'm_socket <0' is always false. Unsigned type value is never <0. servicenetwork.cpp 585
 typedef SOCKET CRYSOCKET; // Internal socket data CRYSOCKET m_socket; bool CServiceNetworkConnection::TryReconnect() { .... // Create new socket if needed if (m_socket == 0) { m_socket = CrySock::socketinet(); if (m_socket < 0) { .... return false; } } .... }

SOCKET , , , .

0 -1, . CryEngine V , - , :
 if (m_socket == CRY_INVALID_SOCKET)

.

47 , , CryEngine5_V547.txt . .




The diagnostic rule V595 finds in the code dereferencing of pointers, the validity check of which is performed below the code, i.e. after using the pointer. In practice, this diagnosis is very steep errors. A small number of false positives occur because the pointer check is performed indirectly, i.e. through one or more other variables, but agree that it will be very difficult for a person to understand this code. I will give three examples of how this diagnostic works, which is particularly surprising how this code works, the rest can be found in the CryEngine5_V595.txt file .

Example 1


V595 The 'm_pPartManager' pointer has been verified against nullptr. Check lines: 1441, 1442. 3denginerender.cpp 1441
 void C3DEngine::RenderInternal(....) { .... m_pPartManager->GetLightProfileCounts().ResetFrameTicks(); if (passInfo.IsGeneralPass() && m_pPartManager) m_pPartManager->Update(); .... }

Dereferencing and checking the m_pPartManager pointer .

Example 2


V595 The 'gEnv-> p3DEngine' pointer was used against nullptr. Check lines: 1477, 1480. gameserialize.cpp 1477
 bool CGameSerialize::LoadLevel(....) { .... // can quick-load if (!gEnv->p3DEngine->RestoreTerrainFromDisk()) return false; if (gEnv->p3DEngine) { gEnv->p3DEngine->ResetPostEffects(); } .... }

Dereferencing and checking the pointer gEnv-> p3DEngine .

Example 3


V595 The 'pSpline' pointer was used before it was verified against nullptr. Check lines: 158, 161. facechankeycleanup.cpp 158
 void FaceChannel::CleanupKeys(....) { CFacialAnimChannelInterpolator backupSpline(*pSpline); // Create the key entries array. int numKeys = (pSpline ? pSpline->num_keys() : 0); .... }

Dereferencing and checking the pSpline pointer .

Miscellaneous Warnings




V622 Consider inspecting the 'switch' statement. It's possible that the operator is missing. mergedmeshrendernode.cpp 999
 static inline void ExtractSphereSet(....) { .... switch (statusPos.pGeom->GetType()) { if (false) { case GEOM_CAPSULE: statusPos.pGeom->GetPrimitive(0, &cylinder); } if (false) { case GEOM_CYLINDER: statusPos.pGeom->GetPrimitive(0, &cylinder); } for (int i = 0; i < 2 && ....; ++i) { .... } break; .... }

Perhaps this code fragment is the strangest of all that occurred in the project CryEngine V. The choice of the case label does not depend on the conditional if statement , even if there is (false) . The switch statement executes an unconditional jump to a label if it satisfies the switch expression . Without the use of the break statement , using such a code, it is possible to “bypass” the execution of unnecessary statements, but again, not all developers can easily accompany such non-obvious code. And why is the same code executed when going to the GEOM_CAPSULE and GEOM_CYLINDER labels ?

V510The 'LogError' function is not expected to receive a class-type variable as the second actual argument. behaviortreenodes_action.cpp 143
 typedef CryStringT<char> string; // The actual fragment name. string m_fragName; //! cast to C string. const value_type* c_str() const { return m_str; } const value_type* data() const { return m_str; }; void LogError(const char* format, ...) const { .... } void QueueAction(const UpdateContext& context) { .... ErrorReporter(*this, context).LogError("....'%s'", m_fragName); .... }

If in the function description it is impossible to specify the number and types of all valid parameters, then the list of formal parameters is completed with an ellipsis (...), which means: “and, possibly, some more arguments”. Only POD (Plain Old Data) types can act as an actual parameter for an ellipse. If the ellipse of the function is passed as a parameter to a class object, it almost always indicates the presence of an error in the program. Instead of a pointer to a string, the contents of the object get into the stack Such a code will lead to the formation of “abracadabra” in the buffer or to the emergency termination of the program. The CryEngine V code uses its own string class, and it already has a suitable c_str () method .

Corrected version:
 LogError("....'%s'", m_fragName.c_str();

A few more suspicious places:
V529 Odd semicolon ';' after 'for' operator. boolean3d.cpp 1314
 int CTriMesh::Slice(....) { .... bop_meshupdate *pmd = new bop_meshupdate, *pmd0; pmd->pMesh[0]=pmd->pMesh[1] = this; AddRef();AddRef(); for(pmd0=m_pMeshUpdate; pmd0->next; pmd0=pmd0->next); // <= pmd0->next = pmd; .... }

. for , .

V535 The variable 'j' is being used for this loop and for the outer loop. Check lines: 3447, 3490. physicalworld.cpp 3490
 void CPhysicalWorld::SimulateExplosion(....) { .... for(j=0;j<pmd->nIslands;j++) // <= line 3447 { .... for(j=0;j<pcontacts[ncont].nborderpt;j++) // <= line 3490 { .... }

The project code is also full of other dangerous code, for example, using one counter in nested and external cycles. Large files with source code contain confusing formatting and changing of some variables in different places - static analysis is indispensable here!

A few more suspicious cycles:
V539 Consider inspecting iterators which are being passed as arguments to function 'erase'. frameprofilerender.cpp 1090
 float CFrameProfileSystem::RenderPeaks() { .... std::vector<SPeakRecord>& rPeaks = m_peaks; // Go through all peaks. for (int i = 0; i < (int)rPeaks.size(); i++) { .... if (age > fHotToColdTime) { rPeaks.erase(m_peaks.begin() + i); // <= i--; } .... }

The analyzer considered that an iterator from another container is passed to the function that performs the action with the container. This is not the case in this code snippet and there is no error. The variable rPeaks is a reference to m_peaks . However, such a code can be misleading not only for the code analyzer, but also for the people who will accompany the code. Do not write like that.

V713 The pointer was used against it. actiongame.cpp 4235
 int CActionGame::OnCollisionImmediate(const EventPhys* pEvent) { .... else if (pMat->GetBreakability() == 2 && pCollision->idmat[0] != pCollision->idmat[1] && (energy = pMat->GetBreakEnergy()) > 0 && pCollision->mass[0] * 2 > energy && .... pMat->GetHitpoints() <= FtoI(min(1E6f, hitenergy / energy)) && pCollision) // <= return 0; .... }

The if statement contains a rather large conditional expression in which the pCollision pointer is accessed everywhere . The error is that the pCollision pointer is checked for equality to zero most recently, i.e. after repeated dereferencing.

V758 The 'commandList' link becomes a reference. renderitemdrawer.cpp 274
 typedef std::shared_ptr<....> CDeviceGraphicsCommandListPtr; CDeviceGraphicsCommandListPtr CDeviceObjectFactory::GetCoreGraphicsCommandList() const { return m_pCoreCommandList; } void CRenderItemDrawer::DrawCompiledRenderItems(....) const { .... { auto& RESTRICT_REFERENCE commandList = *CCryDeviceWrapper:: GetObjectFactory().GetCoreGraphicsCommandList(); passContext....->PrepareRenderPassForUse(commandList); } .... }

A reference to the value stored in the smart pointer is stored in the commandList variable . When the object is destroyed by a smart pointer, the link becomes invalid.

A few more places like this:

Conclusion


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. Regardless of the analyzer used, the static code analysis methodology itself has long been proven to be a very effective way to control the quality of the code and the software as a whole.

Collaboration with Epic Games has demonstrated the benefits of introducing a static analyzer into the Unreal Engine 4 . We helped the developers in all the integration issues of the analyzer and even fixed the errors found so that they could regularly check the new project code. We are ready for a similar cooperation with Crytek.

I suggest everyone to try PVS-Studioon their C / C ++ / C # projects.

The developers of CryEngineV were notified in advance about the verification of the project, so some errors may already be corrected.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. I awaited the Check-Long of the CryEngine the V .

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/307046/

More articles:


All Articles