Ahead of the release of Amnesia: Rebirth, Fractional Games has released the source code for the legendary Amnesia: The Dark Descent and its sequel, Amnesia: A Machine For Pigs. Why not use a static analysis tool to see how terrible mistakes are hidden in the insides of these cult horror games?
Seeing on Reddit the news that the source code of the games " Amnesia: The Dark Descend " and " Amnesia: A Machine for Pigs " had been published, I could not pass by and check this code using PVS-Studio, and at the same time write about this article. Especially considering the fact that on October 20 (and at the time of this article's publication, it was already out) a new part of this series: " Amnesia: Rebirth ".
Amnesia: The Dark Descent was released in 2010 and has become an iconic survival horror game. Honestly, I have never been able to pass it, even a little bit, since I play horror games according to the same algorithm: bet, enter for five minutes, exit by "alt + f4" at the first creep moment and delete the game.But I liked watching the passage of this game on YouTube.
And in case someone is not familiar with PVS-Studio yet, this is a static analyzer that looks for errors and suspicious places in the source code of programs.
I especially like to look into the source code of games, so if you are wondering what mistakes are made in it, you can read my previous articles. Well, or take a look at the articles of my colleagues about checking the source code of games.
After checking, it turned out that a lot of the code in "The Dark Descend" and "A Machine For Pigs" overlapped, and the reports for the two projects were very similar. So almost all of the errors that I will cite below are contained in both projects.
And the largest layer of errors of all the analyzer detected in these projects was the layer of "copy-paste" errors. Hence the title of the article. The main reason for these errors is the " last line effect ".
Well, let's get down to business.
Copy-paste errors
There were a lot of suspicious places similar to inattentive copying. Some cases, perhaps, are somehow caused by the internal logic of the game itself. But if they embarrassed both me and the analyzer, then it was worth at least providing a comment on them. After all, other developers can be just as slow-witted as I am.
Snippet 1.
Let's start with an example where the whole function consists of comparing the results of methods and fields of two objects aObjectDataA and aObjectDataB . I will give this function entirely for clarity. Try to notice 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();
}A picture, so as not to accidentally spy on the answer:
Could you find the error? Yes, the last return is a comparison using aObjectDataA on both sides. Note that all the expressions in the original code were written out in a line, here I have hyphenated so that everything fits exactly into the line width. Imagine what such a defect will look for at the end of the working day. And the analyzer will find it immediately after assembling and running the 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 moment of writing the code, instead of hiding in the depths of the code from several QA stages, making your search many times more difficult.
Note by colleague Andrey Karpov. Yes, this is a classic "last line error". However, this is also a classic error pattern when comparing two objects. See the article " Evil Lives in Comparison Functions ".Fragment 2.
Let's take a look at the code that caused the warning:
I present the code with a screenshot for clarity.
The warning itself looks like this:
V501 There are identical sub-expressions 'lType == eLuxJournalState_OpenNote' to the left and to the right of the '||' operator. LuxJournal.cpp 2262
The analyzer has detected that there is an error in checking the value of the lType variable . Equality is checked twice with the same member of the eLuxJournalState_OpenNote enumerator .
First, I would like such a condition to be written in a "tabular" form to improve readability. See chapter N13 in the mini-book The Biggest Question of Programming, Refactoring, and All That .
if(!( lType == eLuxJournalState_OpenNote
|| lType == eLuxJournalState_OpenDiary
|| lType == eLuxJournalState_OpenNote
|| lType == eLuxJournalState_OpenNarratedDiary))
return false;In this form, it becomes much easier to spot an error even without an analyzer.
However, the question arises, does such an erroneous check lead to distortion of the program logic? After all, it is possible that some other lType value should be checked , but the check was missed due to a copy-paste error. 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 meanings that have the word "Open" in their name. And all three are present in the check. Most likely, there is no distortion of logic here, but it is still impossible to say for sure. So the analyzer either found a logical error that the game developer could fix, or found an "ugly" written place that should have been rewritten for better clarity.
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 cycle, the work goes with the pEntity pointer , which was received via mvAttackerIDs, and if the condition is not met, a message for debugging is issued for the same mvAttackerIDs . However, in the next loop, which is styled in exactly the same way as the previous section of code, pEntity is obtained using mvSearcherIDs . But the warning is still issued with the mention of mvAttackerIDs .
Most likely, the "Searchers" signed code block 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 put a serious pig 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 following suspicious place with three warnings:
- V547 Expression 'pEntity == 0' is always false. LuxScriptHandler.cpp 2444
- V649 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains function return. This means that the second 'if' statement is senseless. Check lines: 2433, 2444. LuxScriptHandler.cpp 2444
- V1051 Consider checking for misprints. It's possible that the 'pTargetEntity' should be checked here. LuxScriptHandler.cpp 2444
Let's 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;
....
}A V547 warning was issued for the second check pEntity == NULL . For the analyzer, this check will always be false , because if this condition were true , then the function would exit higher due to the previous similar check.
The next warning, V649 was issued precisely because we have two identical checks. Usually this case may not be an error. Suddenly, one part of the code implements one logic, and in another part of the code, according to the same check, something else must be implemented. But in this case, the body of the first check consists of return, so the second check, if the condition turns out to be true, will not even reach the point. By tracing such 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 inherently very similar to the previous example. Most likely, all checks were duplicated from the first check if (pEntity == NULL) , and then the checked object was replaced with the required one. In the case of pBody and pTargetBody objects, the replacement was made, but the pTargetEntity object was forgotten. As a result, this object is not checked.
In the example we are considering, if you dig a little deeper into the code, it turns out that such an error in general will not affect the course of the program. The pTargetBody pointer gets its value from the GetBodyInEntity function :
iPhysicsBody *pTargetBody = GetBodyInEntity(pTargetEntity,
asTargetBodyName);The first argument here is just a previously unchecked pointer, which is not used anywhere else. And 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.
And one more suspicious place with copy-paste!
In this method, the fields of the object of the cLuxPlayer class are set to zero .
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 nullified twice:
- V519 The 'mfRollSpeedMul' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 298, 308. LuxPlayer.cpp 308
- V519 The 'mfRollMaxSpeed' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 299, 309. LuxPlayer.cpp 309
Let's look into the class itself and see what fields it contains:
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 blocks of variables with related names: mfRoll , mfLeanRoll and mvCamAnimPos . In Reset, these three blocks are cleared, except for the last two variables from the third block, mfCamAnimPosSpeedMul and mfCamAnimPosMaxSpeed . And it is in the place of these two variables that 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 required ones.
It may be that two missing variables should not have been set to zero, but the probability of the opposite is also very high. And re-assignments will obviously not help in maintaining this code. As you can see, in a long footcloth of the same actions, you may not notice such an error, and the analyzer helps out here.
Fragment 5.5.
This example is very similar to the previous one. I will give you a code snippet and an analyzer warning for it.
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 has been assigned the value 0 twice. As in the previous example, let's look at the declaration of this field:
class cAnimationState
{
....
private:
....
//Properties of the animation
float mfLength;
float mfWeight;
float mfSpeed;
float mfTimePos;
float mfPrevTimePos;
....
}You can see that this declaration block also matches the assignment order in the error code snippet, as in the previous example. Here, in the assignment, instead of the mfLength variable , the value gets mfTimePos . But here the error cannot be explained by copying the block and the "effect of the last line". It may be that mfLength does not need to be assigned a new value, but this location is still suspicious.
Fragment 6.
The analyzer issued a whole bunch of warnings for the next code fragment from "Amnesia: A Machine For Pigs". I will give only a part of the code for which errors of the same kind were issued:
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 mistake here?
The analyzer issued the following warnings:
- V768 The enumeration constant 'eLuxEnemyMoveState_Jogging' is used as a variable of a Boolean-type. LuxEnemyMover.cpp 672
- V768 The enumeration constant 'eLuxEnemyMoveState_Walking' is used as a variable of a Boolean-type. LuxEnemyMover.cpp 680
- V768 The enumeration constant 'eLuxEnemyMoveState_Jogging' is used as a variable of a Boolean-type. LuxEnemyMover.cpp 688
The if-else-if chain is repeated in the original code, and then these warnings were just issued for each body of each else if .
Consider the line pointed to by the parser:
bool bSync = prevMoveState == eLuxEnemyMoveState_Running
|| eLuxEnemyMoveState_Jogging
? true : false;It is not surprising that an error has crept into such an expression, which is also written in a line in the original. And you probably already noticed it. The member of the eLuxEnemyMoveState_Jogging enumeration is not compared with anything; its value is checked. Most likely, the expression 'prevMoveState == eLuxEnemyMoveState_Jogging' was implied.
Such a mistake may seem completely harmless. But in my other article, about checking the Bullet Engine , among the commits to the project, I found a bug fixof the same kind, resulting in forces being applied to objects from the wrong side. And in this case, this mistake was made several times. Well, I also note that the ternary condition is completely meaningless, since it will be fulfilled, in the last place, to the Boolean results of logical operators.
Fragment 7.
And, finally, the last couple of examples of copy-paste errors. This time again in a conditional statement. The analyzer issued a warning for the following 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 it is quite easy to notice a suspicious place in such a fragment, separate from the whole code. However, this error managed to hide from the developers of this game.
The analyzer issued the following warning:
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 shows that both x and y fields are checked . But in the first parenthesis, for some reason this moment was missed and only the field x is checked... Plus, judging by the validation comment, both fields should have been validated. Here somehow it was not the "effect of the last line" that worked, but rather the first, because in the first parenthesis they forgot to replace the call to the x field with the call to the y field .
So such errors are very tricky, since in this case the developer was not even helped by writing an explanatory comment to the condition.
I would recommend in such cases to take it as a habit to record related checks in a tabular form. It's easier to edit this way, and the flaw will be much more noticeable:
if( (avSubDiv.x > 1 || avSubDiv.x > 1)
&& (avSubDiv.x > 0 && avSubDiv.y > 0))Fragment 7.5.
Absolutely the same, in fact, the error was found in a completely 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;
}Well, how did you manage to immediately see where she hid? It is not for nothing that so many examples have already been sorted out :)
The analyzer issued the following warning:
V501 There are identical sub-expressions to the left and to the right of the '==' operator: edge1.tri1 == edge1.tri1 Math.cpp 2914
Let's analyze this fragment in order. 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.tri2And in the second check, judging by the correct part of the check 'edge1.tri2 == edge2.tri1', it was necessary to check the equality of these fields "crosswise":
But instead of checking for edge1.tri1 == edge2.tri2 , a meaningless check was performed edge1.tri1 == edge1.tri1 . But this is all the content of the function, I did not delete anything from it. And all the same, such an error got into the code.
Other errors
Fragment 1.
I will give the following code fragment with 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 else is indented the same as the outermost if ? Is it the else for the outer condition? Well then, you need to place the parentheses, otherwise else refers to the directly preceding 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 not? In the process of writing this article, I changed my opinion several times about which variant of the sequence of the conceived actions for this code is most likely.
If we delve a little deeper into this code, it turns out that the fForwardSpeed variable , with which the comparison is performed in the lower if , cannot have a value less than zero, since it receives a value from the Length method :
inline T Length() const
{
return sqrt( x * x + y * y + z * z);
}Then, most likely, the essence of these checks is that first we check if the element mfMoveSpeed is greater than zero, and then we check its value relative to fForwardSpeed . Moreover, the last two ifs correspond to each other in their wording.
In this case, the original code will work as intended! But it will definitely make the person who comes to edit / refactor it to puzzle.
I thought that I would never come across code that looks like this. Out of interest, I looked into our database of errors found in open-source projects and described in articles. And examples of this error were found in other projects - you can look at them yourself .
And please do not write like that, even if you yourself are clear in this case. Or curly braces or correct indentation, or better, both. Do not plunge into suffering those who come to understand your code, and themselves in the future;)
Fragment 2.
The next error confused me a little, and I tried to find the logic here for a long time. But in the end, it still seems to me that this is most likely a mistake, and a rather gross one.
Let's 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 variable ret , 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 variable ret, and the variable that is checked in the loop condition will never change.
In an unfortunate combination of circumstances, such a cycle could become infinite. Most likely, in this case, this code saves an internal condition that checks the value of the internal variable ret and leads to exit from the function.
Conclusion
Very often, developers use static analysis not regularly, but with long breaks. Or they even run the project through the analyzer 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, perhaps, did not particularly affect the functionality of the game. One gets the impression that the analyzer is not particularly useful. Well, he found such places, but still works.
But the fact is that similar places, but in which the error was not masked, but clearly led to a bug in the program, have already been corrected by long hours of debugging, runs through tests, and the testing department. As a result, when checking a project, the analyzer shows only those problems that have not manifested themselves in any way. Sometimes among such problems there are also serious moments that really influenced the operation of the program, but the probability that the program would follow their path was low, and therefore this error was unknown to the developers.
Therefore, it is extremely important to evaluate the usefulness of static analysis only after its regular use. If a one-time run through PVS-Studio revealed such suspicious and inaccurate places in the code of this game, then how many obvious errors of this kind had to be localized and corrected during the development process.
Use a static analyzer regularly!
If you want to share this article with an English-speaking audience, please use the translation link: Victoria Khanieva. Amnesia: The Dark Descent or How to Forget to Fix Copy Paste .