• Long-Awaited Check of CryEngine V

General and Gameplay Programming

In May 2016, German game-development company Crytek made a decision to upload the source code of their game engine CryEngine V to Github. The engine is written in C++ and has immediately attracted attention of both the open-source developer community and the team of developers of PVS-Studio static analyzer who regularly scan the code of open-source projects to estimate its quality. A lot of great games were created by a number of video-game development studios using various versions of CryEngine, and now the engine has become available to even more developers. This article gives an overview of errors found in the project by PVS-Studio static analyzer.

Introduction

CryEngine is a game engine developed by German company Crytek in 2002 and originally used in first-person shooter Far Cry. A lot of great games were created by a number of video-game development studios using various licensed versions of CryEngine: 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 a release date for their new engine CryEngine V and uploaded its source code to Github soon after.

The project's source code was checked by PVS-Studio static analyzer, version 6.05. This is a tool designed for detecting software errors in program source code in C, C++, and C#. The only true way of using static analysis is to regularly scan code on developers' computers and build-servers. However, in order to demonstrate PVS-Studio's diagnostic capabilities, we run single-time checks of open-source projects and then write articles about errors found. If we like a project, we might scan it again a couple of years later. Such recurring checks are in fact the same as single-time checks since the code accumulates a lot of changes during that time.

For our checks, we pick projects that are simply popular and wide-known as well as projects suggested by our readers via e-mail. That's why CryEngine V was by no means the first game engine among those scanned by our analyzer. Other engines that we have already checked include:

We also checked CryEngine 3 SDK once.

I'd like to elaborate on the check of Unreal Engine 4 engine in particular. Using that project as an example allowed us to demonstrate in every detail what the right way of using static analysis on a real project should look like, covering the whole process from the phase of integrating the analyzer into the project to the phase of cutting warnings to zero with subsequent control over bug elimination in new code. Our work on Unreal Engine 4 project developed into collaboration with Epic Games company, in terms of which our team fixed all the defects found in the engine's source code and wrote a joint article with Epic Games on the accomplished work (it was posted on Unreal Engine Blog). Epic Games also purchased a PVS-Studio license to be able to maintain the quality of their code on their own. Collaboration of this kind is something that we would like to try with Crytek, too.

Analyzer-report structure

In this article, I'd like to answer a few frequently asked questions concerning the number of warnings and false positives, for example, "What is the ratio of false positives?" or "Why are there so few bugs in so large a project?"

To begin with, all PVS-Studio warnings are classified into three severity levels: High, Medium, and Low. The High level holds the most critical warnings, which are almost surely real errors, while the Low level contains the least critical warnings or warnings that are very likely to be false positives. Keep in mind that the codes of errors do not tie them firmly to particular severity levels: distribution of warnings across the levels very much depends on the context.

This is how the warnings of the General Analysis module are distributed across the severity levels for CryEngine V project:

• High: 576 warnings;
• Medium: 814 warnings,
• Low: 2942 warnings.

Figure 1 shows distribution of the warnings across the levels in the form of a pie chart.

Figure 1 - Percentage distribution of warnings across severity levels

It is impossible to include all the warning descriptions and associated code fragments in an article. Our articles typically discuss 10-40 commented cases; some warnings are given as a list; and most have to be left unexamined. In the best-case scenario, project authors, after we inform them, ask for a complete analysis report for close study. The bitter truth is that in most cases the number of High-level warnings alone is more than enough for an article, and CryEngine V is no exception. Figure 2 shows the structure of the High-level warnings issued for this project.

Figure 2 - Structure of High-level warnings

Let's take a closer look at the sectors of this chart:

• Described in the article (6%) - warnings cited in the article and accompanied by code fragments and commentary.
• Presented as a list (46%) - warnings cited as a list. These warnings refer to the same pattern as some of the errors already discussed, so only the warning text is given.
• False Positives (8%) - a certain ratio of false positives we have taken into account for future improvement of the analyzer.
• Other (40%) - all the other warnings issued. These include warnings that we had to leave out so that the article wouldn't grow too large, non-critical warnings, or warnings whose validity could be estimated only by a member of the developer team. As our experience of working on Unreal Engine 4 has shown, such code still "smells" and those warnings get fixed anyway.

Analysis results

Annoying copy-paste

V501 There are identical sub-expressions to the left and to the right of the '-' operator: q2.v.z - q2.v.z entitynode.cpp 93

 bool CompareRotation(const Quat& q1, const Quat& q2, float epsilon) { return (fabs_tpl(q1.v.x - q2.v.x) <= epsilon) && (fabs_tpl(q1.v.y - q2.v.y) <= epsilon) && (fabs_tpl(q2.v.z - q2.v.z) <= epsilon) // <= && (fabs_tpl(q1.w - q2.w) <= epsilon); }

A mistyped digit is probably one of the most annoying typos one can make. In the function above, the analyzer detected a suspicious expression, (q2.v.z - q2.v.z), where variables q1 and q2 seem to have been mixed up.

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 kind of typos deals with copying of constants. In this case, the m_eTFSrc variable is compared twice with the eTF_BC6UH constant. The second of these checks must compare the variable with some other constant whose name differs from the copied one in just one character, for example, eTF_BC6SH.

Two more similar issues:

• V501 There are identical sub-expressions '(td.m_eTF == eTF_BC6UH)' to the left and to the right of the '||' operator. texture.cpp 1214
• V501 There are identical sub-expressions 'geom_colltype_solid' to the left and to the right of the '|' operator. attachmentmanager.cpp 1004

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability 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() .... }

This is an example of lazy copying of a cascade of conditional statements, one of which was left unchanged.

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability 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 example, there was at least a small chance that an extra condition resulted from making too many copies of a code fragment, while the programmer simply forgot to remove one of the checks. In this code, however, the attackStateName variable will never take the value "Charged Throw" because of identical conditional expressions.

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 this function, a typo in the element index prevents the element with index '3', BlendFactor[3], from being filled with a value. This fragment would have remained just one of the many interesting examples of typos, had not the analyzer found two more copies of the same incorrect fragment:

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

Here's that fragment where the element with index '3' is skipped again. I even thought for a moment that there was some intentional pattern to it, but this thought quickly vanished as I saw that the programmer attempted to access all the four elements of the m_auBlendFactor array at the end of the function. It looks like the same code with a typo was simply copied several times in the file ccrydxgldevicecontext.cpp.

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

To finish the section on copy-paste, here is one more interesting error. No matter what result the conditional expression produces, the value m_cEF.m_TempVecs[2][Num] is always computed by the same formula. Judging by the surrounding code, the index is correct: it's exactly the element with index '2' that must be filled with a value. It's just that the formula itself was meant to be different in each case, and the programmer forgot to change the copied code.

Troubles with initialization

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 potential typo that causes a class field to be initialized to its own value.

V603 The object was created but it is not being used. If you wish to call 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(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(nThreadID); m_nRenderFrameID = gEnv->pRenderer->GetFrameID(); m_nRenderMainFrameID = gEnv->pRenderer->GetFrameID(false); } SRenderingPassInfo(threadID id) { SRenderingPassInfo(); // <= SetThreadID(id); }

In this code, incorrect use of constructor was detected. The programmer probably assumed that calling a constructor in a way like that - without parameters - inside another constructor would initialize the class fields, but this assumption was wrong.

Instead, a new unnamed object of type SRenderingPassInfo will be created and immediately destroyed. The class fields, therefore, will remain uninitialized. One way to fix this error is to create a separate initialization function and call it from different constructors.

V688 The 'm_cNewGeomMML' local variable possesses the same name as one of the class members, which can result in a 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 name of the local variable cNewGeomMML coincides with that of a class field. It's usually not an error, but in this particular case it does look strange in comparison to how the other class fields are initialized.

V575 The 'memset' function processes '0' elements. Inspect the third argument. crythreadutil_win32.h 294

 void EnableFloatExceptions(....) { .... CONTEXT ctx; memset(&ctx, sizeof(ctx), 0); // <= .... }

This error is a very interesting one. When calling the memset() function, two arguments were swapped by mistake, which resulted in calling the function to fill 0 bytes. This is the function prototype:

 void * memset ( void * ptr, int value, size_t num );

The function expects to receive the buffer size as the third argument and the value the buffer is to be filled with as the second.

The fixed version:

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

V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. command_buffer.cpp 62

 void CBuffer::Execute() { .... QuatT * pJointsTemp = static_cast( alloca(m_state.m_jointCount * sizeof(QuatT))); .... }

In some parts of the project's code, the alloca() function is used to allocate memory for an array of objects. In the example above, with memory allocated in such a way, neither the constructor, nor the destructor will be called for objects of class QuatT. This defect may result in handling uninitialized variables, and other errors.

Here's a complete list of other defects of this type:

• V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. command_buffer.cpp 67
• V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. posematching.cpp 144
• V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. characterinstance.cpp 280
• V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. characterinstance.cpp 282
• V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. scriptbind_entity.cpp 6252
• V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. jobmanager.cpp 1016
• V630 The '_alloca' function is used to allocate memory for an array of objects which are classes containing constructors. driverd3d.cpp 5859

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

A few fragments were found where the ternary operator ?: returns one and the same value. While in the previous example it could have been done for aesthetic reasons, the reason for doing so in the following fragment is unclear.

 float predictDelta = inputSpeed < 0.0f ? 0.1f : 0.1f; // <= float dict = angle + predictDelta * ( angle - m_prevAngle) / dt ;

A complete list of other defects of this type:

• V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: -1.8f. posealignerc3.cpp 313
• V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: -2.f. posealignerc3.cpp 347
• V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: D3D11_RTV_DIMENSION_TEXTURE2DARRAY. d3dtexture.cpp 2277
• V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: 255U. renderer.cpp 3389
• V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: D3D12_RESOURCE_STATE_GENERIC_READ. dx12device.cpp 151
• V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: 0.1f. vehiclemovementstdboat.cpp 720

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

A variable is assigned to itself, which doesn't look right. The authors should check this code.

Operation precedence

V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority 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; .... }

The function above seems to measure time in a wrong way. The precedence of the addition operator is higher than that of the ternary operator :?, so the value 0 or 1 is added to spawn.delay first, and then the value spawn.duration or fHUGE is written into the endTime variable. This error is quite a common one. To learn more about interesting patterns of errors involving operation precedence collected from the PVS-Studio bug database, see my article: Logical Expressions in C/C++. Mistakes Made by Professionals.

V634 The priority of the '*' operation is higher than that of the '<' operation. It's possible that parentheses should 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....flags |= (....) * angle0_locked << j; } .... }

This is another very interesting error that has to do with the precedence of the multiplication and bitwise shift operations. The latter has lower precedence, so the whole expression is multiplied by one at each iteration (as the angle0_locked constant has the value one), which looks very strange.

This is what the programmer must have wanted that code to look like:

 m_arrModelJoints....flags |= (....) * (angle0_locked << j);

The following file contains a list of 35 suspicious fragments involving precedence of shift operations: CryEngine5_V634.txt.

Undefined behavior

Undefined behavior is the result of executing computer code written in a certain programming language that depends on a number of random factors such as memory state or triggered interrupts. In other words, this result is not prescribed by the language specification. It is considered to be an error to let such a situation occur in your program. Even if it can successfully execute on some compiler, it is not guaranteed to be cross-platform and may fail on another machine, operating system, and even other settings of the same compiler.

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

Under the modern C++ standard, a left shift of a negative value is undefined behavior. The analyzer found a few more similar issues in CryEngine V's code:

• V610 Undefined behavior. Check the shift operator '<'. The left operand '~(TFragSeqStorage(0))' is negative. udpdatagramsocket.cpp 757
• V610 Undefined behavior. Check the shift operator '<'. The left operand '-1' is negative. tetrlattice.cpp 324
• V610 Undefined behavior. Check the shift operator '<'. The left operand '-1' is negative. tetrlattice.cpp 350
• V610 Undefined behavior. Check the shift operator '<'. The left operand '-1' is negative. tetrlattice.cpp 617
• V610 Undefined behavior. Check the shift operator '<'. The left operand '-1' is negative. tetrlattice.cpp 622
• V610 Undefined behavior. Check the shift operator '<'. The left operand '(~(0xF))' is negative. d3ddeferredrender.cpp 876
• V610 Undefined behavior. Check the shift operator '<'. The left operand '(~(0xF))' is negative. d3ddeferredshading.cpp 791
• V610 Undefined behavior. Check the shift operator '<'. The left operand '(~(1 < 0))' is negative. d3dsprites.cpp 1038

V567 Undefined behavior. The 'm_current' variable is modified while being used twice between sequence points. operatorqueue.cpp 105

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

The analyzer detected an expression that causes undefined behavior. A variable is used multiple times between two sequence points, while its value changes. The result of executing such an expression, therefore, can't be determined.

Other similar issues:

• V567 Undefined behavior. The 'itail' variable is modified while being used twice between sequence points. trimesh.cpp 3101
• V567 Undefined behavior. The 'ihead' variable is modified while being used twice between sequence points. trimesh.cpp 3108
• V567 Undefined behavior. The 'ivtx' variable is modified while being used twice between sequence points. boolean3d.cpp 1194
• V567 Undefined behavior. The 'ivtx' variable is modified while being used twice between sequence points. boolean3d.cpp 1202
• V567 Undefined behavior. The 'ivtx' variable is modified while being used twice between sequence points. boolean3d.cpp 1220
• V567 Undefined behavior. The 'm_commandBufferIndex' variable is modified while being used twice between sequence points. xconsole.cpp 180
• V567 Undefined behavior. The 'm_FrameFenceCursor' variable is modified while being used twice between sequence points. ccrydx12devicecontext.cpp 952
• V567 Undefined behavior. The 'm_iNextAnimIndex' variable is modified while being used twice between sequence points. hitdeathreactionsdefs.cpp 192
Errors in conditions

V579 The memcmp function receives the pointer and its size as arguments. 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 programmer made a mistake in the equality operation in the call to the memcmp() function, which leads to passing the pointer size instead of the object size as a function argument. As a result, only the first several bytes of the objects are compared.

The fixed version:

 memcmp(this, &other, sizeof(*this));

Unfortunately, three more similar issues were found in the project:

• V579 The memcpy function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. geomcacherendernode.cpp 286
• V579 The AddObject function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. clipvolumemanager.cpp 145
• V579 The memcmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. graphicspipelinestateset.h 34

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.pPhysGeom || ....) delete[] m_parts.pMatMapping; m_parts.pMatMapping=0; } .... }

I spotted a huge number of code blocks with statements written in one line. These include not only ordinary assignments, but rather loops, conditions, function calls, and sometimes a mixture of all of these (see Figure 3).

Figure 3 - Poor code formatting

In code of size like that, this programming style almost inevitably leads to errors. In the example above, the memory block occupied by an array of objects was to be freed and the pointer was to be cleared when a certain condition was met. However, incorrect code formatting causes the m_parts.pMatMapping pointer to be cleared at every loop iteration. The implications of this problem can't be predicted, but the code does look strange.

