Geekly articles weekly

CMake: Der Fall, in dem die Qualität des Projekts unverzeihlich ist

Bild 1

CMake ist ein plattformübergreifendes System zur Automatisierung von Projekterstellungen. Dieses System ist viel älter als der statische Code-Analysator PVS-Studio, aber niemand hat versucht, den Analysator auf seinen Code anzuwenden und die Fehler zu überprüfen. Wie sich herausstellte, gibt es viele von ihnen. Das CMake-Publikum ist riesig. Neue Projekte beginnen damit und alte werden portiert. Ich schaudere, wenn ich daran denke, wie viele Entwickler einen bestimmten Fehler hätten haben können.

Einführung


CMake ist ein plattformübergreifendes System zur Automatisierung der Softwareerstellung aus dem Quellcode. CMake ist nicht direkt zum Erstellen gedacht, sondern generiert nur Dateien zur Steuerung eines Builds aus CMakeLists.txt-Dateien. Die erste Veröffentlichung des Programms fand im Jahr 2000 statt. Zum Vergleich: Der PVS-Studio-Analysator erschien erst im Jahr 2008. Zu diesem Zeitpunkt war er auf die Suche nach Fehlern ausgerichtet, die durch die Portierung von 32-Bit-Systemen auf 64-Bit-Systeme entstanden waren. Im Jahr 2010 erschien der erste Satz von Allzweckdiagnosen (V501- V545 ). Übrigens enthält der CMake-Code einige Warnungen aus diesem ersten Satz.

Unverzeihliche Fehler


V1040 Möglicher Tippfehler bei der Schreibweise eines vordefinierten Makronamens . Das Makro '__MINGW32_' ähnelt '__MINGW32__'. winapi.h 4112

/* from winternl.h */ #if !defined(__UNICODE_STRING_DEFINED) && defined(__MINGW32_) #define __UNICODE_STRING_DEFINED #endif

Die V1040- Diagnose wurde vor nicht allzu langer Zeit implementiert. Zum Zeitpunkt der Veröffentlichung des Artikels wird er höchstwahrscheinlich noch nicht veröffentlicht. Dennoch haben wir mit seiner Hilfe bereits einen coolen Fehler gefunden.

Der Name __MINGW32_ enthält einen Tippfehler. Am Ende fehlt ein Unterstreichungszeichen. Wenn Sie den Code mit diesem Namen durchsuchen, sehen Sie, dass im Projekt die Version mit zwei Unterstreichungszeichen auf beiden Seiten verwendet wird:

Bild 3

