Clever Geek Handbook

失忆症：黑暗后裔或如何忘记修复复制粘贴

image1.png


在发布《失忆症：重生》之前，Fractional Games已发布了传奇性的《失忆症：黑暗血统》及其续集《失忆症：猪用机器》的源代码。为什么不使用静态分析工具来查看这些邪教恐怖游戏内部隐藏的可怕错误呢？



在Reddit上看到消息，“失忆症：黑暗的降落”和“失忆症：猪的机器”的源代码已经发布，我无法通过PVS-Studio并检查此代码，同时编写关于这篇文章。特别是考虑到10月20日（在本文发布时，它已经发布）了该系列的新内容：“失忆症：重生”。



《失忆症：黑暗后裔》于2010年发布，现已成为备受追捧的恐怖生存游戏。老实说，我从来没有能够通过它，甚至一点点也没有，因为我是按照相同的算法玩恐怖游戏的：下注，输入五分钟，在第一个爬行时刻退出“ alt + f4”，然后删除游戏。但我喜欢在YouTube上观看此游戏的转播。



如果还不熟悉PVS-Studio，这是一个静态分析器，它可以在程序的源代码中查找错误和可疑的位置。





我特别喜欢研究游戏的源代码，因此，如果您想知道游戏中有什么错误，可以阅读我以前的文章。好吧，或者看看我同事关于检查游戏源代码的文章。



经过检查，结果发现“黑暗的降临”和“猪的机器”中的许多代码重叠，并且两个项目的报告非常相似。因此，我将在下面引用的几乎所有错误都包含在两个项目中。



在这些项目中检测到的所有分析仪中，最大的错误层是“复制粘贴”错误层。因此，文章标题。这些错误的主要原因是“最后一行效果”。



好吧，让我们开始做生意。



复制粘贴错误



有很多可疑的地方，与不专心的复制类似。某些情况也许是由游戏本身的内部逻辑引起的。但是，如果他们让我和分析员都感到尴尬，那么至少值得对他们发表评论。毕竟，其他开发人员可能和我一样机灵。



片段1.



让我们从一个示例开始，其中整个功能包括比较两个对象aObjectDataA和aObjectDataB的方法和字段的结果。为了清楚起见，我将完全给出此功能。尝试亲自了解函数中的错误位置： 



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();
}


图片，以免偶然发现答案：



image2.png


您能找到错误吗？是的，最后一次返回是在两侧使用aObjectDataA进行的比较。请注意，原始代码中的所有表达式都写在一行中，在这里我加了连字符，以便所有内容都完全适合行宽。想象一下在工作日结束时会发现什么缺陷。在组装并运行增量分析后，分析仪将立即找到它。



V501在'<'运算符的左侧和右侧有相同的子表达式'aObjectDataA.mpObject-> GetVertexBuffer（）'。 WorldLoaderHplMap.cpp 1123



结果，几乎在编写代码时就会发现这样的错误，而不是隐藏在多个QA阶段的代码深处，这使搜索难度增加了很多倍。
同事安德烈·卡波夫（Andrey Karpov）的笔记。是的，这是典型的“最后一行错误”。但是，这也是比较两个对象时的经典错误模式。请参阅文章“比较函数中的邪恶行为”。
片段2。



让我们看一下引起警告的代码：



image4.png


为了清晰起见，我提供了带有屏幕截图的代码。



警告本身看起来像这样：



V501 '||'的左侧和右侧有相同的子表达式'lType == eLuxJournalState_OpenNote'。操作员。 LuxJournal.cpp 2262



分析器检测到检查lType变量的值时出错。使用eLuxJournalState_OpenNote枚举器的相同成员两次检查是否相等。



首先，我希望以“表格”形式编写这样的条件以提高可读性。请参阅迷你书《编程，重构及所有方面的最大问题》中的N13章。 



if(!(   lType == eLuxJournalState_OpenNote
     || lType == eLuxJournalState_OpenDiary
     || lType == eLuxJournalState_OpenNote
     || lType == eLuxJournalState_OpenNarratedDiary))
  return false;


通过这种形式，即使没有分析仪，也更容易发现错误。



但是，问题来了，这样的错误检查会导致程序逻辑失真吗？毕竟，也许您需要检查其他一些lType值，但是由于复制粘贴错误而错过了该检查。让我们看一下枚举本身： 



