Релиз PVS-Studio для macOS: 64 weaknesses в XNU Kernel
Новая версия PVS-Studio 6.23 работает под управлением macOS и позволяет проверять проекты, написанные на языке C и C++. К этому событию наша команда решила приурочить проверку XNU Kernel.
PVS-Studio для macOS
С выходом версии анализатора для macOS, PVS-Studio можно смело называть кроссплатформенным статическим анализатором кода для C и C++.
Изначально существовала версия только для Windows. Около двух лет назад наша команда поддержала Linux: «Как создавался PVS-Studio под Linux». Также внимательные читали нашего блога должны вспомнить статьи о проверке FreeBSD Kernel (1-я статья, 2-я статья). Тогда анализатор был собран для запуска в PC-BSD и TrueOS. И вот, наконец, мы добрались до macOS!
Легко ли развивать кроссплатформенный продукт?
У этого вопроса есть экономическая и техническая составляющая.
С экономической точки зрения, сделать анализатор кроссплатформенным было правильным решением. Разработка программного обеспечения давно движется в этом направлении, и инструмент для разработчиков таких проектов должен быть соответствующим. Однако, если что-то является полезным — это не значит, что это сразу стоит начинать делать. Вначале мы всегда убеждаемся, что нам хватит сил реализовать что-то в новом направлении, а потом поддерживать это.
С технической стороны сложно только в самом начале, если проект не сразу задумывается как кроссплатформенный. Мы потратили несколько месяцев на адаптацию анализатора в системе Linux. Компиляция проекта под новую платформу не заняла много времени: у нас нет графического интерфейса и код практически не завязан на использование системных API. Большую часть времени заняла адаптация анализатора под новые компиляторы и повышение качества анализа. Другими словами, много усилий требует предотвращение ложных срабатываний.
А что с разработкой под macOS?
К этому моменту мы уже имели проектный файл анализатора для CMake, легко адаптируемый под разные операционные системы. Системы тестирования разных типов тоже были кроссплатформенными. Всё это помогло очень быстро запуститься на macOS.
Особенностью разработки анализатора под macOS стал компилятор Apple LLVM Compiler. Хоть анализатор собирался с помощью GCC и отлично работал, это все же могло повлиять в дальнейшем на совместимость анализатора с компьютерами пользователей. Чтобы не создавать проблем потенциальным пользователям, мы решили поддержать сборку дистрибутива с помощью этого компилятора, который поставляется вместе с Xcode.
Развитие C++ сильно помогает в создании и развитии кроссплатформенных проектов, но разные компиляторы добавляют такие возможности очень неравномерно, поэтому условная компиляция всё ещё активно используется в нескольких местах.
В целом, всё прошло легко и гладко. Как и раньше, большая часть времени ушла на доработку исключений, модификацию сайта, тестирование и другие сопутствующие вопросы. В качестве первого проекта, проверенного с помощью PVS-Studio для macOS, представляем вам XNU Kernel.
Дистрибутив
Со способами загрузки и установки PVS-Studio для macOS вы можете ознакомиться здесь.
XNU Kernel
С чего начать демонстрацию возможностей PVS-Studio для macOS, как не с проверки ядра этой системы! Поэтому первым проектом, проверенным с помощью новой версии анализатора, стал XNU Kernel.
XNU — ядро компьютерных операционных систем, разрабатываемое компанией Apple и используемое в ОС семейства OS X (macOS, iOS, tvOS, watchOS). Подробнее.
Считается, что ядро написано на языке C и C++, но, фактически, это C. Я насчитал 1302 *.c-файла и только 97 *.cpp-файлов. Размер кодовой базы составляет 1929 KLOC. Получается, что это относительно небольшой проект. Для сравнения, кодовая база проекта Chromium в 15 раз больше и содержит 30 MLOC.
Исходный код можно удобно скачать с зеркала на сайте GitHub: xnu.
Результаты проверки
Хотя проект XNU Kernel сравнительно небольшой, изучать в одиночку предупреждения анализатора — большая задача, требующая много времени. Усложняют изучение предупреждений и ложные срабатывания, так как я не проводил предварительную настройку анализатора. Я просто быстро пробежался по предупреждениям, выписывая фрагменты кода, представляющие, на мой взгляд, интерес. Этого более чем достаточно для написания статьи, причем, солидной по размеру. Анализатор PVS-Studio легко находит большое количество интересных ошибок.
Примечание для разработчиков XNU Kernel. У меня не было задачи найти как можно больше ошибок. Поэтому не следует руководствоваться этой статьей для их исправления. Во-первых, это неудобно, так как нет возможности осуществлять навигацию по предупреждениям. Уверен, намного лучше использовать один из форматов, который умеет генерировать PVS-Studio, например, HTML-отчёт с возможностью навигации (он похож на то, что умеет генерировать Clang). Во-вторых, я пропустил многие ошибки просто в силу того, что изучал отчёт поверхностно. Рекомендую разработчикам выполнить самостоятельно более тщательный анализ проекта с помощью PVS-Studio.
Как я уже сказал, мне мешали ложные срабатывания, но на самом деле они не являются проблемой. Если настроить анализатор, то вполне можно сократить количество ложных срабатываний до 10–15%. Поскольку его настройка тоже требует времени и перезапуска процесса анализа, я пропустил этот шаг — мне и без этого несложно насобирать ошибки для статьи. Если же вы планируете выполнять анализ тщательно, то, конечно, следует уделить время настройке.
В основном ложные срабатывания возникают из-за макросов и недостаточно качественно размеченных функций. Например, в XNU Kernel большая их часть связана с использованием функции panic.
Вот как объявлена эта функция:
extern void panic(const char *string, ...)
__attribute__((__format__ (__printf__, 1, 2)));
Функция проаннотирована так, что её входные аргументы интерпретируются по аналогии с аргументами функции printf. Это позволяет компиляторам и анализаторам находить ошибки неправильного форматирования строк. Однако, функция не помечена как не возвращающая управление. В результате, следующий код приводит к возникновению ложных срабатываний:
if (!ptr)
panic("zzzzzz");
memcpy(ptr, src, n);
Здесь анализатор выдаёт предупреждение, что возможно разыменовывание нулевого указателя. С его точки зрения, после вызова функции panic будет вызвана и функция memcpy.
Чтобы избежать подобных ложных срабатываний, следует изменить аннотирование функции, добавив __attribute__((noreturn)):
extern __attribute__((noreturn)) void panic(const char *string, ...)
__attribute__((__format__ (__printf__, 1, 2)));
Давайте теперь посмотрим, что интересного мне удалось заметить в коде XNU Kernel. Всего я выписал 64 ошибки и решил остановиться на этом красивом числе. Дефекты я сгруппировал согласно Common Weakness Enumeration, так как многим знакома эта классификация, и им будет легче понять, о каких ошибках идёт речь в той или иной главе.
CWE-570/CWE-571: Expression is Always False/True
Очень разнообразные ошибки могут приводить к CWE-570/CWE-571, т.е. к ситуациям, когда условие или часть условия всегда ложно/истинно. В случае с проектом XNU Kernel, все эти ошибки, на мой взгляд, связаны с опечатками. PVS-Studio вообще очень хорошо выявляет опечатки.
Фрагмент N1
int
key_parse(
struct mbuf *m,
struct socket *so)
{
....
if ((m->m_flags & M_PKTHDR) == 0 ||
m->m_pkthdr.len != m->m_pkthdr.len) {
ipseclog((LOG_DEBUG,
"key_parse: invalid message length.\n"));
PFKEY_STAT_INCREMENT(pfkeystat.out_invlen);
error = EINVAL;
goto senderror;
}
....
}
Предупреждение PVS-Studio: V501 CWE-570 There are identical sub-expressions 'm→M_dat.MH.MH_pkthdr.len' to the left and to the right of the '!=' operator. key.c 9442
Из-за опечатки член класса сравнивается сам с собой:
m->m_pkthdr.len != m->m_pkthdr.len
Часть условия всегда ложна и, как следствие, длина какого-то сообщения проверяется неправильно. Получается, что программа продолжит обрабатывать некорректные данные. Возможно, это не страшно, но многие уязвимости как раз связаны с тем, что некие входные данные оказались непроверенными или были проверены недостаточным образом. Так что данное место в коде однозначно заслуживает внимания разработчиков.
Фрагмент N2, N3
#define VM_PURGABLE_STATE_MASK 3
kern_return_t
memory_entry_purgeable_control_internal(...., int *state)
{
....
if ((control == VM_PURGABLE_SET_STATE ||
control == VM_PURGABLE_SET_STATE_FROM_KERNEL) &&
(((*state & ~(VM_PURGABLE_ALL_MASKS)) != 0) ||
((*state & VM_PURGABLE_STATE_MASK) >
VM_PURGABLE_STATE_MASK)))
return(KERN_INVALID_ARGUMENT);
....
}
Предупреждение PVS-Studio: V560 CWE-570 A part of conditional expression is always false: ((* state & 3) > 3). vm_user.c 3415
Рассмотрим более подробно вот эту часть выражения:
(*state & VM_PURGABLE_STATE_MASK) > VM_PURGABLE_STATE_MASK
Если подставить значение макроса, то получится:
(*state & 3) > 3
Побитовая операция AND может дать в результате только значения 0, 1 или 2. Бессмысленно проверять, будет ли 0, 1 или 2 больше чем 3. Весьма вероятно, что выражение содержит какую-то опечатку.
Как и в предыдущем случае, некий статус проверен некорректно, что может стать причиной обработки некорректных (недостоверных) данных.
Точно такая же ошибка обнаруживается в файле vm_map.c. Видимо, часть кода была написана с помощью Copy-Paste. Предупреждение: V560 CWE-570 A part of conditional expression is always false: ((* state & 3) > 3). vm_map.c 15809
Фрагмент N4
void
pat_init(void)
{
boolean_t istate;
uint64_t pat;
if (!(cpuid_features() & CPUID_FEATURE_PAT))
return;
istate = ml_set_interrupts_enabled(FALSE);
pat = rdmsr64(MSR_IA32_CR_PAT);
DBG("CPU%d PAT: was 0x%016llx\n", get_cpu_number(), pat);
/* Change PA6 attribute field to WC if required */
if ((pat & ~(0x0FULL << 48)) != (0x01ULL << 48)) {
mtrr_update_action(CACHE_CONTROL_PAT);
}
ml_set_interrupts_enabled(istate);
}
Предупреждение PVS-Studio: V547 CWE-571 Expression is always true. mtrr.c 692
Давайте разберём бессмысленную проверку, в которую, скорее всего, вкралась опечатка:
(pat & ~(0x0FULL << 48)) != (0x01ULL << 48)
Вычислим некоторые выражения:
- ~(0×0FULL << 48) = 0xFFF0FFFFFFFFFFFF
- (0×01ULL << 48) = 0x0001000000000000
Выражение (pat & [0xFFF0FFFFFFFFFFFF]) не может дать в результате значение 0×0001000000000000. Условие всегда истинно. Как следствие, функция mtrr_update_action вызывается всегда.
Фрагмент N5
Сейчас, на мой взгляд, будет очень красивая опечатка: вместо == написали =.
typedef enum {
CMODE_WK = 0,
CMODE_LZ4 = 1,
CMODE_HYB = 2,
VM_COMPRESSOR_DEFAULT_CODEC = 3,
CMODE_INVALID = 4
} vm_compressor_mode_t;
void vm_compressor_algorithm_init(void) {
....
assertf(((new_codec == VM_COMPRESSOR_DEFAULT_CODEC) ||
(new_codec == CMODE_WK) ||
(new_codec == CMODE_LZ4) || (new_codec = CMODE_HYB)),
"Invalid VM compression codec: %u", new_codec);
....
}
Предупреждение PVS-Studio: V768 CWE-571 The expression 'new_codec = CMODE_HYB' is of enum type. It is odd that it is used as an expression of a Boolean-type. vm_compressor_algorithms.c 419
В процессе проверки условия переменной new_codec присваивается значение 2. В итоге условие всегда истинно и assert-макрос на самом деле ничего не проверяет.
Ошибка могла бы быть безобидной. Ну, подумаешь, assert-макрос что-то не проверил — не беда. Однако, помимо этого, ещё и отладочная версия работает неправильно. Ведь портится значение переменной new_codec и используется не тот кодек, который требовался.
Фрагмент N6, N7
void
pbuf_copy_back(pbuf_t *pbuf, int off, int len, void *src)
{
VERIFY(off >= 0);
VERIFY(len >= 0);
VERIFY((u_int)(off + len) <= pbuf->pb_packet_len);
if (pbuf->pb_type == PBUF_TYPE_MBUF)
m_copyback(pbuf->pb_mbuf, off, len, src);
else
if (pbuf->pb_type == PBUF_TYPE_MBUF) {
if (len)
memcpy(&((uint8_t *)pbuf->pb_data)[off], src, len);
} else
panic("%s: bad pb_type: %d", __func__, pbuf->pb_type);
}
Предупреждение PVS-Studio: V517 CWE-570 The use of 'if (A) {…} else if (A) {…}' pattern was detected. There is a probability of logical error presence. Check lines: 340, 343. pf_pbuf.c 340
Для наглядности выделю суть:
if (A)
foo();
else
if (A)
Unreachable_code;
else
panic();
Если условие A истинно, то выполняется тело первого оператора if. Если это не так, то повторная проверка не имеет смысла и сразу происходит вызов функции panic. Часть кода получается вообще недостижима.
Здесь или какая-то ошибка в логике, или опечатка в одном из условий.
Ниже в этом же файле расположена функция pbuf_copy_data, которая, по всей видимости, была написана с помощью Copy-Paste и содержит ту же самую ошибку. Предупреждение: V517 CWE-570 The use of 'if (A) {…} else if (A) {…}' pattern was detected. There is a probability of logical error presence. Check lines: 358, 361. pf_pbuf.c 358
CWE-670: Always-Incorrect Control Flow Implementation
Дефект CWE-670 говорит о том, что в коде, скорее всего, что-то работает не так, как это задумывал программист.
Фрагмент N8, N9, N10
static void
in_ifaddr_free(struct ifaddr *ifa)
{
IFA_LOCK_ASSERT_HELD(ifa);
if (ifa->ifa_refcnt != 0) {
panic("%s: ifa %p bad ref cnt", __func__, ifa);
/* NOTREACHED */
} if (!(ifa->ifa_debug & IFD_ALLOC)) {
panic("%s: ifa %p cannot be freed", __func__, ifa);
/* NOTREACHED */
}
if (ifa->ifa_debug & IFD_DEBUG) {
....
}
Предупреждение PVS-Studio: V646 CWE-670 Consider inspecting the application’s logic. It’s possible that 'else' keyword is missing. in.c 2010
Возможно, в этом коде и нет никакой ошибки. Однако, очень подозрительно выглядит вот это место:
} if (!(ifa->ifa_debug & IFD_ALLOC)) {
Аномалия состоит в том, что так писать не принято. Логичнее было бы начать писать if с новой строки. Авторам кода следует проверить это место. Возможно, здесь пропущено ключевое слово else и код должен быть таким:
} else if (!(ifa->ifa_debug & IFD_ALLOC)) {
Или нужно просто добавить перенос строки, чтобы этот код не смущал ни анализатор, ни коллег, сопровождающих этот код.
Аналогичные подозрительные места можно найти здесь:
- V646 CWE-670 Consider inspecting the application’s logic. It’s possible that 'else' keyword is missing. kern_malloc.c 836
- V646 CWE-670 Consider inspecting the application’s logic. It’s possible that 'else' keyword is missing. ipc_kmsg.c 4229
Фрагмент N11, N12, N13, N14
int
dup2(proc_t p, struct dup2_args *uap, int32_t *retval)
{
....
while ((fdp->fd_ofileflags[new] & UF_RESERVED) == UF_RESERVED)
{
fp_drop(p, old, fp, 1);
procfdtbl_waitfd(p, new);
#if DIAGNOSTIC
proc_fdlock_assert(p, LCK_MTX_ASSERT_OWNED);
#endif
goto startover;
}
....
startover:
....
}
Предупреждение PVS-Studio: V612 CWE-670 An unconditional 'goto' within a loop. kern_descrip.c 628
Это очень странный код. Обратите внимание, что тело оператора while заканчивается оператором goto. При этом в теле цикла нигде не используется оператор continue. Это значит, что тело цикла будет выполняться не больше, чем один раз.
Зачем надо было создавать цикл, если всё равно он не выполнит больше одной итерации? Уж лучше было бы использовать оператор if, тогда это не вызывало бы вопросов. Думаю, что это ошибка, и в цикле что-то написано неправильно. Например, возможно, перед оператором goto не хватает какого-то условия.
Аналогичные «одноразовые» циклы встречаются ещё 3 раза:
- V612 CWE-670 An unconditional 'goto' within a loop. tty.c 1084
- V612 CWE-670 An unconditional 'goto' within a loop. vm_purgeable.c 842
- V612 CWE-670 An unconditional 'return' within a loop. kern_credential.c 930
Разыменовывание нулевых указателей: CWE-476, CWE-628, CWE-690
Существуют различные причины, из-за которых может произойти разыменовывание нулевого указателя и анализатор PVS-Studio, в зависимости от ситуации, может назначать им различные CWE-ID:
- CWE-476: NULL Pointer Dereference
- CWE-628: Function Call with Incorrectly Specified Arguments
- CWE-690: Unchecked Return Value to NULL Pointer Dereference
При написании статьи я посчитал разумным собрать все ошибки этого типа в один раздел.
Фрагмент N15
Начну со сложных и больших функций. Вначале рассмотрим функцию netagent_send_error_response, в которой разыменовывается указатель, переданный в аргументе session.
static int
netagent_send_error_response(
struct netagent_session *session, u_int8_t message_type,
u_int32_t message_id, u_int32_t error_code)
{
int error = 0;
u_int8_t *response = NULL;
size_t response_size = sizeof(struct netagent_message_header);
MALLOC(response, u_int8_t *, response_size,
M_NETAGENT, M_WAITOK);
if (response == NULL) {
return (ENOMEM);
}
(void)netagent_buffer_write_message_header(.....);
if ((error = netagent_send_ctl_data(session->control_unit,
(u_int8_t *)response, response_size))) {
NETAGENTLOG0(LOG_ERR, "Failed to send response");
}
FREE(response, M_NETAGENT);
return (error);
}
Обратите внимание, что указатель session разыменовывается в выражении session→control_unit без какой-либо предварительной проверки. Произойдёт разыменовывание нулевого указателя или нет зависит от того, какие фактические аргументы будут переданы в эту функцию.
Теперь давайте посмотрим, как используется рассмотренная выше функция netagent_send_error_response в функции netagent_send_error_response:
static void
netagent_handle_unregister_message(
struct netagent_session *session, ....)
#pragma unused(payload_length, packet, offset)
u_int32_t response_error = NETAGENT_MESSAGE_ERROR_INTERNAL;
if (session == NULL) {
NETAGENTLOG0(LOG_ERR, "Failed to find session");
response_error = NETAGENT_MESSAGE_ERROR_INTERNAL;
goto fail;
}
netagent_unregister_session_wrapper(session);
netagent_send_success_response(session, .....);
return;
fail:
netagent_send_error_response(
session, NETAGENT_MESSAGE_TYPE_UNREGISTER, message_id,
response_error);
}
Предупреждение PVS-Studio: V522 CWE-628 Dereferencing of the null pointer 'session' might take place. The null pointer is passed into 'netagent_send_error_response' function. Inspect the first argument. Check lines: 427, 972. network_agent.c 427
Здесь проявляет себя Data Flow анализ, реализованный в PVS-Studio. Анализатор подмечает, что если указатель session был равен NULL, то какая-то информация пишется в лог, после чего происходит переход на метку fail.
Далее последует вызов функции netagent_send_error_response:
fail:
netagent_send_error_response(
session, NETAGENT_MESSAGE_TYPE_UNREGISTER, message_id,
response_error);
Обратите внимание, что в функцию в качестве фактического аргумента передаётся злосчастный указатель session, который равен NULL.
Как мы знаем, в функции netagent_send_error_response нет никакой защиты на этот случай, и произойдёт разыменовывание нулевого указателя.
Фрагмент N16
Следующая ситуация аналогична предыдущей. Код функции будет короче, но вот разбираться с ним придётся так же медленно и подробно.
void *
pf_lazy_makewritable(struct pf_pdesc *pd, pbuf_t *pbuf, int len)
{
void *p;
if (pd->lmw < 0)
return (NULL);
VERIFY(pbuf == pd->mp);
p = pbuf->pb_data;
if (len > pd->lmw) {
....
}
Заметьте, что указатель pbuf разыменовывается без предварительной проверки на равенство NULL. В коде есть проверка «VERIFY (pbuf == pd→mp)». Однако pd→mp может оказаться равна NULL, поэтому проверку нельзя рассматривать как защиту от NULL.
Примечание. Прошу помнить, что я совершенно не знаком с кодом XNU Kernel и могу ошибаться. Возможно pd→mp никогда не будет хранить в себе значение NULL. Тогда все мои рассуждения неверны и никакой ошибки здесь нет. Тем не менее, такой код всё равно лучше лишний раз проверить.
Продолжим и посмотрим, как используется рассмотренная функция pf_lazy_makewritable.
static int
pf_test_state_icmp(....)
{
....
if (pf_lazy_makewritable(pd, NULL,
off + sizeof (struct icmp6_hdr)) ==
NULL)
return (PF_DROP);
....
}
Предупреждение PVS-Studio: V522 CWE-628 Dereferencing of the null pointer 'pbuf' might take place. The null pointer is passed into 'pf_lazy_makewritable' function. Inspect the second argument. Check lines: 349, 7460. pf.c 349
В качестве второго фактического аргумента в функцию pf_lazy_makewritable передаётся NULL. Это очень странно.
Предположим, программист думает, что от нулевого указателя программу защитит «VERIFY (pbuf == pd→mp)». Тогда возникает вопрос: зачем вообще было писать подобный код? Зачем вызывать функцию, передавая явно некорректный аргумент?
Поэтому мне кажется, что на самом деле функция pf_lazy_makewritable должна уметь принимать нулевой указатель и как-то по-особенному обрабатывать этот случай, но не делает этого. Этот код заслуживает самой тщательной проверки со стороны программиста, и анализатор PVS-Studio однозначно прав, обращая на него наше внимание.
Фрагмент N17
Можете немного отдохнуть: рассмотрим простой случай.
typedef struct vnode * vnode_t;
int
cache_lookup_path(...., vnode_t dp, ....)
{
....
if (dp && (dp->v_flag & VISHARDLINK)) {
break;
}
if ((dp->v_flag & VROOT) ||
dp == ndp->ni_rootdir ||
dp->v_parent == NULLVP)
break;
....
}
Предупреждение PVS-Studio: V522 CWE-690 There might be dereferencing of a potential null pointer 'dp'. vfs_cache.c 1449
Взглянем на проверку:
if (dp && (dp->v_flag & VISHARDLINK))
Она подсказывает нам, что указатель dp может быть нулевым. Однако далее указатель разыменовывается уже без предварительной проверки:
if ((dp->v_flag & VROOT) || ....)
Фрагмент N18
В предыдущем примере мы встретили ситуацию, когда указатель проверяется перед разыменовыванием, а далее в коде проверку забыли. Но намного чаще можно встретить ситуацию, когда вначале указатель разыменовывается, и только потом проверяется. Не стал исключением и код проекта XNU Kernel. Чтобы лучше понять, о чем речь, рассмотрим сначала синтетический пример:
p[n] = 1;
if (!p) return false;
Давайте теперь взглянем, как выглядят такие ошибки в реальности. Начнём с функции сравнения имён. Функции сравнения очень коварны :).
bool
IORegistryEntry::compareName(....) const
{
const OSSymbol * sym = copyName();
bool isEqual;
isEqual = sym->isEqualTo( name ); // <=
if( isEqual && matched) {
name->retain();
*matched = name;
}
if( sym) // <=
sym->release();
return( isEqual );
}
Предупреждения PVS-Studio: V595 CWE-476 The 'sym' pointer was utilized before it was verified against nullptr. Check lines: 889, 896. IORegistryEntry.cpp 889
Интересующие нас строки кода я выделил комментариями вида »// <=". Как видите, вначале указатель разыменовывается. Далее в коде есть проверка на равенство указателя nullptr. Но сразу понятно, что если указатель будет нулевым, то произойдёт разыменовывание нулевого указателя и функция, на самом деле, не готова к такой ситуации.
Фрагмент N19
Следующая ошибка возникла из-за опечатки.
static int
memorystatus_get_priority_list(
memorystatus_priority_entry_t **list_ptr, size_t *buffer_size,
size_t *list_size, boolean_t size_only)
{
....
*list_ptr = (memorystatus_priority_entry_t*)kalloc(*list_size);
if (!list_ptr) {
return ENOMEM;
}
....
}
Предупреждение PVS-Studio: V595 CWE-476 The 'list_ptr' pointer was utilized before it was verified against nullptr. Check lines: 7175, 7176. kern_memorystatus.c 7175
Анализатор видит, что сначала переменная разыменовывается, а в следующей строке проверяется на равенство nullptr. Эта интересная ошибка возникла из-за того, что программист забыл написать символ '*'. На самом деле код должен был быть таким:
*list_ptr = (memorystatus_priority_entry_t*)kalloc(*list_size);
if (!*list_ptr) {
return ENOMEM;
}
Можно сказать, что ошибка выявлена косвенным образом. Однако это не имеет значения, ведь самое главное, что анализатор обратил внимание на аномальный код и мы увидели ошибку.
Фрагмент N20 — N35
В коде XNU Kernel обнаруживается немало ошибок, выявленных благодаря диагностике V595. Впрочем, рассматривать все их будет скучно. Поэтому я разберу только ещё один случай, а далее приведу список сообщений, указывающих на ошибки.
inline void
inp_decr_sndbytes_unsent(struct socket *so, int32_t len)
{
struct inpcb *inp = (struct inpcb *)so->so_pcb;
struct ifnet *ifp = inp->inp_last_outifp;
if (so == NULL || !(so->so_snd.sb_flags & SB_SNDBYTE_CNT))
return;
if (ifp != NULL) {
if (ifp->if_sndbyte_unsent >= len)
OSAddAtomic64(-len, &ifp->if_sndbyte_unsent);
else
ifp->if_sndbyte_unsent = 0;
}
}
Предупреждение PVS-Studio: V595 CWE-476 The 'so' pointer was utilized before it was verified against nullptr. Check lines: 3450, 3453. in_pcb.c 3450
Предлагаю читателю самостоятельно проследить судьбу указателя so и убедиться, что код написан некорректно.
Другие ошибки:
- V595 CWE-476 The 'startDict' pointer was utilized before it was verified against nullptr. Check lines: 3369, 3373. IOService.cpp 3369
- V595 CWE-476 The 'job' pointer was utilized before it was verified against nullptr. Check lines: 4083, 4085. IOService.cpp 4083
- V595 CWE-476 The 'typeinst' pointer was utilized before it was verified against nullptr. Check lines: 176, 177. OSMetaClass.cpp 176
- V595 CWE-476 The 'name' pointer was utilized before it was verified against nullptr. Check lines: 385, 392. devfs_tree.c 385
- V595 CWE-476 The 'collection' pointer was utilized before it was verified against nullptr. Check lines: 71, 75. OSCollectionIterator.cpp 71
- V595 CWE-476 The 'ifp' pointer was utilized before it was verified against nullptr. Check lines: 2014, 2018. dlil.c 2014
- V595 CWE-476 The 'fakeif' pointer was utilized before it was verified against nullptr. Check lines: 561, 566. if_fake.c 561
- V595 CWE-476 The 'sb' pointer was utilized before it was verified against nullptr. Check lines: 138, 140. in_pcblist.c 138
- V595 CWE-476 The 'tp' pointer was utilized before it was verified against nullptr. Check lines: 2603, 2610. tcp_subr.c 2603
- V595 CWE-476 The 'str_id' pointer was utilized before it was verified against nullptr. Check lines: 1812, 1817. kdebug.c 1812
- V595 CWE-476 The 'sessp' pointer was utilized before it was verified against nullptr. Check lines: 191, 194. subr_prf.c 191
- V595 CWE-476 The 'sessp' pointer was utilized before it was verified against nullptr. Check lines: 1463, 1469. tty.c 1463
- V595 CWE-476 The 'so' pointer was utilized before it was verified against nullptr. Check lines: 6714, 6719. uipc_socket.c 6714
- V595 CWE-476 The 'uap' pointer was utilized before it was verified against nullptr. Check lines: 314, 320. nfs_upcall.c 314
- V595 CWE-476 The 'xfromname' pointer was utilized before it was verified against nullptr. Check lines: 3986, 4006. kpi_vfs.c 3986
- Примечание. На самом деле я не стал внимательно смотреть все предупреждения этого типа. Поэтому, на самом деле ошибок может быть больше.
Фрагмент N36, N37
И последняя пара ошибок на тему использования нулевых указателей.
static void
feth_start(ifnet_t ifp)
{
....
if_fake_ref fakeif;
....
if (fakeif != NULL) {
peer = fakeif->iff_peer;
flags = fakeif->iff_flags;
}
/* check for pending TX */
m = fakeif->iff_pending_tx_packet;
....
}
Предупреждение PVS-Studio: V1004 CWE-476 The 'fakeif' pointer was used unsafely after it was verified against nullptr. Check lines: 566, 572. if_fake.c 572
Думаю, комментировать этот код не требуется. Просто посмотрите, как проверяется и используется указатель fakeif.
Последний аналогичный случай: V1004 CWE-476 The 'rt→rt_ifp' pointer was used unsafely after it was verified against nullptr. Check lines: 138, 140. netsrc.c 140
CWE-119: Improper Restriction of Operations within the Bounds of a Memory Buffer
Встретилась парочка ошибок, связанных с выходом за границу буфера. Очень неприятная разновидность ошибки для такого ответственного проекта, как XNU Kernel.
Разные варианты выхода за границу массива можно классифицировать разными CWE ID, но в данном случае анализатор выбрал CWE-119.
Фрагмент N38
Для начала рассмотрим, как объявлены некоторые макросы.
#define IFNAMSIZ 16
#define IFXNAMSIZ (IFNAMSIZ + 8)
#define MAX_ROUTE_RULE_INTERFACES 10
Нам важно запомнить, что:
- IFXNAMSIZ = 24
- MAX_ROUTE_RULE_INTERFACES = 10
А теперь рассмотрим функцию, где возможен выход за границу буфера при использовании функции snprintf и memset. Т.е. имеют место 2 ошибки.
static inline const char *
necp_get_result_description(....)
{
....
char interface_names[IFXNAMSIZ][MAX_ROUTE_RULE_INTERFACES];
....
for (index = 0; index < MAX_ROUTE_RULE_INTERFACES; index++) {
if (route_rule->exception_if_indices[index] != 0) {
ifnet_t interface = ifindex2ifnet[....];
snprintf(interface_names[index],
IFXNAMSIZ, "%s%d", ifnet_name(interface),
ifnet_unit(interface));
} else {
memset(interface_names[index], 0, IFXNAMSIZ);
}
}
....
}
Предупреждения PVS-Studio:
- V512 CWE-119 A call of the '__builtin___memcpy_chk' function will lead to a buffer overflow. — ADDITIONAL IN CURRENT necp_client.c 1459
- V557 CWE-787 Array overrun is possible. The value of 'length — 1' index could reach 23. — ADDITIONAL IN CURRENT necp_client.c 1460
Обратите внимание, как объявлен двумерный массив interface_names:
char interface_names[IFXNAMSIZ][MAX_ROUTE_RULE_INTERFACES];
// то есть: char interface_names[24][10];
Но при этом используется этот массив, как если бы он был таким:
char interface_names[MAX_ROUTE_RULE_INTERFACES][IFXNAMSIZ];
// то есть: char interface_names[10][24];
Получается какая-то каша из данных.
Кто-то, не подумав, может сказать, что ничего страшного нет, ведь оба массива занимают одинаковое количество байт.
Нет, всё плохо. Элементы массива interface_names[10…23][…] не используются, так как переменная index в цикле принимает значения [0…9]. Зато элементы interface_names[0…9][…] начинают «налезать» друг на друга. Т.е. одни данные затирают другие.
В итоге получается полная туфта. Часть массива остаётся неинициализированной, а другая часть содержит «кашу», когда данные записывались поверх уже записанных данных.
Фрагмент N39
Ниже в этом же файле necp_client.c есть ещё одна функция, содержащая очень похожие ошибки.
#define IFNAMSIZ 16
#define IFXNAMSIZ (IFNAMSIZ + 8)
#define NECP_MAX_PARSED_PARAMETERS 16
struct necp_client_parsed_parameters {
....
char prohibited_interfaces[IFXNAMSIZ]
[NECP_MAX_PARSED_PARAMETERS];
....
};
static int
necp_client_parse_parameters(....,
struct necp_client_parsed_parameters *parsed_parameters)
{
....
u_int32_t length = ....;
....
if (length <= IFXNAMSIZ && length > 0) {
memcpy(parsed_parameters->prohibited_interfaces[
num_prohibited_interfaces],
value, length);
parsed_parameters->prohibited_interfaces[
num_prohibited_interfaces][length - 1] = 0;
....
}
Предупреждение PVS-Studio:
- V512 CWE-119 A call of the '__builtin___memcpy_chk' function will lead to a buffer overflow. — ADDITIONAL IN CURRENT necp_client.c 1459
- V557 CWE-787 Array overrun is possible. The value of 'length — 1' index could reach 23. — ADDITIONAL IN CURRENT necp_client.c 1460
Всё то же самое. Массив:
char prohibited_interfaces[IFXNAMSIZ][NECP_MAX_PARSED_PARAMETERS];
обрабатывается, как будто он такой:
char prohibited_interfaces[NECP_MAX_PARSED_PARAMETERS][IFXNAMSIZ];CWE-563: Assignment to Variable without Use
Обнаруживаемые с помощью PVS-Studio дефекты CWE-563 часто являются последствиями опечаток. Сейчас как раз будет рассмотрена одна такая красивая опечатка.
Фрагмент N40
uint32_t
gss_krb5_3des_unwrap_mbuf(....)
{
....
for (cflag = 1; cflag >= 0; cflag--) {
*minor = gss_krb5_3des_token_get(
ctx, &itoken, wrap, &hash, &offset, &length, reverse);
if (*minor == 0)
break;
wrap.Seal_Alg[0] = 0xff;
wrap.Seal_Alg[0] = 0xff;
}
....
}
Предупреждение PVS-Studio: V519 CWE-563 The 'wrap.Seal_Alg[0]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2070, 2071. gss_krb5_mech.c 2071
Два раза записывают значение 0xff в один и тот же элемент массива. Я посмотрел код по соседству и пришел к выводу, что на самом деле здесь хотели написать:
wrap.Seal_Alg[0] = 0xff;
wrap.Seal_Alg[1] = 0xff;
Если судить по названию функции, то она связана с сетевым протоколом аутентификации. И такой ляп… Ужас-то какой.
Купить PVS-Studio можно здесь. Наш анализатор поможет предотвратить множество таких ошибок!
Фрагмент N41, N42, N43, N44
static struct mbuf *
pf_reassemble(struct mbuf *m0, struct pf_fragment **frag,
struct pf_frent *frent, int mff)
{
....
m->m_pkthdr.csum_flags &= ~CSUM_PARTIAL;
m->m_pkthdr.csum_flags =
CSUM_DATA_VALID | CSUM_PSEUDO_HDR |
CSUM_IP_CHECKED | CSUM_IP_VALID;
....
}
Предупреждение PVS-Studio: V519 CWE-563 The 'm→M_dat.MH.MH_pkthdr.csum_flags' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 758, 759. pf_norm.c 759
Строка:
m->m_pkthdr.csum_flags &= ~CSUM_PARTIAL;
не имеет никакого практического смысла. В следующей строке переменной m→m_pkthdr.csum_flags будет присвоено новое значение. Я не знаю, как на самом деле должен выглядеть правильный код, но рискну предположить, что забыли символ '|'. По моему скромному мнению, код должен выглядеть так:
m->m_pkthdr.csum_flags &= ~CSUM_PARTIAL;
m->m_pkthdr.csum_flags |=
CSUM_DATA_VALID | CSUM_PSEUDO_HDR |
CSUM_IP_CHECKED | CSUM_IP_VALID;
Есть ещё 3 предупреждения, указывающих на аналогичные ошибки:
- V519 CWE-563 The 'm→M_dat.MH.MH_pkthdr.csum_flags' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1349, 1350. pf_norm.c 1350
- V519 CWE-563 The 'm→M_dat.MH.MH_pkthdr.csum_flags' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2984, 2985. ip_input.c 2985
- V519 CWE-563 The 'm→M_dat.MH.MH_pkthdr.csum_flags' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 773, 774. frag6.c 774
CWE-14: Compiler Removal of Code to Clear Buffers
Очень коварный вид дефекта, который невидим в отладочной версии. Если читатель ещё не знаком с ним, то прежде чем продолжить чтение, предлагаю ознакомиться со следующими ссылками:
- Safe Clearing of Private Data.
- V597. The compiler could delete the 'memset' function call, which is used to flush 'Foo' buffer. The RtlSecureZeroMemory () function should be used to erase the private data.
- CWE-14: Compiler Removal of Code to Clear Buffers.
Если читателю непонятно, зачем вообще затирать приватные данные, хранящиеся в памяти, то рекомендую статью «Перезаписывать память — зачем?».
Итак, затирать приватные данные в памяти важно, но иногда компилятор убирает соответствующий код, так как, с его точки зрения, он лишний. Давайте посмотрим, что интересного нашлось в XNU Kernel на эту тему.
Фрагмент N45
__private_extern__ void
YSHA1Final(unsigned char digest[20], YSHA1_CTX* context)
{
u_int32_t i, j;
unsigned char finalcount[8];
....
/* Wipe variables */
i = j = 0;
memset(context->buffer, 0, 64);
memset(context->state, 0, 20);
memset(context->count, 0, 8);
memset(finalcount, 0, 8); // <=
#ifdef SHA1HANDSOFF
YSHA1Transform(context->state, context->buffer);
#endif
}
Предупреждение PVS-Studio: V597 CWE-14 The compiler could delete the 'memset' function call, which is used to flush 'finalcount' buffer. The memset_s () function should be used to erase the private data. sha1mod.c 188
Компилятор с целью оптимизации Release-версии вправе удалить строчку кода, которую я отметил комментарием »// <=". Почти наверняка он так и поступит.
Фрагмент N46
__private_extern__ void
YSHA1Transform(u_int32_t state[5], const unsigned char buffer[64])
{
u_int32_t a, b, c, d, e;
....
state[0] += a;
state[1] += b;
state[2] += c;
state[3] += d;
state[4] += e;
/* Wipe variables */
a = b = c = d = e = 0;
}
Предупреждение PVS-Studio: V1001 CWE-563 The 'a' variable is assigned but is not used until the end of the function. sha1mod.c 120
Компилятор вправе не генерировать код, обнуляющий переменные, так как они больше в функции не используются.
Хочу обратить внимание, что анализатор PVS-Studio интерпретировал эту подозрительную ситуацию как CWE-563. Дело в том, что один и тот же дефект часто можно интерпретировать как разные CWE и в данном случае анализатор выбрал CWE-563. Однако я решил отнести этот код именно к CWE-14, так как это точнее объясняет, что не так с этим кодом.
CWE-783: Operator Precedence Logic Error
Дефект CWE-783 возникает там, где программист перепутал приоритеты операций и написал код, который работает не так, как он планировал. Часто такие ошибки допускаются из-за невнимательности или пропущенных круглых скобок.
Фрагмент N47
int
getxattr(....)
{
....
if ((error = copyinstr(uap->attrname, attrname,
sizeof(attrname), &namelen) != 0)) {
goto out;
}
....
out:
....
return (error);
}
Предупреждение PVS-Studio: V593 CWE-783 Consider reviewing the expression of the 'A = B!= C' kind. The expression is calculated as following: 'A = (B!= C)'. vfs_syscalls.c 10574
Классическая ошибка. Я встречаю большое количество таких ошибок в различных программах (proof). Первопричина в том, что некоторые программисты зачем-то стремятся запихать побольше всего в одну строчку.
В результате, вместо:
Status s = foo();
if (s == Error)
return s;
они пишут:
Status s;
if (s = foo() == Error)
return s;
И вносят в код ошибку.
- Программист ожидает, что выражение вычисляется так: (s = foo ()) == Error.
- На самом деле, выражение вычисляется так: s = (foo () == Error).
В результате оператор return возвращает некорректный статус ошибки, равный 1, а не значение, равное константе Error.
Я регулярно критикую подобный код и рекомендую «не впихивать» в одну строчку сразу несколько действий. «Впихивание» не сильно сокращает размер кода, зато провоцирует различные ошибки. Подробнее про это предлагаю прочитать в моей мини-книге «Главный вопрос программирования, рефакторинга и всего такого». См. главы:
- 11. Не жадничайте на строчках кода
- 16. «Смотрите как я могу» — недопустимо в программировании
Вернёмся к коду из XNU Kernel. В случае ошибки, функция getxattr вернёт значение 1, а не настоящий код ошибки.
Фрагмент N48 — N52
static void
memorystatus_init_snapshot_vmstats(
memorystatus_jetsam_snapshot_t *snapshot)
{
kern_return_t kr = KERN_SUCCESS;
mach_msg_type_number_t count = HOST_VM_INFO64_COUNT;
vm_statistics64_data_t vm_stat;
if ((kr = host_statistics64(.....) != KERN_SUCCESS)) {
printf("memorystatus_init_jetsam_snapshot_stats: "
"host_statistics64 failed with %d\n", kr);
memset(&snapshot->stats, 0, sizeof(snapshot->stats));
} else {
+ ....
}
Предупреждение PVS-Studio: V593 CWE-783 Consider reviewing the expression of the 'A = B!= C' kind. The expression is calculated as following: 'A = (B!= C)'. kern_memorystatus.c 4554
Переменной kr может быть присвоено только два значения: 0 или 1. Из-за этого функция printf всегда распечатывает число 1, а не настоящий статус, который вернула функция host_statistics64.
Статья получается большая. Думаю, я утомлю ей не только себя, но и читателей. Поэтому я сокращаю количество рассматриваемых в статье фрагментов.
Другие аналогичные дефекты рассматривать неинтересно, и я ограничусь списком сообщений:
- V593 CWE-783 Consider reviewing the expression of the 'A = B!= C' kind. The expression is calculated as following: 'A = (B!= C)'. vfs_syscalls.c 10654
- V593 CWE-783 Consider reviewing the expression of the 'A = B!= C' kind. The expression is calculated as following: 'A = (B!= C)'. vfs_syscalls.c 10700
- V593 CWE-783 Consider reviewing the expression of the 'A = B!= C' kind. The expression is calculated as following: 'A = (B!= C)'. vfs_syscalls.c 10759
- V593 CWE-783 Consider reviewing the expression of the 'A = B!= C' kind. The expression is calculated as following: 'A = (B!= C)'. kern_exec.c 2297
CWE-758: Reliance on Undefined, Unspecified, or Implementation-Defined Behavior
Способов получить неопределённое или неуточнённое поведение в программе на языке C или C++ превеликое множество. Поэтому в анализаторе PVS-Studio реализовано достаточно много диагностик, нацеленных на выявление таких проблем: V567, V610, V611, V681, V704, V708, V726, V736.
В случае с XNU анализатор выявил только две слабости CWE-758, связанных с неопределённым поведением, вызванным сдвигом отрицательных чисел.
Фрагмент N53, N54
static void
pfr_prepare_network(union sockaddr_union *sa, int af, int net)
{
....
sa->sin.sin_addr.s_addr = net ? htonl(-1 << (32-net)) : 0;
....
}
Предупреждение PVS-Studio: V610 CWE-758 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. pf_table.c 976
