PVS-Studio, PMDK

Нам предложили проверить с помощью анализатора PVS-Studio коллекцию открытых библиотек PMDK, предназначенную для разработки и отладки приложений с поддержкой энергонезависимой памяти. Собственно, почему бы и нет. Тем более это небольшой проект на языке C и C++ с общим размером кодовой базы около 170 KLOC, если не считать комментарии. А значит, обзор результатов анализа не займёт много сил и времени. Let's go.

Для анализа исходного кода будет использован инструмент PVS-Studio версии 7.08. Читатели нашего блога, естественно, давно знакомы с нашим инструментом, и я не буду на нём останавливаться. Для тех, кто впервые зашёл к нам, предлагаю обратиться к статье "Как быстро посмотреть интересные предупреждения, которые выдает анализатор PVS-Studio для C и C++ кода?" и попробовать бесплатную триальную версию анализатора.

В этот раз я загляну внутрь проекта PMDK и расскажу о замеченных ошибках и недочётах. По моему внутреннему ощущению, их было немного, что говорит о хорошем качестве кода этого проекта. Из интересного можно отметить, что найдено несколько фрагментов неправильного кода, который тем не менее работает правильно :). Что я имею в виду станет понятнее из дальнейшего повествования.

Итак, PMDK — это коллекция библиотек и инструментальных средств с открытым исходным кодом, предназначенных для упрощения разработки, отладки и управления приложениями с поддержкой энергонезависимой памяти. Подробнее здесь: PMDK Introduction. Исходники здесь: pmdk.

Давайте посмотрим, какие ошибки и недочёты я смогу в нём найти. Сразу скажу, что я был далеко не всегда внимателен при анализе отчёта и мог многое пропустить. Поэтому призываю авторов проекта не руководствоваться при исправлении дефектов исключительно этой статьёй, а перепроверить код самостоятельно. А для написания статьи мне будет достаточно и того, что я выписал, просматривая список предупреждений :).

Неправильный код, который работает


Размер выделяемой памяти


Программисты нередко проводят время за отладкой кода, когда программа ведёт себя не так, как надо. Однако, иногда бывают ситуации, когда программа и работает корректно, но при этом код содержит ошибку. Программисту просто повезло, и ошибка не проявляет себя. В проекте PMDK я встретил сразу несколько таких интересных ситуаций и поэтому решил собрать их вместе в отдельной главе.

int main(int argc, char *argv[])
{
  ....
  struct pool *pop = malloc(sizeof(pop));
  ....
}

Предупреждение PVS-Studio: V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'pop' class object. util_ctl.c 717

Классическая опечатка, из-за которой выделяется неверное количество памяти. Оператор sizeof вернёт не размер структуры, а размер указателя на эту структуру. Правильным вариантом будет:

struct pool *pop = malloc(sizeof(pool));

или

struct pool *pop = malloc(sizeof(*pop));

Однако, этот неправильно написанный код прекрасно работает. Дело в том, что структура pool содержит в себе ровно один указатель:

struct pool {
  struct ctl *ctl;
};

Получается, что структура занимает ровно столько, сколько и указатель. Всё хорошо.

Длина строки


Перейдём к следующему случаю, где вновь допущена ошибка с использованием оператора sizeof.

typedef void *(*pmem2_memcpy_fn)(void *pmemdest, const void *src, size_t len,
    unsigned flags);

static const char *initial_state = "No code.";

static int
test_rwx_prot_map_priv_do_execute(const struct test_case *tc,
  int argc, char *argv[])
{
  ....
  char *addr_map = pmem2_map_get_address(map);
  map->memcpy_fn(addr_map, initial_state, sizeof(initial_state), 0);
  ....
}

Предупреждение PVS-Studio: V579 [CWE-687] The memcpy_fn function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. pmem2_map_prot.c 513

Для копирования строки используется указатель на специальную функцию копирования. Обратите внимание на вызов этой функции, а вернее на её третий аргумент.

Программист предполагает, что оператор sizeof вычислит размер строкового литерала. Но, на самом деле, вновь вычисляется размер указателя.

Везение в том, что строка состоит из 8 символов, и её размер совпадает с размером указателя, если происходит сборка 64-битного приложения. В результате все 8 символов строки "No code." будут успешно скопированы.

