Este es un artículo clásico sobre cómo nuestro equipo probó un proyecto abierto de LibrePCB utilizando el analizador de código estático PVS-Studio. Sin embargo, el artículo es interesante porque la verificación se realizó dentro del contenedor Docker. Si usa contenedores, esperamos que el artículo demuestre otra forma simple de integrar el analizador en el proceso de desarrollo.
LibrePCB
LibrePCB es un software gratuito para diseñar circuitos electrónicos y placas de circuito impreso. El código del programa está escrito en C ++, y Qt5 se usa para construir la interfaz gráfica. Recientemente, tuvo lugar el primer lanzamiento oficial de esta aplicación, que marcó la estabilización de su propio formato de archivo (* .lp, * .lplib). Paquetes binarios preparados para Linux, macOS y Windows.
LibrePCB es un pequeño proyecto que contiene solo alrededor de 300,000 líneas de código no vacías en C y C ++. Al mismo tiempo, el 25% de las líneas no vacías son comentarios. Por cierto, este es un gran porcentaje de comentarios. Lo más probable es que esto se deba al hecho de que el proyecto tiene muchos archivos pequeños, una parte importante de los cuales está ocupada por encabezar los comentarios de la información del proyecto y la licencia. Código del sitio de GitHub:
LibrePCB .
El proyecto nos pareció interesante y decidimos comprobarlo. Pero los resultados de la prueba ya no eran tan interesantes. Sí, hubo errores reales. Pero no había nada especial sobre lo que sin duda debemos decirles a los lectores de nuestros artículos. Quizás no escribiríamos un artículo y nos limitamos a enviar los errores a los desarrolladores del proyecto. Sin embargo, lo interesante fue que el proyecto se probó dentro de la imagen de Docker y, por lo tanto, decidimos que valía la pena escribir un artículo.
Docker
Docker : software para automatizar la implementación y administración de aplicaciones en un entorno de virtualización a nivel del sistema operativo. Le permite "empaquetar" la aplicación con todo su entorno y dependencias en un contenedor. Aunque esta tecnología tiene aproximadamente cinco años y muchas compañías han implementado Docker en su infraestructura de proyectos, en el mundo del código abierto esto no era muy notable hasta hace poco.
Nuestra compañía trabaja muy de cerca con proyectos de código abierto, verificando el código fuente usando nuestro propio analizador estático PVS-Studio. Actualmente, se han verificado
más de 300 proyectos . Lo más difícil en esta actividad siempre ha sido la compilación de proyectos de otras personas, pero la introducción de contenedores Docker simplificó enormemente este proceso.
La primera experiencia de probar un proyecto de código abierto en Docker fue con
Azure Service Fabric . Allí, los desarrolladores hicieron la instalación del directorio del archivo fuente en el contenedor y la integración del analizador se limitó a editar uno de los scripts que se ejecutan en el contenedor:
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 diferencia entre el proyecto LibrePCB es que de inmediato proporcionaron el
Dockerfile para construir la imagen y el proyecto en él. Resultó ser aún más conveniente para nosotros. Aquí está la parte del archivo Docker que nos interesa:
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:
No haremos la compilación e instalación del proyecto al ensamblar la imagen. Por lo tanto, recopilamos una imagen en la que el autor del proyecto garantiza el montaje exitoso del proyecto.
Después de iniciar el contenedor, se instaló el analizador y se ejecutaron los siguientes comandos para construir y analizar el proyecto:
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
Por cierto, todas las acciones se realizaron en Windows 10. Es muy agradable que los desarrolladores de todos los sistemas operativos populares también se estén desarrollando en esta dirección. Desafortunadamente, los contenedores con Windows no son tan convenientes. Especialmente debido a la imposibilidad de instalar también software fácilmente.
Errores encontrados
Ahora una sección clásica que contiene una descripción de los errores que encontramos usando el analizador de código estático PVS-Studio. Por cierto, aprovechando esta oportunidad, quiero recordarles que recientemente estamos desarrollando un analizador en la dirección de apoyar el análisis de código para sistemas embebidos. Aquí hay un par de artículos que algunos de nuestros lectores podrían haber omitido:
- PVS-Studio incluye soporte para GNU Arm Embedded Toolchain ;
- PVS-Studio: soporte para los estándares de codificación MISRA C y MISRA C ++ .
Errores tipográficos
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)
Advertencia de PVS-Studio:
V501 CWE-571 Hay
subexpresiones idénticas 'symbVarItemUuid' a la izquierda y a la derecha del operador '&&'. symbolpreviewgraphicsitem.cpp 74
Error tipográfico
clásico : la variable
symbVarItemUuid se verifica dos veces seguidas. Hay una verificación similar arriba, y al mirarla, queda claro que la segunda variable a verificar debe ser
symbVarUuid .
El siguiente fragmento de código tipográfico:
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);
Advertencia de PVS-Studio:
V778 CWE-682 Se encontraron dos fragmentos de código similares. Quizás, este es un error tipográfico y se debe usar la variable 'eMaxPair' en lugar de 'e'. clipper.cpp 2999
Lo más probable es que el código fue escrito usando Copy-Paste. Debido a un descuido en el segundo bloque de texto, olvidaron reemplazar
e-> Top con
eMaxPair-> Top .
Cheques adicionales
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; }
Advertencia de PVS-Studio:
V547 CWE-571 El "contenido" de la expresión siempre es verdadero. html.c 162
Esto probablemente todavía no sea un error, sino simplemente un código redundante. No es necesario volver a verificar el puntero de
contenido . Si es cero, entonces la función termina inmediatamente su trabajo.
Una situación similar:
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); } .... }
Advertencia de PVS-Studio:
V547 CWE-571 La expresión 'e-> OutIdx> = 0' siempre es verdadera. clipper.cpp 2983
Volver a verificar (e-> OutIdx> = 0) no tiene sentido. Sin embargo, quizás esto sea un error. Por ejemplo, podemos suponer que la variable
e-> Top debería estar marcada. Sin embargo, esto es solo una corazonada. No estamos familiarizados con el código del proyecto y no podemos distinguir los errores del código redundante :).
Y otro caso:
QString SExpression::toString(int indent) const { .... if (child.isLineBreak() && nextChildIsLineBreak) { if (child.isLineBreak() && (i > 0) && mChildren.at(i - 1).isLineBreak()) {
Advertencia de PVS-Studio:
V571 CWE-571 Comprobación
periódica . La condición 'child.isLineBreak ()' ya se verificó en la línea 208. sexpression.cpp 209
Error en la logica
void FootprintPreviewGraphicsItem::paint(....) noexcept { .... for (const Circle& circle : mFootprint.getCircles()) { layer = mLayerProvider.getLayer(*circle.getLayerName()); if (!layer) continue;
Advertencia de PVS-Studio:
V547 CWE-571 La expresión 'capa' siempre es verdadera. footprintpreviewgraphicsitem.cpp 177
Como la condición en la segunda
instrucción if es siempre cierta, la rama
else nunca se cumple.
Comprobación de puntero olvidada
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'; .... }
Advertencia de PVS-Studio:
V595 CWE-476 El puntero 'szComment' se utilizó antes de verificarlo con nullptr. Líneas de verificación: 2068, 2073. unzip.c 2068
Si
uReadThis> 0 , entonces el puntero
szComment está
desreferenciado . Esto es peligroso ya que este puntero puede ser nulo. El analizador llega a tal conclusión basándose en el hecho de que además este puntero se verifica para la igualdad
NULL .
Miembro de clase no inicializado
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) {};
Advertencia de PVS-Studio:
V730 CWE-457 No todos los miembros de una clase se inicializan dentro del constructor. Considere inspeccionar: isBad. edge.h 14
Todos los constructores, excepto el primero, inicializan el campo de la clase
isBad . Lo más probable es que el primer constructor simplemente se haya olvidado accidentalmente de hacer esta inicialización. Como resultado, el primer constructor crea un objeto inicializado de forma incompleta, lo que puede conducir a un comportamiento indefinido del programa.
Hay 11
desencadenantes de diagnóstico
V730 más. Sin embargo, dado que no estamos familiarizados con el código, es difícil decir si estas advertencias indican problemas reales. Creo que estas advertencias son mejor estudiadas por los autores del proyecto.
Pérdida de memoria
template <typename ElementType> void ProjectLibrary::loadElements(....) { .... ElementType* element = new ElementType(elementDir, false);
Advertencia de PVS-Studio:
V773 CWE-401 Se lanzó la excepción sin soltar el puntero 'elemento'. Una pérdida de memoria es posible. projectlibrary.cpp 245
Si un elemento ya está en la lista, se lanzará una excepción. Pero esto no destruye el objeto creado previamente, el puntero al cual se almacena en la variable del
elemento .
Tipo de excepción inválido
bool CmdRemoveSelectedSchematicItems::performExecute() { .... throw new LogicError(__FILE__, __LINE__); .... }
Advertencia de PVS-Studio:
V1022 CWE-755 El puntero arrojó una excepción. Considere lanzarlo por valor en su lugar. cmdremoveselectedschematicitems.cpp 143
El analizador detectó una excepción lanzada por el puntero. La mayoría de las veces se acostumbra lanzar excepciones por valor y capturar por referencia. Lanzar un puntero puede hacer que la excepción no se detecte, ya que se detectará por referencia. Además, el uso de un puntero obliga al interceptor a llamar al operador de
eliminación para destruir el objeto creado para que no se produzcan pérdidas de memoria.
En general, el
nuevo operador se escribe aquí por accidente y debe eliminarse. El hecho de que esto es un error se confirma por el hecho de que en todos los demás lugares dice:
throw LogicError(__FILE__, __LINE__);
Uso peligroso 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)); .... }
Advertencia de PVS-Studio:
V522 CWE-628 Puede tener lugar la desreferenciación del puntero nulo 'evento'. El puntero nulo potencial se pasa a la función 'handleMouseWheelEvent'. Inspecciona el primer argumento. Líneas de verificación: 143, 252. graphicsview.cpp 143
El puntero resultante del operador
dynamic_cast se pasa a la función
handleMouseWheelEvent . En él, este puntero se desreferencia sin verificación previa.
Esto es peligroso ya que el operador
dynamic_cast puede devolver
nullptr . Resulta que este código no es mejor que solo usar el
static_cast más
rápido .
Se debe agregar una comprobación de puntero explícita a este código antes de su uso.
Además, el código de este tipo es muy común:
bool GraphicsView::eventFilter(QObject* obj, QEvent* event) { .... QGraphicsSceneMouseEvent* e = dynamic_cast<QGraphicsSceneMouseEvent*>(event); Q_ASSERT(e); if (e->button() == Qt::MiddleButton) .... }
Advertencia de PVS-Studio:
V522 CWE-690 Puede haber una desreferenciación de un puntero nulo potencial 'e'. graphicsview.cpp 206
El puntero se verifica utilizando la macro
Q_ASSERT . Aquí está su descripción:
Imprime un mensaje de advertencia que contiene el nombre del archivo de código fuente y el número de línea si la prueba es falsa.
Q_ASSERT () es útil para probar condiciones previas y posteriores durante el desarrollo. No hace nada si QT_NO_DEBUG se definió durante la compilación.
|
Q_ASSERT es una mala manera de verificar los punteros antes de usarlos. Como regla general, en la versión Release
QT_NO_DEBUG no
está definido. No sé cómo están las cosas en el proyecto LibrePCB. Sin embargo, si
QT_NO_DEBUG se define en la versión, entonces esta es una solución extraña y no estándar.
Si la macro se expande hacia el vacío, resulta que no hay verificación. Y luego no está claro por qué usar
dynamic_cast en absoluto. ¿Por qué no usar
static_cast entonces ?
En general, este código es inodoro y merece una revisión de todos los fragmentos de código similares. Y hay, por cierto, muchos de ellos.
¡Conté 82 casos similares!Conclusión
En general, el proyecto LibrePCB nos pareció de alta calidad. Sin embargo, recomendamos que los autores del proyecto se armen con la herramienta PVS-Studio y realicen independientemente una Revisión de Código de las secciones de código indicadas por el analizador. Estamos listos para proporcionarles una licencia gratuita durante un mes para un análisis completo del proyecto. Además, pueden usar la opción de licencia gratuita del analizador, ya que el código del proyecto está abierto y publicado en el sitio web de GitHub. Pronto escribiremos sobre esta opción de licencia.
Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: Andrey Karpov, Svyatoslav Razmyslov.
Comprobación de LibrePCB con PVS-Studio dentro de un contenedor Docker .