Geekly articles weekly

CMake: o caso em que o projeto é imperdoável, a qualidade do seu código

Quadro 1

CMake é um sistema de automação de plataforma cruzada para projetos de construção. Esse sistema é muito mais antigo que o analisador de código estático do PVS-Studio, enquanto ninguém ainda tentou aplicá-lo ao código e revisar erros. Acontece que existem muitos erros. O público do CMake é enorme. Nele, novos projetos começam e antigos são transferidos. É assustador imaginar quantos programadores podem ter esse ou aquele erro.

1. Introdução


O CMake (da marca multiplataforma inglesa) é um sistema de automação multiplataforma para criar software a partir do código-fonte. O CMake não cria diretamente, mas apenas gera arquivos de controle de compilação a partir dos arquivos CMakeLists.txt. O primeiro lançamento do programa ocorreu em 2000. Para comparação, o analisador estático PVS-Studio apareceu apenas em 2008. Em seguida, concentrou-se em encontrar erros ao transportar programas de sistemas de 32 bits para 64 bits e, em 2010, o primeiro conjunto de diagnósticos de uso geral ( V501 - V545 ) apareceu. A propósito, existem alguns avisos desse primeiro conjunto no código CMake.

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 apenas recentemente. No momento da publicação do artigo, provavelmente, não haverá uma versão com ele, mas com a ajuda desse diagnóstico, já conseguimos encontrar um erro grave.

Aqui eles fizeram um erro de digitação no nome __MINGW32_ . No final, falta um sublinhado. Se você pesquisar por código com esse nome, verifique se o projeto realmente usa a versão com dois sublinhados nos dois lados:

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

Quando a matriz é declarada estaticamente, o operador sizeof calculará seu tamanho em bytes, levando em consideração o número de elementos e o tamanho dos elementos. Ao calcular o valor da variável cch_subkeyname, o programador não levou isso em consideração e recebeu um valor 4 vezes maior que o planejado. Vamos explicar onde é "4 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 uma variável que contém o tamanho do buffer especificado 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 subkeyname é de 512 bytes e é capaz de armazenar 256 caracteres do tipo wchar_t (no Windows wchar_t são 2 bytes). Este valor é 256 e deve ser passado para a função. Em vez disso, 512 é multiplicado por 2 novamente para obter 1024.

Eu acho que como corrigir o erro agora está claro. Em vez de multiplicação, use a divisão:

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

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

O erro descrito pode potencialmente levar a um estouro de buffer. É necessário consertar todos esses lugares:

  • 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 imediatamente antes da verificação de validação. Isso realmente estava causando problemas? Abaixo está outro exemplo de um lugar assim. É feito logo abaixo do papel carbono. Mas, de fato, existem muitos avisos do V595 e a maioria deles não é tão óbvia. Por experiência, posso dizer que a correção dos avisos desse diagnóstico é a mais longa.

  • 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 não inicializado str . E isso surgiu por causa do erro de digitação usual. Ao chamar a função SysAllocStringByteLen, você precisava usar 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]); } .... }

Vários problemas foram encontrados neste pedaço de código. Ao acessar as matrizes de lengthbases e lengthbits, é possível ir além do limite da matriz, porque acima do código, os desenvolvedores escreveram o operador '>' em vez de '> ='. Essa verificação começou a pular um valor inválido. Somos confrontados com um padrão de erro clássico chamado Erro Off-by-one .

A lista inteira de locais para acessar matrizes por índice invá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 pelo ponteiro testRun não será liberada se a função testRun-> StartTest retornar true . Quando outro ramo do código é executado, a memória usando o ponteiro testRun é 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++; .... }

Comparar o caractere * s com um terminal zero é supérfluo. 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 extra 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 alerta que talvez a negação deva ser colocada entre parênteses. Parece que não existe esse erro aqui - apenas colchetes duplos extras. Mas, muito provavelmente, há um erro lógico nessa condição.

A instrução continue é executada se a lista de testes this-> TestsToRun não estiver vazia e cnt estiver ausente nela. É lógico supor que, se a lista de testes estiver vazia, a mesma ação deverá ser executada. Muito provavelmente, a condição deve ser assim:

 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 aqui estou mais confiante na presença de um erro. 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á especificado na configuração do projeto. Muito provavelmente, o operador de negação é supérfluo, porque 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 "; } .... }

Esse código pode ser bastante simplificado reescrevendo a expressão condicional da seguinte maneira:

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

Mais alguns lugares que você pode 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

Avisos diversos


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 é idêntica às duas últimas linhas da função. Este código pode ser simplificado removendo a condição ou há um erro no código e 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 aninhados. Nesse caso, o valor do contador novamente começa a contar a partir do zero no anexo. Isso pode não ser um erro 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

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

A variável tagString é desgastada pelo novo valor em todos os lugares. É difícil dizer qual foi o erro ou por que eles o cometeram. Talvez os operadores '=' e '+ =' estejam 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); }

Forçar AES_SET_UTF8 parece suspeito. Eu acho que esse código enganará qualquer desenvolvedor que se depare com o refinamento deste lugar.

Este código foi copiado para mais um 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, explicarei um pouco como verificar projetos de maneira fácil e fácil no CMake usando o PVS-Studio.

Windows / Visual Studio

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

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

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

Linux / macOS

Nesses sistemas, o arquivo compile_commands.json é usado para verificar o projeto. A propósito, ele pode ser gerado em diferentes sistemas de montagem. No CMake, isso é feito assim:

 cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=On ..

Resta iniciar 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 do 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


O grande público de usuários do CMake é um bom testador do projeto, mas muitos problemas não poderiam ter sido evitados antes do lançamento, usando ferramentas de análise de código estático como o PVS-Studio .

Se você gostou dos resultados do analisador, mas seu projeto não está escrito em C e C ++, quero lembrá-lo de que o analisador também oferece suporte à análise de projetos em C # e Java. Você pode experimentar o analisador em seu projeto, acessando esta página.



Se você deseja compartilhar este artigo com um público que fala inglês, use o link para a tradução: Svyatoslav Razmyslov. CMake: o caso em que a qualidade do projeto é imperdoável .

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

More articles:


All Articles