VVVVVV ??? VVVVVV !!! :)

如果您正在阅读本文,可能是因为您认为标题有误,或者您看到了熟悉的计算机游戏的名称。 VVVVVV是一款独立的平台游戏,其令人愉悦的外部简洁性和令人愉悦的内部复杂性吸引了许多玩家的心。 几天前,VVVVVV诞生了10年,该游戏的作者Terry Cavanagh通过发布其源代码庆祝了这个假期。 它隐藏了什么令人难以置信的东西? 阅读本文的答案。

图1

引言


哦,VVVVVV ...我记得发布后不久就遇到了它,并且是像素复古游戏的忠实拥护者,我非常高兴将其安装在计算机上。 我记得我的第一印象:“就这些吗? 只是在广场房间里跑来跑去? 我当时不知道是什么在等我。 一离开出发地点,我便发现自己身处一个小而又混乱而又富花的二维世界,里面充满了我所不知道的异常景观和像素伪像。

我被游戏迷住了。 最终,尽管遇到了一些挑战,但我还是完全击败了游戏,例如,复杂性高,并熟练地运用了游戏控制-主角不会跳跃,但能够颠倒重力矢量的方向。 我不知道我的角色当时死了多少次,但我敢肯定死亡人数以几百计。 毕竟,每个游戏都有其独特的热情:)

无论如何,让我们回到为纪念游戏周年纪念而发布的源代码。

目前,我是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); .... } 

如您所见,字符串fileSearcholdDirectory具有相同的大小:260个字符。 在格式字符串(第三个sprintf参数)中写入oldDirectory字符串的内容之后,它将看起来像:
 <i>contents_oldDirectory\*.vvvvvv</i> 

该行比oldDirectory的原始值长9个字符。 这是将在fileSearch中写入的字符序列。 如果oldDirectory字符串的长度大于251,会发生什么? 结果字符串将长于fileSearch可能包含的长度,这将导致违反数组范围。 RAM中的哪些数据可能会损坏,以及将导致什么结果,这是一个反问的问题:)

警告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的值->值() 。 重叠的名称是一个相当粗略的错误,在代码检查过程中可能很难自行找到。

警告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函数检查字符串是否为空。 此函数遍历所有字符串元素,并检查每个字符串元素是否为空终止符('\ 0')。 如果我们得到一个长字符串,则将其每个字符与一个空终止符进行比较。

但是我们只需要检查字符串是否为空即可! 您需要做的就是找出第一个字符串字符是否为终端null。 因此,要优化断言中的此检查,值得编写:

 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

如您所见,这么长的列表中丢失了两个类字段初始化。 结果,它们的值保持未定义状态,因此它们可能会在程序的其他位置被错误地读取和使用。 仅通过检查就很难发现这种错误。

图4


警告6


看下面的代码:

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

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; 

totalflipshardestroomdeaths变量是整数,因此为其分配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 poiter:

 /** @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。458分!!! 这就是“整洁的代码”的样子。 尽管有这样一个事实,除了switch语句外,函数中几乎没有其他内容。 在交换机本身中,我计算了300多个case-expression。

您知道,在我们公司中,对于最长文章的竞争很小。 我很想将整个功能代码(3,450行)带到这里,但是这样的胜利将是不公平的,因此,我将自己限制在与巨型开关的链接上。 我建议您点击链接,自己看看它的长度! 因此,除了Game :: updatestate()之外 ,PVS-Studio还发现44个函数的循环复杂度过高,其中10个的复杂度大于200。

图5


结论


我认为以上错误足以满足本文要求。 是的,项目中存在很多错误,但这是一种功能。 通过打开他的代码,Terry Cavanagh表明您不必成为完美的程序员来编写出色的游戏。 十年后的现在,特里颇具讽刺意味地回想起那些时代。 从错误中学习非常重要,而实践是做到这一点的最佳方法。 而且,如果您的练习可以引发VVVVVV之类的游戏,那就太好了! 好吧,现在该再玩一次了:)

这些并不是在游戏代码中发现的所有错误。 如果您想亲自看看还有什么可以找到的-我建议您下载并尝试PVS-Studio ! 另外,请不要忘记我们开源项目提供了免费许可证。

Source: https://habr.com/ru/post/zh-CN484388/


All Articles