Errores que el análisis de código estático no encuentra porque no se usa

Los lectores de nuestros artículos ocasionalmente notan que el analizador de código estático PVS-Studio detecta una gran cantidad de errores que son insignificantes y que no afectan la aplicación. Realmente es así. En su mayor parte, los errores importantes ya se han solucionado debido a las pruebas manuales, los comentarios de los usuarios y otros métodos costosos. Al mismo tiempo, muchos de estos errores podrían haberse encontrado en la etapa de escritura del código y corregidos con una pérdida mínima de tiempo, reputación y dinero. Este artículo proporcionará varios ejemplos de errores reales, que podrían haberse solucionado de inmediato, si los autores del proyecto hubieran utilizado el análisis de código estático.

La idea es muy simple. Buscaremos ejemplos de solicitudes de extracción en GitHub que especifiquen que un problema es una corrección de errores. Luego intentaremos encontrar estos errores utilizando el analizador de código estático PVS-Studio. Si el analizador puede encontrar un error, entonces es un error que podría haberse encontrado en la etapa de escritura del código. Cuanto antes se corrige el error, más barato cuesta.

Desafortunadamente, GitHub nos decepcionó y no logramos hacer un gran artículo elegante sobre el tema. GitHub tiene un problema técnico (o una característica) que no le permite buscar comentarios de solicitudes de extracción en proyectos escritos solo en ciertos lenguajes de programación. O no sé cómo cocinarlo. A pesar de que especifico buscar comentarios en proyectos C, C ++, C #, los resultados se dan para todos los lenguajes, incluidos PHP, Python, JavaScript y otros. Como resultado, la búsqueda de casos adecuados ha resultado ser extremadamente tediosa, y voy a buscar solo algunos ejemplos. Sin embargo, son suficientes para demostrar la utilidad de las herramientas de análisis de código estático cuando se usan regularmente.

¿Qué pasa si el error había sido atrapado en la primera etapa? La respuesta es simple: los programadores no tendrían que esperar a que se muestre y luego buscar y corregir el código defectuoso.

Veamos los errores que PVS-Studio pudo haber detectado de inmediato:

El primer ejemplo está tomado del proyecto SatisfactoryModLoader. Antes de corregir el error, el código tenía el siguiente aspecto:

// gets an API function from the mod handler SML_API PVOID getAPIFunction(std::string name) { bool found = false; for (Registry reg : modHandler.APIRegistry) { if (reg.name == name) { found = true; } } if (!found) { std::string msg = ...; MessageBoxA(NULL, msg.c_str(), "SatisfactoryModLoader Fatal Error", MB_ICONERROR); abort(); } } 

Este código contenía un error, que PVS-Studio emitiría inmediatamente una advertencia para:

V591 La función no nula debería devolver un valor. ModFunctions.cpp 44

La función anterior no tiene declaración de retorno , por lo que devolverá un valor formalmente indefinido. El programador no usó el analizador de código, por lo que tuvo que buscar el error por su cuenta. La función después de editar:

 // gets an API function from the mod handler SML_API PVOID getAPIFunction(std::string name) { bool found = false; PVOID func = NULL; for (Registry reg : modHandler.APIRegistry) { if (reg.name == name) { func = reg.func; found = true; } } if (!found) { std::string msg = ...; MessageBoxA(NULL, msg.c_str(), "SatisfactoryModLoader Fatal Error", MB_ICONERROR); abort(); } return func; } 

Curiosamente, en el commit, el autor marcó el error como crítico: " error crítico corregido donde no se devolvieron las funciones API ".

En la segunda confirmación del historial del proyecto mc6809, las ediciones se introdujeron en el siguiente código:

 void mc6809dis_direct( mc6809dis__t *const dis, mc6809__t *const cpu, const char *const op, const bool b16 ) { assert(dis != NULL); assert(op != NULL); addr.b[MSB] = cpu->dp; addr.b[LSB] = (*dis->read)(dis, dis->next++); ... if (cpu != NULL) { ... } } 

El autor corrigió solo una línea. Reemplazó la expresión

 addr.b[MSB] = cpu->dp; 

para el siguiente

 addr.b[MSB] = cpu != NULL ? cpu->dp : 0; 

En la versión anterior del código no había ninguna comprobación de puntero nulo. Si sucede de modo que se pase un puntero nulo a la función mc6809dis_direct como segundo argumento, su desreferencia se producirá en el cuerpo de la función. El resultado es deplorable e impredecible .

La desreferencia de puntero nulo es uno de los patrones más comunes de los que se nos informa: "No es un error crítico. ¿A quién le importa que prospere en el código? Si se produce la desreferencia, el programa se bloqueará silenciosamente y eso es todo ”. Es extraño y triste escuchar esto de los programadores de C ++, pero la vida sucede.

De todos modos, en este proyecto, tal desreferencia se ha convertido en un error, ya que el tema del commit nos dice: " Solución de error --- Desreferencia NULL ".

