PVS-Studio покопался в ядре FreeBSD

    Около года назад мы смогли проверить ядро Linux. Это была одна из самых обсуждаемых статей о проверке open-source проекта за всё время. Предложения обратить внимание и на FreeBSD тогда активно поступали, но только сейчас появилось достаточно времени, чтобы это сделать.

    О проверяемом проекте


    FreeBSD — это современная операционная система для серверов, десктопов и встроенных компьютерных платформ. Её код прошёл через более чем тридцать лет непрерывного процесса развития, совершенствования и оптимизации. Она хорошо зарекомендовала себя как система для построения интранет и интернет-сетей, и серверов. Она предоставляет надёжные сетевые службы и эффективное управление памятью.

    Несмотря на то, что FreeBSD регулярно проверяется Coverity, я ничуть не жалею, что поработал с этим проектом, т.к. нашёл очень много подозрительных мест. В статье их будет представлено около 40 штук, а для разработчиков (которые получили отчёт проверки ещё до начала написания этой статьи) я подготовил список из ~1000 серьёзных предупреждений анализатора.

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

    Исходный код для проверки был взят с GitHub из ветки 'master'. Репозиторий содержит ~23000 файлов и два десятка конфигураций для сборки под разные платформы, но я проверял только ядро, которое собрал так:

    make buildkernel KERNCONF=MYKERNEL

    Как удалось проверить


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

    Для удобства я установил себе PC-BSD и написал небольшую утилиту на C++, которая сохраняла рабочее окружение запусков компиляторов во время сборки ядра. Полученная информация использовалась для получения препроцессированных файлов и их анализа с помощью PVS-Studio. Такой способ позволил мне быстро проверить проект, не изучая незнакомую мне сборочную систему для интеграции анализатора. А проверка препроцессированных файлов позволяет делать более глубокий анализ кода и находить более сложные и интересные ошибки, например, в макросах. В статье будет приведено несколько таких примеров.

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

    Необычное везение


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



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

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

    Capy-poste и очепятки


    Анализатор PVS-Studio — мощный инструмент статического анализа, который находит самые разные ошибки в коде, но первые диагностики были простыми и делались для поиска самых распространённых ошибок, связанных с опечатками и copy-paste программированием. При просмотре отчёта анализатора, я сортирую его по коду ошибки и обычно начинаю свой рассказ с такого типа диагностических правил.



    V501 There are identical sub-expressions '(uintptr_t) b->handler' to the left and to the right of the '>' operator. ip_fw_sockopt.c 2893
    static int
    compare_sh(const void *_a, const void *_b)
    {
      const struct ipfw_sopt_handler *a, *b;
    
      a = (const struct ipfw_sopt_handler *)_a;
      b = (const struct ipfw_sopt_handler *)_b;
      ....
      if ((uintptr_t)a->handler < (uintptr_t)b->handler)
        return (-1);
      else if ((uintptr_t)b->handler > (uintptr_t)b->handler) // <=
        return (1);
      
      return (0);
    }

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

    V501 There are identical sub-expressions to the left and to the right of the '!=' operator: m->m_pkthdr.len != m->m_pkthdr.len key.c 7208
    int
    key_parse(struct mbuf *m, struct socket *so)
    {
      ....
      if ((m->m_flags & M_PKTHDR) == 0 ||
          m->m_pkthdr.len != m->m_pkthdr.len) { // <=
        ....
        goto senderror;
      }
      ....
    }

    Одно из полей структуры сравнивается само с собой, следовательно, результат этой логической операции всегда будет равен False.

    V501 There are identical sub-expressions to the left and to the right of the '|' operator: PIM_NOBUSRESET | PIM_NOBUSRESET sbp_targ.c 1327
    typedef enum {
      PIM_EXTLUNS      = 0x100,
      PIM_SCANHILO     = 0x80,
      PIM_NOREMOVE     = 0x40,
      PIM_NOINITIATOR  = 0x20,
      PIM_NOBUSRESET   = 0x10, // <=
      PIM_NO_6_BYTE    = 0x08,
      PIM_SEQSCAN      = 0x04,
      PIM_UNMAPPED     = 0x02,
      PIM_NOSCAN       = 0x01
    } pi_miscflag;
    
    static void
    sbp_targ_action1(struct cam_sim *sim, union ccb *ccb)
    {
      ....
      struct ccb_pathinq *cpi = &ccb->cpi;
    
        cpi->version_num = 1; /* XXX??? */
        cpi->hba_inquiry = PI_TAG_ABLE;
        cpi->target_sprt = PIT_PROCESSOR
             | PIT_DISCONNECT
             | PIT_TERM_IO;
        cpi->transport = XPORT_SPI;
        cpi->hba_misc = PIM_NOBUSRESET | PIM_NOBUSRESET; // <=
      ....
    }

    В этом примере в битовой операции участвует одна и та же переменная «PIM_NOBUSRESET», что никак не влияет на результат. Скорее всего тут хотели использовать константу с другим значением, но забыли переименовать переменную.

    V523 The 'then' statement is equivalent to the 'else' statement. saint.c 2023
    GLOBAL void siSMPRespRcvd(....)
    {
      ....
      if (agNULL == frameHandle)
      {
        /* indirect mode */
        /* call back with success */
        (*(ossaSMPCompletedCB_t)(pRequest->completionCB))(agRoot,
           pRequest->pIORequestContext, OSSA_IO_SUCCESS, payloadSize,
           frameHandle);
      }
      else
      {
        /* direct mode */
        /* call back with success */
        (*(ossaSMPCompletedCB_t)(pRequest->completionCB))(agRoot,
           pRequest->pIORequestContext, OSSA_IO_SUCCESS, payloadSize,
           frameHandle);
      }
      ....
    }

    Две ветви условия подписаны разными комментариями: /* indirect mode */ и /* direct mode */, но при этом реализованы одинаковым способом, что очень подозрительно.

    V523 The 'then' statement is equivalent to the 'else' statement. smsat.c 2848
    osGLOBAL void
    smsatInquiryPage89(....)
    {
      ....
      if (oneDeviceData->satDeviceType == SATA_ATA_DEVICE)
      {
        pInquiry[40] = 0x01; /* LBA Low          */
        pInquiry[41] = 0x00; /* LBA Mid          */
        pInquiry[42] = 0x00; /* LBA High         */
        pInquiry[43] = 0x00; /* Device           */
        pInquiry[44] = 0x00; /* LBA Low Exp      */
        pInquiry[45] = 0x00; /* LBA Mid Exp      */
        pInquiry[46] = 0x00; /* LBA High Exp     */
        pInquiry[47] = 0x00; /* Reserved         */
        pInquiry[48] = 0x01; /* Sector Count     */
        pInquiry[49] = 0x00; /* Sector Count Exp */
      }
      else
      {
        pInquiry[40] = 0x01; /* LBA Low          */
        pInquiry[41] = 0x00; /* LBA Mid          */
        pInquiry[42] = 0x00; /* LBA High         */
        pInquiry[43] = 0x00; /* Device           */
        pInquiry[44] = 0x00; /* LBA Low Exp      */
        pInquiry[45] = 0x00; /* LBA Mid Exp      */
        pInquiry[46] = 0x00; /* LBA High Exp     */
        pInquiry[47] = 0x00; /* Reserved         */
        pInquiry[48] = 0x01; /* Sector Count     */
        pInquiry[49] = 0x00; /* Sector Count Exp */
      }
      ....
    }

    Этот пример ещё более подозрительный, чем предыдущей. Скопирован такой большой фрагмент кода, но потом не сделано никаких изменений.

    V547 Expression is always true. Probably the '&&' operator should be used here. qla_hw.c 799
    static int
    qla_tx_tso(qla_host_t *ha, struct mbuf *mp, ....)
    {
      ....
      if ((*tcp_opt != 0x01) || (*(tcp_opt + 1) != 0x01) ||
        (*(tcp_opt + 2) != 0x08) || (*(tcp_opt + 2) != 10)) { // <=
        return -1;
      }
      ....
    }

    Здесь анализатор обнаружил, что условие "(*(tcp_opt + 2) != 0x08) || (*(tcp_opt + 2) != 10)" всегда истинно и это действительно так, если построить таблицу истинности. Но, скорее всего, здесь не нужен оператор '&&', а просто сделали опечатку в смещении адреса. Возможно, код функции должен был быть таким:
    static int
    qla_tx_tso(qla_host_t *ha, struct mbuf *mp, ....)
    {
      ....
      if ((*tcp_opt != 0x01) || (*(tcp_opt + 1) != 0x01) ||
        (*(tcp_opt + 2) != 0x08) || (*(tcp_opt + 3) != 10)) {
        return -1;
      }
      ....
    }

    V571 Recurring check. This condition was already verified in line 1946. sahw.c 1949
    GLOBAL
    bit32 siHDAMode_V(....)
    {
      ....
      if( saRoot->memoryAllocated.agMemory[i].totalLength > biggest)
      {
        if(biggest < saRoot->memoryAllocated.agMemory[i].totalLength)
        {
          save = i;
          biggest = saRoot->memoryAllocated.agMemory[i].totalLength;
        }
      }
      ....
    }

    Очень странный код, если его условно упростить, то увидим следующее:
    if( A > B )
    {
      if (B < A)
      {
        ....
      }
    }

    Два раза подряд проверяется одно и тоже условие. Скорее всего, тут хотели написать другой код.

    Ещё похожее место:
    • V571 Recurring check. This condition was already verified in line 1940. if_rl.c 1941

    Опасные макросы


    V523 The 'then' statement is equivalent to the 'else' statement. agtiapi.c 829
    if (osti_strncmp(buffer, "0x", 2) == 0)
    { 
      maxTargets = osti_strtoul (buffer, &pLastUsedChar, 0);
      AGTIAPI_PRINTK( ".... maxTargets = osti_strtoul  0 \n" );
    }
    else
    {
      maxTargets = osti_strtoul (buffer, &pLastUsedChar, 10);
      AGTIAPI_PRINTK( ".... maxTargets = osti_strtoul 10\n"   );
    }

    Это предупреждение анализатора я сначала пропустил, решив, что это ложное срабатывания. Но после проверки проекта ложные срабатывания надо изучать и улучшать анализатор. Чем я и занялся, после чего встретил такой макрос:
    #define osti_strtoul(nptr, endptr, base)    \
              strtoul((char *)nptr, (char **)endptr, 0)

    Параметр 'base' вообще не используется, а в функцию «strtoul» последним параметром всегда передаётся значение '0'. Хотя в макрос передают значения '0' и '10'. В препроцессированном файле все макросы раскрылись, и код стал одинаковым. Этот макрос используется таким способом несколько десятков раз. Весь список таких мест я отправил разработчикам.

    V733 It is possible that macro expansion resulted in incorrect evaluation order. Check expression: chan — 1 * 20. isp.c 2301
    static void
    isp_fibre_init_2400(ispsoftc_t *isp)
    {
      ....
      if (ISP_CAP_VP0(isp))
        off += ICB2400_VPINFO_PORT_OFF(chan);
      else
        off += ICB2400_VPINFO_PORT_OFF(chan - 1); // <=
      ....
    }

    На первый взгляд в этом фрагменте кода нет ничего подозрительного. Иногда используется значение 'chan', иногда на единицу меньше: 'chan — 1', но посмотрим на определение макроса:
    #define ICB2400_VPOPT_WRITE_SIZE 20
    
    #define  ICB2400_VPINFO_PORT_OFF(chan) \
      (ICB2400_VPINFO_OFF +                \
       sizeof (isp_icb_2400_vpinfo_t) +    \
      (chan * ICB2400_VPOPT_WRITE_SIZE))          // <=

    При передаче в макрос бинарного выражения, там кардинально меняется логика вычислений. Предполагаемое выражение "(chan — 1) * 20" превращается в «chan — 1 *20», т.е. в «chan — 20», и далее в программе используется неверно вычисленный размер.

    О приоритетах операций


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



    V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '|' operator. ata-serverworks.c 166
    ata_serverworks_chipinit(device_t dev)
    {
      ....
      pci_write_config(dev, 0x5a,
               (pci_read_config(dev, 0x5a, 1) & ~0x40) |
               (ctlr->chip->cfg1 == SWKS_100) ? 0x03 : 0x02, 1);
      }
      ....
    }

    Приоритет оператора '?:' ниже побитового ИЛИ '|'. В итоге, в битовых операциях, кроме числовых констант, участвует и результат выражения "(ctlr->chip->cfg1 == SWKS_100)", что неожиданно меняет логику вычислений. Возможно, в этом месте часто получается результат, похожий на правду, поэтому такую ошибку ещё не заметили.

    V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '|' operator. in6.c 1318
    void
    in6_purgeaddr(struct ifaddr *ifa)
    {
      ....
      error = rtinit(&(ia->ia_ifa), RTM_DELETE, ia->ia_flags |
            (ia->ia_dstaddr.sin6_family == AF_INET6) ? RTF_HOST : 0);
      ....
    }

    В другом файле тоже нашлось место с похожей ошибкой с тернарным оператором.

    V547 Expression 'cdb[0] != 0x28 || cdb[0] != 0x2A' is always true. Probably the '&&' operator should be used here. mfi_tbolt.c 1110
    int
    mfi_tbolt_send_frame(struct mfi_softc *sc, struct mfi_command *cm)
    {
      ....
      if (cdb[0] != 0x28 || cdb[0] != 0x2A) {  // <='
        if ((req_desc = mfi_tbolt_build_mpt_cmd(sc, cm)) == NULL) {
          device_printf(sc->mfi_dev, "Mapping from MFI "
              "to MPT Failed \n");
          return 1;
        }
      }
      else
        device_printf(sc->mfi_dev, "DJA NA XXX SYSPDIO\n");
      ....
    }

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



    V590 Consider inspecting the 'error == 0 || error != — 1' expression. The expression is excessive or contains a misprint. nd6.c 2119
    int
    nd6_output_ifp(....)
    {
      ....
      /* Use the SEND socket */
      error = send_sendso_input_hook(m, ifp, SND_OUT,
          ip6len);
      /* -1 == no app on SEND socket */
      if (error == 0 || error != -1)           // <=
          return (error);
      ....
    }

    Проблема этого фрагмента кода заключается в том, что условное выражение не зависит от результата «error == 0». Скорее всего, что-то тут не так:



    Ещё три случая:
    • V590 Consider inspecting the 'error == 0 || error != 35' expression. The expression is excessive or contains a misprint. if_ipw.c 1855
    • V590 Consider inspecting the 'error == 0 || error != 27' expression. The expression is excessive or contains a misprint. if_vmx.c 2747
    • V547 Expression is always true. Probably the '&&' operator should be used here. igmp.c 1939

    V590 Consider inspecting this expression. The expression is excessive or contains a misprint. sig_verify.c 94
    enum uni_ieact {
      UNI_IEACT_CLEAR = 0x00, /* clear call */
      ....
    }
    
    void
    uni_mandate_epref(struct uni *uni, struct uni_ie_epref *epref)
    {
      ....
      maxact = -1;
      FOREACH_ERR(e, uni) {
        if (e->ie == UNI_IE_EPREF)
          continue;
        if (e->act == UNI_IEACT_CLEAR)
          maxact = UNI_IEACT_CLEAR;
        else if (e->act == UNI_IEACT_MSG_REPORT) {
          if (maxact == -1 && maxact != UNI_IEACT_CLEAR)     // <=
            maxact = UNI_IEACT_MSG_REPORT;
        } else if (e->act == UNI_IEACT_MSG_IGNORE) {
          if (maxact == -1)
            maxact = UNI_IEACT_MSG_IGNORE;
        }
      }
      ....
    }

    Тут результат всего условного выражения не зависит от вычисления значения «maxact != UNI_IEACT_CLEAR». Вот как это выглядит в таблице:



    В этой главе я привёл целых 3 способа, как можно ошибиться в, казалось бы, простых формулах. Задумайтесь…

    V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. aacraid.c 2854
    #define EINVAL 22 /* Invalid argument */
    #define EFAULT 14 /* Bad address */
    #define EPERM 1 /* Operation not permitted */
    
    static int
    aac_ioctl_send_raw_srb(struct aac_softc *sc, caddr_t arg)
    {
      ....
      int error, transfer_data = 0;
      ....
      if ((error = copyin((void *)&user_srb->data_len, &fibsize, 
        sizeof (u_int32_t)) != 0)) 
        goto out;
      if (fibsize > (sc->aac_max_fib_size-sizeof(....))) {
        error = EINVAL;
        goto out;
      }
      if ((error = copyin((void *)user_srb, srbcmd, fibsize) != 0)) 
        goto out;
      ....
    out:
      ....
      return(error);
    }

    В этой функции портится код ошибки, когда выполняется присваивание в операторе 'if'. Т.е. в выражении «error = copyin(...) != 0» сначала вычисляется «copyin(...) != 0», а потом результат (значение 0 или 1) записывается в переменную 'error'.

    В документации к функции 'copyin' сказано, что в случае ошибки она возвращает код EFAULT (значение 14), а после такой проверки в код ошибки сохранится результат логической операции, равный '1', а это уже EPERM — совсем другой статус ошибки.

    К сожалению, таких мест много:
    • V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. aacraid.c 2861
    • V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. if_age.c 591
    • V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. if_alc.c 1535
    • V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. if_ale.c 606
    • V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. if_jme.c 807
    • V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. if_msk.c 1626
    • V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. if_stge.c 511
    • V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. hunt_filter.c 973
    • V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. if_smsc.c 1365
    • V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. if_vte.c 431
    • V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. zfs_vfsops.c 498

    Строки




    V541 It is dangerous to print the string 'buffer' into itself. ata-highpoint.c 102
    static int
    ata_highpoint_probe(device_t dev)
    {
      ....
      char buffer[64];
      ....
      strcpy(buffer, "HighPoint ");
      strcat(buffer, idx->text);
      if (idx->cfg1 == HPT_374) {
      if (pci_get_function(dev) == 0)
          strcat(buffer, " (channel 0+1)");
      if (pci_get_function(dev) == 1)
          strcat(buffer, " (channel 2+3)");
      }
      sprintf(buffer, "%s %s controller",
        buffer, ata_mode2str(idx->max_dma));
      ....
    }

    Здесь формируют некую строку в буфере. Потом хотят получить новую строку, сохранив предыдущее значение строки, и добавить к ней ещё два слова. Вроде всё просто.

    Для объяснения, почему здесь будет получен неожиданный результат, я процитирую простой и понятный пример из документации к этой диагностике:
    char s[100] = "test";
    sprintf(s, "N = %d, S = %s", 123, s);

    В результате работы этого кода хочется получить строку:
    N = 123, S = test

    Но на практике в буфере будет сформирована строка:
    N = 123, S = N = 123, S =

    В других ситуациях аналогичный код может привести не только к выводу некорректного текста, но и к аварийному завершению программы. Код может быть исправлен, если использовать для сохранения результата новый буфер. Корректный вариант:
    char s1[100] = "test";
    char s2[100];
    sprintf(s2, "N = %d, S = %s", 123, s1);

    V512 A call of the 'strcpy' function will lead to overflow of the buffer 'p->vendor'. aacraid_cam.c 571
    #define  SID_VENDOR_SIZE   8
      char   vendor[SID_VENDOR_SIZE];
    #define  SID_PRODUCT_SIZE  16
      char   product[SID_PRODUCT_SIZE];
    #define  SID_REVISION_SIZE 4
      char   revision[SID_REVISION_SIZE];
    
    static void
    aac_container_special_command(struct cam_sim *sim, union ccb *ccb,
      u_int8_t *cmdp)
    {
      ....
      /* OEM Vendor defines */
      strcpy(p->vendor,"Adaptec ");          // <=
      strcpy(p->product,"Array           "); // <=
      strcpy(p->revision,"V1.0");            // <=
      ....
    }

    Все три строки здесь заполняются неверно. В массивах нет места для null-терминального символа, из-за чего могут возникать серьёзные проблемы при дальнейшей работе с такими строками. В случае с «p->vendor» и «p->product» можно убрать один пробел. Тогда поместится терминальный ноль, который функция strcpy() добавляет в конец строки. А вот для «p->revision» совсем нет места для символа конца строки, поэтому надо увеличить значение SID_REVISION_SIZE хотя бы на единицу.

    Мне, конечно, сложно судить об этом коде. Возможно, терминальный ноль и не нужен, и всё рассчитано на определенный размер буфера. Тогда неверно выбрана функция strcpy(). В этом случае следовало написать как-то так:
    memcpy(p->vendor,   "Adaptec ",         SID_VENDOR_SIZE);
    memcpy(p->product,  "Array           ", SID_PRODUCT_SIZE);
    memcpy(p->revision, "V1.0",             SID_REVISION_SIZE);

    V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: td->td_name. subr_turnstile.c 1029
    static void
    print_thread(struct thread *td, const char *prefix)
    {
      db_printf("%s%p (tid %d, pid %d, ....", prefix, td, td->td_tid,
          td->td_proc->p_pid, td->td_name[0] != '\0' ? td->td_name :
          td->td_name);
    }

    Подозрительное место. Несмотря на проверку «td->td_name[0] != '\0'», эту строку всё равно выводят на печать.

    Все такие места:
    • V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: td->td_name. subr_turnstile.c 1112
    • V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: td->td_name. subr_turnstile.c 1196

    Операции с памятью


    В этом разделе я расскажу о неправильном использовании следующих функций:
    void bzero(void *b, size_t len);

    Функция bzero() заполняет нулями 'len' байт по указателю 'b'.
    int copyout(const void *kaddr, void *uaddr, size_t len);

    Функция copyout() копирует 'len' байт из 'kaddr' в 'uaddr'.

    V579 The bzero function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. osapi.c 316
    /* Autosense storage */  
    struct scsi_sense_data sense_data;
    
    void
    ostiInitiatorIOCompleted(....)
    {
      ....
      bzero(&csio->sense_data, sizeof(&csio->sense_data));
      ....
    }

    Чтобы обнулить структуру, в функцию bzero() надо передать указатель на структуру и размер обнуляемой памяти в байтах, но тут в функцию передают размер указателя, а не размер структуры.

    Правильный вариант должен выглядеть так:
    bzero(&csio->sense_data, sizeof(csio->sense_data));

    V579 The bzero function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. acpi_package.c 83
    int
    acpi_PkgStr(...., void *dst, ....)
    {
      ....
      bzero(dst, sizeof(dst));
      ....
    }

    В этом примере похожая ситуация: в функцию 'bzero' опять передали размер указателя, а не объекта.

    Правильный вариант должен выглядеть так:
    bzero(dst, sizeof(*dst));

    V579 The copyout function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. if_nxge.c 1498
    int
    xge_ioctl_stats(xge_lldev_t *lldev, struct ifreq *ifreqp)
    {
      ....
      *data = (*data == XGE_SET_BUFFER_MODE_1) ? 'Y':'N';
      if(copyout(data, ifreqp->ifr_data, sizeof(data)) == 0)    // <=
          retValue = 0;
      break;
      ....
    }

    В данном примере копируют память из 'data' в 'ifreqp->ifr_data', при этом размер копируемой памяти равен sizeof(data), т.е. 4 или 8 байт в зависимости от разрядности архитектуры.

    Указатели




    V557 Array overrun is possible. The '2' index is pointing beyond array bound. if_spppsubr.c 4348
    #define AUTHKEYLEN  16
    
    struct sauth {
      u_short  proto;      /* authentication protocol to use */
      u_short  flags;
    #define AUTHFLAG_NOCALLOUT  1  
              /* callouts */
    #define AUTHFLAG_NORECHALLENGE  2  /* do not re-challenge CHAP */
      u_char  name[AUTHNAMELEN];  /* system identification name */
      u_char  secret[AUTHKEYLEN];  /* secret password */
      u_char  challenge[AUTHKEYLEN];  /* random challenge */
    };
    
    static void
    sppp_chap_scr(struct sppp *sp)
    {
      u_long *ch, seed;
      u_char clen;
    
      /* Compute random challenge. */
      ch = (u_long *)sp->myauth.challenge;
      read_random(&seed, sizeof seed);
      ch[0] = seed ^ random();
      ch[1] = seed ^ random();
      ch[2] = seed ^ random(); // <=
      ch[3] = seed ^ random(); // <=
      clen = AUTHKEYLEN;
      ....
    }

    Размер типа 'u_char' — 1 байт в 32-х и 64-х битном приложениях, а размер типа 'u_long' — 4 байта в 32-х битном приложении и 8 байт в 64-х битном приложении. Тогда в 32-х битном приложении при выполнении операции «u_long* ch = (u_long *)sp->myauth.challenge» массив 'ch' будет состоять из 4-х элементов по 4 байта. А в 64-х битном приложении массив 'ch' будет состоять из 2-х элементов по 8 байт. Следовательно, если мы соберём 64-битное ядро, то при обращение к ch[2] и ch[3] происходит выход за границы массива.

    V503 This is a nonsensical comparison: pointer >= 0. geom_vinum_plex.c 173
    gv_plex_offset(...., int *sdno, int growing)
    {
      ....
      *sdno = stripeno % sdcount;
      ....
      KASSERT(sdno >= 0, ("gv_plex_offset: sdno < 0"));
      ....
    }

    Очень интересное место удалось найти с помощью 503-й диагностики. Нет практического смысла проверять, что значение указателя больше или равно 0. Скорее всего, здесь забыли разыменовать указатель «sdno», чтобы сравнить хранимое там значение.

    Ещё два сравнения указателя с нулём:
    • V503 This is a nonsensical comparison: pointer >= 0. geom_vinum_raid5.c 602
    • V503 This is a nonsensical comparison: pointer >= 0. geom_vinum_raid5.c 610

    V522 Dereferencing of the null pointer 'sc' might take place. mrsas.c 4027
    void
    mrsas_aen_handler(struct mrsas_softc *sc)
    {
      ....
      if (!sc) {
        device_printf(sc->mrsas_dev, "invalid instance!\n");
        return;
      }
      if (sc->evt_detail_mem) {
      ....
    }

    Если указатель «sc» нулевой, то выполняется выход из функции. Но тут непонятно, зачем пытаться выполнить разыменование такого указателя «sc->mrsas_dev».

    Список странных мест:
    • V522 Dereferencing of the null pointer 'sc' might take place. mrsas.c 1279
    • V522 Dereferencing of the null pointer 'sc' might take place. tws_cam.c 1066
    • V522 Dereferencing of the null pointer 'sc' might take place. blkfront.c 677
    • V522 Dereferencing of the null pointer 'dev_priv' might take place. radeon_cs.c 153
    • V522 Dereferencing of the null pointer 'ha' might take place. ql_isr.c 728

    V713 The pointer m was utilized in the logical expression before it was verified against nullptr in the same logical expression. ip_fastfwd.c 245
    struct mbuf *
    ip_tryforward(struct mbuf *m)
    {
      ....
      if (pfil_run_hooks(
          &V_inet_pfil_hook, &m, m->m_pkthdr.rcvif, PFIL_IN, NULL) ||
          m == NULL)
        goto drop;
      ....
    }

    Проверка «m == NULL» стоит в неправильном месте. Сначала надо выполнить проверку указателя, а только потом только вызывать функцию pfil_run_hooks().

    Циклы




    V621 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. if_ae.c 1663
    #define  AE_IDLE_TIMEOUT    100
    
    static void
    ae_stop_rxmac(ae_softc_t *sc)
    {
      int i;
      ....
      /*
       * Wait for IDLE state.
       */
      for (i = 0; i < AE_IDLE_TIMEOUT; i--) {  // <=
        val = AE_READ_4(sc, AE_IDLE_REG);
        if ((val & (AE_IDLE_RXMAC | AE_IDLE_DMAWRITE)) == 0)
          break;
        DELAY(100);
      }
      ....
    }

    В исходном коде FreeBSD нашёлся такой интересный и неправильный цикл. Неизвестно зачем, но тут делается декремент счётчика цикла, вместо того, чтобы делать инкремент. Получается, что цикл может выполняться гораздо больше, чем значение AE_IDLE_TIMEOUT, пока не выполнится оператор 'break'.

    Если цикл вовремя не будет остановлен, то произойдёт переполнение знаковой переменной 'i'. Переполнение знаковой переменной является ничем иным, как неопределённым поведением программы. Причем это не абстрактная теоретическая опасность, а вполне реальная. Недавно мой коллега писал статью на эту тему: "Undefined behavior ближе, чем вы думаете".

    Ещё интересный момент. Точно такая же ошибка была обнаружена мной в коде операционной системы Haiku (см. раздел «Предупреждения #17, #18»). Не знаю, кто у кого позаимствовал файл «if_ae.c», но ошибка явно размножается копированием :).

    V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 182, 183. mfi_tbolt.c 183
    mfi_tbolt_adp_reset(struct mfi_softc *sc)
    {
      ....
      for (i=0; i < 10; i++) {
        for (i = 0; i < 10000; i++);
      }
      ....
    }

    Этот небольшой код скорее всего используется для создания задержки, только суммарно тут выполняется 10000 итераций, а не 10*10000, тогда зачем использовать два цикла?

    Я специально привел этот пример, т.к. он является наиболее наглядным, когда использование одной переменной во внешних и вложенных циклах приводит к неожиданным результатам.

    V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 197, 208. linux_vdso.c 208
    void
    __elfN(linux_vdso_reloc)(struct sysentvec *sv, long vdso_adjust)
    {
      ....
      for(i = 0; i < ehdr->e_shnum; i++) {                      // <=
        if (!(shdr[i].sh_flags & SHF_ALLOC))
          continue;
        shdr[i].sh_addr += vdso_adjust;
        if (shdr[i].sh_type != SHT_SYMTAB &&
            shdr[i].sh_type != SHT_DYNSYM)
          continue;
    
        sym = (Elf_Sym *)((caddr_t)ehdr + shdr[i].sh_offset);
        symcnt = shdr[i].sh_size / sizeof(*sym);
    
        for(i = 0; i < symcnt; i++, sym++) {                    // <=
          if (sym->st_shndx == SHN_UNDEF ||
              sym->st_shndx == SHN_ABS)
            continue;
          sym->st_value += vdso_adjust;
        }
      }
      ....
    }

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

    V547 Expression 'j >= 0' is always true. Unsigned type value is always >= 0. safe.c 1596
    static void
    safe_mcopy(struct mbuf *srcm, struct mbuf *dstm, u_int offset)
    {
      u_int j, dlen, slen;                   // <=
      caddr_t dptr, sptr;
    
      /*
       * Advance src and dst to offset.
       */
      j = offset;
      while (j >= 0) {                       // <=
        if (srcm->m_len > j)
          break;
        j -= srcm->m_len;                    // <=
        srcm = srcm->m_next;
        if (srcm == NULL)
          return;
      }
      sptr = mtod(srcm, caddr_t) + j;
      slen = srcm->m_len - j;
    
      j = offset;
      while (j >= 0) {                       // <=
        if (dstm->m_len > j)
          break;
        j -= dstm->m_len;                    // <=
        dstm = dstm->m_next;
        if (dstm == NULL)
          return;
      }
      dptr = mtod(dstm, caddr_t) + j;
      dlen = dstm->m_len - j;
      ....
    }

    В этой функции присутствуют два опасных цикла. Т.к. переменная 'j' (счётчики циклов) имеет беззнаковый тип, то проверка «j >= 0» всегда истинна и циклы являются «вечными». Другая проблема заключается в том, что из этого счётчика постоянно вычитаются значения, следовательно, если будет попытка преодолеть нулевое значение, то переменная 'j' примет максимальное значение типа.

    V711 It is dangerous to create a local variable within a loop with a same name as a variable controlling this loop. powernow.c 733
    static int
    pn_decode_pst(device_t dev)
    {
      ....
      struct pst_header *pst;                                   // <=
      ....
      p = ((uint8_t *) psb) + sizeof(struct psb_header);
      pst = (struct pst_header*) p;
    
      maxpst = 200;
    
      do {
        struct pst_header *pst = (struct pst_header*) p;        // <=
    
        ....
    
        p += sizeof(struct pst_header) + (2 * pst->numpstates);
      } while (cpuid_is_k7(pst->cpuid) && maxpst--);            // <=
      ....
    }

    В теле цикла обнаружено объявление переменной, совпадающей с переменной, используемой для контроля цикла. У меня есть подозрение, что из-за создания локального указателя с таким же именем 'pst', значение внешнего указателя с именем 'pst' не изменяется. Возможно, в условии цикла do....while() всегда проверяется одно и тоже значение «pst->cupid». Разработчикам необходимо перепроверить это место и обязательно дать разные имена переменным.

    Разное


    V569 Truncation of constant value -96. The value range of unsigned char type: [0, 255]. if_rsu.c 1516
    struct ieee80211_rx_stats {
      ....
      uint8_t nf;      /* global NF */
      uint8_t rssi;    /* global RSSI */
      ....
    };
    
    static void
    rsu_event_survey(struct rsu_softc *sc, uint8_t *buf, int len)
    {
      ....
      rxs.rssi = le32toh(bss->rssi) / 2;
      rxs.nf = -96;
      ....
    }

    Очень подозрительно, что беззнаковой переменной «rxs.nf» присваивается отрицательное значение '-96'. В итоге переменная будет иметь значение '160'.

    V729 Function body contains the 'done' label that is not used by any 'goto' statements. zfs_acl.c 2023
    int
    zfs_setacl(znode_t *zp, vsecattr_t *vsecp, ....)
    {
      ....
    top:
      mutex_enter(&zp->z_acl_lock);
      mutex_enter(&zp->z_lock);
      ....
      if (error == ERESTART) {
        dmu_tx_wait(tx);
        dmu_tx_abort(tx);
        goto top;
      }
      ....
    done:                            // <=
      mutex_exit(&zp->z_lock);
      mutex_exit(&zp->z_acl_lock);
    
      return (error);
    }

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

    V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. mac_process.c 352
    static void
    mac_proc_vm_revoke_recurse(struct thread *td, struct ucred *cred,
        struct vm_map *map)
    {
      ....
      if (!mac_mmap_revocation_via_cow) {
        vme->max_protection &= ~VM_PROT_WRITE;
        vme->protection &= ~VM_PROT_WRITE;
      } if ((revokeperms & VM_PROT_READ) == 0)   // <=
        vme->eflags |= MAP_ENTRY_COW |
            MAP_ENTRY_NEEDS_COPY;
      ....
    }

    Напоследок хочу рассказать про подозрительное форматирование, с которым уже столкнулся в самом начале проверки проекта. Здесь код оформлен таким образом, что отсутствие ключевого слова 'else' выглядит подозрительным.

    V705 It is possible that 'else' block was forgotten or commented out, thus altering the program's operation logics. scsi_da.c 3231
    static void
    dadone(struct cam_periph *periph, union ccb *done_ccb)
    {
      ....
      /*
       * If we tried READ CAPACITY(16) and failed,
       * fallback to READ CAPACITY(10).
       */
      if ((state == DA_CCB_PROBE_RC16) &&
        ....
      } else                                                    // <=
      /*
       * Attach to anything that claims to be a
       * direct access or optical disk device,
       * as long as it doesn't return a "Logical
       * unit not supported" (0x25) error.
       */
      if ((have_sense) && (asc != 0x25)                         // <=
        ....
      } else { 
        ....
      }
      ....
    }

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

    Заключение




    Проект FreeBSD проверялся специальной версией PVS-Studio, которая показала отличный результат! Весь полученный материал невозможно было уместить в одной этой статье. Тем не менее, команда разработчиков FreeBSD получила весь список предупреждений анализатора, на которые стоит обратить внимание.

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


    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. PVS-Studio delved into the FreeBSD kernel.

    Прочитали статью и есть вопрос?
    Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio, версия 2015. Пожалуйста, ознакомьтесь со списком.
    PVS-Studio
    389.56
    Static Code Analysis for C, C++, C# and Java
    Share post

    Similar posts

    Comments 65

      +25
      "Обратите внимание на выделенный фрагмент. Для форматирования отступов используется символ табуляции и под условие сдвинули два оператора."
      Когда я участвовал в проекте, связанным с поддержкой своей версии ядра FreeBSD, руководство требовало соблюдения его codingstyle, запрещающего фигурные скобки вокруг единственного оператора. И такая ошибка была очень частой, не только у меня, но и у значительно более опытных разработчиков. Я пытался бороться, но мои коммиты за это заворачивали на code review.
        +29
        Имхо, плохая практика. Всегда при составлении codingstyle настаиваю на обязательном включении в фигурные скобки единственного оператора.
          –28
          А я не вижу смысл писать 3 строки (или даже 4, если кодегайд требует переноса открывающей скобки на новую строку), вместо 2. Просто следить нужно за тем, что пишешь… Тут вон недавно был пост, в котором и вовсе break говорили не использовать, а вызов безымянной функции =\ Меня такие вещи возмущают — почему если кто-то неаакуратен, то ограничивать себя должна вся команда?
            +13
            Проблема в том, что не все следят. Я пришёл к выводу о необходимости такого правила, когда обноружил баг в модуле, который на прошлом тестировании работал, вызванный вот такими строками:

            if (zoom == 1)
            // log(items);

            self.redraw();

            А вся команда должна себя ограничивать потому, во-первых, любой может быть невнимателен, а во-вторых, потом вся команда будет искать этот баг
              0
              Радует, что GCC6 будет ловить (-Wmisleading-indentation) хотя бы часть подобных косяков.
                0
                Да, полезно. Кстати в PVS-Studio тоже давно есть подобная диагностика V628. Анализаторы всегда на шаг впереди компиляторов в этом плане. :)
                  +1
                  Описанное правило не поймает приведённый мной пример: здесь нет 2го if.

                  Кстати, я когда-то писал вам с предложением ввести правило для такого случая, можете перепроверить личку :)
              +16
              Потому что две секунды экономии времени одного невнимательного выливаются двумя днями потерянного времени всей команды на отлов взявшегося "из ниоткуда" загадочного бага.

              "Уставы пишутся кровью." Требовать, чтобы это была именно ваша кровь, достаточно неумно.
                –1
                Может быть, лучше использовать анализатор, подобный PVS, чем писать странные конструкции?
                Нет, я согласен, это хорошая практика, чтобы не наделать ошибок на "ровном месте", "без ничего". Если мы текст набираем в блокноте, собираем из командной строки. Такие конструкции заметны и в распечатке. Но если есть возможность и не писать лишнего, и проверить на ошибки — почему бы и нет?
                  +6
                  Как говорится, "чисто не там, где подметают, а там, где не сорят" ©

                  А вообще:

                    –1
                    Как говорится, «чисто не там, где подметают, а там, где не сорят»

                    Слышал наоборот: чисто не там, где не сорят, а там, где убирают. Эту фразу крутят, как хотят.

                    А вообще:

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

                    Ну, мы же про С++ конкретно. Стоит, да, но такой подход тоже имеет право на жизнь — нормальный код и анализатор.
                      +3
                      Слышал наоборот: чисто не там, где не сорят, а там, где убирают.
                      Ну вообще-то миф о самозарождении мусора мышей в грязном белье был развенчан уже давно. Как кто-то правильно отметил, экстремумы функции следует в первую очередь искать на границах её области определения. В частности, возьмём Луну: количество сорящих = 0, количество мусора = 0. Следовательно, моя гипотеза верна, потому что по Вашей (количество подметающих = 0) на Луне должна быть нехилая куча мусора.
                      Ну, мы же про С++ конкретно.
                      Не знаю насчёт «мы», лично я нигде слова «C++» не упомянал. Я, скажем, работаю c Ruby, но там точно так же — один неаккуратный чудак может испортить неделю всей команде.
                      Стоит, да, но такой подход тоже имеет право на жизнь — нормальный код и анализатор.
                      Конечно. Они ни в коей мере не являются взаимоисключающими.
                        0
                        Ну вообще-то миф о самозарождении мусора мышей в грязном белье был развенчан уже давно

                        Никто не говорит о "самозарождении мусора". Речь о том, что чисто будет в обеих ситуациях — и если не сорят (ваше), и если сорят, но убирают (мое высказывание).

                        Следовательно, моя гипотеза верна, потому что по Вашей (количество подметающих = 0) на Луне должна быть нехилая куча мусора.

                        Нет, это софистика: когда речь идет об уборке ("… там, где убирают"), подразумевается, что мусор есть до начала уборки — иначе убирать не надо. Ваше "количество подметающих=0" неверно, должно быть "количество подметающих имеющийся мусор=0". А так как на Луне нет субъектов, нет мусора, то это некорректный пример.

                        Не знаю насчёт «мы», лично я нигде слова «C++» не упомянал

                        Вся статься — об анализаторе для С++ ©. Ветка, где мы общаемся, начинается с реплики по поводу куска статьи, который говорит об ошибке, связанной с coding style в С проекте.
                          +2
                          То, что "уставы coding-style guidelines пишутся кровью", верно для любого языка программирования. Не вижу необходимости искуственно сужать контекст.
                +1
                Вернуться на хабр без кармы из-за одного комментария все же больнее, чем найти место без открывающейся скобки =\
                  0
                  Просто. Следить. Ага. "Да и вообще, зачем вам тестеры? Вы же профессионалы и не делаете ошибок в коде?"
                  +4
                  И я. Хоть меня и не всегда слушают, не умею я людей убеждать.
                  Радует, что в Rust скобки обязательны, хочется попробовать его в деле...
                    +4
                    Swift от Apple с Вами согласен. Там отсутствие скобок — ошибка...
                      0
                      Любопытно, что в OpenJDK в разных пакетах разный стиль. В java.time, например, к моим патчам требовали фигурные скобки, а в java.util.stream куча кода, где их не ставят. Видимо, общего требования нет.
                        0
                        Возможно стиль зависит от ответственного за ту или иную часть проекта.
                      +1
                      В приведенных выше отрывках кода встречаются однооператорные конструкции, заключенные в фигурные скобки. Так что, видимо, не такая уж это и догма.
                      +1
                      Я не спец в плюсах, но какой смысл в коде задержки:

                      for (i=0; i < 10; i++) {
                      for (i = 0; i < 10000; i++);
                      }

                      пустой цикл разве компилятор не выпилит нафиг?
                        +5
                        Сверху ставят штамп: "собирать только с выключенной оптимизацией" и стараются не дышать ;)

                        ЗЫ еще пишут volatile int i, но в общем тоже практика не фонтан...
                          0
                          Это не совсем пустой цикл, в нём изменяется внешняя переменная 'i'.
                            0
                            При оптимизации должно выпилиться в i=10;
                              0
                              Правда в 10?
                                +3
                                Совсем не 10)))
                                  0
                                  Не правда. 10001 похоже. Невнимательность...
                                    –1
                                    Я вам по секрету скажу, что i будет равно 10000 после чего выйдет из обоих циклов. Тем более, в статье об этом явно написано.
                                      +2
                                      i будет 10001. В статье явно написано, что тут не 10*10000 итераций, а в 10 раз меньше. Т.е. я считал именно итерации цикла, их 10000.
                                        0
                                        Точно, до выхода из первого цикла будет еще увеличение на 1.
                                          0
                                          Мне кажется, что ошибка в миллиарды долларов, это не нулевая ссылка, а цикл for. Сложнее представить вещь, в которой сложнее не ошибиться по невнимательности.
                                            0
                                            Без цикла for его роль занял бы цикл while — а там ошибиться еще проще. К примеру, вовсе забыть об изменении счетчика цикла.

                                            Сам сотню раз так ошибался в таких языках.
                                              0
                                              Поэтому и придумали диапазоны, map, reduce и т.д.
                                                0
                                                Вот только для их адекватного использования нужны замыкания, а для замыканий — либо шаблоны, либо объекты с наследованием, полиморфизмом и виртуальными функциями. Ну или функциональный язык программирования.

                                                Слишком сложная замена для простого цикла. Для такого простого языка как Си без плюсов избавиться от цикла for — невозможно.
                                          0
                                          По крайней мере не я один ошибся)
                                            0
                                            Спать надо больше)
                                  0
                                  int retry = 0, i = 0;
                                  int HostDiag;
                                  
                                  ....
                                  
                                  for (i = 0; i < 10000; i++) ;
                                  
                                  HostDiag = (uint32_t)MFI_READ4(sc, MFI_HDR);
                                  
                                  while (!( HostDiag & DIAG_WRITE_ENABLE)) {
                                    for (i = 0; i < 1000; i++);

                                  Выше по коду ещё несколько таких мест, поэтому я и сделал акцент, что 2 вложенных цикла в результате дадут в 10 раз меньше итераций, чем вроде бы выглядит.
                                  +3
                                  Писать Typo's неправильно. Правильно писать Typos. Апостроф не нужен. Сорри, но режет глаз.
                                    +3
                                    Может это специальаня очепятка :)
                                    +2
                                    Что-то дофига ошибок для freebsd...
                                      0
                                      Микрофикс:
                                      bzero(dst, sizeof(*dst));
                                      
                                      Компилятор обматерит, поскольку dst — void *, нужно знать размер области, куда указываем. Есть шанс, что там лежит указатель, и тогда sizeof(dst) отработает корректно, но тогда лучше писать sizeof(void*) и явно комментировать.
                                        +9
                                        Зная, что PVS найдет дофига багов, уже не столько интересно о них читать, сколько — как отреагировали на них разработчики? Вам вообще фидбек приходит?
                                          0
                                          Разработчики сначала вежливо поговорили, потом кто-то попытался установить ту версию, которая собственно проверялась. По его подсчетам, получилась версия пятилетней давности. Потом его поправили, и выяснилось что версия была все-таки свежая.

                                          https://lists.freebsd.org/pipermail/freebsd-doc/2016-February/026369.html
                                            +1
                                            Я сразу упоминал, что использовал Git репозиторий, но кто-то всё равно попытался найти этот номер ревизии в Subversion. На данный момент уже уточнили этот момент.
                                              0
                                              Я так понимаю, Subversion у них — основной репозиторий, ибо так сложилось исторически. А на гитхабе — копия для широких масс, притом неофициальная (ее даже на оф. сайте нет). Так что определение номера ревизии из SVN — это и правда первое что следовало сделать.

                                              Но вот способ, которым это попытались сделать сначала (подсчет числа коммитов в гите исходя из предположения что каждой ревизии соответствует ровно один коммит)...
                                            0
                                            В этом случае я сразу написал разработчикам ещё до написания статьи и выслал им отчёт анализатора. Нас поблагодарили за помощь. Один разработчик прокомментировал несколько ложных срабатываний с указателями. Объяснив, что на самом деле там не очень тривиальный код и всё будет работать правильно.
                                            +7
                                            Несомненно, PVS молодцы, учитывая объем проекта. Но я бы хотел обратить внимание, как круто ребята рисуют/подбирают картинки к статье! После чтения кода, очень разгружает картинку в голове. Спасибо за интересную статью!
                                              +3
                                              Кому нравятся картинки — напоминаем, что с сайта можно бесплатно скачать кружки, майки, обои с единорогами.
                                                0
                                                "Обои с классическим единорогом 1" дикие цвета, но спасибо!
                                                0
                                                Это в том смысле, что с кода блевать тянет? :)
                                                0
                                                safe.c 1596 — не бесконечный цикл, а просто лишняя проверка.
                                                Там внутри цикла правильные проверки есть для выхода.
                                                  +1

                                                    0
                                                    Было бы здорово если добавили бы поддержку компилятора Texas Instruments (http://processors.wiki.ti.com/index.php/TI_Compiler_Information). Это возможно?
                                                      0
                                                      Спроса нет :-(, к сожалению. Редкий слишком зверь.
                                                        0
                                                        А почему вообще нужна привязка к компилятору? По идее же проводится анализ исходных кодов, проблема как я понимаю только слинковать.
                                                          +1
                                                          Ха-ха… :) Это только кажется, что Си++ один… И не важно, как часто в программах используется всякая экзотика и расширения. Её все равно приходится поддерживать. Он всё равно где-то есть, плюс лезет из системных библиотек и т.п.
                                                            +3
                                                            Но хотя бы просто устраивать охоту за очепятками же можно на любом плюсовом коде? Думаю, что очепяткодетектор много кому пригодился.
                                                      –1
                                                      #sarcasm: Решето!
                                                        +1
                                                        It is possible that macro expansion resulted in incorrect evaluation order. Check expression: chan — 1 * 20.
                                                        Интересно, а в этом случае PVS-Studio как-то сравнивает код до раскрытия макроса и код после? В смысле, она знает, что тут изначально был макрос? Или просто по внешнему виду выражения после препроцессинга догадалась?
                                                          +5
                                                          Знает. Анализатор заглядывает и в раскрытый макрос и в оригинальный исходник. На этом основано существенное уменьшение количества ложных срабатываний.
                                                          0
                                                          Появилась новость на Freebsd.org!

                                                          http://www.freebsd.org/news/newsflash.html#event20160217:02
                                                            0
                                                            Кстати, большие дискуссии развернулись на сайте linux.org.ru и slashdot.org. Правда конструктивного в них не очень много. Тем не менее, возможно кому-то будет интересно почитать:

                                                              +5
                                                              В кои8-то веки зашел на ЛОР. Прочитал пару комментариев… все то же, все те же. Хоть что-то в мире остается неизменным.

                                                              Сказать фразу «конструктивного в них не очень много» по отношению к этому может только очень тактичный человек :)

                                                              Only users with full accounts can post comments. Log in, please.