На самом деле, ситуация ещё сложнее и интереснее. Интерпретация этой ошибки зависит от того, хотели ли скопировать терминальный ноль или нет. Рассмотрим два сценария.

Сценарий 1. Нужно было скопировать терминальный ноль. Тогда я неправ, и это вовсе не безобидная ошибка, которая себя не проявляет. Скопировано не 9 байт, а только 8. Терминального нуля нет, и последствия предсказать невозможно. В этом случае код можно исправить, изменив определение константной строки initial_state следующим образом:

static const char initial_state [] = "No code.";

Теперь значение sizeof(initial_state) равно 9.

Сценарий 2. Терминальный ноль вовсе и не требуется. Например, ниже можно увидеть вот такую строчку кода:

UT_ASSERTeq(memcmp(addr_map, initial_state, strlen(initial_state)), 0);

Как видите, функция strlen вернёт значение 8 и терминальный ноль не участвует в сравнении. Тогда это действительно везение и всё хорошо.

Побитовый сдвиг


Следующий пример связан с операцией побитового сдвига.

static int
clo_parse_single_uint(struct benchmark_clo *clo, const char *arg, void *ptr)
{
  ....
  uint64_t tmax = ~0 >> (64 - 8 * clo->type_uint.size);
  ....
}

Предупреждение PVS-Studio: V610 [CWE-758] Unspecified behavior. Check the shift operator '>>'. The left operand '~0' is negative. clo.cpp 205

Результат сдвига отрицательного значения вправо зависит от реализации компилятора. Поэтому, хотя такой код может работать правильно и ожидаемо при всех существующих на данный момент режимах компиляции приложения, это всё равно везение.

Приоритет операций


И рассмотрим последний случай, связанный с приоритетом операций.

#define BTT_CREATE_DEF_SIZE  (20 * 1UL << 20) /* 20 MB */

Предупреждение PVS-Studio: V634 [CWE-783] The priority of the '*' operation is higher than that of the '<<' operation. It's possible that parentheses should be used in the expression. bttcreate.c 204

Чтобы получить константу, равную значению 20 MB, программист решил выполнить следующие действия:

  • Сдвинул 1 на 20 разрядов, чтобы получить значение 1048576, т.е. 1 MB.
  • Умножил 1 MB на 20.

Другими словами, программист думает, что вычисления происходят так: (20 * (1UL << 20))

Но на самом деле приоритет оператора умножения выше, чем приоритет оператора сдвига и выражение вычисляется так: ((20 * 1UL) << 20).

Согласитесь, вряд ли программист хотел, чтобы выражение вычислилось в такой последовательности. Нет смысла в умножении 20 на 1. Так что перед нами тот случай, когда код работает не так, как задумывал программист.

Но эта ошибка никак не проявит себя. Неважно, как написать:

  • (20 * 1UL << 20)
  • (20 * (1UL << 20))
  • ((20 * 1UL) << 20)

Результат всё равно всегда одинаковый! Всегда получается нужное значение 20971520 и программа работает совершенно корректно.

Другие ошибки


Не там поставленная скобка


#define STATUS_INFO_LENGTH_MISMATCH 0xc0000004

static void
enum_handles(int op)
{
  ....
  NTSTATUS status;
  while ((status = NtQuerySystemInformation(
      SystemExtendedHandleInformation,
      hndl_info, hi_size, &req_size)
        == STATUS_INFO_LENGTH_MISMATCH)) {
    hi_size = req_size + 4096;
    hndl_info = (PSYSTEM_HANDLE_INFORMATION_EX)REALLOC(hndl_info,
        hi_size);
  }
  UT_ASSERT(status >= 0);
  ....
}

Предупреждение PVS-Studio: V593 [CWE-783] Consider reviewing the expression of the 'A = B == C' kind. The expression is calculated as following: 'A = (B == C)'. ut.c 641

Внимательно посмотрите вот сюда:

while ((status = NtQuerySystemInformation(....) == STATUS_INFO_LENGTH_MISMATCH))

Программист хотел сохранить в переменной status значение, которое возвращает функцию NtQuerySystemInformation, а затем сравнить его с константой.

