Wenn Sie diesen Text lesen, haben Sie entweder gedacht, dass etwas mit dem Titel des Artikels nicht stimmt, oder Sie haben den Namen eines bekannten Computerspiels darin gesehen. VVVVVV ist ein Indie-Plattform-Spiel, das mit seiner angenehmen äußeren Einfachheit und ebenso angenehmen inneren Komplexität die Herzen vieler Spieler erobert hat. Vor ein paar Tagen wurde VVVVVV 10 Jahre alt und der Autor des Spiels - Terry Cavanagh - feierte diesen Feiertag mit der Veröffentlichung seines Quellcodes. Was ist "lecker" drin? Lesen Sie die Antwort in diesem Artikel.
Einleitung
Oh, VVVVVV ... Ich erinnere mich, wie ich kurz nach der Veröffentlichung darauf gestoßen bin, und da ich ein großer Fan von Pixel-Retro-Spielen bin, habe ich es glücklich auf meinem Computer installiert. Ich erinnere mich an meine ersten Eindrücke: „Und das ist alles? Nur durch die quadratischen Räume rennen? “, Dachte ich nach ein paar Minuten Spielzeit. Ich wusste noch nicht, was mich erwartete. Sobald ich den Startort verließ, befand ich mich in einer kleinen, aber verwirrten und reich verzierten zweidimensionalen Welt voller ungewöhnlicher Landschaften und Pixelartefakte, die mir unbekannt waren.
Dieses Spiel zog mich an. Trotz der hohen Komplexität, die damals von der ungewöhnlichen Kontrolle gekonnt überwunden wurde (die Hauptfigur weiß nicht, wie man springt, kann aber die Richtung des Schwerkraftvektors für mich selbst umkehren), habe ich es komplett überholt. Ich habe keine Ahnung, wie oft mein Charakter damals gestorben ist, aber ich bin mir sicher, dass die Anzahl der Todesfälle in Hunderten gemessen wird. Trotzdem hat dieses Spiel einen einzigartigen Charme :)
Kehren wir zum Quellcode zurück, der zu
Ehren des Jubiläums des Spiels erstellt wurde .
Im Moment bin ich C ++ - der Entwickler im PVS-Studio-Team - ein statischer Code-Analysator für C, C ++, C # und Java. Neben der eigentlichen Entwicklung befassen wir uns auch mit der Werbung für unser Produkt. Für uns besteht eine der besten Möglichkeiten darin, Artikel über das Überprüfen von Open Source-Projekten zu schreiben. Unsere Leser erhalten interessante Artikel zu Programmierthemen und wir haben die Möglichkeit, die Möglichkeiten von PVS-Studio klar zu demonstrieren. Als ich von der Eröffnung des Quellcodes VVVVVV erfuhr, konnte ich einfach nicht vorbei.
In diesem Artikel untersuchen wir einige interessante Fehler, die der PVS-Studio-Analysator im VVVVVV-Code gefunden hat, und führen eine detaillierte Analyse dieser Fehler durch. Bringen Sie den Schwerkraftvektor wieder in die untere Position und lehnen Sie sich zurück: Wir fangen an!
Übersicht der vom Analysegerät ausgegebenen 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
Zeilen fileSearch und
oldDirectory dieselbe Größe: 260 Zeichen. Die Formatierungszeichenfolge (das dritte Argument für
sprintf )
sieht nach dem Einsetzen des Inhalts der Zeichenfolge
oldDirectory folgendermaßen aus :
<i>_oldDirectory\*.vvvvvv</i>
Diese Zeichenfolge ist 9 Zeichen länger als das ursprüngliche
oldDirectory . Es ist diese Zeichenfolge, die weiter in
fileSearch geschrieben
wird . Was passiert, wenn der
oldDirectory- String länger als 251 ist? Die resultierende Zeichenfolge ist länger als in
fileSearch zulässig , was dazu führt, dass außerhalb des Arrays geschrieben wird. Welche Art von Daten im RAM möglicherweise beschädigt werden und zu welchem Ergebnis sie führen, ist eine rhetorische Frage :)
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:
Ein und dieselbe Variable erhält zweimal hintereinander einen Wert. Außerdem wird diese Variable zwischen Zuweisungen nirgendwo verwendet. Seltsam ... Vielleicht verstößt diese Sequenz nicht gegen die Logik des Programms, aber solche Zuweisungen an sich sprechen für eine gewisse Verwirrung beim Schreiben von Code. Ob dies tatsächlich ein Fehler ist, kann nur der Autor sagen. Der Code enthält zwar hellere Beispiele für diesen Fehler:
void Game::loadquick(....) { .... else if (pKey == "frames") { frames = atoi(pText); frames = 0; } .... }
Hier ist schon klar, dass hier irgendwo entweder ein Fehler in der Logik oder zumindest eine unnötige Zuordnung liegt. Vielleicht wurde die zweite Zeile vorübergehend zum Debuggen geschrieben, und dann vergaßen sie, sie zu löschen. Insgesamt hat PVS-Studio 8 Warnungen zu solchen Situationen 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());
Sehr seltsamer Code. Der Analysator warnt vor der erstellten, aber nicht verwendeten Variablen
pKey , aber tatsächlich stellte sich das Problem als interessanter heraus. Ich habe die Zeile, in der die Warnung ausgegeben wurde, mit einem Pfeil markiert, da diese Funktion mehr als eine
Zeilendefinition mit dem Namen
pKey enthält . Ja, eine andere solche Variable wird innerhalb der
for- Schleife deklariert und überlappt mit ihrem Namen die außerhalb der Schleife deklarierte.
Wenn Sie also auf den Wert der
pKey- Zeichenfolge außerhalb der
for- Schleife zugreifen, erhalten Sie einen Wert, der
pElem-> Value () entspricht . Wenn Sie dies jedoch innerhalb der Schleife tun, erhalten Sie einen Wert, der
edEntityEl-> Value () entspricht . Überlappende Namen sind ein ziemlich schwerwiegender Fehler, der bei der Codeüberprüfung für sich allein sehr schwierig zu finden sein kann.
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 einen Ort für eine mögliche Mikrooptimierung gefunden. Mit der Funktion
strlen wird eine Zeichenfolge im C-Stil auf ungültig überprüft. Diese Funktion "durchläuft" alle Elemente des Strings und prüft sie auf Gleichheit mit Terminal Null ('\ 0'). Wenn eine große Zeichenfolge eingegeben wird, wird jedes seiner Zeichen weiterhin mit einer Zeichenfolge Null verglichen.
Wir müssen aber nur überprüfen, ob der String nicht leer ist! Finden Sie dazu einfach heraus, ob das erste Zeichen der Zeichenfolge eine Terminal-Null ist. Um diese Prüfung in assert zu optimieren, sollten Sie daher schreiben:
str[0] != '\0'
Dies ist die Empfehlung, die der Analysator uns gibt. Natürlich befindet sich der Aufruf der Funktion strlen im Assert-Makro-Zustand, daher wird er nur in der Debug-Version ausgeführt, in der die Geschwindigkeit nicht so wichtig ist. In der Release-Version verschwindet der Funktionsaufruf und der Code funktioniert schnell. Trotzdem wollte ich die Fähigkeiten des Analysators demonstrieren, um Mikrooptimierungen vorzuschlagen.
Warnung 5
Um den folgenden Fehler
anzuzeigen , muss ich hier zwei Codeteile anhängen: die Deklaration der
entclass- Klasse und ihren Konstruktor. Beginnen wir mit der Ankündigung:
class entclass { public: entclass(); void clear(); bool outside(); public:
Der Konstruktor dieser Klasse sieht folgendermaßen aus:
entclass::entclass() { clear(); } void entclass::clear() {
Genug Felder, nicht wahr? Es ist nicht verwunderlich, dass hier ein Fehler lauerte, zu dem PVS-Studio eine Warnung herausgab:
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, ging in einer so großen Liste die Initialisierung von zwei Feldern der Klasse verloren. Somit blieben ihre Werte undefiniert, wodurch an einer anderen Stelle im Programm ein unbekannter Wert gelesen und daraus verwendet werden kann. Es ist sehr schwierig, einen solchen Fehler durch die Augen zu erkennen.
Warnung 6
Betrachten Sie den Code:
void mapclass::loadlevel(....) { .... std::vector<std::string> tmap; .... tmap = otherlevel.loadlevel(rx, ry, game, obj); fillcontent(tmap); ....
Warnung PVS-studio:
V688 Die lokale Variable 'tmap' hat denselben Namen wie einer der Klassenmitglieder, was zu Verwirrung führen kann. Map.cpp 1192
Wenn Sie in die
Mapclass- Klasse
schauen , können Sie dort 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 das Deklarieren eines Vektors mit demselben Namen in einer Funktion den in der Klasse deklarierten Vektor unsichtbar. Es stellt sich heraus, dass sich der
tmap- Vektor während der gesamten
Loadlevel- Funktion nur innerhalb der Funktion ändert. Der in der Klasse deklarierte Vektor bleibt unverändert!
Interessanterweise hat PVS-Studio bis zu 20 solcher Codefragmente entdeckt! Zum größten Teil sind sie temporären Variablen zugeordnet, die der Einfachheit halber als Mitglieder der Klasse deklariert wurden. Der Autor des Spiels (und sein einziger Entwickler) selbst schrieb, dass er diese schlechte Angewohnheit hatte. Sie können darüber in einem Beitrag lesen, den ich am Anfang dieses Artikels verlinkt habe.
Er merkt auch an, dass solche Benennungen zu schädlichen und schwer zu fassenden Fehlern führten. Nun, solche Bugs können wirklich schädlich sein, aber es wird nicht schwierig sein, statische Analysen zu verwenden, um 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);
Um zu verstehen, was los ist, werfen wir einen Blick auf die Definitionen der Variablen im angegebenen Codeabschnitt:
Die Variablen
totalflips und
hardestroomdeaths sind vom Typ integer, daher ist es völlig normal, das Ergebnis der
atoi- Funktion in ihnen
zuzuweisen . Aber was passiert, wenn Sie
std :: string einen ganzzahligen Wert zuweisen? Es zeigt sich, dass aus sprachlicher Sicht eine solche Zuordnung voll gültig ist. Infolgedessen wird ein unverständlicher 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 der Überprüfung auf
nullptr unsicher verwendet wird. Sehen Sie sich die Definition der
Element () -Funktion an, deren Rückgabewert den
pElem- Zeiger initialisiert, um sicherzustellen, dass der Analysator richtig ist:
TiXmlElement *Element() const { return ToElement(); }
Wie Sie dem Kommentar entnehmen können, kann diese Funktion
null zurückgeben .
Stellen Sie sich nun vor, dass dies wirklich passiert ist. Was wird in diesem Fall passieren? Tatsache ist, dass eine solche Situation in keiner Weise behandelt wird. Ja, es wird eine Meldung angezeigt, die besagt, dass ein Fehler aufgetreten ist. Eine Zeile unter dem falschen Zeiger wird jedoch dereferenziert. Eine solche Dereferenzierung führt entweder zu einem Programmabsturz oder zu undefiniertem Verhalten. Dies ist ein ziemlich schwerwiegender Fehler.
Warnung 9
PVS-Studio hat im nächsten Codeabschnitt vier Warnungen ausgegeben:
- 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 gelten für die letzte
if- Anweisung. Das Problem ist, dass alle vier Überprüfungen immer
true zurückgeben . Ich würde nicht sagen, dass dies ein schwerwiegender Fehler ist, aber es hat sich als ziemlich lustig herausgestellt. Der Autor hat sich entschlossen, diese Funktion ernst zu nehmen und für alle Fälle jede Variable erneut zu prüfen :)
Diese Prüfung kann entfernt werden, weil Der Ausführungsthread erreicht sowieso nie den Ausdruck "
return 0; ". Dies ändert zwar nichts an der Logik des Programms, befreit es jedoch von unnötigen Überprüfungen und totem Code.
Warnung 10
In seinem Artikel zum Jubiläum des Spiels sagt Terry ironisch, dass eines der Elemente, die die Logik des Spiels steuern, ein großer Wechsel von der Funktion
Game :: updatestate () war , die sofort für eine große Anzahl verschiedener
Spielzustände verantwortlich ist. 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 schätzte die zyklomatische Komplexität einer Funktion auf 548 Einheiten. Fünfhundertachtundvierzig !!! Das verstehe ich - "ordentlicher Code." Und das trotz der Tatsache, dass es in einer Funktion tatsächlich nur einen Schalterausdruck gibt. Im Switch selbst habe ich über 300 case-Ausdrücke gezählt.
In unserem Büro gibt es einen kleinen Wettbewerb zwischen den Autoren um den längsten Artikel. Ich würde gerne den gesamten Funktionscode hierher bringen (3450 Zeilen), aber ein solcher Sieg wäre unehrlich, daher beschränke ich mich darauf, mich nur
auf den riesigen Schalter zu
beziehen . Ich empfehle ihm zu folgen und die gesamte Skala selbst zu bewerten! Neben
Game :: updatestate () hat PVS-Studio 44 Funktionen mit übermäßiger zyklomatischer Komplexität gefunden, von denen 10 eine Komplexität von mehr als 200 aufweisen.
Fazit
Ich denke, die schriftlichen Fehler reichen für den Artikel aus. Ja, es gab viele Fehler im Projekt, aber genau das ist der Trick: Terry Cavanagh zeigte, dass es nicht notwendig ist, ein guter Programmierer zu sein, um ein gutes Spiel zu machen. Jetzt, nach 10 Jahren, erinnert sich Terry ironisch an diese Zeiten. Aus Ihren Fehlern zu lernen ist wichtig und Übung ist der beste Weg, dies zu tun. Und wenn Ihre Praxis immer noch zu einem Spiel wie VVVVVVV führen kann, dann ist das im Allgemeinen großartig! Ehh ... Ich gehe und ich werde es wahrscheinlich wieder spielen :)
Dies waren nicht alle Fehler, die im Spielcode gefunden wurden. Wenn Sie selbst sehen möchten, was Sie sonst noch finden, empfehle ich Ihnen,
PVS-Studio herunterzuladen und auszuprobieren ! Vergessen Sie auch nicht, dass wir für Open Source-Projekte
eine kostenlose Lizenz anbieten.
Wenn Sie diesen Artikel mit einem englischsprachigen Publikum teilen möchten, verwenden Sie bitte den Link zur Übersetzung: George Gribkov.
VVVVVV ??? VVVVVV !!! :) .