Si el desarrollador del proyecto hubiera utilizado PVS-Studio, podría haber verificado y encontrado la advertencia hace dos meses y medio. Esto es cuando se introdujo el error. Aquí está la advertencia:

V595 El puntero 'cpu' se utilizó antes de que se verificara contra nullptr. Líneas de verificación: 1814, 1821. mc6809dis.c 1814

Por lo tanto, el error se habría solucionado en el momento de su aparición, lo que habría ahorrado el tiempo y los nervios del desarrollador :).

Se encontró un ejemplo de otra solución interesante en el proyecto libmorton.

Código a arreglar:

 template<typename morton> inline bool findFirstSetBitZeroIdx(const morton x, unsigned long* firstbit_location) { #if _MSC_VER && !_WIN64 // 32 BIT on 32 BIT if (sizeof(morton) <= 4) { return _BitScanReverse(firstbit_location, x) != 0; } // 64 BIT on 32 BIT else { *firstbit_location = 0; if (_BitScanReverse(firstbit_location, (x >> 32))) { // check first part firstbit_location += 32; return true; } return _BitScanReverse(firstbit_location, (x & 0xFFFFFFFF)) != 0; } #elif _MSC_VER && _WIN64 .... #elif __GNUC__ .... #endif } 

En su edición, un programador reemplaza la expresión " firstbit_location + = 32 " con " * firstbit_location + = 32 ". El programador esperaba que 32 se agregará al valor de la variable a la que hace referencia el puntero firstbit_location , pero 32 se agregó al puntero mismo. El valor modificado del puntero ya no se usó en ninguna parte y el valor variable esperado permaneció sin cambios.

PVS-Studio emitiría una advertencia a este código:

V1001 Se asigna la variable 'firstbit_location' pero no se usa al final de la función. morton_common.h 22

Bueno, ¿qué tiene de malo la expresión modificada pero aún no utilizada? El diagnóstico V1001 no parece que esté destinado a detectar errores particularmente peligrosos. A pesar de esto, encontró un error importante que influyó en la lógica del programa.

¡Además, resultó que ese error no fue tan fácil de encontrar! ¡No solo ha estado en el programa desde que se creó el archivo, sino que también ha experimentado muchas ediciones en líneas vecinas y ha existido en el proyecto por hasta 3 (!) Años! Todo este tiempo la lógica del programa se rompió y no funcionó de la manera que los desarrolladores esperaban. Si hubieran usado PVS-Studio, el error se habría detectado mucho antes.

Al final, veamos otro buen ejemplo. Mientras recopilaba correcciones de errores en GitHub, me encontré con una solución con el siguiente contenido varias veces. El error corregido estaba aquí:

 int kvm_arch_prepare_memory_region(...) { ... do { struct vm_area_struct *vma = find_vma(current->mm, hva); hva_t vm_start, vm_end; ... if (vma->vm_flags & VM_PFNMAP) { ... phys_addr_t pa = (vma->vm_pgoff << PAGE_SHIFT) + vm_start - vma->vm_start; ... } ... } while (hva < reg_end); ... } 

PVS-Studio emitió una advertencia para este fragmento de código:

V629 Considere inspeccionar la expresión 'vma-> vm_pgoff << 12'. Desplazamiento de bits del valor de 32 bits con una expansión posterior al tipo de 64 bits. mmu.c 1795

Revisé las declaraciones de variables, usadas en la expresión " phys_addr_t pa = (vma-> vm_pgoff << PAGE_SHIFT) + vm_start - vma-> vm_start; " y descubrí que el código dado anteriormente es igual al siguiente ejemplo sintético:

 void foo(unsigned long a, unsigned long b) { unsigned long long x = (a << 12) + b; } 

Si el valor de una variable de 32 bits es mayor que 0xFFFFF , los 12 bits más altos tendrán al menos un valor no nulo. Después de desplazar esta variable hacia la izquierda, estos bits significativos se perderán, lo que dará como resultado información incorrecta escrita en x.

Para eliminar la pérdida de bits altos, primero necesitamos convertir un tipo largo largo sin signo y solo después de este cambio la variable:

 pa = (phys_addr_t)vma->vm_pgoff << PAGE_SHIFT; pa += vm_start - vma->vm_start; 

De esta manera, siempre se escribirá un valor correcto en pa.

Eso estaría bien, pero este error, al igual que el primer ejemplo del artículo, también resultó ser crítico. Su autor escribió al respecto en el comentario. Además, este error encontró su camino hacia una enorme cantidad de proyectos. Para apreciar completamente la magnitud de la tragedia, sugiero que mire la cantidad de resultados cuando busque esta corrección de errores en GitHub. Miedo, ¿no es así?

Así que he adoptado un nuevo enfoque para demostrar los beneficios de un uso regular del analizador de código estático. Espero que lo hayas disfrutado. Descargue y pruebe el analizador de código estático PVS-Studio para verificar sus propios proyectos. Al momento de escribir, tiene alrededor de 700 reglas de diagnóstico implementadas para detectar una variedad de patrones de error. Admite C, C ++, C # y Java.