Программист скорее всего знал, что приоритет оператора сравнения (==) выше, чем у оператора присваивания (=), и поэтому следует использовать скобки. Но опечатался и поставил их не там, где надо. В результате скобки никак не помогают. Корректный код:

while ((status = NtQuerySystemInformation(....)) == STATUS_INFO_LENGTH_MISMATCH)

Из-за этой ошибки, макрос UT_ASSERT никогда не сработает. Ведь в переменную status всегда заносится результат сравнения, то есть ложь (0) или истина (1). Значит условие ([0..1] >= 0) всегда истинно.

Потенциальная утечка памяти


static enum pocli_ret
pocli_args_obj_root(struct pocli_ctx *ctx, char *in, PMEMoid **oidp)
{
  char *input = strdup(in);
  if (!input)
    return POCLI_ERR_MALLOC;

  if (!oidp)
    return POCLI_ERR_PARS;
  ....
}

Предупреждение PVS-Studio: V773 [CWE-401] The function was exited without releasing the 'input' pointer. A memory leak is possible. pmemobjcli.c 238

Если oidp окажется нулевым указателем, то будет потеряна копия строки, созданная с помощью вызова функции strdup. Лучше всего будет перенести проверку до выделения памяти:

static enum pocli_ret
pocli_args_obj_root(struct pocli_ctx *ctx, char *in, PMEMoid **oidp)
{
  if (!oidp)
    return POCLI_ERR_PARS;

  char *input = strdup(in);
  if (!input)
    return POCLI_ERR_MALLOC;
  ....
}

Или можно явно освобождать память:

static enum pocli_ret
pocli_args_obj_root(struct pocli_ctx *ctx, char *in, PMEMoid **oidp)
{
  char *input = strdup(in);
  if (!input)
    return POCLI_ERR_MALLOC;

  if (!oidp)
  {
    free(input);
    return POCLI_ERR_PARS;
  }
  ....
}

Потенциальное переполнение


typedef long long os_off_t;

void
do_memcpy(...., int dest_off, ....., size_t mapped_len, .....)
{
  ....
  LSEEK(fd, (os_off_t)(dest_off + (int)(mapped_len / 2)), SEEK_SET);
  ....
}

Предупреждение PVS-Studio: V1028 [CWE-190] Possible overflow. Consider casting operands, not the result. memcpy_common.c 62

Явное приведение результата сложения к типу os_off_t не имеет смысла. Во-первых, это не защищает от потенциального переполнения, которое может возникнуть при сложении двух значений типа int. Во-вторых, результат сложения и так бы отлично неявно расширился до типа os_off_t. Явное приведение типа просто избыточно.

Думаю, правильнее будет написать так:

LSEEK(fd, dest_off + (os_off_t)(mapped_len) / 2, SEEK_SET);

Здесь беззнаковое значение типа size_t превращается в знаковое (чтоб не было какого-нибудь предупреждения от компилятора). И заодно точно не возникнет переполнение при сложении.

Неправильная защита от переполнения


static DWORD
get_rel_wait(const struct timespec *abstime)
{
  struct __timeb64 t;
  _ftime64_s(&t);
  time_t now_ms = t.time * 1000 + t.millitm;
  time_t ms = (time_t)(abstime->tv_sec * 1000 +
    abstime->tv_nsec / 1000000);

  DWORD rel_wait = (DWORD)(ms - now_ms);

  return rel_wait < 0 ? 0 : rel_wait;
}

Предупреждение PVS-Studio: V547 [CWE-570] Expression 'rel_wait < 0' is always false. Unsigned type value is never < 0. os_thread_windows.c 359

Мне не очень понятно, от какой именно ситуации должна защищать проверка, но она в любом случае не работает. Переменная rel_wait имеет беззнаковый тип DWORD. А значит, сравнение rel_wait < 0 не имеет смысла, так как результатом всегда является истина.

Отсутствие проверки, что память успешно выделена


Проверка того, что память выделена, осуществляется с помощью макросов assert, которые ничего не делают, если компилируется Release версия приложения. Так что можно сказать, что нет никакой обработки ситуации, когда функции malloc возвращают NULL. Пример:

