Новая версия PVS-Studio 6.23 работает под управлением macOS и позволяет проверять проекты, написанные на языке C и C++. К этому событию наша команда решила приурочить проверку XNU Kernel.
С выходом версии анализатора для 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 вы можете ознакомиться здесь.
С чего начать демонстрацию возможностей 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, т.е. к ситуациям, когда условие или часть условия всегда ложно/истинно. В случае с проектом 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 или 3. Бессмысленно проверять, будет ли 0, 1, 2 или 3 больше чем 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)Вычислим некоторые выражения:
Выражение (pat & [0xFFF0FFFFFFFFFFFF]) не может дать в результате значение 0x0001000000000000. Условие всегда истинно. Как следствие, функция 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 говорит о том, что в коде, скорее всего, что-то работает не так, как это задумывал программист.
Фрагмент 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)) {Или нужно просто добавить перенос строки, чтобы этот код не смущал ни анализатор, ни коллег, сопровождающих этот код.
Аналогичные подозрительные места можно найти здесь:
Фрагмент 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 раза:
Существуют различные причины, из-за которых может произойти разыменовывание нулевого указателя и анализатор PVS-Studio, в зависимости от ситуации, может назначать им различные CWE-ID:
При написании статьи я посчитал разумным собрать все ошибки этого типа в один раздел.
Фрагмент 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_handle_unregister_message:
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 и убедиться, что код написан некорректно.
Другие ошибки:
Фрагмент 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
Встретилась парочка ошибок, связанных с выходом за границу буфера. Очень неприятная разновидность ошибки для такого ответственного проекта, как XNU Kernel.
Разные варианты выхода за границу массива можно классифицировать разными CWE ID, но в данном случае анализатор выбрал CWE-119.
Фрагмент N38
Для начала рассмотрим, как объявлены некоторые макросы.
#define IFNAMSIZ 16
#define IFXNAMSIZ (IFNAMSIZ + 8)
#define 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:
Обратите внимание, как объявлен двумерный массив 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:
Всё то же самое. Массив:
char prohibited_interfaces[IFXNAMSIZ][NECP_MAX_PARSED_PARAMETERS];обрабатывается, как будто он такой:
char prohibited_interfaces[NECP_MAX_PARSED_PARAMETERS][IFXNAMSIZ];Обнаруживаемые с помощью 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 предупреждения, указывающих на аналогичные ошибки:
Очень коварный вид дефекта, который невидим в отладочной версии. Если читатель ещё не знаком с ним, то прежде чем продолжить чтение, предлагаю ознакомиться со следующими ссылками:
Если читателю непонятно, зачем вообще затирать приватные данные, хранящиеся в памяти, то рекомендую статью "Перезаписывать память - зачем?".
Итак, затирать приватные данные в памяти важно, но иногда компилятор убирает соответствующий код, так как, с его точки зрения, он лишний. Давайте посмотрим, что интересного нашлось в 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 возникает там, где программист перепутал приоритеты операций и написал код, который работает не так, как он планировал. Часто такие ошибки допускаются из-за невнимательности или пропущенных круглых скобок.
Фрагмент 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;И вносят в код ошибку.
В результате оператор return возвращает некорректный статус ошибки, равный 1, а не значение, равное константе Error.
Я регулярно критикую подобный код и рекомендую "не впихивать" в одну строчку сразу несколько действий. "Впихивание" не сильно сокращает размер кода, зато провоцирует различные ошибки. Подробнее про это предлагаю прочитать в моей мини-книге "Главный вопрос программирования, рефакторинга и всего такого". См. главы:
Вернёмся к коду из 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.
Статья получается большая. Думаю, я утомлю ей не только себя, но и читателей. Поэтому я сокращаю количество рассматриваемых в статье фрагментов.
Другие аналогичные дефекты рассматривать неинтересно, и я ограничусь списком сообщений:
Способов получить неопределённое или неуточнённое поведение в программе на языке 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
Сдвиг отрицательного числа влево приводит к неопределённому поведению. На практике этот код вполне может работать ровно так, как этого ожидает программист. Но всё равно этот код неправильный и его следует исправить. Это можно сделать так:
htonl((unsigned)(-1) << (32-net))Ещё один сдвиг анализатор PVS-Studio находит здесь: V610 CWE-758 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. pf_table.c 983
Следует похвалить разработчиков XNU Kernel за то, что анализатор не смог найти проблем с утечками памяти (CWE-401). Есть только 3 подозрительных места, когда при ошибке инициализации объекта не вызывается оператор delete. При этом я не уверен, что это именно ошибка.
Фрагмент N55, N56, N57
IOService * IODTPlatformExpert::createNub(IORegistryEntry * from)
{
IOService * nub;
nub = new IOPlatformDevice;
if (nub) {
if( !nub->init( from, gIODTPlane )) {
nub->free();
nub = 0;
}
}
return (nub);
}V773 CWE-401 The 'nub' pointer was assigned values twice without releasing the memory. A memory leak is possible. IOPlatformExpert.cpp 1287
Если функция init не сможет инициализировать объект, то возможно произойдёт утечка памяти. На мой взгляд, здесь не хватает оператора delete, и должно было быть написано так:
if( !nub->init( from, gIODTPlane )) {
nub->free();
delete nub;
nub = 0;
}Я не уверен, что прав. Возможно, функция free сама уничтожает объект, выполняя операцию "delete *this;". Я не стал внимательно разбираться, так как к моменту, когда добрался до этих предупреждений, был уже уставшим.
Аналогичные предупреждения анализатора:
Дефект CWE-129 говорит о том, что неправильно или недостаточно проверяются переменные, используемые для индексации элементов в массиве. Как следствие, может произойти выход за границу массива.
Фрагмент N58 - N61
IOReturn
IOStateReporter::updateChannelValues(int channel_index)
{
....
state_index = _currentStates[channel_index];
if (channel_index < 0 ||
channel_index > (_nElements - state_index)
/ _channelDimension) {
result = kIOReturnOverrun; goto finish;
}
....
}Предупреждение PVS-Studio: V781 CWE-129 The value of the 'channel_index' variable is checked after it was used. Perhaps there is a mistake in program logic. Check lines: 852, 855. IOStateReporter.cpp 852
Неправильно реализована защита от отрицательных значений. Вначале всегда извлекается элемент из массива, и только затем следует проверка, что индекс не является отрицательным.
Я думаю, этот код следует переписать следующим образом:
IOReturn
IOStateReporter::updateChannelValues(int channel_index)
{
....
if (channel_index < 0)
{
result = kIOReturnOverrun; goto finish;
}
state_index = _currentStates[channel_index];
if (channel_index > (_nElements - state_index)
/ _channelDimension) {
result = kIOReturnOverrun; goto finish;
}
....
}Возможно, следует ещё добавить проверки, что значение channel_index не превышает размер массива. С кодом я не знаком, поэтому оставляю это на усмотрение разработчиков XNU Kernel.
Аналогичные ошибки:
Дефекты CWE-480 обычно связаны с какими-то опечатками в выражениях. Их, как правило, не очень много, но зато они бывают весьма забавны. Смотришь на такие ошибки и удивляешься, как можно их было сделать. Тем не менее, как не раз мы демонстрировали это в статьях, от таких ошибок не застрахованы даже высококвалифицированные программисты.
Фрагмент N62
#define NFS_UC_QUEUE_SLEEPING 0x0001
static void
nfsrv_uc_proxy(socket_t so, void *arg, int waitflag)
{
....
if (myqueue->ucq_flags | NFS_UC_QUEUE_SLEEPING)
wakeup(myqueue);
....
}Предупреждение PVS-Studio: V617 CWE-480 Consider inspecting the condition. The '0x0001' argument of the '|' bitwise operation contains a non-zero value. nfs_upcall.c 331
Какая-то сущность "пробуждается" чаще, чем надо. Вернее, её "будят" постоянно, независимо от условия. Скорее всего, здесь должно быть написано:
if (myqueue->ucq_flags & NFS_UC_QUEUE_SLEEPING)
wakeup(myqueue);Анализатор PVS-Studio не смог классифицировать следующую ошибку согласно CWE. С моей точки зрения, мы имеем дело с CWE-665.
Фрагмент N63
extern void bzero(void *, size_t);
static struct thread thread_template, init_thread;
struct thread {
....
struct thread_qos_override {
struct thread_qos_override *override_next;
uint32_t override_contended_resource_count;
int16_t override_qos;
int16_t override_resource_type;
user_addr_t override_resource;
} *overrides;
....
};
void
thread_bootstrap(void)
{
....
bzero(&thread_template.overrides,
sizeof(thread_template.overrides));
....
}Предупреждение PVS-Studio: V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'thread_template.overrides' class object. thread.c 377
Взяли адрес переменной, хранящей указатель, и обнулили эту переменную, используя функцию bzero. Фактически, просто записали nullptr в указатель.
Использовать функцию bzero - это очень странный противоестественный способ обнулить значение переменной. Намного проще было написать:
thread_template.overrides = NULL;Отсюда я делаю вывод, что хотели обнулить буфер, но случайно обнулили указатель. То есть правильный код должен был быть таким:
bzero(thread_template.overrides,
sizeof(*thread_template.overrides));CWE-691 свидетельствует о наличии аномалии в последовательности выполнения инструкций. Возможна и другая аномалия - оформление кода не соответствует тому, как он работает. Именно с таким случаем я и столкнулся в коде XNU Kernel.
Фрагмент N64
Ура, мы добрались до рассмотрения последнего фрагмента кода! Возможно, есть и другие ошибки, которые я не заметил, просматривая отчёт, выданный анализатором, но напоминаю, что у меня не было цели выявить как можно больше ошибок. В любом случае, разработчики XNU Kernel смогут более качественно, чем я, изучить отчёт, так как им знаком код проекта. Так что давайте остановимся на красивом числе 64, которое созвучно с названием нашего сайта viva64.
Примечание. Тем, кому интересно, откуда пошло "viva64", предлагаю ознакомиться со статьёй "Как 10 лет назад начинался проект PVS-Studio".
void vm_page_release_startup(vm_page_t mem);
void
pmap_startup(
vm_offset_t *startp,
vm_offset_t *endp)
{
....
// -debug code remove
if (2 == vm_himemory_mode) {
for (i = 1; i <= pages_initialized; i++) {
....
}
}
else
// debug code remove-
/*
* Release pages in reverse order so that physical pages
* initially get allocated in ascending addresses. This keeps
* the devices (which must address physical memory) happy if
* they require several consecutive pages.
*/
for (i = pages_initialized; i > 0; i--) {
if(fill) fillPage(....);
vm_page_release_startup(&vm_pages[i - 1]);
}
....
}Предупреждение PVS-Studio: V705 CWE-691 It is possible that 'else' block was forgotten or commented out, thus altering the program's operation logics. vm_resident.c 1248
Возможно, здесь и нет ошибки. Однако меня очень смущает ключевое слово else. Код оформлен таким образом, что кажется, будто цикл выполняется всегда. На самом деле цикл выполняется только когда условие (2 == vm_himemory_mode) является ложным.
В мире macOS появился новый мощный статический анализатор кода PVS-Studio, способный выявлять ошибки и потенциальные уязвимости в программах на языке C и C++. Приглашаю всех желающих опробовать наш анализатор на своих проектах и оценить его возможности.
Спасибо за внимание и не забывайте поделиться с коллегами информацией о том, что PVS-Studio теперь доступен и для macOS.