enum eLuxJournalState
{
  eLuxJournalState_Main,
  eLuxJournalState_Notes,
  eLuxJournalState_Diaries,
  eLuxJournalState_QuestLog,
  eLuxJournalState_OpenNote,
  eLuxJournalState_OpenDiary,
  eLuxJournalState_OpenNarratedDiary,

  eLuxJournalState_LastEnum,
};


名称中只有三个含义，即单词“ Open”。所有这三个都在支票中。最有可能的是，这里没有逻辑失真，但是仍然不能肯定地说。因此，分析器要么找到了游戏开发人员可以修复的逻辑错误，要么找到了一个“丑陋”的书写位置，为了更好的清晰度，应将其重写。



片段3。



以下情况通常是最明显的复制粘贴错误示例。



V778找到了两个类似的代码片段。也许这是一个错字，应该使用“ mvSearcherIDs”变量而不是“ 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]);
    }
  }
}


在第一个循环中，工作通过pEntity指针进行，该指针是通过mvAttackerIDs接收的，如果不满足条件，则会针对相同的mvAttackerIDs发出调试消息。但是，在下一个循环中（其样式与上一段代码完全相同），使用mvSearcherIDs获得pEntity。但是该警告仍然会提及mvAttackerID。



最有可能的是，从“攻击者”块复制了“搜索者”签名的代码块，将mvAttackerID替换为mvSearcherID，但是else块未更改。结果，错误消息使用了错误数组的元素。



该错误不会影响游戏的逻辑，但是通过这种方式，您可以对必须调试该位置并浪费时间使用错误的mvSearcherIDs元素的人施加沉重的负担。



image5.png


片段4。



分析仪通过以下三个警告指示了以下可疑位置：



  • V547表达式'pEntity == 0'始终为false。LuxScriptHandler.cpp 2444
  • V649有两个带有相同条件表达式的'if'语句。第一个'if'语句包含函数return。这意味着第二个“ if”语句是毫无意义的。检查行：2433、2444。LuxScriptHandler.cpp 2444
  • V1051考虑检查是否有误印。可能应在此处检查“ pTargetEntity”。LuxScriptHandler.cpp 2444


让我们看一下代码： 



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;

  ....
}


为第二次检查pEntity == NULL发出了V547 警告。对于分析器，此检查将始终为false，因为如果此条件为true，则由于先前的类似检查，该函数将退出更高的位置。发出 下一个警告V649正是因为我们有两次相同的检查。通常，这种情况可能不是错误。突然，在代码的一部分中，实现了一种逻辑，而在代码的另一部分中，根据相同的检查，还必须实现其他内容。但是在这种情况下，第一张支票的主体由return组成



，因此，如果条件为真，则第二次检查甚至不会达到该点。通过跟踪此类逻辑，分析器减少了可疑代码错误消息的数量，仅针对非常奇怪的逻辑输出错误消息。



最后一个警告指示的错误本质上与前面的示例非常相似。最有可能的是，所有检查都从第一次检查if（pEntity == NULL）开始重复，然后将检查的对象替换为所需的对象。对于pBody和pTargetBody对象，进行了替换，但忘记了pTargetEntity对象。结果，不检查该对象。



在我们正在考虑的示例中，如果您对代码进行更深入的研究，事实证明，这种错误通常不会影响程序的过程。pTargetBody指针从GetBodyInEntity函数获取其值： 



iPhysicsBody *pTargetBody = GetBodyInEntity(pTargetEntity,
                                            asTargetBodyName);


这里的第一个参数只是一个以前未经检查的指针，在其他任何地方都没有使用。幸运的是，在此函数内部，检查了第一个参数是否为NULL： 



iPhysicsBody* ....::GetBodyInEntity(iLuxEntity* apEntity, ....)
{
  if(apEntity == NULL){
    return NULL;
  }
  ....
}


结果，尽管该代码包含错误，但最终还是可以正常工作。



片段5。



还有一个带有复制粘贴的可疑位置！



image6.png


此方法清除cLuxPlayer类的对象的字段。 



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; //<-
  ....
}


但是由于某些原因，两个变量mfRollSpeedMul和mfRollMaxSpeed两次无效：



  • V519'mfRollSpeedMul '变量被连续分配两次值。也许这是一个错误。检查行：298，308。LuxPlayer.cpp 308
  • V519为'mfRollMaxSpeed'变量连续分配了两次值。也许这是一个错误。检查行：299、309。LuxPlayer.cpp 309


让我们看一下类本身，看看它包含哪些字段： 



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;
  ....
}