static void
remove_extra_node(TOID(struct tree_map_node) *node)
{
  ....
  unsigned char *new_key = (unsigned char *)malloc(new_key_size);
  assert(new_key != NULL);
  memcpy(new_key, D_RO(tmp)->key, D_RO(tmp)->key_size);
  ....
}

Предупреждение PVS-Studio: V575 [CWE-628] The potential null pointer is passed into 'memcpy' function. Inspect the first argument. Check lines: 340, 338. rtree_map.c 340

В других местах нет даже assert:

static void
calc_pi_mt(void)
{
  ....
  HANDLE *workers = (HANDLE *) malloc(sizeof(HANDLE) * pending);
  for (i = 0; i < pending; ++i) {
    workers[i] = CreateThread(NULL, 0, calc_pi,
      &tasks[i], 0, NULL);
    if (workers[i] == NULL)
      break;
  }
  ....
}

Предупреждение PVS-Studio: V522 [CWE-690] There might be dereferencing of a potential null pointer 'workers'. Check lines: 126, 124. pi.c 126

Таких фрагментов кода я насчитал минимум 37 штук. Так что я не вижу смысла перечислять все их в статье.

На первый взгляд, отсутствие проверок можно посчитать просто неаккуратностью и сказать, что это код с запахом. Я не согласен с такой позицией. Программисты недооценивают опасность отсутствия таких проверок. Нулевой указатель вовсе не обязательно сразу проявит себя падением программы при попытке его разыменования. Последствия могут быть более причудливыми и опасными, особенно в многопоточных программах. Чтобы подробнее понять, в чём дело и почему проверки нужны, настоятельно рекомендую всем познакомиться со статьёй "Почему важно проверять, что вернула функция malloc".

Код с запахом


Двойной вызов CloseHandle


static void
prepare_map(struct pmem2_map **map_ptr,
  struct pmem2_config *cfg, struct pmem2_source *src)
{
  ....
  HANDLE mh = CreateFileMapping(....);
  ....
  UT_ASSERTne(CloseHandle(mh), 0);
  ....
}

Предупреждение PVS-Studio: V586 [CWE-675] The 'CloseHandle' function is called twice for deallocation of the same resource. pmem2_map.c 76

Смотря на этот код и предупреждение PVS-Studio понятно, что ничего непонятно. Где тут возможен повторный вызов CloseHandle? Чтобы найти ответ, давайте посмотрим на реализацию макроса UT_ASSERTne.

#define UT_ASSERTne(lhs, rhs)\
  do {\
    /* See comment in UT_ASSERT. */\
    if (__builtin_constant_p(lhs) && __builtin_constant_p(rhs))\
      UT_ASSERT_COMPILE_ERROR_ON((lhs) != (rhs));\
    UT_ASSERTne_rt(lhs, rhs);\
  } while (0)

Сильно понятнее не стало. Что такое UT_ASSERT_COMPILE_ERROR_ON? Что такое UT_ASSERTne_rt?

Не стану загромождать статью описанием каждого макроса и мучать читателя, заставляя его у себя в голове вставлять одни макросы в другие. Посмотрим сразу окончательный вариант раскрытого кода, взятого из препроцессированного файла.

do {
  if (0 && 0) (void)((CloseHandle(mh)) != (0));
  ((void)(((CloseHandle(mh)) != (0)) ||
    (ut_fatal(".....", 76, __FUNCTION__, "......: %s (0x%llx) != %s (0x%llx)",
              "CloseHandle(mh)", (unsigned long long)(CloseHandle(mh)), "0",
              (unsigned long long)(0)), 0))); } while (0);

Удалим всегда ложное условие (0 && 0) и, вообще, всё не относящееся к делу. Получается:

((void)(((CloseHandle(mh)) != (0)) ||
  (ut_fatal(...., "assertion failure: %s (0x%llx) != %s (0x%llx)",
            ....., (unsigned long long)(CloseHandle(mh)), .... ), 0)));

Происходит закрытие хендла. Если при этом произошла ошибка, то генерируется отладочное сообщение и, чтобы получить код ошибки вновь, вызывается CloseHandle для этого же самого некорректного хендла.

