Before you go, check out these stories!

0
Hackernoon logoFixing Code Errors in Amnesia: The Dark Descent by@pvs-studio

Fixing Code Errors in Amnesia: The Dark Descent

Author profile picture

@pvs-studioPVS-Studio

Search for bugs in C, C++, C#, Java on Windows, Linux, macOS. https://www.viva64.com/en/pvs-studio/

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.

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.

Copy-paste errors

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:

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

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:

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

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:

  • 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

Take a look at the code:

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:

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:

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.

But for some reason, the two variablesĀ mfRollSpeedMulĀ and mfRollMaxSpeedĀ are zeroed 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 at the class itself and at its fields:

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

TheĀ mfTimePosĀ variable was set to 0 twice. As in the previous example, let's get into in the declaration of this field:

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:

Where is the error here?

Here are the analyzer 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 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:

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:

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 the 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:

Fragment 7.5.

An absolutely similar error was found in a different place:

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:

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

Other errors

Fragment 1.

Here is the following code snippet with the original indents.

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.

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:

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:

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.

Conclusion

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, test 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!

Previously published at https://www.viva64.com/en/b/0769/

Author profile picture

@pvs-studioPVS-Studio

Read my stories

Search for bugs in C, C++, C#, Java on Windows, Linux, macOS. https://www.viva64.com/en/pvs-studio/

Tags

Join Hacker Noon

Create your free account to unlock your custom reading experience.