Geekly articles weekly

CMake: le cas où la qualité du projet est impardonnable

Image 1

CMake est un système multiplateforme pour automatiser les builds de projet. Ce système est beaucoup plus ancien que l'analyseur de code statique PVS-Studio, mais personne n'a essayé d'appliquer l'analyseur sur son code et de revoir les erreurs. En fait, il y en a beaucoup. Le public de CMake est énorme. De nouveaux projets y démarrent et les anciens sont portés. Je frémis en pensant au nombre de développeurs qui auraient pu avoir une erreur donnée.

Présentation


CMake est un système multiplateforme pour automatiser la création de logiciels à partir du code source. CMake n'est pas destiné directement à la construction, il génère uniquement des fichiers pour contrôler une construction à partir des fichiers CMakeLists.txt. La première version du programme a eu lieu en 2000. À titre de comparaison, l'analyseur PVS-Studio n'est apparu qu'en 2008. À cette époque, il visait à rechercher les bogues résultant du portage de systèmes 32 bits vers des systèmes 64 bits. En 2010, le premier ensemble de diagnostics à usage général est apparu (V501- V545 ). Par ailleurs, le code CMake a quelques avertissements de ce premier ensemble.

Erreurs impardonnables


V1040 Typo possible dans l'orthographe d'un nom de macro prédéfini. La macro '__MINGW32_' est similaire à '__MINGW32__'. winapi.h 4112

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

Le diagnostic V1040 a été implanté il n'y a pas si longtemps. Très probablement, au moment de publier l'article, il ne sera pas encore publié, néanmoins, nous avons déjà trouvé une erreur sympa avec son aide.

Il y a une faute de frappe faite dans le nom __MINGW32_ . À la fin, un caractère de soulignement est manquant. Si vous recherchez le code avec ce nom, vous pouvez voir que la version avec deux caractères de soulignement des deux côtés est utilisée dans le projet:

Image 3

V531 Il est étrange qu'un opérateur sizeof () soit multiplié par sizeof (). 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)) { .... } .... }

Pour un tableau statiquement déclaré, l'opérateur sizeof calculera la taille en octets, en tenant compte du nombre d'éléments et de leur taille. Lors de l'évaluation de la valeur de la variable cch_subkeyname , un développeur ne l'a pas prise en compte et a obtenu une valeur 4 fois supérieure à celle prévue. Expliquons d'où viennent "quatre fois".

Le tableau et sa taille incorrecte sont passés à la fonction RegEnumKeyExW :

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

Le pointeur lpcchName doit pointer vers la variable, contenant la taille du tampon en caractères: "Un pointeur vers une variable qui spécifie la taille du tampon spécifié par le paramètre lpClass , en caractères". La taille du tableau de sous- clés est de 512 octets et peut stocker 256 caractères du type wchar_t (sous Windows, wchar_t est de 2 octets). C'est 256 qui doit être passé à la fonction. Au lieu de cela, 512 est multiplié par 2 et nous obtenons 1024.

Je pense, il est clair maintenant comment corriger cette erreur. Vous devez utiliser la division au lieu de la multiplication:

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

Par ailleurs, la même erreur se produit lors de l'évaluation de la valeur de la variable cch_keyclass .

L'erreur décrite peut potentiellement entraîner un débordement de tampon. Tous ces fragments doivent définitivement être corrigés:

  • V531 Il est étrange qu'un opérateur sizeof () soit multiplié par sizeof (). cmGlobalVisualStudioGenerator.cxx 556
  • V531 Il est étrange qu'un opérateur sizeof () soit multiplié par sizeof (). cmGlobalVisualStudioGenerator.cxx 572
  • V531 Il est étrange qu'un opérateur sizeof () soit multiplié par sizeof (). cmGlobalVisualStudioGenerator.cxx 621
  • V531 Il est étrange qu'un opérateur sizeof () soit multiplié par sizeof (). cmGlobalVisualStudioGenerator.cxx 622
  • V531 Il est étrange qu'un opérateur sizeof () soit multiplié par sizeof (). cmGlobalVisualStudioGenerator.cxx 649

V595 Le pointeur 'this-> BuildFileStream' a été utilisé avant d'être vérifié par rapport à nullptr. Vérifiez les lignes: 133, 134. cmMakefileTargetGenerator.cxx 133

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

Le pointeur this-> BuildFileStream est déréférencé juste avant la vérification de sa validité. Cela n'a-t-il pas posé de problème à personne? Ci-dessous, il y a un autre exemple d'un tel extrait. Il est fait comme une copie carbone. Mais en fait, il existe de nombreux avertissements V595 et la plupart ne sont pas si évidents. D'après mon expérience, je peux dire que la correction des avertissements de ce diagnostic prend le plus de temps.

  • V595 Le pointeur 'this-> FlagFileStream' a été utilisé avant d'être vérifié par rapport à nullptr. Vérifiez les lignes: 303, 304. cmMakefileTargetGenerator.cxx 303

