Нас попросили проверить коллекцию библиотек PMDK с открытым исходным кодом для разработки и отладки приложений с поддержкой NVRAM от PVS-Studio. А почему бы не? Тем более, что это небольшой проект на C и C++ с общим размером кодовой базы около 170 KLOC без комментариев. А значит, просмотр результатов не займет много сил и времени. Пойдем.
Для анализа исходного кода будет использоваться инструмент PVS-Studio 7.08. Конечно, читатели нашего блога давно знакомы с нашим инструментом, поэтому я не буду на нем заострять внимание. Для тех, кто посетил нас впервые, предлагаю обратиться к статье «Как быстро проверить интересные предупреждения, которые выдает анализатор PVS-Studio для кода C и C++?» и попробовать бесплатную пробную версию анализатора.
В этот раз я загляну внутрь проекта PMDK и расскажу о замеченных мною ошибках и недостатках. Внутреннее чувство подсказывало, что их немного, что говорит о высоком качестве кода проекта. Что касается некоторых странностей, то я нашел несколько фрагментов некорректного кода, который тем не менее работал корректно :). Что я имею в виду, станет яснее из остальной части истории.
Итак, PMDK — это набор библиотек и инструментов с открытым исходным кодом, предназначенных для упрощения разработки, отладки и управления приложениями, поддерживающими NVRAM. Подробнее читайте здесь: Введение в PMDK. Исходный код доступен здесь: pmdk.
Посмотрим, какие ошибки и недостатки я смогу в нем найти. Сразу скажу, что я не всегда был внимателен при анализе отчета и мог многое упустить. Поэтому призываю авторов проекта не руководствоваться этой статьей при исправлении дефектов, а самим перепроверять код. Как по мне, для написания статьи достаточно будет привести то, что я отметил при просмотре списка предупреждений :).
Неправильный код, который работает
Размер выделенной памяти
Программисты часто тратят время на отладку кода, когда программа ведет себя не так, как должна. Однако иногда бывают случаи, когда программа работает корректно, но код содержит ошибку. Программисту просто повезло, и ошибка никак себя не проявляет. В проекте PMDK я наткнулся на несколько таких интересных кейсов, поэтому решил собрать их вместе в отдельном разделе.
int main(int argc, char *argv[])
{
....
struct pool *pop = malloc(sizeof(pop));
....
}Предупреждение PVS-Studio: V568 Странно, что оператор sizeof() оценивает размер указателя на класс, а не размер объекта класса pop. util_ctl.c 717
Классическая опечатка, из-за которой выделяется не тот объем памяти. Оператор sizeof вернет размер указателя на структуру вместо размера этой структуры. Правильная версия:
struct pool *pop = malloc(sizeof(pool));or
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] Функция memcpy_fn получает в качестве аргументов указатель и его размер. Возможно, это ошибка. Проверьте третий аргумент. pmem2_map_prot.c 513
Для копирования строки используется указатель на специальную функцию копирования. Обратите внимание на вызов этой функции, а точнее на ее третий аргумент.
Программист предполагает, что оператор sizeof будет вычислять размер строкового литерала. Но, на самом деле, заново вычисляется именно размер указателя.
К счастью, строка состоит из 8 символов, а ее размер соответствует размеру указателя, если строится 64-битное приложение. В результате все 8 символов строки «Без кода». будет успешно скопировано.
На самом деле ситуация еще более сложная и интригующая. Интерпретация этой ошибки зависит от того, хотел ли автор скопировать терминальный ноль или нет. Рассмотрим два сценария.
Сценарий 1. Нужно было скопировать терминальный нуль. Таким образом, я не прав, и это не просто безобидный баг, который никак себя не проявляет. Было скопировано только 8 байт, а не 9 байт. Терминального нуля нет, и последствия нельзя предсказать. В этом случае можно исправить код, изменив определение константной строки 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] Неопределенное поведение. Проверьте оператора смены «››». Левый операнд ‘~0’ отрицательный. кло.cpp 205
Результат смещения отрицательного значения вправо зависит от реализации компилятора. Поэтому, хотя этот код может работать корректно и ожидаемо во всех существующих на данный момент режимах компиляции приложений, это все же удача.
Приоритет операции
И давайте рассмотрим последний случай, связанный с приоритетом операции.
#define BTT_CREATE_DEF_SIZE (20 * 1UL << 20) /* 20 MB */Предупреждение PVS-Studio: V634 [CWE-783] Приоритет операции «*» выше приоритета операции «‹‹». Возможно, в выражении следует использовать скобки. bttcreate.c 204
Чтобы получить константу, равную 20 МБ, программист решил выполнить следующие шаги:
- Сдвинул 1 на 20 бит, чтобы получить значение 1048576, т.е. 1 МБ.
- Умножить 1 МБ на 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 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] Рассмотрим выражение вида «A = B == C». Выражение рассчитывается следующим образом: «А = (В == С)». ут.к 641
Посмотрите внимательно сюда:
while ((status = NtQuerySystemInformation(....) == STATUS_INFO_LENGTH_MISMATCH))Программист хотел сохранить значение, возвращаемое функцией NtQuerySystemInformation, в переменной status, а затем сравнить его с константой.
Программист, вероятно, знал, что приоритет оператора сравнения (==) выше, чем у оператора присваивания (=), и поэтому следует использовать круглые скобки. Но, наверное, ошибся и поставил их не туда. В результате скобки никак не помогают. Правильный код:
while ((status = NtQuerySystemInformation(....)) == STATUS_INFO_LENGTH_MISMATCH)Из-за этой ошибки макрос UT_ASSERT никогда не будет работать. Ведь переменная status всегда содержит результат сравнения, т.е. false (0) или true (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] Выход из функции без отпускания указателя input. Возможна утечка памяти. 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] Возможно переполнение. Рассмотрим приведение операндов, а не результата. 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] Выражение ‘rel_wait ‹ 0’ всегда ложно. Значение беззнакового типа никогда не равно 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] В функцию memcpy передается потенциальный нулевой указатель. Проверьте первый аргумент. Проверить строки: 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] Возможно, происходит разыменование потенциальных нулевых указателей «рабочих». Контрольные строки: 126, 124. рис. в 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] Функция CloseHandle вызывается дважды для освобождения одного и того же ресурса. 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] Необходимо использовать возвращаемое значение функции ‘status_msg_info_and_question’. 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 );Не лучшее решение. Но язык Си такой, какой он есть :).
Анализатор запутался и не понял, что переданная строка действительно изменяется. Если это так, то возвращаемое значение не является самым важным, и вам не нужно его использовать.
Однако, хотя анализатор и запутался, он указывает на запах кода. То, что сбивает с толку анализатора, может сбить с толку и того, кто поддерживает код. Было бы лучше объявить функцию более честно, удалив 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] Выражение «blocks[i]» всегда истинно. куча.с 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] Указатель ‘(char *) mt-›BaseAddress’ использовался небезопасно после его проверки на nullptr. Проверить строки: 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 Давать короткие имена глобальным переменным считается плохой практикой. Предлагается переименовать переменную «ri». карта.с 131
- V707 Давать короткие имена глобальным переменным считается плохой практикой. Предлагается переименовать переменную ‘c’. obj_critnib_mt.c 56
- V707 Давать короткие имена глобальным переменным считается плохой практикой. Предлагается переименовать переменную «Id». obj_list.h 68
- V707 Давать короткие имена глобальным переменным считается плохой практикой. Предлагается переименовать переменную «Id». 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] Первый аргумент функции memmove равен второму аргументу. 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] Функция memmove обрабатывает нулевые элементы. Проверьте третий аргумент. 82
Функция передает 0 байт. Что это — ошибка или просто дополнительная проверка? Модульный тест? Опечатка?
Для меня этот код непонятен и странен.
Зачем использовать анализаторы кода?
Может показаться, что раз найдено мало ошибок, то введение анализатора в процесс разработки кода не оправдано. Но смысл использования инструментов статического анализа не в разовых проверках, а в регулярном выявлении ошибок на этапе написания кода. В противном случае эти ошибки обнаруживаются более дорогими и медленными способами (отладка, тестирование, отзывы пользователей и т. д.). Более подробно эта идея описана в статье Ошибки, которые не находит статический анализ кода, потому что он не используется», с которой рекомендую ознакомиться. И не стесняйтесь заходить на наш сайт, чтобы скачать и попробовать PVS-Studio для сканирования ваших проектов.
Спасибо за внимание!
