If you're reading this text, you've either thought that something was wrong with the headline or you've seen the name of a familiar computer game. VVVVVV is an indie platformer game that has stolen the hearts of many players by its pleasant external simplicity and no less pleasant internal complexity. A few days ago, VVVVVV turned 10 years, and the author of the game — Terry Cavanagh — celebrated this holiday by publishing its source code. What mind-boggling things is it hiding? Read the answer in this article.

Introduction

Overview of Analyzer Warnings

Warning 1

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

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

Warning 2

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

Warning 3

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

Warning 4

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

str[0] != '\0'

Warning 5

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

Oh, VVVVVV… I remember coming across it shortly after the release and being a big fan of pixel retro games, I was so excited to install it on my computer. I remember my first impressions: «Is that all? Just running around the square rooms?» I thought after a few minutes of playing. I didn't know what was waiting for me at the time. As soon as I got out of the starting location, I found myself in a small but confusing and florid two-dimensional world full of unusual landscapes and pixel artifacts unknown to me.I got carried away by the game. Eventually, I completely beat the game despite some challenges, like high complexity with skillfully applied game control, for instance — the main character can't jump, but is able to invert the direction of the gravity vector on himself. I have no idea how many times my character died then, but I'm sure the number of deaths is measured in the tens of hundreds. After all, every game has its own unique zest :)Anyway, let's go back to the source code, posted in honor of the game's anniversary At the moment, I'm a developer of the PVS-Studio, which is a static code analyzer for C, C++, C#, and Java. In addition to directly developing, we are also engaged in our product promotion. For us, one of the best ways to do this is to write articles about checking open source projects. Our readers get engaging articles on programming topics, and we get the opportunity to demonstrate the capabilities of PVS-Studio. So when I heard about the opening of the VVVVVV source code, I just couldn't get past it.In this article, we'll look at some interesting errors found by the PVS-Studio analyzer in the VVVVVV code, and take a detailed look at these errors. Point the gravity vector down and make yourself comfortable — we're just about to start! V512 A call of the 'sprintf' function will lead to overflow of the buffer 'fileSearch'. FileSystemUtils.cpp 307As you can see, the stringsandare of the same size: 260 characters. After writing the contents of thestring in the format string (the thirdargument), it will look like:This line is 9 characters longer than the original value of. It is this sequence of characters that will be written in. What happens if the length of thestring is more than 251? The resulting string will be longer thancould contain, which will lead to violating the array bounds. What data in RAM can be damaged and what result it will lead to is a matter of rhetorical question :) V519 The 'background' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1367, 1373. Map.cpp 1373The same variable is assigned a value twice in a row. However, this variable isn't used anywhere between assignments. Which is weird… This sequence may not violate the logic of the program, but such assignments themselves indicate some confusion when writing code. Whether this is a mistake in fact — only the author will be able to say for sure. Although there are more vivid examples of this error in the code:In this case, it's clear that an error is hiding somewhere either in logic or in redundant assignment. Perhaps, the second line was written temporarily for debugging, and then was just left forgotten. In total, PVS-Studio issued 8 warnings about such cases. V808 'pKey' object of 'basic_string' type was created but was not utilized. editor.cpp 1866This code is very strange. The analyzer warns about the created but not used variable, but in reality, the problem was more interesting. I intentionally highlighted the line triggered the warning with an arrow, as this function contains more than one string definition with the name. That's right, another such variable is declared inside theloop. It overlaps the one that's declared outside of the loop.Thus, if you refer to the value of thestring outside theloop, you'll get the value equal to, but when doing the same inside the loop, you'll get the value equal to. Overlapping names is a rather rough error, which might be very difficult to find on your own during code review. V805 Decreased performance. It is inefficient to identify an empty string by using 'strlen(str) > 0' construct. A more efficient way is to check: str[0] != '\0'. physfs.c 1604The analyzer found a fragment for potential micro-optimization. It uses thefunction to check if the string is empty. This function traverses all string elements and checks each of them for a null-terminator ('\0'). If we get a long string, its each character will be compared with a null-terminator.But we just need to check that the string is empty! All you need to do is to find out if the first string character is a terminal null. Therefore, to optimize this check inside the assert, it's worth writing:That's the recommendation the analyzer gives us. Sure, the call of the strlen function is in condition of themacro, therefore it will only be executed in the debugging version, where the speed isn't that important. In the release version, the call of the function and the code will execute fast. Despite this, I wanted to demonstrate what our analyzer can suggest in terms of micro-optimizations.To demonstrate the essence of another error, I have to cite two code fragments here: theclass declaration and its constructor. Let's start with the declaration:This class constructor looks as follows:Quite many fields, wouldn't you say? It's no wonder, PVS-Studio issued a warning for a bug, hiding here: V730 It is possible that not all members of a class are initialized inside the constructor. Consider inspecting: oldxp, oldyp. Ent.cpp 3As you can see, two class fields initializations got lost in such a long list. As a result, their values remained undefined, so they can be incorrectly read and used somewhere else in the program. It is very difficult to detect such a mistake just by reviewing.

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

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