Other fragments with strange formatting:

• 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. physicalworld.cpp 2449
• 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. articulatedentity.cpp 1723
• 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. articulatedentity.cpp 1726

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

The programmer made a mistake that prevents the color ColorB(255, 255, 255, 255) from ever being selected. The values nRenderMats are first compared one by one with the numbers from 1 to 5, but when comparing them with value ranges, the programmer didn't take into account that values larger than 11 already belong to the range of values larger than 6, so the last condition will never execute.

This cascade of conditions was copied in full into one more fragment:

• V695 Range intersections are possible within conditional expressions. Example: if (A < 5) { ... } else if (A < 2) { ... }. Check lines: 338, 340. modelmesh_debugpc.cpp 340

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 mistake inside a condition was also found in the fragment above, except that this time the code that fails to get control is larger. The values of the constants CHILDBLOCKS_MAX_DIST_FOR_8BITS and CHILDBLOCKS_MAX_DIST_FOR_16BITS are 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] != '"') {} .... }

A large conditional expression contains a subexpression that is always true. The '==' literal will have type int and correspond to the value 15677. The pszScript array consists of elements of type char, and a value of type char can't be equal to 15677, so the pszScript[iSrcBufPos] != '==' expression 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 looks for the first occurrence of the specified substring within another string and returns either a pointer to the first occurrence or an empty pointer. The string "_ddn" is the first to be searched, and "_ddna" is the second, which means that the condition will be true if the shorter string is found. This code might not work as expected; or perhaps this expression is redundant and could be simplified by removing the extra check. V590 Consider inspecting this expression. The expression is excessive or contains a 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 its result does not depend on the (m_State != eFP_PAUSED_OVERRIDE) subexpression. Here's a simpler example:  if ( err == code1 && err != code2) { .... } The result of the whole conditional expression does not depend on the result of the (err != code2) subexpression, which can be clearly seen from the truth table for this example (see Figure 4) Figure 4 - Truth table for a logical expression Comparing unsigned values with zero When scanning projects, we often come across comparisons of unsigned values with zero, which produce either true or false every time. Such code does not always contain a critical bug; it is often a result of too much caution or changing a variable's type from signed to unsigned. Anyway, such comparisons need to be 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; } } .... } I'd like to elaborate on the SOCKET type. It can be both signed and unsigned depending on the platforms, so it is strongly recommended that you use special macros and constants specified in the standard headers when working with this type. In cross-platform projects, comparisons with 0 or -1 are common that result in misinterpretation of error codes. CryEngine V project is no exception, although some checks are done correctly, for example:  if (m_socket == CRY_INVALID_SOCKET) Nevertheless, many parts of the code use different versions of these checks. See the file CryEngine5_V547.txt for other 47 suspicious comparisons of unsigned variables with zero. The code authors need to check these warnings. Dangerous pointers Diagnostic V595 detects pointers that are tested for null after they have been dereferenced. In practice, this diagnostic catches very tough bugs. On rare occasions, it issues false positives, which is explained by the fact that pointers are checked indirectly, i.e. through one or several other variables, but figuring such code out isn't an easy task for a human either, is it? Three code samples are given below that trigger this diagnostic and look especially surprising, as it's not clear why they work at all. For the other warnings of this type see the file CryEngine5_V595.txt. Example 1 V595 The 'm_pPartManager' pointer was utilized before it was 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(); .... } The m_pPartManager pointer is dereferenced and then checked. Example 2 V595 The 'gEnv->p3DEngine' pointer was utilized before it was verified 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(); } .... } The gEnv->p3DEngine pointer is dereferenced and then checked. Example 3 V595 The 'pSpline' pointer was utilized before it was verified against nullptr. Check lines: 158, 161. facechannelkeycleanup.cpp 158  void FaceChannel::CleanupKeys(....) { CFacialAnimChannelInterpolator backupSpline(*pSpline); // Create the key entries array. int numKeys = (pSpline ? pSpline->num_keys() : 0); .... } The pSpline pointer is dereferenced and then checked. Miscellaneous V622 Consider inspecting the 'switch' statement. It's possible that the first 'case' 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; .... } This fragment is probably the strangest of all found in CryEngine V. Whether or not the case label will be selected does not depend on the if statement, even in case of if (false). In the switch statement, an unconditional jump to the label occurs if the condition of the switch statement is met. Without the break statement, one could use such code to "bypass" irrelevant statements, but, again, maintaining such obscure code isn't easy. One more question is, why does the same code execute when jumping to the labels GEOM_CAPSULE and GEOM_CYLINDER? V510 The 'LogError' function is not expected to receive class-type variable as 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); .... } When it is impossible to specify the number and types of all acceptable parameters to a function, one puts ellipsis (...) at the end of the list of parameters in the function declaration, which means "and perhaps a few more". Only POD (Plain Old Data) types can be used as actual parameters to the ellipsis. If an object of a class is passed as an argument to a function's ellipsis, it almost always signals the presence of a bug. In the code above, it is the contents of the object that get to the stack, not the pointer to a string. Such code results in forming "gibberish" in the buffer or a crash. The code of CryEngine V uses a string class of its own, and it already has an appropriate method, c_str(). The fixed version:  LogError("....'%s'", m_fragName.c_str(); A few more suspicious fragments: • V510 The 'LogError' function is not expected to receive class-type variable as second actual argument. behaviortreenodes_core.cpp 1339 • V510 The 'Format' function is not expected to receive class-type variable as second actual argument. behaviortreenodes_core.cpp 2648 • V510 The 'CryWarning' function is not expected to receive class-type variable as sixth actual argument. crypak.cpp 3324 • V510 The 'CryWarning' function is not expected to receive class-type variable as fifth actual argument. crypak.cpp 3333 • V510 The 'CryWarning' function is not expected to receive class-type variable as fifth actual argument. shaderfxparsebin.cpp 4864 • V510 The 'CryWarning' function is not expected to receive class-type variable as fifth actual argument. shaderfxparsebin.cpp 4931 • V510 The 'Format' function is not expected to receive class-type variable as third actual argument. featuretester.cpp 1727 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; .... } This code is very strange. The programmer put a semicolon after the for loop, while the code formatting suggests that it should have a body. 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;jnIslands;j++) // <= line 3447 { .... for(j=0;j The project's code is full of other unsafe fragments; for example, there are cases of using one counter for both nested and outer loops. Large source files contain code with intricate formatting and fragments where the same variables are changed in different parts of the code - you just can't do without static analysis there! A few more strange loops: V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 1630, 1683. entity.cpp 1683 V535 The variable 'i1' is being used for this loop and for the outer loop. Check lines: 1521, 1576. softentity.cpp 1576 V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 2315, 2316. physicalentity.cpp 2316 V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 1288, 1303. shadercache.cpp 1303 V539 Consider inspecting iterators which are being passed as arguments to function 'erase'. frameprofilerender.cpp 1090 float CFrameProfileSystem::RenderPeaks() { .... std::vector& 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 suspected that the function handling a container would receive an iterator from another container. It's a wrong assumption, and there is no error here: the rPeaks variable is a reference to m_peaks. This code, however, may confuse not only the analyzer, but also other programmers who will maintain it. One shouldn't write code in a way like that. V713 The pointer pCollision was utilized in the logical expression before it was verified against nullptr in the same logical expression. 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 includes a rather lengthy conditional expression where the pCollision pointer is used multiple times. What is wrong about this code is that the pointer is tested for null at the very end, i.e. after multiple dereference operations. V758 The 'commandList' reference becomes invalid when smart pointer returned by a function is destroyed. 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); } .... } The commandList variable receives a reference to the value stored in a smart pointer. When this pointer destroys the object, the reference will become invalid. A few more issues of this type: V758 The 'commandList' reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 384 V758 The 'commandList' reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 368 V758 The 'commandList' reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 485 V758 The 'commandList' reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 553 Conclusion It costs almost nothing to fix bugs caught during the coding phase unlike those that get to the testers, while fixing bugs that have made it to the end users involves huge expenses. No matter what analyzer you use, the static analysis technology itself has long proved to be an extremely effective and efficient means to control the quality of program code and software products in general. Our collaboration with Epic Games has shown very well how integration of our analyzer into Unreal Engine 4 has benefited the project. We helped the developers in every aspect of analyzer integration and even fixed the bugs found in the project so that the developer team could continue scanning new code regularly on their own. We've shown that similar collaboration can be achieved with Crytek. Try PVS-Studio on your own C/C++/C# project!     0 Report Article   Sign in to follow this Followers 0 Go to articles General and Gameplay Programming   User Feedback 3 Comments 0 Reviews Xai 1851 Posted August 18, 2016 I wanted to let you know that multiple of early product advertisement / analysis articles didn't feel right. Not terrible useful, not terribly direct in being an add for the product from an outside who had no involvement with the code, and a bit preechy with too many "kinda not ideal code" cases being called errors. This article was different. It was full of useful examples, a useful dialog explaining them ... and should be useful if either the actual coding team who wrote the code read it, or any other C++ programmer looking for common coding mistakes. This article seems quite useful on its own, even without the product you sell (which also seems useful, but that's another topic). Good job sir! Share this comment Link to comment Share on other sites Shannon Barber 1683 Posted August 21, 2016 I believe a shift by a negative number is *unspecified* not *undefined*. Meaning it's well-formed but can behave differently on different machines. Share this comment Link to comment Share on other sites l0calh05t 1800 Posted September 27, 2016 I believe a shift by a negative number is *unspecified* not *undefined*. Meaning it's well-formed but can behave differently on different machines. Nope, it's undefined, see http://stackoverflow.com/questions/4945703/left-shifting-with-a-negative-shift-count Left shifts of negative numbers are also undefined. And right shift of negative numbers is implementation defined. As a general rule, don't shift signed integers or shift by signed integers. Just don't. Share this comment Link to comment Share on other sites Create an account or sign in to comment You need to be a member in order to leave a comment Create an account Sign up for a new account in our community. It's easy! Register a new account Sign in Already have an account? Sign in here. Sign In Now  •      Advertisement   Announcements GameDev.net needs your input - and you could win a$25 Amazon gift card! Click here to learn more. Advertisement GameDev.net and Intel Contest GameDev.net and Intel® have partnered up to bring GameDev.net members a gamedev contest running until December 21, 2018 - Submit your game for Intel® Certification and you could win big!Click here to learn more and submit your game. Latest Featured Articles 0 Improbable's SpatialOS GDK for Unity By GameDev.net 5 hours ago 2 The Faster You Unlearn OOP, The Better For You And Your Software By GameDev.net 5 hours ago 0 How to Implement Custom UI Meshes in Unity By Christopher Mielack December 2 1 Improve Your Game with References By GameDev.net November 26 0 Madsen's Musings Ep.10: 5 Things I Wish I'd Put In My Contract Sooner By nsmadsen November 19 Featured Blogs My Latest Game Engine From Scratch Demo By frankoguy in My C++ Game Engine from Scratch    0 Well now, I can't be the only one having fun! :D By Septopus in Unsettled World    0 Froggy Hop : Postmortem and some simple tricks I've put into it that I think went right. By DexterZ101 in Dexter-Z Game Development Journal    1 Starting Zone By DavinCreed in Dev Pirates: RIP    4 Next Challenge Poll! By khawk in Challenges    5 Advertisement Popular Now 9 Procedural Content Generation By NikitaSadkovStarted Saturday at 12:05 AM 11 Strange performance differences and crashes on different systems By MagoganStarted Friday at 03:13 PM 11 I'm new here ... is this site good for / where should I post? By jyujinkaiStarted Thursday at 09:10 PM 23 is it plausible to host your online game on your home computer/network? By minipongStarted Thursday at 02:59 AM 10 C++ structure of a text based football game in C++ By darrin kStarted Wednesday at 11:33 PM GameDev.net GameDev.net Articles GameDev.net Event Coverage GameDev.net Forums GameDev.net Blogs GameDev.net Gallery GameDev.net News GameDev.net Projects GDNet Chat All Activity Search In Everywhere This Category This Article More options... Find results that contain... All of my search term words Any of my search term words Find results in... Content titles and body Content titles only Home Articles Programming General and Gameplay Programming Long-Awaited Check of CryEngine V 
 
 
 × Existing user? Sign In Sign Up Browse Back Articles & Tutorials Back All Categories Audio Business Game Design Industry Programming Visual Arts Columns Back GameDev Unboxed Event Coverage Back All Events Game Developers Conference Power Up Digital Games Conference GameDev.Market Links News Podcasts Back All Podcasts Game Dev Loadout Archive Community Back Beginners Back Beginners Group Beginners Forum Beginners Resources Blogs Calendar Chat Forums Back All Forums Audio Business Game Design Programming Visual Arts Community GameDev Challenges Affiliates Topical Workshops Gallery Groups Back For Beginners GameDev Challenges All Groups Projects Back All Projects Games Game Assets Game Mods Developer Tools Store Forums Back All Forums For Beginners Audio Back Music and Sound FX Games Career Development Business Back Games Career Development Production and Management Games Business and Law Game Design Back Game Design and Theory Writing for Games Programming Back Artificial Intelligence Engines and Middleware General and Gameplay Programming Graphics and GPU Programming Math and Physics Networking and Multiplayer Visual Arts Back 2D and 3D Art Critique and Feedback Community Back GameDev Challenges GDNet Lounge GDNet Comments, Suggestions, and Ideas Coding Horrors Your Announcements Hobby Project Classifieds Indie Showcase Affiliates Back NeHe Productions AngelCode Topical Workshops Careers Back Contractors Hobby Projects Game Jobs Back Browse on GameDev.Jobs Post a Job Members Back Subscriptions Chat Guidelines Leaderboard Online Users Awards Search Back All Activity My Activity Streams Back Latest Topics Featured Blogs Search var ipsDebug = false; var CKEDITOR_BASEPATH = '//www.gamedev.net/applications/core/interface/ckeditor/ckeditor/'; var ipsSettings = { cookie_path: "/", cookie_prefix: "ips4_", cookie_ssl: true, upload_imgURL: "", message_imgURL: "", notification_imgURL: "", baseURL: "//www.gamedev.net/", jsURL: "//www.gamedev.net/applications/core/interface/js/js.php", csrfKey: "5b60abeacc92091b5d4856c8d2008263", antiCache: "14de000c45", disableNotificationSounds: false, useCompiledFiles: true, links_external: true, memberID: 0, analyticsProvider: "ga", viewProfiles: true, mapProvider: 'google', mapApiKey: "AIzaSyAeT7tk3vnWWmbgVISkLpbhkQvekG19rHM", }; ips.setSetting( 'date_format', jQuery.parseJSON('"mm\/dd\/yy"') ); ips.setSetting( 'date_first_day', jQuery.parseJSON('0') ); ips.setSetting( 'remote_image_proxy', jQuery.parseJSON('1') ); ips.setSetting( 'ipb_url_filter_option', jQuery.parseJSON('"none"') ); ips.setSetting( 'url_filter_any_action', jQuery.parseJSON('"allow"') ); ips.setSetting( 'bypass_profanity', jQuery.parseJSON('0') ); ips.setSetting( 'emoji_style', jQuery.parseJSON('"native"') ); ips.setSetting( 'emoji_shortcodes', jQuery.parseJSON('"1"') ); ips.setSetting( 'emoji_ascii', jQuery.parseJSON('"1"') ); ips.setSetting( 'emoji_cache', jQuery.parseJSON('"1"') ); ips.setSetting( 'quickSearchDefault', jQuery.parseJSON('"all"') ); ips.setSetting( 'quickSearchMinimum', jQuery.parseJSON('3') ); ips.setSetting( 'quickSearchShowAdv', jQuery.parseJSON('true') ); ips.setSetting( 'quickSearchIn', jQuery.parseJSON('"title"') ); { "@context": "http://schema.org", "@type": "Article", "url": "https://www.gamedev.net/articles/programming/general-and-gameplay-programming/long-awaited-check-of-cryengine-v-r4476/", "discussionUrl": "https://www.gamedev.net/articles/programming/general-and-gameplay-programming/long-awaited-check-of-cryengine-v-r4476/", "mainEntityOfPage": "https://www.gamedev.net/articles/programming/general-and-gameplay-programming/long-awaited-check-of-cryengine-v-r4476/", "name": "Long-Awaited Check of CryEngine V", "headline": "Long-Awaited Check of CryEngine V", "text": "In May 2016, German game-development company Crytek made a decision to upload the source code of their game engine CryEngine V to Github. The engine is written in C++ and has immediately attracted attention of both the open-source developer community and the team of developers of PVS-Studio static analyzer who regularly scan the code of open-source projects to estimate its quality. A lot of great games were created by a number of video-game development studios using various versions of CryEngine, and now the engine has become available to even more developers. This article gives an overview of errors found in the project by PVS-Studio static analyzer.\n\nIntroduction\n\nCryEngine is a game engine developed by German company Crytek in 2002 and originally used in first-person shooter Far Cry. A lot of great games were created by a number of video-game development studios using various licensed versions of CryEngine: 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 a release date for their new engine CryEngine V and uploaded its source code to Github soon after.\n\nThe project\u0027s source code was checked by PVS-Studio static analyzer, version 6.05. This is a tool designed for detecting software errors in program source code in C, C++, and C#. The only true way of using static analysis is to regularly scan code on developers\u0027 computers and build-servers. However, in order to demonstrate PVS-Studio\u0027s diagnostic capabilities, we run single-time checks of open-source projects and then write articles about errors found. If we like a project, we might scan it again a couple of years later. Such recurring checks are in fact the same as single-time checks since the code accumulates a lot of changes during that time.\n\nFor our checks, we pick projects that are simply popular and wide-known as well as projects suggested by our readers via e-mail. That\u0027s why CryEngine V was by no means the first game engine among those scanned by our analyzer. Other engines that we have already checked include:\n\n\nUnreal Engine 4 (first check, second check, third check) \n Check of Godot Engine \n Check of Serious Engine \n Check of X-Ray Engine \n Check of Xenko Engine \n\nWe also checked CryEngine 3 SDK once.\n\nI\u0027d like to elaborate on the check of Unreal Engine 4 engine in particular. Using that project as an example allowed us to demonstrate in every detail what the right way of using static analysis on a real project should look like, covering the whole process from the phase of integrating the analyzer into the project to the phase of cutting warnings to zero with subsequent control over bug elimination in new code. Our work on Unreal Engine 4 project developed into collaboration with Epic Games company, in terms of which our team fixed all the defects found in the engine\u0027s source code and wrote a joint article with Epic Games on the accomplished work (it was posted on Unreal Engine Blog). Epic Games also purchased a PVS-Studio license to be able to maintain the quality of their code on their own. Collaboration of this kind is something that we would like to try with Crytek, too.\n\nAnalyzer-report structure\n\nIn this article, I\u0027d like to answer a few frequently asked questions concerning the number of warnings and false positives, for example, \"What is the ratio of false positives?\" or \"Why are there so few bugs in so large a project?\"\n\nTo begin with, all PVS-Studio warnings are classified into three severity levels: High, Medium, and Low. The High level holds the most critical warnings, which are almost surely real errors, while the Low level contains the least critical warnings or warnings that are very likely to be false positives. Keep in mind that the codes of errors do not tie them firmly to particular severity levels: distribution of warnings across the levels very much depends on the context.\n\nThis is how the warnings of the General Analysis module are distributed across the severity levels for CryEngine V project:\n\nHigh: 576 warnings; \n Medium: 814 warnings, \n Low: 2942 warnings. \n\nFigure 1 shows distribution of the warnings across the levels in the form of a pie chart.\n\n\n\nFigure 1 - Percentage distribution of warnings across severity levels\n\nIt is impossible to include all the warning descriptions and associated code fragments in an article. Our articles typically discuss 10-40 commented cases; some warnings are given as a list; and most have to be left unexamined. In the best-case scenario, project authors, after we inform them, ask for a complete analysis report for close study. The bitter truth is that in most cases the number of High-level warnings alone is more than enough for an article, and CryEngine V is no exception. Figure 2 shows the structure of the High-level warnings issued for this project.\n\n\n\nFigure 2 - Structure of High-level warnings\n\nLet\u0027s take a closer look at the sectors of this chart:\n\nDescribed in the article (6%) - warnings cited in the article and accompanied by code fragments and commentary. \n Presented as a list (46%) - warnings cited as a list. These warnings refer to the same pattern as some of the errors already discussed, so only the warning text is given. \n False Positives (8%) - a certain ratio of false positives we have taken into account for future improvement of the analyzer. \n Other (40%) - all the other warnings issued. These include warnings that we had to leave out so that the article wouldn\u0027t grow too large, non-critical warnings, or warnings whose validity could be estimated only by a member of the developer team. As our experience of working on Unreal Engine 4 has shown, such code still \"smells\" and those warnings get fixed anyway. \n\nAnalysis results\n\nAnnoying copy-paste\n\n\n\nV501 There are identical sub-expressions to the left and to the right of the \u0027-\u0027 operator: q2.v.z - q2.v.z entitynode.cpp 93\n\n\nbool\nCompareRotation(const Quat\u0026amp; q1, const Quat\u0026amp; q2, float epsilon)\n{\n return (fabs_tpl(q1.v.x - q2.v.x) \n\nA mistyped digit is probably one of the most annoying typos one can make. In the function above, the analyzer detected a suspicious expression, (q2.v.z - q2.v.z), where variables q1 and q2 seem to have been mixed up.\n\nV501 There are identical sub-expressions \u0027(m_eTFSrc == eTF_BC6UH)\u0027 to the left and to the right of the \u0027||\u0027 operator. texturestreaming.cpp 919\n\n\n//! Texture formats.\nenum ETEX_Format : uint8\n{\n ....\n eTF_BC4U, //!\n\nAnother kind of typos deals with copying of constants. In this case, the m_eTFSrc variable is compared twice with the eTF_BC6UH constant. The second of these checks must compare the variable with some other constant whose name differs from the copied one in just one character, for example, eTF_BC6SH.\n\nTwo more similar issues:\n\n\nV501 There are identical sub-expressions \u0027(td.m_eTF == eTF_BC6UH)\u0027 to the left and to the right of the \u0027||\u0027 operator. texture.cpp 1214 \n V501 There are identical sub-expressions \u0027geom_colltype_solid\u0027 to the left and to the right of the \u0027|\u0027 operator. attachmentmanager.cpp 1004 \n\nV517 The use of \u0027if (A) {...} else if (A) {...}\u0027 pattern was detected. There is a probability of logical error presence. Check lines: 266, 268. d3dhwshader.cpp 266\n\n\nint SD3DShader::Release(EHWShaderClass eSHClass, int nSize)\n{\n ....\n if (eSHClass == eHWSC_Pixel)\n return ((ID3D11PixelShader*)pHandle)-\u0026gt;Release();\n else if (eSHClass == eHWSC_Vertex)\n return ((ID3D11VertexShader*)pHandle)-\u0026gt;Release();\n else if (eSHClass == eHWSC_Geometry) // Release(); // Release(); // Release();\n else if (eSHClass == eHWSC_Compute)\n return ((ID3D11ComputeShader*)pHandle)-\u0026gt;Release();\n else if (eSHClass == eHWSC_Domain)\n return ((ID3D11DomainShader*)pHandle)-\u0026gt;Release()\n ....\n}\n\nThis is an example of lazy copying of a cascade of conditional statements, one of which was left unchanged.\n\nV517 The use of \u0027if (A) {...} else if (A) {...}\u0027 pattern was detected. There is a probability of logical error presence. Check lines: 970, 974. environmentalweapon.cpp 970\n\n\nvoid CEnvironmentalWeapon::UpdateDebugOutput() const\n{\n ....\n const char* attackStateName = \"None\";\n if(m_currentAttackState \u0026amp; // \n\nIn the previous example, there was at least a small chance that an extra condition resulted from making too many copies of a code fragment, while the programmer simply forgot to remove one of the checks. In this code, however, the attackStateName variable will never take the value \"Charged Throw\" because of identical conditional expressions.\n\nV519 The \u0027BlendFactor[2]\u0027 variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1265, 1266. ccrydxgldevicecontext.cpp 1266\n\n\nvoid CCryDXGLDeviceContext::\nOMGetBlendState(...., FLOAT BlendFactor[4], ....)\n{\n CCryDXGLBlendState::ToInterface(ppBlendState, m_spBlendState);\n if ((*ppBlendState) != NULL)\n (*ppBlendState)-\u0026gt;AddRef();\n BlendFactor[0] = m_auBlendFactor[0];\n BlendFactor[1] = m_auBlendFactor[1];\n BlendFactor[2] = m_auBlendFactor[2]; // \n\nIn this function, a typo in the element index prevents the element with index \u00273\u0027, BlendFactor[3], from being filled with a value. This fragment would have remained just one of the many interesting examples of typos, had not the analyzer found two more copies of the same incorrect fragment:\n\nV519 The \u0027m_auBlendFactor[2]\u0027 variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 904, 905. ccrydxgldevicecontext.cpp 905\n\n\nvoid CCryDXGLDeviceContext::\n OMSetBlendState(....const FLOAT BlendFactor[4], ....)\n{\n ....\n m_uSampleMask = SampleMask;\n if (BlendFactor == NULL)\n {\n m_auBlendFactor[0] = 1.0f;\n m_auBlendFactor[1] = 1.0f;\n m_auBlendFactor[2] = 1.0f; // SetBlendColor(m_auBlendFactor[0],\n m_auBlendFactor[1],\n m_auBlendFactor[2],\n m_auBlendFactor[3]);\n m_pContext-\u0026gt;SetSampleMask(m_uSampleMask);\n ....\n}\n\nHere\u0027s that fragment where the element with index \u00273\u0027 is skipped again. I even thought for a moment that there was some intentional pattern to it, but this thought quickly vanished as I saw that the programmer attempted to access all the four elements of the m_auBlendFactor array at the end of the function. It looks like the same code with a typo was simply copied several times in the file ccrydxgldevicecontext.cpp.\n\nV523 The \u0027then\u0027 statement is equivalent to the \u0027else\u0027 statement. d3dshadows.cpp 1410\n\n\nvoid CD3D9Renderer::ConfigShadowTexgen(....)\n{\n ....\n if ((pFr-\u0026gt;m_Flags \u0026amp; DLF_DIRECTIONAL) ||\n (!(pFr-\u0026gt;bUseHWShadowMap) \u0026amp;\u0026amp; !(pFr-\u0026gt;bHWPCFCompare)))\n {\n //linearized shadows are used for any kind of directional\n //lights and for non-hw point lights\n m_cEF.m_TempVecs[2][Num] = 1.f / (pFr-\u0026gt;fFarDist);\n }\n else\n {\n //hw point lights sources have non-linear depth for now\n m_cEF.m_TempVecs[2][Num] = 1.f / (pFr-\u0026gt;fFarDist);\n }\n ....\n}\n\nTo finish the section on copy-paste, here is one more interesting error. No matter what result the conditional expression produces, the value m_cEF.m_TempVecs[2][Num] is always computed by the same formula. Judging by the surrounding code, the index is correct: it\u0027s exactly the element with index \u00272\u0027 that must be filled with a value. It\u0027s just that the formula itself was meant to be different in each case, and the programmer forgot to change the copied code.\n\nTroubles with initialization\n\n\n\nV546 Member of a class is initialized by itself: \u0027eConfigMax(eConfigMax)\u0027. particleparams.h 1013\n\n\nParticleParams() :\n ....\n fSphericalApproximation(1.f),\n fVolumeThickness(1.0f),\n fSoundFXParam(1.f),\n eConfigMax(eConfigMax.VeryHigh), // \n\nThe analyzer detected a potential typo that causes a class field to be initialized to its own value.\n\nV603 The object was created but it is not being used. If you wish to call constructor, \u0027this-\u0026gt;SRenderingPassInfo::SRenderingPassInfo(....)\u0027 should be used. i3dengine.h 2589\n\nSRenderingPassInfo()\n : pShadowGenMask(NULL)\n , nShadowSide(0)\n , nShadowLod(0)\n , nShadowFrustumId(0)\n , m_bAuxWindow(0)\n , m_nRenderStackLevel(0)\n , m_eShadowMapRendering(static_cast(SHADOW_MAP_NONE))\n , m_bCameraUnderWater(0)\n , m_nRenderingFlags(0)\n , m_fZoomFactor(0.0f)\n , m_pCamera(NULL)\n , m_nZoomInProgress(0)\n , m_nZoomMode(0)\n , m_pJobState(nullptr)\n{\n threadID nThreadID = 0;\n gEnv-\u0026gt;pRenderer-\u0026gt;EF_Query(EFQ_MainThreadList, nThreadID);\n m_nThreadID = static_cast(nThreadID);\n m_nRenderFrameID = gEnv-\u0026gt;pRenderer-\u0026gt;GetFrameID();\n m_nRenderMainFrameID = gEnv-\u0026gt;pRenderer-\u0026gt;GetFrameID(false);\n}\n \nSRenderingPassInfo(threadID id)\n{\n SRenderingPassInfo(); // \n\nIn this code, incorrect use of constructor was detected. The programmer probably assumed that calling a constructor in a way like that - without parameters - inside another constructor would initialize the class fields, but this assumption was wrong.\n\nInstead, a new unnamed object of type SRenderingPassInfo will be created and immediately destroyed. The class fields, therefore, will remain uninitialized. One way to fix this error is to create a separate initialization function and call it from different constructors.\n\nV688 The \u0027m_cNewGeomMML\u0027 local variable possesses the same name as one of the class members, which can result in a confusion. terrain_node.cpp 344\n\n\nvoid CTerrainNode::Init(....)\n{\n ....\n m_nOriginX = m_nOriginY = 0; // sector origin\n m_nLastTimeUsed = 0; // basically last time rendered\n\n uint8 m_cNewGeomMML = m_cCurrGeomMML = m_cNewGeomMML_Min ....\n\n m_pLeafData = 0;\n\n m_nTreeLevel = 0;\n ....\n}\n\nThe name of the local variable cNewGeomMML coincides with that of a class field. It\u0027s usually not an error, but in this particular case it does look strange in comparison to how the other class fields are initialized.\n\nV575 The \u0027memset\u0027 function processes \u00270\u0027 elements. Inspect the third argument. crythreadutil_win32.h 294\n\nvoid EnableFloatExceptions(....)\n{\n ....\n CONTEXT ctx;\n memset(\u0026amp;ctx, sizeof(ctx), 0); // \n\nThis error is a very interesting one. When calling the memset() function, two arguments were swapped by mistake, which resulted in calling the function to fill 0 bytes. This is the function prototype:\n\nvoid * memset ( void * ptr, int value, size_t num );\n\nThe function expects to receive the buffer size as the third argument and the value the buffer is to be filled with as the second.\n\nThe fixed version:\n\n\nvoid EnableFloatExceptions(....)\n{\n ....\n CONTEXT ctx;\n memset(\u0026amp;ctx, 0, sizeof(ctx));\n ....\n}\n\nV630 The \u0027_alloca\u0027 function is used to allocate memory for an array of objects which are classes containing constructors. command_buffer.cpp 62\n\nvoid CBuffer::Execute()\n{\n ....\n QuatT * pJointsTemp = static_cast(\n alloca(m_state.m_jointCount * sizeof(QuatT)));\n ....\n}\n\nIn some parts of the project\u0027s code, the alloca() function is used to allocate memory for an array of objects. In the example above, with memory allocated in such a way, neither the constructor, nor the destructor will be called for objects of class QuatT. This defect may result in handling uninitialized variables, and other errors.\n\nHere\u0027s a complete list of other defects of this type:\n\n\nV630 The \u0027_alloca\u0027 function is used to allocate memory for an array of objects which are classes containing constructors. command_buffer.cpp 67 \n V630 The \u0027_alloca\u0027 function is used to allocate memory for an array of objects which are classes containing constructors. posematching.cpp 144 \n V630 The \u0027_alloca\u0027 function is used to allocate memory for an array of objects which are classes containing constructors. characterinstance.cpp 280 \n V630 The \u0027_alloca\u0027 function is used to allocate memory for an array of objects which are classes containing constructors. characterinstance.cpp 282 \n V630 The \u0027_alloca\u0027 function is used to allocate memory for an array of objects which are classes containing constructors. scriptbind_entity.cpp 6252 \n V630 The \u0027_alloca\u0027 function is used to allocate memory for an array of objects which are classes containing constructors. jobmanager.cpp 1016 \n V630 The \u0027_alloca\u0027 function is used to allocate memory for an array of objects which are classes containing constructors. driverd3d.cpp 5859 \n\nV583 The \u0027?:\u0027 operator, regardless of its conditional expression, always returns one and the same value: -1.8f. posealignerc3.cpp 330\n\n\nILINE bool InitializePoseAlignerPinger(....)\n{\n ....\n chainDesc.offsetMin = Vec3(0.0f, 0.0f, bIsMP ? -1.8f : -1.8f);\n chainDesc.offsetMax = Vec3(0.0f, 0.0f, bIsMP ? +0.75f : +1.f);\n ....\n}\n\nA few fragments were found where the ternary operator ?: returns one and the same value. While in the previous example it could have been done for aesthetic reasons, the reason for doing so in the following fragment is unclear.\n\nfloat predictDelta = inputSpeed \u0026lt; 0.0f ? 0.1f : 0.1f; // \u0026lt;=\nfloat dict = angle + predictDelta * ( angle - m_prevAngle) / dt ;\n\nA complete list of other defects of this type:\n\nV583 The \u0027?:\u0027 operator, regardless of its conditional expression, always returns one and the same value: -1.8f. posealignerc3.cpp 313 \n V583 The \u0027?:\u0027 operator, regardless of its conditional expression, always returns one and the same value: -2.f. posealignerc3.cpp 347 \n V583 The \u0027?:\u0027 operator, regardless of its conditional expression, always returns one and the same value: D3D11_RTV_DIMENSION_TEXTURE2DARRAY. d3dtexture.cpp 2277 \n V583 The \u0027?:\u0027 operator, regardless of its conditional expression, always returns one and the same value: 255U. renderer.cpp 3389 \n V583 The \u0027?:\u0027 operator, regardless of its conditional expression, always returns one and the same value: D3D12_RESOURCE_STATE_GENERIC_READ. dx12device.cpp 151 \n V583 The \u0027?:\u0027 operator, regardless of its conditional expression, always returns one and the same value: 0.1f. vehiclemovementstdboat.cpp 720 \n\nV570 The \u0027runtimeData.entityId\u0027 variable is assigned to itself. behaviortreenodes_ai.cpp 1771\n\n\nvoid ExecuteEnterScript(RuntimeData\u0026amp; runtimeData)\n{\n ExecuteScript(m_enterScriptFunction, runtimeData.entityId);\n\n runtimeData.entityId = runtimeData.entityId; // \n\nA variable is assigned to itself, which doesn\u0027t look right. The authors should check this code.\n\nOperation precedence\n\n\n\nV502 Perhaps the \u0027?:\u0027 operator works in a different way than it was expected. The \u0027?:\u0027 operator has a lower priority than the \u0027+\u0027 operator. gpuparticlefeaturespawn.cpp 79\n\nbool HasDuration() { return m_useDuration; }\n\nvoid CFeatureSpawnRate::SpawnParticles(....)\n{\n ....\n SSpawnData\u0026amp; spawn = pRuntime-\u0026gt;GetSpawnData(i);\n const float amount = spawn.amount;\n const int spawnedBefore = int(spawn.spawned);\n const float endTime = spawn.delay +\n HasDuration() ? spawn.duration : fHUGE;\n ....\n}\n\nThe function above seems to measure time in a wrong way. The precedence of the addition operator is higher than that of the ternary operator :?, so the value 0 or 1 is added to spawn.delay first, and then the value spawn.duration or fHUGE is written into the endTime variable. This error is quite a common one. To learn more about interesting patterns of errors involving operation precedence collected from the PVS-Studio bug database, see my article: Logical Expressions in C/C++. Mistakes Made by Professionals.\n\nV634 The priority of the \u0027*\u0027 operation is higher than that of the \u0027\n\nenum joint_flags\n{\n angle0_locked = 1,\n ....\n};\n\nbool CDefaultSkeleton::SetupPhysicalProxies(....)\n{\n ....\n for (int j = 0; .... ; j++)\n {\n // lock axes with 0 limits range\n m_arrModelJoints....flags |= (....) * angle0_locked \n\nThis is another very interesting error that has to do with the precedence of the multiplication and bitwise shift operations. The latter has lower precedence, so the whole expression is multiplied by one at each iteration (as the angle0_locked constant has the value one), which looks very strange.\n\nThis is what the programmer must have wanted that code to look like:\n\nm_arrModelJoints....flags |= (....) * (angle0_locked \u0026lt;\u0026lt; j);\n\nThe following file contains a list of 35 suspicious fragments involving precedence of shift operations: CryEngine5_V634.txt.\n\nUndefined behavior\n\nUndefined behavior is the result of executing computer code written in a certain programming language that depends on a number of random factors such as memory state or triggered interrupts. In other words, this result is not prescribed by the language specification. It is considered to be an error to let such a situation occur in your program. Even if it can successfully execute on some compiler, it is not guaranteed to be cross-platform and may fail on another machine, operating system, and even other settings of the same compiler.\n\n\n\nV610 Undefined behavior. Check the shift operator \u0027\n\n#ifndef physicalplaceholder_h\n#define physicalplaceholder_h\n#pragma once\n....\nconst int NO_GRID_REG = -1\n\nUnder the modern C++ standard, a left shift of a negative value is undefined behavior. The analyzer found a few more similar issues in CryEngine V\u0027s code:\n\nV610 Undefined behavior. Check the shift operator \u0027\n \nV610 Undefined behavior. Check the shift operator \u0027\n \nV610 Undefined behavior. Check the shift operator \u0027\n \nV610 Undefined behavior. Check the shift operator \u0027\n \nV610 Undefined behavior. Check the shift operator \u0027\n \nV610 Undefined behavior. Check the shift operator \u0027\n \nV610 Undefined behavior. Check the shift operator \u0027\n \nV610 Undefined behavior. Check the shift operator \u0027\n \n\nV567 Undefined behavior. The \u0027m_current\u0027 variable is modified while being used twice between sequence points. operatorqueue.cpp 105\n\nbool COperatorQueue::Prepare(....)\n{\n ++m_current \u0026amp;= 1;\n m_ops[m_current].clear();\n return true;\n}\n\nThe analyzer detected an expression that causes undefined behavior. A variable is used multiple times between two sequence points, while its value changes. The result of executing such an expression, therefore, can\u0027t be determined.\n\nOther similar issues:\n\nV567 Undefined behavior. The \u0027itail\u0027 variable is modified while being used twice between sequence points. trimesh.cpp 3101 \n V567 Undefined behavior. The \u0027ihead\u0027 variable is modified while being used twice between sequence points. trimesh.cpp 3108 \n V567 Undefined behavior. The \u0027ivtx\u0027 variable is modified while being used twice between sequence points. boolean3d.cpp 1194 \n V567 Undefined behavior. The \u0027ivtx\u0027 variable is modified while being used twice between sequence points. boolean3d.cpp 1202 \n V567 Undefined behavior. The \u0027ivtx\u0027 variable is modified while being used twice between sequence points. boolean3d.cpp 1220 \n V567 Undefined behavior. The \u0027m_commandBufferIndex\u0027 variable is modified while being used twice between sequence points. xconsole.cpp 180 \n V567 Undefined behavior. The \u0027m_FrameFenceCursor\u0027 variable is modified while being used twice between sequence points. ccrydx12devicecontext.cpp 952 \n V567 Undefined behavior. The \u0027m_iNextAnimIndex\u0027 variable is modified while being used twice between sequence points. hitdeathreactionsdefs.cpp 192 \n\nErrors in conditions\n\n\n\nV579 The memcmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. graphicspipelinestateset.h 58\n\nbool\noperator==(const SComputePipelineStateDescription\u0026amp; other) const\n{\n return 0 == memcmp(this, \u0026amp;other, sizeof(this)); // \n\nThe programmer made a mistake in the equality operation in the call to the memcmp() function, which leads to passing the pointer size instead of the object size as a function argument. As a result, only the first several bytes of the objects are compared.\n\nThe fixed version:\n\nmemcmp(this, \u0026amp;other, sizeof(*this));\nUnfortunately, three more similar issues were found in the project:\n\nV579 The memcpy function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. geomcacherendernode.cpp 286 \n V579 The AddObject function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. clipvolumemanager.cpp 145 \n V579 The memcmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. graphicspipelinestateset.h 34 \n\nV640 The code\u0027s 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\n\nCLivingEntity::~CLivingEntity()\n{\n for(int i=0;i.pPhysGeom || ....)\n delete[] m_parts.pMatMapping; m_parts.pMatMapping=0;\n }\n ....\n}\n\nI spotted a huge number of code blocks with statements written in one line. These include not only ordinary assignments, but rather loops, conditions, function calls, and sometimes a mixture of all of these (see Figure 3).\n\n\n\nFigure 3 - Poor code formatting\n\nIn code of size like that, this programming style almost inevitably leads to errors. In the example above, the memory block occupied by an array of objects was to be freed and the pointer was to be cleared when a certain condition was met. However, incorrect code formatting causes the m_parts.pMatMapping pointer to be cleared at every loop iteration. The implications of this problem can\u0027t be predicted, but the code does look strange.\n\nOther fragments with strange formatting:\n\nV640 The code\u0027s operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. physicalworld.cpp 2449 \n V640 The code\u0027s operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. articulatedentity.cpp 1723 \n V640 The code\u0027s operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. articulatedentity.cpp 1726 \n\nV695 Range intersections are possible within conditional expressions. Example: if (A \n\n\nbool CStatObj::RenderDebugInfo(....)\n{\n ....\n ColorB clr(0, 0, 0, 0);\n if (nRenderMats == 1)\n clr = ColorB(0, 0, 255, 255);\n else if (nRenderMats == 2)\n clr = ColorB(0, 255, 255, 255);\n else if (nRenderMats == 3)\n clr = ColorB(0, 255, 0, 255);\n else if (nRenderMats == 4)\n clr = ColorB(255, 0, 255, 255);\n else if (nRenderMats == 5)\n clr = ColorB(255, 255, 0, 255);\n else if (nRenderMats \u0026gt;= 6) // = 11) // \n\nThe programmer made a mistake that prevents the color ColorB(255, 255, 255, 255) from ever being selected. The values nRenderMats are first compared one by one with the numbers from 1 to 5, but when comparing them with value ranges, the programmer didn\u0027t take into account that values larger than 11 already belong to the range of values larger than 6, so the last condition will never execute.\n\nThis cascade of conditions was copied in full into one more fragment:\nV695 Range intersections are possible within conditional expressions. Example: if (A \n \nV695 Range intersections are possible within conditional expressions. Example: if (A \n\nenum eNodeConstants\n{\n ....\n CHILDBLOCKS_MAX_DIST_FOR_8BITS = BIT(7) - 1, // 127\n CHILDBLOCKS_MAX_DIST_FOR_16BITS = BIT(6) - 1, // 63\n ....\n};\n\nvoid CNodeLiveWriter::Compact()\n{\n ....\n if (dist \n\nA similar mistake inside a condition was also found in the fragment above, except that this time the code that fails to get control is larger. The values of the constants CHILDBLOCKS_MAX_DIST_FOR_8BITS and CHILDBLOCKS_MAX_DIST_FOR_16BITS are such that the second condition will never be true.\n\nV547 Expression \u0027pszScript[iSrcBufPos] != \u0027==\u0027\u0027 is always true. The value range of char type: [-128, 127]. luadbg.cpp 716\n\nbool CLUADbg::LoadFile(const char* pszFile, bool bForceReload)\n{\n FILE* hFile = NULL;\n char* pszScript = NULL, * pszFormattedScript = NULL;\n ....\n while (pszScript[iSrcBufPos] != \u0027 \u0027 \u0026amp;\u0026amp;\n ....\n pszScript[iSrcBufPos] != \u0027=\u0027 \u0026amp;\u0026amp;\n pszScript[iSrcBufPos] != \u0027==\u0027 \u0026amp;\u0026amp; // \n\nA large conditional expression contains a subexpression that is always true. The \u0027==\u0027 literal will have type int and correspond to the value 15677. The pszScript array consists of elements of type char, and a value of type char can\u0027t be equal to 15677, so the pszScript[iSrcBufPos] != \u0027==\u0027 expression is always true.\n\nV734 An excessive expression. Examine the substrings \"_ddn\" and \"_ddna\". texture.cpp 4212\n\nvoid CTexture::PrepareLowResSystemCopy(byte* pTexData, ....)\n{\n ....\n // make sure we skip non diffuse textures\n if (strstr(GetName(), \"_ddn\") // \n\nThe strstr() function looks for the first occurrence of the specified substring within another string and returns either a pointer to the first occurrence or an empty pointer. The string \"_ddn\" is the first to be searched, and \"_ddna\" is the second, which means that the condition will be true if the shorter string is found. This code might not work as expected; or perhaps this expression is redundant and could be simplified by removing the extra check.\n\nV590 Consider inspecting this expression. The expression is excessive or contains a misprint. goalop_crysis2.cpp 3779\n\nvoid COPCrysis2FlightFireWeapons::ParseParam(....)\n{\n ....\n else if (!paused \u0026amp;\u0026amp;\n (m_State == eFP_PAUSED) \u0026amp;\u0026amp; // \n\nThe conditional expression in the ParseParam() function is written in such a way that its result does not depend on the (m_State != eFP_PAUSED_OVERRIDE) subexpression.\n\nHere\u0027s a simpler example:\n\nif ( err == code1 \u0026amp;\u0026amp; err != code2)\n{\n ....\n}\nThe result of the whole conditional expression does not depend on the result of the (err != code2) subexpression, which can be clearly seen from the truth table for this example (see Figure 4)\n\n\n\nFigure 4 - Truth table for a logical expression\n\nComparing unsigned values with zero\n\n\n\nWhen scanning projects, we often come across comparisons of unsigned values with zero, which produce either true or false every time. Such code does not always contain a critical bug; it is often a result of too much caution or changing a variable\u0027s type from signed to unsigned. Anyway, such comparisons need to be checked.\n\nV547 Expression \u0027m_socket \n\ntypedef SOCKET CRYSOCKET;\n// Internal socket data\nCRYSOCKET m_socket;\n\nbool CServiceNetworkConnection::TryReconnect()\n{\n ....\n // Create new socket if needed\n if (m_socket == 0)\n {\n m_socket = CrySock::socketinet();\n if (m_socket \n\nI\u0027d like to elaborate on the SOCKET type. It can be both signed and unsigned depending on the platforms, so it is strongly recommended that you use special macros and constants specified in the standard headers when working with this type.\n\nIn cross-platform projects, comparisons with 0 or -1 are common that result in misinterpretation of error codes. CryEngine V project is no exception, although some checks are done correctly, for example:\n\nif (m_socket == CRY_INVALID_SOCKET)\n\nNevertheless, many parts of the code use different versions of these checks.\n\nSee the file CryEngine5_V547.txt for other 47 suspicious comparisons of unsigned variables with zero. The code authors need to check these warnings.\n\nDangerous pointers\n\n\n\nDiagnostic V595 detects pointers that are tested for null after they have been dereferenced. In practice, this diagnostic catches very tough bugs. On rare occasions, it issues false positives, which is explained by the fact that pointers are checked indirectly, i.e. through one or several other variables, but figuring such code out isn\u0027t an easy task for a human either, is it? Three code samples are given below that trigger this diagnostic and look especially surprising, as it\u0027s not clear why they work at all. For the other warnings of this type see the file CryEngine5_V595.txt.\nExample 1\n\nV595 The \u0027m_pPartManager\u0027 pointer was utilized before it was verified against nullptr. Check lines: 1441, 1442. 3denginerender.cpp 1441\n\nvoid C3DEngine::RenderInternal(....)\n{\n ....\n m_pPartManager-\u0026gt;GetLightProfileCounts().ResetFrameTicks();\n if (passInfo.IsGeneralPass() \u0026amp;\u0026amp; m_pPartManager)\n m_pPartManager-\u0026gt;Update();\n ....\n}\n\nThe m_pPartManager pointer is dereferenced and then checked.\n\nExample 2\n\nV595 The \u0027gEnv-\u0026gt;p3DEngine\u0027 pointer was utilized before it was verified against nullptr. Check lines: 1477, 1480. gameserialize.cpp 1477\n\nbool CGameSerialize::LoadLevel(....)\n{\n ....\n // can quick-load\n if (!gEnv-\u0026gt;p3DEngine-\u0026gt;RestoreTerrainFromDisk())\n return false;\n\n if (gEnv-\u0026gt;p3DEngine)\n {\n gEnv-\u0026gt;p3DEngine-\u0026gt;ResetPostEffects();\n }\n ....\n}\n\nThe gEnv-\u0026gt;p3DEngine pointer is dereferenced and then checked.\n\nExample 3\n\nV595 The \u0027pSpline\u0027 pointer was utilized before it was verified against nullptr. Check lines: 158, 161. facechannelkeycleanup.cpp 158\n\nvoid FaceChannel::CleanupKeys(....)\n{\n\n CFacialAnimChannelInterpolator backupSpline(*pSpline);\n\n // Create the key entries array.\n int numKeys = (pSpline ? pSpline-\u0026gt;num_keys() : 0);\n ....\n}\n\nThe pSpline pointer is dereferenced and then checked.\n\nMiscellaneous\n\n\n\nV622 Consider inspecting the \u0027switch\u0027 statement. It\u0027s possible that the first \u0027case\u0027 operator is missing. mergedmeshrendernode.cpp 999\n\nstatic inline void ExtractSphereSet(....)\n{\n ....\n switch (statusPos.pGeom-\u0026gt;GetType())\n {\n if (false)\n {\n case GEOM_CAPSULE:\n statusPos.pGeom-\u0026gt;GetPrimitive(0, \u0026amp;cylinder);\n }\n if (false)\n {\n case GEOM_CYLINDER:\n statusPos.pGeom-\u0026gt;GetPrimitive(0, \u0026amp;cylinder);\n }\n for (int i = 0; i \n\nThis fragment is probably the strangest of all found in CryEngine V. Whether or not the case label will be selected does not depend on the if statement, even in case of if (false). In the switch statement, an unconditional jump to the label occurs if the condition of the switch statement is met. Without the break statement, one could use such code to \"bypass\" irrelevant statements, but, again, maintaining such obscure code isn\u0027t easy. One more question is, why does the same code execute when jumping to the labels GEOM_CAPSULE and GEOM_CYLINDER?\n\nV510 The \u0027LogError\u0027 function is not expected to receive class-type variable as second actual argument. behaviortreenodes_action.cpp 143\n\ntypedef CryStringT\u0026lt;char\u0026gt; string;\n// The actual fragment name.\nstring m_fragName;\n//! cast to C string.\nconst value_type* c_str() const { return m_str; }\nconst value_type* data() const { return m_str; };\n \nvoid LogError(const char* format, ...) const\n{ .... }\n \nvoid QueueAction(const UpdateContext\u0026amp; context)\n{\n ....\n ErrorReporter(*this, context).LogError(\"....\u0027%s\u0027\", m_fragName);\n ....\n}\n\nWhen it is impossible to specify the number and types of all acceptable parameters to a function, one puts ellipsis (...) at the end of the list of parameters in the function declaration, which means \"and perhaps a few more\". Only POD (Plain Old Data) types can be used as actual parameters to the ellipsis. If an object of a class is passed as an argument to a function\u0027s ellipsis, it almost always signals the presence of a bug. In the code above, it is the contents of the object that get to the stack, not the pointer to a string. Such code results in forming \"gibberish\" in the buffer or a crash. The code of CryEngine V uses a string class of its own, and it already has an appropriate method, c_str().\n\nThe fixed version:\n\nLogError(\"....\u0027%s\u0027\", m_fragName.c_str();\n\nA few more suspicious fragments:\n\n\nV510 The \u0027LogError\u0027 function is not expected to receive class-type variable as second actual argument. behaviortreenodes_core.cpp 1339 \n V510 The \u0027Format\u0027 function is not expected to receive class-type variable as second actual argument. behaviortreenodes_core.cpp 2648 \n V510 The \u0027CryWarning\u0027 function is not expected to receive class-type variable as sixth actual argument. crypak.cpp 3324 \n V510 The \u0027CryWarning\u0027 function is not expected to receive class-type variable as fifth actual argument. crypak.cpp 3333 \n V510 The \u0027CryWarning\u0027 function is not expected to receive class-type variable as fifth actual argument. shaderfxparsebin.cpp 4864 \n V510 The \u0027CryWarning\u0027 function is not expected to receive class-type variable as fifth actual argument. shaderfxparsebin.cpp 4931 \n V510 The \u0027Format\u0027 function is not expected to receive class-type variable as third actual argument. featuretester.cpp 1727 \n\nV529 Odd semicolon \u0027;\u0027 after \u0027for\u0027 operator. boolean3d.cpp 1314\n\nint CTriMesh::Slice(....)\n{\n ....\n bop_meshupdate *pmd = new bop_meshupdate, *pmd0;\n pmd-\u0026gt;pMesh[0]=pmd-\u0026gt;pMesh[1] = this; AddRef();AddRef();\n for(pmd0=m_pMeshUpdate; pmd0-\u0026gt;next; pmd0=pmd0-\u0026gt;next); // next = pmd;\n ....\n}\nThis code is very strange. The programmer put a semicolon after the for loop, while the code formatting suggests that it should have a body.\nV535 The variable \u0027j\u0027 is being used for this loop and for the outer loop. Check lines: 3447, 3490. physicalworld.cpp 3490\n\nvoid CPhysicalWorld::SimulateExplosion(....)\n{\n ....\n for(j=0;jnIslands;j++) // \n\nThe project\u0027s code is full of other unsafe fragments; for example, there are cases of using one counter for both nested and outer loops. Large source files contain code with intricate formatting and fragments where the same variables are changed in different parts of the code - you just can\u0027t do without static analysis there!\n\nA few more strange loops:\n\nV535 The variable \u0027i\u0027 is being used for this loop and for the outer loop. Check lines: 1630, 1683. entity.cpp 1683 \n V535 The variable \u0027i1\u0027 is being used for this loop and for the outer loop. Check lines: 1521, 1576. softentity.cpp 1576 \n V535 The variable \u0027i\u0027 is being used for this loop and for the outer loop. Check lines: 2315, 2316. physicalentity.cpp 2316 \n V535 The variable \u0027i\u0027 is being used for this loop and for the outer loop. Check lines: 1288, 1303. shadercache.cpp 1303 \nV539 Consider inspecting iterators which are being passed as arguments to function \u0027erase\u0027. frameprofilerender.cpp 1090\n\n\nfloat CFrameProfileSystem::RenderPeaks()\n{\n ....\n std::vector\u0026amp; rPeaks = m_peaks;\n \n // Go through all peaks.\n for (int i = 0; i fHotToColdTime)\n {\n rPeaks.erase(m_peaks.begin() + i); // \n\nThe analyzer suspected that the function handling a container would receive an iterator from another container. It\u0027s a wrong assumption, and there is no error here: the rPeaks variable is a reference to m_peaks. This code, however, may confuse not only the analyzer, but also other programmers who will maintain it. One shouldn\u0027t write code in a way like that.\n\nV713 The pointer pCollision was utilized in the logical expression before it was verified against nullptr in the same logical expression. actiongame.cpp 4235\n\n\nint CActionGame::OnCollisionImmediate(const EventPhys* pEvent)\n{\n ....\n else if (pMat-\u0026gt;GetBreakability() == 2 \u0026amp;\u0026amp;\n pCollision-\u0026gt;idmat[0] != pCollision-\u0026gt;idmat[1] \u0026amp;\u0026amp;\n (energy = pMat-\u0026gt;GetBreakEnergy()) \u0026gt; 0 \u0026amp;\u0026amp;\n pCollision-\u0026gt;mass[0] * 2 \u0026gt; energy \u0026amp;\u0026amp;\n ....\n pMat-\u0026gt;GetHitpoints() \n\nThe if statement includes a rather lengthy conditional expression where the pCollision pointer is used multiple times. What is wrong about this code is that the pointer is tested for null at the very end, i.e. after multiple dereference operations.\n\nV758 The \u0027commandList\u0027 reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 274\n\ntypedef std::shared_ptr\u0026lt;....\u0026gt; CDeviceGraphicsCommandListPtr;\n\nCDeviceGraphicsCommandListPtr\nCDeviceObjectFactory::GetCoreGraphicsCommandList() const\n{\n return m_pCoreCommandList;\n}\n\nvoid CRenderItemDrawer::DrawCompiledRenderItems(....) const\n{\n ....\n {\n auto\u0026amp; RESTRICT_REFERENCE commandList = *CCryDeviceWrapper::\n GetObjectFactory().GetCoreGraphicsCommandList();\n\n passContext....-\u0026gt;PrepareRenderPassForUse(commandList);\n }\n ....\n}\n\nThe commandList variable receives a reference to the value stored in a smart pointer. When this pointer destroys the object, the reference will become invalid.\n\nA few more issues of this type:\n\nV758 The \u0027commandList\u0027 reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 384 \n V758 The \u0027commandList\u0027 reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 368 \n V758 The \u0027commandList\u0027 reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 485 \n V758 The \u0027commandList\u0027 reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 553 \n\nConclusion\n\nIt costs almost nothing to fix bugs caught during the coding phase unlike those that get to the testers, while fixing bugs that have made it to the end users involves huge expenses. No matter what analyzer you use, the static analysis technology itself has long proved to be an extremely effective and efficient means to control the quality of program code and software products in general.\n\nOur collaboration with Epic Games has shown very well how integration of our analyzer into Unreal Engine 4 has benefited the project. We helped the developers in every aspect of analyzer integration and even fixed the bugs found in the project so that the developer team could continue scanning new code regularly on their own. We\u0027ve shown that similar collaboration can be achieved with Crytek.\n\nTry PVS-Studio on your own C/C++/C# project! \n", "articleBody": "In May 2016, German game-development company Crytek made a decision to upload the source code of their game engine CryEngine V to Github. The engine is written in C++ and has immediately attracted attention of both the open-source developer community and the team of developers of PVS-Studio static analyzer who regularly scan the code of open-source projects to estimate its quality. A lot of great games were created by a number of video-game development studios using various versions of CryEngine, and now the engine has become available to even more developers. This article gives an overview of errors found in the project by PVS-Studio static analyzer.\n\nIntroduction\n\nCryEngine is a game engine developed by German company Crytek in 2002 and originally used in first-person shooter Far Cry. A lot of great games were created by a number of video-game development studios using various licensed versions of CryEngine: 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 a release date for their new engine CryEngine V and uploaded its source code to Github soon after.\n\nThe project\u0027s source code was checked by PVS-Studio static analyzer, version 6.05. This is a tool designed for detecting software errors in program source code in C, C++, and C#. The only true way of using static analysis is to regularly scan code on developers\u0027 computers and build-servers. However, in order to demonstrate PVS-Studio\u0027s diagnostic capabilities, we run single-time checks of open-source projects and then write articles about errors found. If we like a project, we might scan it again a couple of years later. Such recurring checks are in fact the same as single-time checks since the code accumulates a lot of changes during that time.\n\nFor our checks, we pick projects that are simply popular and wide-known as well as projects suggested by our readers via e-mail. That\u0027s why CryEngine V was by no means the first game engine among those scanned by our analyzer. Other engines that we have already checked include:\n\n\nUnreal Engine 4 (first check, second check, third check) \n Check of Godot Engine \n Check of Serious Engine \n Check of X-Ray Engine \n Check of Xenko Engine \n\nWe also checked CryEngine 3 SDK once.\n\nI\u0027d like to elaborate on the check of Unreal Engine 4 engine in particular. Using that project as an example allowed us to demonstrate in every detail what the right way of using static analysis on a real project should look like, covering the whole process from the phase of integrating the analyzer into the project to the phase of cutting warnings to zero with subsequent control over bug elimination in new code. Our work on Unreal Engine 4 project developed into collaboration with Epic Games company, in terms of which our team fixed all the defects found in the engine\u0027s source code and wrote a joint article with Epic Games on the accomplished work (it was posted on Unreal Engine Blog). Epic Games also purchased a PVS-Studio license to be able to maintain the quality of their code on their own. Collaboration of this kind is something that we would like to try with Crytek, too.\n\nAnalyzer-report structure\n\nIn this article, I\u0027d like to answer a few frequently asked questions concerning the number of warnings and false positives, for example, \"What is the ratio of false positives?\" or \"Why are there so few bugs in so large a project?\"\n\nTo begin with, all PVS-Studio warnings are classified into three severity levels: High, Medium, and Low. The High level holds the most critical warnings, which are almost surely real errors, while the Low level contains the least critical warnings or warnings that are very likely to be false positives. Keep in mind that the codes of errors do not tie them firmly to particular severity levels: distribution of warnings across the levels very much depends on the context.\n\nThis is how the warnings of the General Analysis module are distributed across the severity levels for CryEngine V project:\n\nHigh: 576 warnings; \n Medium: 814 warnings, \n Low: 2942 warnings. \n\nFigure 1 shows distribution of the warnings across the levels in the form of a pie chart.\n\n\n\nFigure 1 - Percentage distribution of warnings across severity levels\n\nIt is impossible to include all the warning descriptions and associated code fragments in an article. Our articles typically discuss 10-40 commented cases; some warnings are given as a list; and most have to be left unexamined. In the best-case scenario, project authors, after we inform them, ask for a complete analysis report for close study. The bitter truth is that in most cases the number of High-level warnings alone is more than enough for an article, and CryEngine V is no exception. Figure 2 shows the structure of the High-level warnings issued for this project.\n\n\n\nFigure 2 - Structure of High-level warnings\n\nLet\u0027s take a closer look at the sectors of this chart:\n\nDescribed in the article (6%) - warnings cited in the article and accompanied by code fragments and commentary. \n Presented as a list (46%) - warnings cited as a list. These warnings refer to the same pattern as some of the errors already discussed, so only the warning text is given. \n False Positives (8%) - a certain ratio of false positives we have taken into account for future improvement of the analyzer. \n Other (40%) - all the other warnings issued. These include warnings that we had to leave out so that the article wouldn\u0027t grow too large, non-critical warnings, or warnings whose validity could be estimated only by a member of the developer team. As our experience of working on Unreal Engine 4 has shown, such code still \"smells\" and those warnings get fixed anyway. \n\nAnalysis results\n\nAnnoying copy-paste\n\n\n\nV501 There are identical sub-expressions to the left and to the right of the \u0027-\u0027 operator: q2.v.z - q2.v.z entitynode.cpp 93\n\n\nbool\nCompareRotation(const Quat\u0026amp; q1, const Quat\u0026amp; q2, float epsilon)\n{\n return (fabs_tpl(q1.v.x - q2.v.x) \n\nA mistyped digit is probably one of the most annoying typos one can make. In the function above, the analyzer detected a suspicious expression, (q2.v.z - q2.v.z), where variables q1 and q2 seem to have been mixed up.\n\nV501 There are identical sub-expressions \u0027(m_eTFSrc == eTF_BC6UH)\u0027 to the left and to the right of the \u0027||\u0027 operator. texturestreaming.cpp 919\n\n\n//! Texture formats.\nenum ETEX_Format : uint8\n{\n ....\n eTF_BC4U, //!\n\nAnother kind of typos deals with copying of constants. In this case, the m_eTFSrc variable is compared twice with the eTF_BC6UH constant. The second of these checks must compare the variable with some other constant whose name differs from the copied one in just one character, for example, eTF_BC6SH.\n\nTwo more similar issues:\n\n\nV501 There are identical sub-expressions \u0027(td.m_eTF == eTF_BC6UH)\u0027 to the left and to the right of the \u0027||\u0027 operator. texture.cpp 1214 \n V501 There are identical sub-expressions \u0027geom_colltype_solid\u0027 to the left and to the right of the \u0027|\u0027 operator. attachmentmanager.cpp 1004 \n\nV517 The use of \u0027if (A) {...} else if (A) {...}\u0027 pattern was detected. There is a probability of logical error presence. Check lines: 266, 268. d3dhwshader.cpp 266\n\n\nint SD3DShader::Release(EHWShaderClass eSHClass, int nSize)\n{\n ....\n if (eSHClass == eHWSC_Pixel)\n return ((ID3D11PixelShader*)pHandle)-\u0026gt;Release();\n else if (eSHClass == eHWSC_Vertex)\n return ((ID3D11VertexShader*)pHandle)-\u0026gt;Release();\n else if (eSHClass == eHWSC_Geometry) // Release(); // Release(); // Release();\n else if (eSHClass == eHWSC_Compute)\n return ((ID3D11ComputeShader*)pHandle)-\u0026gt;Release();\n else if (eSHClass == eHWSC_Domain)\n return ((ID3D11DomainShader*)pHandle)-\u0026gt;Release()\n ....\n}\n\nThis is an example of lazy copying of a cascade of conditional statements, one of which was left unchanged.\n\nV517 The use of \u0027if (A) {...} else if (A) {...}\u0027 pattern was detected. There is a probability of logical error presence. Check lines: 970, 974. environmentalweapon.cpp 970\n\n\nvoid CEnvironmentalWeapon::UpdateDebugOutput() const\n{\n ....\n const char* attackStateName = \"None\";\n if(m_currentAttackState \u0026amp; // \n\nIn the previous example, there was at least a small chance that an extra condition resulted from making too many copies of a code fragment, while the programmer simply forgot to remove one of the checks. In this code, however, the attackStateName variable will never take the value \"Charged Throw\" because of identical conditional expressions.\n\nV519 The \u0027BlendFactor[2]\u0027 variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1265, 1266. ccrydxgldevicecontext.cpp 1266\n\n\nvoid CCryDXGLDeviceContext::\nOMGetBlendState(...., FLOAT BlendFactor[4], ....)\n{\n CCryDXGLBlendState::ToInterface(ppBlendState, m_spBlendState);\n if ((*ppBlendState) != NULL)\n (*ppBlendState)-\u0026gt;AddRef();\n BlendFactor[0] = m_auBlendFactor[0];\n BlendFactor[1] = m_auBlendFactor[1];\n BlendFactor[2] = m_auBlendFactor[2]; // \n\nIn this function, a typo in the element index prevents the element with index \u00273\u0027, BlendFactor[3], from being filled with a value. This fragment would have remained just one of the many interesting examples of typos, had not the analyzer found two more copies of the same incorrect fragment:\n\nV519 The \u0027m_auBlendFactor[2]\u0027 variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 904, 905. ccrydxgldevicecontext.cpp 905\n\n\nvoid CCryDXGLDeviceContext::\n OMSetBlendState(....const FLOAT BlendFactor[4], ....)\n{\n ....\n m_uSampleMask = SampleMask;\n if (BlendFactor == NULL)\n {\n m_auBlendFactor[0] = 1.0f;\n m_auBlendFactor[1] = 1.0f;\n m_auBlendFactor[2] = 1.0f; // SetBlendColor(m_auBlendFactor[0],\n m_auBlendFactor[1],\n m_auBlendFactor[2],\n m_auBlendFactor[3]);\n m_pContext-\u0026gt;SetSampleMask(m_uSampleMask);\n ....\n}\n\nHere\u0027s that fragment where the element with index \u00273\u0027 is skipped again. I even thought for a moment that there was some intentional pattern to it, but this thought quickly vanished as I saw that the programmer attempted to access all the four elements of the m_auBlendFactor array at the end of the function. It looks like the same code with a typo was simply copied several times in the file ccrydxgldevicecontext.cpp.\n\nV523 The \u0027then\u0027 statement is equivalent to the \u0027else\u0027 statement. d3dshadows.cpp 1410\n\n\nvoid CD3D9Renderer::ConfigShadowTexgen(....)\n{\n ....\n if ((pFr-\u0026gt;m_Flags \u0026amp; DLF_DIRECTIONAL) ||\n (!(pFr-\u0026gt;bUseHWShadowMap) \u0026amp;\u0026amp; !(pFr-\u0026gt;bHWPCFCompare)))\n {\n //linearized shadows are used for any kind of directional\n //lights and for non-hw point lights\n m_cEF.m_TempVecs[2][Num] = 1.f / (pFr-\u0026gt;fFarDist);\n }\n else\n {\n //hw point lights sources have non-linear depth for now\n m_cEF.m_TempVecs[2][Num] = 1.f / (pFr-\u0026gt;fFarDist);\n }\n ....\n}\n\nTo finish the section on copy-paste, here is one more interesting error. No matter what result the conditional expression produces, the value m_cEF.m_TempVecs[2][Num] is always computed by the same formula. Judging by the surrounding code, the index is correct: it\u0027s exactly the element with index \u00272\u0027 that must be filled with a value. It\u0027s just that the formula itself was meant to be different in each case, and the programmer forgot to change the copied code.\n\nTroubles with initialization\n\n\n\nV546 Member of a class is initialized by itself: \u0027eConfigMax(eConfigMax)\u0027. particleparams.h 1013\n\n\nParticleParams() :\n ....\n fSphericalApproximation(1.f),\n fVolumeThickness(1.0f),\n fSoundFXParam(1.f),\n eConfigMax(eConfigMax.VeryHigh), // \n\nThe analyzer detected a potential typo that causes a class field to be initialized to its own value.\n\nV603 The object was created but it is not being used. If you wish to call constructor, \u0027this-\u0026gt;SRenderingPassInfo::SRenderingPassInfo(....)\u0027 should be used. i3dengine.h 2589\n\nSRenderingPassInfo()\n : pShadowGenMask(NULL)\n , nShadowSide(0)\n , nShadowLod(0)\n , nShadowFrustumId(0)\n , m_bAuxWindow(0)\n , m_nRenderStackLevel(0)\n , m_eShadowMapRendering(static_cast(SHADOW_MAP_NONE))\n , m_bCameraUnderWater(0)\n , m_nRenderingFlags(0)\n , m_fZoomFactor(0.0f)\n , m_pCamera(NULL)\n , m_nZoomInProgress(0)\n , m_nZoomMode(0)\n , m_pJobState(nullptr)\n{\n threadID nThreadID = 0;\n gEnv-\u0026gt;pRenderer-\u0026gt;EF_Query(EFQ_MainThreadList, nThreadID);\n m_nThreadID = static_cast(nThreadID);\n m_nRenderFrameID = gEnv-\u0026gt;pRenderer-\u0026gt;GetFrameID();\n m_nRenderMainFrameID = gEnv-\u0026gt;pRenderer-\u0026gt;GetFrameID(false);\n}\n \nSRenderingPassInfo(threadID id)\n{\n SRenderingPassInfo(); // \n\nIn this code, incorrect use of constructor was detected. The programmer probably assumed that calling a constructor in a way like that - without parameters - inside another constructor would initialize the class fields, but this assumption was wrong.\n\nInstead, a new unnamed object of type SRenderingPassInfo will be created and immediately destroyed. The class fields, therefore, will remain uninitialized. One way to fix this error is to create a separate initialization function and call it from different constructors.\n\nV688 The \u0027m_cNewGeomMML\u0027 local variable possesses the same name as one of the class members, which can result in a confusion. terrain_node.cpp 344\n\n\nvoid CTerrainNode::Init(....)\n{\n ....\n m_nOriginX = m_nOriginY = 0; // sector origin\n m_nLastTimeUsed = 0; // basically last time rendered\n\n uint8 m_cNewGeomMML = m_cCurrGeomMML = m_cNewGeomMML_Min ....\n\n m_pLeafData = 0;\n\n m_nTreeLevel = 0;\n ....\n}\n\nThe name of the local variable cNewGeomMML coincides with that of a class field. It\u0027s usually not an error, but in this particular case it does look strange in comparison to how the other class fields are initialized.\n\nV575 The \u0027memset\u0027 function processes \u00270\u0027 elements. Inspect the third argument. crythreadutil_win32.h 294\n\nvoid EnableFloatExceptions(....)\n{\n ....\n CONTEXT ctx;\n memset(\u0026amp;ctx, sizeof(ctx), 0); // \n\nThis error is a very interesting one. When calling the memset() function, two arguments were swapped by mistake, which resulted in calling the function to fill 0 bytes. This is the function prototype:\n\nvoid * memset ( void * ptr, int value, size_t num );\n\nThe function expects to receive the buffer size as the third argument and the value the buffer is to be filled with as the second.\n\nThe fixed version:\n\n\nvoid EnableFloatExceptions(....)\n{\n ....\n CONTEXT ctx;\n memset(\u0026amp;ctx, 0, sizeof(ctx));\n ....\n}\n\nV630 The \u0027_alloca\u0027 function is used to allocate memory for an array of objects which are classes containing constructors. command_buffer.cpp 62\n\nvoid CBuffer::Execute()\n{\n ....\n QuatT * pJointsTemp = static_cast(\n alloca(m_state.m_jointCount * sizeof(QuatT)));\n ....\n}\n\nIn some parts of the project\u0027s code, the alloca() function is used to allocate memory for an array of objects. In the example above, with memory allocated in such a way, neither the constructor, nor the destructor will be called for objects of class QuatT. This defect may result in handling uninitialized variables, and other errors.\n\nHere\u0027s a complete list of other defects of this type:\n\n\nV630 The \u0027_alloca\u0027 function is used to allocate memory for an array of objects which are classes containing constructors. command_buffer.cpp 67 \n V630 The \u0027_alloca\u0027 function is used to allocate memory for an array of objects which are classes containing constructors. posematching.cpp 144 \n V630 The \u0027_alloca\u0027 function is used to allocate memory for an array of objects which are classes containing constructors. characterinstance.cpp 280 \n V630 The \u0027_alloca\u0027 function is used to allocate memory for an array of objects which are classes containing constructors. characterinstance.cpp 282 \n V630 The \u0027_alloca\u0027 function is used to allocate memory for an array of objects which are classes containing constructors. scriptbind_entity.cpp 6252 \n V630 The \u0027_alloca\u0027 function is used to allocate memory for an array of objects which are classes containing constructors. jobmanager.cpp 1016 \n V630 The \u0027_alloca\u0027 function is used to allocate memory for an array of objects which are classes containing constructors. driverd3d.cpp 5859 \n\nV583 The \u0027?:\u0027 operator, regardless of its conditional expression, always returns one and the same value: -1.8f. posealignerc3.cpp 330\n\n\nILINE bool InitializePoseAlignerPinger(....)\n{\n ....\n chainDesc.offsetMin = Vec3(0.0f, 0.0f, bIsMP ? -1.8f : -1.8f);\n chainDesc.offsetMax = Vec3(0.0f, 0.0f, bIsMP ? +0.75f : +1.f);\n ....\n}\n\nA few fragments were found where the ternary operator ?: returns one and the same value. While in the previous example it could have been done for aesthetic reasons, the reason for doing so in the following fragment is unclear.\n\nfloat predictDelta = inputSpeed \u0026lt; 0.0f ? 0.1f : 0.1f; // \u0026lt;=\nfloat dict = angle + predictDelta * ( angle - m_prevAngle) / dt ;\n\nA complete list of other defects of this type:\n\nV583 The \u0027?:\u0027 operator, regardless of its conditional expression, always returns one and the same value: -1.8f. posealignerc3.cpp 313 \n V583 The \u0027?:\u0027 operator, regardless of its conditional expression, always returns one and the same value: -2.f. posealignerc3.cpp 347 \n V583 The \u0027?:\u0027 operator, regardless of its conditional expression, always returns one and the same value: D3D11_RTV_DIMENSION_TEXTURE2DARRAY. d3dtexture.cpp 2277 \n V583 The \u0027?:\u0027 operator, regardless of its conditional expression, always returns one and the same value: 255U. renderer.cpp 3389 \n V583 The \u0027?:\u0027 operator, regardless of its conditional expression, always returns one and the same value: D3D12_RESOURCE_STATE_GENERIC_READ. dx12device.cpp 151 \n V583 The \u0027?:\u0027 operator, regardless of its conditional expression, always returns one and the same value: 0.1f. vehiclemovementstdboat.cpp 720 \n\nV570 The \u0027runtimeData.entityId\u0027 variable is assigned to itself. behaviortreenodes_ai.cpp 1771\n\n\nvoid ExecuteEnterScript(RuntimeData\u0026amp; runtimeData)\n{\n ExecuteScript(m_enterScriptFunction, runtimeData.entityId);\n\n runtimeData.entityId = runtimeData.entityId; // \n\nA variable is assigned to itself, which doesn\u0027t look right. The authors should check this code.\n\nOperation precedence\n\n\n\nV502 Perhaps the \u0027?:\u0027 operator works in a different way than it was expected. The \u0027?:\u0027 operator has a lower priority than the \u0027+\u0027 operator. gpuparticlefeaturespawn.cpp 79\n\nbool HasDuration() { return m_useDuration; }\n\nvoid CFeatureSpawnRate::SpawnParticles(....)\n{\n ....\n SSpawnData\u0026amp; spawn = pRuntime-\u0026gt;GetSpawnData(i);\n const float amount = spawn.amount;\n const int spawnedBefore = int(spawn.spawned);\n const float endTime = spawn.delay +\n HasDuration() ? spawn.duration : fHUGE;\n ....\n}\n\nThe function above seems to measure time in a wrong way. The precedence of the addition operator is higher than that of the ternary operator :?, so the value 0 or 1 is added to spawn.delay first, and then the value spawn.duration or fHUGE is written into the endTime variable. This error is quite a common one. To learn more about interesting patterns of errors involving operation precedence collected from the PVS-Studio bug database, see my article: Logical Expressions in C/C++. Mistakes Made by Professionals.\n\nV634 The priority of the \u0027*\u0027 operation is higher than that of the \u0027\n\nenum joint_flags\n{\n angle0_locked = 1,\n ....\n};\n\nbool CDefaultSkeleton::SetupPhysicalProxies(....)\n{\n ....\n for (int j = 0; .... ; j++)\n {\n // lock axes with 0 limits range\n m_arrModelJoints....flags |= (....) * angle0_locked \n\nThis is another very interesting error that has to do with the precedence of the multiplication and bitwise shift operations. The latter has lower precedence, so the whole expression is multiplied by one at each iteration (as the angle0_locked constant has the value one), which looks very strange.\n\nThis is what the programmer must have wanted that code to look like:\n\nm_arrModelJoints....flags |= (....) * (angle0_locked \u0026lt;\u0026lt; j);\n\nThe following file contains a list of 35 suspicious fragments involving precedence of shift operations: CryEngine5_V634.txt.\n\nUndefined behavior\n\nUndefined behavior is the result of executing computer code written in a certain programming language that depends on a number of random factors such as memory state or triggered interrupts. In other words, this result is not prescribed by the language specification. It is considered to be an error to let such a situation occur in your program. Even if it can successfully execute on some compiler, it is not guaranteed to be cross-platform and may fail on another machine, operating system, and even other settings of the same compiler.\n\n\n\nV610 Undefined behavior. Check the shift operator \u0027\n\n#ifndef physicalplaceholder_h\n#define physicalplaceholder_h\n#pragma once\n....\nconst int NO_GRID_REG = -1\n\nUnder the modern C++ standard, a left shift of a negative value is undefined behavior. The analyzer found a few more similar issues in CryEngine V\u0027s code:\n\nV610 Undefined behavior. Check the shift operator \u0027\n \nV610 Undefined behavior. Check the shift operator \u0027\n \nV610 Undefined behavior. Check the shift operator \u0027\n \nV610 Undefined behavior. Check the shift operator \u0027\n \nV610 Undefined behavior. Check the shift operator \u0027\n \nV610 Undefined behavior. Check the shift operator \u0027\n \nV610 Undefined behavior. Check the shift operator \u0027\n \nV610 Undefined behavior. Check the shift operator \u0027\n \n\nV567 Undefined behavior. The \u0027m_current\u0027 variable is modified while being used twice between sequence points. operatorqueue.cpp 105\n\nbool COperatorQueue::Prepare(....)\n{\n ++m_current \u0026amp;= 1;\n m_ops[m_current].clear();\n return true;\n}\n\nThe analyzer detected an expression that causes undefined behavior. A variable is used multiple times between two sequence points, while its value changes. The result of executing such an expression, therefore, can\u0027t be determined.\n\nOther similar issues:\n\nV567 Undefined behavior. The \u0027itail\u0027 variable is modified while being used twice between sequence points. trimesh.cpp 3101 \n V567 Undefined behavior. The \u0027ihead\u0027 variable is modified while being used twice between sequence points. trimesh.cpp 3108 \n V567 Undefined behavior. The \u0027ivtx\u0027 variable is modified while being used twice between sequence points. boolean3d.cpp 1194 \n V567 Undefined behavior. The \u0027ivtx\u0027 variable is modified while being used twice between sequence points. boolean3d.cpp 1202 \n V567 Undefined behavior. The \u0027ivtx\u0027 variable is modified while being used twice between sequence points. boolean3d.cpp 1220 \n V567 Undefined behavior. The \u0027m_commandBufferIndex\u0027 variable is modified while being used twice between sequence points. xconsole.cpp 180 \n V567 Undefined behavior. The \u0027m_FrameFenceCursor\u0027 variable is modified while being used twice between sequence points. ccrydx12devicecontext.cpp 952 \n V567 Undefined behavior. The \u0027m_iNextAnimIndex\u0027 variable is modified while being used twice between sequence points. hitdeathreactionsdefs.cpp 192 \n\nErrors in conditions\n\n\n\nV579 The memcmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. graphicspipelinestateset.h 58\n\nbool\noperator==(const SComputePipelineStateDescription\u0026amp; other) const\n{\n return 0 == memcmp(this, \u0026amp;other, sizeof(this)); // \n\nThe programmer made a mistake in the equality operation in the call to the memcmp() function, which leads to passing the pointer size instead of the object size as a function argument. As a result, only the first several bytes of the objects are compared.\n\nThe fixed version:\n\nmemcmp(this, \u0026amp;other, sizeof(*this));\nUnfortunately, three more similar issues were found in the project:\n\nV579 The memcpy function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. geomcacherendernode.cpp 286 \n V579 The AddObject function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. clipvolumemanager.cpp 145 \n V579 The memcmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. graphicspipelinestateset.h 34 \n\nV640 The code\u0027s 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\n\nCLivingEntity::~CLivingEntity()\n{\n for(int i=0;i.pPhysGeom || ....)\n delete[] m_parts.pMatMapping; m_parts.pMatMapping=0;\n }\n ....\n}\n\nI spotted a huge number of code blocks with statements written in one line. These include not only ordinary assignments, but rather loops, conditions, function calls, and sometimes a mixture of all of these (see Figure 3).\n\n\n\nFigure 3 - Poor code formatting\n\nIn code of size like that, this programming style almost inevitably leads to errors. In the example above, the memory block occupied by an array of objects was to be freed and the pointer was to be cleared when a certain condition was met. However, incorrect code formatting causes the m_parts.pMatMapping pointer to be cleared at every loop iteration. The implications of this problem can\u0027t be predicted, but the code does look strange.\n\nOther fragments with strange formatting:\n\nV640 The code\u0027s operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. physicalworld.cpp 2449 \n V640 The code\u0027s operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. articulatedentity.cpp 1723 \n V640 The code\u0027s operational logic does not correspond with its formatting. The second statement will always be executed. It is possible that curly brackets are missing. articulatedentity.cpp 1726 \n\nV695 Range intersections are possible within conditional expressions. Example: if (A \n\n\nbool CStatObj::RenderDebugInfo(....)\n{\n ....\n ColorB clr(0, 0, 0, 0);\n if (nRenderMats == 1)\n clr = ColorB(0, 0, 255, 255);\n else if (nRenderMats == 2)\n clr = ColorB(0, 255, 255, 255);\n else if (nRenderMats == 3)\n clr = ColorB(0, 255, 0, 255);\n else if (nRenderMats == 4)\n clr = ColorB(255, 0, 255, 255);\n else if (nRenderMats == 5)\n clr = ColorB(255, 255, 0, 255);\n else if (nRenderMats \u0026gt;= 6) // = 11) // \n\nThe programmer made a mistake that prevents the color ColorB(255, 255, 255, 255) from ever being selected. The values nRenderMats are first compared one by one with the numbers from 1 to 5, but when comparing them with value ranges, the programmer didn\u0027t take into account that values larger than 11 already belong to the range of values larger than 6, so the last condition will never execute.\n\nThis cascade of conditions was copied in full into one more fragment:\nV695 Range intersections are possible within conditional expressions. Example: if (A \n \nV695 Range intersections are possible within conditional expressions. Example: if (A \n\nenum eNodeConstants\n{\n ....\n CHILDBLOCKS_MAX_DIST_FOR_8BITS = BIT(7) - 1, // 127\n CHILDBLOCKS_MAX_DIST_FOR_16BITS = BIT(6) - 1, // 63\n ....\n};\n\nvoid CNodeLiveWriter::Compact()\n{\n ....\n if (dist \n\nA similar mistake inside a condition was also found in the fragment above, except that this time the code that fails to get control is larger. The values of the constants CHILDBLOCKS_MAX_DIST_FOR_8BITS and CHILDBLOCKS_MAX_DIST_FOR_16BITS are such that the second condition will never be true.\n\nV547 Expression \u0027pszScript[iSrcBufPos] != \u0027==\u0027\u0027 is always true. The value range of char type: [-128, 127]. luadbg.cpp 716\n\nbool CLUADbg::LoadFile(const char* pszFile, bool bForceReload)\n{\n FILE* hFile = NULL;\n char* pszScript = NULL, * pszFormattedScript = NULL;\n ....\n while (pszScript[iSrcBufPos] != \u0027 \u0027 \u0026amp;\u0026amp;\n ....\n pszScript[iSrcBufPos] != \u0027=\u0027 \u0026amp;\u0026amp;\n pszScript[iSrcBufPos] != \u0027==\u0027 \u0026amp;\u0026amp; // \n\nA large conditional expression contains a subexpression that is always true. The \u0027==\u0027 literal will have type int and correspond to the value 15677. The pszScript array consists of elements of type char, and a value of type char can\u0027t be equal to 15677, so the pszScript[iSrcBufPos] != \u0027==\u0027 expression is always true.\n\nV734 An excessive expression. Examine the substrings \"_ddn\" and \"_ddna\". texture.cpp 4212\n\nvoid CTexture::PrepareLowResSystemCopy(byte* pTexData, ....)\n{\n ....\n // make sure we skip non diffuse textures\n if (strstr(GetName(), \"_ddn\") // \n\nThe strstr() function looks for the first occurrence of the specified substring within another string and returns either a pointer to the first occurrence or an empty pointer. The string \"_ddn\" is the first to be searched, and \"_ddna\" is the second, which means that the condition will be true if the shorter string is found. This code might not work as expected; or perhaps this expression is redundant and could be simplified by removing the extra check.\n\nV590 Consider inspecting this expression. The expression is excessive or contains a misprint. goalop_crysis2.cpp 3779\n\nvoid COPCrysis2FlightFireWeapons::ParseParam(....)\n{\n ....\n else if (!paused \u0026amp;\u0026amp;\n (m_State == eFP_PAUSED) \u0026amp;\u0026amp; // \n\nThe conditional expression in the ParseParam() function is written in such a way that its result does not depend on the (m_State != eFP_PAUSED_OVERRIDE) subexpression.\n\nHere\u0027s a simpler example:\n\nif ( err == code1 \u0026amp;\u0026amp; err != code2)\n{\n ....\n}\nThe result of the whole conditional expression does not depend on the result of the (err != code2) subexpression, which can be clearly seen from the truth table for this example (see Figure 4)\n\n\n\nFigure 4 - Truth table for a logical expression\n\nComparing unsigned values with zero\n\n\n\nWhen scanning projects, we often come across comparisons of unsigned values with zero, which produce either true or false every time. Such code does not always contain a critical bug; it is often a result of too much caution or changing a variable\u0027s type from signed to unsigned. Anyway, such comparisons need to be checked.\n\nV547 Expression \u0027m_socket \n\ntypedef SOCKET CRYSOCKET;\n// Internal socket data\nCRYSOCKET m_socket;\n\nbool CServiceNetworkConnection::TryReconnect()\n{\n ....\n // Create new socket if needed\n if (m_socket == 0)\n {\n m_socket = CrySock::socketinet();\n if (m_socket \n\nI\u0027d like to elaborate on the SOCKET type. It can be both signed and unsigned depending on the platforms, so it is strongly recommended that you use special macros and constants specified in the standard headers when working with this type.\n\nIn cross-platform projects, comparisons with 0 or -1 are common that result in misinterpretation of error codes. CryEngine V project is no exception, although some checks are done correctly, for example:\n\nif (m_socket == CRY_INVALID_SOCKET)\n\nNevertheless, many parts of the code use different versions of these checks.\n\nSee the file CryEngine5_V547.txt for other 47 suspicious comparisons of unsigned variables with zero. The code authors need to check these warnings.\n\nDangerous pointers\n\n\n\nDiagnostic V595 detects pointers that are tested for null after they have been dereferenced. In practice, this diagnostic catches very tough bugs. On rare occasions, it issues false positives, which is explained by the fact that pointers are checked indirectly, i.e. through one or several other variables, but figuring such code out isn\u0027t an easy task for a human either, is it? Three code samples are given below that trigger this diagnostic and look especially surprising, as it\u0027s not clear why they work at all. For the other warnings of this type see the file CryEngine5_V595.txt.\nExample 1\n\nV595 The \u0027m_pPartManager\u0027 pointer was utilized before it was verified against nullptr. Check lines: 1441, 1442. 3denginerender.cpp 1441\n\nvoid C3DEngine::RenderInternal(....)\n{\n ....\n m_pPartManager-\u0026gt;GetLightProfileCounts().ResetFrameTicks();\n if (passInfo.IsGeneralPass() \u0026amp;\u0026amp; m_pPartManager)\n m_pPartManager-\u0026gt;Update();\n ....\n}\n\nThe m_pPartManager pointer is dereferenced and then checked.\n\nExample 2\n\nV595 The \u0027gEnv-\u0026gt;p3DEngine\u0027 pointer was utilized before it was verified against nullptr. Check lines: 1477, 1480. gameserialize.cpp 1477\n\nbool CGameSerialize::LoadLevel(....)\n{\n ....\n // can quick-load\n if (!gEnv-\u0026gt;p3DEngine-\u0026gt;RestoreTerrainFromDisk())\n return false;\n\n if (gEnv-\u0026gt;p3DEngine)\n {\n gEnv-\u0026gt;p3DEngine-\u0026gt;ResetPostEffects();\n }\n ....\n}\n\nThe gEnv-\u0026gt;p3DEngine pointer is dereferenced and then checked.\n\nExample 3\n\nV595 The \u0027pSpline\u0027 pointer was utilized before it was verified against nullptr. Check lines: 158, 161. facechannelkeycleanup.cpp 158\n\nvoid FaceChannel::CleanupKeys(....)\n{\n\n CFacialAnimChannelInterpolator backupSpline(*pSpline);\n\n // Create the key entries array.\n int numKeys = (pSpline ? pSpline-\u0026gt;num_keys() : 0);\n ....\n}\n\nThe pSpline pointer is dereferenced and then checked.\n\nMiscellaneous\n\n\n\nV622 Consider inspecting the \u0027switch\u0027 statement. It\u0027s possible that the first \u0027case\u0027 operator is missing. mergedmeshrendernode.cpp 999\n\nstatic inline void ExtractSphereSet(....)\n{\n ....\n switch (statusPos.pGeom-\u0026gt;GetType())\n {\n if (false)\n {\n case GEOM_CAPSULE:\n statusPos.pGeom-\u0026gt;GetPrimitive(0, \u0026amp;cylinder);\n }\n if (false)\n {\n case GEOM_CYLINDER:\n statusPos.pGeom-\u0026gt;GetPrimitive(0, \u0026amp;cylinder);\n }\n for (int i = 0; i \n\nThis fragment is probably the strangest of all found in CryEngine V. Whether or not the case label will be selected does not depend on the if statement, even in case of if (false). In the switch statement, an unconditional jump to the label occurs if the condition of the switch statement is met. Without the break statement, one could use such code to \"bypass\" irrelevant statements, but, again, maintaining such obscure code isn\u0027t easy. One more question is, why does the same code execute when jumping to the labels GEOM_CAPSULE and GEOM_CYLINDER?\n\nV510 The \u0027LogError\u0027 function is not expected to receive class-type variable as second actual argument. behaviortreenodes_action.cpp 143\n\ntypedef CryStringT\u0026lt;char\u0026gt; string;\n// The actual fragment name.\nstring m_fragName;\n//! cast to C string.\nconst value_type* c_str() const { return m_str; }\nconst value_type* data() const { return m_str; };\n \nvoid LogError(const char* format, ...) const\n{ .... }\n \nvoid QueueAction(const UpdateContext\u0026amp; context)\n{\n ....\n ErrorReporter(*this, context).LogError(\"....\u0027%s\u0027\", m_fragName);\n ....\n}\n\nWhen it is impossible to specify the number and types of all acceptable parameters to a function, one puts ellipsis (...) at the end of the list of parameters in the function declaration, which means \"and perhaps a few more\". Only POD (Plain Old Data) types can be used as actual parameters to the ellipsis. If an object of a class is passed as an argument to a function\u0027s ellipsis, it almost always signals the presence of a bug. In the code above, it is the contents of the object that get to the stack, not the pointer to a string. Such code results in forming \"gibberish\" in the buffer or a crash. The code of CryEngine V uses a string class of its own, and it already has an appropriate method, c_str().\n\nThe fixed version:\n\nLogError(\"....\u0027%s\u0027\", m_fragName.c_str();\n\nA few more suspicious fragments:\n\n\nV510 The \u0027LogError\u0027 function is not expected to receive class-type variable as second actual argument. behaviortreenodes_core.cpp 1339 \n V510 The \u0027Format\u0027 function is not expected to receive class-type variable as second actual argument. behaviortreenodes_core.cpp 2648 \n V510 The \u0027CryWarning\u0027 function is not expected to receive class-type variable as sixth actual argument. crypak.cpp 3324 \n V510 The \u0027CryWarning\u0027 function is not expected to receive class-type variable as fifth actual argument. crypak.cpp 3333 \n V510 The \u0027CryWarning\u0027 function is not expected to receive class-type variable as fifth actual argument. shaderfxparsebin.cpp 4864 \n V510 The \u0027CryWarning\u0027 function is not expected to receive class-type variable as fifth actual argument. shaderfxparsebin.cpp 4931 \n V510 The \u0027Format\u0027 function is not expected to receive class-type variable as third actual argument. featuretester.cpp 1727 \n\nV529 Odd semicolon \u0027;\u0027 after \u0027for\u0027 operator. boolean3d.cpp 1314\n\nint CTriMesh::Slice(....)\n{\n ....\n bop_meshupdate *pmd = new bop_meshupdate, *pmd0;\n pmd-\u0026gt;pMesh[0]=pmd-\u0026gt;pMesh[1] = this; AddRef();AddRef();\n for(pmd0=m_pMeshUpdate; pmd0-\u0026gt;next; pmd0=pmd0-\u0026gt;next); // next = pmd;\n ....\n}\nThis code is very strange. The programmer put a semicolon after the for loop, while the code formatting suggests that it should have a body.\nV535 The variable \u0027j\u0027 is being used for this loop and for the outer loop. Check lines: 3447, 3490. physicalworld.cpp 3490\n\nvoid CPhysicalWorld::SimulateExplosion(....)\n{\n ....\n for(j=0;jnIslands;j++) // \n\nThe project\u0027s code is full of other unsafe fragments; for example, there are cases of using one counter for both nested and outer loops. Large source files contain code with intricate formatting and fragments where the same variables are changed in different parts of the code - you just can\u0027t do without static analysis there!\n\nA few more strange loops:\n\nV535 The variable \u0027i\u0027 is being used for this loop and for the outer loop. Check lines: 1630, 1683. entity.cpp 1683 \n V535 The variable \u0027i1\u0027 is being used for this loop and for the outer loop. Check lines: 1521, 1576. softentity.cpp 1576 \n V535 The variable \u0027i\u0027 is being used for this loop and for the outer loop. Check lines: 2315, 2316. physicalentity.cpp 2316 \n V535 The variable \u0027i\u0027 is being used for this loop and for the outer loop. Check lines: 1288, 1303. shadercache.cpp 1303 \nV539 Consider inspecting iterators which are being passed as arguments to function \u0027erase\u0027. frameprofilerender.cpp 1090\n\n\nfloat CFrameProfileSystem::RenderPeaks()\n{\n ....\n std::vector\u0026amp; rPeaks = m_peaks;\n \n // Go through all peaks.\n for (int i = 0; i fHotToColdTime)\n {\n rPeaks.erase(m_peaks.begin() + i); // \n\nThe analyzer suspected that the function handling a container would receive an iterator from another container. It\u0027s a wrong assumption, and there is no error here: the rPeaks variable is a reference to m_peaks. This code, however, may confuse not only the analyzer, but also other programmers who will maintain it. One shouldn\u0027t write code in a way like that.\n\nV713 The pointer pCollision was utilized in the logical expression before it was verified against nullptr in the same logical expression. actiongame.cpp 4235\n\n\nint CActionGame::OnCollisionImmediate(const EventPhys* pEvent)\n{\n ....\n else if (pMat-\u0026gt;GetBreakability() == 2 \u0026amp;\u0026amp;\n pCollision-\u0026gt;idmat[0] != pCollision-\u0026gt;idmat[1] \u0026amp;\u0026amp;\n (energy = pMat-\u0026gt;GetBreakEnergy()) \u0026gt; 0 \u0026amp;\u0026amp;\n pCollision-\u0026gt;mass[0] * 2 \u0026gt; energy \u0026amp;\u0026amp;\n ....\n pMat-\u0026gt;GetHitpoints() \n\nThe if statement includes a rather lengthy conditional expression where the pCollision pointer is used multiple times. What is wrong about this code is that the pointer is tested for null at the very end, i.e. after multiple dereference operations.\n\nV758 The \u0027commandList\u0027 reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 274\n\ntypedef std::shared_ptr\u0026lt;....\u0026gt; CDeviceGraphicsCommandListPtr;\n\nCDeviceGraphicsCommandListPtr\nCDeviceObjectFactory::GetCoreGraphicsCommandList() const\n{\n return m_pCoreCommandList;\n}\n\nvoid CRenderItemDrawer::DrawCompiledRenderItems(....) const\n{\n ....\n {\n auto\u0026amp; RESTRICT_REFERENCE commandList = *CCryDeviceWrapper::\n GetObjectFactory().GetCoreGraphicsCommandList();\n\n passContext....-\u0026gt;PrepareRenderPassForUse(commandList);\n }\n ....\n}\n\nThe commandList variable receives a reference to the value stored in a smart pointer. When this pointer destroys the object, the reference will become invalid.\n\nA few more issues of this type:\n\nV758 The \u0027commandList\u0027 reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 384 \n V758 The \u0027commandList\u0027 reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 368 \n V758 The \u0027commandList\u0027 reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 485 \n V758 The \u0027commandList\u0027 reference becomes invalid when smart pointer returned by a function is destroyed. renderitemdrawer.cpp 553 \n\nConclusion\n\nIt costs almost nothing to fix bugs caught during the coding phase unlike those that get to the testers, while fixing bugs that have made it to the end users involves huge expenses. No matter what analyzer you use, the static analysis technology itself has long proved to be an extremely effective and efficient means to control the quality of program code and software products in general.\n\nOur collaboration with Epic Games has shown very well how integration of our analyzer into Unreal Engine 4 has benefited the project. We helped the developers in every aspect of analyzer integration and even fixed the bugs found in the project so that the developer team could continue scanning new code regularly on their own. We\u0027ve shown that similar collaboration can be achieved with Crytek.\n\nTry PVS-Studio on your own C/C++/C# project! \n", "dateCreated": "2016-08-04T07:43:23+0000", "datePublished": "2016-08-11T16:59:42+0000", "dateModified": "2016-08-11T16:59:42+0000", "pageStart": 1, "pageEnd": 1, "author": { "@type": "Person", "name": "PVS-studio team", "image": "https://www.gamedev.net/uploads/profile/photo-thumb-229739.png", "url": "https://www.gamedev.net/profile/229739-pvs-studio-team/" }, "publisher": { "@type": "Organization", "name": "PVS-studio team", "image": "https://www.gamedev.net/uploads/profile/photo-thumb-229739.png", "logo": { "@type": "ImageObject", "url": "https://www.gamedev.net/uploads/profile/photo-thumb-229739.png" }, "url": "https://www.gamedev.net/profile/229739-pvs-studio-team/" }, "interactionStatistic": [ { "@type": "InteractionCounter", "interactionType": "http://schema.org/ViewAction", "userInteractionCount": 15990 }, { "@type": "InteractionCounter", "interactionType": "http://schema.org/FollowAction", "userInteractionCount": 11 }, { "@type": "InteractionCounter", "interactionType": "http://schema.org/ReviewAction", "userInteractionCount": 0 }, { "@type": "InteractionCounter", "interactionType": "http://schema.org/CommentAction", "userInteractionCount": 3 } ], "image": { "@type": "ImageObject", "url": "https://www.gamedev.net/uploads/6778daef1350db9a06fc16c24bd59cd8.png", "width": 250, "height": 150 }, "aggregateRating": { "@type": "AggregateRating", "ratingValue": 4.4, "reviewCount": 0, "bestRating": "5" }, "commentCount": 3 } { "@context": "http://www.schema.org", "@type": "WebSite", "name": "GameDev.net", "url": "https://www.gamedev.net/", "potentialAction": { "type": "SearchAction", "query-input": "required name=query", "target": "https://www.gamedev.net/search/?q={query}" }, "inLanguage": [ { "@type": "Language", "name": "English (USA)", "alternateName": "en-US" } ] } { "@context": "http://www.schema.org", "@type": "Organization", "name": "GameDev.net", "url": "https://www.gamedev.net/", "address": { "@type": "PostalAddress", "streetAddress": "", "addressLocality": null, "addressRegion": null, "postalCode": null, "addressCountry": null } } { "@context": "http://schema.org", "@type": "BreadcrumbList", "itemListElement": [ { "@type": "ListItem", "position": 1, "item": { "@id": "https://www.gamedev.net/articles/", "name": "Articles" } }, { "@type": "ListItem", "position": 2, "item": { "@id": "https://www.gamedev.net/articles/programming/", "name": "Programming" } }, { "@type": "ListItem", "position": 3, "item": { "@id": "https://www.gamedev.net/articles/programming/general-and-gameplay-programming/", "name": "General and Gameplay Programming" } } ] } { "@context": "http://schema.org", "@type": "ContactPage", "url": "https://www.gamedev.net/contact/" } Important Information By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.   I accept GameDev.net is your game development community. Create an account for your GameDev Portfolio and participate in the largest developer community in the games industry. Sign me up!