PVS-Studio покопался во внутренностях Linux (3.18.1)

Соавтор: Святослав Размыслов SvyatoslavMC.

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

Что мы проверяли

Ядро Linux было взято с сайта The Linux Kernel Archives. Проверялась Latest Stable Kernel 3.18.1.

К тому моменту, как я пишу эту статью, уже появилось ядро 3.19-rc1. К сожалению, проверка проекта и написание статьи занимают немало времени. Поэтому будем довольствоваться проверкой не самой последней версии.

Тем, кто скажет, что мы должны были проверить именно самую последнюю версию, я хочу ответить следующее.

1. Мы регулярно проверяем большое количество проектов. Помимо бесплатной проверки проектов, у нас много других задач. И поэтому нет никакой возможности начинать всё сначала, только потому, что появилась новая версия. Так мы можем никогда ничего не опубликовать :).


2. 99% найденных ошибок осталось на своём месте. Так что эту статью смело можно использовать чтобы немного улучшить код ядра Linux.


3. Цель статьи — реклама PVS-Studio. Если мы можем найти ошибки в проекте версии X, то мы сможем найти что-то и в версии Y. Наши проверки достаточно поверхностны (мы не знакомы с проектом) и их целью является написание вот таких статей. Настоящую пользу проекту приносит приобретение лицензии на PVS-Studio и его регулярное использование.

Как мы проверяли

Для проверки ядра использовался статический анализатор кода PVS-Studio версии 5.21.

Для проверки Linux Kernel был использован дистрибутив Ubuntu-14.04, для которого имелось много подробных инструкций по конфигурированию и сборке ядра. Анализатор проверяет препроцессированные файлы, которые лучше получать для успешно компилируемых файлов, поэтому сборка проекта один из самых важных этапов проверки.

Далее была написана небольшая утилита на C++, которая для каждого запущенного процесса компилятора сохраняла командную строку, текущую директорию и переменные окружения. Читателям, знакомым с продуктами PVS-Studio, должна сразу вспомниться утилита PVS-Studio Standalone, которая позволяет проверить любой проект под Windows. Там для обращения к процессам используется WinAPI, поэтому для Linux данный механизм мониторинга был переписан, остальной же код, связанный с запуском препроцессирования и проверки, был полностью перенесён, и проверка ядра Linux стала лишь вопросом времени.

В начале о безопасности

Как-то так сложилось, что анализатор PVS-Studio воспринимают исключительно как инструмент для поиска ошибок. Но никто не обращает внимание, что PVS-Studio умеет обнаруживать и ряд уязвимостей. Мы, конечно, виноваты в этом сами. Надо немного выправить ситуацию.

Можно по-разному воспринимать те сообщения, которые выдаёт PVS-Studio. Например, с одной стороны это опечатка, а с другой может быть уязвимостью. Всё зависит от точки зрения.

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

Опасное использование функции memcmp()

static unsigned char eprom_try_esi(
  struct atm_dev *dev, unsigned short cmd,
  int offset, int swap)
{
  unsigned char buf[ZEPROM_SIZE];
  struct zatm_dev *zatm_dev;
  int i;

  zatm_dev = ZATM_DEV(dev);
  for (i = 0; i < ZEPROM_SIZE; i += 2) {
    eprom_set(zatm_dev,ZEPROM_CS,cmd); /* select EPROM */
    eprom_put_bits(zatm_dev,ZEPROM_CMD_READ,ZEPROM_CMD_LEN,cmd);
    eprom_put_bits(zatm_dev,i >> 1,ZEPROM_ADDR_LEN,cmd);
    eprom_get_byte(zatm_dev,buf+i+swap,cmd);
    eprom_get_byte(zatm_dev,buf+i+1-swap,cmd);
    eprom_set(zatm_dev,0,cmd); /* deselect EPROM */
  }
  memcpy(dev->esi,buf+offset,ESI_LEN);
  return memcmp(dev->esi,"\0\0\0\0\0",ESI_LEN);
}

Предупреждение PVS-Studio: V642 Saving the 'memcmp' function result inside the 'unsigned char' type variable is inappropriate. The significant bits could be lost breaking the program's logic. zatm.c 1168

Обратите внимание на оператор 'return' в самом конце тела функции.

Функция 'memcmp' возвращает следующие значения типа 'int':

• < 0 — buf1 less than buf2;


• 0 — buf1 identical to buf2;


• > 0 — buf1 greater than buf2;

Обратите внимание:

• "> 0", означает любые числа, а вовсе не 1;


• "< 0", это не обязательно -1.

Возвращаемыми значениями могут быть: -100, 2, 3, 100, 256, 1024, 5555 и так далее. Это значит, что этот результат нельзя преобразовывать в тип 'unsigned char' (это тип, который возвращает функция).

