Wenn Sie diesen Text lesen, haben Sie entweder gedacht, dass etwas mit der Überschrift nicht stimmt, oder Sie haben den Namen eines bekannten Computerspiels gesehen. VVVVVV ist ein Indie-Plattformspiel, das durch seine angenehme äußere Einfachheit und nicht weniger angenehme innere Komplexität vielen Spielern die Herzen gestohlen hat. Vor einigen Tagen wurde VVVVVV 10 Jahre alt, und der Autor des Spiels - Terry Cavanagh - feierte diesen Feiertag mit der Veröffentlichung seines Quellcodes. Was für verrückte Dinge verbirgt es? Lesen Sie die Antwort in diesem Artikel.
Einleitung
Oh, VVVVVV ... Ich erinnere mich, dass ich kurz nach der Veröffentlichung darauf gestoßen bin und ein großer Fan von Pixel-Retro-Spielen war. Ich war so aufgeregt, es auf meinem Computer zu installieren. Ich erinnere mich an meine ersten Eindrücke: "Ist das alles? Nur durch die quadratischen Räume rennen? “, Dachte ich nach ein paar Minuten des Spielens. Ich wusste damals nicht, was mich erwartete. Sobald ich den Startort verlassen hatte, befand ich mich in einer kleinen, aber verwirrenden und floriden zweidimensionalen Welt voller ungewöhnlicher Landschaften und Pixelartefakte, die mir unbekannt waren.
Ich wurde vom Spiel mitgerissen. Letztendlich habe ich das Spiel trotz einiger Herausforderungen, wie zum Beispiel hoher Komplexität mit geschickt angewandter Spielsteuerung, komplett geschlagen - die Hauptfigur kann nicht springen, aber die Richtung des Gravitationsvektors auf sich selbst umkehren. Ich habe keine Ahnung, wie oft mein Charakter damals gestorben ist, aber ich bin sicher, dass die Anzahl der Todesfälle in Zehntausenden gemessen wird. Immerhin hat jedes Spiel seinen eigenen Reiz :)
Wie auch immer, kehren wir zum Quellcode zurück, der
zu Ehren des Jubiläums des Spiels veröffentlicht wurde .
Im Moment bin ich Entwickler des PVS-Studios, eines statischen Code-Analysators für C, C ++, C # und Java. Neben der direkten Entwicklung engagieren wir uns auch für unsere Produktwerbung. Für uns besteht eine der besten Möglichkeiten darin, Artikel über das Überprüfen von Open Source-Projekten zu schreiben. Unsere Leser erhalten spannende Artikel zu Programmierthemen und wir haben die Möglichkeit, die Funktionen von PVS-Studio zu demonstrieren. Als ich von der Eröffnung des VVVVVV-Quellcodes hörte, kam ich einfach nicht daran vorbei.
In diesem Artikel untersuchen wir einige interessante Fehler, die der PVS-Studio-Analysator im VVVVVV-Code gefunden hat, und werfen einen detaillierten Blick auf diese Fehler. Zeigen Sie mit dem Gravitationsvektor nach unten und machen Sie es sich bequem - wir stehen kurz vor dem Start!
Übersicht der Analyzer-Warnungen
Warnung 1
V512 Ein Aufruf der Funktion 'sprintf' führt zum Überlauf des Puffers '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]; .... strcpy(oldDirectory, output); sprintf(fileSearch, "%s\\*.vvvvvv", oldDirectory); .... }
Wie Sie sehen, haben die Zeichenfolgen
fileSearch und
oldDirectory dieselbe Größe: 260 Zeichen. Nach dem Schreiben des Inhalts der
oldDirectory- Zeichenfolge in die
Formatzeichenfolge (das dritte
sprintf- Argument) sieht es folgendermaßen aus:
<i>contents_oldDirectory\*.vvvvvv</i>
Diese Zeile ist 9 Zeichen länger als der ursprüngliche Wert von
oldDirectory . Diese Zeichenfolge wird in
fileSearch geschrieben . Was passiert, wenn der
oldDirectory- String länger als 251 ist? Der resultierende String ist länger als
fileSearch enthalten könnte, was dazu führt, dass die Array-Grenzen verletzt werden. Welche Daten im RAM beschädigt werden können und zu welchem Ergebnis sie führen, ist rhetorisch fraglich :)
Warnung 2
V519 Der 'Hintergrund'-Variablen werden nacheinander zweimal Werte zugewiesen. Vielleicht ist das ein Fehler. Überprüfen Sie die Zeilen: 1367, 1373. Map.cpp 1373
void mapclass::loadlevel(....) { .... case 4:
Dieselbe Variable erhält zweimal hintereinander einen Wert. Diese Variable wird jedoch nicht zwischen Zuweisungen verwendet. Was seltsam ist ... Diese Sequenz verstößt möglicherweise nicht gegen die Logik des Programms, aber solche Zuweisungen selbst weisen auf Verwirrung beim Schreiben von Code hin. Ob dies tatsächlich ein Fehler ist, kann nur der Autor mit Sicherheit sagen. Der Code enthält zwar anschaulichere Beispiele für diesen Fehler:
void Game::loadquick(....) { .... else if (pKey == "frames") { frames = atoi(pText); frames = 0; } .... }
In diesem Fall ist klar, dass sich ein Fehler entweder in der Logik oder in der redundanten Zuordnung versteckt. Vielleicht wurde die zweite Zeile vorübergehend zum Debuggen geschrieben und dann einfach vergessen. Insgesamt hat PVS-Studio 8 Warnungen in solchen Fällen ausgegeben.
Warnung 3
V808 'pKey'-Objekt vom Typ' basic_string 'wurde erstellt, aber nicht verwendet. editor.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());
Dieser Code ist sehr seltsam. Der Analysator warnt vor der erstellten, aber nicht verwendeten Variablen
pKey , aber in Wirklichkeit war das Problem interessanter. Ich habe die Zeile, die die Warnung ausgelöst hat, absichtlich mit einem Pfeil markiert, da diese Funktion mehr als eine String-Definition mit dem Namen
pKey enthält . Richtig, eine weitere solche Variable wird in der
for- Schleife deklariert. Sie überlappt diejenige, die außerhalb der Schleife deklariert wurde.
Wenn Sie also auf den Wert der
pKey- Zeichenfolge außerhalb der
for- Schleife verweisen, erhalten Sie den Wert
pElem-> Value () . Wenn Sie dies jedoch innerhalb der Schleife tun, erhalten Sie den Wert
edEntityEl -> Wert () . Überlappende Namen sind ein ziemlich grober Fehler, der bei der Codeüberprüfung möglicherweise nur sehr schwer auffindbar ist.
Warnung 4
V805 Leistungseinbußen. Es ist ineffizient, eine leere Zeichenfolge mit dem Konstrukt 'strlen (str)> 0' zu identifizieren. Eine effizientere Möglichkeit besteht darin, Folgendes zu überprüfen: str [0]! = '\ 0'. physfs.c 1604
static char *prefDir = NULL; .... const char *PHYSFS_getPrefDir(const char *org, const char *app) { .... assert(strlen(prefDir) > 0); ... return prefDir; }
Der Analysator hat ein Fragment für eine mögliche Mikrooptimierung gefunden. Mit der Funktion
strlen wird überprüft, ob die Zeichenfolge leer ist. Diese Funktion durchläuft alle Zeichenfolgenelemente und sucht nach einem Nullterminator ('\ 0'). Wenn wir eine lange Zeichenfolge erhalten, wird jedes Zeichen mit einem Nullterminator verglichen.
Aber wir müssen nur überprüfen, ob der String leer ist! Sie müssen lediglich herausfinden, ob das erste Zeichen eine Terminal-Null ist. Um diese Prüfung innerhalb der Zusicherung zu optimieren, lohnt es sich daher, Folgendes zu schreiben:
str[0] != '\0'
Das ist die Empfehlung, die uns der Analysator gibt. Sicher, der Aufruf der Funktion strlen ist im Zustand des Assert-Makros, daher wird er nur in der Debug-Version ausgeführt, in der die Geschwindigkeit nicht so wichtig ist. In der Release-Version werden der Aufruf der Funktion und der Code schnell ausgeführt. Trotzdem wollte ich zeigen, was unser Analysegerät in Bezug auf Mikrooptimierungen vorschlagen kann.
Warnung 5
Um die Essenz eines anderen Fehlers zu demonstrieren, muss ich hier zwei Codefragmente zitieren: die Deklaration der
entclass- Klasse und ihren Konstruktor. Beginnen wir mit der Deklaration:
class entclass { public: entclass(); void clear(); bool outside(); public:
Dieser Klassenkonstruktor sieht folgendermaßen aus:
entclass::entclass() { clear(); } void entclass::clear() {
Ziemlich viele Felder, sagst du nicht? Es ist kein Wunder, dass PVS-Studio eine Warnung für einen Fehler ausgegeben hat, die sich hier versteckt:
V730 Es ist möglich, dass nicht alle Member einer Klasse im Konstruktor initialisiert werden. Betrachten Sie die Überprüfung: oldxp, oldyp. Ent.cpp 3
Wie Sie sehen, gingen in einer so langen Liste zwei Initialisierungen von Klassenfeldern verloren. Infolgedessen blieben ihre Werte undefiniert, sodass sie falsch gelesen und an anderer Stelle im Programm verwendet werden können. Es ist sehr schwierig, einen solchen Fehler nur durch Überprüfung zu erkennen.
Warnung 6
Schau dir diesen Code an:
void mapclass::loadlevel(....) { .... std::vector<std::string> tmap; .... tmap = otherlevel.loadlevel(rx, ry, game, obj); fillcontent(tmap); ....
PVS-Studio Warnung:
V688 Die lokale Variable 'tmap' hat denselben Namen wie einer der Klassenmitglieder, was zu Verwirrung führen kann. Map.cpp 1192
Tatsächlich können Sie in der
Mapclass- Klasse denselben Vektor mit demselben Namen finden:
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;
Leider macht die gleichnamige Vektordeklaration innerhalb der Funktion den in der Klasse deklarierten Vektor unsichtbar. In stellt sich heraus, dass der
tmap- Vektor nur innerhalb der
loadlevel- Funktion
geändert wird. Der in der Klasse deklarierte Vektor bleibt gleich!
Interessanterweise hat PVS-Studio 20 solcher Codefragmente gefunden! Zum größten Teil beziehen sie sich auf temporäre Variablen, die aus Gründen der Bequemlichkeit als Klassenmitglieder deklariert wurden. Der Spieleautor (und sein einziger Entwickler) schrieb über sich, dass er diese schlechte Angewohnheit hatte. Sie können darüber in der Post lesen - der Link ist am Anfang des Artikels angegeben.
Er bemerkte auch, dass solche Namen zu schädlichen Fehlern führten, die schwer zu entdecken waren. Solche Fehler können sehr zerstörerisch sein, aber wenn Sie statische Analysen verwenden, wird es weniger schwierig, sie zu finden :)
Warnung 7
V601 Der Integer-Typ wird implizit in den Zeichen-Typ umgewandelt. Game.cpp 4997
void Game::loadquick(....) { .... else if (pKey == "totalflips") { totalflips = atoi(pText); } else if (pKey == "hardestroom") { hardestroom = atoi(pText);
Werfen wir einen Blick auf die Definitionen der Variablen aus dem angegebenen Teil des Codes, um zu verstehen, was los ist:
totalflips- und
hardestroomdeaths- Variablen sind Ganzzahlen, daher ist es ganz normal, ihnen das Ergebnis der
atoi- Funktion
zuzuweisen . Aber was passiert, wenn Sie
std :: string einen ganzzahligen Wert zuweisen? Eine solche Zuordnung erweist sich aus sprachlicher Sicht als gültig. Dadurch wird ein unklarer Wert in die Variable
hardestroom geschrieben !
Warnung 8
V1004 Der Zeiger 'pElem' wurde unsicher verwendet, nachdem er gegen nullptr verifiziert wurde. Überprüfen Sie die Zeilen: 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();
Der Analysator warnt, dass der
pElem- Zeiger unmittelbar nach seiner Überprüfung auf
nullptr nicht sicher verwendet wird . Überprüfen Sie die Definition der
Element () -Funktion, die den Wert zurückgibt, der
wiederum den
pElem- Poiter initialisiert, um sicherzustellen, dass der Analysator richtig ist:
TiXmlElement *Element() const { return ToElement(); }
Wie wir dem Kommentar entnehmen können, gibt diese Funktion möglicherweise
null zurück .
Nun stell dir vor, dass es wirklich passiert ist. Was wird in diesem Fall passieren? Tatsache ist, dass diese Situation in keiner Weise behandelt wird. Ja, es wird eine Meldung angezeigt, dass ein Fehler aufgetreten ist, aber der falsche Zeiger wird nur eine Zeile darunter dereferenziert. Eine solche Dereferenzierung führt entweder zum Absturz des Programms oder zu undefiniertem Verhalten. Dies ist ein ziemlich schwerwiegender Fehler.
Warnung 9
Dieses Codefragment löste vier Warnungen des PVS-Studio-Analysators aus:
- V560 Ein Teil des bedingten Ausdrucks ist immer wahr: x> = 0. editor.cpp 1137
- V560 Ein Teil des bedingten Ausdrucks ist immer wahr: y> = 0. editor.cpp 1137
- V560 Ein Teil des bedingten Ausdrucks ist immer wahr: x <40. editor.cpp 1137
- V560 Ein Teil des bedingten Ausdrucks ist immer wahr: 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; }
Alle Warnungen beziehen sich auf die letzte
if- Anweisung. Das Problem ist, dass alle vier darin durchgeführten Prüfungen immer
true zurückgeben . Ich würde nicht sagen, dass es ein schwerwiegender Fehler ist, aber es ist ziemlich lustig. Der Autor hat sich entschlossen, diese Funktion ernst zu nehmen und für alle Fälle jede Variable erneut zu prüfen :)
Er hätte diese Prüfung entfernen können, da der Ausführungsablauf sowieso nicht zum Ausdruck "
return 0; " Es ändert nichts an der Programmlogik, hilft aber dabei, redundante Prüfungen und toten Code loszuwerden.
Warnung 10
In seinem Artikel zum Jubiläum des Spiels bemerkte Terry ironischerweise, dass eines der Elemente, die die Logik des Spiels kontrollierten, der große Wechsel von der Funktion
Game :: updatestate () war , die gleichzeitig für eine große Anzahl verschiedener
Spielzustände verantwortlich war . Und es wurde ziemlich erwartet, dass ich die folgende Warnung finden würde:
V2008 Cyclomatic Complexity: 548. Überlegen Sie, ob Sie die Funktion 'Game :: updatestate' überarbeiten möchten. Game.cpp 612
Ja, Sie haben es richtig verstanden: PVS-Studio gab der Funktion die folgende Komplexitätsbewertung - 548. Fünfhundertachtundvierzig !!! So sieht der "nette Code" aus. Und das trotz der Tatsache, dass außer der switch-Anweisung fast nichts anderes in der Funktion ist. Im Switch selbst habe ich mehr als 300 case-expressions gezählt.
Sie wissen, in unserer Firma gibt es einen kleinen Wettbewerb um den längsten Artikel. Ich würde gerne den gesamten Funktionscode (3.450 Zeilen) hierher bringen, aber ein solcher Gewinn wäre unfair, also beschränke ich mich nur auf den
Link zum Riesenschalter. Ich empfehle Ihnen, dem Link zu folgen und seine Länge selbst zu sehen! Aus diesem
Grund hat PVS-Studio neben
Game :: updatestate () auch 44 Funktionen mit überhöhter zyklomatischer Komplexität gefunden, von denen 10 eine Komplexitätszahl von mehr als 200 hatten.
Fazit
Ich denke die obigen Fehler reichen für diesen Artikel aus. Ja, es gab viele Fehler im Projekt, aber es ist eine Art Feature. Als Terry Cavanagh seinen Code öffnete, zeigte er, dass man kein perfekter Programmierer sein muss, um ein großartiges Spiel zu schreiben. Jetzt, 10 Jahre später, erinnert sich Terry ironisch an diese Zeiten. Es ist wichtig, aus Ihren Fehlern zu lernen, und Übung ist der beste Weg, dies zu tun. Und wenn dein Training zu einem Spiel wie VVVVVV führen kann, ist es einfach großartig! Nun ... es ist höchste Zeit, es noch einmal zu spielen :)
Dies waren nicht alle Fehler, die im Spielcode gefunden wurden. Wenn Sie selbst sehen möchten, was sonst noch zu finden ist, empfehlen wir Ihnen
, PVS-Studio herunterzuladen und zu testen. Vergessen Sie auch nicht, dass wir Open Source-Projekte mit kostenlosen Lizenzen versehen.