Just before the release of the "Amnesia: Rebirth" game, the vendor "Fractional Games" opened the source code of the legendary "Amnesia: The Dark Descent" and its sequel "Amnesia: A Machine For Pigs". Why not use the static analysis tool to see what dreadful mistakes are hidden in the inside of these cult horror games?
After seeing the news on Reddit that the source code of the games "Amnesia: The Dark Descent" and "Amnesia: A Machine for Pigs" was released, I couldn't pass by and not check this code using PVS-Studio, and at the same time write an article about it. Especially since the new part of this game-series - "Amnesia: Rebirth" is released on the 20th of October (and at the moment of posting this article the game is already released).
"Amnesia: The Dark Descent" was released in 2010 and became a cult game in the survival horror genre. Frankly speaking, I have never been able to play through it, even a bit. The reason is that in horror games I play by one algorithm: install, run for five minutes, exit by "alt+f4" at the first creepy moment and delete the game. But I liked watching this game run through videos on YouTube.
In case someone is not familiar with PVS-Studio yet, this is a static analyzer that looks for errors and suspicious places in source code of programs.
I especially like delving into source code of games. So, if you are interested in what errors are made in games, you can read my previous articles. Also check out the articles by my colleagues about checking source code of games.
After checking, it turned out that a large amount of code overlaps between "The Dark Descent" and "A Machine For Pigs", and the reports for these two projects were very similar. So almost all the errors that I will cite further take place in both projects.
The better half of errors found by the analyzer in these projects was copy-paste errors. This explains the title of the article. The main reason for these errors is the "last line effect".
Let's get right to it.
There were a lot of suspicious places that looked like inattentive copying. Some cases may be due to the internal logic of the game itself. Once they confused both the analyzer and me, at least a comment could be of use. After all, other developers may be as clueless as I am.
Fragment 1.
Let's start with an example where the entire function consists of comparing method results and fields values of two objects aObjectDataA and aObjectDataB. I will cite the entire function for clarity. Try to see for yourself where the error was made in the function:
static bool SortStaticSubMeshesForBodies(const ....& aObjectDataA,
const ....& aObjectDataB)
{
//Is shadow caster check
if( aObjectDataA.mpObject->GetRenderFlagBit(....)
!= aObjectDataB.mpObject->GetRenderFlagBit(....))
{
return aObjectDataA.mpObject->GetRenderFlagBit(....)
< aObjectDataB.mpObject->GetRenderFlagBit(....);
}
//Material check
if( aObjectDataA.mpPhysicsMaterial != aObjectDataB.mpPhysicsMaterial)
{
return aObjectDataA.mpPhysicsMaterial < aObjectDataB.mpPhysicsMaterial;
}
//Char collider or not
if( aObjectDataA.mbCharCollider != aObjectDataB.mbCharCollider)
{
return aObjectDataA.mbCharCollider < aObjectDataB.mbCharCollider;
}
return aObjectDataA.mpObject->GetVertexBuffer()
< aObjectDataA.mpObject->GetVertexBuffer();
}Here's a pic for you to avoid accidentally spying on the answer:
Did you find a bug? So, in the last return, there is a comparison using aObjectDataA on both sides. Note that all the expressions in the original code were written in a line. Here I broke lines so that everything fit exactly in the width of the line. Just imagine how hard it will be to look for such a flaw at the end of the working day. Whereas the analyzer will find it immediately after building the project and running incremental analysis.
V501 There are identical sub-expressions 'aObjectDataA.mpObject->GetVertexBuffer()' to the left and to the right of the '<' operator. WorldLoaderHplMap.cpp 1123
As a result, such an error will be found almost at the time of writing the code, instead of hiding in the depths of the code from several stages of Q&A, making your search much more difficult.
Note by my colleague Andrey Karpov. Yes, this is a classic "last line effect" error. Moreover, this is also a classic pattern of the error related to comparing two objects. See the article "The Evil within the Comparison Functions".
Fragment 2.
Let's take a quick look at the code that triggered the warning:
Here is a screenshot of the code for clarity.
This is how the warning looks like:
V501 There are identical sub-expressions 'lType == eLuxJournalState_OpenNote' to the left and to the right of the '||' operator. LuxJournal.cpp 2262
The analyzer found that there is an error in the check of the lType variable value. The equality with the same element of the eLuxJournalState_OpenNote enumerator is checked twice.
First, I wish this condition would be written in a table-like format for better readability. See the chapter 13 from the mini-book "The ultimate question of programming, refactoring, and everything" for more details.
if(!( lType == eLuxJournalState_OpenNote
|| lType == eLuxJournalState_OpenDiary
|| lType == eLuxJournalState_OpenNote
|| lType == eLuxJournalState_OpenNarratedDiary))
return false;In this form, it becomes much easier to notice the error even without the analyzer.
Anyway, here comes a question - does such an erroneous check lead to logic distortion of the program? After all, one may need to check some other lType value, but the check was missed due to a copy-paste error. So, let's take a look at the enumeration itself:
enum eLuxJournalState
{
eLuxJournalState_Main,
eLuxJournalState_Notes,
eLuxJournalState_Diaries,
eLuxJournalState_QuestLog,
eLuxJournalState_OpenNote,
eLuxJournalState_OpenDiary,
eLuxJournalState_OpenNarratedDiary,
eLuxJournalState_LastEnum,
};There are only three values with the word "Open" in their name. All three are present in the check. Most likely, there is no logic distortion here, but we can hardly know for sure. So, the analyzer either found a logical error that the game developer could fix, or found an "ugly" written snippet that would be worth rewriting for better elegancy.
Fragment 3.
The following case is generally the most obvious example of a copy-paste error.
V778 Two similar code fragments were found. Perhaps, this is a typo and 'mvSearcherIDs' variable should be used instead of 'mvAttackerIDs'. LuxSavedGameTypes.cpp 615
void cLuxMusicHandler_SaveData::ToMusicHandler(....)
{
....
// Enemies
//Attackers
for(size_t i=0; i<mvAttackerIDs.Size(); ++i)
{
iLuxEntity *pEntity = apMap
->GetEntityByID(mvAttackerIDs[i]);
if(....)
{
....
}
else
{
Warning("....", mvAttackerIDs[i]);
}
}
//Searchers
for(size_t i=0; i<mvSearcherIDs.Size(); ++i)
{
iLuxEntity *pEntity = apMap->GetEntityByID(mvSearcherIDs[i]);
if(....)
{
....
}
else
{
Warning("....", mvAttackerIDs[i]);
}
}
}In the first loop, the pEntity pointer (obtained via mvAttackerIDs) is handled. If the condition is not met, a debug message is issued for the same mvAttackerIDs. However, in the next loop, which is formed the same as the previous code section, pEntity is obtained using mvSearcherIDs. While the warning is still issued with the mention of mvAttackerIDs.
Most likely, the code block with the note "Searchers" was copied from the "Attackers" block, mvAttackerIDs was replaced with mvSearcherIDs, but the else block was not changed. As a result, the error message uses an element of the wrong array.
This error does not affect the logic of the game, but this way you can play a dirty trick on a person who will have to debug this place, and waste time working with the wrong mvSearcherIDs element.
Fragment 4.
The analyzer indicated the next suspicious fragment with as many as three warnings:
Take a look at the code:
void __stdcall cLuxScriptHandler::PlaceEntityAtEntity(....)
{
cLuxMap *pMap = gpBase->mpMapHandler->GetCurrentMap();
iLuxEntity *pEntity = GetEntity(....);
if(pEntity == NULL) return;
if(pEntity->GetBodyNum() == 0)
{
....
}
iPhysicsBody *pBody = GetBodyInEntity(....);
if(pBody == NULL) return;
iLuxEntity *pTargetEntity = GetEntity(....);
if(pEntity == NULL) return; // <=
iPhysicsBody *pTargetBody = GetBodyInEntity(....);
if(pTargetBody == NULL) return;
....
}The V547 warning was issued for the second pEntity == NULL check. For the analyzer, this check will always be false, since if this condition were true, the function would exit earlier due to a previous similar check.
The next warning (V649) was issued just for the fact that we have two identical checks. Usually this case may not be a mistake. Who knows, may be one part of the code implements the same logic, and another part of the code must do something else based on the same check. But in this case, the body of the first check consists of return, so it won't even get to the second check if the condition is true. By tracking this logic, the analyzer reduces the number of false messages for suspicious code and outputs them only for very strange logic.
The error indicated by the last warning is very similar in nature to the previous example. Most likely, all checks were duplicated from the first if(pEntity == NULL) check, and then the object being checked was replaced with the required one. In the case of the pBody and pTargetBody objects, the replacement was made, but the pTargetEntity object was forgotten. As a result, this object is not checked.
If you dig a bit deeper into the code of the example we are considering, it turns out that such an error will not affect the program performance. The pTargetBody pointer gets its value from the GetBodyInEntity function:
iPhysicsBody *pTargetBody = GetBodyInEntity(pTargetEntity,
asTargetBodyName);The first passed argument here is an unchecked pointer that is not used anywhere else. Fortunately, inside this function there is a check of the first argument for NULL:
iPhysicsBody* ....::GetBodyInEntity(iLuxEntity* apEntity, ....)
{
if(apEntity == NULL){
return NULL;
}
....
}As a result, this code works correctly in the end, although it contains an error.
Fragment 5.
Another suspicious place with copy-paste!
In this method, the fields of the cLuxPlayer class object are zeroed.
void cLuxPlayer::Reset()
{
....
mfRoll=0;
mfRollGoal=0;
mfRollSpeedMul=0; // <=
mfRollMaxSpeed=0; // <=
mfLeanRoll=0;
mfLeanRollGoal=0;
mfLeanRollSpeedMul=0;
mfLeanRollMaxSpeed=0;
mvCamAnimPos =0;
mvCamAnimPosGoal=0;
mfRollSpeedMul=0; // <=
mfRollMaxSpeed=0; // <=
....
}But for some reason, the two variables mfRollSpeedMul and mfRollMaxSpeed are zeroed twice:
Let's look at the class itself and at its fields:
class cLuxPlayer : ....
{
....
private:
....
float mfRoll;
float mfRollGoal;
float mfRollSpeedMul;
float mfRollMaxSpeed;
float mfLeanRoll;
float mfLeanRollGoal;
float mfLeanRollSpeedMul;
float mfLeanRollMaxSpeed;
cVector3f mvCamAnimPos;
cVector3f mvCamAnimPosGoal;
float mfCamAnimPosSpeedMul;
float mfCamAnimPosMaxSpeed;
....
}Interestingly, there are three similar variable blocks with related names: mfRoll, mfLeanRoll, and mvCamAnimPos. In Reset, these three blocks are reset to zero, except for the last two variables from the third block, mfCamAnimPosSpeedMul and mfCamAnimPosMaxSpeed. Just in place of these two variables, duplicated assignments are found. Most likely, all these assignments were copied from the first assignment block, and then the variable names were replaced with the necessary ones.
It may be that the two missing variables should not have been reset, but the opposite is also very likely. In any case, repeated assignments will not be of great help in supporting this code. As you can see, in a long set of identical actions, you may not notice such an error, and the analyzer helps you out here.
Fragment 5.5.
The code is very similar to the previous one. Let me give you a code snippet and a warning from the analyzer for it right away.
V519 The 'mfTimePos' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 49, 53. AnimationState.cpp 53
cAnimationState::cAnimationState(....)
{
....
mfTimePos = 0;
mfWeight = 1;
mfSpeed = 1.0f;
mfBaseSpeed = 1.0f;
mfTimePos = 0;
mfPrevTimePos=0;
....
}The mfTimePos variable was set to 0 twice. As in the previous example, let's get into in the declaration of this field:
class cAnimationState
{
....
private:
....
//Properties of the animation
float mfLength;
float mfWeight;
float mfSpeed;
float mfTimePos;
float mfPrevTimePos;
....
}You may notice that this declarations block also corresponds to the assignment order in the erroneous code snippet, as in the previous example. Here in the assignment, mfTimePos gets the value instead of the mfLength variable. Except for in this case, the error can't be explained by copying the block and the "last line effect". mfLength may not need to be assigned a new value, but this piece of code is still dubious.
Fragment 6.
This part of code from "Amnesia: A Machine For Pigs" sparked off the analyzer to issue a truckload of warnings. I will give only a part of the code that triggered errors of the same kind:
void cLuxEnemyMover::UpdateMoveAnimation(float afTimeStep)
{
....
if(prevMoveState != mMoveState)
{
....
//Backward
if(mMoveState == eLuxEnemyMoveState_Backward)
{
....
}
....
//Walking
else if(mMoveState == eLuxEnemyMoveState_Walking)
{
bool bSync = prevMoveState == eLuxEnemyMoveState_Running
|| eLuxEnemyMoveState_Jogging
? true : false;
....
}
....
}
}Where is the error here?
Here are the analyzer warnings:
The if-else-if sequence in the original code is repeated, and further these warnings were issued for each body of each else if.
Let's consider the line that the analyzer points to:
bool bSync = prevMoveState == eLuxEnemyMoveState_Running
|| eLuxEnemyMoveState_Jogging
? true : false;No surprise, an error crept into such an expression, originally written in line. And I'm sure you've already noticed it. The eLuxEnemyMoveState_Jogging enumeration element is not compared with anything, but its value is checked. Most likely, the expression 'prevMoveState == eLuxEnemyMoveState_Jogging'was meant.
Such a mistake may seem quite harmless. But in another article about checking the Bullet Engine, among the commits to the project, I found a fix for an error of the same kind, which led to the fact that forces were applied to objects from the wrong side. As for this case, this error was made several times. Well, note that the ternary condition is completely meaningless, since it will be applied to the boolean results of logical operators in the last place.
Fragment 7.
Finally, the last couple of examples of copy-paste errors. This time again in a conditional statement. The analyzer issued a warning for this piece of code:
void iParticleEmitter::SetSubDivUV(const cVector2l &avSubDiv)
{
//Check so that there is any subdivision
// and that no sub divison axis is
//equal or below zero
if( (avSubDiv.x > 1 || avSubDiv.x > 1) && (avSubDiv.x >0 && avSubDiv.y >0))
{
....
}
....
}I think that in such a separate fragment from the entire code, it is quite easy to notice an awkward place. Nonetheless, the error found its way to hide from developers of this game.
The analyzer issued the following message:
V501 There are identical sub-expressions to the left and to the right of the '||' operator: avSubDiv.x > 1 || avSubDiv.x > 1 ParticleEmitter.cpp 199
The second parenthesis in the condition indicates that both x and y fields are checked. But in the first parenthesis, for some reason, this point was missed and only the x field is checked. In addition, judging by the review comment, both fields should have been checked. So it is not the "last line effect" that has worked here, but rather the "first line effect", since in the first parenthesis the author forgot to replace accessing the x field with accessing the y field.
Obviously, such errors are very insidious, since in this case even the explanatory comment to the condition did not help the developer.
In such cases, I would recommend making it a habit to record related checks in tabular form. This way, it is easier both to edit, and notice a defect:
if( (avSubDiv.x > 1 || avSubDiv.x > 1)
&& (avSubDiv.x > 0 && avSubDiv.y > 0))Fragment 7.5.
An absolutely similar error was found in a different place:
static bool EdgeTriEqual(const cTriEdge &edge1, const cTriEdge &edge2)
{
if(edge1.tri1 == edge2.tri1 && edge1.tri2 == edge2.tri2)
return true;
if(edge1.tri1 == edge1.tri1 && edge1.tri2 == edge2.tri1)
return true;
return false;
}Did you get a chance to see where it was hiding? It is not for nothing that we've already dealt with so many examples :)
The analyzer has issued a warning:
V501 There are identical sub-expressions to the left and to the right of the '==' operator: edge1.tri1 == edge1.tri1 Math.cpp 2914
We'll sort this fragment through one part after another. Obviously, the first check checks the equality of the fields edge1.tri1 and edge2.tri2, and at the same time the equality of edge1.tri2 and edge2.tri2:
edge1.tri1 -> edge2.tri1
edge1.tri2 -> edge2.tri2In the second check, judging by the correct part of the check 'edge1.tri2 == edge2.tri1', equality of these fields had to be checked in a crosswise way:
But instead of checking for edge1.tri1 == edge2.tri2, there was a pointless check edge1.tri1 == edge1.tri1. By the way, all this is in the function, I removed nothing. Still such an error has wormed into the code.
Fragment 1.
Here is the following code snippet with the original indents.
void iCharacterBody::CheckMoveCollision(....)
{
....
/////////////////////////////////////
//Forward velocity reflection
//Make sure that new velocity points in the right direction
//and that it is not too large!
if(mfMoveSpeed[eCharDir_Forward] != 0)
{
vForwardVel = ....;
float fForwardSpeed = vForwardVel.Length();
if(mfMoveSpeed[eCharDir_Forward] > 0)
if(mfMoveSpeed[eCharDir_Forward] > fForwardSpeed)
mfMoveSpeed[eCharDir_Forward] = fForwardSpeed;
else
if(mfMoveSpeed[eCharDir_Forward] < fForwardSpeed)
mfMoveSpeed[eCharDir_Forward] = -fForwardSpeed;
}
....
}PVS-Studio warning: V563 It is possible that this 'else' branch must apply to the previous 'if' statement. CharacterBody.cpp 1591
This example can be confusing. Why does else have the same indent as the outer one at the if level? Is it implied that else is for the outermost condition? Well, then one has to place braces correctly, otherwise else refers to the right-front if.
if(mfMoveSpeed[eCharDir_Forward] > 0)
{
if(mfMoveSpeed[eCharDir_Forward] > fForwardSpeed)
mfMoveSpeed[eCharDir_Forward] = fForwardSpeed;
}
else if(mfMoveSpeed[eCharDir_Forward] < fForwardSpeed)
{
mfMoveSpeed[eCharDir_Forward] = -fForwardSpeed;
}Or is it not so? When writing this article, I changed my mind several times about which version of the actions sequence for this code was most likely.
If we dig a bit deeper into this code, it turns out that the variable fForwardSpeed, which is compared in the lower if, can't have a value less than zero, since it gets the value from the Length method:
inline T Length() const
{
return sqrt( x * x + y * y + z * z);
}Then, most likely, the point of these checks is that we first check whether the mfMoveSpeed element is greater than zero, and then check its value relative to fForwardSpeed. Moreover, the last two if statements correspond to each other in terms of the wording.
In this case, the original code will work as intended! But it will definitely make the one who comes to edit/refactor it rack their brains.
I thought I'd never come across code that looked like this. Out of interest, I looked at our collection of errors found in open-source projects and described in articles. Examples of this error were also found in other projects - you can look at them yourself.
Please, don't write like this, even if you are clear about it yourself. Use braces, or correct indentation, or better - both. Don't make those who come to understand your code suffer, or yourself in the future ;)
Fragment 2.
This error has taken me aback, so it took a while to find logic here. In the end, it still seems to me that this is most likely a mistake, a quite whopping one.
Take a look at the code:
bool cBinaryBuffer::DecompressAndAdd(char *apSrcData, size_t alSize)
{
....
///////////////////////////
// Init decompression
int ret = inflateInit(&zipStream);
if (ret != Z_OK) return false;
///////////////////////////
// Decompress, chunk by chunk
do
{
//Set current output chunk
zipStream.avail_out = lMaxChunkSize;
....
//Decompress as much as possible to current chunk
int ret = inflate(&zipStream, Z_NO_FLUSH);
if(ret != Z_OK && ret != Z_STREAM_END)
{
inflateEnd(&zipStream);
return false;
}
....
}
while (zipStream.avail_out == 0 && ret != Z_STREAM_END);
....
return true;
}V711 It is dangerous to create a local variable within a loop with a same name as a variable controlling this loop. BinaryBuffer.cpp 371
So, we have a ret variable, which controls the exit from the do-while loop. But inside this loop, instead of assigning a new value to this external variable, a new variable named ret is declared. As a result, it overrides the external ret variable, and the variable that is checked in the loop condition will never change.
With mishap coinciding, such a loop could become infinite. Most likely, in this case, it is an internal condition that saves this code. It checks the value of the internal ret variable and leads to the exit of the function.
Very often, developers do not use static analysis regularly, but with long breaks. Or even run the project through the analyzer only once. As a result of this approach, the analyzer often does not detect anything serious or finds something like the examples we are considering, which may not particularly affect the game's performance. One gets the impression that the analyzer is not really useful. Well, it found such places, but everything still works.
The fact is that there were similar places where a mistake was on the surface, and definitely resulted in a program error. These fragments have already been refined due to long hours of debugging, tests runs, Q&A department. As a result, when the analyzer checks the project only once, it shows only those problems that did not manifest themselves in any way. Sometimes such problems comprise critical issues that actually affected the program but will not likely to follow their scenario. Therefore, this error was unknown to the developers.
That's why it is extremely important to evaluate the usefulness of static analysis only after its regular use. Once a one-time run through PVS-Studio revealed such suspicious and sloppy fragments in the code of this game, imagine how many obvious errors of this kind had to be localized and fixed in course of development.
Use a static analyzer regularly!