- PVSM.RU - https://www.pvsm.ru -
CMake — это кроссплатформенная система автоматизации сборки проектов. Эта система намного старше, чем статический анализатор кода PVS-Studio, при этом ещё никто не попробовал применить его к коду и сделать обзор ошибок. Ошибок, оказывается, много. Аудитория CMake огромна. На нём начинаются новые проекты и переносятся старые. Страшно представить, у скольких программистов могла проявиться та или иная ошибка.
CMake [1] (от англ. cross-platform make) — это кроссплатформенная система автоматизации сборки программного обеспечения из исходного кода. CMake не занимается непосредственно сборкой, а лишь генерирует файлы управления сборкой из файлов CMakeLists.txt. Первый выпуск программы состоялся в 2000 году. Для сравнения, статический анализатор PVS-Studio [2] появился только в 2008 году. Тогда он был ориентирован на поиск ошибок портирования программ с 32-х битных систем на 64-битные, а в 2010 году появился первый набор диагностик общего назначения (V501 [3]-V545 [4]). Кстати, на коде CMake есть несколько предупреждений из этого первого набора.
V1040 [5] Possible typo in the spelling of a pre-defined macro name. The '__MINGW32_' macro is similar to '__MINGW32__'. winapi.h 4112
/* from winternl.h */
#if !defined(__UNICODE_STRING_DEFINED) && defined(__MINGW32_)
#define __UNICODE_STRING_DEFINED
#endif
Диагностика V1040 [5] только недавно была реализована. На момент публикации статьи, скорее всего, ещё не будет релиза с ней, но с помощью этой диагностики уже удалось найти крутую ошибку.
Здесь допустили опечатку в имени __MINGW32_. В конце не хватает одного символа подчёркивания. Если сделать поиск по коду с этим именем, то можно убедиться, что в проекте действительно используют версию именно с двумя подчёркиваниями с двух сторон:
V531 [6] It is odd that a sizeof() operator is multiplied by 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)) {
....
}
....
}
Когда массив объявлен статически, то оператор sizeof посчитает его размер в байтах с учётом и количества элементов, и размера элементов. При вычислении значения переменной cch_subkeyname программист не учёл этого и получил значение в 4 раза больше запланированного. Поясним откуда это «в 4 раза».
Массив и его неправильный размер передаётся в функцию RegEnumKeyExW [7]:
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.
Думаю, как исправить ошибку теперь понятно. Нужно вместо умножения использовать деление:
DWORD cch_subkeyname = sizeof(subkeyname) / sizeof(subkeyname[0]);
Кстати, точно такая же ошибка происходит и при вычислении значения переменной cch_keyclass.
Описанная ошибка может потенциально привести к переполнению буфера. Нужно обязательно исправить все такие места:
V595 [8] The 'this->BuildFileStream' pointer was utilized before it was verified against nullptr. Check lines: 133, 134. cmMakefileTargetGenerator.cxx 133
void cmMakefileTargetGenerator::CreateRuleFile()
{
....
this->BuildFileStream->SetCopyIfDifferent(true);
if (!this->BuildFileStream) {
return;
}
....
}
Указатель this->BuildFileStream разыменовывается прямо перед проверкой на валидность. Неужели это ни у кого не вызывало проблем? Ниже ещё один пример такого места. Оно сделано прямо под копирку. Но на самом деле, предупреждений V595 [8] очень много и большинство из них не такие очевидные. По опыту могу сказать, что исправлять предупреждения этой диагностики дольше всего.
V614 [9] Uninitialized pointer 'str' used. 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;
};
Анализатор обнаружил использование неинициализированного указателя str. А возникло это из-за обычной опечатки. При вызове функции SysAllocStringByteLen надо было использовать указатель src.str.
V557 [10] Array overrun is possible. The value of 'lensymbol' index could reach 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]);
}
....
}
Сразу несколько проблем обнаружено в этом фрагменте кода. При обращении к массивам lengthbases и lengthbits возможен выход за границу массива, потому что выше по коду разработчики написали оператор '>' вместо '>='. Такая проверка стала пропускать одно недопустимое значение. Перед нами не что иное, как классический паттерн ошибки под названием Off-by-one Error [11].
Весь список мест обращений к массивам по невалидному индексу:
V773 [12] The function was exited without releasing the 'testRun' pointer. A memory leak is possible. 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;
}
Анализатор обнаружил утечку памяти. Память по указателю testRun не освобождается, если функция testRun->StartTest возвращает true. При выполнении другой ветви кода память по указателю testRun освобождается в функции this-> FinishTestProcess.
V773 [12] The function was exited without closing the file referenced by the 'fd' handle. A resource leak is possible. 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;
}
V590 [13] Consider inspecting the '* s != '' && * s == ' '' expression. The expression is excessive or contains a misprint. 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 != '' && *s == ' ')
s++;
....
}
Сравнение символа *s с терминальным нулём является лишним. Условие цикла while зависит только от того, равен символ пробелу или нет. Это не ошибка, но лишнее усложнение кода.
V592 [14] The expression was enclosed by parentheses twice: ((expression)). One pair of parentheses is unnecessary or misprint is present. 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);
}
....
}
Анализатор предупреждает, что, возможно, отрицание должно быть вынесено за скобки. Вроде, такой ошибки тут нет — просто лишние двойные скобки. Но, скорее всего, в этом условии присутствует логическая ошибка.
Оператор continue выполняется в том случае, если список тестов this->TestsToRun не пуст и cnt в нём отсутствует. Логично предположить, что если список тестов пуст, то необходимо выполнить то же самое действие. Скорее всего, условие должно быть таким:
if (this->TestsToRun.empty() ||
std::find(this->TestsToRun.begin(), this->TestsToRun.end(), cnt) ==
this->TestsToRun.end()) {
continue;
}
V592 [14] The expression was enclosed by parentheses twice: ((expression)). One pair of parentheses is unnecessary or misprint is present. 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;
}
....
}
Похожий пример, но тут я больше уверен в наличии ошибки. Функция IsSet(«CMAKE_WARN_DEPRECATED») проверяет, что значение CMAKE_WARN_DEPRECATED задано глобально, а функция IsOn(«CMAKE_WARN_DEPRECATED») проверяет, что значение задано в конфигурации проекта. Скорее всего, оператор отрицания является лишним, т.к. в обоих случаях корректно задать одинаковые значения type и level.
V728 [15] An excessive check can be simplified. The '(A && !B) || (!A && B)' expression is equivalent to the 'bool(A) != bool(B)' expression. 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 ";
}
....
}
Такой код можно сильно упростить, если переписать условное выражение таким образом:
} else if (success != this->TestProperties->WillFail)
{
this->TestResult.Status = cmCTestTestHandler::COMPLETED;
outputStream << " Passed ";
}
Ещё несколько мест, которые можно упростить:
V523 [16] The 'then' statement is equivalent to the subsequent code fragment. 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));
}
Выражение в последнем условии идентично последним двум строкам функции. Этот код можно упростить, удалив условие, либо в коде присутствует ошибка и её следует исправить.
V535 [17] The variable 'i' is being used for this loop and for the outer loop. Check lines: 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;
}
}
}
....
}
Переменная i используется как счётчик цикла во внешнем и вложенном циклах. При этом значение счётчика снова начинает отсчитываться от нуля во вложенном. Возможно, здесь это не ошибка, но код подозрительный.
V519 [18] The 'tagString' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 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";
}
}
....
}
Переменная tagString перетирается новым значением во всех местах. Сложно сказать, в чём ошибка, или зачем так сделали. Возможно, перепутали операторы '=' и '+='.
Весь список таких мест:
V519 [18] The 'aes->aes_set' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 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);
}
Принудительная установка значения AES_SET_UTF8 выглядит подозрительно. Я думаю, такой код введёт в заблуждение любого разработчика, который столкнётся с доработкой этого места.
Такой код скопировали ещё в одно место:
В этом разделе я немного расскажу, как легко и просто проверять проекты на CMake с помощью PVS-Studio.
Windows/Visual Studio
Для Visual Studio можно сгенерировать проектный файл с помощью CMake GUI или следующей команды:
cmake -G "Visual Studio 15 2017 Win64" ..
Далее можно открыть .sln файл и проверять проект с помощью плагина [19] для Visual Studio.
Linux/macOS
На этих системах для проверки проекта используется файл compile_commands.json. Кстати, его можно сгенерировать в разных сборочных системах. В CMake это делается так:
cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=On ..
Осталось запустить анализатор в каталоге с .json файлом:
pvs-studio-analyzer analyze -l /path/to/PVS-Studio.lic
-o /path/to/project.log -e /path/to/exclude-path -j<N>
Также мы разработали модуль для CMake-проектов. Некоторым нравится использовать его. CMake-модуль и примеры его использования можно найти в нашем репозитории на GitHub: pvs-studio-cmake-examples [20].
Огромная аудитория пользователей CMake является хорошими тестировщиками проекта, но многие проблемы можно было и не допускать до релиза, применяя такие инструменты статического анализа кода, как PVS-Studio [2].
Если вам понравились результаты работы анализатора, но ваш проект написан не на языках C и C++, то хочу напомнить, что анализатор поддерживает ещё анализ проектов на языках C# и Java. Попробовать анализатор на своём проекте можно, перейдя на эту [21] страницу.
Автор: SvyatoslavMC
Источник [22]
Сайт-источник PVSM.RU: https://www.pvsm.ru
Путь до страницы источника: https://www.pvsm.ru/pvs-studio/327483
Ссылки в тексте:
[1] CMake: https://cmake.org/
[2] PVS-Studio: https://www.viva64.com/ru/pvs-studio/
[3] V501: https://www.viva64.com/ru/w/v501/
[4] V545: https://www.viva64.com/ru/w/v545/
[5] V1040: https://www.viva64.com/ru/w/v1040/
[6] V531: https://www.viva64.com/ru/w/v531/
[7] RegEnumKeyExW: https://docs.microsoft.com/en-us/windows/win32/api/winreg/nf-winreg-regenumkeyexw
[8] V595: https://www.viva64.com/ru/w/v595/
[9] V614: https://www.viva64.com/ru/w/v614/
[10] V557: https://www.viva64.com/ru/w/v557/
[11] Off-by-one Error: https://cwe.mitre.org/data/definitions/193.html
[12] V773: https://www.viva64.com/ru/w/v773/
[13] V590: https://www.viva64.com/ru/w/v590/
[14] V592: https://www.viva64.com/ru/w/v592/
[15] V728: https://www.viva64.com/ru/w/v728/
[16] V523: https://www.viva64.com/ru/w/v523/
[17] V535: https://www.viva64.com/ru/w/v535/
[18] V519: https://www.viva64.com/ru/w/v519/
[19] плагина: https://marketplace.visualstudio.com/items?itemName=EvgeniyRyzhkov.PVS-Studio
[20] pvs-studio-cmake-examples: https://github.com/viva64/pvs-studio-cmake-examples
[21] эту: https://www.viva64.com/ru/pvs-studio-download/
[22] Источник: https://habr.com/ru/post/464147/?utm_campaign=464147&utm_source=habrahabr&utm_medium=rss
Нажмите здесь для печати.