Ошибки, вроде как, и нет. Раз хендл невалиден, то ничего страшного, что для него дважды вызывается функция CloseHandle. Однако, этот код с запахом. Более идеологически верным было бы вызывать функцию только один раз и сохранять статус, который она вернула, чтобы затем при необходимости вывести его значение в сообщении.

Несоответствие интерфейса реализации (снятие константности)


static int
status_push(PMEMpoolcheck *ppc, struct check_status *st, uint32_t question)
{
  ....
  } else {
    status_msg_info_and_question(st->msg);            // <=
    st->question = question;
    ppc->result = CHECK_RESULT_ASK_QUESTIONS;
    st->answer = PMEMPOOL_CHECK_ANSWER_EMPTY;
    PMDK_TAILQ_INSERT_TAIL(&ppc->data->questions, st, next);
  }
  ....
}

Анализатор выдаёт сообщение: V530 [CWE-252] The return value of function 'status_msg_info_and_question' is required to be utilized. check_util.c 293

Причина в том, что функция status_msg_info_and_question, с точки зрения анализатора, не меняет состояние внешних по отношению к ней объектов, в том числе переданную константную строчку. Т.е. функция просто что-то считает и возвращает результат. А раз так, то странно не использовать результат, который возвращает эта функция. И, хотя анализатор в этот раз не прав, он указывает на код с запахом. Давайте посмотрим, как устроена вызываемая функция status_msg_info_and_question.

static inline int
status_msg_info_and_question(const char *msg)
{
  char *sep = strchr(msg, MSG_SEPARATOR);
  if (sep) {
    *sep = ' ';
    return 0;
  }
  return -1;
}

При вызове функции strchr происходит неявное снятие константности. Дело в том, что в C она объявлена так:

char * strchr ( const char *, int );

Не лучшее решение. Но язык C такой, какой есть :).

Анализатор запутался и не понял, что переданная строка на самом деле изменяется. А раз так, то возвращаемое значения не самое главное и его можно не использовать.

Тем не менее, хоть анализатор и запутался, он указывает на код с запахом. То, что сбивает с толку анализатор, может сбивать с толку и человека, который сопровождает код. Лучше было бы объявить функцию более честно, убрав const:

static inline int
status_msg_info_and_question(char *msg)
{
  char *sep = strchr(msg, MSG_SEPARATOR);
  if (sep) {
    *sep = ' ';
    return 0;
  }
  return -1;
}

Так и намерения сразу понятнее, и анализатор будет молчать.

Переусложненный код


static struct memory_block
heap_coalesce(struct palloc_heap *heap,
  const struct memory_block *blocks[], int n)
{
  struct memory_block ret = MEMORY_BLOCK_NONE;

  const struct memory_block *b = NULL;
  ret.size_idx = 0;
  for (int i = 0; i < n; ++i) {
    if (blocks[i] == NULL)
      continue;
    b = b ? b : blocks[i];
    ret.size_idx += blocks[i] ? blocks[i]->size_idx : 0;
  }
  ....
}

Предупреждение PVS-Studio: V547 [CWE-571] Expression 'blocks[i]' is always true. heap.c 1054

Если blocks[i] == NULL, то сработает оператор continue и цикл начнёт следующую итерацию. Поэтому повторная проверка элемента blocks[i] не имеет смысла и тернарный оператор является лишним. Код можно упростить:

....
for (int i = 0; i < n; ++i) {
  if (blocks[i] == NULL)
    continue;
  b = b ? b : blocks[i];
  ret.size_idx += blocks[i]->size_idx;
}
....

Подозрительное использование нулевого указателя


void win_mmap_fini(void)
{
  ....
  if (mt->BaseAddress != NULL)
    UnmapViewOfFile(mt->BaseAddress);
  size_t release_size =
    (char *)mt->EndAddress - (char *)mt->BaseAddress;
  void *release_addr = (char *)mt->BaseAddress + mt->FileLen;
  mmap_unreserve(release_addr, release_size - mt->FileLen);
  ....
}

Предупреждение PVS-Studio: V1004 [CWE-119] The '(char *) mt->BaseAddress' pointer was used unsafely after it was verified against nullptr. Check lines: 226, 235. win_mmap.c 235

Указатель mt->BaseAddress может быть нулевым, о чём свидетельствует проверка:

