Geekly articles weekly

CMake: o caso em que a qualidade do projeto é imperdoável

Quadro 1

O CMake é um sistema multiplataforma para automatizar a criação de projetos. Esse sistema é muito mais antigo que o analisador de código estático do PVS-Studio, mas ninguém tentou aplicar o analisador em seu código e revisar os erros. Como se viu, existem muitos deles. O público do CMake é enorme. Novos projetos começam nele e os antigos são portados. Estremeço ao pensar em quantos desenvolvedores poderiam ter tido algum erro.

1. Introdução


O CMake é um sistema multiplataforma para automatizar a criação de software a partir do código fonte. O CMake não se destina diretamente à criação, apenas gera arquivos para controlar uma compilação a partir dos arquivos CMakeLists.txt. O primeiro lançamento do programa ocorreu em 2000. Para comparação, o analisador PVS-Studio apareceu apenas em 2008. Naquela época, o objetivo era procurar erros resultantes da transferência de sistemas de 32 bits para sistemas de 64 bits. Em 2010, o primeiro conjunto de diagnósticos de uso geral apareceu (V501- V545 ). A propósito, o código CMake possui alguns avisos desse primeiro conjunto.

Erros imperdoáveis


V1040 Possível erro de digitação na ortografia de um nome de macro predefinido. A macro '__MINGW32_' é semelhante a '__MINGW32__'. winapi.h 4112

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

O diagnóstico V1040 foi implementado não muito tempo atrás. Provavelmente, no momento da publicação do artigo, ele ainda não será lançado. No entanto, já encontramos um erro interessante com sua ajuda.

Há um erro de digitação feito no nome __MINGW32_ . No final, um caractere sublinhado está ausente. Se você pesquisar o código com esse nome, poderá ver que a versão com dois caracteres sublinhados nos dois lados é usada no projeto:

Quadro 3

V531 É estranho que um operador sizeof () seja multiplicado 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 uma matriz declarada estaticamente, o operador sizeof calculará o tamanho em bytes, levando em consideração o número de elementos e seu tamanho. Ao avaliar o valor da variável cch_subkeyname , um desenvolvedor não levou em consideração e obteve um valor 4 vezes maior que o pretendido. Vamos explicar de onde vêm "quatro vezes".

A matriz e seu tamanho incorreto são passados ​​para a função RegEnumKeyExW :

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

O ponteiro lpcchName deve apontar para a variável, contendo o tamanho do buffer em caracteres: "Um ponteiro para uma variável que especifica o tamanho do buffer especificado pelo parâmetro lpClass , em caracteres". O tamanho da matriz do nome da subchave é de 512 bytes e pode armazenar 256 caracteres do tipo wchar_t (no Windows, wchar_t é de 2 bytes). É 256 que deve ser passado para a função. Em vez disso, 512 é multiplicado por 2 e obtemos 1024.

Acho que agora está claro como corrigir esse erro. Você precisa usar a divisão em vez da multiplicação:

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

A propósito, o mesmo erro ocorre ao avaliar o valor da variável cch_keyclass .

O erro descrito pode levar ao estouro do buffer. Todos esses fragmentos definitivamente precisam ser corrigidos:

  • V531 É estranho que um operador sizeof () seja multiplicado por sizeof (). cmGlobalVisualStudioGenerator.cxx 556
  • V531 É estranho que um operador sizeof () seja multiplicado por sizeof (). cmGlobalVisualStudioGenerator.cxx 572
  • V531 É estranho que um operador sizeof () seja multiplicado por sizeof (). cmGlobalVisualStudioGenerator.cxx 621
  • V531 É estranho que um operador sizeof () seja multiplicado por sizeof (). cmGlobalVisualStudioGenerator.cxx 622
  • V531 É estranho que um operador sizeof () seja multiplicado por sizeof (). cmGlobalVisualStudioGenerator.cxx 649

V595 O ponteiro 'this-> BuildFileStream' foi utilizado antes de ser verificado no nullptr. Verifique as linhas: 133, 134. cmMakefileTargetGenerator.cxx 133

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

O ponteiro this-> BuildFileStream é desreferenciado logo antes da verificação de sua validade. Isso não causou problemas para ninguém? Abaixo há outro exemplo desse snippet. É feito como uma cópia carbono. Mas, de fato, existem muitos avisos do V595 e a maioria deles não é tão óbvia. Pela minha experiência, posso dizer que a correção dos avisos desse diagnóstico leva mais tempo.

  • V595 O ponteiro 'this-> FlagFileStream' foi utilizado antes de ser verificado no nullptr. Verifique as linhas: 303, 304. cmMakefileTargetGenerator.cxx 303

