Модераторы: xvr
  

Поиск:

Ответ в темуСоздание новой темы Создание опроса
> В ядре FreeBSD выявлено как минимум 40 ошибок с по, В ядре FreeBSD выявлено как минимум 40 о 
:(
    Опции темы
Alticor
Дата 17.2.2016, 21:31 (ссылка) | (нет голосов) Загрузка ... Загрузка ... Быстрая цитата Цитата


Новичок



Профиль
Группа: Участник
Сообщений: 6
Регистрация: 15.12.2015

Репутация: нет
Всего: нет



user posted image


Около года назад мы смогли проверить ядро 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. Но для разработчиков, которые хотят интегрировать анализатор в свой проект, обычно не возникает никаких проблем с этим. Они пользуются каким-нибудь способом интеграции, описанным в документации. Преимущество утилит мониторинга в том, что они позволяют быстро попробовать анализатор, если у проекта нестандартная сборочная система.


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

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

        

user posted image


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

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


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

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


user posted image


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", и далее в программе используется неверно вычисленный размер.


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

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


user posted image


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' никогда не получает управления. Для доказательства ошибки в этом и следующих примерах я буду приводить таблицу истинности для спорных логических выражений. Пример для этого случая:


user posted image


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". Скорее всего, что-то тут не так:


user posted image


Ещё три случая:
  • 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". Вот как это выглядит в таблице:


user posted image


В этой главе я привёл целых 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
Продолжение следует...

Это сообщение отредактировал(а) Alticor - 17.2.2016, 21:41
PM MAIL   Вверх
Google
  Дата 23.8.2019, 12:38 (ссылка)  





  Вверх
  
Ответ в темуСоздание новой темы Создание опроса
Правила форума "С/С++: Программирование под Unix/Linux"
xvr
  • Проставьте несколько ключевых слов темы, чтобы её можно было легче найти.
  • Не забывайте пользоваться кнопкой "Код".
  • Вопросы мобильной разработки тут
  • Телепатов на форуме нет! Задавайте чёткий, конкретный и полный вопрос. Указывайте полностью ошибки компилятора и компоновщика.
  • Новое сообщение должно иметь прямое отношение к разделу форума. Флуд, флейм, оффтопик запрещены.
  • Категорически запрещается обсуждение вареза, "кряков", взлома программ и т.д.

Если Вам понравилась атмосфера форума, заходите к нам чаще! С уважением, xvr.

 
 
0 Пользователей читают эту тему (0 Гостей и 0 Скрытых Пользователей)
0 Пользователей:
« Предыдущая тема | C/C++: Программирование под Unix/Linux | Следующая тема »


 




[ Время генерации скрипта: 0.0930 ]   [ Использовано запросов: 22 ]   [ GZIP включён ]


Реклама на сайте     Информационное спонсорство

 
По вопросам размещения рекламы пишите на vladimir(sobaka)vingrad.ru
Отказ от ответственности     Powered by Invision Power Board(R) 1.3 © 2003  IPS, Inc.