Amazon Lumberyard: Soul Cry

Games are one of the most popular software products. This is a huge industry in which a new game engine appeared - Amazon Lumberyard. The project is still in beta status and it has time to correct errors, improve the quality of the code. The developers of the engine have a lot of work to do in order not to disappoint millions of gamers and game developers in the near future.

Introduction

Amazon Lumberyard is a free, AAA class cross-platform game engine developed by Amazon and based on the CryEngine engine architecture , which was licensed from Crytek in 2015. By the way, the CryEngine analysis has already been conducted twice by me in August 2016 and April 2017. At the same time, I have to note that the code only got worse after the year. And the other day I decided to see what Amazon did based on this game engine. They worked very well on the environment. Documentation for developers and software for the deployment of the working environment are made very cool and at a high level. But the trouble again! I hope that Amazon has a lot more resources to work with the project, and they still pay attention to the quality of the code. With this review, I hope to draw the attention of developers to the quality of the code and push for a new approach to the development of this game engine. The current state of the code was in such a deplorable state that I changed the title of the article several times and redrawn the title picture as I looked at the report with the analysis results. The first version of the picture was less emotional:

The source code for Amazon Lumberyard of the latest available version 1.14.0.1 was analyzed. Source code is taken from the repository on Github . One of the first games on the Lumberyard engine should be Star Citizen . Potential players who are waiting for her, I also invite you to look at what is currently under the hood of the game.

Integration with PVS-Studio

PVS-Studio was used as a static code analyzer . It is available for Windows, Linux and macOS. Those. to analyze the cross-platform project, there is even something to choose from for more comfortable work. In addition to C and C ++, analysis of projects in C # is supported. Java plans . The vast majority of the code in the world is written in these languages ​​(not without errors, of course), so try the PVS-Studio analyzer on your project, you will learn many interesting things ;-).

WAF is used as the Lumberyard assembly system, which was also in CryEngine. The analyzer does not have a special method for integration with this build system. I decided to work with the project on Windows and chose this way to start the analysis: compilation monitoring system. Project file for Visual Studio is autogenerated. It can be used to build a project and view the analyzer report.

The list of commands for analysis looks like this:

cd /path/to/lumberyard/dev
lmbr_waf.bat ...
CLMonitor.exe monitor
MSBuild.exe ... LumberyardSDK_vs15.sln ...
CLMonitor.exe analyze --log /path/to/report.plog

The report, as I said, can be viewed in Visual Studio.

About Igor and Qualcomm

Amazon Lumberyard positions itself as a cross-platform game engine. Promoting a project to the masses with such features is easy, but it is very difficult to maintain. One of the warnings from PVS-Studio was issued to a code fragment, where programmer Igor struggled with the Qualcomm compiler. Perhaps he solved his problem, but left an extremely suspicious code. I decided to make it a picture.

V523 The 'then' statement is equivalent to the 'else' statement. toglsloperand.c 700

Here the same code is executed, regardless of the calculated condition. Against the background of the comments left, such a decision looks suspicious.

In general, the project is not the only place where the conditions need to be simplified for clarity, or to correct the real error. Here is a list of such places:

• V523 The 'then' statement is equivalent to the 'else' statement. livingentity.cpp 1385
• V523 The 'then' statement is equivalent to the 'else' statement. tometalinstruction.c 4201
• V523 The 'then' statement is equivalent to the 'else' statement. scripttable.cpp 905
• V523 The 'then' statement is equivalent to the 'else' statement. budgetingsystem.cpp 701
• V523 The 'then' statement is equivalent to the 'else' statement. editorframeworkapplication.cpp 562
• V523 The 'then' statement is equivalent to the 'else' statement. particleitem.cpp 130
• V523 The 'then' statement is equivalent to the 'else' statement. trackviewnodes.cpp 1223
• V523 The 'then' statement is equivalent to the 'else' statement. propertyoarchive.cpp 447

Python ++

The analyzer found such a funny code:

V709 CWE-682 Suspicious comparison found found: 'a == b == c'. Remember that 'a == b == c' is not equal to 'a == b && b == c'. toglslinstruction.c 564