V614 Ponteiro não inicializado 'str' usado. 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; };

O analisador detectou o uso do ponteiro de str não inicializado. Apareceu devido a um erro de digitação comum. Ao chamar a função SysAllocStringByteLen , deve-se ter usado o ponteiro src.str .

A saturação da matriz V557 é possível. O valor do índice 'lensymbol' pode chegar 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 trecho de código oculta vários problemas ao mesmo tempo. Ao acessar bases de comprimento e matrizes de comprimento , um índice de matriz pode sair dos limites, pois os desenvolvedores escreveram o operador '>' em vez de '> =' acima. Essa verificação começou a perder um valor inaceitável. Aqui, temos apenas um padrão de erro clássico chamado Erro Off-by-one .

Aqui está a lista completa de operações de acesso à matriz por um índice não válido:

  • A saturação da matriz V557 é possível. O valor do índice 'lensymbol' pode chegar a 28. archive_read_support_format_rar.c 2750
  • A saturação da matriz V557 é possível. O valor do índice 'lensymbol' pode chegar a 28. archive_read_support_format_rar.c 2751
  • A saturação da matriz V557 é possível. O valor do índice 'lensymbol' pode chegar a 28. archive_read_support_format_rar.c 2753
  • A saturação da matriz V557 é possível. O valor do índice 'lensymbol' pode chegar a 28. archive_read_support_format_rar.c 2754
  • A saturação da matriz V557 é possível. O valor do índice 'offssymbol' pode chegar a 60. archive_read_support_format_rar.c 2797

Vazamento de memória


V773 A função foi encerrada sem liberar o ponteiro 'testRun'. É possível um vazamento de memória. 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; }

O analisador detectou um vazamento de memória. A memória do ponteiro testRun não é liberada, se a função testRun-> StartTest retornar true . Ao executar outra ramificação de código, essa memória é liberada na função this-> FinishTestProcess.

Vazamento de recursos


V773 A função foi encerrada sem fechar o arquivo referenciado pelo identificador 'fd'. Um vazamento de recurso é possível. 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; }

Lógica estranha em condições


V590 Considere inspecionar a expressão '* s! =' \ 0 '&& * s ==' ''. A expressão é excessiva ou contém uma impressão incorreta. 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++; .... }

* A comparação de caracteres com nulo é redundante. A condição do loop while depende apenas se o caractere é igual a um espaço ou não. Isso não é um erro, mas uma complicação desnecessária do código.

V592 A expressão foi colocada entre parênteses duas vezes: ((expressão)). Um par de parênteses é desnecessário ou a impressão incorreta está presente. 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); } .... }

O analisador avisa que a operação de negação provavelmente deve ser retirada dos colchetes. Parece que não existe esse bug aqui - apenas colchetes duplos desnecessários. Mas o mais provável é que haja um erro lógico no código.

O operador continue é executado apenas no caso, se a lista de testes this-> TestsToRun não estiver vazia e cnt estiver ausente. É razoável supor que, se a lista de testes estiver vazia, a mesma ação precisará ocorrer. Muito provavelmente, a condição deve ser a seguinte:

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

V592 A expressão foi colocada entre parênteses duas vezes: ((expressão)). Um par de parênteses é desnecessário ou a impressão incorreta está presente. 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; } .... }

É um exemplo semelhante, mas desta vez estou mais confiante de que um erro ocorre. A função IsSet ("CMAKE_WARN_DEPRECATED") verifica se o valor CMAKE_WARN_DEPRECATED está definido globalmente e a função IsOn ("CMAKE_WARN_DEPRECATED) verifica se o valor está definido na configuração do projeto. Provavelmente, o operador complementar é redundante, pois nos dois casos, é correto definir os mesmos valores de tipo e nível.

V728 Uma verificação excessiva pode ser simplificada. O '(A &&! B) || (! A && B) 'expressão é equivalente à expressão' 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 pode ser mais simples. Pode-se reescrever a expressão condicional da seguinte maneira:

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

Mais alguns lugares para simplificar:

  • V728 Uma verificação excessiva pode ser simplificada. O '(A && B) || (! A &&! B) 'expressão é equivalente à expressão' bool (A) == bool (B) '. cmCTestTestHandler.cxx 702
  • V728 Uma verificação excessiva pode ser simplificada. O '(A &&! B) || (! A && B) 'expressão é equivalente à expressão' bool (A)! = Bool (B) '. digest_sspi.c 443
  • V728 Uma verificação excessiva pode ser simplificada. O '(A &&! B) || (! A && B) 'expressão é equivalente à expressão' bool (A)! = Bool (B) '. tcp.c 1295
  • V728 Uma verificação excessiva pode ser simplificada. O '(A &&! B) || (! A && B) 'expressão é equivalente à expressão' bool (A)! = Bool (B) '. testDynamicLoader.cxx 58
  • V728 Uma verificação excessiva pode ser simplificada. O '(A &&! B) || (! A && B) 'expressão é equivalente à expressão' bool (A)! = Bool (B) '. testDynamicLoader.cxx 65
  • V728 Uma verificação excessiva pode ser simplificada. O '(A &&! B) || (! A && B) 'expressão é equivalente à expressão' bool (A)! = Bool (B) '. testDynamicLoader.cxx 72

Vários avisos


V523 A instrução 'then' é equivalente ao fragmento de código subsequente. 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)); }

