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

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

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

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

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

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



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

Для проверки ядра использовался статический анализатор кода 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



Небольшой пример того, как вредно называть переменные коротко и неинформативно. Теперь из-за опечатки в букве '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



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



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

V523 The 'then' statement is equivalent to the 'else' statement. saint.c 2023



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

V523 The 'then' statement is equivalent to the 'else' statement. smsat.c 2848



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

V547 Expression is always true. Probably the '&&' operator should be used here. qla_hw.c 799



Здесь анализатор обнаружил, что условие "(*(tcp_opt + 2) != 0x08) || (*(tcp_opt + 2) != 10)" всегда истинно и это действительно так, если построить таблицу истинности. Но, скорее всего, здесь не нужен оператор '&&', а просто сделали опечатку в смещении адреса. Возможно, код функции должен был быть таким:



V571 Recurring check. This condition was already verified in line 1946. sahw.c 1949



Очень странный код, если его условно упростить, то увидим следующее:



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

Ещё похожее место:

• 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



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



Параметр '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



На первый взгляд в этом фрагменте кода нет ничего подозрительного. Иногда используется значение 'chan', иногда на единицу меньше: 'chan - 1', но посмотрим на определение макроса:



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



Приоритет оператора '?:' ниже побитового ИЛИ '|'. В итоге, в битовых операциях, кроме числовых констант, участвует и результат выражения "(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



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

V547 Expression 'cdb[0] != 0x28 || cdb[0] != 0x2A' is always true. Probably the '&&' operator should be used here. mfi_tbolt.c 1110



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




V590 Consider inspecting the 'error == 0 || error != - 1' expression. The expression is excessive or contains a misprint. nd6.c 2119



Проблема этого фрагмента кода заключается в том, что условное выражение не зависит от результата "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



Тут результат всего условного выражения не зависит от вычисления значения "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



В этой функции портится код ошибки, когда выполняется присваивание в операторе '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