Pendant toute la durée de leur existence, les gens ont déployé des efforts considérables pour étudier presque toute la zone du ciel étoilé. À ce jour, nous avons examiné des centaines de milliers d'astéroïdes, de comètes, de nébuleuses et d'étoiles, de galaxies et de planètes. Pour voir toute cette beauté vous-même, il n'est pas nécessaire de quitter la maison et de vous acheter un télescope. Vous pouvez installer Stellarium - un planétarium virtuel sur votre ordinateur, et regarder le ciel nocturne, confortablement allongé sur le canapé ... Mais est-ce confortable? Pour trouver la réponse à cette question, nous allons vérifier Stellarium pour les erreurs de code informatique.
Un peu sur le projet ...
Selon la
description du site Wikipedia,
Stellarium est un planétarium virtuel open source disponible pour Linux, Mac OS X, Microsoft Windows, Symbian, Android et iOS, ainsi que MeeGo. Le programme utilise les technologies OpenGL et Qt pour créer un ciel réaliste en temps réel. Avec Stellarium, vous pouvez voir ce que vous pouvez voir avec un télescope moyen et même grand. Le programme fournit également des observations des éclipses solaires et du mouvement des comètes.
Stellarium a été créé par le programmeur français Fabian Chereau, qui a lancé le projet à l'été 2001. Parmi les autres développeurs éminents figurent Robert Spearman, Johannes Gadzhozik, Matthew Gates, Timothy Reeves, Bogdan Marinov et Johan Meeris, qui est responsable de l'œuvre.
... et sur l'analyseur
L'analyse du projet a été réalisée à l'aide de l'analyseur de code statique PVS-Studio. Il s'agit d'un outil de détection d'erreurs et de vulnérabilités potentielles dans le code source des programmes écrits en C, C ++ et C # (bientôt en Java!). Il fonctionne sur Windows, Linux et macOS. Il est conçu pour ceux qui ont besoin d'améliorer la qualité de leur code.
L'analyse était assez simple. Tout d'abord, j'ai
téléchargé le projet Stellarium depuis GitHub, puis j'ai installé tous les packages nécessaires à l'assemblage. Étant donné que le projet est construit à l'aide de Qt Creator, j'ai utilisé le système de suivi du lancement du compilateur, qui est intégré à la version
autonome de l'analyseur. Vous pouvez y voir le rapport d'analyse terminé.
Les nouveaux lecteurs et utilisateurs de
Stellarium se sont peut-être demandé: pourquoi la licorne apparaît-elle dans le titre de l'article et comment est-elle liée à l'analyse de code? Je réponds: je suis l'un des développeurs de PVS-Studio, et la licorne est notre mascotte espiègle préférée. Alors, allez!
J'espère que grâce à cet article, les lecteurs apprendront quelque chose de nouveau par eux-mêmes, et les développeurs de Stellarium pourront éliminer certaines erreurs et améliorer la qualité du code.
Apportez-vous du café avec un croissant d'air et installez-vous confortablement, car nous allons à la partie la plus intéressante - un aperçu des résultats de l'analyse et de l'analyse des erreurs!
Conditions suspectes
Pour un plus grand plaisir de lecture, je suggère de ne pas regarder directement l'avertissement de l'analyseur, mais d'essayer ici et plus loin de trouver les erreurs vous-même.
void QZipReaderPrivate::scanFiles() { ....
Avertissement PVS-Studio: V654 La condition 'start_of_directory == - 1' de la boucle est toujours vraie. qzip.cpp 617
Pourriez-vous trouver une erreur? Si oui, louez-le.
L'erreur réside dans l'état de la
boucle while . C'est toujours vrai, car la variable
start_of_directory ne change pas dans le corps de la boucle. Très probablement, le cycle ne sera pas éternel, car il contient
retour et
pause , mais un tel code semble étrange.
Il me semble que dans le code ils ont oublié de faire l'affectation
start_of_directory = pos à l'endroit où la signature est vérifiée. Ensuite, la déclaration de
rupture est peut-être superflue. Dans ce cas, le code peut être réécrit comme ceci:
int i = 0; int start_of_directory = -1; EndOfDirectory eod; while (start_of_directory == -1) { const int pos = device->size() - int(sizeof(EndOfDirectory)) - i; if (pos < 0 || i > 65535) { qWarning() << "QZip: EndOfDirectory not found"; return; } device->seek(pos); device->read((char *)&eod, sizeof(EndOfDirectory)); if (readUInt(eod.signature) == 0x06054b50) start_of_directory = pos; ++i; }
Cependant, je ne suis pas sûr de ce que le code devrait être comme ça. Il est préférable que les développeurs du projet analysent eux-mêmes cette partie du programme et apportent les modifications nécessaires.
Une autre condition étrange:
class StelProjectorCylinder : public StelProjector { public: .... protected: .... virtual bool intersectViewportDiscontinuityInternal(const Vec3d& capN, double capD) const { static const SphericalCap cap1(1,0,0); static const SphericalCap cap2(-1,0,0); static const SphericalCap cap3(0,0,-1); SphericalCap cap(capN, capD); return cap.intersects(cap1) && cap.intersects(cap2) && cap.intersects(cap2); } };
Avertissement PVS-Studio: V501 Il existe des sous-expressions identiques «cap.intersects (cap2)» à gauche et à droite de l'opérateur «&&». StelProjectorClasses.hpp 175
Comme vous l'avez probablement déjà deviné, l'erreur réside dans la dernière ligne de la fonction: le programmeur a fait une faute de frappe, et à la fin il s'est avéré que la fonction renvoie le résultat quelle que soit la valeur de
cap3 .
Ce type d'erreur est extrêmement courant: dans presque tous les projets vérifiés, nous avons rencontré des fautes de frappe associées aux noms des formulaires
nom1 et
nom2 , etc. En règle générale, ces erreurs sont liées au copier-coller.
Cette instance de code est un excellent exemple d'un autre modèle d'erreur commun, pour lequel nous avons même mené une mini-étude distincte. Mon collègue Andrei Karpov l'a appelé «
l'effet de dernière ligne» . Si vous n'êtes pas familier avec ce matériel, je vous suggère d'ouvrir un onglet dans le navigateur pour le lire plus tard, mais pour l'instant, continuons.
void BottomStelBar::updateText(bool updatePos) { .... updatePos = true; .... if (location->text() != newLocation || updatePos) { updatePos = true; .... } .... if (fov->text() != str) { updatePos = true; .... } .... if (fps->text() != str) { updatePos = true; .... } if (updatePos) { .... } }
Avertissements de PVS-Studio:- V560 Une partie de l'expression conditionnelle est toujours vraie: updatePos. StelGuiItems.cpp 732
- V547 L' expression 'updatePos' est toujours vraie. StelGuiItems.cpp 831
- V763 Le paramètre 'updatePos' est toujours réécrit dans le corps de la fonction avant d'être utilisé. StelGuiItems.cpp 690
La valeur du paramètre
updatePos est toujours écrasée avant d'être utilisée, c'est-à-dire la fonction fonctionnera de la même manière, quelle que soit la valeur qui lui est transmise.
Ça a l'air bizarre, non? Dans tous les endroits où le paramètre
updatePos est
impliqué , c'est
vrai . Cela signifie que les conditions
if (location-> text ()! = NewLocation || updatePos) et
if (updatePos) seront toujours vraies.
Un autre extrait:
void LandscapeMgr::onTargetLocationChanged(StelLocation loc) { .... if (pl && flagEnvironmentAutoEnabling) { QSettings* conf = StelApp::getInstance().getSettings(); setFlagAtmosphere(pl->hasAtmosphere() & conf->value("landscape/flag_atmosphere", true).toBool()); setFlagFog(pl->hasAtmosphere() & conf->value("landscape/flag_fog", true).toBool()); setFlagLandscape(true); } .... }
Avertissements de PVS-Studio:- V792 La fonction 'toBool' située à droite de l'opérateur '&' sera appelée quelle que soit la valeur de l'opérande gauche. Peut-être vaut-il mieux utiliser '&&'. LandscapeMgr.cpp 782
- V792 La fonction 'toBool' située à droite de l'opérateur '&' sera appelée quelle que soit la valeur de l'opérande gauche. Peut-être vaut-il mieux utiliser '&&'. LandscapeMgr.cpp 783
L'analyseur a détecté une expression suspecte dans les arguments des fonctions
setFlagAtmosphere et
setFlagFog . En effet: des deux côtés de l'opérateur de bit
& il y a des valeurs de type
bool . Au lieu de l'opérateur
& , vous devez utiliser l'opérateur
&& , et maintenant je vais vous expliquer pourquoi.
Oui, le résultat de cette expression sera toujours correct. Avant d'utiliser les bits «et», les deux opérandes seront promus
int . En C ++, une telle conversion est
sans ambiguïté : false est converti en 0 et true est converti en 1. Par conséquent, le résultat de cette expression sera le même que si l'opérateur
&& était utilisé.
Mais il y a une nuance. Lors du calcul du résultat de l'opération
&& , le soi-disant "calcul paresseux" est utilisé. Si la valeur de l'opérande gauche est
fausse , alors la valeur droite n'est même pas calculée, car le "et" logique retournera en tout cas
faux . Ceci est fait pour économiser les ressources informatiques et vous permet d'écrire des conceptions plus complexes. Par exemple, vous pouvez vérifier que le pointeur n'est pas nul et, dans l'affirmative, le déréférencer pour effectuer une vérification supplémentaire. Exemple:
if (ptr && ptr-> foo ()) .
Ce "calcul paresseux" n'est pas effectué à l'aide de l'opérateur au niveau du bit
& : les expressions
conf-> value ("...", true) .toBool () seront évaluées à chaque fois, quelle que soit la valeur
pl-> hasAtmosphere () .
Dans de rares cas, cela est fait exprès. Par exemple, si le calcul de l'opérande droit a des «effets secondaires», le résultat est utilisé plus tard. Ce n'est pas très bon de le faire non plus, car cela complique la compréhension du code et complique sa maintenance. De plus, l'ordre de calcul des opérandes
& n'est pas défini, donc dans certains cas d'utilisation de telles "astuces", vous pouvez obtenir un comportement indéfini.
Si vous devez enregistrer des effets secondaires - faites-le sur une ligne distincte et enregistrez le résultat dans une variable distincte. Les personnes qui travailleront avec ce code à l'avenir vous seront reconnaissantes :)
Nous passons au sujet suivant.
Gestion incorrecte de la mémoire
Commençons le sujet de la mémoire dynamique avec ce fragment:
GLUEShalfEdge* __gl_meshMakeEdge(GLUESmesh* mesh) { GLUESvertex* newVertex1 = allocVertex(); GLUESvertex* newVertex2 = allocVertex(); GLUESface* newFace = allocFace(); GLUEShalfEdge* e; if ( newVertex1 == NULL || newVertex2 == NULL || newFace == NULL) { if (newVertex1 != NULL) { memFree(newVertex1); } if (newVertex2 != NULL) { memFree(newVertex2); } if (newFace != NULL) { memFree(newFace); } return NULL; } e = MakeEdge(&mesh->eHead); if (e == NULL) { return NULL; } MakeVertex(newVertex1, e, &mesh->vHead); MakeVertex(newVertex2, e->Sym, &mesh->vHead); MakeFace(newFace, e, &mesh->fHead); return e; }
Avertissements de PVS-Studio:- V773 La fonction a été fermée sans libérer le pointeur 'newVertex1'. Une fuite de mémoire est possible. mesh.c 312
- V773 La fonction a été fermée sans libérer le pointeur 'newVertex2'. Une fuite de mémoire est possible. mesh.c 312
- V773 La fonction a été quittée sans libérer le pointeur 'newFace'. Une fuite de mémoire est possible. mesh.c 312
La fonction alloue de la mémoire à plusieurs structures et la transmet aux pointeurs
newVertex1 ,
newVertex2 (noms intéressants, non?) Et
newFace . Si l'un d'eux s'avère être nul, alors toute la mémoire réservée à l'intérieur de la fonction est libérée, après quoi le flux de contrôle quitte la fonction.
Que se passe-t-il si la mémoire des trois structures est allouée correctement et que la fonction
MakeEdge (& mesh-> eHead) renvoie
NULL ? Le flux de contrôle atteindra le deuxième
retour .
Étant donné que les pointeurs
newVertex1 ,
newVertex2 et
newFace sont des variables locales, ils cesseront d'exister après avoir quitté la fonction. Mais la libération de la mémoire qui leur appartenait n'aura pas lieu. Il restera réservé, mais nous n'y aurons plus accès.
De telles situations sont appelées fuites de mémoire. Un scénario typique avec une telle erreur: avec une utilisation prolongée du programme, il commence à consommer de plus en plus de RAM, jusqu'à son épuisement.
Il est à noter que dans cet exemple, le troisième
retour n'est pas erroné. Les fonctions
MakeVertex et
MakeFace transfèrent les adresses de la mémoire allouée à d'autres structures de données, déléguant ainsi la responsabilité de sa libération.
Le prochain défaut est dans la méthode, qui prend 90 lignes. Pour plus de commodité, je l'ai réduit, ne laissant que des zones à problèmes.
void AstroCalcDialog::drawAngularDistanceGraph() { .... QVector<double> xs, ys; .... }
Il ne reste qu'une ligne. Permettez-moi de vous donner un indice: c'est la seule mention des objets
xs et
ys .
Avertissements de PVS-Studio:- L'objet 'xs' V808 de type 'QVector' a été créé mais n'a pas été utilisé. AstroCalcDialog.cpp 5329
- L'objet V808 'ys' de type 'QVector' a été créé mais n'a pas été utilisé. AstroCalcDialog.cpp 5329
Les vecteurs
xs et
ys sont créés, mais ne sont utilisés nulle part. Il s'avère que chaque fois que vous utilisez la méthode
drawAngularDistanceGraph , une création et une suppression supplémentaires d'un conteneur vide se produisent. Je pense que cette annonce est restée dans le code après refactoring. Bien sûr, ce n'est pas une erreur, mais vous devez supprimer le code supplémentaire.
Moulages de type étrange
Un autre exemple après un peu de mise en forme ressemble à ceci:
void SatellitesDialog::updateSatelliteData() { ....
Afin de comprendre quelle est l'erreur, vous devrez regarder les prototypes
des constructeurs de classe Qcolor :
Avertissements de PVS-Studio:- V674 Le littéral '0.4' de type 'double' est implicitement converti en type 'int' lors de l'appel de la fonction 'QColor'. Inspectez le premier argument. SatellitesDialog.cpp 413
- V674 Le littéral '0.4' de type 'double' est implicitement converti en type 'int' lors de l'appel de la fonction 'QColor'. Inspectez le deuxième argument. SatellitesDialog.cpp 413
- V674 Le littéral '0.4' de type 'double' est implicitement converti en type 'int' lors de l'appel de la fonction 'QColor'. Inspectez le troisième argument. SatellitesDialog.cpp 413
La classe
Qcolor n'a pas de constructeurs qui acceptent le type
double , donc les arguments de l'exemple seront implicitement convertis en
int . Cela fait que les champs
r ,
g ,
b de l'objet
buttonColor ont une valeur de
0 .
Si le programmeur avait l'intention de créer un objet à partir de valeurs de type
double , il devrait utiliser un constructeur différent.
Par exemple, vous pouvez utiliser un constructeur qui accepte
Qrgb en écrivant:
buttonColor = QColor(QColor::fromRgbF(0.4, 0.4, 0.4));
Cela aurait pu être fait différemment. Qt utilise des valeurs réelles dans la plage [0,0, 1,0] ou des valeurs entières dans la plage [0, 255] pour indiquer les couleurs RVB.
Par conséquent, le programmeur pourrait traduire les valeurs de réel en entier en écrivant comme ceci:
buttonColor = QColor((int)(255 * 0.4), (int)(255 * 0.4), (int)(255 * 0.4));
ou juste
buttonColor = QColor(102, 102, 102);
Êtes-vous ennuyé? Ne vous inquiétez pas: des erreurs plus intéressantes nous attendent.
"Licorne dans l'espace." Vue depuis le Stellarium.Autres erreurs
En fin de compte, je vous ai laissé un peu plus délicieux :) Passons à l'un d'eux.
HipsTile* HipsSurvey::getTile(int order, int pix) { .... if (order == orderMin && !allsky.isNull()) { int nbw = sqrt(12 * 1 << (2 * order)); int x = (pix % nbw) * allsky.width() / nbw; int y = (pix / nbw) * allsky.width() / nbw; int s = allsky.width() / nbw; QImage image = allsky.copy(x, y, s, s); .... } .... }
PVS-Studio Warning: V634 La priorité de l'opération '*' est supérieure à celle de l'opération '<<'. Il est possible que des parenthèses soient utilisées dans l'expression. StelHips.cpp 271
Eh bien, avez-vous pu détecter l'erreur? Considérez plus en détail l'expression
(12 * 1 << (ordre 2 *)) . L'analyseur rappelle que l'opération '
* ' a une priorité plus élevée que l'opération de décalage de bit '
<< '. Il est facile de voir que multiplier
12 par
1 est inutile, et les crochets autour de l'
ordre 2 * ne
sont pas nécessaires.
, : int nbw = sqrt(12 * (1 << 2 * order)); <i>12 </i> .
Remarque De plus, je tiens à noter que si la valeur de l'opérande de droite '
<< ' est supérieure ou égale au nombre de bits de l'opérande de gauche, le résultat n'est pas défini. Étant donné que les littéraux numériques sont
int par défaut, ce qui prend
32 bits, la valeur du paramètre
order ne doit pas dépasser
15 . Sinon, l'évaluation de l'expression peut entraîner un comportement indéfini.
Nous continuons. La méthode ci-dessous est très déroutante, mais je suis sûr qu'un lecteur sophistiqué gérera la détection d'erreur :)
QCPRange QCPStatisticalBox:: getKeyRange(bool& foundRange, SignDomain inSignDomain) const { foundRange = true; if (inSignDomain == sdBoth) { return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5); } else if (inSignDomain == sdNegative) { if (mKey + mWidth * 0.5 < 0) return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5); else if (mKey < 0) return QCPRange(mKey - mWidth * 0.5, mKey); else { foundRange = false; return QCPRange(); } } else if (inSignDomain == sdPositive) { if (mKey - mWidth * 0.5 > 0) return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5); else if (mKey > 0) return QCPRange(mKey, mKey + mWidth * 0.5); else { foundRange = false; return QCPRange(); } } foundRange = false; return QCPRange(); }
Avertissement PVS-Studio: V779 Code inaccessible détecté. Il est possible qu'une erreur soit présente. qcustomplot.cpp 19512.
Le fait est que toutes les succursales
if ... else ont un
retour . Par conséquent, le flux de contrôle n'atteint jamais les deux dernières lignes.
Dans l'ensemble, cet exemple fonctionnera normalement et fonctionnera correctement. Mais la seule présence d'un code inaccessible est un signal. Dans ce cas, il indique la structure incorrecte de la méthode, ce qui complique grandement la lisibilité et l'intelligibilité du code.
Ce fragment de code devrait être refactorisé, obtenant une fonction plus soignée à la sortie. Par exemple, comme ceci:
QCPRange QCPStatisticalBox:: getKeyRange(bool& foundRange, SignDomain inSignDomain) const { foundRange = true; switch (inSignDomain) { case sdBoth: { return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5); break; } case sdNegative: { if (mKey + mWidth * 0.5 < 0) return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5); else if (mKey < 0) return QCPRange(mKey - mWidth * 0.5, mKey); break; } case sdPositive: { if (mKey - mWidth * 0.5 > 0) return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5); else if (mKey > 0) return QCPRange(mKey, mKey + mWidth * 0.5); break; } } foundRange = false; return QCPRange(); }
La dernière de notre critique sera l'erreur que j'ai le plus appréciée. Le code spot d'incident est court et simple:
Plane::Plane(Vec3f &v1, Vec3f &v2, Vec3f &v3) : distance(0.0f), sDistance(0.0f) { Plane(v1, v2, v3, SPolygon::CCW); }
Avez-vous remarqué quelque chose de suspect? Tout le monde ne peut pas :)
PVS-Studio Warning: 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
Le programmeur s'attendait à ce que certains champs de l'objet soient initialisés à l'intérieur du constructeur imbriqué, mais cela s'est avéré ainsi: lorsque le constructeur
Plane (Vec3f & v1, Vec3f & v2, Vec3f & v3) est appelé, un objet temporaire sans nom est créé à l'intérieur, qui est immédiatement supprimé. Par conséquent, une partie de l'objet reste non initialisée.
Pour que le code fonctionne correctement, vous devez utiliser la fonctionnalité pratique et sûre de C ++ 11 - le constructeur délégué:
Plane::Plane(Vec3f& v1, Vec3f& v2, Vec3f& v3) : Plane(v1, v2, v3, SPolygon::CCW) { distance = 0.0f; sDistance = 0.0f; }
Mais si vous utilisez le compilateur pour les anciennes versions de la langue, vous pouvez écrire comme ceci:
Plane::Plane(Vec3f &v1, Vec3f &v2, Vec3f &v3) : distance(0.0f), sDistance(0.0f) { this->Plane::Plane(v1, v2, v3, SPolygon::CCW); }
Ou alors:
Plane::Plane(Vec3f &v1, Vec3f &v2, Vec3f &v3) : distance(0.0f), sDistance(0.0f) { new (this) Plane(v1, v2, v3, SPolygon::CCW); }
Je note que les deux dernières méthodes sont
très dangereuses . Par conséquent, vous devez être très prudent et bien comprendre comment ces méthodes fonctionnent.
Conclusion
Quelles conclusions peut-on tirer de la qualité du code Stellarium? Honnêtement, il n'y a pas eu beaucoup d'erreurs. De plus, dans l'ensemble du projet, je n'ai trouvé aucune erreur dans laquelle le code est lié à un comportement non défini. Pour le projet opensource, la qualité du code s'est avérée être à un niveau élevé, pour lequel je porte mon chapeau aux développeurs. Vous êtes super! J'ai été ravi et intéressé de revoir votre projet.
Qu'en est-il du planétarium lui-même - je l'utilise assez souvent. Malheureusement, vivant dans une ville, je peux rarement profiter d'un ciel nocturne clair et Stellarium me permet d'être n'importe où dans le monde sans me lever du canapé. C'est vraiment confortable!
J'aime particulièrement le mode «Constellation art». La vue des énormes personnages couvrant tout le ciel dans une étrange danse est à couper le souffle.
"Une danse étrange." Vue depuis le Stellarium.Les terriens ont tendance à faire des erreurs, et il n'y a rien de honteux dans le fait que ces erreurs fuient dans le code. Pour cela, des outils d'analyse de code, tels que PVS-Studio, sont développés. Si vous êtes l'un des terriens -
comme ça, je vous suggère de le
télécharger et de l'essayer vous-même .
J'espère que vous avez été intéressé par la lecture de mon article et que vous avez appris quelque chose de nouveau et d'utile pour vous-même. Et je souhaite aux développeurs une correction précoce des erreurs trouvées.
Abonnez-vous à nos chaînes et restez à l'écoute pour les nouvelles du monde de la programmation!
Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: George Gribkov.
Into Space Again: comment la licorne a visité le stellarium