Video games are among the most popular software products. Now a new game engine, Amazon Lumberyard, has joined this huge industry. The project is currently in the beta phase and still has time to get rid of the bugs and improve. Its authors have a lot of work to do to make sure they don't disappoint millions of gamers and game developers in the nearest future.
Amazon Lumberyard is a free cross-platform triple-A game engine developed by Amazon and based on the architecture of CryEngine, which was licensed from Crytek in 2015. I already checked CryEngine twice, in august 2016 and april 2017, and I'm sorry to say it, but the code quality had decreased since the first check. I was wondering the other day how Amazon had used the engine and took a look at the new product. I must admit they did make a great environment. The user documentation and environment deployment software are really awesome too. But the code is messed up again! I hope Amazon can afford to allocate much more resources for that project and will start caring about code quality at last. By writing this review I hope to draw their attention to this problem and persuade them to take a new approach to the development process. The code's current state is so bad that I had to change the title and the featured image of the article several times as I was going through the analysis report. The first version of the image was less emotional:
The source files under analysis refer to the latest Amazon Lumberyard version, 1.14.0.1, and were downloaded from the GitHub repository. Star Citizen is one of the first games to be built on the Lumberyard engine. If you are looking to play it, then welcome to take a peek under the hood with me.
The check was done with the static analyzer PVS-Studio, which is available on Windows, Linux, and macOS. It means that you can choose among several options for comfortable work when checking a cross-platform project. In addition to C and C++, the analyzer can check projects written in C#. We also plan on adding a Java module in the future. The vast majority of the code all over the world is written (not without mistakes, of course) in these languages, so you definitely should try PVS-Studio with your project - you'll be surprised at what you'll find ;-).
Just like CryEngine, Lumberyard uses the WAF build system. PVS-Studio has no special means to integrate with this system, so I chose to run the check in Windows mode using the compiler monitoring system. The Visual Studio solution file is generated automatically; you can use it to build the application and view the analysis report.
The analysis command rundown looks something 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.plogAs I already said, the report can be viewed in Visual Studio.
Amazon Lumberyard is positioned as a cross-platform game engine. The 'cross-platform' feature makes a product easy to promote but hard to maintain. One of the PVS-Studio warnings was triggered by a code fragment where a programmer named Igor was struggling with the Qualcomm compiler. Maybe he did solve the task, but the code he left was still very suspicious. Here's a screenshot.
V523 The 'then' statement is equivalent to the 'else' statement. toglsloperand.c 700
Both conditions have the same logic. With all those comments, this solution doesn't look right.
What you see in this code is either redundant conditions or a real bug, and it's not the only such case:
Here's a funny code fragment found by the analyzer:
V709 CWE-682 Suspicious comparison found: 'a == b == c'. Remember that 'a == b == c' is not equal to 'a == b && b == c'. toglslinstruction.c 564
void CallBinaryOp(....)
{
....
uint32_t src1SwizCount = GetNumSwizzleElements(....);
uint32_t src0SwizCount = GetNumSwizzleElements(....);
uint32_t dstSwizCount = GetNumSwizzleElements(....);
....
if (src1SwizCount == src0SwizCount == dstSwizCount) // <=
{
....
}
....
}Sadly, this code successfully compiles in C++, but its logic is nothing like what you expect. In C++, expressions are evaluated based on operator precedence, with implicit casts where necessary.
A check like that would be fine in a language such as Python. But here the developer just ended up "shooting themselves in the foot".
Three more "finishing shots":
Here we'll talk about V501, our first general-analysis diagnostic. The number of defects found with this diagnostic alone would be sufficient for a large article. And Amazon Lumberyard proves that perfectly.
Reviewing similar defects gets boring quickly, so I'll include only a couple of examples here and just list the rest at the end of this section.
V501 There are identical sub-expressions to the left and to 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 variable hotY is missing in the condition, which is a classic typo.
V501 There are identical sub-expressions 'sp.m_pTexture == m_pTexture' to the left and to the right of the '&&' operator. shadercomponents.h 487
V501 There are identical sub-expressions 'sp.m_eCGTextureType == m_eCGTextureType' to the left and to the right of the '&&' operator. shadercomponents.h 487
bool operator != (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)
{
return false;
}
return true;
}This code fragment contains two copy-paste related bugs at once - see the arrows.
V501 There are identical sub-expressions to the left and to the right of the '==' operator: pSrc.GetLen() == pSrc.GetLen() fbxpropertytypes.h 978
inline bool FbxTypeCopy(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;
}Say 'Hello' to the AUTODESK developers! This bug comes from their library FBX SDK and has to do with swapped variables pSrc and pDst. I believe there are lots of other users beside Lumberyard whose projects depend on this code.
V501 There are identical sub-expressions to the left and to the right of the '&&' 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)
{
static int nPrevWidth = 0;
if (....)
{
....
}
else
{
pTS->pRT_ALD_1->Apply(10, m_nTexStateLinear);
pTS->pRT_RGB_1->Apply(11, m_nTexStateLinear);
}
}
....
}Getting back to Lumberyard, the condition above checks for the pointer pTS->pRT_ALD_1 twice, while one of those checks must be pTS->pRT_RGB_1 instead. Even after my explanation, you may still miss the difference, but it's there - in the tiny substrings ALD and RGB. Next time you hear someone say manual code review is enough, show them this example.
And if they are not convinced, here are five more:
As promised, here's the list of the rest V501 warnings without code:
V502 is PVS-Studio's second toughest diagnostic. It's older than some of the new programming languages, which don't allow making this mistake anymore. In C++, though, this warning will always have a job, I suspect.
Let's start with a small simple example.
V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '+' operator. zipencryptor.cpp 217
bool ZipEncryptor::ParseKey(....)
{
....
size_t pos = i * 2 + (v1 == 0xff) ? 1 : 2;
RCLogError("....", pos);
return false;
....
}The addition operator has higher precedence than the ternary operator. The evaluation logic of this expression is, therefore, quite different from what the author intended.
The error can be fixed as follows:
size_t pos = i * 2 + (v1 == 0xff ? 1 : 2);V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority 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);
}Now, here's an example of code that handles the camera position. There's a bug in this code, but it's not easy to see. I reformatted the code a bit for the article, but, believe me, the original version is even less readable.
The error is hiding in this subexpression:
camPostion.z - OceanToggle::IsActive() ? .... : ....So, now you know that if the camera in your game starts acting up suddenly, it's because the engine developers didn't invest into static code analysis :D.
Other similar warnings:
Amazon Lumberyard is based on CryEngine's code, and, sadly, not the best version of it. It's the analysis report that makes me think so. The developers of CryEngine fixed some of the bugs in its latest version based on my two reviews, but those bugs are still living in the code of Lumberyard. We have greatly improved PVS-Studio over the last year, too, and can now detect some more bugs shared by both engines. Lumberyard is a bit messier, though. Amazon, in fact, inherited all of CryEngine's technical debt and are now building up their own, just like any other company does :).
I'll show you just a couple of bugs fixed in the latest version of CryEngine and now found only in 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
That's what the Lumberyard developers are going to feel like when they learn CryEngine has this bug long fixed and they are the only ones who still have it.
There are two more defects like that, by the way:
Another 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 has been rewritten entirely, while Lumberyard still retains the initialization error.
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
{
string sInput(sSpec);
const int MAXWORDS = 8;
string sWords[MAXWORDS];
int iC = 0, iWord = 0;
for (; iWord < MAXWORDS; !sWords[iWord].empty(), iWord++)
{
sWords[iWord] = sInput.Tokenize("_", iC);
}
....
}This suspicious loop is no longer present in CryEngine.
All of those getting started with PVS-Studio typically go through the same experience: they find a bug introduced a few months earlier and realize they have just saved their users from it, and that makes them happy. It was after this revelation that many of our customers started using PVS-Studio on a regular basis.
Sometimes a company has to experience that more than once to finally start caring about quality control. The following defect is shared by 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())
{
return 0xffffffff; // minimum possible priority
}
else
{
return m_extensionInfo[id].serializationPriority;
}
}As I said, the CryEngine version that Amazon Lumberyard is based on is not the freshest available. Still, PVS-Studio spotted a bug that's currently shared by both engines. In the code above, the index should be checked using the '>=' operator...
This indexing bug is really critical. What's more, there are six of them! 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];
}
return NULL;
}Someone made a bunch of similar mistakes, which weren't fixed only because I hadn't mentioned them in my previous reviews.
The other warnings:
The fact that errors live for so long can be explained only by a lack of testing. Some programmers believe static analysis can effectively find bugs only in unused code. But that's not true. They forget that most users keep silent about the rare, irregular bugs, but when such bugs do occur they damage the company's progress, reputation, and sales, if any.
By now, you must have noticed that copy-paste programming is the source of much trouble. PVS-Studio employs a number of diverse diagnostics to detect such bugs. This section shows a few examples of copy-paste related defects found with the V561 diagnostic.
The code below contains suspicious declarations of variables with identical names belonging to overlapping scopes.
V561 CWE-563 It's probably better to assign value to 'pLibrary' 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 reassigned as expected. The whole line with the pointer initialization code and the type declaration was copied and pasted under the condition.
Here are all the warnings of this type:
That's quite a lot, isn't it? Some of these are exact clones of the fragment above.
The engine's code contains tons of variables assigned to themselves. In some cases, it's code left for debugging purposes; in others, it's just nicely formatted code (which is also a frequent source of errors), so I'll show you just one such code fragment, which I feel most suspicious about.
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; // <=
}
....
}There's no use keeping the else branch in the current version of the code. But it may also indicate a mistake: the programmer probably intended to assign the variable an opposite value:
bValue = !bValueThe developers, however, should check this case for themselves to be sure.
This section contains lots of examples of broken error-handling code.
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;
}
}The programmer left out the return nullptr; line, so the invalid value of the result variable will now be used elsewhere in the code.
This is an exact copy of the snippet above:
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
....
}This is quite an interesting example of a missing return statement. This mistake enables indexing into an empty container.
Example 3.
V564 CWE-480 The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. toglslinstruction.c 2914
void SetDataTypes(....)
{
....
// 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]));
}
....
}Incorrect check of the flag bits. The negation operator is applied to the value of the flag rather than the whole expression. Correct version:
if(!(psContext->flags & ....))Other warnings of this type:
Example 4.
V596 CWE-390 The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error(FOO); prefabobject.cpp 1491
static std::vector<std::string> PyGetPrefabLibrarys()
{
CPrefabManager* pPrefabManager = GetIEditor()->GetPrefabMa....;
if (!pPrefabManager)
{
std::runtime_error("Invalid Prefab Manager.");
}
....
}Exception-throwing error. Correct version:
throw std::runtime_error("Invalid Prefab Manager.");The rest errors of this type:
V549 CWE-688 The first argument of 'memcmp' function is equal to the second argument. meshutils.h 894
struct VertexLess
{
....
bool operator()(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 two vectors: if they are equal, the else branch is executed, where the values of the vectors' first elements are compared using the memcmp() function. But the problem is that the first and second arguments of this function are the same! The array elements are accessed in quite a complicated way using the indexes a and b - it must be one of them that was mistyped.
V611 CWE-762 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. 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];
}
....
}The memory block referred to by the data pointer is released using the wrong operator. The correct operator is delete[].
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(); // <= fail
return Events::ProcessingResult::Success;
}Every branch of the conditional statement ends with a return statement, preventing control from executing some of the code.
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();
}
return true; // <=
}
else
{
return false; // <=
}
emit SignalFocused(this); // <= fail
}You can easily spot the error in this fragment, but when coding for a long time, you get less focused and let defects like this slip into the release version.
V622 CWE-478 Consider inspecting the 'switch' statement. It's possible that the first 'case' operator is missing. datum.cpp 872
AZ_INLINE bool IsDataGreaterEqual(....)
{
switch (type.GetType())
{
AZ_Error("ScriptCanvas", false, "....");
return false;
case Data::eType::Number:
return IsDataGreaterEqual<Data::NumberType>(lhs, rhs);
....
case Data::eType::AABB:
AZ_Error("ScriptCanvas", false, "....",
Data::Traits<Data::AABBType>::GetName());
return false;
case Data::eType::OBB:
AZ_Error("ScriptCanvas", false, "....",
Data::Traits<Data::OBBType>::GetName());
return false;
....
}If switch contains code outside the case/default labels, control will never reach that code.
I included 95 warnings in this article, 25 of them accompanied by examples. How much is that of the total number? Well, what you saw is just one third of the High-level warnings, which I just quickly scrolled through. Add Medium and Low levels, a suite of diagnostics for micro-optimizations, and other features left unused - all of that would make hundreds of evident bugs and thousands of unclear cases.
Now ask yourself, "Can one make a good game engine with such an attitude to one's product?" There's no code quality control. They just took the code of CryEngine with the old bugs and added their own. CryEngine itself is taken care of only after we post a review. Amazon, with all its resources, has every chance to improve their code and make the coolest game engine ever!
But there's no reason to be sad. Over thirty other game developer companies use PVS-Studio. The list of these companies and their products can be found on the "Our customers" page at our website (enable the "Game Development" filter). That's how we are gradually making the world a better place. I hope we'll help Amazon Lumberyard become better too :)
My coworker has recently written an article about code quality in game software: "Static Analysis in Video Game Development: Top 10 Software Bugs". Come by and take a look!
And, of course, here's the PVS-Studio download link ;-)