voidCallBinaryOp(....){
  ....
  uint32_t src1SwizCount = GetNumSwizzleElements(....);
  uint32_t src0SwizCount = GetNumSwizzleElements(....);
  uint32_t dstSwizCount = GetNumSwizzleElements(....);
  ....
  if (src1SwizCount == src0SwizCount == dstSwizCount) // <=
  {
    ....
  }
  ....
}

In C ++, such code, unfortunately, is compiled successfully, but it is executed in a completely different way. Expressions in C ++ are evaluated in order of priority, performing implicit castes, if necessary.

Such a check would be correct, for example, in the Python language. But in this situation, the developer "shot himself in the foot."

3 more control shots:

• V709 CWE-682 Suspicious comparison found: 'a == b == c'. Remember that 'a == b == c' is not equal to 'a == b && b == c'. toglslinstruction.c 654
• V709 CWE-682 Suspicious comparison found: 'a == b == c'. Remember that 'a == b == c' is not equal to 'a == b && b == c'. toglslinstruction.c 469
• V709 CWE-682 Suspicious comparison found: 'a == b == c'. Remember that 'a == b == c' is not equal to 'a == b && b == c'. tometalinstruction.c 539

The first and one of the best diagnostics

It will be a question of warning V501 - the first general-purpose diagnosis in PVS-Studio. Errors found only by this diagnostic alone may be enough to write an article. And the Amazon Lumberyard project is doing great.

Unfortunately, it’s boring to look through similar errors for a long time, so in this section I will comment on only a couple of code fragments, and I’ll give the rest of the warnings.

V501 There are identical to the left and the right of the '|| operator: hotX <0 || hotX <0 editorutils.cpp 166

QCursor CMFCUtils::LoadCursor(....)
{
  ....
  if (!pm.isNull() && (hotX < 0 || hotX < 0))
  {
    QFile f(path);
    f.open(QFile::ReadOnly);
    QDataStream stream(&f);
    stream.setByteOrder(QDataStream::LittleEndian);
    f.read(10);
    quint16 x;
    stream >> x;
    hotX = x;
    stream >> x;
    hotY = x;
  }
  ....
}

The condition is not enough variable hot the Y . Classic typo.

V501 There are identical sub-expressions 'sp.m_pTexture == m_pTexture' operator. shadercomponents.h 487

V501 There are identical sub-expressions 'sp.m_eCGTextureType == m_eCGTextureType' operator. shadercomponents.h 487

booloperator != (const SCGTexture& sp) const
{
  if (sp.m_RegisterOffset == m_RegisterOffset &&
      sp.m_Name == m_Name &&
      sp.m_pTexture == m_pTexture &&              // <= 1
      sp.m_RegisterCount == m_RegisterCount &&
      sp.m_eCGTextureType == m_eCGTextureType &&  // <= 2
      sp.m_BindingSlot == m_BindingSlot &&
      sp.m_Flags == m_Flags &&
      sp.m_pAnimInfo == m_pAnimInfo &&
      sp.m_pTexture == m_pTexture &&              // <= 1
      sp.m_eCGTextureType == m_eCGTextureType &&  // <= 2
      sp.m_bSRGBLookup == m_bSRGBLookup &&
      sp.m_bGlobal == m_bGlobal)
  {
      returnfalse;
  }
  returntrue;
}

Two copy-pastes were found in this fragment at once. For clarity, painted on the arrows.

V501 There are identical to the left and the right of the operator: pSrc.GetLen () == pSrc.GetLen () fbxpropertytypes.h 978

inlineboolFbxTypeCopy(FbxBlob& pDst, const FbxString& pSrc){
    bool lCastable = pSrc.GetLen() == pSrc.GetLen();
    FBX_ASSERT( lCastable );
    if( lCastable )
        pDst.Assign(pSrc.Buffer(), (int)pSrc.GetLen());
    return lCastable;
}

Here I want to say hello to developers from AUTODESK . This error is from their FBX SDK library . Mixed up the pSrc and pDst variables . I think, besides Lumberyard, there are plenty of other users whose projects depend on the code with this error.

V501 operator: pTS-> pRT_ALD_1 && pTS-> pRT_ALD_1 d3d_svo.cpp 857