В результате неявного приведения типа могут быть отброшены значащие биты, что приведет к нарушению логики выполнения программы.

Опасность такого рода ошибок заключается в том, что возвращаемое значение может зависеть от архитектуры и реализации конкретной функции на данной архитектуре. Например, программа будет корректно работать в 32-битном варианте и некорректно в 64-битном.

Ну и что? Подумаешь, неправильно будет проверено что-то, связанное с EPROM. Ошибка, конечно, но при чём тут уязвимость?

А то, что диагностика V642 может выявить и уязвимость! Не верите? Пожалуйста, вот идентичный код из MySQL/MariaDB.

typedef char my_bool;
...
my_bool check(...) {
  return memcmp(...);
}

Не PVS-Studio нашёл эту ошибку. Но мог бы.

Эта ошибка послужила причиной серьезной уязвимости в MySQL/MariaDB до версий 5.1.61, 5.2.11, 5.3.5, 5.5.22. Суть в том, что при подключении пользователя MySQL /MariaDB вычисляется токен (SHA от пароля и хэша), который сравнивается с ожидаемым значением функцией 'memcmp'. На некоторых платформах возвращаемое значение может выпадать из диапазона [-128..127]. В итоге, в 1 случае из 256 процедура сравнения хэша с ожидаемым значением всегда возвращает значение 'true', независимо от хэша. В результате, простая команда на bash даёт злоумышленнику рутовый доступ к уязвимому серверу MySQL, даже если он не знает пароль. Причиной этому стал код в файле 'sql/password.c', показанный выше. Более подробное описание этой проблемы можно прочитать здесь: Security vulnerability in MySQL/MariaDB.

Вернёмся теперь к Linux. Вот ещё один опасный фрагмент кода:

void sci_controller_power_control_queue_insert(....)
{
  ....
  for (i = 0; i < SCI_MAX_PHYS; i++) {
    u8 other;
    current_phy = &ihost->phys[i];
  
    other = memcmp(current_phy->frame_rcvd.iaf.sas_addr,
                   iphy->frame_rcvd.iaf.sas_addr,
                   sizeof(current_phy->frame_rcvd.iaf.sas_addr));

    if (current_phy->sm.current_state_id == SCI_PHY_READY &&
        current_phy->protocol == SAS_PROTOCOL_SSP &&
        other == 0) {
      sci_phy_consume_power_handler(iphy);
      break;
    }
  }
  ....
}

Предупреждение PVS-Studio: V642 Saving the 'memcmp' function result inside the 'unsigned char' type variable is inappropriate. The significant bits could be lost breaking the program's logic. host.c 1846

Результат работы функции memcmp() помещается в переменную other, имеющую тип unsigned char. Не думаю, что эта какая-то уязвимость, но работа SCSI контроллер в опасности.

Ещё парочку таких мест можно найти здесь:

• V642 Saving the 'memcmp' function result inside the 'unsigned char' type variable is inappropriate. The significant bits could be lost breaking the program's logic. zatm.c 1168


• V642 Saving the 'memcmp' function result inside the 'unsigned char' type variable is inappropriate. The significant bits could be lost breaking the program's logic. host.c 1789

Опасное использование функции memset()

Продолжим искать опасные места. Обратим свой взор на функции, которые «подчищают» за собой приватные данные. Обычно это различные функции шифрования. К сожалению, не всегда обнуление памяти написано правильно. И тогда можно получить крайне неприятный результат. Подробнее про эти неприятные моменты можно узнать из этой статьи: "Перезаписывать память — зачем?".

Рассмотрим пример неправильного кода:

static int crypt_iv_tcw_whitening(....)
{
  ....
  u8 buf[TCW_WHITENING_SIZE];
  ....
  out:
  memset(buf, 0, sizeof(buf));
  return r;
}

Предупреждение PVS-Studio: V597 The compiler could delete the 'memset' function call, which is used to flush 'buf' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. dm-crypt.c 708

На первый взгляд всё хорошо. Функция crypt_iv_tcw_whitening() выделила на стеке временный буфер, что-то зашифровала, а затем обнулила буфер с приватными данными с помощью вызова функции memset(). Но, на самом деле, вызов функции memset() будет удалён компилятором при оптимизации. С точки зрения языка C/C++ после обнуления буфера он никак не используется. А значит буфер можно не обнулять.

При этом, заметить такую ошибку крайне сложно. Написать юнит-тест сложно. Под отладчиком такая ошибка тоже не будет видна (в Debug-версии будет присутствовать вызов функции memset).

