Erreurs que l'analyse de code statique ne trouve pas car il n'est pas utilisé

Les lecteurs de nos articles notent parfois que l'analyseur de code statique PVS-Studio détecte un grand nombre d'erreurs non significatives et n'affectant pas l'application. C'est vraiment le cas. Pour la plupart, des bogues importants ont déjà été corrigés en raison de tests manuels, des commentaires des utilisateurs et d'autres méthodes coûteuses. Dans le même temps, bon nombre de ces erreurs auraient pu être trouvées au stade de l'écriture du code et corrigées avec une perte minimale de temps, de réputation et d'argent. Cet article fournira plusieurs exemples d'erreurs réelles, qui auraient pu être corrigées immédiatement, si les auteurs du projet avaient utilisé l'analyse de code statique.

L'idée est très simple. Nous rechercherons des exemples de demandes d'extraction sur GitHub qui spécifient qu'un problème est un correctif. Ensuite, nous essaierons de trouver ces bogues en utilisant l'analyseur de code statique PVS-Studio. Si une erreur a pu être trouvée par l'analyseur, alors c'est un bogue qui aurait pu être trouvé au stade de l'écriture du code. Plus le bug est corrigé tôt, moins il coûte cher.

Malheureusement, GitHub nous a laissé tomber et nous n'avons pas réussi à faire un gros article chic sur le sujet. GitHub lui-même a un problème (ou une fonctionnalité) qui ne vous permet pas de rechercher des commentaires de demandes d'extraction dans des projets écrits uniquement dans certains langages de programmation. Ou je ne sais pas comment le faire cuire. Malgré cela, je spécifie de rechercher des commentaires dans les projets C, C ++, C #, les résultats sont donnés pour tous les langages, y compris PHP, Python, JavaScript et autres. En conséquence, la recherche de cas appropriés s'est avérée extrêmement fastidieuse, et je vais en citer quelques exemples. Cependant, ils suffisent à démontrer l'utilité des outils d'analyse de code statique lorsqu'ils sont utilisés régulièrement.

Et si le bogue avait été attrapé au tout début? La réponse est simple: les programmeurs n'auraient pas à attendre qu'il apparaisse, puis rechercher et corriger le code défectueux.

Voyons les erreurs que PVS-Studio aurait pu détecter immédiatement:

Le premier exemple est tiré du projet SatisfactoryModLoader. Avant de corriger l'erreur, le code ressemblait à ceci:

// gets an API function from the mod handler SML_API PVOID getAPIFunction(std::string name) { bool found = false; for (Registry reg : modHandler.APIRegistry) { if (reg.name == name) { found = true; } } if (!found) { std::string msg = ...; MessageBoxA(NULL, msg.c_str(), "SatisfactoryModLoader Fatal Error", MB_ICONERROR); abort(); } } 

Ce code contenait une erreur, que PVS-Studio enverrait immédiatement un avertissement à:

V591 La fonction non vide doit renvoyer une valeur. ModFunctions.cpp 44

La fonction ci-dessus n'a pas de déclaration de retour , elle renverra donc une valeur formellement non définie. Le programmeur n'a pas utilisé l'analyseur de code, il a donc dû chercher le bogue par lui-même. La fonction après l'édition:

 // gets an API function from the mod handler SML_API PVOID getAPIFunction(std::string name) { bool found = false; PVOID func = NULL; for (Registry reg : modHandler.APIRegistry) { if (reg.name == name) { func = reg.func; found = true; } } if (!found) { std::string msg = ...; MessageBoxA(NULL, msg.c_str(), "SatisfactoryModLoader Fatal Error", MB_ICONERROR); abort(); } return func; } 

Curieusement, dans le commit, l'auteur a marqué le bogue comme critique: " correction d' un bogue critique où les fonctions API n'étaient pas retournées ".

Dans le deuxième commit de l'historique du projet mc6809, des modifications ont été introduites dans le code suivant:

 void mc6809dis_direct( mc6809dis__t *const dis, mc6809__t *const cpu, const char *const op, const bool b16 ) { assert(dis != NULL); assert(op != NULL); addr.b[MSB] = cpu->dp; addr.b[LSB] = (*dis->read)(dis, dis->next++); ... if (cpu != NULL) { ... } } 

L'auteur n'a corrigé qu'une seule ligne. Il a remplacé l'expression

 addr.b[MSB] = cpu->dp; 

pour le suivant

 addr.b[MSB] = cpu != NULL ? cpu->dp : 0; 

Dans l'ancienne version du code, il n'y avait aucune vérification pour un pointeur nul. Si cela se produit de sorte qu'un pointeur nul soit passé à la fonction mc6809dis_direct en tant que deuxième argument, sa déréférence se produira dans le corps de la fonction. Le résultat est déplorable et imprévisible .

La déréférence de pointeur nul est l'un des modèles les plus courants dont on nous parle: "Ce n'est pas un bug critique. Qui se soucie qu'il prospère dans le code? En cas de déréférence, le programme se bloque discrètement et c'est tout. » C'est étrange et triste d'entendre cela des programmeurs C ++, mais la vie arrive.

Quoi qu'il en soit, dans ce projet, une telle déréférence s'est transformée en bogue, comme le sujet du commit nous le dit: " Correction de bogue --- Déréférence nulle ".