void CSvoRenderer::ConeTracePass(SSvoTargetsSet* pTS)
{
  ....
  if (pTS->pRT_ALD_1 && pTS->pRT_ALD_1)
  {
    staticint nPrevWidth = 0;
    if (....)
    {
      ....
    }
    else
    {
      pTS->pRT_ALD_1->Apply(10, m_nTexStateLinear);
      pTS->pRT_RGB_1->Apply(11, m_nTexStateLinear);
    }
  }
  ....
}

Returning to the Lumberyard code. The condition checks the same pTS-> pRT_ALD_1 pointer . One of them was supposed to be pTS-> pRT_RGB_1 . Perhaps, even after the explanation, it is not immediately possible to see the difference, but there is one: the difference in the short substrings ALD and RGB . If you are told that a manual Code Review is enough, then show this example.

And if this example is not enough, then there are 5 more

• V501 There are identical to the left and the right of the '|| operator:! pTS-> pRT_ALD_0 ||! pTS-> pRT_ALD_0 d3d_svo.cpp 1041
• V501 operator: m_pRT_AIR_MIN && m_pRT_AIR_MIN d3d_svo.cpp 1808
• V501 operator: m_pRT_AIR_MAX && m_pRT_AIR_MAX d3d_svo.cpp 1819
• V501 operator: m_pRT_AIR_SHAD && m_pRT_AIR_SHAD d3d_svo.cpp 1830
• V501 operator: s_pPropertiesPanel && s_pPropertiesPanel entityobject.cpp 1700

And as I promised, here is a list of the remaining V501 warnings without code examples:

About camera position in games

There is the second cool diagnosis in PVS-Studio - V502 . This diagnostics is older than some new programming languages ​​in which such an error can no longer be made. And for C ++, this error will be relevant, perhaps, always.

Let's start with a small simple example.

V502 Perhaps the '?:' Operator was different. The '?:' Operator has a lower limit than the '+' operator. zipencryptor.cpp 217

bool ZipEncryptor::ParseKey(....)
{
  ....
  size_t pos = i * 2 + (v1 == 0xff) ? 1 : 2;
  RCLogError("....", pos);
  returnfalse;
  ....
}

The operation of addition has a higher priority than the ternary operator. Consequently, this expression has a completely different calculation logic than the author intended.

You can fix the error this way:

size_t pos = i * 2 + (v1 == 0xff ? 1 : 2);

V502 Perhaps the '?:' Operator was different. The '?:' Operator has a lower limit than the '-' operator. 3dengine.cpp 1898

float C3DEngine::GetDistanceToSectorWithWater()
{
  ....
  return (bCameraInTerrainBounds && (m_pTerrain &&
          m_pTerrain->GetDistanceToSectorWithWater() > 0.1f)) ?
          m_pTerrain->GetDistanceToSectorWithWater() :
          max(camPostion.z - OceanToggle::IsActive() ?
          OceanRequest::GetOceanLevel() : GetWaterLevel(), 0.1f);
}

Here is an example of the code where they work with the camera position. An example is difficult for perception by the eyes and there is an error in it. For publication, the formatting of the code was changed, but in the source file this code is even more unreadable.

An error is present in this substring:

camPostion.z - OceanToggle::IsActive() ? .... : ....

If the camera in your game suddenly began to behave inadequately, then you should know, the developers saved on static code analysis: D.

Other examples with similar warnings:

• V502 Perhaps the '?:' Operator was different. The '?:' Operator has a lower limit than the '-' operator. scriptbind_ai.cpp 5203
• V502 Perhaps the '?:' Operator was different. The '?:' Operator has a lower limit than the '+' operator. qcolumnwidget.cpp 136
• V502 Perhaps the '?:' Operator was different. The '?:' Operator has a operator. shapetool.h 98

CryEngine Heritage

Amazon Lumberyard is based on CryEngine code . And not on the best of his version. I made such conclusion, having looked at the report of the analyzer. Some bugs found have already been fixed in the latest version of CryEngine from my two code reviews, but are still present in the Lumberyard code. Also over the past year, the analyzer has been significantly improved and it was possible to find additional errors that are now present in two game engines. But the situation with Lumberyard is worse. The legacy of Amazon essentially got all the technical debt of CryEngine. Well, its own technical debt, of course, in each company appears on its own :).