有趣的是，存在三个具有相似名称的相似变量块：mfRoll，mfLeanRoll和mvCamAnimPos。在Reset中，这三个块被清除，除了第三个块中的最后两个变量mfCamAnimPosSpeedMul和mfCamAnimPosMaxSpeed。并且找到这两个变量的地方就是重复的分配。最有可能的是，所有这些分配都是从第一个分配块中复制的，然后用所需的变量名替换了变量名。



可能两个缺失变量不应设置为零，但是相反的可能性也很高。重新分配显然将无助于维护此代码。如您所见，在执行相同操作时，您可能不会注意到这样的错误，分析仪会在此处提供帮助。



片段5.5。



此示例与上一个示例非常相似。我将为您提供一个代码段和一个分析器警告。



V519为'mfTimePos'变量连续分配了两次值。也许这是一个错误。检查行：49、53。AnimationState.cpp 53 



cAnimationState::cAnimationState(....)
{
  ....
  mfTimePos = 0;
  mfWeight = 1;
  mfSpeed = 1.0f;
  mfBaseSpeed = 1.0f;
  mfTimePos = 0;
  mfPrevTimePos=0;
  ....
}


已为mfTimePos变量分配了两次值0，如上例所示，让我们看一下该字段的声明： 



class cAnimationState
{
  ....
private:
  ....
  //Properties of the animation
  float mfLength;
  float mfWeight;
  float mfSpeed;
  float mfTimePos;
  float mfPrevTimePos;
  ....
}


您可以看到该声明块也与错误代码段中的分配顺序匹配，如上例所示。在此，在赋值中，该值代替mfLength变量，而得到mfTimePos。但是，此处无法通过复制块和“最后一行效果”来解释错误。可能不需要为mfLength分配新值，但是此位置仍然可疑。



片段6.



分析器针对“失忆症：猪用机器”中的下一个代码片段发出了很多警告。我将只给出发出相同类型错误的部分代码： 



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;
      ....
    }
    ....
  }
}


这里的错误在哪里？



分析仪发出以下警告：



  • V768枚举常量'eLuxEnemyMoveState_Jogging'用作布尔型变量。LuxEnemyMover.cpp 672
  • V768枚举常量'eLuxEnemyMoveState_Walking'用作布尔型变量。LuxEnemyMover.cpp 680
  • V768枚举常量'eLuxEnemyMoveState_Jogging'用作布尔型变量。LuxEnemyMover.cpp 688


在原始代码中重复了if-else-if链，然后仅针对彼此的if的每个主体发出这些警告。



考虑解析器指向的行： 



bool bSync =    prevMoveState == eLuxEnemyMoveState_Running
             || eLuxEnemyMoveState_Jogging
              ? true : false;


毫不奇怪的是，错误已经蔓延到这样的表达式中，该表达式也以原始形式写在一行中。您可能已经注意到了。eLuxEnemyMoveState_Jogging枚举的成员未与任何内容进行比较；将检查其值。最有可能暗含了表达式“ prevMoveState == eLuxEnemyMoveState_Jogging”。



这样的错误似乎完全无害。但是在另一篇关于检查Bullet Engine的文章中，我找到了一个错误修复程序相同的类型，导致从错误的方向将力施加到对象上。在这种情况下，这个错误发生了几次。好吧，我也注意到，三元条件是完全没有意义的，因为最后它会满足逻辑运算符的布尔结果。



片段7。



最后是复制粘贴错误的最后两个示例。这次再次在条件语句中。分析器针对以下代码发出警告： 



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))
  {
    ....
  }
  ....
}


我认为在这样一个片段中将可疑位置与整个代码分开很容易。但是，此错误设法对游戏开发人员隐藏了。



分析器发出以下警告：



V501 '||'的左侧和右侧都有相同的子表达式。运算符：avSubDiv.x> 1 || avSubDiv.x> 1 ParticleEmitter.cpp 199



条件中的第二个括号显示同时检查了x和y字段。但是在第一个括号中，由于某种原因，错过了这一刻，只检查了字段x... 另外，根据验证注释判断，两个字段都应该已经验证。在某种程度上，起作用的不是“最后一行的效果”，而是起作用的，因为在第一个括号中，他们忘记了将对x字段的调用替换为对y字段的调用。



因此，这样的错误是非常隐蔽的，因为在这种情况下，开发人员甚至都没有通过写条件注释来帮助开发人员。



我建议在这种情况下，以习惯的方式以表格形式记录相关支票。通过这种方式进行编辑更容易，并且缺陷将更加明显： 



