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

    Linux and PVS-Studio
    Соавтор: Святослав Размыслов 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 < 5) {… } else if (A < 2) {… }. Check lines: 2147, 2162. phy.c 2162

    Второе условие никогда не выполняется. Для наглядности упростим код:
    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 < 5) {… } else if (A < 2) {… }. Check lines: 3858, 3860. scsi_debug.c 3860

    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 < 5) {… } else if (A < 2) {… }. Check lines: 439, 441. ad5933.c 441

    Есть ещё парочка схожих ситуаций, которые не буду приводить, чтобы не делать статью излишне длинной:
    • 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 < 10' of loop is always true. qla3xxx.c 149

    Функция пытается залочить драйвер. Если это не получается, она ждёт 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 < C' kind. The expression is calculated as following: 'A = (B < C)'. nftlmount.c 92

    Если возникнет ошибка, функция должна распечатать о ней информацию. В том числе должен быть распечатан код ошибки. Но, на самом деле, будет распечатано (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 '<<. The left operand '-1' is negative. hid-core.c 1016

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

    Предвижу возражение в духе: «но ведь работает!». Возможно и работает. Но, я думаю, ядро 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 не стало исключением. Однако, разовые проверки кода статическим анализатором является неправильным способом его применения. Да, они позволяют написать вот такую рекламную статью, но приносят мало пользы проекту.

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

    Linux and PVS-Studio

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

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


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

    Прочитали статью и есть вопрос?
    Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio и CppCat, версия 2014. Пожалуйста, ознакомьтесь со списком.
    PVS-Studio 383,23
    Ищем ошибки в C, C++ и C# на Windows и Linux
    Поделиться публикацией
    Комментарии 163
    • +46
      3. Цель статьи — реклама PVS-Studio.

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

      Но ваша статья действительна интересна, даже если не работаешь на С++, все равно интересно видеть узкие места для того что бы в будущем следить за ними.

      Спасибо за статью, побольше бы такой рекламы!
      • +29
        Думаю, в данном случае эта фраза даже лишняя. Давно пора привыкнуть к тому, что статьи про PVS-Studio это реклама.
        • +10
          Не лишняя, не все понимают, судя по комментариям. Регулярно встречаются люди, которые считают, что разработчики им должны что-то, потому что они взяли их проект для проверки.
          • 0
            А, тогда действительно не лишняя.
          • +12
            Я думаю, слово «демонстрация» более наглядно отражает. Реклама — это просто реклама. В рекламе могут быть иначе преподнесены некоторые вещи, либо игрой слов покупателю намеренно завышают ожидания и т.д. А тут — наглядная демонстрация возможностей и результата.

            А вот к чему и правда давно надо привыкнуть — к тому, что найденные проблемы всегда репортятся разработчикам. )) Но порой тяжело себя сдержать, чтобы не спросить…
            • +7
              Если вдаваться в детали, то это стоит воспринимать не как демонстрацию возможностей программы, а скорее как демонстрацию проделанной человеком работы. Потому что проанализировать over9000 предупреждений и отобрать из них значимые — довольно большая работа, как мне кажется. Стоит только гадать, сколько времени на это ушло при анализе ядра Linux.
              • 0
                Зачем гадать, давайте спросим.
                • +3
                  На беглый осмотр предупреждений и выписывание примеров для статьи у меня ушло около 6 часов чистого времени. Точно не засекал.
                  • +1
                    а на получение предупреждений?
                    • 0
                      Дольше. Как и написание статьи
                      Призываю SvyatoslavMC для ответа на вопрос. Получение лога это полностью его заслуга.
                      • 0
                        Сборка ядра в виртуальной машине длилась примерно 5 часов, следовательно, в процессе проверки на каждый процесс комплиятора запускалось по 2 процесса: препроцессирование и анализ. Примерное время было оценено в 1,5-2 раза больше сборки, поэтому всё было запущено на ночь. С утра мы открыли лог в PVS-Sutuio Standalone и начали анализ результатов.
                        • +1
                          Нет, я про время, потраченное до запуска сбора :)
                          • 0
                            а на получение предупреждений?

                            На каждом препроцессированном файле запускался процесс PVS-Studio, и каждый такой процесс писал предупреждения в результирующий лог, один для всех. Если это не есть получение предупреждений, то поясните.
                            • +1
                              Для получения предупреждений надо было написать скрипт, заставить работать систему сборки с ним — я про это время спрашивал.
                              • 0
                                Ну так про это в начале статьи написано.[Resolved]
                                • +1
                                  там не указано время, на это потраченное :)
                                  что именно было сделано написано, а сколько времени ушло?
                                  • 0
                                    Время сбора параметров компиляции = время сборки. Время препроцессирования и анализа каждого файла = ~2*(время сборки). Ни одного скрипта не использовалось.
                                    • +1
                                      ОК, мой мозг взорван.

                                      «Далее была написана небольшая утилита на C++,» — вот это сколько потребовало времени? Включая отладку чтоб работали с ней все билдскрипты.
                                      • +2
                                        Наверное нужен ответ, сколько всего провозилось с получением лога, включая сборку, прикручивание утилиты и т.п. По моим ощущением, на это ушло дня два. Могу ошибаться.
                • 0
                  Зачем выводить «незначимые» предупреждения (и что это вообще такое)?
                  • 0
                    Затем, что в статическом анализе пока что не придумали, как это сделать. Это предупреждения, которые предупреждают об ошибке, которой на самом деле нет. Есть она или нет на данный момент может сказать только человек после некоторого «ручного» анализа.
            • +4
              Спасибо за статью, побольше бы такой рекламы!
              Ну не все согласны. :) Мои статьи временами вызывают жуткий butthurt.
              • 0
                Интересно, почему разработчики того же ядра или другие люди связанные с Linux, свободной, чистокровно открытой платформой не гнушаются Coverity, с помощью которой постоянно проверяет ядро мейнтейнерами? Ведь это чистая воды поприетарщина которая от вас отличается лишь масштабом финансирования, да да, у них гранты для проверки опенсорса, и есть свой сервис которые это делает и выдает только результат. Когда вы пишите достаточно интересные статьи. Я понимаю вам просто денег и соответственно времени нет чтобы рекламироваться в таком масштабе, а делаете это по своему.

                Но я не понимаю этих людей которым выдают результаты с указанием на ошибки, а они это признают рекламой и отказываются от них только из-за этого. Жесть.
                • +5
                  Не уверен, что эти комментаторы с reddit являются разработчиками ядра.
                  • +2
                    Я имел ввиду тех людей на реддите, они наверно тогда вообще не знают о "… не гнушаются Coverity, с помощью которой постоянно проверяет ядро мейнтейнерами...". Борцуны за всеобщую открытость и альтруизм без капли поприетарщины нигде. Святая простота…
                  • 0
                    почему разработчики того же ядра или другие люди связанные с Linux, свободной, чистокровно открытой платформой не гнушаются Coverity, с помощью которой постоянно проверяет ядро мейнтейнерами? Ведь это чистая воды поприетарщина которая от вас отличается лишь масштабом финансирования

                    Coverity отличается от PVS-Studio очень сильно, а не «лишь масштабом финансирования». Coverity пишет предупреждения в стиле «А вот если условие в if (...) в строке 120 будет true, а счетчик i в for (i =… ) в строке 130 будет 7, то в строке 140 в выражении a[i] у вас будет выход за границы массива, потому что массив объявлен в строке 50 как int a[N], а N определен в строке 10 как int N = 5». PVS-Studio так делать не умеет.
                    • –1
                      То что вы написали и есть следствие причины.
                      • +2
                        Ваше описание очень напомнило мне html-лог статического анализатора Clang. Все его собщения выглядят как «если… если… если», довольно не просто анализировать такие результаты, и тем более доказать ошибку, потому что на примере проекта Wine, я видел несколько сотен сообщений, которые связаны с пропуском какой-нибудь логики в операторах «if» или «switch», но раз таких мест реально столько много, то, возможно, не предполагается, что этот код получает управление. По моему опыту общего у Clang и PVS-Studio не нашлось, поэтому статья была крупная и разносторонняя, но с PVS-Studio на много легче. Coverity честно не видел, но приведёный вами пример — один в один Clang.
                        • 0
                          На то, как выглядит Coverity можно посмотреть загуглив «Coverity screenshot» например.

                          image

                          А можно посмотреть официальное демо-видео:
                          .
                    • +5
                      Если Tatyanazaxarova — сотрудник, то такие посты нарушение правил реддита.
                      • +3
                        Наконец-то написали на lkml. Все ждал когда там появится ссылка на статью. Интересно будет почитать комментарии разрабов.

                        lkml.org/lkml/2015/1/3/54
                        • +1
                          Имхо наибольший butthurt вызвало качество перевода (я тут не специалист, на много англоязычного народу жалуется что трудно читать — попробуйте Grammarly или может стоит воспользоваться услугами тех.переводчиков и/или пруфридеров) и нарушение правил Reddit'a (может действительно стоит оформлять статью как sponsoring link в таком случае или не публиковаться на реддите).
                          • +1
                            Согласен. Не увидел там баттхерта, пара человек написала, что их напряг плохой английский и что сложно было читать статью.
                            Еще пара написала, что это должен быть sponsor link.
                            Перевод для русского человека выглядит нормальным, но для англоязычного… Там беда с временем и с порядком слов.

                            Аналог на русском:

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

                            И так далее.
                          • –1
                            «Linux kernel source code was checked and is checked by everything and anything.» — англоговорящему такие обороты как by everything and anything реально голову взрывают ))
                            Не говоря уж о грамматических ошибках типа «there are no possibility».
                            Зря сэкономили на вычитке английского текста!
                          • +2
                            Это нас выдрессировали безвкусной рекламой повсюду. А так — посетите как-нибудь, или посмотрите записи «ночь пожирателей рекламы».
                            • +3
                              Вообще в подобных случаях достаточно написать в начале параграф «Раскрытие принадлежности» («Disclosures of Affiliation»), где явно указать, что авторами статьи являются представители заинтересованной стороны, а целью — демонстрация возможностей программного продукта. Явное указание целей статьи — залог верного восприятия адекватным читателем и подспорье в ответах неадекватному.
                              Слово «реклама» употреблять в таком случае совсем необязательно, т.к. оно несёт негативную коннотацию (в т.ч.: гиперболизация, искажение фактов, эмоциональность в ущерб информативности и пр.).

                              С другой стороны, любое маркетинговое усилие должно учитывать возможные последствия для репутации. Максимизация полезной отдачи для адресной аудитории (FOSS-сообщества в данном случае) от каждой подобной статьи помогает формировать позитивный имидж продукта/компании. Если, к примеру, отправлять подробные багрепорты после каждой статьи и способствовать максимально быстрому исправлению найденных ошибок, ну и, конечно, внимательнее подходить к переводу статьи на иностранные языки, серия статей получит куда более позитивный отклик и, в итоге, больший охват. Не утверждаю, впрочем, что это верная стратегия для всех случаев.
                            • –30
                              Всё впорядке
                              static int sha384_ssse3_final(struct shash_desc *desc, u8 *hash)
                              {
                                u8 D[SHA512_DIGEST_SIZE];
                                ...
                                memset(D, 0, SHA512_DIGEST_SIZE);
                                ...
                              }
                              
                              • +10
                                Поясните.
                                • –26
                                  Ну у вас сообщение — «Опасное использование memset», Которое подразумивает то, что может быть задета память не относящаяся к переменной D. Но в данном случае это не так, размер D такой же как и тот что задается в memset. А то что он больше нужного размера для этой ф-ии это совсем другая история.

                                  P.S. Минусующии, объяснитесь пожалуйста.
                                  • +17
                                    Вы статью точно внимательно прочитали? Там же после этого куска кода написано:
                                    Предупреждение 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

                                    «Опасное использование memset» это лишь общее описание типов ошибок. В заголовке ведь не указано точный тип ошибок, верно?
                                    Или в вашем понимании ошибки memset могут быть лишь связанные с порчей памяти?
                                    • –34
                                      RtlSecureZeroMemory в линуксе?!
                                      • +19
                                        Вы статью точно внимательно прочитали? Там же после этого куска кода написано:
                                        PVS-Studio неуместно рекомендует использовать функцию RtlSecureZeroMemory(), так как ориентирован на Windows. В Linux такой функции, конечно, нет. Но здесь главное предупредить, а подобрать нужную функцию уже не сложно.
                                        • +35
                                          Чукча не читатель…
                                          • –52
                                            Сначала узнайте больше о архитектуре ядра линукса.
                                            • +24
                                              Вы там празднуете еще что ли?
                                        • –28
                                          В линуксе вполне нормально использовать memset в ядре.
                                          • +4
                                            Может вы путаете потоко-безопасность с оптимизациями компилятора?
                                            • +33
                                              Вообще — да, в данном случае — нет. Нужно использовать memzero_explicit().
                                          • +3
                                            Сколько раз уже давал ссылку на свой пост по этому поводу. Прочитайте.

                                            Думаю в ядре имеются, похожие на те что в комменте, макросы или функции реализации подобного функционала. Или вы хотите сказать что это только лишь Windows-specific компиляторов (msvc) трабла?
                                            • +1
                                              Ну и для всех кто заинтересовался данным неприятным моментом с оптимизациями компилятора рекомендую прочитать хорошую статью на эту тему:
                                              «MSC06-C. Beware of compiler optimizations»
                                              www.securecoding.cert.org/confluence/display/seccode/MSC06-C.+Beware+of+compiler+optimizations
                                              п.с. карму подпортили хз почему, ссылку пришлось так вставить.

                                              И убедится в том что это не потенциальная угроза, а вполне себе реальная.
                                              Для затравки оттуда:
                                              Subclause 5.1.2.3 of the C Standard [ISO/IEC 9899:2011] states:

                                              In the abstract machine, all expressions are evaluated as specified by the semantics. An actual implementation need not evaluate part of an expression if it can deduce that its value is not used and that no needed side effects are produced (including any caused by calling a function or accessing a volatile object).

                                              This clause gives compilers the leeway to remove code deemed unused or unneeded when building a program. Although this functionality is usually beneficial, sometimes the compiler removes code that it thinks is not needed but has been added for a specific (often security-related) purpose.
                                        • +19
                                          (Надо на такой случай будет заказать нарисовать картинку facepalm с участием единорога)

                                          Предлагаю прочитать статью.

                                          А вообще это очень хорошо демонстрирует пользу от статического анализа. Не видит человек ошибок, не видит. Правда печалит, даже если явно в статье рассказано про ошибку, всё равно её не видят. Боюсь многие полезные диагностики анализатора списываются как ложные предупреждения. Эх.
                                          • +2
                                            А вы картинки сами рисуете?
                                            • +1
                                              Единорогов и котов нам рисовали на заказ. Ну а мы просто из них коллажи делаем в меру своих способностей.
                                            • +5
                                              Для единорога это уже будет facehoof, в качестве референсных изображений художник может посмотреть на картинке в гугле по запросу facehoof.
                                            • +10
                                              Да не то оно подразумевает!

                                              ПРОЧИТАЙТЕ СТАТЬЮ!

                                              Вкратце: компилятор видит последний memset и может в целях оптимизации его выкинуть.
                                              Значит, в памяти останется шматок дайджеста.
                                              От этого ничего нигде не упадёт.
                                              Но если хакер каким-то образом доберётся до памяти — он может целенаправленно разыскать такой шматок и использовать в своих грязных целях. В этом и состоит опасность!
                                        • +18
                                          Решето!
                                          • +2
                                            Andrey2008, а вы последний мой комментарий здесь видели? Если не сочли нужным отвечать, то ладно, просто вдруг не заметили :-)

                                            Эта статья хорошая. Были интересные ошибки, которых я раньше в ваших статьях не видел.

                                            Интересно, а V519 срабатывает только если данные присваивания идут совсем подряд или между ними может быть другой код? А если там вызов функции, будет сработка? Вроде такого кода:

                                            a->b = fn1();
                                            a->b = fn2();
                                            • +14
                                              У меня цифр под рукой нет. Всё в офисе. Решил, что отвечу после праздников.

                                              По поводу диагностики V519. Она «звучит» в описании так:

                                              /*
                                              Генерирует предупреждение V519.
                                              Одному и тому же объекту дважды подряд присваиваются значения.
                                              Высока вероятность опечатки.
                                              Пример:
                                              x = 1;
                                              x = 2;
                                              Исключения:
                                              1) Объект участвует во втором выражении в правой части:
                                                x = 1;
                                                x = x + Foo();
                                              2) Объект является результатом выражения с участием функции.
                                               (На самом деле отбрасываем больше случаев. Если слева выражение,
                                               которое представляет не унарную операцию разыменования '*' или
                                               не обращение к элементу массива - то мы не анализируем дальше).
                                               GetRef() = 1;
                                               GetRef() = 1;
                                              3) Слева используется инкремент или декремент.
                                               *p++ = 1;
                                               *p++ = 1;
                                              4) Для индексации к элементам используется неконстантное выражение.
                                               int i;
                                               p[i] = 1; 
                                               Foo(); //Foo может менять i.
                                               p[i] = 2;
                                              5) Не ругаться, если первый раз переменной типа указатель (или класс) присваивается 0.
                                                 p = nullptr;
                                                 p = new int[n];
                                              6) Если слева переменная имеет особые исключительные имена. Имена:
                                                 gLibNcFTP_Uses_Me_To_Quiet_Variable_Unused_Warnings (используетсяв макросе LIBNCFTP_USE_VAR)
                                              7) Если в одном блоке более одной ситуации, когда подряд присваивается значение одной и
                                                 той-же переменной, то это безопасно. Пример:
                                                  res = RegQueryValueEx(...);
                                                  res = RegQueryValueEx(...);
                                                  res = RegQueryValueEx(...);
                                              8) Если первая строка это объявление переменной.
                                                 Если в первой строке мы объявляем ссылку:
                                                 int &x = a;
                                                 x = 2;
                                              9) Если первая строка это объявление переменной. И это не Си файл.
                                              10) Если первая строка это объявление переменной.
                                                  Инициализируем литералом/константой 0, -1 или "".
                                                  Возможно, есть приведение к какому-то типу: HRESULT hr = ((HRESULT)0L);
                                              11) Если между двумя присваиваниями упоминается данный объект:
                                                  A = 10;
                                                  x = A;
                                                  A = 20;
                                              12) Если между двумя присваиваниями имеется:
                                                  *case;
                                                  *default;
                                                  *break;
                                                  *continue;
                                                  *goto
                                                  *return
                                                  *delete
                                                  *высказывание ((void)0); Такое бывает, когда исчезает assert().
                                                  *пустое высказывание ';'. Такое бывает, когда исчезает что-то типа VivaAssert().
                                                  *throw
                                              13) Если между двумя присваиваниями имеется вызов неитерабельной функции и при этом объект
                                                  является глобальным (или ссылкой, или статической переменной,
                                                  и мы не разыменовываем указатель или брался адрес этой переменной ). 
                                                  Примеры:
                                                  1) A = B; A = FOO();
                                                  2) A = B; FOO(); A = C;
                                                    Исключение из исключения, случай, когда справа одинаковый вызов функции:
                                                      A = z.F(1);
                                                      A = z.F(1); //Скорее всего это copy-paste.
                                                      В общем Исключение N13 не срабатывет, если 2 строки совпадают.
                                                  3) И ещё отдельно рассматриваем такой случай:
                                                     temp = strchr(hoststr, '/');
                                                     *temp = '\0';
                                                     hostlen = strlen( hoststr );
                                                     *temp = '/';
                                              14) Между двумя присваиваниями есть #if/#else/#endif
                                              15) Всегда безопасно, если первый раз присваиваем -1. 
                                                  При этом между первым и вторым присваиванием есть хоть один вызов функции или оператор new.
                                              16) Создаётся список с помощью выражения вида: t = t->next = ptr; 
                                                  Не ругаемся, если записываем результат присваивания в переменную A, являющуюся левой частью для A->B.
                                              
                                              Уровни:
                                                По умолчанию уровень 2. 
                                                Если первая строка это объявление переменной - уровень 3.
                                                На 3 уровень, если переменная имеет имя: hr, hres, err, result
                                              
                                                Если это массив и строки рядом, то на 1 уровень.
                                              */
                                              • +21
                                                Во, спасибо, это многое проясняет. На мой взгляд этот комментарий даже больше несёт рекламной пользы, чем сама статья. По нему видно, сколько работы проделано за какой-то диагностикой. Бывает сравнительно легко написать детектор нового вида багов, но потом начинается самая сложная работа — найти идеальный баланс между false-positive и false-negative и расставить приоритеты.

                                                Мне кажется, было бы круто написать цикл статей в другом разрезе — не про конкретный опенсорсный проект со списком багов, а про конкретный вид багов с примерами из разных проектов. Рассказать хотя бы вкратце, откуда взялись наиболее интересные исключения. Конкретно здесь можно рассказать, что такое неитерабельные функции и как вы определяете, может ли Foo менять i. Показать, какие ложные сработки тем не менее остались и какие полезные сработки вы потеряли, введя исключения. Думаю, помимо рекламной пользы написание такой статьи позволит взглянуть на старый детектор свежим взглядом и, возможно, доработать его :-)
                                                • +2
                                                  >как вы определяете, может ли Foo менять i

                                                  Скорее всего — никак. Настолько глубокий анализ кода вряд ли имеет смысл (да и вряд ли возможен в принципе), достаточно просто принять, что «может менять».
                                                  • +5
                                                    Так и есть. Вернее, анализатор что-то пытается понять, но чуть что, сразу склеивает ласты. Ибо лучше считать, что меняет.
                                                • +1
                                                  Респект. Мне больше всего понравился пункт 6. Видно, что разрабы реально хотят пофиксить все репорты. Так держать!

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


                                                Постоянно просматриваю ваши статьи и не раз видел подобное, в большинстве случаев, так же как с выкидыванием проверок на NULL при оптимизации(например), чаще всего это выглядит как оправдание не профессионализма или лени.
                                                Но хотелось написать не об этом. А о том чаще всего ваши замечания, точнее даже явные ошибки/уязвимости в статьях остаются неисправленными. А цитату о «небезопасных сдвигах» привести как пример того что действительно важные проекты должны иметь ввиду и исправлять.

                                                Я вот к чему, возьму за пример таких проектов FFMPEG. Постоянно просматриваю их зеркало репозитория на githube и часто замечаю, в комментарии коммита присутствует строка «CID********», а это заметил пару лет назад. Потом как-то вы в своих статьях упоминали про Coverity, я заинтересовался и понял что мейнтейнеры постоянно проверяют свой проект с помощью этого анализатора, точнее у Coverity есть такой сервис который в итоге выводит список подозрительных мест по типу вашего формата.

                                                Так вот, даже такие мелочи как «небезопасные сдвиги» которые работали правильно, а может быть иногда и нет, в течении 10 и более лет, они стараются исправить и учесть маловероятные варианты. В большинстве случаев на все это указывает Coverity.

                                                Судя по таким исправлениям, предупреждения вашего анализатора и Coverity пересекаются процентов на 80% наверно.
                                                Хотел как-то посмотреть на список возможных замечаний как у вас но у них ничего не нашел.

                                                Кстати, достаточно легко отследить исправления на замечания Coverity отфильтровав коммиты FFMPEG по тексту «CID»(для удобства советую TortoiseGit). Судя по логу они начали исправлять найденные замечания еще с 2006г. Думаю, можно найти несколько интересных идей для новых диагностик.
                                                • +2
                                                  Не могу ответить, насколько мы пересекаемся с Coverity по диагностикам. Сам не знаю и самому интересно.
                                                  Знаю только, что мы явно дешевле. Собственно мы пытаемся занять нишу где-то между Gimpel PC-Lint и Coverity.
                                                  • +4
                                                    А, вот вопросик назрел. Если мы перейдем к вам, можно будет выбросить pc-lint, вы насколько его покрываете? А то придется писать комменты вида

                                                    //lint -e… //V… (не помю вашего формата)

                                                    если этого можно избежать, мы бы с радостью…
                                                    • +4
                                                      Не проводили такое сравнение. Но не думаю, что сильно пересекаемся. PC-Lint во многом направлен на определённые стандарты кодирования и не важно насколько они «совместимы» с современной жизнью. Надо поддержать стандарт кодирования X и они его поддерживают. Например, в PC-Lint есть правило №1063 для выявление кода вида:

                                                      ClassX(const ClassX x) { m_v = x.m_v; }

                                                      Беда — зацикливание на бесконечном вызове конструктора для вызова конструктора. В правильном коде надо передавать аргумент по ссылке: ClassX(const ClassX &x).

                                                      Вроде как полезная диагностика. Однако мы её не поддерживаем, и не собираемся поддерживать. И как следствие никогда не будем пересекаться.

                                                      Всё дело в том, что это диагностика может быть полезна только для древних или убогих компиляторов. Даже VS2005 просто не компилирует этот код, выдавая ошибку:

                                                      error C2652: 'ClassX' : illegal copy constructor: first parameter must not be a 'ClassX'

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

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

                                                      Так что пробуйте и пишите нам. Пообщаемся в почте по разным вопросам.

                                                      По поводу ложных срабатываний хочу ещё обратить внимание на следующее:
                                                      1. Предлагаю познакомиться со статьёй "Работа с ложными срабатываниями в PVS-Studio и CppCat"
                                                      2. Предлагаю познакомиться с "Новый механизм подавления ненужных сообщений анализатора". Можно не исправляя старые ошибки, начать использовать анализатор и искать ошибки только в свежем коде.

                                                • +8
                                                  «Далее была написана небольшая утилита на C++, которая для каждого запущенного процесса компилятора сохраняла командную строку, текущую директорию и переменные окружения.»
                                                  А на bash подменить команду gcc, сохранить все что надо и запустить обычный gcc сильно сложнее было?
                                                  • +1
                                                    Кстати, а проверялось ядро же с дефолтными для Ubuntu настройками конфига? И тут нужно сказать что тогда большая часть вручную подключаемых модулей/вариантов реализации тех или иных технологий просто не участвовали в компиляции… Вот если бы проверили с конфигом где установлено максимально возможное кол-во всего и вся, тогда можно было бы говорить что проверили все ядро. Но насколько я знаю, включить все в конфиге можно лишь вручную. С помощью menuconfig вроде можно включить лишь рендумные настройки, но это не то все равно. Или может есть способ кроме вручную?
                                                    • +1
                                                      Наши проверки достаточно поверхностны (мы не знакомы с проектом) и их целью является написание вот таких статей.

                                                      Использовался графический конфигуратор gconfig или xconfig, где я применил дефолные настройки, иначе, без соответствуюего опыта, там сходу не разберёшься.
                                                    • –37
                                                      В апрстрим баги заслали?
                                                        • –61
                                                          То есть не заслали. Вместо того, чтобы оформить их как баги (то есть помочь сообществу) — вывалили как дамп без структурирования и сопровождающей информации.

                                                          Почему-то мне захотелось влепить статье минус. Реклама-рекламой, пользы сообществу — ноль.
                                                          • +24
                                                            Зато ваш комментарий принес радость и пользу всему миру. То-то Вы потрудились над ним…
                                                            • –50
                                                              Над чем я должен был «трудиться»? Над проприетарной поделкой? А смысл?

                                                              С опенсорсом же я вполне много работаю: bugs.launchpad.net/~george-shuklin
                                                            • +39
                                                              Мы проверили более 200 открытых проектов. В одной только базе на данный момент выписано 1732 предупреждения. Не про все проекты мы писали статьи. Нет смысла писать, если найдено всего 5 ляпов. Но разработчики уведомлены об этих предупреждениях.

                                                              Заморачиваться с каждым предупреждением по отдельности мы не будем. И тем более не будем делать патчи. Это колоссальный объем работ, который нам ничего не даст. Про статью узнают сотни или тысячи людей. А об открытых багах или патчах узнают десятки. При этом предлагается потратить в N раз больше времени. Это нецелесообразно. Простите, но мир жесток. :)
                                                              • +23
                                                                кококо реклама кококо сообщество кококо не оформлено как баги

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

                                                                Меня всегда поражало, как в кококопенсорсных программах продолжают из релиза в релиз перекладывать баги, просто потому, что никто не оформляет. Ну правильно, проще поныть, что никто не оформляет, чем оформить и написать фикс. Сколько там 12309™ фиксили? 4 года, кажется?
                                                                • –45
                                                                  Такое ощущение, что я вам что-то должен. Например, высказывать уважение.
                                                                  • 0
                                                                    Возможно вы удивитесь тому, сколько багов живет от релиза к релизу в кукаркекомерчесских проектах, только потому, что нельзя их вот так просто взять и посмотреть, и потыкать носом в ошибки.
                                                                    • +3
                                                                      Я прекрасно знаю, сколько живут баги в той же макоси, и за любовь исправлять давно известные не в следующем релизе, а «когда-нибудь сделаем» просто их ненавижу.

                                                                      Но в случае с опенсорсом то самое сообщество может уже готовые, найденные баги и их причины записать в багтрекер и в следующем релизе получить исправленную версию. Но нет, у нас же сообщество диванных кукаретиков, нам не надо исправленную версию, нам надо поныть.
                                                                      • 0
                                                                        Смешно читать обвинения в деванизьме от, судя по всему, очередного диванного эксперта. Опенсорс проекты — это не только университетские романтики, пишущие код для удовольствия и для которых нет проблем закрыть баг, состоящий из двух строчек. Серьезные опенсорс проекты имеют свои бюджеты, дедлайны и менеджеров, кроме того, десятки тысяч (или миллионов, в случае с линуксом) строчек кода. И у того же линукса багтрекер имеет сотни тикетов, на которые нужны ресурсы и время. А просто так, самостоятельно внести исправления в код, без предварительного согласования с разработчиком, достаточно проблематично. Разные группы разработчиков закрывают тикеты по-разному, в зависимости от загруженности. Если проект активно развивается и в него добавляется множество фич, то и его багтрекер растет в геометрической прогрессии. Если проект достиг определенного потолка в разработке, то у разработчиков есть больше времени на решение накопившихся багов.
                                                                        • +3
                                                                          Спасибо кэп, но aulandsdalen, судя по всему, писал конкретно о тех людях, не будем показывать пальцем, назовем его «сообщество»(как выше в посте), которые жалуются что автор данной статьи со своей «поприетарной поделкой» не удосужился оформить все найденные потенциальные баги хотя бы как отдельные багрепорты и что это сообщество вместо того чтобы их оформить, фактически скопипастить их с англ. версии статьи и отослать в нужное место в нужном формате.

                                                                          П.С. Сам являюсь этим сообществом, чаще всего отправляю патчи в небольшие опенсорс проекты(с десяток таких наверно), из крупных пару патчей было принято в kernel, ну и в мой любимый Qt.
                                                                          • –1
                                                                            Дело в том, что в «сообществе», а тем более хабросообществе, это является нормальной практикой — отправлять разработчикам баги, дыры и т.п. Ребята похвастались, что нашли ошибки и даже отправили их «вроде туда, я не разобрался». Понятно, что они ничего ни кому не должны, но негативный осадок остался.
                                                                            • +7
                                                                              Оххххх…

                                                                              aulandsdalen:
                                                                              Такое ощущение, что разработчики PVS кому-то что-то должны. Вам указали на ошибки, разобрали их, просто разжевали так, что их понял даже я, хотя к Линуксу отношусь прохладно, а вы, вместо того, чтобы самому дооформить как баги и радоваться, вы просто кукарекаете и так и сидите с этими багами.

                                                                              Andrey2008:
                                                                              Заморачиваться с каждым предупреждением по отдельности мы не будем. И тем более не будем делать патчи. Это колоссальный объем работ, который нам ничего не даст. Про статью узнают сотни или тысячи людей. А об открытых багах или патчах узнают десятки. При этом предлагается потратить в N раз больше времени. Это нецелесообразно. Простите, но мир жесток. :)

                                                                              Теперь вопрос, зачем все это обсуждение если все свелось к этому: (мое видиние данной ситуации)
                                                                              — Вы отправили соответствующие багрепорты в нужное место в нужном формате?
                                                                              — Нет, написали правда один багрепорт на оф. багзиллу…
                                                                              — Ах нет, арррр. Как вы могли в своем подсайте написать отчет о найденых багах в нашем святая святых Linux kernel'e и при этом еще издевательски открыто пишите что это реклама в начале статьи!?
                                                                              — Нууу… у нас просто нет ресурса и времени на это, так как мы посчитали что это обойдется нам достаточно дорого в человеко/часах, а выхлопа практически никакого не будет.
                                                                              — Я вас не уважаю, думаю, надо влепить минус.
                                                                              — Вы сами можете оформить подробно описанные баги, чтобы сэкономить время можете скопировать текст из англ. версии статьи.
                                                                              — Почему Я это должен делать!? А? У меня вон по ссылке и так стопицот вываленых дампов на багтрекер убубнты с указанием версии и где упало.
                                                                              — Почему вы так культурно кукарекаете? Почему тратите время на комментарии если могли бы за это время отослать эти злополучные багрепорты?
                                                                              — Ты, диванный эксперт, хоть знаешь что такое крупный опенсор…

                                                                              Ну разве не так? :) Старый добрый хабр, старый добрый срач.
                                                              • +16
                                                                Я написал сюда: bugzilla.kernel.org. Может это не то место, но я больше не понял куда надо. :)
                                                                • +4
                                                                  А можно точнее ссылку на баг(и)?
                                                                  • +12
                                                                    Ага, нашёл (не все, но, возможно, я плохо искал).

                                                                    Спасибо, вот это полезная вещь для сообщества.
                                                                • –3
                                                                  Я вам страшную тайну открою, но в огромном числе проектов для вычисления смещения члена структуры используется макрос вида

                                                                  #define OFFSETOF(t,m) ((size_t)&((t*)0)->m)

                                                                  И никогда это не считалось криминалом
                                                                  • +2
                                                                    При использовании gcc лучше использовать __builtin_offsetof.
                                                                    • +3
                                                                      В стандарте C99 есть offsetof (из заголовочного файла stddef.h). Лучше всего использовать именно его, а не __builtin_offsetof или ещё что‐то, и только при использовании компиляторов не поддерживающих C99 использовать #define.

                                                                      Кстати, из имеющихся у меня компиляторов gcc и clang используют __builtin_offsetof, pcc и tcc используют ((size_t)&((t*)0)->m) (только используют всё же более приличные имена параметров), ekopath использует и то, и то (я недостаточно знаком с этим компилятором, чтобы сказать, когда реально используется: __builtin_offsetof из /opt/ekopath/lib/5.0.1/include/stddef.h, а когда другой вариант из /opt/ekopath/include/5.0.1/stl/ansi/_cstddef.h, но pathcc -E на простейшую програмку выводит __builtin_offsetof).

                                                                      Короче подключайте stddef.h и используйте offsetof. Разработчики компиляторов лучше знают, как это надо реализовать. __builtin_offsetof в стандарте нет.
                                                                  • +1
                                                                    А это и не криминал. В статье написано немного про другое.
                                                                    • +1
                                                                      Думаю, это немного другое. Здесь работают не с внешним указателем, а я явно с 0. Скорее всего компилятор догадывается зачем такое нужно и сделает работающий код. А вот для переменной он может оптимизировать код и сделать его нерабочим. Но в любом случае лучше так не делать и использовать средства, которые есть в системных заголовочных файлах. Они сделаны с учетом работы компилятора.
                                                                      Вот ещё хороший пример на тему, что вроде как работает, но лучше не надо: this == 0.

                                                                      • +5
                                                                        Спасибо, полезный продукт. А можно тотже «заряд», но уже по ядру FreeBSD?
                                                                        • +3
                                                                          просто по заявлению bsd сообщества, их ядро чутли не идеально (по количеству ошибок на обьем кода), ну как все наверное знают. интересно что скажет ваше приложение.
                                                                          • НЛО прилетело и опубликовало эту надпись здесь
                                                                            • +1
                                                                              Там куча варнингов при сборке шлангом ядра с включёнными варнингами.
                                                                              Позвольте с этим не согласиться. Бо́льшая часть кода FreeBSD собирается с включёнными варнингами и -Werror. Интереса ради пересобрал ядро (r276106), вот пара цифр:

                                                                              $ grep '^cc ' /tmp/buildkernel.log | wc -l
                                                                                  3452
                                                                              $ grep warning: /tmp/buildkernel.log | wc -l
                                                                                    56
                                                                              $ cc -v
                                                                              FreeBSD clang version 3.4.1 (tags/RELEASE_34/dot1-final 208032) 20140512
                                                                              Target: i386-unknown-freebsd11.0
                                                                              Thread model: posix
                                                                              

                                                                              Итого: 56 предупреждений (16 из них в contributed коде типа zfs) на 3452 вызова компилятора, или 1.62%.
                                                                              • НЛО прилетело и опубликовало эту надпись здесь
                                                                        • +6
                                                                          Поправьте, если я ошибаюсь, но в C11 появилась стандартная функция memset_s, которую компилятор не должен выкидывать. Думаю, в новых проектах лучше использовать её.
                                                                          • 0
                                                                            Проверил свой «велосипед» PVS-Studio, нашел несколько косяков, подумал, да, действительно они могли бы быть серьезными уязвимостями, если бы он не был велосипедом написанным для себя на один раз…
                                                                            • +5
                                                                              1) И linux когда-то был велосипедом для себя на один раз.
                                                                              2) Зато вы стали лучше понимать то, что ваш код делает.
                                                                              • 0
                                                                                Иногда возникает соблазн из проекта, написанного «для себя на 1 раз» утянуть что-то в новый реальный проект.
                                                                              • +6
                                                                                Интересно было бы проверить OpenBSD. Вроде они там очень скрупулёзно относятся к качеству кода.
                                                                                • –3
                                                                                  я так понимаю если они неотчищают память где находится хэш, то к ниму можно както добраться? т.е. по сути закладка.
                                                                                  • +1
                                                                                    Помните Heartbleed где атакуюшему возвращались рендумные участки памяти системы с этой уязвимостью, а теперь представьте сколько раз программа в которой компилятор выбросил такую очистку, например вашего пароля от аккаунта, могла оставить в памяти эти данные которые утекли злоумышленнику, собственно на это и расчет был. Конечно были, есть и будут другие способы получения доступа к памяти подверженного какой-либо уязвимость, но этот пример просто идеально тут вписался.

                                                                                    Навряд закладки, вы даже не представляете насколько часто можно увидеть одни и те же ляпы с этим злосчастным мемсетом…
                                                                                    • –2
                                                                                      ну да, согласен. но если брать в масштабе, то можно представить, что для большенства такое «упущение» — это баг, но для «меньшества» (для крупных проектов) — это доступ. т.е. если я не зжог пароль на листке бумаги, то к ниму (както — теоритически) можно получить доступ.
                                                                                  • +3
                                                                                    а расскажите, как вообще у вас дела с false positive и вот этим вот всем?
                                                                                  • +5
                                                                                    Вы, конечно, молодцы, но когда читаешь названия некоторых ошибок/предупреждений, понимаешь, что вам есть куда улучшаться.

                                                                                    Range intersections are possible within conditional expressions.
                                                                                    Что это такое? Почему бы не написать проще: Неиспользуемые ветки условных выражений.

                                                                                    А следующее:
                                                                                    Consider reviewing the expression of the 'A = B < C' kind. The expression is calculated as following: 'A = (B < C)'.
                                                                                    Пишите проще: Возможно неверное вычисление из-за использования приоритетов операций по-умолчанию. 'A = B < C' вычисляется как 'A = (B < C)

                                                                                    The use of 'if (A) {...} else if (A) {...}' pattern was detected.
                                                                                    Тут уже лучше. Но почему бы не написать: Проверяется одинаковое условие в разных ветках составного условного выражения.
                                                                                    • +2
                                                                                      Второй и третий случай в Вашем объяснении мне показался более запутанным, а в первом случае так вообще «пересечение условий» != «неиспользованные ветки».
                                                                                      • +2
                                                                                        Я в FindBugs в аналогичном первому случаю баге написал «Useless condition: it's known that {2} at this point», где {2} — это то, что заведомо истинно во второй ветке. В FindBugs с правильной формулировкой, кстати, сложнее. Было бы понятнее написать «Useless condition: {условие, которое в исходнике} is always {true/false} at this point», но так как это анализатор байткода, он не всегда может достоверно восстановить, что было в исходнике. Может оказаться, что в коде проверка if(x > 2), а в байткоде компилятор решит поменять на x <= 2. В PVS-Studio такой проблемы нет. У нас, кстати, с английским тоже есть проблемы, так что патчи к файлу сообщений от знающих людей принимаются :-)
                                                                                    • +6
                                                                                      Проверьте пожалуйста Pulseaudio, а то они сами не могут.
                                                                                      • 0
                                                                                        Лучше systemd :)
                                                                                        • +5
                                                                                          … удалите из интернета
                                                                                          • 0
                                                                                            Напишите лучше
                                                                                            • +4
                                                                                              не нужно быть поваром, чтобы чувствовать вкус пищи.
                                                                                              • –2
                                                                                                В данном случае ситуацию можно описать, скорее, как «всё говно»
                                                                                          • 0
                                                                                            Как же хорошо что в Gentoo его можно не использовать.
                                                                                          • +3
                                                                                            Просмотрел свеженькую версию прям из git репозитория. Увидел лишь несколько действительно стремных мест. Итак.

                                                                                            1----
                                                                                            «V590» «Consider inspecting this expression. The expression is excessive or contains a misprint.» «sink-input.c» [«1881»]
                                                                                            if ((state == PA_SINK_INPUT_DRAINED || state == PA_SINK_INPUT_RUNNING) &&
                                                                                            !(i->thread_info.state == PA_SINK_INPUT_DRAINED || i->thread_info.state != PA_SINK_INPUT_RUNNING))
                                                                                            pa_atomic_store(&i->thread_info.drained, 1);
                                                                                            Достаточно странная проверка.
                                                                                            В случае когда i->thread_info.state == PA_SINK_INPUT_RUNNING и первое условие(до &&) TRUE, то вторая часть тоже TRUE и тогда установится флаг «drained».
                                                                                            В случае когда i->thread_info.state == PA_SINK_INPUT_DRAINED, вторая часть — FALSE
                                                                                            В случае когда (i->thread_info.state != PA_SINK_INPUT_DRAINED && i->thread_info.state != PA_SINK_INPUT_RUNNING), вторая часть —
                                                                                            FALSE. А кроме этих двух состояний там еще 3.

                                                                                            Фактически вторую часть можно было бы упростить до:
                                                                                            if ((state == PA_SINK_INPUT_DRAINED || state == PA_SINK_INPUT_RUNNING)
                                                                                            && i->thread_info.state == PA_SINK_INPUT_RUNNING)
                                                                                            Но так ли это? Я не знаю.

                                                                                            2----
                                                                                            «V593» «Consider reviewing the expression of the 'A = B < C' kind. The expression is calculated as following: 'A = (B < C)'.» «alsa-util.c» [«446»]
                                                                                            if ((err = snd_pcm_sw_params_current(pcm, swparams) < 0)) {
                                                                                            pa_log_warn(«Unable to determine current swparams: %s\n», pa_alsa_strerror(err));
                                                                                            return err;
                                                                                            }

                                                                                            Ну, тут все ясно, опечатка с позицией скобки. Ниже, кстати код где аналогично проверяется результат функции — корректен.

                                                                                            3----
                                                                                            «V646» «Consider inspecting the application's logic. It's possible that 'else' keyword is missing.» «pacmd.c» [«336»]
                                                                                            if (watch_socket->revents & POLLHUP) {

                                                                                            } if (watch_socket->revents & POLLOUT) {

                                                                                            }

                                                                                            Опечатка. отсутствует «else». Но никак на работу это не повлияет так как флаги POLLHUP и POLLOUT по своей природе являются взаимоисключающие.
                                                                                            linux.die.net/man/3/poll

                                                                                            POLLOUT — Normal data may be written without blocking.
                                                                                            POLLHUP — The device has been disconnected. This event and POLLOUT are mutually-exclusive; a stream can never be writable if a hangup has occurred. However, this event and POLLIN, POLLRDNORM, POLLRDBAND, or POLLPRI are not mutually-exclusive. This flag is only valid in the revents bitmask; it shall be ignored in the events member.

                                                                                            4----
                                                                                            «V556» «The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }.» «module-tunnel.c» [«694»]
                                                                                            Упрощенно:
                                                                                            switch ((pa_source_state_t) state) {
                                                                                            case PA_SOURCE_SUSPENDED:
                                                                                            case PA_SOURCE_IDLE:
                                                                                            case PA_SOURCE_RUNNING:
                                                                                            case PA_SOURCE_UNLINKED:
                                                                                            case PA_SOURCE_INIT:
                                                                                            case PA_SINK_INVALID_STATE:

                                                                                            Смотрим на последнюю строчку — PA_SINK_INVALID_STATE. Из pa_source_state_t enum'a есть аналогичный, который тут должен быть — PA_SOURCE_INVALID_STATE. Но тут все работает как задумано, оба имеют значение "-1". Вот, если вдруг кто-то чего поменяет… Это будет совсем другая история.

                                                                                            5----
                                                                                            «V636» «The expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;.» «module-raop-sink.c» [«385»]
                                                                                            Самое интересное предупреждение.

                                                                                            uint32_t silence_overhead = 0;
                                                                                            pa_memchunk silence_tmp;
                                                                                            pa_memchunk_reset(&silence_tmp);
                                                                                            silence_tmp.memblock = pa_memblock_new(u->core->mempool, 4096);
                                                                                            silence_tmp.length = 4096;
                                                                                            p = pa_memblock_acquire(silence_tmp.memblock);
                                                                                            memset(p, 0, 4096);
                                                                                            pa_memblock_release(silence_tmp.memblock);
                                                                                            pa_raop_client_encode_sample(u->raop, &silence_tmp, &silence);
                                                                                            pa_assert(0 == silence_tmp.length);
                                                                                            silence_overhead = silence_tmp.length — 4096;
                                                                                            silence_ratio = silence_tmp.length / 4096; raop, &silence_tmp, &silence);

                                                                                            где эта длина может измениться. Давайте посмотрим на функцию:
                                                                                            github.com/pulseaudio/pulseaudio/blob/master/src/modules/raop/raop_client.c#L472

                                                                                            Меняется только в одном месте:
                                                                                            raw->length -= 4;

                                                                                            Значит «silence_tmp.length» будет меньше 4096 или равно, но навряд.
                                                                                            Получается «silence_ratio», когда silence_tmp.length < 4096, будет равно 0.

                                                                                            Но это еще не все, давайте взглянем на строчку выше от той на которую указал анализатор:
                                                                                            silence_overhead = silence_tmp.length — 4096;

                                                                                            Ойойой… Тут же переполнение беззнакового типа в случае когда «silence_tmp.length < 4096».
                                                                                            Тут не хватает проверки и соответствующей логики или же в pa_raop_client_encode_sample()
                                                                                            необходимо не отнимать. а прибавлять:
                                                                                            raw->length -= 4; ---> raw->length += 4;

                                                                                            P.S. У кого есть время — оформите пожалуйста на бегтрекере. Времени на это не осталось.
                                                                                            P.P.S. Не ругайте за оформление коммента, теги не работают.
                                                                                            • +3
                                                                                              Блин, что за парсер такой… теги не пашут, зато строки с "< ----" съедает.
                                                                                              В последнем замечании там я написал:

                                                                                              int32_t silence_overhead = 0;
                                                                                              pa_memchunk silence_tmp;
                                                                                              pa_memchunk_reset(&silence_tmp);
                                                                                              silence_tmp.memblock = pa_memblock_new(u->core->mempool, 4096);
                                                                                              silence_tmp.length = 4096;
                                                                                              p = pa_memblock_acquire(silence_tmp.memblock);
                                                                                              memset(p, 0, 4096);
                                                                                              pa_memblock_release(silence_tmp.memblock);
                                                                                              pa_raop_client_encode_sample(u->raop, &silence_tmp, &silence);
                                                                                              pa_assert(0 == silence_tmp.length);
                                                                                              silence_overhead = silence_tmp.length — 4096;
                                                                                              silence_ratio = silence_tmp.length / 4096; < =====

                                                                                              Вот на эту строку указывает анализатор. Значение silence_tmp.length меняется немного выше при вызове функции:
                                                                                              pa_raop_client_encode_sample(u->raop, &silence_tmp, &silence);
                                                                                              • +4
                                                                                                Что за неадекватный сайт. Плюс поставить в карму без публикаций нельзя, а минус можно. И убрать теперь нельзя.
                                                                                            • 0
                                                                                              Опечатка. отсутствует «else».

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

                                                                                              Для 2,4 попробую сделать патч и ещё cppcheck прогнать.
                                                                                              • 0
                                                                                                Ага, увидел некоторые патчи в гите, спасибо. Надеюсь с последним замечанием разберетесь, но тут, скорее всего, нужно просто указать это место разрабам pulseaudio и пусть сами решают что не так.
                                                                                                • 0
                                                                                                  Да, с 1 и 5 попробую ещё повозиться. Если не разберусь, то отпишу им.
                                                                                                  Спасибо за мини-отчёт.
                                                                                              • 0
                                                                                                >Опечатка. отсутствует «else». Но никак на работу это не повлияет так как флаги POLLHUP и POLLOUT по своей природе являются взаимоисключающими.

                                                                                                а вот не факт, только если мы на 100% уверены, что биты этих флагов _никогда_ не будут заданы одновременно, или могут обе ветки сработать. ну а в одном бите(тогда была бы 100% взаимоисключаемость) они лежать не могут, потому что тогда кому-то пришлось бы быть нулем и использовать просто побитовое и не вышло бы
                                                                                                • 0
                                                                                                  Факт, по крайней мере так говорит официальная документация. Иначе, предположим, что вместо POLLOUT был бы один из (POLLIN, POLLRDNORM, POLLRDBAND, POLLPRI). Тогда вполне вероятно что логика может отработать некорректно, то есть отработать и там и там. Судя по остальному коду, оформлении и т.д. той функции, это просто удачное стечение обстоятельств что забыли else как раз там где это не критично.
                                                                                                  • 0
                                                                                                    Да, кстати, пробежался глазами по всему файлу — в аналогичных местах else есть, скорее всего опечатка.
                                                                                                • +1
                                                                                                  Ещё раз спасибо за отчёт. Отправил четвёртый патч в pulseaudio, исправляет проблему 1. Писал о ней в IRC разработчикам, но видимо, ни у кого не нашлось времени. Посмотрел git blame и вернул код, который был раньше:
                                                                                                  -        !(i->thread_info.state == PA_SINK_INPUT_DRAINED || i->thread_info.state != PA_SINK_INPUT_RUNNING))
                                                                                                  +        !(i->thread_info.state == PA_SINK_INPUT_DRAINED || i->thread_info.state == PA_SINK_INPUT_RUNNING))
                                                                                                  

                                                                                                  Выглядит более логично.

                                                                                                  Проблема 5 вроде как неактуальна, потому что написана версия 2 модуля raop.
                                                                                              • 0
                                                                                                Зачесались руки проверить XBMC и Erlang/OTP :)
                                                                                                • +2
                                                                                                  Тем временем по вашим замечаниям уже начали высылать патчи, к примеру:
                                                                                                  lkml.org/lkml/2015/1/4/137 и другие от этого автора.

                                                                                                  Остановился на этом так как у меня возник вопрос.
                                                                                                  С одним из фиксом замечания из статьи в этом патче еще исправлялась похожая проблема, но ее PVS не выловил.
                                                                                                  Вот участок кода с ошибкой.
                                                                                                  github.com/torvalds/linux/blob/master/drivers/scsi/ipr.c#L3993

                                                                                                  Приведу комментарий Торвальдса на этот счет:
                                                                                                  > Especially since one very strange piece of code seems to be written in
                                                                                                  > such a way that a NUL needs to be placed where a NUL is present already.

                                                                                                  Actually, it's worse than that. This:

                                                                                                  > len = snprintf(fname, 99, "%s", buf);
                                                                                                  > — fname[len-1] = '\0';

                                                                                                  is complete garbage, since the return value of snprintf() is not the
                                                                                                  length of the result, but length of what the result *would* have been.

                                                                                                  So if the string doesn't fit in 99 bytes, it will actively corrupt
                                                                                                  some random memory after the string. It's not writing zero to what was
                                                                                                  already zero, it's corrupting memory.

                                                                                                  Так как практически никогда snprintf не использовал полез читать о нем, например тут www.cplusplus.com/reference/cstdio/snprintf/

                                                                                                  «If the resulting string would be longer than n-1 characters, the remaining characters are discarded and not stored, but counted for the value returned by the function.» Собственно то что и написал Торвальдс.

                                                                                                  Ок, действительно плохо получается. Решил проверить что и как, написал пример — ideone.com/6FWa37
                                                                                                  Думаю, объяснять что к чему там не надо. Очень каверзный момент с этим «snprintf».

                                                                                                  Гугл так же выдал пару ссылок о этой функции на российские сайты, я на автомате клацнул, пролистал, ничего нового. Но тут глаз остановился на комментарии из этой статьи…
                                                                                                  demin.ws/blog/russian/2013/01/28/use-snprintf-on-different-platforms/

                                                                                                  Походу, такого рода ошибки можно найти будет где угодно если они даже в ядре нашлись.

                                                                                                  А теперь у меня вопрос для разработчиков PVS, реально ли такие специфические вещи типа как с этим snprintf добавить в анализатор? Так как эта функция так же будет поддерживаться начиная с MSVC 2014
                                                                                                  blogs.msdn.com/b/vcblog/archive/2014/06/03/visual-studio-14-ctp.aspx

                                                                                                  C99 library features: Most of the remaining C99 library features are implemented. snprintf is implemented, the printf and scanf families of functions now support the new C99 format string improvements, the strtod and scanf families of functions now support hexadecimal floating-point and library conformance is better improved by software updates and adjustments.

                                                                                                  П.С. Почему-то не работают теги
                                                                                                  • 0
                                                                                                    Поправочка, msvc 14 она же Жора, она же Гоша, она же msvc 2015…
                                                                                                    • 0
                                                                                                      Спасибо за расследование. Записал себе потом повнимательнее разобраться с этим и подумать, можно ли какую новую диагностику сделать.
                                                                                                    • 0
                                                                                                      А разве вот это вот выражение
                                                                                                      if (module_type == 0 || module_type != PORTS ||
                                                                                                            module_type != MODEM) {
                                                                                                          pr_err("failed to set a type of module");
                                                                                                          return -1;
                                                                                                        }
                                                                                                      

                                                                                                      не always true при PORTS != MODEM? module_type тогда уж точно либо не PORTS, либо не MODEM, и выражение не excessive, а точно неправильное.
                                                                                                      • 0
                                                                                                        Если честно, то я не понял комментарий. Если бы выражение не было подозрительным, я бы его и не выписал. :) Оно странное в разных смыслах.

                                                                                                        • 0
                                                                                                          Я думаю, имелось в виду следующее. Ваша диагностика проанализировав первые два выражения в условии решила, что одно из них лишнее (сравнение с нулем). А если бы взяли еще и третье выражение, то получилось бы, что все оно целиком всегда истинно. Если первое — это просто странно (но не критично, и, наверное, предупреждения такого рода можно отключить), то второе — почти наверняка ошибка.
                                                                                                          • 0
                                                                                                            Очевидно ошибка есть. По логике этого выражения тут два варианта должно быть.
                                                                                                            1. if (module_type == 0 || module_type == PORTS || module_type == MODEM)
                                                                                                            2. или if (module_type == 0 || (module_type != PORTS && module_type != MODEM) )

                                                                                                            По логике кода github.com/torvalds/linux/blob/master/drivers/staging/dgap/dgap.c#L1022
                                                                                                            я пока не могу понять какой вариант подходит.
                                                                                                            • 0
                                                                                                              Мне кажется, верно второе. А сравнение с нулем — наследие предыдущих версий. Но голову на отсечение не дам :)
                                                                                                            • 0
                                                                                                              Да, это именно то, что я имел в виду, спасибо.
                                                                                                              • 0
                                                                                                                Такая диагностика есть. И скорее всего, она выводилась, но я её проглядел или к тому моменту уже выписал V590.
                                                                                                              • 0
                                                                                                                PVS ругается, мол, Consider inspecting the 'module_type == 0 || module_type != 68' expression. The expression is excessive or contains a misprint., тогда как ИМХО куда полезнее и серьёзнее обратить внимание на второй и третий члены выражения, где оба знаки — неравенство.

                                                                                                                Иными словами, module_type == 0 || module_type != PORTS бывает false при module_type == PORTS, а module_type != PORTS || module_type != MODEM не бывает false вообще никогда. То есть, если первые два подвыражения просто подозрительные, то второе и третье прям кричат об ошибке :)

                                                                                                                Ну и я бы на месте PVS скорее на это ругался. PVS, согласно вашим статьям, вполне себе распознаёт и ругается на паттерны вроде if (a != 1 || a != 2), просто, видимо, упомянутый вами ворнинг приводится первым.

                                                                                                                Или это я с утра не выспался сегодня.
                                                                                                                • 0
                                                                                                                  Как я уже ответил выше, скорее всего здесь выводится и V560 (проверить сейчас не могу, да и лень). Нередко бывает, что одна и та же стока диагностируется как подозрительная двумя, а иногда и тремя разными сообщениями. Обычно я оставляю в статье первое сообщение, которое попалось под руку.
                                                                                                            • 0
                                                                                                              Есть ли в PVS проверка инициализации переменных перед их использованием?
                                                                                                              • 0
                                                                                                                Не знаю про PVS-студию, но вообще-то такая проверка есть практически в любом современном компиляторе, разве что не включена по умолчанию. То есть, конечно, PVS тоже может на это ругаться, но мне кажется, нацелен несколько на иной класс ошибок (которые компиляторами обычно не ловятся).
                                                                                                                • +1
                                                                                                                  Встретилось такое: по таймеру вызывается функция
                                                                                                                  void foo(Obj *obj) {
                                                                                                                  if (obj->a > 0) { --obj->a; return; }
                                                                                                                  
                                                                                                                  if (какое-то условие && obj->a <= 0) {
                                                                                                                  obj->a = 300;
                                                                                                                  //другие действия
                                                                                                                  }
                                                                                                                  }
                                                                                                                  

                                                                                                                  при первом вызове подразумевалось, что a == 0, чего не было на самом деле, т.к «a» не была проинициализирована в обьекте. Компилятор VS2012 не ругался (настройки стандартные).