Geekly articles weekly

CMake: el caso cuando el proyecto es imperdonable la calidad de su código

Imagen 1

CMake es un sistema de automatización multiplataforma para proyectos de construcción. Este sistema es mucho más antiguo que el analizador de código estático PVS-Studio, aunque nadie ha intentado aplicarlo al código y revisar los errores. Resulta que hay muchos errores. La audiencia de CMake es enorme. En él, comienzan nuevos proyectos y se transfieren los antiguos. Da miedo imaginar cuántos programadores podrían tener este o aquel error.

Introduccion


CMake (de la marca inglesa multiplataforma) es un sistema de automatización multiplataforma para crear software a partir del código fuente. CMake no compila directamente, sino que solo genera archivos de control de compilación a partir de archivos CMakeLists.txt. La primera versión del programa tuvo lugar en 2000. A modo de comparación, el analizador estático PVS-Studio apareció solo en 2008. Luego se centró en encontrar errores al portar programas de sistemas de 32 bits a sistemas de 64 bits, y en 2010 apareció el primer conjunto de diagnósticos de uso general ( V501 - V545 ). Por cierto, hay algunas advertencias de este primer conjunto en el código CMake.

Errores imperdonables


V1040 Posible error tipográfico en la ortografía de un nombre de macro predefinido. La macro '__MINGW32_' es similar a '__MINGW32__'. winapi.h 4112

/* from winternl.h */ #if !defined(__UNICODE_STRING_DEFINED) && defined(__MINGW32_) #define __UNICODE_STRING_DEFINED #endif

El diagnóstico de V1040 solo se ha implementado recientemente. En el momento de la publicación del artículo, lo más probable es que no haya un lanzamiento con él, pero con la ayuda de este diagnóstico ya hemos logrado encontrar un error difícil.

Aquí hicieron un error tipográfico en el nombre __MINGW32_ . Al final, falta un guión bajo. Si busca por código con este nombre, puede asegurarse de que el proyecto realmente use la versión con dos guiones bajos en ambos lados:

Cuadro 3


