Cela fait trois mois que 2018 s'est terminée. Pour beaucoup, il vient de passer, mais pour nous, développeurs de PVS-Studio, l'année a été mouvementée. Nous étions en train de transpirer, en concurrence intrépide pour répandre le mot sur l'analyse statique et recherchions des erreurs dans les projets open source, écrits en C, C ++, C # et Java. Dans cet article, nous avons rassemblé pour vous le top 10 des plus intéressants!
Pour trouver les endroits les plus intrigants, nous avons utilisé l'analyseur de code statique
PVS-Studio . Il peut détecter des bogues et des vulnérabilités potentielles dans le code, écrit dans les langues répertoriées ci-dessus.
Si vous êtes impatient de rechercher des erreurs par vous-même, vous pouvez toujours télécharger et essayer notre analyseur. Nous proposons la
version gratuite de l'analyseur pour les étudiants et les développeurs enthousiastes, la
licence gratuite pour les développeurs de projets open-source, ainsi que la
version d'essai pour le monde entier et son chien. Qui sait, peut-être que l'année prochaine, vous pourrez créer votre propre top 10? :)
Remarque: Je vous invite à vous vérifier et avant de regarder l'avertissement de l'analyseur, essayez de révéler vous-même les défauts. Combien d'erreurs pourrez-vous trouver?
Dixième place
Source:
Into Space Again: comment la licorne a visité le stellariumCette erreur a été détectée lors de la vérification d'un planétarium virtuel appelé Stellarium.
Le fragment de code ci-dessus, bien que petit, est lourd d'une erreur assez délicate:
Plane::Plane(Vec3f &v1, Vec3f &v2, Vec3f &v3) : distance(0.0f), sDistance(0.0f) { Plane(v1, v2, v3, SPolygon::CCW); }
Vous l'avez trouvé?
Avertissement PVS-Studio :
V603 L'objet a été créé mais il n'est pas utilisé. Si vous souhaitez appeler le constructeur, 'this-> Plane :: Plane (....)' doit être utilisé. Plane.cpp 29
L'auteur du code avait l'intention d'initialiser les champs de certains objets, en utilisant un autre constructeur, imbriqué dans le principal. Eh bien, au lieu de cela, il n'a réussi à créer un objet temporaire détruit qu'en quittant son champ d'application. Ce faisant, plusieurs champs d'objet resteront non initialisés.
L'auteur aurait dû utiliser un constructeur délégué, introduit en C ++ 11, au lieu d'un appel de constructeur imbriqué. Par exemple, il aurait pu écrire comme ceci:
Plane::Plane(Vec3f& v1, Vec3f& v2, Vec3f& v3) : Plane(v1, v2, v3, SPolygon::CCW) { distance = 0.0f; sDistance = 0.0f; }
De cette façon, tous les champs nécessaires auraient été correctement initialisés. N'est-ce pas merveilleux?
Neuvième place
Source:
Perl 5: comment masquer les erreurs dans les macrosUne macro très remarquable se détache dans toute sa beauté sur la neuvième place.
Lors de la collecte d'erreurs pour la rédaction d'un article, mon collègue Svyatoslav est tombé sur un avertissement, émis par l'analyseur, qui concernait l'utilisation des macros. Le voici:
PP(pp_match) { .... MgBYTEPOS_set(mg, TARG, truebase, RXp_OFFS(prog)[0].end); .... }
Pour découvrir ce qui n'allait pas, Svyatoslav a creusé plus profondément. Il a ouvert la définition de macro et a vu qu'elle contenait plusieurs macros imbriquées, dont certaines à leur tour avaient également des macros imbriquées. Il était si difficile de donner un sens à cela, alors il a dû utiliser un fichier prétraité. Malheureusement, cela n'a pas aidé. Voici ce que Svyatoslav a trouvé dans la ligne de code précédente:
(((targ)->sv_flags & 0x00000400) && (!((targ)->sv_flags & 0x00200000) || S_sv_only_taint_gmagic(targ)) ? (mg)->mg_len = ((prog->offs)[0].end), (mg)->mg_flags |= 0x40 : ((mg)->mg_len = (((targ)->sv_flags & 0x20000000) && !__builtin_expect(((((PL_curcop)->cop_hints + 0) & 0x00000008) ? (_Bool)1 :(_Bool)0),(0))) ? (ssize_t)Perl_utf8_length( (U8 *)(truebase), (U8 *)(truebase)+((prog->offs)[0].end)) : (ssize_t)((prog->offs)[0].end), (mg)->mg_flags &= ~0x40));
Avertissement PVS-Studio :
V502 L'opérateur '?:'
Fonctionne peut-être d'une manière différente de celle attendue. L'opérateur '?:' A une priorité plus faible que l'opérateur '&&'. pp_hot.c 3036
Je pense qu'il serait difficile de simplement remarquer une telle erreur. Nous nous attardons sur ce code depuis longtemps, mais, franchement, nous n'y avons pas trouvé d'erreur. Quoi qu'il en soit, c'est un exemple assez amusant de code mal lisible.
Ils disent que les macros sont mauvaises. Bien sûr, il y a des cas où les macros sont indispensables, mais si vous pouvez remplacer une macro par une fonction - vous devriez certainement le faire.
Les macros imbriquées sont particulièrement semées d'embûches. Non seulement parce qu'il est difficile de les comprendre, mais aussi parce qu'ils peuvent donner des résultats imprévisibles. Si un programmeur fait une erreur dans une telle macro, il sera beaucoup plus difficile de la trouver dans une macro que dans une fonction.
Huitième place
Source:
Chrome: autres erreursL'exemple suivant est tiré de la série d'articles sur l'analyse du projet Chromium. L'erreur se cachait dans la bibliothèque WebRTC.
std::vector<SdpVideoFormat> StereoDecoderFactory::GetSupportedFormats() const { std::vector<SdpVideoFormat> formats = ....; for (const auto& format : formats) { if (cricket::CodecNamesEq(....)) { .... formats.push_back(stereo_format); } } return formats; }
Avertissement PVS-Studio: les itérateurs V789 CWE-672 pour le conteneur 'formats', utilisés dans la boucle for basée sur la plage, deviennent invalides lors de l'appel de la fonction 'push_back'. stereocodecfactory.cc 89
L'erreur est que la taille du vecteur de
formats varie dans la boucle for basée sur la plage. Les boucles basées sur des plages sont basées sur des itérateurs, c'est pourquoi le changement de la taille du conteneur à l'intérieur de ces boucles peut entraîner l'invalidation de ces itérateurs.
Cette erreur persiste si vous réécrivez la boucle avec une utilisation explicite des itérateurs. Pour plus de clarté, je peux citer le code suivant:
for (auto format = begin(formats), __end = end(formats); format != __end; ++format) { if (cricket::CodecNamesEq(....)) { .... formats.push_back(stereo_format); } }
Par exemple, lorsque vous utilisez la méthode
push_back , une réallocation de vecteur peut se produire - de cette façon, les itérateurs adresseront un emplacement de mémoire non valide.
Pour éviter de telles erreurs, suivez la règle: ne modifiez jamais la taille d'un conteneur à l'intérieur d'une boucle avec des conditions liées à ce conteneur. Elle concerne également les boucles basées sur des plages et les boucles utilisant des itérateurs. Vous êtes invités à lire cette
discussion sur StackOverflow qui couvre le sujet des opérations provoquant l'invalidation des itérateurs.
Septième place
Source:
Godot: sur l'utilisation régulière d'analyseurs statiquesLe premier exemple de l'industrie du jeu sera un extrait de code que nous avons trouvé dans le moteur de jeu Godot. Il faudra probablement un peu de travail pour remarquer l'erreur, mais je suis sûr que nos lecteurs familiers y feront face.
void AnimationNodeBlendSpace1D::add_blend_point( const Ref<AnimationRootNode> &p_node, float p_position, int p_at_index) { ERR_FAIL_COND(blend_points_used >= MAX_BLEND_POINTS); ERR_FAIL_COND(p_node.is_null()); ERR_FAIL_COND(p_at_index < -1 || p_at_index > blend_points_used); if (p_at_index == -1 || p_at_index == blend_points_used) { p_at_index = blend_points_used; } else { for (int i = blend_points_used - 1; i > p_at_index; i++) { blend_points[i] = blend_points[i - 1]; } } .... }
Avertissement PVS-Studio: V621 CWE-835 Envisagez d'inspecter l'opérateur «pour». Il est possible que la boucle soit mal exécutée ou ne soit pas exécutée du tout. animation_blend_space_1d.cpp 113
Examinons de près l'état de la boucle. La variable de compteur est initialisée par la valeur
blend_points_used - 1 . De plus, à en juger par deux vérifications précédentes (dans
ERR_FAIL_COND et dans
if ), il devient clair qu'au moment de l'exécution de la boucle
blend_points_used ,
blend_points_used sera toujours supérieur à
p_at_index . Ainsi, soit la condition de boucle est toujours vraie, soit la boucle n'est pas exécutée du tout.
Si
blend_points_used - 1 == p_at_index , la boucle ne s'exécute pas.
Dans tous les autres cas, la vérification
i> p_at_index sera toujours vraie, car le compteur
i monte à chaque itération de boucle.
Il semble que la boucle soit éternelle, mais ce n'est pas le cas.
Premièrement, un débordement d'entier de la variable
i (qui est un comportement indéfini) se produira. Cela signifie que nous ne devons pas nous y fier.
Si
i n'était
pas signé int , alors après que le compteur atteigne la plus grande valeur possible, l'opérateur
i ++ le transformerait en
0 . Un tel comportement est défini par la norme et est appelé "habillage non signé". Cependant, vous devez être conscient que l'utilisation d'un tel mécanisme n'est
pas non plus une bonne idée .
C'était le premier point, mais nous avons toujours le deuxième! Le cas est que nous n'atteindrons même pas un débordement d'entier. L'index du tableau sortira des limites un peu plus tôt. Cela signifie qu'il y aura une tentative d'accès à la mémoire en dehors du bloc alloué pour le tableau. Ce qui est également un comportement indéfini. Un exemple classique :)
Je peux vous donner quelques recommandations pour éviter plus facilement des erreurs similaires:
- Écrivez un code simple et compréhensible
- Examinez le code plus en profondeur et écrivez plus de tests pour le code nouvellement écrit
- Utilisez des analyseurs statiques;)
Sixième place
Source:
Amazon Lumberyard: un cri d'angoisseVoici un autre exemple de l'industrie gamedev, à savoir du code source du moteur AAA d'Amazon Lumberyard.
void TranslateVariableNameByOperandType(....) {
Avertissement PVS-Studio :
V523 L'
instruction 'then' est équivalente à l'instruction 'else'. toglsloperand.c 700
Amazon Lumberyard est développé en tant que moteur multiplateforme. Pour cette raison, les développeurs essaient de prendre en charge autant de compilateurs que possible. Comme nous pouvons le voir dans les commentaires, un programmeur Igor s'est opposé au compilateur Qualcomm.
Nous ne savons pas s'il a réussi à mener à bien sa tâche et à parcourir les vérifications du compilateur "paranoïaque", mais il a laissé un code très étrange. La chose étrange à ce sujet est que les branches
alors - et
else - de l'instruction
if contiennent un code absolument identique. Très probablement, une telle erreur résulte de l'utilisation d'une méthode de copier-coller bâclée.
Je ne sais même pas quoi conseiller ici. Je souhaite donc aux développeurs d'Amazon Lumberyard tout le meilleur pour corriger les erreurs et bonne chance au développeur Igor!
Cinquième place
Source:
Encore une fois, l'analyseur PVS-Studio s'est avéré plus attentif qu'une personneUne histoire intéressante s'est produite avec l'exemple suivant. Mon collègue Andrey Karpov préparait un article sur une autre vérification du framework Qt. En écrivant quelques erreurs notables, il est tombé sur l'avertissement de l'analyseur, qu'il a considéré faux. Voici ce fragment de code et l'avertissement:
QWindowsCursor::CursorState QWindowsCursor::cursorState() { enum { cursorShowing = 0x1, cursorSuppressed = 0x2 }; CURSORINFO cursorInfo; cursorInfo.cbSize = sizeof(CURSORINFO); if (GetCursorInfo(&cursorInfo)) { if (cursorInfo.flags & CursorShowing)
Avertissement PVS-Studio: V616 CWE-480 La constante nommée 'CursorShowing' avec la valeur 0 est utilisée dans l'opération au niveau du bit. qwindowscursor.cpp 669
Ce qui veut dire que PVS-Studio se plaignait à l'endroit, qui évidemment n'avait pas d'erreur! Il est impossible que la constante
CursorShowing soit
0 , car quelques lignes au-dessus sont initialisées par
1 .
Étant donné qu'Andrey utilisait une version d'analyseur instable, il a mis en doute l'exactitude de l'avertissement. Il a soigneusement regardé ce morceau de code et n'a toujours pas trouvé de bogue. Il a finalement donné un faux positif dans le bugtracker afin que d'autres collègues puissent remédier à la situation.
Seule une analyse détaillée a montré que PVS-Studio s'est avéré plus prudent qu'une personne à nouveau. La valeur
0x1 est affectée à une constante nommée appelée
cursorShowing tandis que
CursorShowing participe à une opération "et" au niveau du bit. Ce sont deux constantes totalement différentes, la première commence par une lettre minuscule, la seconde - par une majuscule.
Le code se compile avec succès, car la classe
QWindowsCursor contient vraiment une constante avec ce nom. Voici sa définition:
class QWindowsCursor : public QPlatformCursor { public: enum CursorState { CursorShowing, CursorHidden, CursorSuppressed }; .... }
Si vous n'affectez pas explicitement une valeur à une constante enum, elle sera initialisée par défaut. Comme
CursorShowing est le premier élément de l'énumération, il lui sera attribué
0 .
Pour éviter de telles erreurs, vous ne devez pas donner aux entités des noms trop similaires. Vous devez particulièrement suivre cette règle si les entités sont du même type ou peuvent être implicitement converties les unes aux autres. Comme dans de tels cas, il sera presque impossible de remarquer l'erreur, mais le code incorrect sera toujours compilé et vivra dans une rue facile à l'intérieur de votre projet.
Quatrième place
Source:
vous tirer une balle dans le pied lors de la manipulation des données d'entréeNous nous rapprochons des trois premiers finalistes et le prochain en ligne est l'erreur du projet FreeSWITCH.
static const char *basic_gets(int *cnt) { .... int c = getchar(); if (c < 0) { if (fgets(command_buf, sizeof(command_buf) - 1, stdin) != command_buf) { break; } command_buf[strlen(command_buf)-1] = '\0'; break; } .... }
Avertissement PVS-Studio: V1010 CWE-20 Les données contaminées non
contrôlées sont utilisées dans l'index: 'strlen (command_buf)'.
L'analyseur vous avertit que certaines données non contrôlées sont utilisées dans l'expression
strlen (command_buf) - 1 . En effet: si
command_buf est une chaîne vide en termes de langage C (contenant le seul caractère - '\ 0'),
strlen (command_buf) retournera
0 . Dans un tel cas,
command_buf [-1] sera accessible, ce qui est un comportement non défini. C'est mauvais!
Le zeste réel de cette erreur n'est pas
pourquoi elle se produit, mais
comment . Cette erreur est l'un de ces plus beaux exemples, que vous "touchez" par vous-même, reproduisez. Vous pouvez exécuter FreeSwitch, entreprendre certaines actions qui mèneront à l'exécution du fragment de code mentionné ci-dessus et passer une chaîne vide à l'entrée du programme.
En conséquence, avec un mouvement subtil de la main, un programme de travail se transforme en un non-travail! Vous pouvez trouver les détails sur la façon de reproduire cette erreur dans l'article source par le lien donné ci-dessus. En attendant, permettez-moi de vous fournir un résultat révélateur:
Gardez à l'esprit que les données de sortie peuvent être n'importe quoi, vous devez donc toujours les vérifier. De cette façon, l'analyseur ne se plaindra pas et le programme deviendra plus fiable.
Il est maintenant temps de choisir notre vainqueur: nous sommes maintenant dans la phase finale! Soit dit en passant, les finalistes des bugs ont déjà attendu longtemps, puis se sont ennuyés et ont même commencé à être frileux. Regardez ce qu'ils ont mis en scène pendant notre absence!
Troisième place
Source:
NCBI Genome Workbench: la recherche scientifique menacéeUn extrait de code du projet NCBI Genome Workbench, qui est un ensemble d'outils pour étudier et analyser les données génétiques, ouvre les trois premiers gagnants. Même si vous n'avez pas besoin d'être un surhumain génétiquement modifié pour trouver ce bug, seules quelques personnes connaissent la possibilité de faire une erreur ici.
void tds_answer_challenge(....) { .... if (ntlm_v == 1) { .... memset(hash, 0, sizeof(hash)); memset(passwd_buf, 0, sizeof(passwd_buf)); ... } else { .... } }
Avertissements de PVS-Studio:- V597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider le tampon 'hash'. La fonction memset_s () doit être utilisée pour effacer les données privées. challenge.c 365
- V597 Le compilateur pourrait supprimer l'appel de fonction 'memset', qui est utilisé pour vider le tampon 'passwd_buf'. La fonction memset_s () doit être utilisée pour effacer les données privées. challenge.c 366
Avez-vous trouvé un bug? Si oui, vous êtes un attaboy! .. ou un surhumain génétiquement modifié.
Le fait est que les compilateurs d'optimisation modernes peuvent faire beaucoup pour permettre à un programme construit de fonctionner plus rapidement. Y compris le fait que les compilateurs peuvent désormais suivre qu'un tampon, passé à
memset , n'est utilisé nulle part ailleurs.
Dans ce cas, ils peuvent supprimer l'appel "inutile" de
memset , ayant tous les droits pour cela. Ensuite, le tampon qui stocke les données importantes peut rester en mémoire pour le plus grand plaisir des attaquants.
Dans ce contexte, ce commentaire de geek "avec la sécurité est préférable d'être pédant" semble encore plus drôle. À en juger par un petit nombre d'avertissements donnés pour ce projet, ses développeurs ont fait de leur mieux pour être précis et écrire du code sûr. Cependant, comme nous pouvons le voir, on peut facilement ignorer un tel défaut de sécurité. Selon l'énumération commune des faiblesses, ce défaut est classé comme
CWE-14 : suppression du code par le compilateur pour effacer les tampons.
Vous devez utiliser la fonction
memset_s () pour que la désallocation de mémoire soit sûre. La fonction est à la fois plus sûre que
memset () et ne peut pas être ignorée par un compilateur.
Deuxième place
Source:
Comment PVS-Studio s'est avéré plus attentif que trois et demi programmeursUn médaillé d'argent nous a été gentiment envoyé par l'un de nos clients. Il était sûr que l'analyseur avait émis des faux positifs.
Evgeniy a reçu l'e-mail, a regardé à travers cela et a envoyé à Svyatoslav. Svyatoslav a examiné de près le morceau de code envoyé par le client et a pensé: "comment est-il possible que l'analyseur ait fait une telle erreur?". Il est donc allé demander conseil à Andrey. Il a également vérifié cet endroit et déterminé: en effet, l'analyseur a généré des faux positifs.
Alors ça va, il fallait réparer. Ce n'est qu'après que Svyatoslav a commencé à faire des exemples synthétiques pour créer la tâche dans notre traqueur de bogues, qu'il a eu ce qui n'allait pas.
Aucun des programmeurs n'a pu trouver les erreurs, mais elles étaient vraiment dans le code. Franchement, l'auteur de cet article n'a pas non plus pu les trouver malgré le fait que l'analyseur a clairement émis des avertissements pour les endroits erronés!
Trouverez-vous un bug aussi astucieux? Testez-vous sur la vigilance et l'attention.
Avertissement PVS-Studio:- V560 Une partie de l'expression conditionnelle est toujours fausse: (ch> = 0x0FF21). decodew.cpp 525
- V560 Une partie de l'expression conditionnelle est toujours vraie: (ch <= 0x0FF3A). decodew.cpp 525
- V560 Une partie de l'expression conditionnelle est toujours fausse: (ch> = 0x0FF41). decodew.cpp 525
- V560 Une partie de l'expression conditionnelle est toujours vraie: (ch <= 0x0FF5A). decodew.cpp 525
Si vous l'avez fait - bravo à vous!
L'erreur réside dans le fait que l'opérateur de négation logique (!) N'est pas appliqué à la condition entière, mais seulement à sa première sous-expression:
!((ch >= 0x0FF10) && (ch <= 0x0FF19))
Si cette condition est vraie, la valeur de la variable
ch se situe dans la plage [0x0FF10 ... 0x0FF19]. Ainsi, quatre autres comparaisons sont déjà dénuées de sens: elles seront toujours vraies ou fausses.
Pour éviter de telles erreurs, il convient de s'en tenir à quelques règles. Tout d'abord, il est très pratique et informatif d'aligner le code comme un tableau. Deuxièmement, vous ne devez pas surcharger les expressions avec des parenthèses. Par exemple, ce code pourrait être réécrit comme ceci:
const bool isLetterOrDigit = (ch >= 0x0FF10 && ch <= 0x0FF19)
De cette façon, il y aura moins de parenthèses et d'autre part - vous remarquerez plus probablement une erreur occasionnelle.
Voici la cerise sur le gâteau - passons à la première place!
Première place
Source:
Shocked System: erreurs intéressantes dans le code source du Legendary System ShockLe top finaliste d'aujourd'hui est une erreur du légendaire System Shock! C'est un jeu sorti il y a assez longtemps en 1994, qui est devenu un prédécesseur et une source d'inspiration pour des jeux emblématiques tels que Dead Space, BioShock et Deus Ex.
Mais d'abord, j'ai quelque chose à avouer. Ce que je vais vous montrer maintenant, ne contient aucune erreur. En fait, ce n'est même pas un morceau de code, mais je n'ai pas pu m'empêcher de le partager avec vous!
Le fait est qu'en analysant le code source du jeu, ma collègue Victoria a découvert de nombreux commentaires fascinants. Dans différents fragments, elle a trouvé des remarques pleines d'esprit et d'ironie, et même de la poésie.
Voici à quoi ressemblent les commentaires laissés par les développeurs dans les jeux dans les dernières années 90 ... Soit dit en passant, Doug Church - un concepteur en chef de System Shock, était également occupé à écrire du code. Qui sait, peut-être que certains de ces commentaires ont été écrits par lui? Espérons que les trucs d'hommes en serviettes ne sont pas son travail :)
Conclusion
En conclusion, je voudrais remercier
mes collègues d' avoir recherché de nouveaux bugs et d'avoir écrit à leur sujet dans des articles. Merci les gars! Sans vous, cet article ne serait pas si intéressant.
Je voudrais également parler un peu de nos réalisations, car toute l'année, nous n'avons pas été occupés à rechercher uniquement des erreurs. Nous avons également développé et amélioré l'analyseur, ce qui a entraîné des changements importants.
Par exemple, nous avons ajouté la prise en charge de plusieurs nouveaux compilateurs et étendu la liste des règles de diagnostic. Nous avons également implémenté la prise en charge initiale des normes
MISRA C et MISRA C ++ . La nouvelle fonctionnalité la plus importante et la plus longue a été la prise en charge d'une nouvelle langue. Oui, nous pouvons maintenant analyser le code en
Java ! Et en plus, nous avons une
icône renouvelée :)
Je remercie également nos lecteurs. Merci d'avoir lu nos articles et de nous avoir écrit! Vous êtes si réactif et si important pour nous!
Notre top 10 des erreurs C ++ de 2018 a pris fin. Quels fragments avez-vous le plus appréciés et pourquoi? Avez-vous rencontré des exemples intéressants en 2018?
Bien, à la prochaine fois!