Si le développeur du projet avait utilisé PVS-Studio, il aurait pu vérifier et trouver l'avertissement il y a deux mois et demi. C'est à ce moment que le bug a été introduit. Voici l'avertissement:

V595 Le pointeur 'cpu' a été utilisé avant d'être vérifié par rapport à nullptr. Lignes de contrôle: 1814, 1821. mc6809dis.c 1814

Ainsi, le bug aurait été corrigé au moment de son apparition, ce qui aurait fait gagner du temps et des nerfs au développeur :).

Un exemple d'un autre correctif intéressant a été trouvé dans le projet libmorton.

Code à fixer:

 template<typename morton> inline bool findFirstSetBitZeroIdx(const morton x, unsigned long* firstbit_location) { #if _MSC_VER && !_WIN64 // 32 BIT on 32 BIT if (sizeof(morton) <= 4) { return _BitScanReverse(firstbit_location, x) != 0; } // 64 BIT on 32 BIT else { *firstbit_location = 0; if (_BitScanReverse(firstbit_location, (x >> 32))) { // check first part firstbit_location += 32; return true; } return _BitScanReverse(firstbit_location, (x & 0xFFFFFFFF)) != 0; } #elif _MSC_VER && _WIN64 .... #elif __GNUC__ .... #endif } 

Dans sa modification, un programmeur remplace l'expression " firstbit_location + = 32 " par " * firstbit_location + = 32 ". Le programmeur s'attendait à ce que 32 soient ajoutés à la valeur de la variable référencée par le pointeur firstbit_location , mais 32 a été ajouté au pointeur lui-même. La valeur modifiée du pointeur n'a plus été utilisée nulle part et la valeur de variable attendue est restée inchangée.

PVS-Studio enverrait un avertissement à ce code:

V1001 La variable 'firstbit_location' est affectée mais n'est pas utilisée à la fin de la fonction. morton_common.h 22

Eh bien, qu'est-ce qui est si mauvais à propos de l'expression modifiée mais encore inutilisée? Le diagnostic V1001 ne semble pas être destiné à détecter des bogues particulièrement dangereux. Malgré cela, il a trouvé une erreur importante qui a influencé la logique du programme.

De plus, il s'est avéré que cette erreur n'était pas si facile à trouver! Non seulement il est dans le programme depuis la création du fichier, mais il a également connu de nombreuses modifications dans les lignes voisines et existe dans le projet depuis 3 (!) Ans! Pendant tout ce temps, la logique du programme a été brisée, et cela n'a pas fonctionné comme les développeurs s'y attendaient. S'ils avaient utilisé PVS-Studio, le bogue aurait été détecté beaucoup plus tôt.

À la fin, regardons un autre bel exemple. Pendant que je collectais des correctifs de bogues sur GitHub, je suis tombé plusieurs fois sur un correctif avec le contenu suivant . L'erreur fixe était ici:

 int kvm_arch_prepare_memory_region(...) { ... do { struct vm_area_struct *vma = find_vma(current->mm, hva); hva_t vm_start, vm_end; ... if (vma->vm_flags & VM_PFNMAP) { ... phys_addr_t pa = (vma->vm_pgoff << PAGE_SHIFT) + vm_start - vma->vm_start; ... } ... } while (hva < reg_end); ... } 

PVS-Studio a émis un avertissement pour cet extrait de code:

V629 Envisagez d'inspecter l'expression 'vma-> vm_pgoff << 12'. Décalage binaire de la valeur 32 bits avec une extension ultérieure au type 64 bits. mmu.c 1795

J'ai vérifié les déclarations de variables, utilisées dans l'expression " phys_addr_t pa = (vma-> vm_pgoff << PAGE_SHIFT) + vm_start - vma-> vm_start; " et j'ai découvert que le code donné ci-dessus est égal à l'exemple synthétique suivant:

 void foo(unsigned long a, unsigned long b) { unsigned long long x = (a << 12) + b; } 

Si la valeur de la variable a 32 bits est supérieure à 0xFFFFF , les 12 bits les plus élevés auront au moins une valeur non nulle. Après avoir déplacé cette variable vers la gauche, ces bits significatifs seront perdus, entraînant des informations incorrectes écrites en x.

Pour éliminer la perte de bits élevés, nous devons d'abord convertir un au type long long non signé et seulement après ce décalage, la variable:

 pa = (phys_addr_t)vma->vm_pgoff << PAGE_SHIFT; pa += vm_start - vma->vm_start; 

De cette façon, une valeur correcte sera toujours écrite en pa.

Ce serait bien, mais ce bogue, le même que le premier exemple de l'article, s'est également révélé critique. Son auteur a écrit à ce sujet dans le commentaire. De plus, cette erreur a trouvé son chemin dans un nombre énorme de projets. Pour apprécier pleinement l'ampleur de la tragédie, je suggère de regarder le nombre de résultats lors de la recherche de ce correctif sur GitHub. Effrayant, non?

J'ai donc adopté une nouvelle approche pour démontrer les avantages d'une utilisation régulière d'analyseur de code statique. J'espère que ça vous a plu. Téléchargez et essayez l'analyseur de code statique PVS-Studio pour vérifier vos propres projets. Au moment de la rédaction de ce document, il avait environ 700 règles de diagnostic mises en œuvre pour détecter une variété de modèles d'erreur. Prend en charge C, C ++, C # et Java.