Святослав Размыслов

19 Авг 2019

Теги:

#Cpp

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

19 Авг 2019

Автор: Святослав Размыслов

Введение
Непростительные ошибки
Утечка памяти
Утечка ресурсов
Странная логика в условиях
Разные предупреждения
Как найти ошибки в проекте на CMake
Заключение

CMake - это кроссплатформенная система автоматизации сборки проектов. Эта система намного старше, чем статический анализатор кода PVS-Studio, при этом ещё никто не попробовал применить его к коду и сделать обзор ошибок. Ошибок, оказывается, много. Аудитория CMake огромна. На нём начинаются новые проекты и переносятся старые. Страшно представить, у скольких программистов могла проявиться та или иная ошибка.

Введение

CMake (от англ. cross-platform make) - это кроссплатформенная система автоматизации сборки программного обеспечения из исходного кода. CMake не занимается непосредственно сборкой, а лишь генерирует файлы управления сборкой из файлов CMakeLists.txt. Первый выпуск программы состоялся в 2000 году. Для сравнения, статический анализатор PVS-Studio появился только в 2008 году. Тогда он был ориентирован на поиск ошибок портирования программ с 32-х битных систем на 64-битные, а в 2010 году появился первый набор диагностик общего назначения (V501-V545). Кстати, на коде CMake есть несколько предупреждений из этого первого набора.

Непростительные ошибки

V1040 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 только недавно была реализована. На момент публикации статьи, скорее всего, ещё не будет релиза с ней, но с помощью этой диагностики уже удалось найти крутую ошибку.

Здесь допустили опечатку в имени __MINGW32_. В конце не хватает одного символа подчёркивания. Если сделать поиск по коду с этим именем, то можно убедиться, что в проекте действительно используют версию именно с двумя подчёркиваниями с двух сторон:

V531 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:

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

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

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

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

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 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 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 Consider inspecting the '* s != '\0' && * 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 != '\0' && *s == ' ')
    s++;
  ....
}

Сравнение символа *s с терминальным нулём является лишним. Условие цикла while зависит только от того, равен символ пробелу или нет. Это не ошибка, но лишнее усложнение кода.

V592 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 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 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 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 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 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 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 файл и проверять проект с помощью плагина для 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.

Заключение

Огромная аудитория пользователей CMake является хорошими тестировщиками проекта, но многие проблемы можно было и не допускать до релиза, применяя такие инструменты статического анализа кода, как PVS-Studio.

Если вам понравились результаты работы анализатора, но ваш проект написан не на языках C и C++, то хочу напомнить, что анализатор поддерживает ещё анализ проектов на языках C# и Java. Попробовать анализатор на своём проекте можно, перейдя на эту страницу.

#Cpp