Devtool

Нам предложили проверить с помощью анализатора 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 inttest_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 intclo_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 0xc0000004static voidenum_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_retpocli_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_retpocli_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_retpocli_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;voiddo_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 DWORDget_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 voidremove_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 voidcalc_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 voidprepare_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 intstatus_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 intstatus_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 intstatus_msg_info_and_question(char *msg){  char *sep = strchr(msg, MSG_SEPARATOR);  if (sep) {    *sep = ' ';    return 0;  }  return -1;}

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

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

static struct memory_blockheap_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

Странное

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

voiddo_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.
• Код содержит опечатку.

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

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