VVVVVV ??? VVVVVV !!! :)

如果阅读此文本，则意味着您或者认为文章标题有误，或者您在其中看到了熟悉的计算机游戏的名称。 VVVVVV是一款独立的平台游戏，以其令人愉悦的外部简洁性和同样令人愉悦的内部复杂性赢得了许多玩家的青睐。 几天前，VVVVVV已经10岁了，该游戏的作者Terry Cavanagh通过发布源代码庆祝了这个假期。 里面有什么“鲜美”？ 阅读本文的答案。

引言

哦，VVVVVV ...我记得发行后不久就遇到了它，作为像素复古游戏的忠实拥护者，我很高兴将其安装在计算机上。 我记得我的第一印象：“那么，这就是全部吗？ 只是在广场房间周围跑？”游戏几分钟后，我想。 我还不知道我在等什么。 一离开出发地点，我便发现自己身处一个小而又混乱又华丽的二维世界，里面充满了我所不知道的异常景观和像素伪像。

这场比赛把我拖了下去。 尽管具有很高的复杂性，并且在那个时候被异常控制巧妙地击败了（主要角色不知道如何跳跃，但是能够为我自己反转重力矢量的方向），我还是完全通过了它。 我不知道我的角色到那时死了多少次，但是我敢肯定死亡人数以几百计。 尽管如此，这个游戏还是有一些独特的魅力：）

让我们回到为纪念游戏周年而布置的源代码。

目前，我是C ++-PVS-Studio团队的开发人员-静态代码分析器，用于C，C ++，C＃和Java。 除了开发本身，我们还从事产品的推广。 对我们而言，最好的方法之一就是撰写有关检查开源项目的文章。 我们的读者会收到有关编程主题的有趣文章，我们有机会清楚地展示PVS-Studio的功能。 因此，当我了解源代码VVVVVV的打开时，我只是无法通过。

在本文中，我们将研究PVS-Studio分析仪在VVVVVV代码中发现的一些有趣的错误，并对这些错误进行详细分析。 将重力矢量返回到“向下”位置并坐下：我们开始了！

分析仪发出的警报概述

警告1

V512调用'sprintf'函数将导致缓冲区'fileSearch'溢出。 FileSystemUtils.cpp 307

#define MAX_PATH 260 .... void PLATFORM_migrateSaveData(char *output) { char oldLocation[MAX_PATH]; char newLocation[MAX_PATH]; char oldDirectory[MAX_PATH]; char fileSearch[MAX_PATH]; .... /* Same place, different layout. */ strcpy(oldDirectory, output); sprintf(fileSearch, "%s\\*.vvvvvv", oldDirectory); .... } 

如您所见， fileSearch和oldDirectory行的大小相同：260个字符。 将字符串oldDirectory的内容代入后，格式字符串（ sprintf的第三个参数）将如下所示：

 <i>_oldDirectory\*.vvvvvv</i> 

该字符串比原始oldDirectory长9个字符。 正是这个字符序列将被进一步写入fileSearch 。 如果oldDirectory字符串长于251，该怎么办？ 结果字符串将长于fileSearch可以容纳的长度，这将导致在数组之外进行写入。 内存中的什么样的数据可能会损坏以及将导致什么结果，这是一个反问：

警告2

