Notre lectorat est en croissance, nous écrivons donc encore et encore des articles qui expliquent comment utiliser correctement la méthodologie d'analyse de code statique. Nous considérons qu'il est très important d'expliquer que les outils d'analyse statique ne doivent pas être utilisés sporadiquement, mais régulièrement. Encore une fois, nous démontrons cela avec un exemple pratique, revérifiant le projet Godot.
Utilisez régulièrement des analyseurs
En me préparant à prendre la parole lors de la conférence des développeurs de jeux, j'ai décidé d'obtenir de nouveaux exemples d'erreurs intéressantes que l'outil
PVS-Studio peut détecter. À cet effet, plusieurs moteurs de jeu ont été testés, dont Godot. Je n'ai trouvé aucune erreur particulièrement intéressante pour le rapport, mais je voulais écrire un article sur les erreurs ordinaires. Ces erreurs démontrent très bien la pertinence de l'utilisation régulière d'outils d'analyse de code statique.
Il convient de noter que nous avons déjà
vérifié ce projet en 2015, et l'auteur a travaillé avec les erreurs que nous avons notées.
Ici vous pouvez voir le commit correspondant.
3 ans se sont écoulés. Le projet a changé. L'analyseur PVS-Studio a également changé et de nouveaux diagnostics y sont apparus. Par conséquent, il n'est pas surprenant que j'ai pu écrire facilement et rapidement suffisamment d'exemples d'erreurs pour écrire cet article.
Cependant, quelque chose d'autre est important. Lors du développement de Godot ou de tout autre projet, de nouvelles erreurs apparaissent constamment et sont corrigées. Les erreurs non détectées «se déposent» dans le code pendant une longue période, puis bon nombre d'entre elles peuvent être détectées lors de l'exécution d'une analyse de code statique. De ce fait, il existe parfois un faux sentiment que les analyseurs statiques ne trouvent que des erreurs inintéressantes dans les sections de code rarement utilisées. Bien sûr, c'est le cas si vous n'utilisez pas correctement l'analyseur et ne l'exécutez que de temps en temps, par exemple, peu de temps avant la publication de la version.
Oui, lors de la rédaction d'articles, nous effectuons nous-mêmes des contrôles ponctuels des projets ouverts. Mais nous avons un objectif différent. Nous voulons démontrer les capacités d'un analyseur de code pour détecter les défauts. Cette tâche a peu à voir avec l'amélioration de la qualité du code de projet dans son ensemble et la réduction des coûts associés à la correction des erreurs.
Encore une fois sur l'essentiel. Le point de l'analyse de code statique n'est pas de trouver d'anciennes erreurs. Ces anciennes erreurs sont généralement insignifiantes, sinon elles auraient interféré avec les utilisateurs et auraient déjà été trouvées et corrigées. Le point de l'analyse statique est de corriger rapidement les erreurs dans le code nouvellement écrit ou modifié. Cela réduit le temps de débogage, le nombre de plaintes des utilisateurs et, finalement, le budget du projet développé.
Passons maintenant aux bugs que les lecteurs de nos publications aiment tant.
Erreurs de copier-coller
Voyons ce que j'ai remarqué en étudiant le rapport PVS-Studio. Je vais commencer par mon diagnostic V501 préféré, qui trouve des
erreurs dans presque tous les projets que nous vérifions :).
Erreur N1virtual bool can_export(....) { .... if (!exists_export_template("uwp_" + platform_infix + "_debug.zip", &err) || !exists_export_template("uwp_" + platform_infix + "_debug.zip", &err)) { valid = false; r_missing_templates = true; } .... }
Avertissement PVS-Studio: V501 CWE-570 Il existe des sous-expressions identiques '! Exists_export_template ("uwp_" + platform_infix + "_debug.zip", & err)' à gauche et à droite de '||' opérateur. export.cpp 1135
Erreur de copier-coller classique. L'appel de fonction a été copié mais pas modifié. Le nom du deuxième fichier à traiter doit se terminer par "_release.zip".
Erreurs N2, N3 static String dump_node_code(SL::Node *p_node, int p_level) { .... if (bnode->statements[i]->type == SL::Node::TYPE_CONTROL_FLOW || bnode->statements[i]->type == SL::Node::TYPE_CONTROL_FLOW) { code += scode;
Avertissement PVS-Studio: V501 CWE-570 Il existe des sous-expressions identiques «bnode-> instructions [i] -> type == SL :: Node :: TYPE_CONTROL_FLOW» à gauche et à droite de «||» opérateur. test_shader_lang.cpp 183
void EditorSpinSlider::_notification(int p_what) { if (p_what == MainLoop::NOTIFICATION_WM_FOCUS_OUT || p_what == MainLoop::NOTIFICATION_WM_FOCUS_OUT) { if (grabbing_spinner) { Input::get_singleton()->set_mouse_mode(Input::MOUSE_MODE_VISIBLE); grabbing_spinner = false; grabbing_spinner_attempt = false; } } .... }
Avertissement PVS-Studio: V501 CWE-570 Il existe des sous-expressions identiques 'p_what == MainLoop :: NOTIFICATION_WM_FOCUS_OUT' à gauche et à droite de '||' opérateur. editor_spin_slider.cpp 157
Je pense que les erreurs sont clairement visibles et ne nécessitent aucune explication. Exactement le même copier-coller classique, comme dans le premier cas.
Erreur N4 String SoftBody::get_configuration_warning() const { .... Transform t = get_transform(); if ((ABS(t.basis.get_axis(0).length() - 1.0) > 0.05 || ABS(t.basis.get_axis(1).length() - 1.0) > 0.05 || ABS(t.basis.get_axis(0).length() - 1.0) > 0.05)) { if (!warning.empty()) .... }
Avertissement PVS-Studio: V501 CWE-570 Il existe des sous-expressions identiques à gauche et à droite de '||' opérateur. soft_body.cpp 399
Ici, la première ligne a été copiée deux fois. Mais le numéro de l'axe des coordonnées n'a été modifié que dans la deuxième ligne. Et ils ont oublié de modifier la troisième ligne. Ce n'est rien d'autre que "l'effet de
dernière ligne ".
Remarque En ce moment, en plus de «Last Line Effect», j'ai fait les observations intéressantes suivantes: «
La fonction la plus dangereuse dans le monde C / C ++ », «Le
mal vit dans les fonctions de comparaison ». Et maintenant, je vais faire l'annonce d'un nouvel article, dont je prévois de rédiger dans un avenir proche. Le titre de travail est "0, 1, 2". Cela devrait être intéressant et instructif. Je vous invite à vous abonner à l'une des chaînes afin de ne pas manquer:
twitter ,
vk.com ,
Instagram ,
télégramme et
rss "old school".
Erreur N5 void ScrollContainer::_notification(int p_what) { .... if (h_scroll->is_visible_in_tree() && h_scroll->get_parent() == this) size.y -= h_scroll->get_minimum_size().y; if (v_scroll->is_visible_in_tree() && v_scroll->get_parent() == this) size.x -= h_scroll->get_minimum_size().x; .... }
Avertissement PVS-Studio: V778 CWE-682 Deux fragments de code similaires ont été trouvés. Il s'agit peut-être d'une faute de frappe et la variable «v_scroll» devrait être utilisée à la place de «h_scroll». scroll_container.cpp 249
En ce qui concerne ce morceau de code, je ne suis pas complètement sûr qu'il y ait une erreur. Cependant, je suis d'accord avec l'analyseur que le deuxième bloc semble très suspect. Très probablement, le code a été écrit à l'aide de Copy-Paste, et dans le deuxième bloc de texte, ils ont oublié de remplacer
h_scroll par
v_scroll .
Le code devrait probablement être comme ceci:
if (h_scroll->is_visible_in_tree() && h_scroll->get_parent() == this) size.y -= h_scroll->get_minimum_size().y; if (v_scroll->is_visible_in_tree() && v_scroll->get_parent() == this) size.x -= v_scroll->get_minimum_size().x;
Erreur N6Un autre cas où un fragment de code suffisamment grand a été copié et modifié sans succès. La ligne d'erreur est marquée par mon commentaire "// <=".
void ShaderGLES2::bind_uniforms() { .... const Map<uint32_t, Variant>::Element *E = uniform_defaults.front(); while (E) { int idx = E->key(); int location = version->uniform_location[idx]; if (location < 0) { E = E->next(); continue; } Variant v; v = E->value(); _set_uniform_variant(location, v); E = E->next(); } const Map<uint32_t, CameraMatrix>::Element *C = uniform_cameras.front(); while (C) { int idx = E->key();
Avertissement PVS-Studio: V522 CWE-476 Le déréférencement du pointeur nul «E» peut avoir lieu. shader_gles2.cpp 102
L'erreur a été détectée indirectement. Grâce à l'analyse
du flux de données, PVS-Studio a révélé que le pointeur
E peut être nul au moment du déréférencement.
L'erreur est que dans le fragment de code copié, ils ont oublié de remplacer
E dans
C en un seul endroit. En raison de cette erreur, la fonction fonctionne de manière très étrange et fait des choses étranges.
Typos
Erreur N7Il est difficile pour les programmeurs qui écrivent dans des langages autres que C ou C ++ d'imaginer qu'une faute de frappe peut être faite en écrivant une virgule au lieu d'un astérisque *, et le code sera compilé. Mais c'est le cas.
LRESULT OS_Windows::WndProc(....) { .... BITMAPINFO bmi; ZeroMemory(&bmi, sizeof(BITMAPINFO)); bmi.bmiHeader.biSize = sizeof(BITMAPINFOHEADER); bmi.bmiHeader.biWidth = dib_size.x; bmi.bmiHeader.biHeight = dib_size.y; bmi.bmiHeader.biPlanes = 1; bmi.bmiHeader.biBitCount = 32; bmi.bmiHeader.biCompression = BI_RGB; bmi.bmiHeader.biSizeImage = dib_size.x, dib_size.y * 4; .... }
Avertissement PVS-Studio: V521 CWE-480 De telles expressions utilisant l'opérateur ',' sont dangereuses. Assurez-vous que l'expression est correcte. os_windows.cpp 776
La variable
bmi.bmiHeader.biSizeImage se voit attribuer la valeur de la variable
dib_size.x . Ensuite, l'opérateur virgule ',' est exécuté, dont la
priorité est inférieure à celle de l'opérateur '='. Le résultat de l'expression
dib_size.y * 4 n'est utilisé en aucune façon.
Au lieu d'une virgule dans l'expression, l'opérateur de multiplication '*' doit être utilisé. Premièrement, une telle expression aurait un sens. Deuxièmement, ci-dessous, il existe une option similaire, mais déjà correcte, pour initialiser la même variable:
bmi.bmiHeader.biSizeImage = dib_size.x * dib_size.y * 4;
Erreurs N8, N9 void Variant::set(....) { .... int idx = p_index; if (idx < 0) idx += 4; if (idx >= 0 || idx < 4) { Color *v = reinterpret_cast<Color *>(_data._mem); (*v)[idx] = p_value; valid = true; } .... }
Avertissement PVS-Studio: V547 CWE-571 Expression 'idx> = 0 || idx <4 'est toujours vrai. variant_op.cpp 2152
Tout index sera considéré comme correct. Pour corriger l'erreur, vous devez remplacer le
|| sur
&& :
if (idx >= 0 && idx < 4) {
Cette erreur logique est probablement due à la négligence, donc je suis enclin à l'attribuer aux fautes de frappe.
Exactement la même erreur peut être observée dans le même fichier ci-dessous. La faute de l'erreur de reproduction est apparemment le copier-coller.
Erreur multiple: V547 CWE-571 Expression 'idx> = 0 || idx <4 'est toujours vrai. variant_op.cpp 2527
Erreur N10Un exemple d'erreur dont on veut s'exclamer: WTF?!
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 "for". 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
Notez la condition d'arrêt de boucle:
i> p_at_index . C'est toujours vrai, car la variable
i est initialisée avec la valeur
blend_points_used - 1 . De plus, à partir de deux vérifications antérieures, il s'ensuit que
blend_points_used> p_at_index .
La condition ne peut devenir fausse que lorsqu'un débordement de la variable de signe
i se produit, ce qui est un comportement non défini. De plus, il n'atteindra pas le débordement, car le tableau dépassera la limite beaucoup plus tôt.
Avant nous, à mon avis, il y a une belle faute de frappe quand ils ont écrit «>» au lieu de «<». Oui, j'ai une vision perverse de la beauté des erreurs :).
Le bon cycle:
for (int i = blend_points_used - 1; i < p_at_index; i++) {
Erreur N11Un autre cas tout aussi brillant d'une faute de frappe dans l'état du cycle.
void AnimationNodeStateMachineEditor::_state_machine_pos_draw() { .... int idx = -1; for (int i = 0; node_rects.size(); i++) { if (node_rects[i].node_name == playback->get_current_node()) { idx = i; break; } } .... }
Avertissement PVS-Studio: V693 CWE-835 Envisagez d'inspecter l'expression conditionnelle de la boucle. Il est possible que 'i <X.size ()' soit utilisé au lieu de 'X.size ()'. animation_state_machine_editor.cpp 852
Un débordement du tableau peut se produire, car la valeur de
i augmente de façon incontrôlable. Code sécurisé:
for (int i = 0; i < node_rects.size(); i++) {
Erreur N12 GDScriptDataType GDScriptCompiler::_gdtype_from_datatype( const GDScriptParser::DataType &p_datatype) const { .... switch (p_datatype.kind) { .... case GDScriptParser::DataType::NATIVE: { result.kind = GDScriptDataType::NATIVE; result.native_type = p_datatype.native_type; } break; case GDScriptParser::DataType::SCRIPT: { result.kind = GDScriptDataType::SCRIPT; result.script_type = p_datatype.script_type; result.native_type = result.script_type->get_instance_base_type(); } case GDScriptParser::DataType::GDSCRIPT: { result.kind = GDScriptDataType::GDSCRIPT; result.script_type = p_datatype.script_type; result.native_type = result.script_type->get_instance_base_type(); } break; .... }
PVS-Studio Warning: V796 CWE-484 Il est possible que l'instruction 'break' soit manquante dans l'instruction switch. gdscript_compiler.cpp 135
J'ai accidentellement oublié d'écrire une déclaration de
rupture . Par conséquent, lorsqu'il entre dans le
cas GDScriptParser :: DataType :: SCRIPT, les variables se verront attribuer des valeurs, comme si c'était le
cas GDScriptParser :: DataType :: GDSCRIPT .
Erreur N13L'erreur suivante peut être classée comme copier-coller. Cependant, je ne sais pas si une ligne aussi courte a été copiée. Nous considérerons donc cela comme une simple faute de frappe lors de la frappe.
void CPUParticles::_particles_process(float p_delta) { .... if (flags[FLAG_DISABLE_Z]) { p.velocity.z = 0.0; p.velocity.z = 0.0; } .... }
Avertissement PVS-Studio: V519 CWE-563 La variable 'p.velocity.z' se voit attribuer des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 664, 665. cpu_particles.cpp 665
Deux fois l'affectation de la même variable. Ci-dessous, vous pouvez voir le fragment de code suivant:
if (flags[FLAG_DISABLE_Z]) { p.velocity.z = 0.0; p.transform.origin.z = 0.0; }
Très probablement, pour le premier cas, il aurait dû être écrit de la même manière.
Erreur N14 bool AtlasTexture::is_pixel_opaque(int p_x, int p_y) const { if (atlas.is_valid()) { return atlas->is_pixel_opaque( p_x + region.position.x + margin.position.x, p_x + region.position.y + margin.position.y ); } return true; }
Avertissement PVS-Studio: le paramètre V751 'p_y' n'est pas utilisé dans le corps de la fonction. texture.cpp 1085
Fragment de la description du diagnostic
V751 :
L'analyseur a détecté une fonction suspecte, dont l'un des paramètres n'est jamais utilisé. En même temps, son autre paramètre est utilisé plusieurs fois, ce qui, éventuellement, indique la présence d'une erreur.Comme vous pouvez le voir, c'est effectivement le cas, et c'est très suspect. La variable
p_x est utilisée deux fois et
p_y n'est pas utilisée. Très probablement, il devrait être écrit:
return atlas->is_pixel_opaque( p_x + region.position.x + margin.position.x, p_y + region.position.y + margin.position.y );
Soit dit en passant, dans le code source, l'appel de fonction est écrit sur une seule ligne. Pour cette raison, l'erreur est plus difficile à remarquer. Si l'auteur du code écrivait les arguments réels dans une colonne, comme je l'ai fait dans l'article, l'erreur retiendrait immédiatement mon attention. N'oubliez pas que le formatage des tableaux est très utile et évite beaucoup de fautes de frappe. Voir le chapitre «Aligner le même type de code avec un« tableau »dans l'article«
Le principal problème de programmation, de refactoring et tout ça ».
Erreur N15 bool SpriteFramesEditor::can_drop_data_fw(....) const { .... Vector<String> files = d["files"]; if (files.size() == 0) return false; for (int i = 0; i < files.size(); i++) { String file = files[0]; String ftype = EditorFileSystem::get_singleton()->get_file_type(file); if (!ClassDB::is_parent_class(ftype, "Texture")) { return false; } } .... }
PVS-Studio Warning: V767 Accès suspect à l'élément du tableau 'files' par un index constant dans une boucle. sprite_frames_editor_plugin.cpp 602
La boucle traite le même fichier à toutes les itérations de la boucle. Tapez ici:
String file = files[0];
Doit être:
String file = files[i];
Autres erreurs
Erreur N16 CSGBrush *CSGBox::_build_brush() { .... for (int i = 0; i < 6; i++) { .... if (i < 3) face_points[j][(i + k) % 3] = v[k] * (i >= 3 ? -1 : 1); else face_points[3 - j][(i + k) % 3] = v[k] * (i >= 3 ? -1 : 1); .... } .... }
L'analyseur PVS-Studio produit immédiatement deux réponses à ce code:
- V547 CWE-570 L'expression 'i> = 3' est toujours fausse. csg_shape.cpp 939
- V547 CWE-571 L'expression 'i> = 3' est toujours vraie. csg_shape.cpp 941
En effet, cet opérateur ternaire dans les deux expressions semble très étrange:
i >= 3 ? -1 : 1
Dans un cas, la condition est toujours vraie et dans l'autre, elle est fausse. Difficile de dire à quoi devrait ressembler ce code. Peut-être est-il simplement redondant et peut être écrit comme ceci:
for (int i = 0; i < 6; i++) { .... if (i < 3) face_points[j][(i + k) % 3] = v[k]; else face_points[3 - j][(i + k) % 3] = -v[k]; .... }
Je peux me tromper et le code doit être corrigé d'une manière complètement différente.
Erreur N17Il n'y avait presque pas d'erreurs comme V595, bien qu'elles soient généralement
trouvées en abondance dans tous les projets . Apparemment, ces erreurs ont été corrigées après une vérification précédente, puis des erreurs de ce type ne sont presque pas apparues. Je n'ai vu que quelques faux positifs et une erreur.
bool CanvasItemEditor::_get_bone_shape(....) { .... Node2D *from_node = Object::cast_to<Node2D>( ObjectDB::get_instance(bone->key().from)); .... if (!from_node->is_inside_tree()) return false;
Avertissement PVS-Studio: V595 CWE-476 Le pointeur 'from_node' a été utilisé avant d'être vérifié par rapport à nullptr. Vérifiez les lignes: 565, 567. canvas_item_editor_plugin.cpp 565
Le pointeur
from_node est d' abord déréférencé pour appeler la fonction
is_inside_tree, et ce n'est qu'ensuite qu'il est vérifié pour l'égalité
nullptr . Les chèques doivent être échangés:
if (!from_node) return false; if (!from_node->is_inside_tree()) return false;
Erreur N18 enum JoystickList { .... JOY_AXIS_MAX = 10, .... }; static const char *_axes[] = { "Left Stick X", "Left Stick Y", "Right Stick X", "Right Stick Y", "", "", "L2", "R2" }; int InputDefault::get_joy_axis_index_from_string(String p_axis) { for (int i = 0; i < JOY_AXIS_MAX; i++) { if (p_axis == _axes[i]) { return i; } } ERR_FAIL_V(-1); }
Avertissement PVS-Studio: le dépassement de la baie V557 CWE-125 est possible. La valeur de l'index «i» pourrait atteindre 9. input_default.cpp 1119
Le tableau
_axes se compose de huit éléments. Dans ce cas, la constante
JOY_AXIS_MAX , qui définit le nombre d'itérations de la boucle, est de 10. Il s'avère que la boucle dépasse la limite du tableau.
Erreur N19Et la dernière fonction très étrange, qui, apparemment, est conçue pour tester quelque chose. La fonction est longue, je vais donc la donner en image (cliquez sur l'image pour l'agrandir).
Avertissement PVS-Studio: V779 CWE-561 Code inaccessible détecté. Il est possible qu'une erreur soit présente. test_math.cpp 457
Il existe plusieurs
instructions de retour inconditionnelles dans une fonction. Dans l'image, je les ai marqués avec des ovales rouges. Il semble que plusieurs tests unitaires différents ont été collectés pour cette fonction, mais ont oublié de supprimer les
NULL de retour supplémentaires . Par conséquent, la fonction ne vérifie pas ce qu'elle doit vérifier. Presque tout le corps d'une fonction est constitué de code inaccessible.
Peut-être, bien sûr, c'est une sorte d'idée rusée. Mais il me semble que cela s'est produit par hasard et que le code devrait être corrigé.
Finissons là-dessus. Certes, si vous regardez attentivement le rapport de l'analyseur, vous pouvez trouver d'autres erreurs. Cependant, même écrit plus que suffisant pour écrire un article. Ensuite, cela deviendra ennuyeux pour moi et le lecteur :).
Conclusion
L'article décrit des erreurs qui n'existeraient pas si le code était régulièrement analysé à l'aide de PVS-Studio. Cependant, plus important encore, en utilisant une analyse régulière, on pourrait immédiatement trouver et éliminer de nombreuses autres erreurs. Mon collègue a décrit cette idée plus en détail dans sa note: «
Philosophie de l'analyse de code statique: nous avons 100 programmeurs, l'analyseur a trouvé peu d'erreurs, est-ce inutile? ". Je recommande fortement de passer 10 minutes à lire cet article court mais très important.
Merci de votre attention. J'invite tout le monde à télécharger et à essayer l'analyse statique de PVS-Studio pour tester vos propres projets.
Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: Andrey Karpov.
Godot: sur l'utilisation régulière des analyseurs statiques .