In this section, I will cite just a couple of errors that have been fixed in the latest version of CryEngine, and now remain only a problem of the Lumberyard project.

V519 The 'BlendFactor [2]' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 1283, 1284. ccrydxgldevicecontext.cpp 1284

Approximately such emotions will be experienced by the developers of Lumberyard, when they learn that this error has remained only with them.

By the way there are two more:

• V519 The 'm_auBlendFactor [2]' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 919, 920. ccrydxgldevicecontext.cpp 920
• V519 The 'm_auBlendFactor [2]' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 926, 927. ccrydxgldevicecontext.cpp 927

There is this error:

V546 Member of a class is initialized by itself: 'eConfigMax (eConfigMax.VeryHigh)'. particleparams.h 1837

ParticleParams() :
  ....
  fSphericalApproximation(1.f),
  fVolumeThickness(1.0f),
  fSoundFXParam(1.f),
  eConfigMax(eConfigMax.VeryHigh), // <=
  fFadeAtViewCosAngle(0.f)
{}

In CryEngine, this class was generally rewritten, and then the initialization error remained.

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

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

Suspicious cycle, which has already been rewritten in CryEngine too.

Mistakes live longer than you think.

The users who are starting to use PVS-Studio for the first time have approximately the same situation: they find an error, find out that they added it several months ago and are happy to realize that they miraculously avoided manifestation of the problem with their users. Many of our clients came to the regular use of PVS-Studio just after such stories.

Sometimes a company has to go through such situations several times in order to launch quality control processes. Here is an example about CryEngine and Lumberyard:

V557 CWE-119 Array overrun is possible. The 'id' index is pointing beyond array bound. gameobjectsystem.cpp 113

uint32 CGameObjectSystem::GetExtensionSerializationPriority(....)
{
  if (id > m_extensionInfo.size())
  {
    return0xffffffff; // minimum possible priority
  }
  else
  {
    return m_extensionInfo[id].serializationPriority;
  }
}

As you know, Amazon Lumberyard is not based on the latest version of CryEngine. Nevertheless, with the help of the PVS-Studio analyzer, we managed to find an error that is present in two game engines now. It was necessary to check the index with the operator '> =' ... The

error with indexing is serious. Moreover, there are six such places ! Here is another example:

V557 CWE-119 Array overrun is possible. The 'index' index is pointing beyond array bound. vehicleseatgroup.cpp 73

CVehicleSeat* CVehicleSeatGroup::GetSeatByIndex(unsigned index)
{
  if (index >= 0 && index <= m_seats.size())
  {
    return m_seats[index];
  }
  returnNULL;
}

Someone has made the same type of errors, and they were not corrected just because they didn’t fall into the reviews of CryEngine errors.

Remaining warnings:

• V557 CWE-119 Array overrun is possible. The 'id' index is pointing beyond array bound. gameobjectsystem.cpp 195
• V557 CWE-119 Array overrun is possible. The 'id' index is pointing beyond array bound. gameobjectsystem.cpp 290
• V557 CWE-119 Array overrun is possible. The 'stateId' index is beyond array bound. vehicleanimation.cpp 311
• V557 CWE-119 Array overrun is possible. The 'stateId' index is beyond array bound. vehicleanimation.cpp 354

The long existence of errors in the code can be explained only by the appropriate level of testing project. It is believed that static analysis finds errors only in unused code. So, it is not. Developers forget that the majority of users silently suffer from unobvious bugs in programs, and the manifestation of these rarest bugs often adversely affects the work of the entire company, its reputation and its sales, if any.

Different shades of copy-paste programming

Having reached this section of the article, you probably noticed that programming with the Copy-Paste method is the cause of many problems. In PVS-Studio, the search for such errors is implemented in various diagnostics. In this section, we will give examples of copypastes found using the V561 .

Here is an example of suspicious code when declaring variables with the same name in intersecting scopes.