if (mt->BaseAddress != NULL)

Однако ниже этот указатель уже используется в арифметических действиях без проверки. Например, здесь:

size_t release_size =
  (char *)mt->EndAddress - (char *)mt->BaseAddress;

Будет получено некое большое целочисленное значение, равное фактически значению указателя mt->EndAddress. Возможно, это и не ошибка, но выглядит всё это очень подозрительно, и мне кажется, код следует перепроверить. Запах заключается в том, что код непонятен и ему явно не хватает поясняющих комментариев.

Короткие имена глобальных переменных


Я считаю, что код пахнет, если содержит глобальные переменные с короткими именами. Легко опечататься и случайно использовать в какой-то функции не локальную, а глобальную переменную. Пример:

static struct critnib *c;

Предупреждения PVS-Studio на такие переменные:

  • V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'ri' variable. map.c 131
  • V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'c' variable. obj_critnib_mt.c 56
  • V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'Id' variable. obj_list.h 68
  • V707 Giving short names to global variables is considered to be bad practice. It is suggested to rename 'Id' variable. obj_list.c 34

Странное


PVS-Studio: Странный код в функции

Наиболее странный код мне встретился в функции do_memmove. Анализатор выдал два срабатывания, которые свидетельствуют или об очень серьезных ошибках, или о том, что я просто не понимаю, что имелось в виду. Поскольку код весьма странен, я решил рассмотреть выданные предупреждения в отдельном разделе статьи. Итак, первое предупреждение выдано здесь.

void
do_memmove(char *dst, char *src, const char *file_name,
    size_t dest_off, size_t src_off, size_t bytes,
    memmove_fn fn, unsigned flags, persist_fn persist)
{
  ....
  /* do the same using regular memmove and verify that buffers match */
  memmove(dstshadow + dest_off, dstshadow + dest_off, bytes / 2);
  verify_contents(file_name, 0, dstshadow, dst, bytes);
  verify_contents(file_name, 1, srcshadow, src, bytes);
  ....
}

Предупреждение PVS-Studio: V549 [CWE-688] The first argument of 'memmove' function is equal to the second argument. memmove_common.c 71

Обратите внимание, что первый и второй аргумент функции совпадают. Таким образом, функция по факту ничего не делает. Какие мне на ум приходят варианты:

  • Хотелось "потрогать" блок памяти. Но произойдёт ли это в реальности? Не удалит ли оптимизирующий компилятор код, который копирует блок памяти сам в себя?
  • Это какой-то юнит-тест на работу функции memmove.
  • Код содержит опечатку.

А вот не менее странный фрагмент в этой же функции:

void
do_memmove(char *dst, char *src, const char *file_name,
    size_t dest_off, size_t src_off, size_t bytes,
    memmove_fn fn, unsigned flags, persist_fn persist)
{
  ....
  /* do the same using regular memmove and verify that buffers match */
  memmove(dstshadow + dest_off, srcshadow + src_off, 0);
  verify_contents(file_name, 2, dstshadow, dst, bytes);
  verify_contents(file_name, 3, srcshadow, src, bytes);
  ....
}

Предупреждение PVS-Studio: V575 [CWE-628] The 'memmove' function processes '0' elements. Inspect the third argument. memmove_common.c 82

Функция перемещает 0 байт. Что это? Юнит-тест? Опечатка?

Для меня этот код непонятен и странен.

Зачем использовать анализаторы кода?


Может показаться, что раз найдено мало ошибок, то и внедрение анализатора в процесс разработки кода малообоснованно. Но смысл использования инструментов статического анализа не в разовых проверках, а в регулярном выявлении ошибок ещё на этапе написания кода. В противном случае, эти ошибки выявляются более дорогими и медленными способами (отладка, тестирование, отзывы пользователей и так далее). Более подробно эта мысль изложена в статье "Ошибки, которые не находит статический анализ кода, потому что он не используется", с которой я рекомендую познакомиться. А затем приходите к нам на сайт скачать и попробовать PVS-Studio для проверки своих проектов.

Спасибо за внимание!


Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Static code analysis of the PMDK library collection by Intel and errors that are not actual errors.

Комментарии (4)