Warning 7

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;

Warning 8

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?

"); } pElem->QueryIntAttribute("version", &version); // <= // save this for later hRoot = TiXmlHandle(pElem); } .... }

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

Warning 9

V560 A part of conditional expression is always true: x >= 0. editor.cpp 1137

V560 A part of conditional expression is always true: y >= 0. editor.cpp 1137

V560 A part of conditional expression is always true: x < 40. editor.cpp 1137

V560 A part of conditional expression is always 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; }

Warning 10

Look at this code:PVS-Studio warning: V688 The 'tmap' local variable possesses the same name as one of the class members, which can result in a confusion. Map.cpp 1192Indeed, looking inside theclass, you can find the same vector with the same name there:Unfortunately, same name vector declaration inside the function makes the vector declared in the class invisible. In turns out that thevector gets changed only inside of thefunction. The vector declared in the class remains the same!Interestingly, PVS-Studio has found 20 of such code fragments! For the most part, they relate to temporary variables that have been declared «for convenience» as class members. The game author (and its only developer) wrote about himself that he used to have this bad habit. You can read about it in the post — the link is given at the beginning of the article.He also noted that such names led to harmful bugs that were difficult to detect. Well, such errors may be really destructive, but catching them becomes less difficult if you use static analysis :) V601 The integer type is implicitly cast to the char type. Game.cpp 4997To understand what's going on, let's take a look at the variables' definitions from the given part of code:andvariables are integer, so it's perfectly normal to assign them the result of thefunction. But what happens if you assign an integer value to? Such assignment turns out to be valid from the language perspective. As a result, an unclear value will be written in thevariable! V1004 The 'pElem' pointer was used unsafely after it was verified against nullptr. Check lines: 1739, 1744. editor.cpp 1744The analyzer warns that thepointer is unsafely used right after its checking for. To make sure the analyzer is right, let's check out the definition of thefunction which returns the value which, in turns, initializes thepoiter:As we can see from the comment, this function might returnNow imagine that it really happened. What will happen in this case? The fact is that this situation won't be handled in any way. Yes, there will be a message that something went wrong, but the incorrect pointer will be dereferenced just one line below. Such dereferencing will result in either the program crash or undefined behavior. This is a pretty serious mistake.This code fragment triggered four PVS-Studio analyzer warnings:All warnings relate to the laststatement. The problem is that all four checks, performed in it, will always return. I wouldn't say it's a serious mistake, but it's quite funny. The author decided to take this function seriously and just in case checked each variable again :)He could have removed this check, as the execution flow won't get to the expression "" anyway. It won't change the program logic, but will help to get rid of redundant checks and dead code.In his article on the game's anniversary, Terry ironically noted that one of the elements that controlled the game's logic was the huge switch from thefunction, responsible for a large number of different states of the game at the same time. And it was quite expected that I would find the following warning: V2008 Cyclomatic complexity: 548. Consider refactoring the 'Game::updatestate' function. Game.cpp 612Yes, you got it right: PVS-Studio gave the function the following complexity rating — 548. Five hundred and forty-eight!!! This is how the «neat code» looks like. And this is despite the fact that, except for the switch statement there is almost nothing else in the function. In the switch itself, I counted more than 300 case-expressions.You know, in our company we have a small competition for the longest article. I'd love to bring the entire function code (3,450 lines) here, but such a win would be unfair, so I'll just limit myself to the link to the giant switch. I recommend that you follow the link and see its length for yourself! For the matter of that, in addition to, PVS-Studio has also found 44 functions with inflated cyclomatic complexity, 10 of which had a complexity number of more than 200.

Conclusion

I think the above errors are enough for this article. Yes, there were a lot of errors in the project, but it's a kind of a feature. By opening his code, Terry Cavanagh showed that you don't have to be a perfect programmer to write a great game. Now, 10 years later, Terry recalls those times with irony. It's important to learn from your mistakes, and practice is the best way to do it. And if your practice can give rise to a game like VVVVVV, it's just magnificent! Well… It's high time to play it one more time :)These weren't all errors found in the game code. If you want to see for yourself what else can be found — I suggest that you download and try PVS-Studio ! Also, don't forget that we provide open source projects with free licenses.