V561 CWE-563 It's not better to declare it’s variable than to declare it anew. Previous declaration: entityobject.cpp, line 4703. entityobject.cpp 4706

void CEntityObject::OnMenuConvertToPrefab()
{
  ....
  IDataBaseLibrary* pLibrary = GetIEditor()->Get....;
  if (pLibrary == NULL)
  {
    IDataBaseLibrary* pLibrary = GetIEditor()->Get....;
  }
  if (pLibrary == NULL)
  {
    QString sError = tr(....);
    CryMessageBox(....);
    return;
  }
  ....
}

The 'pLibrary' pointer is not overwritten, as expected. The initialization of this pointer was completely copied under the condition along with the type declaration.

I will give a list of all similar places:

• V561 CWE-563 It's not better to declare it anew. Previous declaration: toglsloperand.c, line 838. toglsloperand.c 1224
• V561 CWE-563 It's not better to declare it anew. Previous declaration: toglsloperand.c, line 838. toglsloperand.c 1305
• V561 CWE-563 It's not better to declare it anew. Previous declaration: attachmentmanager.cpp, line 409. attachmentmanager.cpp 458
• V561 CWE-563 It's not better to declare it nThreadID variable than to declare it anew. Previous declaration: d3dmeshbaker.cpp, line 797. d3dmeshbaker.cpp 867
• V561 CWE-563 It's a bit different than to declare it anew. Previous declaration: assetimportermanager.cpp, line 720. assetimportermanager.cpp 728
• V561 CWE-563 It's not better to declare value to 'pNode' variable than to declare it anew. Previous declaration: breakpointsctrl.cpp, line 340. breakpointsctrl.cpp 349
• V561 CWE-563 It's not better to declare it’s variable than to declare it anew. Previous declaration: prefabobject.cpp, line 1443. prefabobject.cpp 1446
• V561 CWE-563 It's not better to declare it’s variable than to declare it anew. Previous declaration: prefabobject.cpp, line 1470. prefabobject.cpp 1473
• V561 CWE-563 It’s variable than to declare it anew. Previous declaration: fileutil.cpp, line 110. fileutil.cpp 130
• V561 CWE-563 It’s variable than to declare it anew. Previous declaration: attributeitemlogiccallbacks.cpp, line 291. attributeitemlogiccallbacks.cpp 303
• V561 CWE-563 It's not better to declare it anew. Previous declaration: qgradientselectorwidget.cpp, line 475. qgradientselectorwidget.cpp 488

A large list ... some of the places listed are even full copies of the example described.

Initialization by own value

In the code of the game engine found a lot of places where the variable is assigned to itself. Somewhere this code remained for debugging, somewhere the code is simply beautifully decorated (it is also often the source of errors), therefore I will give one most suspicious code fragment for me.

V570 The 'behaviorParams.ignoreOnVehicleDestroyed' variable is assigned to itself. vehiclecomponent.cpp 168

bool CVehicleComponent::Init(....)
{
  ....
  if (!damageBehaviorTable.getAttr(....)
  {
    behaviorParams.ignoreOnVehicleDestroyed = false;
  }
  else
  {
    behaviorParams.ignoreOnVehicleDestroyed =      // <=
      behaviorParams.ignoreOnVehicleDestroyed;     // <=
  }
  ....
}

In the current view, the else branch is not needed at all. But perhaps this code fragment contains an error: we wanted to assign the opposite value to the variable:

bValue = !bValue

But with the results of this diagnosis, developers better get to know yourself.

Handling problematic situations

Many examples will be given in this section when something went wrong in error handling.

Example 1 .

V606 Ownerless token 'nullptr'. dx12rootsignature.cpp 599

RootSignature* RootSignatureCache::AcquireRootSignature(....)
{
  ....
  RootSignature* result = new RootSignature(m_pDevice);
  if (!result->Init(params))
  {
    DX12_ERROR("Could not create root signature!");
    nullptr;
  }
  m_RootSignatureMap[hash] = result;
    return result;
  }
}

Forgot to write return nullptr; . Now the invalid value of the result variable will be used elsewhere in the code.

Exactly the same code copied to another place:

• V606 Ownerless token 'nullptr'. dx12rootsignature.cpp 621

