0 AD ist ein dreidimensionales Spiel im Genre der historischen Strategie in Echtzeit, das von der Freiwilligengemeinschaft entwickelt wurde. Die Größe der Codebasis ist klein und ich habe beschlossen, das Spiel als Pause von großen Projekten wie Android und XNU Kernel zu überprüfen. Vor uns liegt also ein Projekt mit 165.000 Codezeilen in C ++. Mal sehen, was daran interessant ist, kann mit dem statischen Analysegerät PVS-Studio ermittelt werden.
0 AD
0 AD (0 A.D.) ist ein kostenloses dreidimensionales Spiel im Genre der historischen Strategie in Echtzeit, das von einer Community von Freiwilligen entwickelt wurde (die Hauptentwickler sind im Wildfire Games-Team vereint). Das Spiel ermöglicht es Ihnen, Zivilisationen zu kontrollieren, die in der Zeit von 500 v. Chr. Existierten. - 1 Jahr v. e. Ab Sommer 2018 ist das Projekt in Alpha-Version. [
Beschreibung aus Wikipedia].
Warum genau 0 n. Chr.?
Ich bat einen Kollegen von
Egor Bredikhin (
EgorBredikhin ), ein kleines offenes Projekt auszuwählen und für mich zu prüfen, mit dem ich zwischen anderen Aufgaben arbeiten konnte. Er schickte mir ein Protokoll für das 0 AD-Projekt. Auf die Frage: "Warum dieses Projekt?" - Es gab eine Antwort: "Ja, ich habe gerade dieses Spiel gespielt, eine gute Echtzeitstrategie." Ok, dann ist es 0 AD :).
Fehlerdichte
Ich möchte die Autoren von 0 AD für die gute Qualität des C ++ - Codes loben. Gut gemacht, schaffen Sie es selten, eine so geringe Fehlerdichte zu erreichen. Dies sind natürlich nicht alle Fehler, sondern diejenigen, die mit PVS-Studio erkannt werden können. Wie gesagt, obwohl PVS-Studio nicht alle Fehler findet, können wir dennoch sicher über die Beziehung zwischen der Fehlerdichte und der Qualität des Codes als Ganzes sprechen.
Ein paar Zahlen. Die Gesamtzahl der nicht leeren Codezeilen beträgt 231270. Davon sind 28,7% Kommentare. Insgesamt 165.000 Zeilen reinen C ++ - Codes.
Die Anzahl der vom Analysegerät ausgegebenen Nachrichten war gering, und nachdem ich sie alle angesehen hatte, schrieb ich 19 Fehler aus. Ich werde all diese Fehler später in diesem Artikel betrachten. Vielleicht habe ich etwas verpasst, weil der Fehler ein harmloser, schlampiger Code ist. Im Allgemeinen ändert dies jedoch nichts am Bild.
Also habe ich 19 Fehler in 165.000 Codezeilen gefunden. Wir berechnen die Fehlerdichte: 19 * 1000/165000 = 0,115.
Der Einfachheit halber runden wir ab und gehen davon aus, dass der PVS-Studio-Analysator 0,1 Fehler pro 1000 Codezeilen im Spielcode erkennt.
Tolles Ergebnis! Zum Vergleich habe ich in meinem letzten
Artikel über Android berechnet, dass ich mindestens 0,25 Fehler pro 1000 Codezeilen festgestellt habe. Tatsächlich ist die Fehlerdichte dort sogar noch größer. Ich habe einfach nicht die Kraft gefunden, den gesamten Bericht sorgfältig zu analysieren.
Oder nehmen Sie als Beispiel die EFL-Kernbibliotheken, die ich sorgfältig
untersucht und die Anzahl der Fehler berechnet habe. Darin erkennt PVS-Studio 0,71 Fehler pro 1000 Codezeilen.
Die Autoren von 0 AD sind also gut gemacht. Aus Gründen der Fairness sollte jedoch beachtet werden, dass ihnen die geringe Menge an in C ++ geschriebenem Code hilft. Leider wächst die Komplexität des Projekts umso schneller, je größer das Projekt ist, und die Fehlerdichte steigt nicht linear (
mehr ) an.
Fehler
Schauen wir uns nun die 19 Fehler an, die ich im Spiel gefunden habe. Zur Analyse des Projekts habe ich
PVS-Studio Version 6.24 verwendet. Ich schlage vor, Sie versuchen, die Demoversion herunterzuladen und die Projekte zu überprüfen, an denen Sie arbeiten.
Hinweis Wir positionieren PVS-Studio als B2B-Lösung. Für kleine Projekte und einzelne Entwickler haben wir eine kostenlose Lizenzoption: "
So nutzen Sie PVS-Studio kostenlos ."
Fehler N1Betrachten wir zunächst einen komplexen Fehler. In der Tat ist es nicht kompliziert, aber Sie müssen sich mit einem ziemlich großen Stück Code vertraut machen.
void WaterManager::CreateWaveMeshes() { .... int nbNeighb = 0; .... bool found = false; nbNeighb = 0; for (int p = 0; p < 8; ++p) { if (CoastalPointsSet.count(xx+around[p][0] + (yy + around[p][1])*SideSize)) { if (nbNeighb >= 2) { CoastalPointsSet.erase(xx + yy*SideSize); continue; } ++nbNeighb;
PVS-Studio-
Warnung :
V547 CWE-570 Ausdruck 'nbNeighb> = 2' ist immer falsch. WaterManager.cpp 581
Auf den ersten Blick erscheint die Meldung des Analysators seltsam. Warum ist die Bedingung
nbNeighb> = 2 immer falsch? In der Tat gibt es im Körper der Schleife ein Inkrement der Variablen
nbNeighb !
Wenn Sie unten nachsehen, sehen Sie eine
break- Anweisung, die die Ausführung der Schleife unterbricht. Wenn daher die Variable
nbNeighb erhöht wird, wird der Zyklus gestoppt. Somit erreicht der Wert der Variablen
nbNeighb niemals einen Wert größer als 1.
Der Code enthält eindeutig einen logischen Fehler.
Fehler N2 void CmpRallyPointRenderer::MergeVisibilitySegments( std::deque<SVisibilitySegment>& segments) { .... segments.erase(segments.end()); .... }
PVS-Studio Warnung:
V783 CWE-119 Es kann zu einer Dereferenzierung des ungültigen Iterators 'segment.end ()' kommen. CCmpRallyPointRenderer.cpp 1290
Sehr, sehr seltsamer Code. Vielleicht wollte der Programmierer das letzte Element aus dem Container entfernen. In diesem Fall sollte der Code folgendermaßen aussehen:
segments.erase(segments.end() - 1);
Obwohl es dann einfacher wäre zu schreiben:
segments.pop_back();
Um ehrlich zu sein, ist mir nicht ganz klar, was genau hier hätte geschrieben werden sollen.
Fehler N3, N4Ich habe beschlossen, zwei Fehler zusammen zu betrachten, da sie mit einem Ressourcenleck zusammenhängen und zunächst zeigen müssen, was das Makro
WARN_RETURN ist .
#define WARN_RETURN(status)\ do\ {\ DEBUG_WARN_ERR(status);\ return status;\ }\ while(0)
Wie Sie sehen können, bewirkt das Makro
WARN_RETURN, dass die Funktion den Body verlässt. Schauen wir uns nun ungenaue Möglichkeiten zur Verwendung dieses Makros an.
Das erste Fragment.
Status sys_generate_random_bytes(u8* buf, size_t count) { FILE* f = fopen("/dev/urandom", "rb"); if (!f) WARN_RETURN(ERR::FAIL); while (count) { size_t numread = fread(buf, 1, count, f); if (numread == 0) WARN_RETURN(ERR::FAIL); buf += numread; count -= numread; } fclose(f); return INFO::OK; }
PVS-Studio Warnung:
V773 CWE-401 Die Funktion wurde beendet, ohne das '
f' -
Handle loszulassen . Ein Ressourcenleck ist möglich. unix.cpp 332
Wenn die
Fread- Funktion die Daten nicht lesen kann, wird die Funktion
sys_generate_random_bytes beendet , ohne den Dateideskriptor freizugeben. In der Praxis ist dies kaum möglich. Es ist zweifelhaft, dass Sie keine Daten aus "/ dev / urandom" lesen können. Der Code ist jedoch schlampig.
Das zweite Fragment.
Status sys_cursor_create(....) { .... sys_cursor_impl* impl = new sys_cursor_impl; impl->image = image; impl->cursor = XcursorImageLoadCursor(wminfo.info.x11.display, image); if(impl->cursor == None) WARN_RETURN(ERR::FAIL); *cursor = static_cast<sys_cursor>(impl); return INFO::OK; }
PVS-Studio Warnung: V773 CWE-401 Die Funktion wurde beendet, ohne den 'impl'-Zeiger loszulassen. Ein Speicherverlust ist möglich. x.cpp 421
Wenn der Cursor nicht geladen werden kann, tritt ein Speicherverlust auf.
Fehler N5 Status LoadHeightmapImageOs(....) { .... shared_ptr<u8> fileData = shared_ptr<u8>(new u8[fileSize]); .... }
PVS-Studio Warnung:
V554 CWE-762 Falsche Verwendung von shared_ptr. Der mit 'new []' zugewiesene Speicher wird mit 'delete' bereinigt. MapIO.cpp 54
Die richtige Option:
shared_ptr<u8[]> fileData = shared_ptr<u8>(new u8[fileSize]);
Fehler N6 FUTrackedPtr(ObjectClass* _ptr = NULL) : ptr(_ptr) { if (ptr != NULL) FUTracker::TrackObject((FUTrackable*) ptr); ptr = ptr; }
PVS-Studio Warnung:
V570 Die Variable 'ptr' wird sich selbst zugewiesen. FUTracker.h 122
Fehler N7, N8 std::wstring TraceEntry::EncodeAsText() const { const wchar_t action = (wchar_t)m_action; wchar_t buf[1000]; swprintf_s(buf, ARRAY_SIZE(buf), L"%#010f: %c \"%ls\" %lu\n", m_timestamp, action, m_pathname.string().c_str(), (unsigned long)m_size); return buf; }
PVS-Studio Warnung:
V576 CWE-628 Falsches Format. Überprüfen Sie das fünfte tatsächliche Argument der Funktion 'swprintf_s'. Das Argument vom Typ char wird erwartet. trace.cpp 93
Hier stoßen wir auf eine verwirrte und undeutliche Geschichte einer alternativen Implementierung der
swprintf- Funktion in Visual C ++. Ich werde es nicht
nacherzählen , sondern mich auf die Dokumentation zur
V576- Diagnose beziehen (siehe Abschnitt "Breite Linien").
In diesem Fall funktioniert dieser Code höchstwahrscheinlich beim Kompilieren in Visual C ++ für Windows und beim Erstellen für Linux oder MacOS korrekt.
Ähnlicher Fehler: V576 CWE-628 Falsches Format. Überprüfen Sie das vierte tatsächliche Argument der Funktion 'swprintf_s'. Das Argument vom Typ char wird erwartet. vfs_tree.cpp 211
Fehler N9, N10, N11Klassisch Zu Beginn wird der Zeiger verwendet und erst dann überprüft.
static void TEST_CAT2(char* dst, ....) { strcpy(dst, dst_val);
PVS-Studio Warnung:
V595 CWE-476 Der Zeiger 'dst' wurde verwendet, bevor er gegen nullptr verifiziert wurde. Überprüfen Sie die Zeilen: 140, 143. test_secure_crt.h 140
Ich denke, der Fehler bedarf keiner Erklärung. Ähnliche Warnungen:
- V595 CWE-476 Der Zeiger 'dst' wurde verwendet, bevor er gegen nullptr verifiziert wurde. Überprüfen Sie die Zeilen: 150, 153. test_secure_crt.h 150
- V595 CWE-476 Der Zeiger 'dst' wurde verwendet, bevor er gegen nullptr verifiziert wurde. Überprüfen Sie die Zeilen: 314, 317. test_secure_crt.h 314
Fehler N12 typedef int tbool; void MikkTSpace::setTSpace(...., const tbool bIsOrientationPreserving, ....) { .... m_NewVertices.push_back(bIsOrientationPreserving > 0.5 ? 1.0f : (-1.0f)); .... }
V674 CWE-682 Das Literal '0.5' vom Typ 'double' wird mit einem Wert vom Typ 'int' verglichen. Überprüfen Sie den Ausdruck 'bIsOrientationPreserving> 0.5'. MikktspaceWrap.cpp 137
Es macht keinen Sinn, eine Variable vom Typ
int mit einer Konstanten von 0,5 zu vergleichen. Darüber hinaus ist dies in Bezug auf die Bedeutung im Allgemeinen eine Variable vom Typ Boolesch, was bedeutet, dass ein Vergleich mit 0,5 sehr seltsam aussieht. Angenommen, anstelle von
bIsOrientationPreserving sollte hier
eine andere Variable verwendet werden.
Fehler N13 virtual Status ReplaceFile(const VfsPath& pathname, const shared_ptr<u8>& fileContents, size_t size) { ScopedLock s; VfsDirectory* directory; VfsFile* file; Status st; st = vfs_Lookup(pathname, &m_rootDirectory, directory, &file, VFS_LOOKUP_ADD|VFS_LOOKUP_CREATE);
PVS-Studio Warnung:
V749 CWE-675 Der Destruktor des 's'-Objekts wird nach Verlassen des Objektbereichs ein zweites Mal aufgerufen. vfs.cpp 165
Vor dem Erstellen einer Datei muss ein Objekt vom Typ
ScopedLock ein Objekt „entsperren“. Hierzu wird explizit ein Destruktor aufgerufen. Das Problem ist, dass der Destruktor für das
s- Objekt beim Beenden der Funktion automatisch wieder aufgerufen wird. Das heißt, Der Destruktor wird zweimal aufgerufen. Ich habe das Gerät der
ScopedLock- Klasse nicht studiert, aber Sie sollten dies auf keinen Fall tun. Oft führt ein solcher Doppelaufruf an den Destruktor zu undefiniertem Verhalten oder anderen unangenehmen Fehlern. Selbst wenn der Code jetzt
einwandfrei funktioniert, ist es sehr einfach, alles zu
beschädigen, indem Sie die Implementierung der
ScopedLock- Klasse
ändern .
Fehler N14, N15, N16, N17 CFsmEvent* CFsm::AddEvent( unsigned int eventType ) { .... pEvent = new CFsmEvent( eventType ); if ( !pEvent ) return NULL; .... }
PVS-Studio Warnung:
V668 CWE-570 Es macht keinen Sinn, den Zeiger 'pEvent' gegen Null zu testen, da der Speicher mit dem Operator 'new' zugewiesen wurde. Die Ausnahme wird bei einem Speicherzuordnungsfehler generiert. fsm.cpp 259
Das Überprüfen des Zeigers ist nicht sinnvoll, da im Falle eines Speicherzuordnungsfehlers eine Ausnahme
std :: bad_alloc ausgelöst wird .
Die Prüfung ist also überflüssig, aber der Fehler ist nicht schwerwiegend. Alles ist jedoch viel schlimmer, wenn eine Logik im Hauptteil der
if-Anweisung ausgeführt wird . Betrachten Sie den folgenden Fall:
CFsmTransition* CFsm::AddTransition(....) { .... CFsmEvent* pEvent = AddEvent( eventType ); if ( !pEvent ) return NULL;
Analyzer-Warnung: V668 CWE-570 Es macht keinen Sinn, den Zeiger 'pNewTransition' gegen Null zu testen, da der Speicher mit dem Operator 'new' zugewiesen wurde. Die Ausnahme wird bei einem Speicherzuordnungsfehler generiert. fsm.cpp 289
Es wird versucht, Speicher
freizugeben, dessen Adresse im
pEvent- Zeiger gespeichert ist. Dies wird natürlich nicht passieren und ein Speicherverlust wird auftreten.
Als ich anfing, mich mit diesem Code zu beschäftigen, stellte sich heraus, dass alles komplizierter war und es vielleicht nicht einen Fehler gab, sondern zwei. Jetzt werde ich erklären, was mit diesem Code falsch ist. Dazu müssen wir uns mit dem
AddEvent- Funktionsgerät vertraut machen.
CFsmEvent* CFsm::AddEvent( unsigned int eventType ) { CFsmEvent* pEvent = NULL;
Beachten Sie, dass die Funktion nicht immer einen Zeiger auf ein neues Objekt zurückgibt, das mit dem
neuen Operator erstellt wurde. Manchmal wird ein vorhandenes Objekt aus dem Container
m_Events übernommen . Der Zeiger auf das neu erstellte Objekt wird übrigens auch in
m_Events platziert.
Und hier stellt sich die Frage:
Wem gehören die Objekte, deren Zeiger im Container
m_Events gespeichert sind, und welche sollen sie zerstören? Ich bin mit dem Projekt nicht vertraut, aber höchstwahrscheinlich gibt es irgendwo Code, der all diese Objekte zerstört. Das Löschen eines Objekts in der
Funktion CFsm :: AddTransition ist dann im Allgemeinen überflüssig.
Ich hatte den Eindruck, dass Sie einfach das folgende Codefragment löschen können:
if ( !pNewTransition ) { delete pEvent; return NULL; }
Andere Fehler:
- V668 CWE-571 Es macht keinen Sinn, den Zeiger 'ret' gegen null zu testen, da der Speicher mit dem Operator 'new' zugewiesen wurde. Die Ausnahme wird bei einem Speicherzuordnungsfehler generiert. TerrainTextureEntry.cpp 120
- V668 CWE-571 Es macht keinen Sinn, den 'Antwort'-Zeiger gegen Null zu testen, da der Speicher mit dem' neuen 'Operator zugewiesen wurde. Die Ausnahme wird bei einem Speicherzuordnungsfehler generiert. SoundManager.cpp 542
Fehler N18, N19 static void dir_scan_callback(struct de *de, void *data) { struct dir_scan_data *dsd = (struct dir_scan_data *) data; if (dsd->entries == NULL || dsd->num_entries >= dsd->arr_size) { dsd->arr_size *= 2; dsd->entries = (struct de *) realloc(dsd->entries, dsd->arr_size * sizeof(dsd->entries[0])); } if (dsd->entries == NULL) {
PVS-Studio-
Warnung :
V701 CWE-401 realloc () mögliches Leck: Wenn realloc () beim Zuweisen von Speicher fehlschlägt, geht der ursprüngliche Zeiger 'dsd-> entry' verloren. Ziehen Sie in Betracht, einem temporären Zeiger realloc () zuzuweisen. mongoose.cpp 2462
Wenn die Größe des Arrays nicht mehr ausreicht, wird der Speicher mithilfe der
Realloc- Funktion zugewiesen. Der Fehler besteht darin, dass der Wert des Zeigers auf den ursprünglichen Speicherblock sofort mit dem neuen Wert überschrieben wird, der von der
Realloc- Funktion zurückgegeben wird.
Wenn die Speicherzuordnung fehlschlägt, gibt die
Realloc- Funktion NULL zurück, und diese NULL wird in die Variable
dsd-> entry geschrieben. Danach ist es unmöglich, den Speicherblock
freizugeben, dessen Adresse zuvor in
dsd-> Einträgen gespeichert wurde. Ein Speicherverlust tritt auf.
Ein weiterer Fehler: V701 CWE-401 realloc () mögliches Leck: Wenn realloc () beim Zuweisen von Speicher fehlschlägt, geht der ursprüngliche Zeiger 'Buffer' verloren. Ziehen Sie in Betracht, einem temporären Zeiger realloc () zuzuweisen. Preprocessor.cpp 84
Fazit
Ich kann nicht sagen, dass sich der Artikel diesmal als faszinierend herausgestellt hat oder dass ich viele schreckliche Fehler gezeigt habe. Was zu tun ist, ist nicht erforderlich.
Was ich sehe, dann schreibe ich .
Vielen Dank für Ihre Aufmerksamkeit. Zur Abwechslung
beende ich den Artikel mit einer Einladung, mir auf Instagram
@pvs_studio_unicorn und auf Twitter
@Code_Analysis zu folgen.
Wenn Sie diesen Artikel einem englischsprachigen Publikum zugänglich machen möchten, verwenden Sie bitte den Link zur Übersetzung: Andrey Karpov.
Gute Arbeit, Autoren des Spiels 0 AD!