if(   (avSubDiv.x > 1 || avSubDiv.x > 1)
   && (avSubDiv.x > 0 && avSubDiv.y > 0))


片段7.5。



实际上，在完全不同的位置发现了错误，这完全相同： 



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;
}


好吧，您如何设法立即看到她躲在哪里？并非毫无疑问，已经整理出了很多示例：)



分析器发出以下警告：



V501 '=='运算符左右两侧有相同的子表达式：edge1.tri1 == edge1.tri1 Math.cpp 2914



让我们分析一下片段顺序。显然，先检查检查领域的平等edge1.tri1和edge2.tri2，并在同一时间平等edge1.tri2和edge2.tri2： 



edge1.tri1 -> edge2.tri1
edge1.tri2 -> edge2.tri2


在第二个检查中，通过'edge1.tri2 == edge2.tri1'检查的正确部分来判断，有必要“横向”检查这些字段的相等性：



image7.png


但是，不是检查edge1.tri1 == edge2.tri2，而是执行了没有意义的检查edge1.tri1 == edge1.tri1。但这是功能的全部内容，我没有从中删除任何内容。同样，这样的错误进入了代码。



其他错误



片段1.



我将使用原始缩进给出以下代码片段。 



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警告：V563此“ else”分支可能必须应用于先前的“ if”语句。 CharacterBody.cpp 1591



此示例可能会令人困惑。为什么别的缩进一样的最外层，如果？外部条件是否其他？那么，您需要放置括号，否则else指的是紧接在前面的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;
}


或不？在撰写本文的过程中，我多次就此代码中最有可能构想的动作序列的哪个变体改变了看法。



如果我们对代码进行更深入的研究，结果发现fForwardSpeed变量（与if进行比较）不能小于零，因为它从Length方法接收一个值： 



inline T Length() const
{
  return sqrt( x * x + y * y +  z * z);
}


然后，最有可能的是，这些检查的本质是：首先检查元素mfMoveSpeed是否大于零，然后检查相对于fForwardSpeed的值。此外，最后两个if的措词彼此对应。



在这种情况下，原始代码将按预期工作！但这肯定会让来编辑/重构它的人感到困惑。



我以为我永远也不会碰到像这样的代码。出于兴趣，我调查了开源项目中发现的错误数据库，并在文章中进行了描述。在其他项目中也发现了此错误的示例-您可以自己查看它们。



而且，即使您自己很清楚，也请不要这样写。或花括号或正确的压痕，或两者都更好。不要陷入使那些开始了解您的代码以及将来会自己的人的痛苦；）



片段2。



下一个错误使我有些困惑，我试图在这里找到很长时间的逻辑。但是最后，在我看来，这仍然很可能是一个错误，而且是一个严重错误。



让我们看一下代码： 



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在循环中创建与控制该循环的变量同名的局部变量是很危险的。BinaryBuffer.cpp 371



因此，我们有一个变量ret，它控制do-while循环的退出。但是在此循环中，没有声明给该外部变量分配新值，而是声明了一个名为ret的新变量。结果，它将覆盖外部变量ret，并且在循环条件下检查的变量将永远不会更改。



在不幸的情况下，这种循环可能会变得无止境。在这种情况下，此代码最有可能保存一个内部条件，以检查内部变量ret的值。 并导致退出该功能。



image8.png


结论



开发人员通常不定期使用静态分析，但会花费很长时间。或者他们甚至通过分析器运行项目一次。这种方法的结果是，分析器通常不会检测到任何严重的问题，也不会发现我们正在考虑的示例之类的东西，这也许并没有特别影响游戏的功能。人们会觉得分析仪不是特别有用。好吧，他找到了这些地方，但仍然可以工作。



事实是，经过长时间的调试，运行测试以及测试部门，已经纠正了类似的地方，但是并未掩盖错误，但显然导致了程序中的错误。结果，在检查项目时，分析器仅显示那些尚未以任何方式表现出来的问题。有时，在这些问题中，还有一些确实影响程序运行的严重时刻，但是程序遵循其路径的可能性很小，因此开发人员不知道此错误。



因此，仅在常规使用后评估静态分析的有效性非常重要。如果一次通过PVS-Studio运行时在该游戏的代码中发现了如此可疑和不正确的位置，那么在开发过程中必须定位并纠正这种明显的错误数量。



定期使用静态分析仪！





如果您想与讲英语的读者分享这篇文章，请使用翻译链接：Victoria Khanieva。失忆症：黑暗后裔或如何忘记修复复制粘贴。


More articles:


All Articles