- PVSM.RU - https://www.pvsm.ru -

CMake: тот случай, когда проекту непростительно качество его кода

Picture 1

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_. В конце не хватает одного символа подчёркивания. Если сделать поиск по коду с этим именем, то можно убедиться, что в проекте действительно используют версию именно с двумя подчёркиваниями с двух сторон:

Picture 3

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.

Описанная ошибка может потенциально привести к переполнению буфера. Нужно обязательно исправить все такие места:

  • 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 [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] очень много и большинство из них не такие очевидные. По опыту могу сказать, что исправлять предупреждения этой диагностики дольше всего.

  • V595 The 'this->FlagFileStream' pointer was utilized before it was verified against nullptr. Check lines: 303, 304. cmMakefileTargetGenerator.cxx 303

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].

Весь список мест обращений к массивам по невалидному индексу:

  • 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 [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  ";
}

Ещё несколько мест, которые можно упростить:

  • 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 [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 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 [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 выглядит подозрительно. Я думаю, такой код введёт в заблуждение любого разработчика, который столкнётся с доработкой этого места.

Такой код скопировали ещё в одно место:

  • 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 или следующей команды:

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