Релиз PVS-Studio для macOS: 64 weaknesses в Apple 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%016llxn", 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)

Вычислим некоторые выражения:

• ~(0x0FULL << 48) = 0xFFF0FFFFFFFFFFFF
• (0x01ULL << 48) = 0x0001000000000000

Выражение (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: 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

Очень коварный вид дефекта, который невидим в отладочной версии. Если читатель ещё не знаком с ним, то прежде чем продолжить чтение, предлагаю ознакомиться со следующими ссылками:

1. Safe Clearing of Private Data.
2. 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.
3. 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 %dn", 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

Сдвиг отрицательного числа влево приводит к неопределённому поведению. На практике этот код вполне может работать ровно так, как этого ожидает программист. Но всё равно этот код неправильный и его следует исправить. Это можно сделать так:

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

CWE-401: Improper Release of Memory Before Removing Last Reference ('Memory Leak')

Следует похвалить разработчиков 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;». Я не стал внимательно разбираться, так как к моменту, когда добрался до этих предупреждений, был уже уставшим.

Аналогичные предупреждения анализатора:

• V773 CWE-401 The 'inst' pointer was assigned values twice without releasing the memory. A memory leak is possible. IOUserClient.cpp 246
• V773 CWE-401 The 'myself' pointer was assigned values twice without releasing the memory. A memory leak is possible. IOPMrootDomain.cpp 9151

CWE-129: Improper Validation of Array Index

Дефект 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.

Аналогичные ошибки:

• 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: 651, 654. IOStateReporter.cpp 651
• V781 CWE-129 The value of the 'pri' variable is checked after it was used. Perhaps there is a mistake in program logic. Check lines: 267, 269. pktsched_fq_codel.c 267
• V781 CWE-129 The value of the 'pcid' variable is checked after it was used. Perhaps there is a mistake in program logic. Check lines: 224, 225. pmap_pcid.c 224

CWE-480: Use of Incorrect Operator

Дефекты 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);

CWE-665: Improper Initialization

Анализатор 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: Insufficient Control Flow Management

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.