V614 Pointeur non initialisé 'str' utilisé. 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; };

L'analyseur a détecté l'utilisation du pointeur str non initialisé. Il est apparu en raison d'une faute de frappe ordinaire. Lors de l'appel de la fonction SysAllocStringByteLen , on aurait dû utiliser le pointeur src.str .

V557 Le dépassement de matrice est possible. La valeur de l'indice 'lensymbol' pourrait atteindre 28. 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]); } .... }

Ce morceau de code cache plusieurs problèmes à la fois. Lors de l'accès aux bases de données et aux tableaux de longueurs , un index de tableau peut sortir des limites, car les développeurs ont écrit l'opérateur «>» au lieu de «> =» ci-dessus. Cette vérification a commencé à manquer une valeur inacceptable. Ici, nous n'avons rien d'autre qu'un modèle d'erreur classique appelé Erreur Off-by-one .

Voici la liste complète des opérations d'accès aux tableaux par un index non valide:

  • V557 Le dépassement de matrice est possible. La valeur de l'indice 'lensymbol' pourrait atteindre 28. archive_read_support_format_rar.c 2750
  • V557 Le dépassement de matrice est possible. La valeur de l'indice 'lensymbol' pourrait atteindre 28. archive_read_support_format_rar.c 2751
  • V557 Le dépassement de matrice est possible. La valeur de l'index 'lensymbol' pourrait atteindre 28. archive_read_support_format_rar.c 2753
  • V557 Le dépassement de matrice est possible. La valeur de l'indice 'lensymbol' pourrait atteindre 28. archive_read_support_format_rar.c 2754
  • V557 Le dépassement de matrice est possible. La valeur de l'index 'offssymbol' pourrait atteindre 60. archive_read_support_format_rar.c 2797

Fuite de mémoire


V773 La fonction a été fermée sans libérer le pointeur 'testRun'. Une fuite de mémoire est possible. 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; }

L'analyseur a détecté une fuite de mémoire. La mémoire du pointeur testRun n'est pas libérée si la fonction testRun-> StartTest renvoie true . Lors de l'exécution d'une autre branche de code, cette mémoire est libérée dans la fonction this-> FinishTestProcess.

Fuite de ressources


V773 La fonction a été fermée sans fermer le fichier référencé par le descripteur 'fd'. Une fuite de ressource est possible. 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; }

Logique étrange dans des conditions


V590 Envisagez d'inspecter l' expression '* s! =' \ 0 '&& * s ==' ''. L'expression est excessive ou contient une erreur d'impression. 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++; .... }

La comparaison des caractères de * avec null est redondante. La condition de la boucle while dépend uniquement du fait que le caractère soit égal ou non à un espace. Ce n'est pas une erreur, mais une complication inutile du code.

V592 L'expression a été placée entre parenthèses deux fois: ((expression)). Une paire de parenthèses n'est pas nécessaire ou une erreur d'impression est présente. 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); } .... }

L'analyseur avertit que l'opération de négation doit probablement être retirée des crochets. Il semble qu'il n'y ait pas un tel bug ici - juste des doubles crochets inutiles. Mais très probablement, il y a une erreur logique dans le code.

L'opérateur continue n'est exécuté que dans le cas où la liste des tests this-> TestsToRun n'est pas vide et cnt est absent. Il est raisonnable de supposer que si la liste de tests est vide, la même action doit avoir lieu. Très probablement, la condition devrait être la suivante:

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

V592 L'expression a été placée entre parenthèses deux fois: ((expression)). Une paire de parenthèses n'est pas nécessaire ou une erreur d'impression est présente. 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; } .... }

