Нам предложили проверить с помощью анализатора 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
Странное
Наиболее странный код мне встретился в функции 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.