Geekly articles weekly

CMake: le cas où le projet est impardonnable la qualité de son code

Image 1

CMake est un système d'automatisation multiplateforme pour les projets de construction. Ce système est beaucoup plus ancien que l'analyseur de code statique PVS-Studio, alors que personne n'a encore essayé de l'appliquer au code et de revoir les erreurs. Il s'avère qu'il y a beaucoup d'erreurs. L'audience de CMake est énorme. Sur celui-ci, de nouveaux projets commencent et les anciens sont transférés. Il est effrayant d’imaginer combien de programmeurs pourraient avoir telle ou telle erreur.

Présentation


CMake (de la marque anglaise multiplateforme) est un système d'automatisation multiplateforme pour la construction de logiciels à partir de code source. CMake ne construit pas directement, mais génère uniquement des fichiers de contrôle de construction à partir des fichiers CMakeLists.txt. La première version du programme a eu lieu en 2000. À titre de comparaison, l'analyseur statique PVS-Studio n'est apparu qu'en 2008. Ensuite, il s'est concentré sur la recherche d'erreurs lors du portage de programmes de systèmes 32 bits vers des systèmes 64 bits, et en 2010, le premier ensemble de diagnostics à usage général ( V501 - V545 ) est apparu. Soit dit en passant, il y a quelques avertissements de ce premier ensemble sur le code CMake.

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

Les diagnostics V1040 n'ont été mis en œuvre que récemment. Au moment de la publication de l'article, il n'y aura probablement pas de publication avec, mais avec l'aide de ce diagnostic, nous avons déjà réussi à trouver une erreur difficile.

Ici, ils ont fait une faute de frappe dans le nom __MINGW32_ . Au final, un trait de soulignement manque. Si vous recherchez par code avec ce nom, vous pouvez vous assurer que le projet utilise réellement la version avec deux traits de soulignement des deux côtés:

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)) { .... } .... }

Lorsque le tableau est déclaré statiquement, l'opérateur sizeof calculera sa taille en octets, en tenant compte à la fois du nombre d'éléments et de la taille des éléments. Lors du calcul de la valeur de la variable cch_subkeyname, le programmeur n'en a pas tenu compte et a reçu une valeur 4 fois plus importante que prévu. Expliquons où c'est "4 fois".

Le tableau et sa taille incorrecte sont transmis à 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 une variable contenant la taille du tampon spécifiée 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 il peut stocker 256 caractères de type wchar_t (dans Windows wchar_t, il est de 2 octets). Cette valeur est 256 et doit être transmise à la fonction. Au lieu de cela, 512 est à nouveau multiplié par 2 pour obtenir 1024.

Je pense que comment corriger l'erreur est maintenant clair. Au lieu de la multiplication, utilisez la division:

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

Soit dit en passant, exactement la même erreur se produit lors du calcul de la valeur de la variable cch_keyclass .

L'erreur décrite peut potentiellement entraîner un débordement de tampon. Il est nécessaire de fixer tous ces endroits:

  • 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 validation. Est-ce que cela causait vraiment des problèmes? Voici un autre exemple d'un tel endroit. Il se fait juste sous le papier carbone. Mais en fait, il existe de nombreux avertissements V595 et la plupart ne sont pas si évidents. Par expérience, je peux dire que la correction des avertissements de ce diagnostic est la plus longue.

  • 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 non initialisé str . Et cela est dû à la faute de frappe habituelle. Lors de l'appel de la fonction SysAllocStringByteLen, vous avez 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]); } .... }

Plusieurs problèmes ont été trouvés dans ce morceau de code. Lors de l'accès aux tableaux de bases de longueur et de bits de longueur, il est possible d'aller au-delà de la limite du tableau, car au-dessus du code, les développeurs ont écrit l'opérateur '>' au lieu de '> ='. Une telle vérification a commencé à ignorer une valeur non valide. Nous sommes confrontés à un modèle d'erreur classique appelé Erreur Off-by-one .

La liste complète des endroits pour accéder aux tableaux par 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 par pointeur testRun n'est pas libérée si la fonction testRun-> StartTest renvoie true . Lorsqu'une autre branche de code est exécutée, la mémoire utilisant le pointeur testRun 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 du caractère * s avec un zéro terminal est superflue. 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 supplémentaire 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 la négation doit peut-être être mise hors des crochets. Il semble qu'il n'y ait pas une telle erreur ici - juste des doubles crochets supplémentaires. Mais, très probablement, il y a une erreur logique dans cette condition.

L'instruction continue est exécutée si la liste des tests this-> TestsToRun n'est pas vide et que cnt y est absent. Il est logique de supposer que si la liste de tests est vide, alors la même action doit être effectuée. Très probablement, la condition devrait être comme ceci:

 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; } .... }

Un exemple similaire, mais ici je suis plus confiant en présence d'une erreur. 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 spécifiée dans la configuration du projet. Très probablement, l'opérateur de négation est superflu, 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 "; } .... }

Un tel code peut être grandement simplifié en réécrivant l'expression conditionnelle de cette façon:

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

Quelques autres endroits que vous pouvez 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 identique aux deux dernières lignes de la fonction. Ce code peut être simplifié en supprimant la condition, ou il y a une erreur dans le code et doit être corrigé.

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 imbriquées. Dans ce cas, la valeur du compteur recommence à compter à partir de zéro dans celle incluse. Ce n'est peut-être pas une erreur 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

 oid 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 effilochée par la nouvelle valeur à tous les endroits. Il est difficile de dire quelle était l'erreur 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); }

Forcer AES_SET_UTF8 semble suspect. Je pense qu'un tel code induira en erreur tout développeur confronté au raffinement de cet endroit.

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 erreurs dans un projet sur CMake


Dans cette section, je vais vous expliquer comment vérifier facilement et facilement des projets sur CMake à l'aide de PVS-Studio.

Windows / Visual Studio

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

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

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

Linux / macOS

Sur ces systèmes, le fichier compile_commands.json est utilisé pour vérifier le projet. Soit dit en passant, il peut être généré dans différents systèmes d'assemblage. Dans CMake, cela se fait comme ceci:

 cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=On ..

Reste à démarrer 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


Le vaste public d'utilisateurs de CMake est un bon testeur du projet, mais de nombreux problèmes n'auraient pas pu être évités avant la sortie, en utilisant des outils d'analyse de code statique comme 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 tiens à vous rappeler que l'analyseur prend également en charge l'analyse des projets en C # et Java. Vous pouvez essayer l'analyseur sur votre projet en allant sur cette page.



Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: Svyatoslav Razmyslov. CMake: le cas où la qualité du projet est impardonnable .

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

More articles:


All Articles