Example 2.

V606 Ownerless token 'false'. fillspacetool.cpp 191

bool FillSpaceTool::FillHoleBasedOnSelectedElements()
{
  ....
  if (validEdgeList.size() == 2)
  {
    ....
  }
  if (validEdgeList.empty())
  {
      ....
      for (int i = 0, iVertexSize(....); i < iVertexSize; ++i)
      {
          validEdgeList.push_back(....);
      }
  }
  if (validEdgeList.empty())                  // <=
  {
      false;                                  // <= fail
  }
  std::vector<BrushEdge3D> linkedEdgeList;
  std::set<int> usedEdgeSet;
  linkedEdgeList.push_back(validEdgeList[0]); // <= fail
  ....
}

A very interesting example of an error with a missing return statement . Now it is possible to access the index by an empty container.

Example 3 .

V564 CWE-480 The '&' operator is applied to bool type value. You have probably forgotten to include the operator. toglslinstruction.c 2914

voidSetDataTypes(....){
 ....
 // Check assumption that both the values which MOVC might pick// have the same basic data type.if(!psContext->flags & HLSLCC_FLAG_AVOID_TEMP_REGISTER_ALIASING)
 {
   ASSERT(GetOperandDataType(psContext, &psInst->asOperands[2])
     == GetOperandDataType(psContext, &psInst->asOperands[3]));
 }
 ....
}

Incorrectly checked for bitics in the flag. The negation operator applies to the value of the flag, not the entire expression. Correctly write like this:

if(!(psContext->flags & ....))

More similar warnings:

• V564 CWE-480 The '|' operator is applied to bool type value. You've probably forgotten to include the parentheses or intended to use the '||' operator. d3dhwshader.cpp 1832
• V564 CWE-480 The '&' operator is applied to bool type value. You have probably forgotten to include the operator. trackviewdialog.cpp 2112
• V564 CWE-480 The '|' operator is applied to bool type value. You've probably forgotten to include the parentheses or intended to use the '||' operator. imagecompiler.cpp 1039

Example 4.

V596 CWE-390 The object was not used. The 'throw' keyword could be missing: throw runtime_error (FOO); prefabobject.cpp 1491

staticstd::vector<std::string> PyGetPrefabLibrarys()
{
  CPrefabManager* pPrefabManager = GetIEditor()->GetPrefabMa....;
  if (!pPrefabManager)
  {
      std::runtime_error("Invalid Prefab Manager.");
  }
  ....
}

Error generating exception. It was necessary to write like this:

throwstd::runtime_error("Invalid Prefab Manager.");

The entire list of such errors:

• V596 CWE-390 The 'throw' keyword could be missing: throw runtime_error (FOO); prefabobject.cpp 1515
• V596 CWE-390 The 'throw' keyword could be missing: throw runtime_error (FOO); prefabobject.cpp 1521
• V596 CWE-390 The 'throw' keyword could be missing: throw runtime_error (FOO); prefabobject.cpp 1543
• V596 CWE-390 The 'throw' keyword could be missing: throw runtime_error (FOO); prefabobject.cpp 1549
• V596 CWE-390 The 'throw' keyword could be missing: throw runtime_error (FOO); prefabobject.cpp 1603
• V596 CWE-390 The 'throw' keyword could be missing: throw runtime_error (FOO); prefabobject.cpp 1619
• V596 CWE-390 The 'throw' keyword could be missing: throw runtime_error (FOO); prefabobject.cpp 1644

A couple of problems when working with memory

V549 CWE-688 The first argument of the memcmp function is equal to the second argument. meshutils.h 894

structVertexLess
{
 ....
 booloperator()(int a, int b)const{
   ....
   if (m.m_links[a].links.size() != m.m_links[b].links.size())
   {
     res = (m.m_links[a].links.size() <
            m.m_links[b].links.size()) ? -1 : +1;
   }
   else
   {
     res = memcmp(&m.m_links[a].links[0], &m.m_links[a].links[0],
     sizeof(m.m_links[a].links[0]) * m.m_links[a].links.size());
   }
   ....
 }
 ....
};