C'est un exemple similaire, mais cette fois, je suis plus confiant qu'une erreur se produit. La fonction IsSet ("CMAKE_WARN_DEPRECATED") vérifie que la valeur CMAKE_WARN_DEPRECATED est définie globalement et la fonction IsOn ("CMAKE_WARN_DEPRECATED) vérifie que la valeur est définie dans la configuration du projet. Très probablement, l'opérateur complémentaire est redondant, car dans les deux cas, il est correct de définir les mêmes valeurs de type et de niveau.

V728 Un contrôle excessif peut être simplifié. Le '(A &&! B) || (! A && B) 'est équivalente à l'expression' 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 "; } .... }

Ce code peut être plus simple. On peut réécrire l'expression conditionnelle de la manière suivante:

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

Quelques autres endroits pour simplifier:

  • V728 Un contrôle excessif peut être simplifié. Le '(A && B) || (! A &&! B) 'est équivalente à l'expression' bool (A) == bool (B) '. cmCTestTestHandler.cxx 702
  • V728 Un contrôle excessif peut être simplifié. Le '(A &&! B) || (! A && B) 'est équivalente à l'expression' bool (A)! = Bool (B) '. digest_sspi.c 443
  • V728 Un contrôle excessif peut être simplifié. Le '(A &&! B) || (! A && B) 'est équivalente à l'expression' bool (A)! = Bool (B) '. tcp.c 1295
  • V728 Un contrôle excessif peut être simplifié. Le '(A &&! B) || (! A && B) 'est équivalente à l'expression' bool (A)! = Bool (B) '. testDynamicLoader.cxx 58
  • V728 Un contrôle excessif peut être simplifié. Le '(A &&! B) || (! A && B) 'est équivalente à l'expression' bool (A)! = Bool (B) '. testDynamicLoader.cxx 65
  • V728 Un contrôle excessif peut être simplifié. Le '(A &&! B) || (! A && B) 'est équivalente à l'expression' bool (A)! = Bool (B) '. testDynamicLoader.cxx 72

Avertissements divers


V523 L' instruction 'then' est équivalente au fragment de code suivant. 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)); }

L'expression dans la dernière condition est similaire aux deux dernières lignes de la fonction. Un développeur peut simplifier ce code en supprimant la condition, ou il y a une erreur dans le code et elle doit être corrigée.

V535 La variable 'i' est utilisée pour cette boucle et pour la boucle externe. Vérifiez les lignes: 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; } } } .... }

La variable i est utilisée comme compteur de boucles dans les boucles externes et internes. Dans le même temps, la valeur du compteur recommence à zéro dans la boucle intérieure. Ce n'est peut-être pas un bug ici, mais le code est suspect.

V519 La variable 'tagString' reçoit des valeurs successives deux fois. C'est peut-être une erreur. Vérifiez les lignes: 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"; } } .... }

La variable tagString est remplacée par une nouvelle valeur à tous les endroits. Il est difficile de dire quel est le problème ou pourquoi ils l'ont fait. Peut-être que les opérateurs «=» et «+ =» étaient confus.

La liste complète de ces lieux:

  • V519 La variable 'tagString' reçoit des valeurs successives deux fois. C'est peut-être une erreur. Vérifiez les lignes: 94, 96. cmCPackLog.cxx 96
  • V519 La variable 'tagString' reçoit des valeurs successives deux fois. C'est peut-être une erreur. Vérifiez les lignes: 104, 106. cmCPackLog.cxx 106
  • V519 La variable 'tagString' reçoit des valeurs successives deux fois. C'est peut-être une erreur. Vérifiez les lignes: 114, 116. cmCPackLog.cxx 116
  • V519 La variable 'tagString' reçoit des valeurs successives deux fois. C'est peut-être une erreur. Vérifiez les lignes: 125, 127. cmCPackLog.cxx 127

V519 La variable 'aes-> aes_set' se voit attribuer des valeurs deux fois de suite. C'est peut-être une erreur. Vérifier les lignes: 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); }

Le réglage forcé de la valeur AES_SET_UTF8 semble suspect. Je pense qu'un tel code va dérouter tout développeur, qui vient d'affiner ce fragment.

Ce code a été copié dans un autre endroit:

  • V519 La variable 'aes-> aes_set' se voit attribuer des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 4066, 4068. archive_string.c 4068

Comment trouver des bogues dans un projet sur CMake


Dans cette section, je vais vous expliquer brièvement comment vérifier des projets CMake avec PVS-Studio aussi facilement qu'un-deux-trois.

Windows / Visual Studio

Pour Visual Studio, vous pouvez générer un fichier de projet à l'aide de l'interface graphique CMake ou la commande suivante:

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

Ensuite, vous pouvez ouvrir le fichier .sln et vérifier le projet à l'aide du plug - in pour Visual Studio.

Linux / macOS

Le fichier compile_commands.json est utilisé pour les vérifications sur ces systèmes. Soit dit en passant, il peut être généré dans différents systèmes de génération. Voici comment vous le faites dans CMake:

 cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=On ..

La dernière chose à faire est d'exécuter l'analyseur dans le répertoire avec le fichier .json:

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

Nous avons également développé un module pour les projets CMake. Certaines personnes aiment l'utiliser. Le module CMake et des exemples de son utilisation peuvent être trouvés dans notre référentiel sur GitHub: pvs-studio-cmake-examples .

Conclusion


Un large public d'utilisateurs de CMake est idéal pour tester le projet, mais de nombreux problèmes pourraient être évités avant la sortie en utilisant des outils d'analyse de code statique, tels que PVS-Studio .

Si vous avez aimé les résultats de l'analyseur, mais que votre projet n'est pas écrit en C et C ++, je voudrais rappeler que l'analyseur prend également en charge l'analyse des projets en C # et Java. Vous pouvez tester l'analyseur sur votre projet en allant sur cette page .

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

More articles:


All Articles