{"id":387467,"date":"2024-06-29T07:19:51","date_gmt":"2024-06-29T07:19:51","guid":{"rendered":"http:\/\/savepearlharbor.com\/?p=387467"},"modified":"-0001-11-30T00:00:00","modified_gmt":"2024-06-29T07:19:51","slug":"","status":"publish","type":"post","link":"https:\/\/savepearlharbor.com\/?p=387467","title":{"rendered":"Amnesia: The Dark Descent or How to Forget to Fix Copy Paste<\/span>"},"content":{"rendered":"
<\/div>\n
\n
\n
\n
\n
\"image1.png\"<\/div>\n

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?
<\/a>
After seeing the news on Reddit that the source code of the games «Amnesia: The Dark Descent<\/a>» and «Amnesia: A Machine for Pigs<\/a>» 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 \u2014 «Amnesia: Rebirth<\/a>» is released on the 20th of October (and at the moment of posting this article the game is already released).<\/p>\n

«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.<\/p>\n

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.<\/p>\n

<\/p>\n

<\/div>\n

<\/a>
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<\/a> about checking source code of games.<\/p>\n

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.<\/p>\n

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<\/a>«.<\/p>\n

Let’s get right to it.<\/p>\n

Copy-paste errors<\/h2>\n

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.<\/p>\n

Fragment 1.<\/b><\/p>\n

Let’s start with an example where the entire function consists of comparing method results and fields values of two objects aObjectDataA<\/i> and aObjectDataB<\/i>. I will cite the entire function for clarity. Try to see for yourself where the error was made in the function:<\/p>\n

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(); }<\/code><\/pre>\n
  Here’s a pic for you to avoid accidentally spying on the answer:<\/p>\n
\"image2.png\"<\/div>\n
  Did you find a bug? So, in the last return<\/i>, there is a comparison using aObjectDataA<\/i> 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.<\/p>\n
  V501<\/a> There are identical sub-expressions ‘aObjectDataA.mpObject->GetVertexBuffer()’ to the left and to the right of the ‘<‘ operator. WorldLoaderHplMap.cpp 1123<\/p>\n
  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.<\/p>\n
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<\/a>«.<\/p><\/blockquote>\n
Fragment 2.<\/b><\/p>\n
  Let’s take a quick look at the code that triggered the warning:<\/p>\n
\"image4.png\"<\/div>\n
  Here is a screenshot of the code for clarity.<\/p>\n
  This is how the warning looks like:<\/p>\n
  V501<\/a> There are identical sub-expressions ‘lType == eLuxJournalState_OpenNote’ to the left and to the right of the ‘||’ operator. LuxJournal.cpp 2262<\/p>\n
  The analyzer found that there is an error in the check of the lType<\/i> variable value. The equality with the same element of the eLuxJournalState_OpenNote<\/i> enumerator is checked twice.<\/p>\n
  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<\/a>» for more details. <\/p>\n
if(!(   lType == eLuxJournalState_OpenNote      || lType == eLuxJournalState_OpenDiary      || lType == eLuxJournalState_OpenNote      || lType == eLuxJournalState_OpenNarratedDiary))   return false;<\/code><\/pre>\n
  In this form, it becomes much easier to notice the error even without the analyzer.<\/p>\n
  Anyway, here comes a question \u2014 does such an erroneous check lead to logic distortion of the program? After all, one may need to check some other lType<\/i> value, but the check was missed due to a copy-paste error. So, let’s take a look at the enumeration itself:<\/p>\n
enum eLuxJournalState {   eLuxJournalState_Main,   eLuxJournalState_Notes,   eLuxJournalState_Diaries,   eLuxJournalState_QuestLog,   eLuxJournalState_OpenNote,   eLuxJournalState_OpenDiary,   eLuxJournalState_OpenNarratedDiary,    eLuxJournalState_LastEnum, };<\/code><\/pre>\n
  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.<\/p>\n
  Fragment 3.<\/b><\/p>\n
  The following case is generally the most obvious example of a copy-paste error.<\/p>\n
  V778<\/a> Two similar code fragments were found. Perhaps, this is a typo and ‘mvSearcherIDs’ variable should be used instead of ‘mvAttackerIDs’. LuxSavedGameTypes.cpp 615<\/p>\n
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]);     }   } }<\/code><\/pre>\n
  In the first loop, the pEntity<\/i> pointer (obtained via mvAttackerIDs<\/i>) is handled. If the condition is not met, a debug message is issued for the same mvAttackerIDs<\/i>. However, in the next loop, which is formed the same as the previous code section, pEntity<\/i> is obtained using mvSearcherIDs<\/i>. While the warning is still issued with the mention of mvAttackerIDs<\/i>.<\/p>\n
  Most likely, the code block with the note «Searchers» was copied from the «Attackers» block, mvAttackerIDs<\/i> was replaced with mvSearcherIDs<\/i>, but the else<\/i> block was not changed. As a result, the error message uses an element of the wrong array.<\/p>\n
  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<\/i> element.<\/p>\n
\"image5.png\"<\/div>\n
  Fragment 4.<\/b><\/p>\n
  The analyzer indicated the next suspicious fragment with as many as three warnings:<\/p>\n