V531 Es ist seltsam, dass ein Operator sizeof () mit sizeof () multipliziert wird. cmGlobalVisualStudioGenerator.cxx 558

 bool IsVisualStudioMacrosFileRegistered(const std::string& macrosFile, const std::string& regKeyBase, std::string& nextAvailableSubKeyName) { .... if (ERROR_SUCCESS == result) { wchar_t subkeyname[256]; // <= DWORD cch_subkeyname = sizeof(subkeyname) * sizeof(subkeyname[0]); // <= wchar_t keyclass[256]; DWORD cch_keyclass = sizeof(keyclass) * sizeof(keyclass[0]); FILETIME lastWriteTime; lastWriteTime.dwHighDateTime = 0; lastWriteTime.dwLowDateTime = 0; while (ERROR_SUCCESS == RegEnumKeyExW(hkey, index, subkeyname, &cch_subkeyname, 0, keyclass, &cch_keyclass, &lastWriteTime)) { .... } .... }

Für ein statisch deklariertes Array berechnet der Operator sizeof die Größe in Byte unter Berücksichtigung der Anzahl der Elemente und ihrer Größe. Bei der Auswertung des Werts der Variablen cch_subkeyname hat ein Entwickler dies nicht berücksichtigt und einen viermal höheren Wert als beabsichtigt erhalten. Lassen Sie uns erklären, woher "viermal" kommt.

Das Array und seine falsche Größe werden an die Funktion RegEnumKeyExW übergeben :

 LSTATUS RegEnumKeyExW( HKEY hKey, DWORD dwIndex, LPWSTR lpName, // <= subkeyname LPDWORD lpcchName, // <= cch_subkeyname LPDWORD lpReserved, LPWSTR lpClass, LPDWORD lpcchClass, PFILETIME lpftLastWriteTime );

Der Zeiger lpcchName muss auf die Variable zeigen, die die Puffergröße in Zeichen enthält: "Ein Zeiger auf eine Variable, die die Größe des durch den Parameter lpClass angegebenen Puffers in Zeichen angibt." Die Größe des Subkeyname- Arrays beträgt 512 Byte und kann 256 Zeichen vom Typ wchar_t speichern (in Windows beträgt wchar_t 2 Byte). Es ist 256, die an die Funktion übergeben werden soll. Stattdessen wird 512 mit 2 multipliziert und wir erhalten 1024.

Ich denke, es ist jetzt klar, wie dieser Fehler behoben werden kann. Sie müssen Division statt Multiplikation verwenden:

 DWORD cch_subkeyname = sizeof(subkeyname) / sizeof(subkeyname[0]);

Übrigens tritt der gleiche Fehler bei der Auswertung des Wertes der Variablen cch_keyclass auf .

Der beschriebene Fehler kann möglicherweise zu einem Pufferüberlauf führen. Alle diese Fragmente müssen definitiv korrigiert werden:

  • V531 Es ist seltsam, dass ein Operator sizeof () mit sizeof () multipliziert wird. cmGlobalVisualStudioGenerator.cxx 556
  • V531 Es ist seltsam, dass ein Operator sizeof () mit sizeof () multipliziert wird. cmGlobalVisualStudioGenerator.cxx 572
  • V531 Es ist seltsam, dass ein Operator sizeof () mit sizeof () multipliziert wird. cmGlobalVisualStudioGenerator.cxx 621
  • V531 Es ist seltsam, dass ein Operator sizeof () mit sizeof () multipliziert wird. cmGlobalVisualStudioGenerator.cxx 622
  • V531 Es ist seltsam, dass ein Operator sizeof () mit sizeof () multipliziert wird. cmGlobalVisualStudioGenerator.cxx 649

V595 Der Zeiger 'this-> BuildFileStream' wurde verwendet, bevor er gegen nullptr verifiziert wurde. Überprüfen Sie die Zeilen: 133, 134. cmMakefileTargetGenerator.cxx 133

 void cmMakefileTargetGenerator::CreateRuleFile() { .... this->BuildFileStream->SetCopyIfDifferent(true); if (!this->BuildFileStream) { return; } .... }

Der Zeiger this-> BuildFileStream wird unmittelbar vor der Überprüfung auf seine Gültigkeit dereferenziert. Hat das niemandem Probleme bereitet? Unten finden Sie ein weiteres Beispiel für ein solches Snippet. Es ist wie eine Kopie gemacht. Tatsächlich gibt es jedoch viele V595- Warnungen, von denen die meisten nicht so offensichtlich sind. Aus meiner Erfahrung kann ich sagen, dass das Korrigieren von Warnungen dieser Diagnose am längsten dauert.

  • V595 Der Zeiger 'this-> FlagFileStream' wurde verwendet, bevor er gegen nullptr verifiziert wurde. Überprüfen Sie die Zeilen: 303, 304. cmMakefileTargetGenerator.cxx 303

V614 Nicht initialisierter Zeiger 'str' verwendet. cmVSSetupHelper.h 80

 class SmartBSTR { public: SmartBSTR() { str = NULL; } SmartBSTR(const SmartBSTR& src) { if (src.str != NULL) { str = ::SysAllocStringByteLen((char*)str, ::SysStringByteLen(str)); } else { str = ::SysAllocStringByteLen(NULL, 0); } } .... private: BSTR str; };

Der Analysator hat die Verwendung des nicht initialisierten Str- Zeigers festgestellt. Es erschien aufgrund eines gewöhnlichen Tippfehlers. Beim Aufrufen der Funktion SysAllocStringByteLen sollte der Zeiger src.str verwendet werden .

V557 Array-Überlauf ist möglich. Der Wert des 'Lensymbol'-Index könnte 28 erreichen. Archive_read_support_format_rar.c 2749

 static int64_t expand(struct archive_read *a, int64_t end) { .... if ((lensymbol = read_next_symbol(a, &rar->lengthcode)) < 0) goto bad_data; if (lensymbol > (int)(sizeof(lengthbases)/sizeof(lengthbases[0]))) goto bad_data; if (lensymbol > (int)(sizeof(lengthbits)/sizeof(lengthbits[0]))) goto bad_data; len = lengthbases[lensymbol] + 2; if (lengthbits[lensymbol] > 0) { if (!rar_br_read_ahead(a, br, lengthbits[lensymbol])) goto truncated_data; len += rar_br_bits(br, lengthbits[lensymbol]); rar_br_consume(br, lengthbits[lensymbol]); } .... }

Dieser Code verbirgt mehrere Probleme gleichzeitig. Beim Zugriff auf Längenbasis- und Längenbit- Arrays kann ein Array-Index außerhalb der Grenzen liegen, da Entwickler den Operator '>' anstelle von '> =' oben geschrieben haben. Bei dieser Überprüfung wurde ein inakzeptabler Wert übersehen. Hier haben wir nichts als ein klassisches Fehlermuster namens Off-by-One-Fehler .

Hier ist die gesamte Liste der Array-Zugriffsvorgänge anhand eines ungültigen Index:

  • V557 Array-Überlauf ist möglich. Der Wert des 'Lensymbol'-Index könnte 28 erreichen. Archive_read_support_format_rar.c 2750
  • V557 Array-Überlauf ist möglich. Der Wert des 'Lensymbol'-Index könnte 28 erreichen. Archive_read_support_format_rar.c 2751
  • V557 Array-Überlauf ist möglich. Der Wert des 'Lensymbol'-Index könnte 28 erreichen. Archive_read_support_format_rar.c 2753
  • V557 Array-Überlauf ist möglich. Der Wert des 'Lensymbol'-Index könnte 28 erreichen. Archive_read_support_format_rar.c 2754
  • V557 Array-Überlauf ist möglich. Der Wert des 'offssymbol'-Index könnte 60 erreichen. Archive_read_support_format_rar.c 2797

Speicherverlust


V773 Die Funktion wurde beendet, ohne den Zeiger 'testRun' loszulassen. Ein Speicherverlust ist möglich. cmCTestMultiProcessHandler.cxx 193

 void cmCTestMultiProcessHandler::FinishTestProcess(cmCTestRunTest* runner, bool started) { .... delete runner; if (started) { this->StartNextTests(); } } bool cmCTestMultiProcessHandler::StartTestProcess(int test) { .... cmCTestRunTest* testRun = new cmCTestRunTest(*this); // <= .... if (testRun->StartTest(this->Completed, this->Total)) { return true; // <= } } this->FinishTestProcess(testRun, false); // <= return false; }

Der Analysator hat einen Speicherverlust festgestellt. Der Speicher des testRun- Zeigers wird nicht freigegeben, wenn die Funktion testRun-> StartTest true zurückgibt. Wenn Sie einen anderen Code-Zweig ausführen, wird dieser Speicher in der Funktion this-> FinishTestProcess freigegeben.

Ressourcenleck


V773 Die Funktion wurde beendet, ohne die Datei zu schließen, auf die das Handle 'fd' verweist. Ein Ressourcenleck ist möglich. rhash.c 450

 RHASH_API int rhash_file(....) { FILE* fd; rhash ctx; int res; hash_id &= RHASH_ALL_HASHES; if (hash_id == 0) { errno = EINVAL; return -1; } if ((fd = fopen(filepath, "rb")) == NULL) return -1; if ((ctx = rhash_init(hash_id)) == NULL) return -1; // <= fclose(fd); ??? res = rhash_file_update(ctx, fd); fclose(fd); rhash_final(ctx, result); rhash_free(ctx); return res; }

Seltsame Logik unter Bedingungen


V590 Überprüfen Sie den Ausdruck '* s! =' \ 0 '&& * s ==' ''. Der Ausdruck ist übertrieben oder enthält einen Druckfehler. archive_cmdline.c 76

 static ssize_t get_argument(struct archive_string *as, const char *p) { const char *s = p; archive_string_empty(as); /* Skip beginning space characters. */ while (*s != '\0' && *s == ' ') s++; .... }

* s Zeichenvergleich mit null ist redundant. Der Zustand der while- Schleife hängt nur davon ab, ob das Zeichen einem Leerzeichen entspricht oder nicht. Dies ist kein Fehler, sondern eine unnötige Komplikation des Codes.

V592 Der Ausdruck wurde zweimal in Klammern gesetzt: ((Ausdruck)). Ein Klammerpaar ist nicht erforderlich oder es liegt ein Druckfehler vor. cmCTestTestHandler.cxx 899

 void cmCTestTestHandler::ComputeTestListForRerunFailed() { this->ExpandTestsToRunInformationForRerunFailed(); ListOfTests finalList; int cnt = 0; for (cmCTestTestProperties& tp : this->TestList) { cnt++; // if this test is not in our list of tests to run, then skip it. if ((!this->TestsToRun.empty() && std::find(this->TestsToRun.begin(), this->TestsToRun.end(), cnt) == this->TestsToRun.end())) { continue; } tp.Index = cnt; finalList.push_back(tp); } .... }

Der Analysator warnt davor, dass der Negationsvorgang wahrscheinlich aus den Klammern genommen werden sollte. Es scheint, dass es hier keinen solchen Fehler gibt - nur unnötige doppelte Klammern. Höchstwahrscheinlich liegt jedoch ein logischer Fehler im Code vor.

Der Operator continue wird nur ausgeführt, wenn die Liste der Tests this-> TestsToRun nicht leer ist und cnt nicht vorhanden ist. Es ist davon auszugehen, dass bei leerer Testliste dieselbe Aktion ausgeführt werden muss. Höchstwahrscheinlich sollte der Zustand wie folgt sein:

 if (this->TestsToRun.empty() || std::find(this->TestsToRun.begin(), this->TestsToRun.end(), cnt) == this->TestsToRun.end()) { continue; }

V592 Der Ausdruck wurde zweimal in Klammern gesetzt: ((Ausdruck)). Ein Klammerpaar ist nicht erforderlich oder es liegt ein Druckfehler vor. cmMessageCommand.cxx 73

 bool cmMessageCommand::InitialPass(std::vector<std::string> const& args, cmExecutionStatus&) { .... } else if (*i == "DEPRECATION") { if (this->Makefile->IsOn("CMAKE_ERROR_DEPRECATED")) { fatal = true; type = MessageType::DEPRECATION_ERROR; level = cmake::LogLevel::LOG_ERROR; } else if ((!this->Makefile->IsSet("CMAKE_WARN_DEPRECATED") || this->Makefile->IsOn("CMAKE_WARN_DEPRECATED"))) { type = MessageType::DEPRECATION_WARNING; level = cmake::LogLevel::LOG_WARNING; } else { return true; } ++i; } .... }

Es ist ein ähnliches Beispiel, aber dieses Mal bin ich sicherer, dass ein Fehler auftritt. Die Funktion IsSet ("CMAKE_WARN_DEPRECATED") prüft, ob der Wert CMAKE_WARN_DEPRECATED global gesetzt ist, und die Funktion IsOn ("CMAKE_WARN_DEPRECATED) prüft, ob der Wert in der Projektkonfiguration gesetzt ist. Höchstwahrscheinlich ist der komplementäre Operator redundant, da es in beiden Fällen richtig ist, die gleichen Werte für Typ und Ebene festzulegen.

V728 Eine übermäßige Überprüfung kann vereinfacht werden. Das '(A &&! B) || (! A && B) 'Ausdruck entspricht dem Ausdruck' bool (A)! = Bool (B) '. cmCTestRunTest.cxx 151

 bool cmCTestRunTest::EndTest(size_t completed, size_t total, bool started) { .... } else if ((success && !this->TestProperties->WillFail) || (!success && this->TestProperties->WillFail)) { this->TestResult.Status = cmCTestTestHandler::COMPLETED; outputStream << " Passed "; } .... }

Dieser Code kann einfacher sein. Man kann den bedingten Ausdruck folgendermaßen umschreiben:

 } else if (success != this->TestProperties->WillFail) { this->TestResult.Status = cmCTestTestHandler::COMPLETED; outputStream << " Passed "; }

Noch ein paar Orte zur Vereinfachung:

  • V728 Eine übermäßige Überprüfung kann vereinfacht werden. Die '(A && B) || (! A &&! B) 'Ausdruck entspricht dem Ausdruck' bool (A) == bool (B) '. cmCTestTestHandler.cxx 702
  • V728 Eine übermäßige Überprüfung kann vereinfacht werden. Das '(A &&! B) || (! A && B) 'Ausdruck entspricht dem Ausdruck' bool (A)! = Bool (B) '. Digest_sspi.c 443
  • V728 Eine übermäßige Überprüfung kann vereinfacht werden. Das '(A &&! B) || (! A && B) 'Ausdruck entspricht dem Ausdruck' bool (A)! = Bool (B) '. tcp.c 1295
  • V728 Eine übermäßige Überprüfung kann vereinfacht werden. Das '(A &&! B) || (! A && B) 'Ausdruck entspricht dem Ausdruck' bool (A)! = Bool (B) '. testDynamicLoader.cxx 58
  • V728 Eine übermäßige Überprüfung kann vereinfacht werden. Das '(A &&! B) || (! A && B) 'Ausdruck entspricht dem Ausdruck' bool (A)! = Bool (B) '. testDynamicLoader.cxx 65
  • V728 Eine übermäßige Überprüfung kann vereinfacht werden. Das '(A &&! B) || (! A && B) 'Ausdruck entspricht dem Ausdruck' bool (A)! = Bool (B) '. testDynamicLoader.cxx 72

Verschiedene Warnungen


V523 Die Anweisung 'then' entspricht dem nachfolgenden Codefragment. archive_read_support_format_ar.c 415

 static int _ar_read_header(struct archive_read *a, struct archive_entry *entry, struct ar *ar, const char *h, size_t *unconsumed) { .... /* * "__.SYMDEF" is a BSD archive symbol table. */ if (strcmp(filename, "__.SYMDEF") == 0) { archive_entry_copy_pathname(entry, filename); /* Parse the time, owner, mode, size fields. */ return (ar_parse_common_header(ar, entry, h)); } /* * Otherwise, this is a standard entry. The filename * has already been trimmed as much as possible, based * on our current knowledge of the format. */ archive_entry_copy_pathname(entry, filename); return (ar_parse_common_header(ar, entry, h)); }

Der Ausdruck in der letzten Bedingung ähnelt den letzten beiden Zeilen der Funktion. Ein Entwickler kann diesen Code vereinfachen, indem er die Bedingung entfernt, oder es liegt ein Fehler im Code vor, der behoben werden sollte.

V535 Die Variable 'i' wird für diese Schleife und für die äußere Schleife verwendet. Überprüfen Sie die Zeilen: 2220, 2241. multi.c 2241

 static CURLMcode singlesocket(struct Curl_multi *multi, struct Curl_easy *data) { .... for(i = 0; (i< MAX_SOCKSPEREASYHANDLE) && // <= (curraction & (GETSOCK_READSOCK(i) | GETSOCK_WRITESOCK(i))); i++) { unsigned int action = CURL_POLL_NONE; unsigned int prevaction = 0; unsigned int comboaction; bool sincebefore = FALSE; s = socks[i]; /* get it from the hash */ entry = sh_getentry(&multi->sockhash, s); if(curraction & GETSOCK_READSOCK(i)) action |= CURL_POLL_IN; if(curraction & GETSOCK_WRITESOCK(i)) action |= CURL_POLL_OUT; actions[i] = action; if(entry) { /* check if new for this transfer */ for(i = 0; i< data->numsocks; i++) { // <= if(s == data->sockets[i]) { prevaction = data->actions[i]; sincebefore = TRUE; break; } } } .... }

Die Variable i wird als Schleifenzähler in der äußeren und inneren Schleife verwendet. Gleichzeitig beginnt der Wert des Zählers in der inneren Schleife wieder bei Null. Es ist hier vielleicht kein Fehler, aber der Code ist verdächtig.

V519 Der Variablen 'tagString' werden zweimal hintereinander Werte zugewiesen. Vielleicht ist das ein Fehler. Überprüfen Sie die Zeilen: 84, 86. cmCPackLog.cxx 86

 void cmCPackLog::Log(int tag, const char* file, int line, const char* msg, size_t length) { .... if (tag & LOG_OUTPUT) { output = true; display = true; if (needTagString) { if (!tagString.empty()) { tagString += ","; } tagString = "VERBOSE"; } } if (tag & LOG_WARNING) { warning = true; display = true; if (needTagString) { if (!tagString.empty()) { tagString += ","; } tagString = "WARNING"; } } .... }

Die Variable tagString wird an allen Stellen mit einem neuen Wert überschrieben. Es ist schwer zu sagen, was das Problem ist oder warum sie es getan haben. Vielleicht waren die Operatoren '=' und '+ =' durcheinander.

Die gesamte Liste solcher Orte:

  • V519 Der Variablen 'tagString' werden zweimal hintereinander Werte zugewiesen. Vielleicht ist das ein Fehler. Überprüfen Sie die Zeilen: 94, 96. cmCPackLog.cxx 96
  • V519 Der Variablen 'tagString' werden zweimal hintereinander Werte zugewiesen. Vielleicht ist das ein Fehler. Überprüfen Sie die Zeilen: 104, 106. cmCPackLog.cxx 106
  • V519 Der Variablen 'tagString' werden zweimal hintereinander Werte zugewiesen. Vielleicht ist das ein Fehler. Überprüfen Sie die Zeilen: 114, 116. cmCPackLog.cxx 116
  • V519 Der Variablen 'tagString' werden zweimal hintereinander Werte zugewiesen. Vielleicht ist das ein Fehler. Überprüfen Sie die Zeilen: 125, 127. cmCPackLog.cxx 127

V519 Der Variablen 'aes-> aes_set' werden zweimal hintereinander Werte zugewiesen. Vielleicht ist das ein Fehler. Überprüfen Sie die Zeilen: 4052, 4054. archive_string.c 4054

 int archive_mstring_copy_utf8(struct archive_mstring *aes, const char *utf8) { if (utf8 == NULL) { aes->aes_set = 0; // <= } aes->aes_set = AES_SET_UTF8; // <= .... return (int)strlen(utf8); }

Die erzwungene Einstellung des Werts AES_SET_UTF8 sieht verdächtig aus. Ich denke, ein solcher Code wird jeden Entwickler verwirren, der dieses Fragment verfeinert.

Dieser Code wurde an einen anderen Ort kopiert:

  • V519 Der Variablen 'aes-> aes_set' werden zweimal hintereinander Werte zugewiesen. Vielleicht ist das ein Fehler. Überprüfen Sie die Zeilen: 4066, 4068. archive_string.c 4068

So finden Sie Fehler in einem Projekt auf CMake


In diesem Abschnitt werde ich Ihnen kurz erklären, wie Sie CMake-Projekte mit PVS-Studio so einfach wie eins, zwei, drei überprüfen können.

Windows / Visual Studio

Für Visual Studio können Sie eine Projektdatei mithilfe der CMake-GUI oder des folgenden Befehls generieren:

 cmake -G "Visual Studio 15 2017 Win64" ..

Als Nächstes können Sie die SLN-Datei öffnen und das Projekt mithilfe des Plugins für Visual Studio überprüfen.

Linux / MacOS

Die Datei compile_commands.json wird für Überprüfungen auf diesen Systemen verwendet. Übrigens kann es in verschiedenen Build-Systemen generiert werden. So machen Sie es in CMake:

 cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=On ..

Als letztes müssen Sie den Analysator im Verzeichnis mit der .json-Datei ausführen:

 pvs-studio-analyzer analyze -l /path/to/PVS-Studio.lic -o /path/to/project.log -e /path/to/exclude-path -j<N>

Wir haben auch ein Modul für CMake-Projekte entwickelt. Manche Leute benutzen es gerne. Das CMake-Modul und Beispiele für seine Verwendung finden Sie in unserem Repository auf GitHub: pvs-studio-cmake-examples .

Fazit


Ein großes Publikum von CMake-Benutzern ist großartig, um das Projekt zu testen, aber viele Probleme könnten vor der Veröffentlichung durch die Verwendung statischer Code-Analyse-Tools wie PVS-Studio verhindert werden .

Wenn Ihnen die Analyseergebnisse gefallen haben, Ihr Projekt jedoch nicht in C und C ++ geschrieben ist, möchte ich Sie daran erinnern, dass der Analysator auch die Analyse von Projekten in C # und Java unterstützt. Sie können den Analysator in Ihrem Projekt testen, indem Sie auf diese Seite gehen .

Source: https://habr.com/ru/post/de464145/

More articles:


All Articles