Il s'agit d'un article classique sur la façon dont notre équipe a testé un projet LibrePCB ouvert à l'aide de l'analyseur de code statique PVS-Studio. Cependant, l'article est intéressant en ce que la vérification a été effectuée à l'intérieur du conteneur Docker. Si vous utilisez des conteneurs, nous espérons que l'article montrera une autre manière simple d'intégrer l'analyseur dans le processus de développement.
LibrePCB
LibrePCB est un logiciel gratuit pour la conception de circuits électroniques et de circuits imprimés. Le code du programme est écrit en C ++, et Qt5 est utilisé pour construire l'interface graphique. Récemment, la première version officielle de cette application a eu lieu, qui a marqué la stabilisation de son propre format de fichier (* .lp, * .lplib). Paquets binaires préparés pour Linux, macOS et Windows.
LibrePCB est un petit projet ne contenant qu'environ 300 000 lignes de code non vides en C et C ++. Dans le même temps, 25% des lignes non vides sont des commentaires. Soit dit en passant, il s'agit d'un pourcentage important pour les commentaires. Cela est probablement dû au fait que le projet contient de nombreux petits fichiers, dont une partie importante est occupée par les commentaires de titre des informations du projet et de la licence. Code du site GitHub:
LibrePCB .
Le projet nous a semblé intéressant et nous avons décidé de le vérifier. Mais les résultats des tests n'étaient plus aussi intéressants. Oui, il y a eu de vraies erreurs. Mais il n'y avait rien de spécial sur lequel nous devons certainement dire aux lecteurs de nos articles. Peut-être ne voudrions-nous pas écrire un article et nous limiter à envoyer les erreurs aux développeurs du projet. Cependant, le point intéressant était que le projet a été testé à l'intérieur de l'image Docker, et nous avons donc décidé qu'il valait toujours la peine d'écrire un article.
Docker
Docker - logiciel pour automatiser le déploiement et la gestion des applications dans un environnement de virtualisation au niveau du système d'exploitation. Il vous permet de "conditionner" l'application avec tous ses environnements et dépendances dans un conteneur. Bien que cette technologie ait environ cinq ans et que de nombreuses entreprises aient longtemps implémenté Docker dans leur infrastructure de projet, dans le monde open source, cela n'était pas très visible jusqu'à récemment.
Notre entreprise travaille en étroite collaboration avec des projets open source, vérifiant le code source à l'aide de notre propre analyseur statique PVS-Studio. Actuellement,
plus de 300 projets ont été vérifiés. La chose la plus difficile dans cette activité a toujours été la compilation de projets d'autres personnes, mais l'introduction de conteneurs Docker a grandement simplifié ce processus.
La première expérience de test d'un projet open source dans Docker a été avec
Azure Service Fabric . Là, les développeurs ont fait l'installation du répertoire du fichier source dans le conteneur et l'intégration de l'analyseur s'est limitée à l'édition d'un des scripts exécutés dans le conteneur:
diff --git a/src/build.sh b/src/build.sh index 290c57d..2a286dc 100755 --- a/src/build.sh +++ b/src/build.sh @@ -193,6 +193,9 @@ BuildDir() cd ${ProjBinRoot}/build.${DirName} + pvs-studio-analyzer analyze --cfg /src/PVS-Studio.cfg \ + -o ./service-fabric-pvs.log -j4 + if [ "false" = ${SkipBuild} ]; then if (( $NumProc <= 0 )); then NumProc=$(($(getconf _NPROCESSORS_ONLN)+0))
La différence entre le projet LibrePCB est qu'ils ont immédiatement fourni le
Dockerfile pour la construction de l'image et du projet. Cela s'est avéré encore plus pratique pour nous. Voici la partie du fichier Docker qui nous intéresse:
FROM ubuntu:14.04 # install packages RUN DEBIAN_FRONTEND=noninteractive \ apt-get -q update \ && apt-get -qy upgrade \ && apt-get -qy install git g++ qt5-default qttools5-dev-tools qt5-doc \ qtcreator libglu1-mesa-dev dia \ && apt-get clean # checkout librepcb RUN git clone --recursive https:
Nous ne ferons pas la compilation et l'installation du projet lors de l'assemblage de l'image. Ainsi, nous avons collecté une image dans laquelle l'auteur du projet garantit le montage réussi du projet.
Après le démarrage du conteneur, l'analyseur a été installé et les commandes suivantes ont été exécutées pour générer et analyser le projet:
cd /opt/LibrePCB mkdir build && cd build qmake -r ../librepcb.pro pvs-studio-analyzer trace -- make -j2 pvs-studio-analyzer analyze -l /mnt/Share/PVS-Studio.lic -r /opt/LibrePCB \ -o /opt/LibrePCB/LibrePCB.log -v -j4 cp -R -L -a /opt/LibrePCB /mnt/Share
Soit dit en passant, toutes les actions ont été effectuées dans Windows 10. Il est très agréable que les développeurs de tous les systèmes d'exploitation populaires se développent également dans cette direction. Malheureusement, les conteneurs avec Windows ne sont pas aussi pratiques. Surtout en raison de l'impossibilité d'installer également des logiciels facilement.
Bogues trouvés
Maintenant, une section classique contenant une description des erreurs que nous avons trouvées en utilisant l'analyseur de code statique PVS-Studio. Soit dit en passant, saisissant cette opportunité, je tiens à vous rappeler que récemment nous développons un analyseur dans le sens de soutenir l'analyse de code pour les systèmes embarqués. Voici quelques articles que certains de nos lecteurs auraient pu ignorer:
- PVS-Studio inclut la prise en charge de la chaîne d'outils intégrée GNU Arm ;
- PVS-Studio: prise en charge des normes de codage MISRA C et MISRA C ++ .
Typos
SymbolPreviewGraphicsItem::SymbolPreviewGraphicsItem( const IF_GraphicsLayerProvider& layerProvider, const QStringList& localeOrder, const Symbol& symbol, const Component* cmp, const tl::optional<Uuid>& symbVarUuid, const tl::optional<Uuid>& symbVarItemUuid) noexcept { if (mComponent && symbVarUuid && symbVarItemUuid) .... if (mComponent && symbVarItemUuid && symbVarItemUuid)
Avertissement PVS-Studio:
V501 CWE-571 Il existe des sous-expressions identiques «symbVarItemUuid» à gauche et à droite de l'opérateur «&&». symbolpreviewgraphicsitem.cpp 74
Typo classique : la variable
symbVarItemUuid est vérifiée deux fois de suite. Il y a une vérification similaire ci-dessus, et en la regardant, il devient clair que la deuxième variable à vérifier doit être
symbVarUuid .
L'extrait de code de faute de frappe suivant:
void Clipper::DoMaxima(TEdge *e) { .... if (e->OutIdx >= 0) { AddOutPt(e, e->Top); e->OutIdx = Unassigned; } DeleteFromAEL(e); if (eMaxPair->OutIdx >= 0) { AddOutPt(eMaxPair, e->Top);
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 «eMaxPair» devrait être utilisée à la place de «e». clipper.cpp 2999
Très probablement, le code a été écrit à l'aide de Copy-Paste. En raison d'un oubli dans le deuxième bloc de texte, ils ont oublié de remplacer
e-> Top par
eMaxPair-> Top .
Contrôles supplémentaires
static int rndr_emphasis(hoedown_buffer *ob, const hoedown_buffer *content, const hoedown_renderer_data *data) { if (!content || !content->size) return 0; HOEDOWN_BUFPUTSL(ob, "<em>"); if (content) hoedown_buffer_put(ob, content->data, content->size); HOEDOWN_BUFPUTSL(ob, "</em>"); return 1; }
PVS-Studio Warning:
V547 CWE-571 Le 'contenu' de l'expression est toujours vrai. html.c 162
Ce n'est probablement pas encore une erreur, mais simplement du code redondant. Pas besoin de revérifier le pointeur de
contenu . S'il est nul, la fonction termine immédiatement son travail.
Une situation similaire:
void Clipper::DoMaxima(TEdge *e) { .... else if( e->OutIdx >= 0 && eMaxPair->OutIdx >= 0 ) { if (e->OutIdx >= 0) AddLocalMaxPoly(e, eMaxPair, e->Top); DeleteFromAEL(e); DeleteFromAEL(eMaxPair); } .... }
Avertissement PVS-Studio:
V547 CWE-571 L'expression 'e-> OutIdx> = 0' est toujours vraie. clipper.cpp 2983
Revérifier (e-> OutIdx> = 0) n'a pas de sens. Cependant, c'est peut-être une erreur. Par exemple, nous pouvons supposer que la variable
e-> Top doit être cochée. Cependant, ce n'est qu'une intuition. Nous ne connaissons pas le code du projet et ne pouvons pas distinguer les erreurs du code redondant :).
Et un autre cas:
QString SExpression::toString(int indent) const { .... if (child.isLineBreak() && nextChildIsLineBreak) { if (child.isLineBreak() && (i > 0) && mChildren.at(i - 1).isLineBreak()) {
PVS-Studio Warning:
V571 CWE-571
Contrôle récurrent. La condition 'child.isLineBreak ()' a déjà été vérifiée à la ligne 208. sexpression.cpp 209
Erreur de logique
void FootprintPreviewGraphicsItem::paint(....) noexcept { .... for (const Circle& circle : mFootprint.getCircles()) { layer = mLayerProvider.getLayer(*circle.getLayerName()); if (!layer) continue;
Avertissement PVS-Studio:
V547 CWE-571 L'expression "couche" est toujours vraie. footprintpreviewgraphicsitem.cpp 177
Puisque la condition dans la seconde
instruction if est toujours vraie, la branche
else n'est jamais satisfaite.
Vérification du pointeur oublié
extern int ZEXPORT unzGetGlobalComment ( unzFile file, char * szComment, uLong uSizeBuf) { .... if (uReadThis>0) { *szComment='\0'; if (ZREAD64(s->z_filefunc,s->filestream,szComment,uReadThis)!=uReadThis) return UNZ_ERRNO; } if ((szComment != NULL) && (uSizeBuf > s->gi.size_comment)) *(szComment+s->gi.size_comment)='\0'; .... }
Avertissement PVS-Studio:
V595 CWE-476 Le pointeur 'szComment' a été utilisé avant d'être vérifié par rapport à nullptr. Vérifier les lignes: 2068, 2073. dézipper.c 2068
Si
uReadThis> 0 , le pointeur
szComment est
déréférencé . C'est dangereux car ce pointeur peut être nul. L'analyseur tire une telle conclusion en se basant sur le fait que ce pointeur est également vérifié pour l'égalité
NULL .
Membre de classe non initialisé
template <class T> class Edge { public: using VertexType = Vector2<T>; Edge(const VertexType &p1, const VertexType &p2, T w=-1) : p1(p1), p2(p2), weight(w) {};
Avertissement PVS-Studio:
V730 CWE-457 Tous les membres d'une classe ne sont pas initialisés dans le constructeur. Pensez à inspecter: isBad. edge.h 14
Tous les constructeurs, à l'exception du premier, initialisent le champ de la classe
isBad . Très probablement, le premier constructeur a simplement accidentellement oublié de faire cette initialisation. Par conséquent, le premier constructeur crée un objet incomplètement initialisé, ce qui peut entraîner un comportement de programme non défini.
Il existe 11 autres
déclencheurs de diagnostic
V730 . Cependant, comme nous ne connaissons pas le code, il est difficile de dire si ces avertissements indiquent des problèmes réels. Je pense que ces avertissements sont mieux étudiés par les auteurs du projet.
Fuite de mémoire
template <typename ElementType> void ProjectLibrary::loadElements(....) { .... ElementType* element = new ElementType(elementDir, false);
Avertissement PVS-Studio:
V773 CWE-401 L'exception a été levée sans libérer le pointeur 'élément'. Une fuite de mémoire est possible. projectlibrary.cpp 245
Si un élément est déjà dans la liste, une exception sera levée. Mais cela ne détruit pas l'objet créé précédemment, dont le pointeur est stocké dans la variable d'
élément .
Type d'exception non valide
bool CmdRemoveSelectedSchematicItems::performExecute() { .... throw new LogicError(__FILE__, __LINE__); .... }
Avertissement PVS-Studio:
V1022 CWE-755 Une exception a été levée par le pointeur. Pensez plutôt à le jeter par valeur. cmdremoveselectedschematicitems.cpp 143
L'analyseur a détecté une exception levée par le pointeur. Le plus souvent, il est habituel de lever des exceptions par valeur et de les attraper par référence. Lancer un pointeur peut empêcher l'exception d'être interceptée, car elle sera interceptée par référence. En outre, l'utilisation d'un pointeur oblige l'intercepteur à appeler l'opérateur de
suppression pour détruire l'objet créé afin qu'aucune fuite de mémoire ne se produise.
En général, le
nouvel opérateur est écrit ici par accident et doit être supprimé. Que c'est une erreur est confirmé par le fait que dans tous les autres endroits, il est dit:
throw LogicError(__FILE__, __LINE__);
Utilisation dangereuse de dynamic_cast
void GraphicsView::handleMouseWheelEvent( QGraphicsSceneWheelEvent* event) noexcept { if (event->modifiers().testFlag(Qt::ShiftModifier)) .... } bool GraphicsView::eventFilter(QObject* obj, QEvent* event) { .... handleMouseWheelEvent(dynamic_cast<QGraphicsSceneWheelEvent*>(event)); .... }
Avertissement PVS-Studio:
V522 CWE-628 Le déréférencement de l'événement "pointeur nul" peut avoir lieu. Le pointeur null potentiel est transmis à la fonction 'handleMouseWheelEvent'. Inspectez le premier argument. Vérifiez les lignes: 143, 252. graphicsview.cpp 143
Le pointeur résultant de l'opérateur
dynamic_cast est transmis à la fonction
handleMouseWheelEvent . Dans ce document, ce pointeur est déréférencé sans vérification préalable.
Ceci est dangereux car l'opérateur
dynamic_cast peut retourner
nullptr . Il s'avère que ce code n'est pas mieux que d'utiliser simplement le
static_cast plus
rapide .
Une vérification explicite du pointeur doit être ajoutée à ce code avant utilisation.
De plus, ce type de code est très courant:
bool GraphicsView::eventFilter(QObject* obj, QEvent* event) { .... QGraphicsSceneMouseEvent* e = dynamic_cast<QGraphicsSceneMouseEvent*>(event); Q_ASSERT(e); if (e->button() == Qt::MiddleButton) .... }
Avertissement PVS-Studio:
V522 CWE-690 Il peut y avoir un déréférencement d'un pointeur nul potentiel 'e'. graphicsview.cpp 206
Le pointeur est vérifié à l'aide de la macro
Q_ASSERT . Voici sa description:
Imprime un message d'avertissement contenant le nom du fichier de code source et le numéro de ligne si le test est faux.
Q_ASSERT () est utile pour tester les pré- et post-conditions pendant le développement. Cela ne fait rien si QT_NO_DEBUG a été défini lors de la compilation.
|
Q_ASSERT est un mauvais moyen de vérifier les pointeurs avant utilisation. En règle générale, dans la version Release
QT_NO_DEBUG n'est pas défini. Je ne sais pas comment sont les choses dans le projet LibrePCB. Cependant, si
QT_NO_DEBUG est défini dans la version, il s'agit d'une solution étrange et non standard.
Si la macro se développe dans le vide, il s'avère qu'il n'y a pas de vérification. Et puis, on ne sait pas du tout pourquoi utiliser
dynamic_cast . Pourquoi ne pas utiliser
static_cast alors ?
En général, ce code est inodore et mérite un examen de tous les fragments de code similaires. Et il y en a d'ailleurs beaucoup.
J'ai compté 82 cas similaires!Conclusion
D'une manière générale, le projet LibrePCB nous a semblé de grande qualité. Néanmoins, nous recommandons que les auteurs du projet s'arment avec l'outil PVS-Studio et effectuent indépendamment une révision du code des sections de code indiquées par l'analyseur. Nous sommes prêts à leur fournir une licence gratuite pendant un mois pour une analyse complète du projet. De plus, ils peuvent utiliser l'option de licence gratuite de l'analyseur, car le code du projet est ouvert et publié sur le site Web de GitHub. Nous écrirons bientôt sur cette option de licence.
Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: Andrey Karpov, Svyatoslav Razmyslov.
Vérification de LibrePCB avec PVS-Studio dans un conteneur Docker .