A expressão na última condição é semelhante às duas últimas linhas da função. Um desenvolvedor pode simplificar esse código removendo a condição ou há um erro no código e ele deve ser corrigido.

V535 A variável 'i' está sendo usada para esse loop e para o loop externo. Verifique as linhas: 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; } } } .... }

A variável i é usada como um contador de loop nos loops externos e internos. Ao mesmo tempo, o valor do contador começa novamente a partir do zero no loop interno. Pode não ser um bug aqui, mas o código é suspeito.

V519 A variável 'tagString' recebe valores duas vezes sucessivamente. Talvez isso seja um erro. Verifique as linhas: 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"; } } .... }

A variável tagString é substituída por um novo valor em todos os lugares. É difícil dizer qual é o problema ou por que eles o fizeram. Talvez os operadores '=' e '+ =' estivessem confusos.

A lista completa desses lugares:

  • V519 A variável 'tagString' recebe valores duas vezes sucessivamente. Talvez isso seja um erro. Verifique as linhas: 94, 96. cmCPackLog.cxx 96
  • V519 A variável 'tagString' recebe valores duas vezes sucessivamente. Talvez isso seja um erro. Verifique as linhas: 104, 106. cmCPackLog.cxx 106
  • V519 A variável 'tagString' recebe valores duas vezes sucessivamente. Talvez isso seja um erro. Verifique as linhas: 114, 116. cmCPackLog.cxx 116
  • V519 A variável 'tagString' recebe valores duas vezes sucessivamente. Talvez isso seja um erro. Verifique as linhas: 125, 127. cmCPackLog.cxx 127

V519 A variável 'aes-> aes_set' recebe valores duas vezes sucessivamente. Talvez isso seja um erro. Verifique as linhas: 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); }

A configuração forçada do valor AES_SET_UTF8 parece suspeita. Eu acho que esse código confundirá qualquer desenvolvedor que refine esse fragmento.

Este código foi copiado para outro local:

  • V519 A variável 'aes-> aes_set' recebe valores duas vezes sucessivamente. Talvez isso seja um erro. Verifique as linhas: 4066, 4068. archive_string.c 4068

Como encontrar erros em um projeto no CMake


Nesta seção, mostrarei brevemente como verificar projetos do CMake com o PVS-Studio tão fácil quanto um-dois-três.

Windows / Visual Studio

Para o Visual Studio, você pode gerar um arquivo de projeto usando o CMake GUI ou o seguinte comando:

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

Em seguida, você pode abrir o arquivo .sln e verificar o projeto usando o plug - in do Visual Studio.

Linux / macOS

O arquivo compile_commands.json é usado para verificações nesses sistemas. A propósito, ele pode ser gerado em diferentes sistemas de compilação. É assim que você faz no CMake:

 cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=On ..

A última coisa a fazer é executar o analisador no diretório com o arquivo .json:

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

Também desenvolvemos um módulo para projetos CMake. Algumas pessoas gostam de usá-lo. O módulo CMake e exemplos de seu uso podem ser encontrados em nosso repositório no GitHub: pvs-studio-cmake-examples .

Conclusão


Um grande público de usuários do CMake é ótimo para testar o projeto, mas muitos problemas podem ser evitados antes do lançamento usando ferramentas de análise de código estáticas, como o PVS-Studio .

Se você gostou dos resultados do analisador, mas seu projeto não está escrito em C e C ++, gostaria de lembrar que o analisador também suporta a análise de projetos em C # e Java. Você pode testar o analisador em seu projeto, acessando esta página .

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

More articles:


All Articles