При этом хочу подчеркнуть. Это не «теоретически-возможное поведение компилятора», а очень даже практическое. Компиляторы действительно удаляют вызов функции memset(). Подробнее про это можно прочитать в описании диагностики V597.

PVS-Studio неуместно рекомендует использовать функцию RtlSecureZeroMemory(), так как ориентирован на Windows. В Linux такой функции, конечно, нет. Но здесь главное предупредить, а подобрать нужную функцию уже не сложно.

Следующий аналогичный пример:

static int sha384_ssse3_final(struct shash_desc *desc, u8 *hash)
{
  u8 D[SHA512_DIGEST_SIZE];

  sha512_ssse3_final(desc, D);

  memcpy(hash, D, SHA384_DIGEST_SIZE);
  memset(D, 0, SHA512_DIGEST_SIZE);

  return 0;
}

Предупреждение PVS-Studio: V597 The compiler could delete the 'memset' function call, which is used to flush 'D' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. sha512_ssse3_glue.c 222

А ниже пример, где могут быть не обнулены сразу 4 буфера: keydvt_out, keydvt_in, ccm_n, mic. Код взят из файла security.c (строки 525 — 528).

int wusb_dev_4way_handshake(....)
{
  ....
  struct aes_ccm_nonce ccm_n;
  u8 mic[8];
  struct wusb_keydvt_in keydvt_in;
  struct wusb_keydvt_out keydvt_out;
  ....
error_dev_update_address:
error_wusbhc_set_gtk:
error_wusbhc_set_ptk:
error_hs3:
error_hs2:
error_hs1:
  memset(hs, 0, 3*sizeof(hs[0]));
  memset(&keydvt_out, 0, sizeof(keydvt_out));
  memset(&keydvt_in, 0, sizeof(keydvt_in));
  memset(&ccm_n, 0, sizeof(ccm_n));
  memset(mic, 0, sizeof(mic));
  if (result < 0)
    wusb_dev_set_encryption(usb_dev, 0);
error_dev_set_encryption:
  kfree(hs);
error_kzalloc:
  return result;
  ....
}

И последний пример, где в памяти остаётся «болтаться» какой-то пароль:

int
E_md4hash(const unsigned char *passwd, unsigned char *p16,
  const struct nls_table *codepage)
{
  int rc;
  int len;
  __le16 wpwd[129];

  /* Password cannot be longer than 128 characters */
  if (passwd) /* Password must be converted to NT unicode */
    len = cifs_strtoUTF16(wpwd, passwd, 128, codepage);
  else {
    len = 0;
    *wpwd = 0; /* Ensure string is null terminated */
  }

  rc = mdfour(p16, (unsigned char *) wpwd, len * sizeof(__le16));
  memset(wpwd, 0, 129 * sizeof(__le16));

  return rc;
}

Предупреждение PVS-Studio: V597 The compiler could delete the 'memset' function call, which is used to flush 'wpwd' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. smbencrypt.c 224

На этом остановлюсь. Ещё 3 неудачных memset() можно найти здесь:

• sha256_ssse3_glue.c 214


• dev-sysfs.c 104


• qp.c 143

Опасные проверки

У анализатора PVS-Studio есть диагностика V595, которая выявляет ситуации, когда указатель в начале разыменовывается, а потом проверяется на равенство NULL. Иногда с этой диагностикой всё просто и понятно. Рассмотрим такой простой случай:

static int tc_ctl_action(struct sk_buff *skb, struct nlmsghdr *n)
{
  struct net *net = sock_net(skb->sk);
  struct nlattr *tca[TCA_ACT_MAX + 1];
  u32 portid = skb ? NETLINK_CB(skb).portid : 0;
  ....
}

Предупреждение PVS-Studio: V595 The 'skb' pointer was utilized before it was verified against nullptr. Check lines: 949, 951. act_api.c 949

Здесь всё просто. Если указатель 'skb' будет равен нулю, то быть беде. Указатель разыменовывается в первой строке.

Следует отметить, что анализатор ругается не потому, что увидел разыменование непроверенного указателя. Так было бы слишком много ложных срабатываний. Ведь не всегда аргумент функции может быть равен 0. Возможно, проверка осуществлялась где-то ещё.

Логика работы иная. PVS-Studio считает код опасным, если указатель разыменовывается, а потом ниже проверяется. Если указатель проверяют, то предполагают, что он может быть равен 0. Следовательно это повод выдать предупреждение.

С этим простым примером разобрались. Но нам интересен вовсе не он.

Теперь перейдём к более сложному случаю, связанному с оптимизациями, которые выполняют компиляторы.

static int podhd_try_init(struct usb_interface *interface,
        struct usb_line6_podhd *podhd)
{
  int err;
  struct usb_line6 *line6 = &podhd->line6;

