# Ядро macOS, есть ли червячки в этом яблоке?

В самом начале этого года Apple выложили в открытый доступ исходный код системных компонентов macOS 11\.0 – Big Sur, включая XNU – ядро операционной системы macOS\. Пару лет назад исходный код ядра уже проверялся PVS\-Studio в связи с выходом анализатора для macOS\. Прошло достаточно много времени, и вышел новый релиз исходного кода ядра\. Почему бы и не провести повторную проверку\.

![0818_XNU_MacOS_Kernel_ru/image1.png](https://import.viva64.com/docx/blog/0818_XNU_MacOS_Kernel_ru/image1.png)

## Что это за проект, Apple и open\-source?

XNU – X is Not Unix – используется и разрабатывается Apple в качестве ядра операционных систем OS X\. Исходные коды этого ядра 20 лет назад были опубликованы под лицензией APSL \([Apple Public Source License](https://en.wikipedia.org/wiki/Apple_Public_Source_License)\) вместе с OC Darwin\. Раньше Darwin можно было даже установить в качестве полноценной операционной системы, однако теперь это стало невозможно\. Причиной публикации исходного кода является тот факт, что он во многом основан на других open\-source проектах\.

Исходные коды компонентов можно найти [тут](https://opensource.apple.com/)\. Для проверки я использовала [зеркало проекта](https://github.com/apple/darwin-xnu) на GitHub\.

## Предыдущая проверка

Как я уже упомянула, этот проект ранее проверялся нами с помощью PVS\-Studio\. С предыдущими результатами можно познакомиться в статье: "[Релиз PVS\-Studio для macOS: 64 weaknesses в Apple XNU Kernel](https://pvs-studio.ru/ru/blog/posts/cpp/0566/)"\. После публикации мой коллега Святослав также отправил статью разработчикам на почту, но ответа не получил\. Так что я предполагаю, что наша проверка никак не связана с исправлениями, которые мы дальше рассмотрим\. Разработчикам пришлось искать их другим путём\. А могли бы просто взять и запустить PVS\-Studio :\)\. Сейчас, после публикации статей, мы в основном пишем об этом в GitHub репозиторий проекта\.

Мне стало интересно, были ли исправлены ошибки, описанные в предыдущей статье, или всё так и осталось\. Большинство из найденных ошибок действительно были исправлены\. Это показывает, что отобранные предупреждения анализатора оказались верными\. Хотя для написания статьи с отчётом работал человек, не участвующий в разработке XNU, то есть близко не знакомый с этим исходным кодом\.

Я приведу здесь несколько примеров исправлений\. Но, чтобы не раздувать объём статьи, не буду полностью приводить объяснение ошибок\. Если из исправления будет неясно, в чём была проблема, то вы всегда можете обратиться к первой статье по проверке этого проекта\. Я не буду разбирать все исправленные фрагменты, большинство из фрагментов всё\-таки было поправлено\. А фрагментов в предыдущей статье было ни много ни мало 64\!

Перейдём к рассмотрению исправлений примеров из прошлой статьи\.

Фрагмент N1, в котором член класса сравнивался сам с собой:

```cpp
int
key_parse(
      struct mbuf *m,
      struct socket *so)
{
  ....
  if ((m->m_flags & M_PKTHDR) == 0 ||
      m->m_pkthdr.len != m->m_pkthdr.len) {
    ....
    goto senderror;
  }
  ....
}
```

Был исправлен следующим образом:

![0818_XNU_MacOS_Kernel_ru/image2.png](https://import.viva64.com/docx/blog/0818_XNU_MacOS_Kernel_ru/image2.png)

Где макрос, из которого получена переменная _orglen_, выглядит следующим образом:

```cpp
#define PFKEY_UNUNIT64(a) ((a) << 3)
```

Выходит, что анализатор оказался прав: сравнение было некорректным и должно было проводиться с переменной _orglen_, которая даже присутствовала в коде до исправления\.

Еще один пример исправления, который я хочу привести здесь, – фрагмент N5, где знак равно всё\-таки был исправлен на проверку на равенство\.

![0818_XNU_MacOS_Kernel_ru/image3.png](https://import.viva64.com/docx/blog/0818_XNU_MacOS_Kernel_ru/image3.png)

Накосячить в условии _assertf_ – одно, но ещё и перезаписать переменную для отладочной версии – такое точно стоит поправить\.

Фрагменты 6 и 7 были исправлены одинаково\. Оказалось, что во вложенной проверке перепутали значение перечислителя для сравнения\. Вместо _PBUF\_TYPE\_MBUF_ во внутренней проверке должен быть элемент _PBUF\_TYPE\_MEMORY_ в обоих случаях\.

![0818_XNU_MacOS_Kernel_ru/image4.png](https://import.viva64.com/docx/blog/0818_XNU_MacOS_Kernel_ru/image4.png)

В случае фрагментов N8, 9, 10 исправление было таким:

![0818_XNU_MacOS_Kernel_ru/image5.png](https://import.viva64.com/docx/blog/0818_XNU_MacOS_Kernel_ru/image5.png)

На это исправление я обратила внимание, так как серьёзная часть коммита в целом \(обновление репозитория до xnu\-4903\.270\.47 от 11 января\) содержит помимо прочего много правок код\-стайла\. Это может указывать на то, что для данной версии кодовая база была подчищена с помощью разных инструментов качества кода\. Что сделает эту проверку PVS\-Studio более интересной\. Ведь видно, что качество кодовой базы уже было улучшено другими инструментами\.

Что касается фрагментов 11, 12, 13, 14 – был исправлен только фрагмент 11:

![0818_XNU_MacOS_Kernel_ru/image6.png](https://import.viva64.com/docx/blog/0818_XNU_MacOS_Kernel_ru/image6.png)

Остальные остались прежними\. Похоже, кто\-то невнимательно прочитал наш отчёт ;\) \(или отчёт анализатора, использованный для улучшения качества кода в этом коммите\)\. Приведу здесь код, на который было выдано одно из предупреждений, чтобы не было сомнений в аналогичности ошибки:

```cpp
static int
kauth_resolver_getwork(user_addr_t message)
{
  struct kauth_resolver_work *workp;
  int error;

  KAUTH_RESOLVER_LOCK();
  error = 0;
  while ((workp = TAILQ_FIRST(....)) == NULL) { // <=
    thread_t thread = current_thread();
    struct uthread *ut = get_bsdthread_info(thread);

    ut->uu_save.uus_kauth.message = message;
    error = msleep0(....);
    KAUTH_RESOLVER_UNLOCK();
    /*
     * If this is a wakeup from another thread in the resolver
     * deregistering it, error out the request-for-work thread
     */
    if (!kauth_resolver_identity) {
      printf("external resolver died");
      error = KAUTH_RESOLVER_FAILED_ERRCODE;
    }
    return error; // <=
  }
  return kauth_resolver_getwork2(message);
}
```

Предупреждение PVS\-Studio: [V612](https://pvs-studio.ru/ru/docs/warnings/v612/) An unconditional 'return' within a loop\. kern\_credential\.c 951

Я привела код почти целиком, чтобы можно было сформировать общее представление о том, что происходит в этой функции\. В случае отмеченного цикла при выполнении условия входа в него будет совершён один проход по телу цикла, завершающийся возвращением _error_\. Видимо, подразумевалось, что если выполняется условие _\(workp \= TAILQ\_FIRST\(\.\.\.\.\)\) \=\= NULL_, то нужно найти причину ошибки и завершить функцию возвращением информации об ошибке\. Однако по какой\-то причине вместо _if_ был написан _while_, как и во фрагменте из предыдущей статьи\. Строчка _error \= msleep0\(\.\.\.\.\)_ выглядит в коде таким образом:

```cpp

error = msleep0(&kauth_resolver_unsubmitted,
                kauth_resolver_mtx,
                PCATCH,
                "GRGetWork",
                0, 
                kauth_resolver_getwork_continue);
```

Здесь последним аргументом передаётся указатель на функцию _kauth\_resolver\_getwork\_continue_\. В теле этой функции есть условие, аналогичное условию цикла, на который нам указал анализатор\. Но в нём уже корректно используется _if_, а не _while_\.

```cpp
static int
kauth_resolver_getwork_continue(int result)
{
  ....
  if (TAILQ_FIRST(&kauth_resolver_unsubmitted) == NULL) {
    ....
    return error;
  }
  ....
}
```

В принципе этот код работает немного сложнее, чем я описала\. В нём присутствует рекурсия \(в методе _kauth\_resolver\_getwork\_continue_\), и, как я поняла, он нацелен на нахождение потоков, которые можно перезагрузить\. Но я не стала вдаваться в подробности, так как _while_ всё равно лишний\. Возможно, он остался здесь с того времени, когда исходный код выполнял ту же задачу, но без использования рекурсии\. 

Это примеры из начала статьи\. Проскочим в середину и возьмём фрагмент N40\. В нём одному и тому же элементу дважды присваивается одно значение:

Предупреждение PVS\-Studio: [V519](https://pvs-studio.ru/ru/docs/warnings/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

Эта ошибка, конечно же, тоже была поправлена: 

![0818_XNU_MacOS_Kernel_ru/image7.png](https://import.viva64.com/docx/blog/0818_XNU_MacOS_Kernel_ru/image7.png)

Ну и ближе к концу статьи, фрагмент 62 был исправлен так, как и было предложено в предыдущей статье\. Причём это было единственной правкой в том файле\.

![0818_XNU_MacOS_Kernel_ru/image8.png](https://import.viva64.com/docx/blog/0818_XNU_MacOS_Kernel_ru/image8.png)

Фрагменты 63 и 64 также были исправлены, но там код был изменён капитально\. Поэтому понять, какое исправление было именно для рассмотренного предупреждения, сложно\.

## Новые находки

После этого долгого вступления перейду к ошибкам, которые привлекли моё внимание при последней проверке исходного кода XNU статическим анализатором PVS\-Studio\. Скажу честно, мне тяжело далась работа с отчётом, так как проект имеет сложный код и у меня нет опыта работы с подобной кодовой базой\. Но предупреждения PVS\-Studio достаточно подробны и имеют ссылку на документацию с примерами правильного и неправильного кода и описанием возможной проблемы, что очень меня выручило\.

К этой проверке cloc насчитал в проекте 1346 \*\.c файлов, 1822 С/C\+\+ хэдера и 225 \*\.cpp файлов\.

Ну и перейдём к разбору интересных находок\.

**Фрагмент N1**

```cpp
void
pe_identify_machine(__unused boot_args *args)
{
  ....
  // Start with default values.
  gPEClockFrequencyInfo.timebase_frequency_hz = 1000000000;
  gPEClockFrequencyInfo.bus_frequency_hz      =  100000000;
  ....
  gPEClockFrequencyInfo.dec_clock_rate_hz = 
    gPEClockFrequencyInfo.timebase_frequency_hz;
  gPEClockFrequencyInfo.bus_clock_rate_hz =
   gPEClockFrequencyInfo.bus_frequency_hz;
  .... 
  gPEClockFrequencyInfo.bus_to_dec_rate_den =
    gPEClockFrequencyInfo.bus_clock_rate_hz /
    gPEClockFrequencyInfo.dec_clock_rate_hz;
}
```

Предупреждение PVS\-Studio: [V1064](https://pvs-studio.ru/ru/docs/warnings/v1064/) The 'gPEClockFrequencyInfo\.bus\_clock\_rate\_hz' operand of integer division is less than the 'gPEClockFrequencyInfo\.dec\_clock\_rate\_hz' one\. The result will always be zero\. pe\_identify\_machine\.c 72

Все используемые здесь поля имеют целочисленный тип:

```cpp
extern clock_frequency_info_t gPEClockFrequencyInfo;

struct clock_frequency_info_t {
  unsigned long bus_clock_rate_hz;
  unsigned long dec_clock_rate_hz;
  unsigned long bus_to_dec_rate_den;
  unsigned long long bus_frequency_hz;
  unsigned long timebase_frequency_hz;
  ....
};
```

Через промежуточные присвоения полю _gPEClockFrequencyInfo\.bus\_clock\_rate\_hz,_ являющемуся делимым, присваивается значение 100000000, а полю\-делителю _gPEClockFrequencyInfo\.dec\_clock\_rate\_hz_ присваивается значение 1000000000\. Делитель в этом случае в десять раз больше делимого\. Так как все поля здесь являются целочисленными, поле _gPEClockFrequencyInfo\.bus\_to\_dec\_rate\_den_ окажется равным 0\. 

Судя по наименованию результирующего поля _bus\_to\_dec\_rate\_den_, делитель и делимое были перепутаны местами\. Я допускаю возможность, что код был написан с расчётом на то, что исходные значения изменятся и результат уже не будет равен 0\. Но этот код всё равно кажется мне очень подозрительным\.

**Фрагмент N2**

```cpp
void
sdt_early_init( void )
{
  ....
  if (MH_MAGIC_KERNEL != _mh_execute_header.magic) {
  ....
  } else {
    ....
    for (....) {
    const char *funcname;
    unsigned long best;                           // <=
    ....
    funcname = "<unknown>";
    for (i = 0; i < orig_st->nsyms; i++) {
      char *jname = strings + sym[i].n_un.n_strx;
      ....
      if ((unsigned long)sym[i].n_value > best) { // <=
        best = (unsigned long)sym[i].n_value;
        funcname = jname;
      }
    }
    .....
  }
}
```

Предупреждение PVS\-Studio: [V614](https://pvs-studio.ru/ru/docs/warnings/v614/) Uninitialized variable 'best' used\. sdt\.c 572

Насколько я поняла, этот метод ищет название некоей функции\. В алгоритме используется переменная _best_, возможно, это положение наилучшего кандидата на результат\. Однако изначально эта переменная только объявляется без инициализации\. Следующее же использование сверяет значение некоего элемента с переменной _best_, которая будет неинициализированной на тот момент\. Еще страннее, что она инициализируется только внутри условия, в котором используется её же значение\.

Неинициализированные переменные могут приводить к непредсказуемым результатам\. И, хотя эта ошибка может показаться достаточно банальной, она всё ещё часто встречается при проверках разных проектов с помощью PVS\-Studio\. Например, совсем недавно мой коллега Андрей описывал [интересный случай](https://pvs-studio.ru/ru/blog/posts/cpp/0796/) такой ошибки\.

**Фрагмент N3**

```cpp
int
cdevsw_isfree(int index)
{
  struct cdevsw * devsw;
 
  if (index < 0) {
    if (index == -1) {
      index = 0;
    } else {
      index = -index; 
    }
    devsw = &cdevsw[index];
    for (; index < nchrdev; index++, devsw++) {
      if (memcmp(....) == 0) {
        break;
      }
    }
  }
 
  if (index < 0 || index >= nchrdev) {
    return -1;
  }
  ....
  return index;
}
```

Предупреждение PVS\-Studio: [V560](https://pvs-studio.ru/ru/docs/warnings/v560/) A part of conditional expression is always false: index < 0\. bsd\_stubs\.c:236

Небольшой пример того, что анализатор отслеживает возможные значения переменных\. В начале функции переменная _index_ сравнивается с нулём\. Если она меньше него, то во внутреннем блоке ей будет присвоено значение не меньше нуля\. А следующий внешний _if_ снова проверяет, имеет ли _index_ значение меньше нуля, но это невозможно\.

Логики программы этот момент не меняет, но есть вероятность, что подразумевалось какое\-то иное условие\. Ну и в любом случае лишние проверки не делают код читабельнее и понятнее\.

**Фрагмент N4**

```cpp
int
nfs_vinvalbuf_internal(....)
{
  struct nfsbuf *bp;
  ....
  off_t end = ....;

  /* check for any dirty data before the EOF */
  if ((bp->nb_dirtyend > 0) && (bp->nb_dirtyoff < end))
  {
    /* clip dirty range to EOF */
    if (bp->nb_dirtyend > end)
    {
      bp->nb_dirtyend = end;

      if (bp->nb_dirtyoff >= bp->nb_dirtyend)             // <=
      {
        bp->nb_dirtyoff = bp->nb_dirtyend = 0;
      }
    }

    if ((bp->nb_dirtyend > 0) && (bp->nb_dirtyoff < end)) // <=
    {
      ....
    }
  }
  ....
}
```

Предупреждения PVS\-Studio:

* [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'bp\-\>nb\_dirtyoff \>\= bp\-\>nb\_dirtyend' is always false\. nfs\_bio\.c 3858
* [V560](https://pvs-studio.ru/ru/docs/warnings/v560/) A part of conditional expression is always true: \(bp\-\>nb\_dirtyoff < end\)\. nfs\_bio\.c 3862

В случае этого фрагмента к анализатору точно стоит прислушаться и попытаться упростить код\. Учтите, что тут он хотя бы приведён с сокращениями\. 

Начнём с первого предупреждения\. Анализатор решил, что _nb\_dirtyoff_ не может быть больше или равен _nb\_dirtyend_\. Разберёмся почему\. Перед подозрительной проверкой есть ещё два _if_ с проверками _\(bp\-\>nb\_dirtyend \> 0\) && \(bp\-\>nb\_dirtyoff < end\)_ и _bp\-\>nb\_dirtyend \> end_\. А также осуществляется присвоение _bp\-\>nb\_dirtyend \= end_\.

Почему же третья проверка _bp\-\>nb\_dirtyoff \>\= bp\-\>nb\_dirtyend_ будет всегда _false_? 

![0818_XNU_MacOS_Kernel_ru/image9.png](https://import.viva64.com/docx/blog/0818_XNU_MacOS_Kernel_ru/image9.png)

Всё просто\. Из условий выходит, что _nb\_dirtyoff_ меньше, чем _end_, а _nb\_dirtyend_ равно _end_\. В итоге _nb\_dirtyend_ точно больше, чем _nb\_dirtyoff_\. Присвоение _bp\-\>nb\_dirtyoff \= bp\-\>nb\_dirtyend \= 0_ никогда не будет выполнено\.

В итоге вот такой участок кода:

```cpp
if ((bp->nb_dirtyend > 0) && (bp->nb_dirtyoff < end)) {
  /* clip dirty range to EOF */
  if (bp->nb_dirtyend > end) {
    bp->nb_dirtyend = end;
    if (bp->nb_dirtyoff >= bp->nb_dirtyend) {  // <=
      bp->nb_dirtyoff = bp->nb_dirtyend = 0;
    }
  }
}
```

Можно упростить хотя бы до такого:

```cpp
if ((bp->nb_dirtyend > 0) && (bp->nb_dirtyoff < end)) {
  if (bp->nb_dirtyend > end) {
    bp->nb_dirtyend = end;
  }
}
```

Но только если в настоящий момент этот алгоритм работает корректно\.

Второе предупреждение указывает на четвёртый _if_, вложенный в первый\. 

```cpp
if ((bp->nb_dirtyend > 0) && (bp->nb_dirtyoff < end))
```

Здесь анализатор выдаёт предупреждение на основании того, что присвоение нуля никогда не будет выполнено\. В итоге во внешнем условии уже была проверка _bp\-\>nb\_dirtyoff < end_ и внутренняя проверка из\-за ошибки в условии выше становится бессмысленной\.

**Фрагмент N5**

```cpp
tcp_output(struct tcpcb *tp)
{
  ....
  if (isipv6) {
    ....
    if (len + optlen) {
      ....
    }
  } else {
    ....
    if (len + optlen) {
      ....
    }
  }
  ....
}
```

Предупреждение PVS\-Studio: [V793](https://pvs-studio.ru/ru/docs/warnings/v793/) It is odd that the result of the 'len \+ optlen' statement is a part of the condition\. Perhaps, this statement should have been compared with something else\.

Это достаточно простой недочёт\. В условии вместо логического выражения просто складываются две переменные\. В итоге выражение будет ложным, только если сумма окажется равной нулю\. Если это и подразумевалось, то, возможно, стоит сделать сравнение с 0 явным, чтобы вопроса о правильности условия точно не возникало\.

Конечно, может быть, что так и задумано, но чуть выше в коде есть вот такая проверка:

```cpp
if (len + optlen + ipoptlen > tp->t_maxopd) {
  ....
}
```

Это наводит на мысль, что, скорее всего, в двух _if_'ах, на которые указал анализатор, также должно было проводиться сравнение\.

Ещё замечу, что эта функция, сокращённая тут до 16 строк, занимает в оригинале 2268 строк\! Ещё один возможный повод для рефакторинга ;\)

Второе предупреждение на этот же участок:

[V793](https://pvs-studio.ru/ru/docs/warnings/v793/) It is odd that the result of the 'len \+ optlen' statement is a part of the condition\. Perhaps, this statement should have been compared with something else\.

**Фрагмент N6**

```cpp
int
ttyinput(int c, struct tty *tp)
{
  ....
  if (tp->t_rawq.c_cc + tp->t_canq.c_cc) {
  ....
}
```

Предупреждение PVS\-Studio: [V793](https://pvs-studio.ru/ru/docs/warnings/v793/) It is odd that the result of the 'tp\-\>t\_rawq\.c\_cc \+ tp\-\>t\_canq\.c\_cc' statement is a part of the condition\. Perhaps, this statement should have been compared with something else\. tty\.c 568

Аналогичный случай\. Тут повыше в коде снова есть проверка, которая не просто использует сумму, а сравнивает результат с другой переменной:

```cpp
if (   tp->t_rawq.c_cc + tp->t_canq.c_cc > I_HIGH_WATER – 3 // <=
    && ....) {
  ....
}
```

В упрощённом коде условие, на которое указал анализатор, выглядит заметно\. Но в оригинале оно было вложено в несколько _if_\. Так что при код\-ревью такое можно и пропустить, а анализатор не пропустит ;\)

**Фрагмент N7**

```cpp
errno_t
mbuf_adjustlen(mbuf_t m, int amount)
{
  /* Verify m_len will be valid after adding amount */
  if (amount > 0) {
    int used =  (size_t)mbuf_data(m)
              - (size_t)mbuf_datastart(m)
              + m->m_len;

    if ((size_t)(amount + used) > mbuf_maxlen(m)) {
      ....
    }
  ....
  return 0;
}
```

Предупреждение PVS\-Studio: [V1028](https://pvs-studio.ru/ru/docs/warnings/v1028/) Possible overflow\. Consider casting operands of the 'amount \+ used' operator to the 'size\_t' type, not the result\. kpi\_mbuf\.c

Снова ошибка в условии, но уже совсем другого рода\. Вместо приведения к _size\_t_ операндов сложения, чтобы результат точно поместился в числовой тип, к _size\_t_ приводится результат сложения\. Если в итоге сложения возникнет переполнение, то с результатом _mbuf\_maxlen\(m\)_ будет сравниваться бессмысленное значение, приведённое к _size\_t_\. Раз программист всё\-таки хотел защититься от переполнения, то стоит его сделать правильно:

```cpp
if ((size_t)amount + used > mbuf_maxlen(m))
```

Таких срабатываний было несколько, стоит обратить на этот момент внимание\.

* [V1028](https://pvs-studio.ru/ru/docs/warnings/v1028/) Possible overflow\. Consider casting operands, not the result\. vm\_compressor\_pager\.c 1165
* [V1028](https://pvs-studio.ru/ru/docs/warnings/v1028/) Possible overflow\. Consider casting operands, not the result\. vm\_compressor\_pager\.c 1131
* [V1028](https://pvs-studio.ru/ru/docs/warnings/v1028/) Possible overflow\. Consider casting operands, not the result\. audit\_worker\.c 241
* [V1028](https://pvs-studio.ru/ru/docs/warnings/v1028/) Possible overflow\. Consider casting operands of the '\(\(u\_int32\_t\) slp \* hz\) \+ 999999' operator to the 'long' type, not the result\. tty\.c 2199

**Фрагмент N8**

```cpp
int
fdavail(proc_t p, int n)
{
  ....
  char *flags;
  int i;
  int lim;
  ....
  lim = (int)MIN(....);
  if ((i = lim - fdp->fd_nfiles) > 0 && (n -= i) <= 0) // <=
  {
    return 1;
  }
  ....
  for (....)
  {
    if (*fpp == NULL && !(*flags & UF_RESERVED) && --n <= 0)
    {
      return 1;
    }
  }
  return 0;
}
```

Предупреждение PVS\-Studio: [V1019](https://pvs-studio.ru/ru/docs/warnings/v1019/) Compound assignment expression 'n \-\= i' is used inside condition\. kern\_descrip\.c\_99 3916

Этот код, на мой взгляд, является крайне сложночитаемым\. Возможно, условие, на которое указал анализатор, стоит переписать в более простом виде:

```cpp
i = lim - fdp->fd_nfiles;
if (i > 0)
{
  n -= i;
  if(n <= 0)
    return 1;
}
```

Этот код выглядит менее эффективным, но точно является более понятным\. Для быстрой проверки равнозначности эффективности этого кода можно зайти на [Godbolt](https://godbolt.org/) \(Compiler Explorer\), где, кстати, можно тестировать работу диагностик PVS\-Studio\. Анализатор легко найти среди инструментов этого сервиса\.

Если не включать оптимизации, то ассемблерный код [получится на пару строк больше](https://godbolt.org/z/9bhqjb)\. А вот с оптимизациями [разницы уже нет никакой](https://godbolt.org/z/fqefbz)\. Так что писать тут хитрый код нет смысла, компилятор всё сам сделает, как надо\.

Но, если обратить внимание на тело этого _if_, новое значение _n_ в нём не используется\. То есть вполне возможно, что никакое присвоение здесь и не нужно\. Тогда можно обойтись таким кодом:

```cpp
i = lim - fdp->fd_nfiles;
if (i > 0) {
  if(n – i <= 0)
    return 1;
}
```

И, более того, исходный код может приводить к ошибке при дальнейшем использовании переменной _n_\. Если выражение _\(n \-\= i\) <\= 0_ окажется ложным, то далее будет использоваться уже новое значение _n_\. Так как я не работала вплотную с исходным кодом, мне сложно сказать, какое поведение является верным\.

**Фрагмент N9**

```cpp
static errno_t
vsock_put_message_listening(struct vsockpcb *pcb, 
                            enum vsock_operation op,
                            struct vsock_address src, 
                            struct vsock_address dst)
{
  switch (op)
  {
    case VSOCK_REQUEST:
      ....
      if (....)
      {
        vsock_pcb_safe_reset_address(pcb, dst, src);
        ....
      }
      ....
      done:
        ....
        break;
    case VSOCK_RESET:
      error = vsock_pcb_safe_reset_address(pcb, dst, src);
      break;
    default:
      vsock_pcb_safe_reset_address(pcb, dst, src);
      ....
      break;
  }
  return error;
}
```

Предупреждение PVS\-Studio: [V764](https://pvs-studio.ru/ru/docs/warnings/v764/) Possible incorrect order of arguments passed to 'vsock\_pcb\_safe\_reset\_address' function: 'dst' and 'src'\. vsock\_domain\.c 549

Этот момент запросто может оказаться и не ошибкой\. Но крайне подозрительно, что сигнатура вызываемой в этом фрагменте функции выглядит так:

```cpp
static errno_t
vsock_pcb_safe_reset_address(struct vsockpcb *pcb, 
                             struct vsock_address src, 
                             struct vsock_address dst)
```

При использовании этой функции в данном фрагменте последние два аргумента с аналогичными именами передаются в другом порядке\.

Срабатывания на тот же фрагмент:

* [V764](https://pvs-studio.ru/ru/docs/warnings/v764/) Possible incorrect order of arguments passed to 'vsock\_pcb\_safe\_reset\_address' function: 'dst' and 'src'\. vsock\_domain\.c 587
* [V764](https://pvs-studio.ru/ru/docs/warnings/v764/) Possible incorrect order of arguments passed to 'vsock\_pcb\_safe\_reset\_address' function: 'dst' and 'src'\. vsock\_domain\.c 590

**Фрагмент N10**

```cpp
int
ifclassq_tbr_set(struct ifclassq *ifq, ....)
{
  struct tb_regulator *tbr;
  ....
 
  tbr = &ifq->ifcq_tbr;
  ....
  tbr->tbr_rate = TBR_SCALE(rate / 8) / machclk_freq;
  ....
  tbr->tbr_last = read_machclk();
 
  if (   tbr->tbr_rate > 0               // <=
      && (ifp->if_flags & IFF_UP))
  { 
    ....
  } else {
    ....
  }
  ....
  return 0;
}
```

Предупреждение PVS\-Studio: [V1051](https://pvs-studio.ru/ru/docs/warnings/v1051/) Consider checking for misprints\. It's possible that the 'tbr\-\>tbr\_last' should be checked here\. classq\_subr\.c 685

В проекте эта диагностика работала не лучшим образом, так как в коде постоянно над телом условия или цикла инициализировались сторонние переменные с именами, похожими на используемые в условии\. Поэтому на этот раз диагностика выдала несколько явно ложных предупреждений\. Но рассматриваемое нами срабатывание всё же показалось мне подозрительным, так как проверяемое поле _tbr\_rate_ не использовалось в теле условия и было инициализировано на 35 строк выше этой проверки\. А вот поле _tbr\_last_, инициализированное прямо перед этой проверкой, больше нигде не используется\. Можно предположить, что проверить нужно было его вместо _tbr\_rate_\.

**Фрагмент N11**

```cpp
void
audit_arg_mac_string(struct kaudit_record *ar, ....)
{
  if (ar->k_ar.ar_arg_mac_string == NULL)
  {
    ar->k_ar.ar_arg_mac_string = kheap_alloc(....);
  }
  ....
  if (ar->k_ar.ar_arg_mac_string == NULL)
  {
    if (ar->k_ar.ar_arg_mac_string == NULL) // <=
    {
      return;
    }
  }
  ....
}
```

Предупреждение PVS\-Studio: [V571](https://pvs-studio.ru/ru/docs/warnings/v571/) Recurring check\. The 'if \(ar\-\>k\_ar\.ar\_arg\_mac\_string \=\= NULL\)' condition was already verified in line 245\. audit\_mac\.c 246

Предупреждение PVS\-Studio: [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'ar\-\>k\_ar\.ar\_arg\_mac\_string \=\= NULL' is always true\. audit\_mac\.c 246

На этот код анализатор выдал сразу два предупреждения\. 

Сначала взгляд может зацепиться за то, что проверка в самом первом _if_ и во втором совпадает\. Но тут всё правильно: внутри тела первой проверки аллоцируется память, а для второй проверки есть пояснение:

```cpp
/*
 * XXX This should be a rare event.
 * If kheap_alloc() returns NULL,
 * the system is low on kernel virtual memory. To be
 * consistent with the rest of audit, just return
 * (may need to panic if required to for audit).
 */
```

Судя по этому комментарию, во второй проверке не должно быть никакой внутренней проверки\. Нужно просто выйти из метода\. Так что, скорее всего, внутренняя проверка была продублирована случайно и не имеет никакого смысла\.

Хотя возможен и тот вариант, что во внутренней проверке нужно было проверить какое\-то другое поле\. Но сюда закралась ошибка копипасты, и разработчик забыл поправить имя поля\. 

**Фрагмент N12**

```cpp
int
utf8_encodestr(....)
{
  u_int16_t ucs_ch;
  int swapbytes = ....;
  ....
  ucs_ch = swapbytes ? OSSwapInt16(*ucsp++) : *ucsp++;
  ....
}
```

Предупреждение PVS\-Studio: [V567](https://pvs-studio.ru/ru/docs/warnings/v567/) Undefined behavior\. The 'ucsp' variable is modified while being used twice between sequence points\. vfs\_utfconv\.c 298

Макросы – очень коварная штука\. Возможно, вы даже уже встречались с нашей статьей "[Вред макросов для C\+\+ кода](https://habr.com/ru/company/pvs-studio/blog/444612/)"\. Я обычно при написании статей избегаю срабатываний на макросы\. С ними всегда всё оказывается сложно без знакомства с кодовой базой проекта\.

Но в случае этой ошибки всё оказалось чуть проще\. Хотя, чтобы дойти до причины и развернуть цепочку макросов, пришлось прыгнуть в ту ещё кроличью нору\. Собственно, цепочка эта начинается с выражения _OSSwapInt16\(\*ucsp\+\+\)_\.

![0818_XNU_MacOS_Kernel_ru/image10.png](https://import.viva64.com/docx/blog/0818_XNU_MacOS_Kernel_ru/image10.png)

Потом я поняла, что есть способ попроще, и просто обратилась к \.i файлу, который остался после проверки проекта\. По нему строка с этим макросом развернулась следующим образом:

```cpp
ucs_ch = swapbytes
? ( (__uint16_t)(__builtin_constant_p(*ucsp++)
   ? ((__uint16_t)(  (((__uint16_t)(*ucsp++) & 0xff00U) >> 8)
                   | (((__uint16_t)(*ucsp++) & 0x00ffU) << 8)))
   : _OSSwapInt16(*ucsp++)))
: *ucsp++;
```

Больше всего здесь нас интересует вот этот участок выражения:

```cpp
  (((__uint16_t)(*ucsp++) & 0xff00U) >> 8)
| (((__uint16_t)(*ucsp++) & 0x00ffU) << 8)
```

Никакой из операторов в выражении не является точкой следования\. Так как точно неизвестно, какой из аргументов оператора \| будет вычисляться первым, значение _\*uscp_ оказывается неопределённым\.

Для диагностики [V567](https://pvs-studio.ru/ru/docs/warnings/v567/) PVS\-Studio предоставляет крайне подробную документацию\. Если вам интересно, почему такой код может приводить к неопределённому поведению, документация может стать хорошим началом изучения этой проблемы\.

Однако это ещё не всё\! Есть очень интересный и важный момент\. Готова поспорить, что человек, писавший этот код, планировал увеличить значение _\*ucsp_ только один раз\. Но, на самом деле, значение увеличится дважды\. Это не видно и непонятно\. Макросы очень и очень опасны из\-за вот таких случаев\. Во многих ситуациях лучше написать обыкновенную функцию\. Скорее всего, компилятор автоматически выполнит подстановку и никакого ухудшения производительности не произойдёт\.

**Фрагмент N13**

```cpp
struct pf_status pf_status;

int
pf_insert_state(struct pf_state *s, ....)
{
  ....
  if (....) {
    s->id = htobe64(pf_status.stateid++);
    ....
  }
  ....
}
```

Предупреждение PVS\-Studio: [V567](https://pvs-studio.ru/ru/docs/warnings/v567/) Undefined behavior\. The 'pf\_status\.stateid' variable is modified while being used twice between sequence points\. pf\.c 1440

И снова коварные макросы смешали все карты для инкремента\. Рассмотрим строку с вызовом _htobe64_, которая оказалась подозрительной для анализатора после препроцессинга:

```cpp
s->id = (__builtin_constant_p(pf_status.stateid++) ? 
((__uint64_t)((((__uint64_t)(pf_status.stateid++) &
0xff00000000000000ULL) >> 56) | (((__uint64_t)(pf_status.stateid++) &
0x00ff000000000000ULL) >> 40) | (((__uint64_t)(pf_status.stateid++) &
0x0000ff0000000000ULL) >> 24) | (((__uint64_t)(pf_status.stateid++) &
0x000000ff00000000ULL) >> 8)  | (((__uint64_t)(pf_status.stateid++) &
0x00000000ff000000ULL) << 8)  | (((__uint64_t)(pf_status.stateid++) &
0x0000000000ff0000ULL) << 24) | (((__uint64_t)(pf_status.stateid++) &
0x000000000000ff00ULL) << 40) | (((__uint64_t)(pf_status.stateid++) &
0x00000000000000ffULL) << 56))) : _OSSwapInt64(pf_status.stateid++));
```

![0818_XNU_MacOS_Kernel_ru/image11.png](https://import.viva64.com/docx/blog/0818_XNU_MacOS_Kernel_ru/image11.png)

Проблема собственно та же, что и в предыдущем примере\. Во внутренней цепочке с операндами \| и & нет точек следования\. Поэтому неизвестно, какое значение примет _pf\_status\.stateid_ на моменте выполнения каждой операции\. Результат также неопределён\.

И, опять\-таки, переменная увеличивается несколько раз подряд, что является неприятным сюрпризом от макроса :\)\.

Вот остальные срабатывания этой диагностики на этом проекте:

* [V567](https://pvs-studio.ru/ru/docs/warnings/v567/) Undefined behavior\. The 'ip\_id' variable is modified while being used twice between sequence points\. ip\_id\.c 186
* [V567](https://pvs-studio.ru/ru/docs/warnings/v567/) Undefined behavior\. The 'lp' variable is modified while being used twice between sequence points\. nfs\_boot\.c 505
* [V567](https://pvs-studio.ru/ru/docs/warnings/v567/) Undefined behavior\. The 'lp' variable is modified while being used twice between sequence points\. nfs\_boot\.c 497
* [V567](https://pvs-studio.ru/ru/docs/warnings/v567/) Undefined behavior\. The 'ip\_id' variable is modified while being used twice between sequence points\. kdp\_udp\.c 588
* [V567](https://pvs-studio.ru/ru/docs/warnings/v567/) Undefined behavior\. The 'ip\_id' variable is modified while being used twice between sequence points\. kdp\_udp\.c 665
* [V567](https://pvs-studio.ru/ru/docs/warnings/v567/) Undefined behavior\. The 'ip\_id' variable is modified while being used twice between sequence points\. kdp\_udp\.c 1543

**Фрагмент N14**

```cpp
__private_extern__ boolean_t
ipsec_send_natt_keepalive(....)
{
  ....
  struct udphdr *uh = (__typeof__(uh))(void *)(  (char *)m_mtod(m)
                                                + sizeof(*ip));
  ....
  if (....)
  {
    uh->uh_sport = (u_short)sav->natt_encapsulated_src_port;
  } else {
    uh->uh_sport = htons((u_short)esp_udp_encap_port);
  }
  uh->uh_sport = htons((u_short)esp_udp_encap_port);
  ....
}
```

Предупреждение PVS\-Studio: [V519](https://pvs-studio.ru/ru/docs/warnings/v519/) The 'uh\-\>uh\_sport' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 4866, 4870\. ipsec\.c 4870

В этом фрагменте возникла подозрительная ситуация: полю _uh\_sport_ в зависимости от определённого условия присваиваются разные значения\. Однако сразу после _if\-else_ этому же полю снова присваивается значение, такое же как в ветке _else_\. В итоге этот _if\-else_ блок теряет смысл, так как значение поля всё равно будет перезаписано\.

**Фрагмент N15**

```cpp
static kern_return_t
vm_shared_region_slide_page_v3(vm_offset_t vaddr, ....)
{
  ....
  uint8_t *page_content = (uint8_t *)vaddr;
  uint16_t page_entry;
  ....
  uint8_t* rebaseLocation = page_content;
  uint64_t delta = page_entry;
  do {
    rebaseLocation += delta;
    uint64_t value;
    memcpy(&value, rebaseLocation, sizeof(value));
    ....
    bool isBind = (value & (1ULL << 62)) == 1;   // <=
    if (isBind) {
      return KERN_FAILURE;
    }
    ....
  } while (delta != 0);
  ....
}
```

Предупреждение PVS\-Studio: [V547](https://pvs-studio.ru/ru/docs/warnings/v547/)  Expression '\(value & \(1ULL << 62\)\) \=\= 1' is always false\. vm\_shared\_region\.c 2820

Тут получилось много кода из\-за использования большого числа переменных\. Но интересует нас только указанная мною строка с инициализацией _isBind_\. Разберём это выражение по шагам\. 

В результате побитового сдвига создаётся маска с единственной единицей в 63\-ем бите\. Результат побитового & с переменной value может принимать только значения 0 или 0x4000000000000000\. А никакое из этих значений не равно 1\. В итоге условие всегда будет ложным\.

Судя по тому, что выполнение этого условия должно было привести к выходу из функции с возвратом _KERN\_FAILURE_, возможно, как раз значение 0x4000000000000000 является тем самым исключительным случаем, после которого нужно выйти из функции\. Тогда результат побитовых операций надо было сравнивать с этим числом, а не с 1\. Ну, или написать так:

```cpp
bool isBind = (value & (1ULL << 62)) != 0;
```

**Фрагмент N16**

```cpp
int
vn_path_package_check(char *path, int pathlen, ....)
{
  char *ptr, *end;
  int comp = 0;
  ....
  end = path + 1;
  while (end < path + pathlen && *end != '\0') {
    while (end < path + pathlen && *end == '/' && *end != '\0') {
      end++;
    }
    ptr = end;

    while (end < path + pathlen && *end != '/' && *end != '\0') {
      end++;
    }
    ....
  }
  ....
}
```

Предупреждение PVS\-Studio: [V590](https://pvs-studio.ru/ru/docs/warnings/v590/) Consider inspecting this expression\. The expression is excessive or contains a misprint\. vfs\_subr\.c 3589

Эта диагностика всегда указывает на излишний код\. Иногда под ним скрывается более серьёзная ошибка\. Но здесь это, скорее всего, просто недочёт\. Предупреждение было выдано на первый внутренний _while_\. Нет смысла проверять, что символ одновременно равен '/' и не равен '\\0'\. Достаточно только первой проверки, так как если _\*end_ равен '/', то он точно не может быть равен '\\0'\.

Следующий _while_ содержит столько же проверок\. Но из\-за того, что в обоих случаях проверяется неравенство, эти проверки могут работать вместе\. Возможно, сначала был написан второй _while_, а первый был скопирован с него с изменённой проверкой для '/'\. Тогда перед нами недочёт, возникший из\-за копипасты\.

## Заключение

В этот раз в проекте нашлось несколько меньше ошибок, чем в предыдущей статье\. Весьма вероятно, что в процесс разработки XNU был внедрён статический анализ и другие инструменты контроля качества кода\. Почти наверняка на проекте используется Clang Static Analyzer\. Но ошибки и недочёты всё\-таки нашлись\. Я не стала приводить здесь некоторые срабатывания на подозрительные места, вывод по которым можно сделать только на основании большего понимания кодовой базы\.

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

Если вам интересно, какие ошибки может помочь найти статический анализ в целом и PVS\-Studio конкретно, то можете познакомиться с нашей [подборкой статей про проверку исходных проектов](https://pvs-studio.ru/ru/blog/inspections/)\. Там есть проверки кода не только операционных систем, но и, например, компиляторов и других инструментов программирования, которыми вы, возможно, пользуетесь ежедневно\. Например, совсем недавно вышла [статья про проверку Qt6](https://pvs-studio.ru/ru/blog/posts/cpp/0801/)\.