Geekly articles weekly

CMake: el caso cuando la calidad del proyecto es imperdonable

Imagen 1

CMake es un sistema multiplataforma para automatizar compilaciones de proyectos. Este sistema es mucho más antiguo que el analizador de código estático PVS-Studio, pero nadie ha intentado aplicar el analizador en su código y revisar los errores. Al final resultó que, hay muchos de ellos. La audiencia de CMake es enorme. Se inician nuevos proyectos y se portan los antiguos. Me estremezco al pensar en cuántos desarrolladores podrían haber tenido un error determinado.

Introduccion


CMake es un sistema multiplataforma para automatizar la creación de software a partir del código fuente. CMake no está diseñado directamente para compilar, solo genera archivos para controlar una 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 PVS-Studio apareció solo en 2008. En ese momento, su objetivo era buscar errores resultantes de la portabilidad de sistemas de 32 bits a los de 64 bits. En 2010, apareció el primer conjunto de diagnósticos de uso general (V501- V545 ). Por cierto, el código CMake tiene algunas advertencias de este primer conjunto.

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 V1040 se implementó no hace mucho tiempo. Lo más probable es que, al momento de publicar el artículo, aún no se publique, sin embargo, ya encontramos un error genial con su ayuda.

Hay un error tipográfico hecho en el nombre __MINGW32_ . Al final, falta un carácter subrayado. Si busca el código con este nombre, puede ver que la versión con dos caracteres subrayados en ambos lados se utiliza en el proyecto:

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)) { .... } .... }

Para una matriz declarada estáticamente, el operador sizeof calculará el tamaño en bytes, teniendo en cuenta la cantidad de elementos y su tamaño. Al evaluar el valor de la variable cch_subkeyname , un desarrollador no lo tuvo en cuenta y obtuvo un valor 4 veces mayor de lo previsto. Vamos a explicar de dónde vienen "cuatro 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 la variable, que contiene el tamaño del búfer 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 puede almacenar 256 caracteres del tipo wchar_t (en Windows, wchar_t es de 2 bytes). Es 256 que se debe pasar a la función. En cambio, 512 se multiplica por 2 y obtenemos 1024.

Creo que ahora está claro cómo corregir este error. Necesita usar la división en lugar de la multiplicación:

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

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

El error descrito puede conducir potencialmente a un desbordamiento del búfer. Todos esos fragmentos definitivamente deben corregirse:

  • 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 está desreferenciado justo antes de verificar su validez. ¿Eso no causó ningún problema a nadie? A continuación hay otro ejemplo de tal fragmento. Está hecho como una copia al carbón. Pero, de hecho, hay muchas advertencias V595 y la mayoría de ellas no son tan obvias. Desde mi experiencia, puedo decir que corregir las advertencias de este diagnóstico lleva más tiempo.

  • 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. Apareció debido a un error tipográfico ordinario. Al llamar a la función SysAllocStringByteLen , uno debería haber usado 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]); } .... }

Este código oculta varios problemas a la vez. Al acceder a las bases de longitud y a las matrices de bits de longitud , un índice de matriz puede estar fuera de los límites, ya que los desarrolladores escribieron el operador '>' en lugar de '> =' arriba. Esta verificación comenzó a perder un valor inaceptable. Aquí no tenemos más que un patrón de error clásico llamado Off-by-one Error .

Aquí está la lista completa de operaciones de acceso a la matriz por un í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 del puntero testRun no se libera si la función testRun-> StartTest devuelve verdadero . Al ejecutar otra rama de código, esta memoria 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++; .... }

* La comparación de caracteres con nulo es redundante. 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 innecesaria 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 la operación de negación probablemente debería eliminarse de los corchetes. Parece que no hay tal error aquí, solo corchetes innecesarios. Pero lo más probable es que haya un error lógico en el código.

El operador de continuación se ejecuta solo en el caso si la lista de pruebas this-> TestsToRun no está vacía y cnt está ausente. Es razonable suponer que si la lista de pruebas está vacía, se debe realizar la misma acción. Lo más probable es que la condición sea la siguiente:

 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; } .... }

Es un ejemplo similar, pero esta vez estoy más seguro de que se produce un error. La función IsSet ("CMAKE_WARN_DEPRECATED") verifica que el valor CMAKE_WARN_DEPRECATED esté configurado globalmente, y la función IsOn ("CMAKE_WARN_DEPRECATED) verifica que el valor esté configurado en la configuración del proyecto. Lo más probable es que el operador complementario sea redundante, ya que 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 "; } .... }

Este código puede ser más simple. Uno puede reescribir la expresión condicional de la siguiente manera:

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

Algunos lugares más para 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

Varias advertencias


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 similar a las dos últimas líneas de la función. Un desarrollador puede simplificar este código eliminando la condición, o si hay un error en el código y debe solucionarse.

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 externo e interno. Al mismo tiempo, el valor del contador nuevamente comienza desde cero en el bucle interno. Puede que no sea 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

 void 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 se sobrescribe con un nuevo valor en todos los lugares. Es difícil decir cuál es el problema o por qué lo hicieron. Quizás, los operadores '=' y '+ =' estaban confusos.

La lista completa de dichos 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); }

La configuración forzada del valor AES_SET_UTF8 parece sospechosa. Creo que dicho código confundirá a cualquier desarrollador que llegue a refinar este fragmento.

Este código fue copiado a otro lugar:

  • 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 explicaré brevemente cómo verificar proyectos CMake con PVS-Studio tan fácil como uno-dos-tres.

Windows / Visual Studio

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

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

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

Linux / macOS

El archivo compile_commands.json se usa para verificaciones en estos sistemas. Por cierto, se puede generar en diferentes sistemas de compilación. Así es como lo haces en CMake:

 cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=On ..

Lo último que debe hacer es ejecutar 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 hemos desarrollado 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


Una gran audiencia de usuarios de CMake es excelente para probar el proyecto, pero muchos problemas podrían evitarse antes del lanzamiento mediante el uso de 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 ++, me gustaría recordar 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 .

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

More articles:


All Articles