Нам предложили проверить с помощью анализатора 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.
