# CMake: тот случай, когда проекту непростительно качество его кода CMake \- это кроссплатформенная система автоматизации сборки проектов\. Эта система намного старше, чем статический анализатор кода PVS\-Studio, при этом ещё никто не попробовал применить его к коду и сделать обзор ошибок\. Ошибок, оказывается, много\. Аудитория CMake огромна\. На нём начинаются новые проекты и переносятся старые\. Страшно представить, у скольких программистов могла проявиться та или иная ошибка\. ![0658_CMake_ru/image1.png](https://import.viva64.com/docx/blog/0658_CMake_ru/image1.png) ## Введение [CMake](https://cmake.org/) \(от англ\. cross\-platform make\) \- это кроссплатформенная система автоматизации сборки программного обеспечения из исходного кода\. CMake не занимается непосредственно сборкой, а лишь генерирует файлы управления сборкой из файлов CMakeLists\.txt\. Первый выпуск программы состоялся в 2000 году\. Для сравнения, статический анализатор [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/) появился только в 2008 году\. Тогда он был ориентирован на поиск ошибок портирования программ с 32\-х битных систем на 64\-битные, а в 2010 году появился первый набор диагностик общего назначения \([V501](https://pvs-studio.ru/ru/docs/warnings/v501/)\-[V545](https://pvs-studio.ru/ru/docs/warnings/v545/)\)\. Кстати, на коде CMake есть несколько предупреждений из этого первого набора\. ## Непростительные ошибки [V1040](https://pvs-studio.ru/ru/docs/warnings/v1040/) Possible typo in the spelling of a pre\-defined macro name\. The '\_\_MINGW32\_' macro is similar to '\_\_MINGW32\_\_'\. winapi\.h 4112 ```cpp /* from winternl.h */ #if !defined(__UNICODE_STRING_DEFINED) && defined(__MINGW32_) #define __UNICODE_STRING_DEFINED #endif ``` Диагностика [V1040](https://pvs-studio.ru/ru/docs/warnings/v1040/) только недавно была реализована\. На момент публикации статьи, скорее всего, ещё не будет релиза с ней, но с помощью этой диагностики уже удалось найти крутую ошибку\. Здесь допустили опечатку в имени _\_\_MINGW32\__\. В конце не хватает одного символа подчёркивания\. Если сделать поиск по коду с этим именем, то можно убедиться, что в проекте действительно используют версию именно с двумя подчёркиваниями с двух сторон: ![0658_CMake_ru/image3.png](https://import.viva64.com/docx/blog/0658_CMake_ru/image3.png) [V531](https://pvs-studio.ru/ru/docs/warnings/v531/) It is odd that a sizeof\(\) operator is multiplied by sizeof\(\)\. cmGlobalVisualStudioGenerator\.cxx 558 ```cpp 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)) { .... } .... } ``` Когда массив объявлен статически, то оператор _sizeof_ посчитает его размер в байтах с учётом и количества элементов, и размера элементов\. При вычислении значения переменной _cch\_subkeyname_ программист не учёл этого и получил значение в 4 раза больше запланированного\. Поясним откуда это "в 4 раза"\. Массив и его неправильный размер передаётся в функцию [RegEnumKeyExW](https://docs.microsoft.com/en-us/windows/win32/api/winreg/nf-winreg-regenumkeyexw): ```cpp LSTATUS RegEnumKeyExW( HKEY hKey, DWORD dwIndex, LPWSTR lpName, // <= subkeyname LPDWORD lpcchName, // <= cch_subkeyname LPDWORD lpReserved, LPWSTR lpClass, LPDWORD lpcchClass, PFILETIME lpftLastWriteTime ); ``` Указатель _lpcchName_ должен указывать на переменную, содержащую размер буфера в символах: "A pointer to a variable that specifies the size of the buffer specified by the _lpClass_ parameter, in characters"\. Размер массива _subkeyname_ составляет 512 байт и он способен хранить 256 символов типа _wchar\_t_ \(в Windows wchar\_t это 2 байта\)\. Именно это значение в 256 и должно быть передано в функцию\. Вместо этого 512 ещё раз умножается на 2 и получается 1024\. Думаю, как исправить ошибку теперь понятно\. Нужно вместо умножения использовать деление: ```cpp DWORD cch_subkeyname = sizeof(subkeyname) / sizeof(subkeyname[0]); ``` Кстати, точно такая же ошибка происходит и при вычислении значения переменной _cch\_keyclass_\. Описанная ошибка может потенциально привести к переполнению буфера\. Нужно обязательно исправить все такие места: * V531 It is odd that a sizeof\(\) operator is multiplied by sizeof\(\)\. cmGlobalVisualStudioGenerator\.cxx 556 * V531 It is odd that a sizeof\(\) operator is multiplied by sizeof\(\)\. cmGlobalVisualStudioGenerator\.cxx 572 * V531 It is odd that a sizeof\(\) operator is multiplied by sizeof\(\)\. cmGlobalVisualStudioGenerator\.cxx 621 * V531 It is odd that a sizeof\(\) operator is multiplied by sizeof\(\)\. cmGlobalVisualStudioGenerator\.cxx 622 * V531 It is odd that a sizeof\(\) operator is multiplied by sizeof\(\)\. cmGlobalVisualStudioGenerator\.cxx 649 [V595](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'this\-\>BuildFileStream' pointer was utilized before it was verified against nullptr\. Check lines: 133, 134\. cmMakefileTargetGenerator\.cxx 133 ```cpp void cmMakefileTargetGenerator::CreateRuleFile() { .... this->BuildFileStream->SetCopyIfDifferent(true); if (!this->BuildFileStream) { return; } .... } ``` Указатель _this\-\>BuildFileStream_ разыменовывается прямо перед проверкой на валидность\. Неужели это ни у кого не вызывало проблем? Ниже ещё один пример такого места\. Оно сделано прямо под копирку\. Но на самом деле, предупреждений [V595](https://pvs-studio.ru/ru/docs/warnings/v595/) очень много и большинство из них не такие очевидные\. По опыту могу сказать, что исправлять предупреждения этой диагностики дольше всего\. * V595 The 'this\-\>FlagFileStream' pointer was utilized before it was verified against nullptr\. Check lines: 303, 304\. cmMakefileTargetGenerator\.cxx 303 [V614](https://pvs-studio.ru/ru/docs/warnings/v614/) Uninitialized pointer 'str' used\. cmVSSetupHelper\.h 80 ```cpp 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; }; ``` Анализатор обнаружил использование неинициализированного указателя _str_\. А возникло это из\-за обычной опечатки\. При вызове функции _SysAllocStringByteLen_ надо было использовать указатель _src\.str_\. [V557](https://pvs-studio.ru/ru/docs/warnings/v557/) Array overrun is possible\. The value of 'lensymbol' index could reach 28\. archive\_read\_support\_format\_rar\.c 2749 ```cpp 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]); } .... } ``` Сразу несколько проблем обнаружено в этом фрагменте кода\. При обращении к массивам _lengthbases_ и _lengthbits_ возможен выход за границу массива, потому что выше по коду разработчики написали оператор '\>' вместо '\>\='\. Такая проверка стала пропускать одно недопустимое значение\. Перед нами не что иное, как классический паттерн ошибки под названием [Off\-by\-one Error](https://cwe.mitre.org/data/definitions/193.html)\. Весь список мест обращений к массивам по невалидному индексу: * V557 Array overrun is possible\. The value of 'lensymbol' index could reach 28\. archive\_read\_support\_format\_rar\.c 2750 * V557 Array overrun is possible\. The value of 'lensymbol' index could reach 28\. archive\_read\_support\_format\_rar\.c 2751 * V557 Array overrun is possible\. The value of 'lensymbol' index could reach 28\. archive\_read\_support\_format\_rar\.c 2753 * V557 Array overrun is possible\. The value of 'lensymbol' index could reach 28\. archive\_read\_support\_format\_rar\.c 2754 * V557 Array overrun is possible\. The value of 'offssymbol' index could reach 60\. archive\_read\_support\_format\_rar\.c 2797 ## Утечка памяти [V773](https://pvs-studio.ru/ru/docs/warnings/v773/) The function was exited without releasing the 'testRun' pointer\. A memory leak is possible\. cmCTestMultiProcessHandler\.cxx 193 ```cpp 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; } ``` Анализатор обнаружил утечку памяти\. Память по указателю _testRun_ не освобождается, если функция _testRun\-\>StartTest_ возвращает _true_\. При выполнении другой ветви кода память по указателю _testRun_ освобождается в функции _this\-\> FinishTestProcess_\. ## Утечка ресурсов [V773](https://pvs-studio.ru/ru/docs/warnings/v773/) The function was exited without closing the file referenced by the 'fd' handle\. A resource leak is possible\. rhash\.c 450 ```cpp 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; } ``` ## Странная логика в условиях [V590](https://pvs-studio.ru/ru/docs/warnings/v590/) Consider inspecting the '\* s \!\= '\\0' && \* s \=\= ' '' expression\. The expression is excessive or contains a misprint\. archive\_cmdline\.c 76 ```cpp 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++; .... } ``` Сравнение символа _\*s_ с терминальным нулём является лишним\. Условие цикла _while_ зависит только от того, равен символ пробелу или нет\. Это не ошибка, но лишнее усложнение кода\. [V592](https://pvs-studio.ru/ru/docs/warnings/v592/) The expression was enclosed by parentheses twice: \(\(expression\)\)\. One pair of parentheses is unnecessary or misprint is present\. cmCTestTestHandler\.cxx 899 ```cpp 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); } .... } ``` Анализатор предупреждает, что, возможно, отрицание должно быть вынесено за скобки\. Вроде, такой ошибки тут нет \- просто лишние двойные скобки\. Но, скорее всего, в этом условии присутствует логическая ошибка\. Оператор _continue_ выполняется в том случае, если список тестов _this\-\>TestsToRun_ не пуст и _cnt_ в нём отсутствует\. Логично предположить, что если список тестов пуст, то необходимо выполнить то же самое действие\. Скорее всего, условие должно быть таким: ```cpp if (this->TestsToRun.empty() || std::find(this->TestsToRun.begin(), this->TestsToRun.end(), cnt) == this->TestsToRun.end()) { continue; } ``` [V592](https://pvs-studio.ru/ru/docs/warnings/v592/) The expression was enclosed by parentheses twice: \(\(expression\)\)\. One pair of parentheses is unnecessary or misprint is present\. cmMessageCommand\.cxx 73 ```cpp bool cmMessageCommand::InitialPass(std::vector 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; } .... } ``` Похожий пример, но тут я больше уверен в наличии ошибки\. Функция _IsSet\("CMAKE\_WARN\_DEPRECATED"\)_ проверяет, что значение _CMAKE\_WARN\_DEPRECATED_ задано глобально, а функция _IsOn\("CMAKE\_WARN\_DEPRECATED"\)_ проверяет, что значение задано в конфигурации проекта\. Скорее всего, оператор отрицания является лишним, т\.к\. в обоих случаях корректно задать одинаковые значения _type_ и _level_\. [V728](https://pvs-studio.ru/ru/docs/warnings/v728/) An excessive check can be simplified\. The '\(A && \!B\) \|\| \(\!A && B\)' expression is equivalent to the 'bool\(A\) \!\= bool\(B\)' expression\. cmCTestRunTest\.cxx 151 ```cpp 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 "; } .... } ``` Такой код можно сильно упростить, если переписать условное выражение таким образом: ```cpp } else if (success != this->TestProperties->WillFail) { this->TestResult.Status = cmCTestTestHandler::COMPLETED; outputStream << " Passed "; } ``` Ещё несколько мест, которые можно упростить: * V728 An excessive check can be simplified\. The '\(A && B\) \|\| \(\!A && \!B\)' expression is equivalent to the 'bool\(A\) \=\= bool\(B\)' expression\. cmCTestTestHandler\.cxx 702 * V728 An excessive check can be simplified\. The '\(A && \!B\) \|\| \(\!A && B\)' expression is equivalent to the 'bool\(A\) \!\= bool\(B\)' expression\. digest\_sspi\.c 443 * V728 An excessive check can be simplified\. The '\(A && \!B\) \|\| \(\!A && B\)' expression is equivalent to the 'bool\(A\) \!\= bool\(B\)' expression\. tcp\.c 1295 * V728 An excessive check can be simplified\. The '\(A && \!B\) \|\| \(\!A && B\)' expression is equivalent to the 'bool\(A\) \!\= bool\(B\)' expression\. testDynamicLoader\.cxx 58 * V728 An excessive check can be simplified\. The '\(A && \!B\) \|\| \(\!A && B\)' expression is equivalent to the 'bool\(A\) \!\= bool\(B\)' expression\. testDynamicLoader\.cxx 65 * V728 An excessive check can be simplified\. The '\(A && \!B\) \|\| \(\!A && B\)' expression is equivalent to the 'bool\(A\) \!\= bool\(B\)' expression\. testDynamicLoader\.cxx 72 ## Разные предупреждения [V523](https://pvs-studio.ru/ru/docs/warnings/v523/) The 'then' statement is equivalent to the subsequent code fragment\. archive\_read\_support\_format\_ar\.c 415 ```cpp 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)); } ``` Выражение в последнем условии идентично последним двум строкам функции\. Этот код можно упростить, удалив условие, либо в коде присутствует ошибка и её следует исправить\. [V535](https://pvs-studio.ru/ru/docs/warnings/v535/) The variable 'i' is being used for this loop and for the outer loop\. Check lines: 2220, 2241\. multi\.c 2241 ```cpp 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; } } } .... } ``` Переменная _i_ используется как счётчик цикла во внешнем и вложенном циклах\. При этом значение счётчика снова начинает отсчитываться от нуля во вложенном\. Возможно, здесь это не ошибка, но код подозрительный\. [V519](https://pvs-studio.ru/ru/docs/warnings/v519/) The 'tagString' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 84, 86\. cmCPackLog\.cxx 86 ```cpp 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"; } } .... } ``` Переменная _tagString_ перетирается новым значением во всех местах\. Сложно сказать, в чём ошибка, или зачем так сделали\. Возможно, перепутали операторы '\=' и '\+\='\. Весь список таких мест: * V519 The 'tagString' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 94, 96\. cmCPackLog\.cxx 96 * V519 The 'tagString' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 104, 106\. cmCPackLog\.cxx 106 * V519 The 'tagString' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 114, 116\. cmCPackLog\.cxx 116 * V519 The 'tagString' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 125, 127\. cmCPackLog\.cxx 127 [V519](https://pvs-studio.ru/ru/docs/warnings/v519/) The 'aes\-\>aes\_set' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 4052, 4054\. archive\_string\.c 4054 ```cpp 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); } ``` Принудительная установка значения _AES\_SET\_UTF8_ выглядит подозрительно\. Я думаю, такой код введёт в заблуждение любого разработчика, который столкнётся с доработкой этого места\. Такой код скопировали ещё в одно место: * V519 The 'aes\-\>aes\_set' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 4066, 4068\. archive\_string\.c 4068 ## Как найти ошибки в проекте на CMake В этом разделе я немного расскажу, как легко и просто проверять проекты на CMake с помощью PVS\-Studio\. **Windows/Visual Studio** Для Visual Studio можно сгенерировать проектный файл с помощью CMake GUI или следующей команды: ```cpp cmake -G "Visual Studio 15 2017 Win64" .. ``` Далее можно открыть \.sln файл и проверять проект с помощью [плагина](https://marketplace.visualstudio.com/items?itemName=EvgeniyRyzhkov.PVS-Studio) для Visual Studio\. **Linux/macOS** На этих системах для проверки проекта используется файл compile\_commands\.json\. Кстати, его можно сгенерировать в разных сборочных системах\. В CMake это делается так: ```cpp cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=On .. ``` Осталось запустить анализатор в каталоге с \.json файлом: ```cpp pvs-studio-analyzer analyze -l /path/to/PVS-Studio.lic -o /path/to/project.log -e /path/to/exclude-path -j ``` Также мы разработали модуль для CMake\-проектов\. Некоторым нравится использовать его\. CMake\-модуль и примеры его использования можно найти в нашем репозитории на GitHub: [pvs\-studio\-cmake\-examples](https://github.com/viva64/pvs-studio-cmake-examples)\. ## Заключение Огромная аудитория пользователей CMake является хорошими тестировщиками проекта, но многие проблемы можно было и не допускать до релиза, применяя такие инструменты статического анализа кода, как [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/)\. Если вам понравились результаты работы анализатора, но ваш проект написан не на языках C и C\+\+, то хочу напомнить, что анализатор поддерживает ещё анализ проектов на языках C\# и Java\. Попробовать анализатор на своём проекте можно, перейдя на [эту](https://pvs-studio.ru/ru/pvs-studio/download/) страницу\.