  if ((interface == NULL) || (podhd == NULL))
    return -ENODEV;
  ....
}

Предупреждение PVS-Studio: V595 The 'podhd' pointer was utilized before it was verified against nullptr. Check lines: 96, 98. podhd.c 96

Это пример кода, увидев который, люди начинают спорить и говорить, что всё хорошо. Они рассуждают так.

Пусть указатель podhd равен NULL. Некрасиво выглядит выражение &podhd->line6. Но ошибки нет. Здесь нет доступа к памяти. Просто вычисляется адрес одного из членов класса. Да, значение указателя 'line6' некорректно. Оно указывает «в никуда». Но ведь этот указатель не используется. Вычислили некорректный адрес, ну и что? Ниже в коде расположена проверка и если 'podhd', то функция завершит свою работу. Указатель 'line6' нигде не используется, а значит на практике никакой ошибки не будет.

Господа, вы не правы! Так делать всё равно нельзя. Не ленитесь и исправляйте такой код.

При оптимизации компилятор рассуждает так. Вот здесь указатель разыменовывается: podhd->line6. Ага, программист знает что делает. Значит указатель здесь точно не равен нулю. Отлично, запомним это.

Дальше компилятор встречает проверку:

if ((interface == NULL) || (podhd == NULL))
  return -ENODEV;

И оптимизирует её. Он считает, что указатель 'podhd' не равен нулю. Поэтому он сократит проверку до:

if ((interface == NULL))
  return -ENODEV;

Как и в случае с memset() дополнительная сложность в том, что мы не увидим отсутствие проверки в Debug() версии. Такую ошибку очень сложно искать.

В результате, если передать в функцию нулевой указатель, то функция не вернёт статус (-ENODEV), а продолжит свою работу. Последствия могут быть непредсказуемы.

Важно следующее. Компилятор может удалить важную проверку указателя из плохо написанного кода. Т.е. существуют функции, которые только кажется, что проверяют указатели. На самом деле, они будут работать с нулевым указателем. Не знаю, можно ли это как-то использовать. Но, на мой взгляд, такую ситуацию вполне можно рассматривать как потенциальную уязвимость.

Ещё один подобный пример:

int wpa_set_keys(struct vnt_private *pDevice, void *ctx,
     bool fcpfkernel) __must_hold(&pDevice->lock)
{
  ....
  if (is_broadcast_ether_addr(&param->addr[0]) ||
      (param->addr == NULL)) {
  ....
}

Предупреждение PVS-Studio: V713 The pointer param->addr was utilized in the logical expression before it was verified against nullptr in the same logical expression. wpactl.c 333

Компилятор при оптимизации может сократить проверку до:

if (is_broadcast_ether_addr(&param->addr[0]))

Ядро Linux большое. Так что предупреждений V595 было выдано более 200 штук. К своему стыду, я поленился их просматривать и выбрал только один пример для статьи. Остальные подозрительные места я предлагаю изучить кому-то из разработчиков. Я привожу их списком: Linux-V595.txt.

Да, далеко не все эти предупреждения указывают на реальную ошибку. Часто указатель точно не может быть равен нулю. Тем не менее, этот список стоит изучить. Почти наверняка можно найти пару десятков настоящих ошибок.

Найденные подозрительные места

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

Некорректные логические условия

void b43legacy_phy_set_antenna_diversity(....)
{
  ....
  if (phy->rev >= 2) {
    b43legacy_phy_write(
      dev, 0x0461, b43legacy_phy_read(dev, 0x0461) | 0x0010);
    ....
  } else if (phy->rev >= 6)
    b43legacy_phy_write(dev, 0x049B, 0x00DC);
  ....
}

Предупреждение PVS-Studio: V695 Range intersections are possible within conditional expressions. Example: if (A

Второе условие никогда не выполняется. Для наглядности упростим код:

if ( A >= 2)
  X();
else if ( A >= 6)
  Y();

Как видите, не существует такого значения в переменной 'A', при котором будет вызвана функция Y().

Рассмотрим другие похожие случаи. В пояснении они не нуждаются.

static int __init scsi_debug_init(void)
{
  ....
  if (scsi_debug_dev_size_mb >= 16)
    sdebug_heads = 32;
  else if (scsi_debug_dev_size_mb >= 256)
   sdebug_heads = 64;
  ....
}

Предупреждение PVS-Studio: V695 Range intersections are possible within conditional expressions. Example: if (A

static ssize_t ad5933_store(....)
{
  ....
  /* 2x, 4x handling, see datasheet */
  if (val > 511)
    val = (val >> 1) | (1 << 9);
  else if (val > 1022)
    val = (val >> 2) | (3 << 9);
  ....
}

Предупреждение PVS-Studio: V695 Range intersections are possible within conditional expressions. Example: if (A

Есть ещё парочка схожих ситуаций, которые не буду приводить, чтобы не делать статью излишне длинной:

• V695 Range intersections are possible within conditional expressions. Example: if (A < 5) {… } else if (A < 2) {… }. Check lines: 1417, 1422. bnx2i_hwi.c 1422


• V695 Range intersections are possible within conditional expressions. Example: if (A < 5) {… } else if (A < 2) {… }. Check lines: 4815, 4831. stv090x.c 4831

Теперь рассмотрим другую разновидность подозрительных условий.

static int dgap_parsefile(char **in)
{
  ....
  int module_type = 0;
  ....
  module_type = dgap_gettok(in);
  if (module_type == 0 || module_type != PORTS ||
      module_type != MODEM) {
    pr_err("failed to set a type of module");
    return -1;
  }
  ....
}

Предупреждение PVS-Studio: V590 Consider inspecting the 'module_type == 0 || module_type != 68' expression. The expression is excessive or contains a misprint. dgap.c 6733

Я не знаком с кодом и у меня нет идей, как должна выглядеть эта проверка. Поэтому воздержусь от комментариев. Вот ещё одно похожее место:

• V590 Consider inspecting the 'conc_type == 0 || conc_type != 65' expression. The expression is excessive or contains a misprint. dgap.c 6692

«Вырви глаз»

В ходе изучения сообщений анализатора я встретил функцию name_msi_vectors(). Она хоть и короткая, но читать ее совершенно не хочется. Видимо, именно поэтому она содержит очень подозрительную строчку.

static void name_msi_vectors(struct ipr_ioa_cfg *ioa_cfg)
{
  int vec_idx, n = sizeof(ioa_cfg->vectors_info[0].desc) - 1;

  for (vec_idx = 0; vec_idx < ioa_cfg->nvectors; vec_idx++) {
    snprintf(ioa_cfg->vectors_info[vec_idx].desc, n,
       "host%d-%d", ioa_cfg->host->host_no, vec_idx);
    ioa_cfg->vectors_info[vec_idx].
      desc[strlen(ioa_cfg->vectors_info[vec_idx].desc)] = 0;
  }
}

Предупреждение: V692 An inappropriate attempt to append a null character to a string. To determine the length of a string by 'strlen' function correctly, a string ending with a null terminator should be used in the first place. ipr.c 9409

Подозрительной является последняя строка:

ioa_cfg->vectors_info[vec_idx].
  desc[strlen(ioa_cfg->vectors_info[vec_idx].desc)] = 0;

Сейчас я её упрощу и сразу станет понятно, что здесь что-то не так:

S[strlen(S)] = 0;

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

Бесконечное ожидание

static int ql_wait_for_drvr_lock(struct ql3_adapter *qdev)
{
  int i = 0;

  while (i < 10) {
    if (i)
      ssleep(1);

    if (ql_sem_lock(qdev,
        QL_DRVR_SEM_MASK,
        (QL_RESOURCE_BITS_BASE_CODE | (qdev->mac_index)
         * 2) << 1)) {
      netdev_printk(KERN_DEBUG, qdev->ndev,
              "driver lock acquired\n");
      return 1;
    }
  }

  netdev_err(qdev->ndev,
             "Timed out waiting for driver lock...\n");
  return 0;
}

Предупреждение PVS-Studio: V654 The condition 'i

Функция пытается залочить драйвер. Если это не получается, она ждёт 1 секунду и повторяет попытку. Всего должно быть сделано 10 попыток.

Но, на самом деле, будет бесконечное количество попыток. Причина в том, что переменная 'i' нигде не увеличивается.

Неправильная информация об ошибке

static int find_boot_record(struct NFTLrecord *nftl)
{
  ....
  if ((ret = nftl_read_oob(mtd, block * nftl->EraseSize +
                           SECTORSIZE + 8, 8, &retlen,
                           (char *)&h1) < 0) ) {
    printk(KERN_WARNING "ANAND header found at 0x%x in mtd%d, "
           "but OOB data read failed (err %d)\n",
           block * nftl->EraseSize, nftl->mbd.mtd->index, ret);
    continue;
  ....
}

Предупреждение PVS-Studio: V593 Consider reviewing the expression of the 'A = B

Если возникнет ошибка, функция должна распечатать о ней информацию. В том числе должен быть распечатан код ошибки. Но, на самом деле, будет распечатано (err 0) или (err 1), а не настоящий код ошибки.

Причина — программист запутался в приоритетах операции. В начале он хотел поместить результат работы функции nftl_read_oob() в переменную 'ret'. Затем он хочет сравнить эту переменную с 0. Если (ret < 0), то нужно распечатать сообщение об ошибке.

На самом деле, все работает не так. В начале результат работы функции nftl_read_oob() сравнивается с 0. Результатом сравнения является значение 0 или 1. Этот значение будет помещено в переменную 'ret'.

Таким образом, если функция nftl_read_oob() вернула отрицательное значение, то ret == 1. Будет выведено сообщение, но неправильное.

В условии видно, что используются дополнительные скобки. Неизвестно, были они написаны чтобы подавить предупреждение компилятора о присваивании внутри 'if' или чтобы явно указать последовательность операций. Если второе, то мы имеем дело с опечаткой — закрывающаяся скобка стоит не на своём месте. Правильно будет так:

if ((ret = nftl_read_oob(mtd, block * nftl->EraseSize +
                         SECTORSIZE + 8, 8, &retlen,
                         (char *)&h1)) < 0 ) {

Подозрение на опечатку

int wl12xx_acx_config_hangover(struct wl1271 *wl)
{
  ....
  acx->recover_time = cpu_to_le32(conf->recover_time);
  acx->hangover_period = conf->hangover_period;
  acx->dynamic_mode = conf->dynamic_mode;
  acx->early_termination_mode = conf->early_termination_mode;
  acx->max_period = conf->max_period;
  acx->min_period = conf->min_period;
  acx->increase_delta = conf->increase_delta;
  acx->decrease_delta = conf->decrease_delta;
  acx->quiet_time = conf->quiet_time;
  acx->increase_time = conf->increase_time;
  acx->window_size = acx->window_size;         <<<---
  ....
}

Предупреждение PVS-Studio: V570 The 'acx->window_size' variable is assigned to itself. acx.c 1728

Везде поля одной структуры копируются в поля другой структуры. И только в одном месте вдруг написано:

acx->window_size = acx->window_size;

Это ошибка? Или так и задумывалось? Я не могу дать ответ.

Подозрительное восьмеричное число

static const struct XGI330_LCDDataDesStruct2  XGI_LVDSNoScalingDesData[] = {
  {0,  648,  448,  405,  96, 2}, /* 00 (320x200,320x400,
                                        640x200,640x400) */
  {0,  648,  448,  355,  96, 2}, /* 01 (320x350,640x350) */
  {0,  648,  448,  405,  96, 2}, /* 02 (360x400,720x400) */
  {0,  648,  448,  355,  96, 2}, /* 03 (720x350) */
  {0,  648,    1,  483,  96, 2}, /* 04 (640x480x60Hz) */
  {0,  840,  627,  600, 128, 4}, /* 05 (800x600x60Hz) */
  {0, 1048,  805,  770, 136, 6}, /* 06 (1024x768x60Hz) */
  {0, 1328,    0, 1025, 112, 3}, /* 07 (1280x1024x60Hz) */
  {0, 1438,    0, 1051, 112, 3}, /* 08 (1400x1050x60Hz)*/
  {0, 1664,    0, 1201, 192, 3}, /* 09 (1600x1200x60Hz) */
  {0, 1328,    0, 0771, 112, 6}  /* 0A (1280x768x60Hz) */
                  ^^^^
                  ^^^^
};

Предупреждение PVS-Studio: V536 Be advised that the utilized constant value is represented by an octal form. Oct: 0771, Dec: 505. vb_table.h 1379

Все числа в этой структуре заданы в десятичном формате. И вдруг встречается одно восьмеричное число: 0771. Анализатор это насторожило. И меня тоже.

Есть подозрение, что этот ноль написан для того, чтобы красиво выровнять столбик. Но тогда это явно некорректное значение.

Подозрительная строка

static void sig_ind(PLCI *plci)
{
  ....
  byte SS_Ind[] =
    "\x05\x02\x00\x02\x00\x00"; /* Hold_Ind struct*/
  byte CF_Ind[] =
    "\x09\x02\x00\x06\x00\x00\x00\x00\x00\x00";
  byte Interr_Err_Ind[] =
    "\x0a\x02\x00\x07\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00";
  byte CONF_Ind[] =
    "\x09\x16\x00\x06\x00\x00\0x00\0x00\0x00\0x00";
                              ^^^^^^^^^^^^^^^^^^^
  ....
}

Предупреждение PVS-Studio: V638 A terminal null is present inside a string. The '\0x00' characters were encountered. Probably meant: '\x00'. message.c 4883

В массивах содержатся некие магические значения. Подозрение вызывает содержимое массива CONF_Ind[]. В нем чередуются нулевые символы с текстом «x00». Мне кажется, это опечатка и, на самом деле, должно быть написано так:

byte CONF_Ind[] =
  "\x09\x16\x00\x06\x00\x00\x00\x00\x00\x00";

Т.е. '0' перед 'x' лишний и написан случайно. В результате «x00» интерпретируется как текст, а не как код символа.

Подозрительное форматирование кода

static int grip_xt_read_packet(....)
{
  ....
  if ((u ^ v) & 1) {
    buf = (buf << 1) | (u >> 1);
    t = strobe;
    i++;
  } else

  if ((((u ^ v) & (v ^ w)) >> 1) & ~(u | v | w) & 1) {
  ....
}

Предупреждение PVS-Studio: V705 It is possible that 'else' block was forgotten or commented out, thus altering the program's operation logics. grip.c 152

Я не думаю, что здесь есть ошибка. Но код отформатирован очень плохо. Поэтому и привожу его в статье. Лучше лишний раз проверить.

Неопределённое поведение в операциях сдвига

static s32 snto32(__u32 value, unsigned n)
{
  switch (n) {
  case 8:  return ((__s8)value);
  case 16: return ((__s16)value);
  case 32: return ((__s32)value);
  }
  return value & (1 << (n - 1)) ? value | (-1 << n) : value;
}

Предупреждение PVS-Studio: V610 Undefined behavior. Check the shift operator '

Сдвиг отрицательных значений приводит к неопределённому поведению. Про это я много раз писал и не буду останавливаться на этом подробно. Тем, кто не знает в чём тут дело, предлагаю для ознакомления статью "Не зная брода, не лезь в воду. Часть третья (про сдвиги)".

Предвижу возражение в духе: «но ведь работает!». Возможно и работает. Но, я думаю, ядро Linux это не то место, где можно полагаться на такой подход. Код лучше переписать.

Таких сдвигов весьма много, поэтому я собрал их в отдельный файл: Linux-V610.txt.

Путаница с enum

Есть вот такой два enum:

enum iscsi_param {
  ....
  ISCSI_PARAM_CONN_PORT,
  ISCSI_PARAM_CONN_ADDRESS,        <<<<----
  ....
};

enum iscsi_host_param {
  ISCSI_HOST_PARAM_HWADDRESS,
  ISCSI_HOST_PARAM_INITIATOR_NAME,
  ISCSI_HOST_PARAM_NETDEV_NAME,
  ISCSI_HOST_PARAM_IPADDRESS,       <<<<----
  ISCSI_HOST_PARAM_PORT_STATE,
  ISCSI_HOST_PARAM_PORT_SPEED,
  ISCSI_HOST_PARAM_MAX,
};

Обратите внимание на константу ISCSI_PARAM_CONN_ADDRESS и ISCSI_HOST_PARAM_IPADDRESS. Их названия похожи. Видимо, это и стало причиной путаницы.

Рассмотрим фрагмент кода:

int iscsi_conn_get_addr_param(
  struct sockaddr_storage *addr,
  enum iscsi_param param, char *buf)
{
  ....
  switch (param) {
  case ISCSI_PARAM_CONN_ADDRESS:
  case ISCSI_HOST_PARAM_IPADDRESS:        <<<<----
  ....
  case ISCSI_PARAM_CONN_PORT:
  case ISCSI_PARAM_LOCAL_PORT:
  ....
  default:
    return -EINVAL;
  }

  return len;
}

Предупреждение PVS-Studio: V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. libiscsi.c 3501

Константа ISCSI_HOST_PARAM_IPADDRESS не относится к enum iscsi_param. Скорее всего это опечатка, и должна использоваться константа ISCSI_PARAM_CONN_ADDRESS.

Аналогичные предупреждения PVS-Studio:

• V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. svm.c 1360


• V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. vmx.c 2690


• V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. request.c 2842


• V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. request.c 2868

Странный цикл

Здесь я не смогу показать фрагмент кода. Фрагмент большой, и я не знаю, как его сократить и красиво отформатировать. Поэтому я напишу псевдокод.

void pvr2_encoder_cmd ()
{
  do {
    ....
    if (A) break;
    ....
    if (B) break;
    ....
    if (C) continue;
    ....
    if (E) break;
    ....
  } while(0);
}

Цикл выполняется 1 раз. Есть подозрение, что такая конструкция создана для того, чтобы обойтись без оператора goto. Если что-то пошло не так, вызывается оператор 'break', и начинают выполняться операторы, расположенные после цикла.

Меня смущает, что в одном месте написан оператор 'continue', а не 'break'. При этом работает оператор 'continue', как 'break'. Поясню.

Вот что говорит стандарт:

§6.6.2 in the standard: «The continue statement (...) causes control to pass to the loop-continuation portion of the smallest enclosing iteration-statement, that is, to the end of the loop.» (Not to the beginning.)

Таким образом, после вызова оператора 'continue' будет проверено условие (0), и цикл завершится так как условие ложно.

Возможно 2 варианта.

1. Код корректен. Оператор 'continue' должен прерывать цикл. Тогда я рекомендую заменить его для единообразия на 'break', чтобы он путал разработчиков, которые будут работать с этим кодом в дальнейшем.


2. Оператор 'continue' должен по задумке программиста возобновлять цикл. Тогда код некорректен и его следует переписать.

Copy-Paste ошибка

void dm_change_dynamic_initgain_thresh(
  struct net_device *dev, u32 dm_type, u32 dm_value)
{
  ....
  if (dm_type == DIG_TYPE_THRESH_HIGH)
  {
    dm_digtable.rssi_high_thresh = dm_value;
  }
  else if (dm_type == DIG_TYPE_THRESH_LOW)
  {
    dm_digtable.rssi_low_thresh = dm_value;
  }
  else if (dm_type == DIG_TYPE_THRESH_HIGHPWR_HIGH)      <<--
  {                                                      <<--
    dm_digtable.rssi_high_power_highthresh = dm_value;   <<--
  }                                                      <<--
  else if (dm_type == DIG_TYPE_THRESH_HIGHPWR_HIGH)      <<--
  {                                                      <<--
    dm_digtable.rssi_high_power_highthresh = dm_value;   <<--
  }                                                      <<--
  ....
}

Предупреждение PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1755, 1759. r8192U_dm.c 1755

Код писался с помощью Copy-Paste и в одном блоке текста забыли заменить:

• DIG_TYPE_THRESH_HIGHPWR_HIGH на DIG_TYPE_THRESH_HIGHPWR_LOW


• rssi_high_power_highthresh на rssi_high_power_lowthresh

Плюс прошу разработчиков посмотреть сюда:

• V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1670, 1672. rtl_dm.c 1670


• V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 530, 533. ioctl.c 530

Повторная инициализация

Есть странные места, где переменной два раза подряд присваивают разные значения. Думаю, стоит проверить эти участки кода.

static int saa7164_vbi_fmt(struct file *file, void *priv,
                           struct v4l2_format *f)
{
  /* ntsc */
  f->fmt.vbi.samples_per_line = 1600;           <<<<----
  f->fmt.vbi.samples_per_line = 1440;           <<<<----
  f->fmt.vbi.sampling_rate = 27000000;
  f->fmt.vbi.sample_format = V4L2_PIX_FMT_GREY;
  f->fmt.vbi.offset = 0;
  f->fmt.vbi.flags = 0;
  f->fmt.vbi.start[0] = 10;
  f->fmt.vbi.count[0] = 18;
  f->fmt.vbi.start[1] = 263 + 10 + 1;
  f->fmt.vbi.count[1] = 18;
  return 0;
}

Предупреждение PVS-Studio: V519 The 'f->fmt.vbi.samples_per_line' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1001, 1002. saa7164-vbi.c 1002

static int saa7164_vbi_buffers_alloc(struct saa7164_port *port)
{
  ....
  /* Init and establish defaults */
  params->samplesperline = 1440;
  params->numberoflines = 12;                           <<<<----
  params->numberoflines = 18;                           <<<<----
  params->pitch = 1600;                                 <<<<----
  params->pitch = 1440;                                 <<<<----
  params->numpagetables = 2 +
    ((params->numberoflines * params->pitch) / PAGE_SIZE);
  params->bitspersample = 8;
   ....
}

Предупреждения:

• V519 The 'params->numberoflines' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 118, 119. saa7164-vbi.c 119


• V519 The 'params->pitch' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 120, 121. saa7164-vbi.c 121

Заключение

В любом большом проекте можно найти какие-то ошибки. Ядро Linux не стало исключением. Однако, разовые проверки кода статическим анализатором является неправильным способом его применения. Да, они позволяют написать вот такую рекламную статью, но приносят мало пользы проекту.

Используйте статический анализ регулярно, и вы сэкономите массу времени, обнаруживая ряд ошибок на самом раннем этапе их возникновения. Защите свой проект от багов с помощью статического анализатора!

Предлагаю всем желающим попробовать PVS-Studio на своих проектах. Анализатор работает в среде Windows. Если кто-то хочет использовать его при разработке крупных Linux приложений, то напишите нам, и мы обсудим возможные варианты заключения контракта по адаптации PVS-Studio для ваших проектов и задач.

Эта статья на английском

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. PVS-Studio dives into Linux insides (3.18.1).

This entry passed through the Full-Text RSS service - if this is your content and you're reading it on someone else's site, please read the FAQ at http://ift.tt/jcXqJW.