V531 Es extraño que un operador sizeof () se multiplique por sizeof (). cmGlobalVisualStudioGenerator.cxx 558

 bool IsVisualStudioMacrosFileRegistered(const std::string& macrosFile, const std::string& regKeyBase, std::string& nextAvailableSubKeyName) { .... if (ERROR_SUCCESS == result) { wchar_t subkeyname[256]; // <= DWORD cch_subkeyname = sizeof(subkeyname) * sizeof(subkeyname[0]); // <= wchar_t keyclass[256]; DWORD cch_keyclass = sizeof(keyclass) * sizeof(keyclass[0]); FILETIME lastWriteTime; lastWriteTime.dwHighDateTime = 0; lastWriteTime.dwLowDateTime = 0; while (ERROR_SUCCESS == RegEnumKeyExW(hkey, index, subkeyname, &cch_subkeyname, 0, keyclass, &cch_keyclass, &lastWriteTime)) { .... } .... }

Cuando la matriz se declara estáticamente, el operador sizeof calculará su tamaño en bytes, teniendo en cuenta tanto el número de elementos como el tamaño de los elementos. Al calcular el valor de la variable cch_subkeyname, el programador no tuvo esto en cuenta y recibió un valor 4 veces mayor de lo planeado. Vamos a explicar dónde está "4 veces".

La matriz y su tamaño incorrecto se pasan a la función RegEnumKeyExW :

 LSTATUS RegEnumKeyExW( HKEY hKey, DWORD dwIndex, LPWSTR lpName, // <= subkeyname LPDWORD lpcchName, // <= cch_subkeyname LPDWORD lpReserved, LPWSTR lpClass, LPDWORD lpcchClass, PFILETIME lpftLastWriteTime );

El puntero lpcchName debe apuntar a una variable que contenga el tamaño del búfer especificado en caracteres: "Un puntero a una variable que especifica el tamaño del búfer especificado por el parámetro lpClass , en caracteres". El tamaño de la matriz de subclaves es de 512 bytes y es capaz de almacenar 256 caracteres del tipo wchar_t (en Windows wchar_t es de 2 bytes). Este valor es 256 y debe pasarse a la función. En cambio, 512 se multiplica por 2 nuevamente para obtener 1024.

Creo que ahora está claro cómo solucionar el error. En lugar de multiplicar, usa la división:

 DWORD cch_subkeyname = sizeof(subkeyname) / sizeof(subkeyname[0]);

Por cierto, se produce exactamente el mismo error al calcular el valor de la variable cch_keyclass .

El error descrito puede conducir potencialmente a un desbordamiento del búfer. Es necesario arreglar todos esos lugares:

  • V531 Es extraño que un operador sizeof () se multiplique por sizeof (). cmGlobalVisualStudioGenerator.cxx 556
  • V531 Es extraño que un operador sizeof () se multiplique por sizeof (). cmGlobalVisualStudioGenerator.cxx 572
  • V531 Es extraño que un operador sizeof () se multiplique por sizeof (). cmGlobalVisualStudioGenerator.cxx 621
  • V531 Es extraño que un operador sizeof () se multiplique por sizeof (). cmGlobalVisualStudioGenerator.cxx 622
  • V531 Es extraño que un operador sizeof () se multiplique por sizeof (). cmGlobalVisualStudioGenerator.cxx 649

V595 El puntero 'this-> BuildFileStream' se utilizó antes de que se verificara contra nullptr. Líneas de verificación: 133, 134. cmMakefileTargetGenerator.cxx 133

 void cmMakefileTargetGenerator::CreateRuleFile() { .... this->BuildFileStream->SetCopyIfDifferent(true); if (!this->BuildFileStream) { return; } .... }

El puntero this-> BuildFileStream se desreferencia justo antes de la verificación de validación. ¿Esto realmente estaba causando algún problema? A continuación se muestra otro ejemplo de tal lugar. Está hecho justo debajo del papel carbón. Pero, de hecho, hay muchas advertencias V595 y la mayoría de ellas no son tan obvias. Por experiencia, puedo decir que corregir las advertencias de este diagnóstico es el más largo.

  • V595 El puntero 'this-> FlagFileStream' se utilizó antes de que se verificara contra nullptr. Líneas de verificación: 303, 304. cmMakefileTargetGenerator.cxx 303

V614 Puntero no inicializado 'str' utilizado. cmVSSetupHelper.h 80

 class SmartBSTR { public: SmartBSTR() { str = NULL; } SmartBSTR(const SmartBSTR& src) { if (src.str != NULL) { str = ::SysAllocStringByteLen((char*)str, ::SysStringByteLen(str)); } else { str = ::SysAllocStringByteLen(NULL, 0); } } .... private: BSTR str; };

El analizador detectó el uso del puntero str no inicializado. Y esto surgió debido al error tipográfico habitual. Al llamar a la función SysAllocStringByteLen, tenía que usar el puntero src.str .

V557 Array overrun es posible. El valor del índice 'lensymbol' podría llegar a 28. archive_read_support_format_rar.c 2749

 static int64_t expand(struct archive_read *a, int64_t end) { .... if ((lensymbol = read_next_symbol(a, &rar->lengthcode)) < 0) goto bad_data; if (lensymbol > (int)(sizeof(lengthbases)/sizeof(lengthbases[0]))) goto bad_data; if (lensymbol > (int)(sizeof(lengthbits)/sizeof(lengthbits[0]))) goto bad_data; len = lengthbases[lensymbol] + 2; if (lengthbits[lensymbol] > 0) { if (!rar_br_read_ahead(a, br, lengthbits[lensymbol])) goto truncated_data; len += rar_br_bits(br, lengthbits[lensymbol]); rar_br_consume(br, lengthbits[lensymbol]); } .... }

Se encontraron varios problemas en este fragmento de código. Al acceder a las matrices de bases de longitud y bits de longitud, es posible ir más allá del límite de la matriz, porque encima del código, los desarrolladores escribieron el operador '>' en lugar de '> ='. Tal verificación comenzó a omitir un valor no válido. Nos enfrentamos a un patrón de error clásico llamado Error Off-by-one .

La lista completa de lugares para acceder a matrices por índice no válido:

  • V557 Array overrun es posible. El valor del índice 'lensymbol' podría llegar a 28. archive_read_support_format_rar.c 2750
  • V557 Array overrun es posible. El valor del índice 'lensymbol' podría llegar a 28. archive_read_support_format_rar.c 2751
  • V557 Array overrun es posible. El valor del índice 'lensymbol' podría llegar a 28. archive_read_support_format_rar.c 2753
  • V557 Array overrun es posible. El valor del índice 'lensymbol' podría llegar a 28. archive_read_support_format_rar.c 2754
  • V557 Array overrun es posible. El valor del índice 'offssymbol' podría alcanzar 60. archive_read_support_format_rar.c 2797

Pérdida de memoria


V773 Se salió de la función sin soltar el puntero 'testRun'. Una pérdida de memoria es posible. cmCTestMultiProcessHandler.cxx 193

 void cmCTestMultiProcessHandler::FinishTestProcess(cmCTestRunTest* runner, bool started) { .... delete runner; if (started) { this->StartNextTests(); } } bool cmCTestMultiProcessHandler::StartTestProcess(int test) { .... cmCTestRunTest* testRun = new cmCTestRunTest(*this); // <= .... if (testRun->StartTest(this->Completed, this->Total)) { return true; // <= } this->FinishTestProcess(testRun, false); // <= return false; }

El analizador detectó una pérdida de memoria. La memoria por puntero testRun no se libera si la función testRun-> StartTest devuelve verdadero . Cuando se ejecuta otra rama de código, la memoria que usa el puntero testRun se libera en la función this-> FinishTestProcess .

Fuga de recursos


V773 La función se cerró sin cerrar el archivo al que hace referencia el identificador 'fd'. Una fuga de recursos es posible. rhash.c 450

 RHASH_API int rhash_file(....) { FILE* fd; rhash ctx; int res; hash_id &= RHASH_ALL_HASHES; if (hash_id == 0) { errno = EINVAL; return -1; } if ((fd = fopen(filepath, "rb")) == NULL) return -1; if ((ctx = rhash_init(hash_id)) == NULL) return -1; // <= fclose(fd); ??? res = rhash_file_update(ctx, fd); fclose(fd); rhash_final(ctx, result); rhash_free(ctx); return res; }

Extraña lógica en condiciones


V590 Considere inspeccionar la expresión '* s! =' \ 0 '&& * s ==' ''. La expresión es excesiva o contiene un error de imprenta. archive_cmdline.c 76

 static ssize_t get_argument(struct archive_string *as, const char *p) { const char *s = p; archive_string_empty(as); /* Skip beginning space characters. */ while (*s != '\0' && *s == ' ') s++; .... }

Comparar el carácter * s con un terminal cero es superfluo. La condición del ciclo while depende solo de si el carácter es igual a un espacio o no. Esto no es un error, sino una complicación adicional del código.

V592 La expresión estaba entre paréntesis dos veces: ((expresión)). Un par de paréntesis es innecesario o hay un error de imprenta. cmCTestTestHandler.cxx 899

 void cmCTestTestHandler::ComputeTestListForRerunFailed() { this->ExpandTestsToRunInformationForRerunFailed(); ListOfTests finalList; int cnt = 0; for (cmCTestTestProperties& tp : this->TestList) { cnt++; // if this test is not in our list of tests to run, then skip it. if ((!this->TestsToRun.empty() && std::find(this->TestsToRun.begin(), this->TestsToRun.end(), cnt) == this->TestsToRun.end())) { continue; } tp.Index = cnt; finalList.push_back(tp); } .... }

El analizador advierte que tal vez la negación debería quedar entre corchetes. Parece que no hay tal error aquí, solo corchetes dobles adicionales. Pero, muy probablemente, hay un error lógico en esta condición.

La instrucción continue se ejecuta si la lista de pruebas this-> TestsToRun no está vacía y cnt está ausente. Es lógico suponer que si la lista de prueba está vacía, se debe realizar la misma acción. Lo más probable es que la condición sea así:

 if (this->TestsToRun.empty() || std::find(this->TestsToRun.begin(), this->TestsToRun.end(), cnt) == this->TestsToRun.end()) { continue; }

V592 La expresión estaba entre paréntesis dos veces: ((expresión)). Un par de paréntesis es innecesario o hay un error de imprenta. cmMessageCommand.cxx 73

 bool cmMessageCommand::InitialPass(std::vector<std::string> const& args, cmExecutionStatus&) { .... } else if (*i == "DEPRECATION") { if (this->Makefile->IsOn("CMAKE_ERROR_DEPRECATED")) { fatal = true; type = MessageType::DEPRECATION_ERROR; level = cmake::LogLevel::LOG_ERROR; } else if ((!this->Makefile->IsSet("CMAKE_WARN_DEPRECATED") || this->Makefile->IsOn("CMAKE_WARN_DEPRECATED"))) { type = MessageType::DEPRECATION_WARNING; level = cmake::LogLevel::LOG_WARNING; } else { return true; } ++i; } .... }

Un ejemplo similar, pero aquí tengo más confianza en presencia de un error. La función IsSet ("CMAKE_WARN_DEPRECATED") verifica que el valor CMAKE_WARN_DEPRECATED se establece globalmente, y la función IsOn ("CMAKE_WARN_DEPRECATED") verifica que el valor esté especificado en la configuración del proyecto. Lo más probable es que el operador de negación sea superfluo, porque en ambos casos, es correcto establecer los mismos valores de tipo y nivel .

V728 Se puede simplificar una verificación excesiva. El '(A &&! B) || (! A && B) 'expresión es equivalente a la expresión' bool (A)! = Bool (B) '. cmCTestRunTest.cxx 151

 bool cmCTestRunTest::EndTest(size_t completed, size_t total, bool started) { .... } else if ((success && !this->TestProperties->WillFail) || (!success && this->TestProperties->WillFail)) { this->TestResult.Status = cmCTestTestHandler::COMPLETED; outputStream << " Passed "; } .... }

Dicho código puede simplificarse enormemente reescribiendo la expresión condicional de esta manera:

 } else if (success != this->TestProperties->WillFail) { this->TestResult.Status = cmCTestTestHandler::COMPLETED; outputStream << " Passed "; }

Algunos lugares más que puedes simplificar:

  • V728 Se puede simplificar una verificación excesiva. El '(A y B) || (! A &&! B) 'expresión es equivalente a la expresión' bool (A) == bool (B) '. cmCTestTestHandler.cxx 702
  • V728 Se puede simplificar una verificación excesiva. El '(A &&! B) || (! A && B) 'expresión es equivalente a la expresión' bool (A)! = Bool (B) '. digest_sspi.c 443
  • V728 Se puede simplificar una verificación excesiva. El '(A &&! B) || (! A && B) 'expresión es equivalente a la expresión' bool (A)! = Bool (B) '. tcp.c 1295
  • V728 Se puede simplificar una verificación excesiva. El '(A &&! B) || (! A && B) 'expresión es equivalente a la expresión' bool (A)! = Bool (B) '. testDynamicLoader.cxx 58
  • V728 Se puede simplificar una verificación excesiva. El '(A &&! B) || (! A && B) 'expresión es equivalente a la expresión' bool (A)! = Bool (B) '. testDynamicLoader.cxx 65
  • V728 Se puede simplificar una verificación excesiva. El '(A &&! B) || (! A && B) 'expresión es equivalente a la expresión' bool (A)! = Bool (B) '. testDynamicLoader.cxx 72

Advertencias varias


V523 La declaración 'then' es equivalente al fragmento de código posterior. archive_read_support_format_ar.c 415

 static int _ar_read_header(struct archive_read *a, struct archive_entry *entry, struct ar *ar, const char *h, size_t *unconsumed) { .... /* * "__.SYMDEF" is a BSD archive symbol table. */ if (strcmp(filename, "__.SYMDEF") == 0) { archive_entry_copy_pathname(entry, filename); /* Parse the time, owner, mode, size fields. */ return (ar_parse_common_header(ar, entry, h)); } /* * Otherwise, this is a standard entry. The filename * has already been trimmed as much as possible, based * on our current knowledge of the format. */ archive_entry_copy_pathname(entry, filename); return (ar_parse_common_header(ar, entry, h)); }

La expresión en la última condición es idéntica a las dos últimas líneas de la función. Este código puede simplificarse eliminando la condición, o si hay un error en el código y debe corregirse.

V535 La variable 'i' se está utilizando para este bucle y para el bucle externo. Líneas de verificación: 2220, 2241. multi.c 2241

 static CURLMcode singlesocket(struct Curl_multi *multi, struct Curl_easy *data) { .... for(i = 0; (i< MAX_SOCKSPEREASYHANDLE) && // <= (curraction & (GETSOCK_READSOCK(i) | GETSOCK_WRITESOCK(i))); i++) { unsigned int action = CURL_POLL_NONE; unsigned int prevaction = 0; unsigned int comboaction; bool sincebefore = FALSE; s = socks[i]; /* get it from the hash */ entry = sh_getentry(&multi->sockhash, s); if(curraction & GETSOCK_READSOCK(i)) action |= CURL_POLL_IN; if(curraction & GETSOCK_WRITESOCK(i)) action |= CURL_POLL_OUT; actions[i] = action; if(entry) { /* check if new for this transfer */ for(i = 0; i< data->numsocks; i++) { // <= if(s == data->sockets[i]) { prevaction = data->actions[i]; sincebefore = TRUE; break; } } } .... }

La variable i se usa como contador de bucles en los bucles externos y anidados. En este caso, el valor del contador nuevamente comienza a contar desde cero en el adjunto. Esto puede no ser un error aquí, pero el código es sospechoso.

V519 La variable 'tagString' tiene valores asignados dos veces seguidas. Quizás esto sea un error. Líneas de verificación: 84, 86. cmCPackLog.cxx 86

 oid cmCPackLog::Log(int tag, const char* file, int line, const char* msg, size_t length) { .... if (tag & LOG_OUTPUT) { output = true; display = true; if (needTagString) { if (!tagString.empty()) { tagString += ","; } tagString = "VERBOSE"; } } if (tag & LOG_WARNING) { warning = true; display = true; if (needTagString) { if (!tagString.empty()) { tagString += ","; } tagString = "WARNING"; } } .... }

La variable tagString está deshilachada por el nuevo valor en todos los lugares. Es difícil decir cuál fue el error o por qué lo cometieron. Quizás los operadores '=' y '+ =' estaban confundidos.

La lista completa de tales lugares:

  • V519 La variable 'tagString' tiene valores asignados dos veces seguidas. Quizás esto sea un error. Líneas de verificación: 94, 96. cmCPackLog.cxx 96
  • V519 La variable 'tagString' tiene valores asignados dos veces seguidas. Quizás esto sea un error. Líneas de verificación: 104, 106. cmCPackLog.cxx 106
  • V519 La variable 'tagString' tiene valores asignados dos veces seguidas. Quizás esto sea un error. Líneas de verificación: 114, 116. cmCPackLog.cxx 116
  • V519 La variable 'tagString' tiene valores asignados dos veces seguidas. Quizás esto sea un error. Líneas de verificación: 125, 127. cmCPackLog.cxx 127

V519 La variable 'aes-> aes_set' tiene valores asignados dos veces sucesivamente. Quizás esto sea un error. Líneas de verificación: 4052, 4054. archive_string.c 4054

 int archive_mstring_copy_utf8(struct archive_mstring *aes, const char *utf8) { if (utf8 == NULL) { aes->aes_set = 0; // <= } aes->aes_set = AES_SET_UTF8; // <= .... return (int)strlen(utf8); }

Forzar AES_SET_UTF8 parece sospechoso. Creo que dicho código engañará a cualquier desarrollador que se enfrente con el refinamiento de este lugar.

Este código se copió a un lugar más:

  • V519 La variable 'aes-> aes_set' tiene valores asignados dos veces sucesivamente. Quizás esto sea un error. Líneas de verificación: 4066, 4068. archive_string.c 4068

Cómo encontrar errores en un proyecto en CMake


En esta sección, le contaré un poco cómo verificar fácil y fácilmente los proyectos en CMake utilizando PVS-Studio.

Windows / Visual Studio

Para Visual Studio, puede generar el archivo del proyecto utilizando la GUI de CMake o el siguiente comando:

 cmake -G "Visual Studio 15 2017 Win64" ..

A continuación, puede abrir el archivo .sln y probar el proyecto utilizando el complemento para Visual Studio.

Linux / macOS

En estos sistemas, el archivo compile_commands.json se usa para verificar el proyecto. Por cierto, se puede generar en diferentes sistemas de ensamblaje. En CMake, esto se hace así:

 cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=On ..

Queda por iniciar el analizador en el directorio con el archivo .json:

 pvs-studio-analyzer analyze -l /path/to/PVS-Studio.lic -o /path/to/project.log -e /path/to/exclude-path -j<N>

También desarrollamos un módulo para proyectos CMake. A algunas personas les gusta usarlo. El módulo CMake y ejemplos de su uso se pueden encontrar en nuestro repositorio en GitHub: pvs-studio-cmake-examples .

Conclusión


La gran audiencia de usuarios de CMake es un buen probador del proyecto, pero muchos problemas no pudieron haberse evitado antes del lanzamiento, utilizando herramientas de análisis de código estático como PVS-Studio .

Si le gustaron los resultados del analizador, pero su proyecto no está escrito en C y C ++, quiero recordarle que el analizador también admite el análisis de proyectos en C # y Java. Puede probar el analizador en su proyecto yendo a esta página.



Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: Svyatoslav Razmyslov. CMake: el caso cuando la calidad del proyecto es imperdonable .

Source: https://habr.com/ru/post/464147/

More articles:


All Articles