The condition compares the sizes of the two vectors. If they are equal, then in the else branch the values ​​of the first elements of the vectors are compared using the memcmp () function . But the first and second arguments of this function are the same! Access to the elements of the array is quite cumbersome. There are indices a and b . Most likely, a typo in them.

V611 CWE-762 Consider inspecting this code. It's probably better to use 'delete [] data;'. vectorn.h 102

~vectorn_tpl()
{
  if (!(flags & mtx_foreign_data))
  {
    delete[] data;
  }
}
vectorn_tpl& operator=(const vectorn_tpl<ftype>& src)
{
  if (src.len != len && !(flags & mtx_foreign_data))
  {
    delete data;  // <=
    data = new ftype[src.len];
  }
  ....
}

Memory by dat pointer is freed by the wrong operator. The delete [] operator should be used everywhere .

Unreachable code

V779 CWE-561 Unreachable code detected. It is possible that an error is present. fbxskinimporter.cpp 67

Events::ProcessingResult FbxSkinImporter::ImportSkin(....)
{
  ....
  if (BuildSceneMeshFromFbxMesh(....)
  {
    context.m_createdData.push_back(std::move(createdData));
    return Events::ProcessingResult::Success;   // <=
  }
  else
  {
    return Events::ProcessingResult::Failure;   // <=
  }
  context.m_createdData.push_back();            // <= failreturn Events::ProcessingResult::Success;
}

All branches of the conditional statement are terminated by exiting the function. In this case, some code is not executed.

V779 CWE-561 Unreachable code detected. It is possible that an error is present. dockablelibrarytreeview.cpp 153

bool DockableLibraryTreeView::Init(IDataBaseLibrary* lib)
{
  ....
  if (m_treeView && m_titleBar && m_defaultView)
  {
    if (m_treeView->topLevelItemCount() > 0)
    {
      ShowTreeView();
    }
    else
    {
      ShowDefaultView();
    }
    returntrue;                // <=
  }
  else
  {
    returnfalse;               // <=
  }
  emit SignalFocused(this);     // <= fail
}

It is easy to notice an error on this code snippet. But if you write code for a long time, attention decreases sharply and such errors easily get into the project.

V622 CWE-478 Consider inspecting the 'switch' statement. It's possible that the operator is missing. datum.cpp 872

AZ_INLINE boolIsDataGreaterEqual(....){
  switch (type.GetType())
  {
    AZ_Error("ScriptCanvas", false, "....");
    returnfalse;
  case Data::eType::Number:
    return IsDataGreaterEqual<Data::NumberType>(lhs, rhs);
  ....
  case Data::eType::AABB:
    AZ_Error("ScriptCanvas", false, "....",
      Data::Traits<Data::AABBType>::GetName());
    returnfalse;
  case Data::eType::OBB:
    AZ_Error("ScriptCanvas", false, "....",
      Data::Traits<Data::OBBType>::GetName());
    returnfalse;
  ....
}

If the switch contains code outside the case / default statement , then it is never executed.

Conclusion

The article includes 95 warnings of the analyzer, 25 of them with code examples. How much material is it from the entire analyzer report? I quickly scrolled through a third of the High level alerts . There are also Medium and Low, a group of diagnostics for searching optimizations and other undeveloped analyzer capabilities - these are hundreds of obvious errors and thousands of unexplored warnings.

And here the reader should ask himself the question: “Is it possible with such an approach to the project to release a good game engine?”. There is no quality control code. The CryEngine code with old errors was taken as a basis, new errors were added. CryEngine itself is being finalized only after the next review of the code. Amazon has every chance with its resources to work towards the quality of the code and release the coolest game engine!

Do not get very upset. Among the clients of PVS-Studio there are more than thirty other companies that are engaged in games. You can get acquainted with them and their products on the page of our site “ Clients ” by selecting the “Game Development” filter. So we are gradually improving the world. Perhaps we can improve on Amazon Lumberyard :).

On the subject of quality gaming software colleague recently I wrote an article, I suggest interested to read: " The static analysis of the video game industry: the top 10 programming errors ."

Link to the download of the PVS-Studio analyzer , as without it ;-)

If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Amazon Lumberyard: A Scream of Anguish