V519为 “背景”变量连续两次分配值。 也许这是一个错误。 检查行：1367、1373。Map.cpp 1373

 void mapclass::loadlevel(....) { .... case 4: //The Warpzone tmap = warplevel.loadlevel(rx, ry, game, obj); fillcontent(tmap); roomname = warplevel.roomname; tileset = 1; background = 3; // <= dwgfx.rcol = warplevel.rcol; dwgfx.backgrounddrawn = false; warpx = warplevel.warpx; warpy = warplevel.warpy; background = 5; // <= if (warpy) background = 4; if (warpx) background = 3; if (warpx && warpy) background = 5; break; .... } 

一个变量和同一变量连续两次分配一个值。 此外，在分配之间，此变量不在任何地方使用。 奇怪...也许这个顺序并没有违反程序的逻辑，但是这样的分配本身在编写代码时会带来一些混乱。 这是否实际上是一个错误只能由作者说。 尽管在代码中有更详细的示例说明此错误：

 void Game::loadquick(....) { .... else if (pKey == "frames") { frames = atoi(pText); frames = 0; } .... } 

在这里已经很清楚，这里的某个地方要么是逻辑错误，要么至少是不必要的分配。 也许第二行是为调试而临时编写的，然后他们忘记了删除它。 总计，PVS-Studio针对此类情况发出了8条警告。

警告3

创建了'basic_string'类型的V808'pKey'对象，但未使用。 编辑器.cpp 1866

 void editorclass::load(std::string &_path) { .... std::string pKey(pElem->Value()); .... if (pKey == "edEntities") { int i = 0; for (TiXmlElement *edEntityEl = pElem->FirstChildElement(); edEntityEl; edEntityEl = edEntityEl->NextSiblingElement()) { std::string pKey(edEntityEl->Value()); // <= //const char* pText = edEntityEl->GetText() ; if (edEntityEl->GetText() != NULL) { edentity[i].scriptname = std::string(edEntityEl->GetText()); } edEntityEl->QueryIntAttribute("x", &edentity[i].x); edEntityEl->QueryIntAttribute("y", &edentity[i].y); edEntityEl->QueryIntAttribute("t", &edentity[i].t); edEntityEl->QueryIntAttribute("p1", &edentity[i].p1); edEntityEl->QueryIntAttribute("p2", &edentity[i].p2); edEntityEl->QueryIntAttribute("p3", &edentity[i].p3); edEntityEl->QueryIntAttribute("p4", &edentity[i].p4); edEntityEl->QueryIntAttribute("p5", &edentity[i].p5); edEntityEl->QueryIntAttribute("p6", &edentity[i].p6); i++; } EditorData::GetInstance().numedentities = i; } .... } 

非常奇怪的代码。 分析器会警告已创建但尚未使用的变量pKey ，但实际上问题却变得更加有趣。 我特别用箭头标记了发出警告的行，因为此函数包含多个名为pKey的行定义。 是的，另一个这样的变量在for循环内声明，并且其名称与在循环外声明的变量重叠。

因此，如果在for循环外访问pKey字符串的值，则将得到等于pElem-> Value（）的值 ，但是如果在循环内进行相同的操作，则将得到等于edEntityEl-> Value（）的值 。 名称重叠是一个相当严重的错误，在代码检查过程中很难独自找到。

警告4

V805性能下降 。 使用'strlen（str）> 0'构造来识别空字符串效率不高。 一种更有效的方法是检查：str [0]！='\ 0'。 第1604章

 static char *prefDir = NULL; .... const char *PHYSFS_getPrefDir(const char *org, const char *app) { .... assert(strlen(prefDir) > 0); ... return prefDir; } /* PHYSFS_getPrefDir */ 

分析仪找到了可能进行微优化的地方。 它使用strlen函数检查C样式的字符串是否无效。 此函数“遍历”字符串的所有元素，并检查每个元素与终端零（'\ 0'）的相等性。 如果输入了大字符串，则仍将其每个字符与零字符串进行比较。

但是我们只需要检查字符串是否为空即可！ 为此，只需找出字符串的第一个字符是否为零终止符即可。 因此，要在assert中优化此检查，应编写：

 str[0] != '\0' 

这是分析仪给我们的建议。 当然，strlen函数调用处于assert宏条件下，因此只能在速度不太重要的调试版本中执行。 在发行版中，函数调用将消失并且代码将快速运行。 尽管如此，我还是想证明分析仪提出微优化的功能。

警告5

为了显示以下错误，我需要在此处附加两段代码： entclass类的声明及其构造函数。 让我们从公告开始：

 class entclass { public: entclass(); void clear(); bool outside(); public: //Fundamentals bool active, invis; int type, size, tile, rule; int state, statedelay; int behave, animate; float para; int life, colour; //Position and velocity int oldxp, oldyp; float ax, ay, vx, vy; int cx, cy, w, h; float newxp, newyp; bool isplatform; int x1, y1, x2, y2; //Collision Rules int onentity; bool harmful; int onwall, onxwall, onywall; //Platforming specific bool jumping; bool gravity; int onground, onroof; int jumpframe; //Animation int framedelay, drawframe, walkingframe, dir, actionframe; int yp; int xp; }; 

该类的构造函数如下所示：

 entclass::entclass() { clear(); } void entclass::clear() { // Set all values to a default, // required for creating a new entity active = false; invis = false; type = 0; size = 0; tile = 0; rule = 0; state = 0; statedelay = 0; life = 0; colour = 0; para = 0; behave = 0; animate = 0; xp = 0; yp = 0; ax = 0; ay = 0; vx = 0; vy = 0; w = 16; h = 16; cx = 0; cy = 0; newxp = 0; newyp = 0; x1 = 0; y1 = 0; x2 = 320; y2 = 240; jumping = false; gravity = false; onground = 0; onroof = 0; jumpframe = 0; onentity = 0; harmful = false; onwall = 0; onxwall = 0; onywall = 0; isplatform = false; framedelay = 0; drawframe = 0; walkingframe = 0; dir = 0; actionframe = 0; } 

足够的字段，不是吗？ PVS-Studio向其发出警告，潜伏在这里并不奇怪。

V730可能并非所有类的成员都在构造函数中初始化。 考虑检查：oldxp，oldyp。 Ent.cpp 3

如您所见，在这么大的列表中，该类的两个字段的初始化丢失了。 因此，它们的值仍未定义，因此，可以在程序中的其他位置读取和使用未知值。 通过眼睛检测这种错误非常困难。

警告6

考虑以下代码：

 void mapclass::loadlevel(....) { .... std::vector<std::string> tmap; .... tmap = otherlevel.loadlevel(rx, ry, game, obj); fillcontent(tmap); .... //  tmap    . } 

警告PVS-studio： V688 “ tmap”局部变量与一个类成员具有相同的名称，这可能导致混乱。 第1192章

实际上，如果您查看mapclass类内部，则可以在其中找到具有相同名称的相同向量：

 class mapclass { public: .... std::vector <int> roomdeaths; std::vector <int> roomdeathsfinal; std::vector <int> areamap; std::vector <int> contents; std::vector <int> explored; std::vector <int> vmult; std::vector <std::string> tmap; // <= .... }; 

不幸的是，在函数内部声明具有相同名称的向量会使在类中声明的向量不可见。 事实证明，在整个loadlevel函数中， tmap向量仅在函数内部发生变化。 在类中声明的向量保持不变！

有趣的是，PVS-Studio发现了多达20个这样的代码片段！ 在大多数情况下，它们与临时变量相关联，为方便起见，它们被声明为类的成员。 游戏的作者（及其唯一的开发者）自己写道，他曾经有这种不良习惯。 您可以在本文开头链接到的文章中了解有关此内容的信息。

他还指出，这种命名会导致有害且难以捕获的错误。 好吧，这样的错误确实很有害，但是使用静态分析来捕获它们并不困难:)

警告7

V601整数类型隐式转换为char类型。 游戏.cpp 4997

 void Game::loadquick(....) { .... else if (pKey == "totalflips") { totalflips = atoi(pText); } else if (pKey == "hardestroom") { hardestroom = atoi(pText); // <= } else if (pKey == "hardestroomdeaths") { hardestroomdeaths = atoi(pText); } .... } 

要了解问题所在，让我们看一下代码给定部分中的变量定义：

 //Some stats: int totalflips; std::string hardestroom; int hardestroomdeaths; 

变量totalflips和hardestroomdeaths是整数类型，因此将结果分配给其中的atoi函数是完全正常的。 但是，如果为std :: string分配一个整数值，会发生什么？ 事实证明，从语言的角度来看，这样的分配是完全有效的。 结果，一些难以理解的值将被写入hardestroom变量！

警告8

V1004在对nullptr进行了验证之后，不安全地使用了“ pElem”指针。 检查行：1739、1744。editor.cpp 1744

 void editorclass::load(std::string &_path) { .... TiXmlHandle hDoc(&doc); TiXmlElement *pElem; TiXmlHandle hRoot(0); version = 0; { pElem = hDoc.FirstChildElement().Element(); // should always have a valid root // but handle gracefully if it does if (!pElem) { printf("No valid root! Corrupt level file?\n"); } pElem->QueryIntAttribute("version", &version); // <= // save this for later hRoot = TiXmlHandle(pElem); } .... } 

分析器警告说，在检查了nullptr之后，会立即不安全地使用pElem指针。 为了确保分析器正确，请看一下Element（）函数的定义，该函数的返回值初始化pElem指针：

 /** @deprecated use ToElement. Return the handle as a TiXmlElement. This may return null. */ TiXmlElement *Element() const { return ToElement(); } 

从注释中可以看到，此函数可以返回null 。

现在想象一下这确实发生了。 在这种情况下会发生什么？ 事实是这种情况不会以任何方式处理。 是的，将显示一条消息，指出出了点问题，但是从字面上看，错误指针下面的一行将被取消引用。 反过来，这种取消引用将导致程序崩溃或不确定的行为。 这是一个非常严重的错误。

警告9

在代码的下一部分中，PVS-Studio发出了四个警告：

• V560条件表达式的一部分始终为true：x> =0。editor.cpp 1137
• V560条件表达式的一部分始终为true：y> =0。editor.cpp 1137
• V560条件表达式的一部分始终为true：x <40。editor.cpp 1137
• V560条件表达式的一部分始终为true：y <30。editor.cpp 1137

 int editorclass::at( int x, int y ) { if(x<0) return at(0,y); if(y<0) return at(x,0); if(x>=40) return at(39,y); if(y>=30) return at(x,29); if(x>=0 && y>=0 && x<40 && y<30) { return contents[x+(levx*40)+vmult[y+(levy*30)]]; } return 0; } 

所有警告均适用于最后一个if语句。 问题在于，对其执行的所有四个检查将始终返回true 。 我不会说这是一个严重的错误，但事实证明这很有趣。 作者决定认真对待此功能，以防万一，再次检查每个变量:)

该支票可以删除，因为 无论如何，执行线程将永远不会到达表达式“ return 0; ”。 尽管这不会改变程序的逻辑，但是它将使程序摆脱不必要的检查和无效代码。

警告10

特里在其游戏周年纪念日的文章中讽刺地说，控制游戏逻辑的要素之一是对Game :: updatestate（）函数的巨大切换，该功能立即负责大量不同的游戏状态。 很期待我会发现以下警告：

V2008循环复杂度：548。请考虑重构“游戏::更新状态”功能。 Game.cpp 612

是的，您理解正确：PVS-Studio估计功能的圈复杂度为548个单位。 五百四十八！ 据我了解-“整洁的代码”。 而且，尽管事实上，函数中只不过有一个switch表达式。 在开关本身中，我计算了300多个case表达式。

在我们的办公室里，作者之间为争取最长的文章而进行的竞争很小。 我很乐意将所有功能代码都带到这里（3450行），但是这样的胜利是不诚实的，因此我将自己局限于简单地提及巨大的开关。 我建议您遵循它并自己评估整体规模！ 顺便说一下，除了Game :: updatestate（）之外 ，PVS-Studio还发现多达44个函数的循环复杂性过高，其中10个复杂度超过200。

结论

我认为书面错误足以满足本文要求。 是的，项目中存在很多错误，但这确实是窍门：布置好自己的代码后，Terry Cavanagh证明，不必成为优秀的程序员来制作出色的游戏。 现在，十年后，特里颇具讽刺意味地回想起那些时代。 从错误中学习很重要，而实践是做到这一点的最佳方法。 而且，如果您的练习仍然可以产生像VVVVVVV这样的游戏，那么通常来说这太好了！ 恩...我去了，我可能会再玩一次:)

这些并非游戏代码中发现的所有错误。 如果您想自己看看还能找到什么，我建议下载并尝试使用PVS-Studio ！ 同样不要忘记，对于开源项目，我们提供免费许可证。

如果您想与讲英语的读者分享这篇文章，请使用翻译链接：George Gribkov。 VVVVVV ??? VVVVVV !!! :)