# CMake: the Case when the Project's Quality is Unforgivable CMake is a cross\-platform system for automating project builds\. This system is much older than the PVS\-Studio static code analyzer, but no one has tried to apply the analyzer on its code and review the errors\. As it turned out, there are a lot of them\. The CMake audience is huge\. New projects start on it and old ones are ported\. I shudder to think of how many developers could have had any given error\. ![0658_CMake/image1.png](https://import.viva64.com/docx/blog/0658_CMake/image1.png) ## Introduction [CMake](https://cmake.org/) is a cross\-platform system for automating software building from source code\. CMake isn't meant directly for building, it only generates files to control a build from CMakeLists\.txt files\. The first release of the program took place in 2000\. For comparison, the PVS\-Studio analyzer appeared only in 2008\. At that time, it was aimed at searching for bugs resulted from porting 32\-bit systems to 64\-bit ones\. In 2010, the first set of general purpose diagnostics appeared \(V501\-[V545](https://pvs-studio.com/en/docs/warnings/v545/)\)\. By the way, the CMake code has a few warnings from this first set\. ## Unforgivable Errors [V1040](https://pvs-studio.com/en/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 ``` The [V1040](https://pvs-studio.com/en/docs/warnings/v1040/) diagnostic was implemented not so long ago\. Most likely, at the time of posting the article, it won't be released yet, nevertheless, we already found a cool error with its help\. There's a typo made in the name _\_\_MINGW32\__\. At the end, one underline character is missing\. If you search the code with this name, you can see that the version with two underline characters on both sides is used in the project: ![0658_CMake/image3.png](https://import.viva64.com/docx/blog/0658_CMake/image3.png) [V531](https://pvs-studio.com/en/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)) { .... } .... } ``` For a statically declared array, the _sizeof_ operator will calculate size in bytes, taking into account the number of elements and their size\. When evaluating the value of the _cch\_subkeyname_ variable, a developer didn't take it into account and got a value 4 times greater than intended\. Let's explain where "four times" come from\. The array and its wrong size is passed to the function [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 ); ``` The _lpcchName_ pointer must point to the variable, containing the buffer size in characters: "A pointer to a variable that specifies the size of the buffer specified by the _lpClass_ parameter, in characters"\. The _subkeyname_ array size is 512 bytes and can store 256 characters of the _wchar\_t_ type \(in Windows, wchar\_t is 2 bytes\)\. It is 256 that should be passed to the function\. Instead, 512 is multiplied by 2 and we get 1024\. I think, it's clear now how to correct this error\. You need to use division instead of multiplication: ```cpp DWORD cch_subkeyname = sizeof(subkeyname) / sizeof(subkeyname[0]); ``` By the way, the same error occurs when evaluating the value of the _cch\_keyclass_ variable\. The error described can potentially lead to buffer overflow\. All such fragments definitely have to be corrected: * 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.com/en/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; } .... } ``` The pointer _this\-\>BuildFileStream_ is dereferenced right before the check for its validity\. Didn't that cause any problems for anyone? Below there is another example of such snippet\. It's made just like a carbon copy\. But in fact, there are a lot of [V595](https://pvs-studio.com/en/docs/warnings/v595/) warnings and most of them are not so obvious\. From my experience, I can say that correcting warnings of this diagnostic takes the longest time\. * V595 The 'this\-\>FlagFileStream' pointer was utilized before it was verified against nullptr\. Check lines: 303, 304\. cmMakefileTargetGenerator\.cxx 303 [V614](https://pvs-studio.com/en/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; }; ``` The analyzer detected usage of the uninitialized _str_ pointer\. It appeared due to an ordinary typo\. When calling the _SysAllocStringByteLen _function, one should have used the _src\.str_ pointer\. [V557](https://pvs-studio.com/en/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]); } .... } ``` This piece of code hides several problems at once\. When accessing _lengthbases_ and _lengthbits_ arrays, an array index might go out of bounds, as developers wrote the '\>' operator instead of '\>\=' above\. This check began to miss one unacceptable value\. Here we have nothing but a classic error pattern called [Off\-by\-one Error](https://cwe.mitre.org/data/definitions/193.html)\. Here's the entire list of array access operations by a non\-valid index: * 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 ## Memory Leak [V773](https://pvs-studio.com/en/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; } ``` The analyzer detected a memory leak\. The memory by the _testRun _pointer isn't released, if the function _testRun\-\>StartTest _returns _true_\. When executing another code branch, this memory gets released in the function _this\-\> FinishTestProcess\._ ## Resource Leak [V773](https://pvs-studio.com/en/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; } ``` ## Strange Logic in Conditions [V590](https://pvs-studio.com/en/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 _character comparison with null is redundant\. The condition of the _while_ loop depends only on whether the character is equal to a space or not\. This is not an error, but an unnecessary complication of the code\. [V592](https://pvs-studio.com/en/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); } .... } ``` The analyzer warns that the negation operation probably should be taken out of brackets\. It seems that there is no such a bug here \- just unnecessary double brackets\. But most likely, there is a logic error in the code\. The _continue_ operator is executed only in the case if the list of tests _this\-\>TestsToRun_ isn't empty and _cnt_ is absent in it\. It is reasonable to assume that if the tests list is empty, the same action needs to take place\. Most probably, the condition should be as follows: ```cpp if (this->TestsToRun.empty() || std::find(this->TestsToRun.begin(), this->TestsToRun.end(), cnt) == this->TestsToRun.end()) { continue; } ``` [V592](https://pvs-studio.com/en/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; } .... } ``` It's a similar example, but this time I'm more confident that an error takes place\. The function _IsSet\("CMAKE\_WARN\_DEPRECATED"\) _checks that the value _CMAKE\_WARN\_DEPRECATED _is set globally, and the function _IsOn\("CMAKE\_WARN\_DEPRECATED"\)_ checks that the value is set in the project configuration\. Most likely, the complementary operator is redundant, as in both cases, it's correct to set same values of _type _and _level\._ [V728](https://pvs-studio.com/en/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 "; } .... } ``` This code can be simpler\. One can rewrite the conditional expression in the following way: ```cpp } else if (success != this->TestProperties->WillFail) { this->TestResult.Status = cmCTestTestHandler::COMPLETED; outputStream << " Passed "; } ``` A few more places to simplify: * 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 ## Various Warnings [V523](https://pvs-studio.com/en/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)); } ``` The expression in the last condition is similar to the last two lines of the function\. A developer can simplify this code by removing the condition, or there is an error in the code and it should be fixed\. [V535](https://pvs-studio.com/en/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; } } } .... } ``` The _i_ variable is used as a loop counter in the outer and inner loops\. At the same time, the value of the counter again begins from zero in the inner loop\. It might not be a bug here, but the code is suspicious\. [V519](https://pvs-studio.com/en/docs/warnings/v519/) The 'tagString' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 84, 86\. cmCPackLog\.cxx 86 ```cpp 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"; } } .... } ``` The _tagString_ variable is overwritten with a new value in all places\. It's hard to say what's the issue or why they did it\. Perhaps, the '\=' and '\+\=' operators were muddled\. The entire list of such places: * 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.com/en/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); } ``` Forced setting of the _AES\_SET\_UTF8_ value looks suspicious\. I think such code will confuse any developer, who comes to refining this fragment\. This code was copied to another place: * V519 The 'aes\-\>aes\_set' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 4066, 4068\. archive\_string\.c 4068 ## How to Find Bugs in a Project on CMake In this section, I'll briefly tell you how to check CMake projects with PVS\-Studio as easy as one\-two\-three\. **Windows/Visual Studio** For Visual Studio, you can generate a project file using CMake GUI or the following command: ```cpp cmake -G "Visual Studio 15 2017 Win64" .. ``` Next, you can open the \.sln file and check the project using the [plugin](https://marketplace.visualstudio.com/items?itemName=EvgeniyRyzhkov.PVS-Studio) for Visual Studio\. **Linux/macOS** The file compile\_commands\.json is used for checks on these systems\. By the way, it can be generated in different build systems\. This is how you do it in CMake: ```cpp cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=On .. ``` The last thing to do is run the analyzer in the directory with the \.json file: ```cpp pvs-studio-analyzer analyze -l /path/to/PVS-Studio.lic -o /path/to/project.log -e /path/to/exclude-path -j ``` We have also developed a module for CMake projects\. Some people like using it\. CMake module and examples of its usage can be found in our repository on GitHub: [pvs\-studio\-cmake\-examples](https://github.com/viva64/pvs-studio-cmake-examples)\. ## Conclusion A huge audience of CMake users is great to test the project, but many issues could be prevented before the release by using static code analysis tools, such as [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/)\. If you liked the analyzer results, but your project isn't written in C and C\+\+, I'd like to remind that the analyzer also supports analysis of projects in C\# and Java\. You can test the analyzer on your project by going to this [page](https://pvs-studio.com/en/pvs-studio/download/)\.