Около года назад мы смогли проверить ядро Linux. Это была одна из самых обсуждаемых статей о проверке open-source проекта за всё время. Предложения обратить внимание и на FreeBSD тогда активно поступали, но только сейчас появилось достаточно времени, чтобы это сделать.
О проверяемом проекте
https://www.freebsd.org/ - это современная операционная система для серверов, десктопов и встроенных компьютерных платформ. Её код прошёл через более чем тридцать лет непрерывного процесса развития, совершенствования и оптимизации. Она хорошо зарекомендовала себя как система для построения интранет и интернет-сетей, и серверов. Она предоставляет надёжные сетевые службы и эффективное управление памятью.
Несмотря на то, что FreeBSD регулярно https://scan.coverity.com/projects/freebsd, я ничуть не жалею, что поработал с этим проектом, т.к. нашёл очень много подозрительных мест. В статье их будет представлено около 40 штук, а для разработчиков (которые получили отчёт проверки ещё до начала написания этой статьи) я подготовил список из ~1000 серьёзных предупреждений анализатора.
На мой взгляд, из выписанных предупреждений анализатора многие места являются настоящими ошибками, но о критичности я ничего не могу сказать, т.к. не являюсь разработчиком операционной системы. Я думаю, это хороший повод для дискуссий и общения с авторами проекта.
Исходный код для проверки был взят с https://github.com/freebsd/freebsd из ветки 'master'. Репозиторий содержит ~23000 файлов и два десятка конфигураций для сборки под разные платформы, но я проверял только ядро, которое собрал так:
| Код |
|
Как удалось проверить
Для проверки ядра использовался статический анализатор кода http://www.viva64.com/ru/pvs-studio/ версии 6.01.
Для удобства я установил себе PC-BSD и написал небольшую утилиту на C++, которая сохраняла рабочее окружение запусков компиляторов во время сборки ядра. Полученная информация использовалась для получения http://www.viva64.com/ru/t/0076/ и их анализа с помощью PVS-Studio. Такой способ позволил мне быстро проверить проект, не изучая незнакомую мне сборочную систему для интеграции анализатора. А проверка препроцессированных файлов позволяет делать более глубокий анализ кода и находить более сложные и интересные ошибки, например, в макросах. В статье будет приведено несколько таких примеров.
Ядро Linux проверялось аналогичным способом, а для пользователей Windows данный режим проверки доступен в утилите http://www.viva64.com/ru/b/0243/, входящей в дистрибутив PVS-Studio. Но для разработчиков, которые хотят интегрировать анализатор в свой проект, обычно не возникает никаких проблем с этим. Они пользуются каким-нибудь способом интеграции, описанным в документации. Преимущество утилит мониторинга в том, что они позволяют быстро попробовать анализатор, если у проекта нестандартная сборочная система.
Необычное везение
Первую возможную ошибку я нашёл ещё до запуска анализатора, даже до того, как собрал ядро, потому что сборку прервала ошибка линковки. Перейдя к файлу, указанному в ошибке, я увидел следующее:
Обратите внимание на выделенный фрагмент. Для форматирования отступов используется символ табуляции и под условие сдвинули два оператора. Но последний оператор на самом деле не относится к условию и будет выполняться всегда. Возможно, здесь забыли добавить фигурные скобки.
К одной статье был комментарий, что мы просто переписываем предупреждения анализатора, но это не так. Перед проверкой проекта надо убедиться, что он корректно компилируется, а после получения отчёта, предупреждения анализатора необходимо изучить/разобрать и разъяснить читателю. Точно такую же работу проделывает команда поддержки пользователей анализатора, отвечая на электронные письма. Не редки случаи, когда пользователи присылают примеры ложных срабатываний, по их мнению, а на деле это оказывается самой настоящей ошибкой.
Capy-poste и очепятки
Анализатор PVS-Studio - мощный инструмент статического анализа, который находит самые разные ошибки в коде, но первые диагностики были простыми и делались для поиска самых распространённых ошибок, связанных с опечатками и copy-paste программированием. При просмотре отчёта анализатора, я сортирую его по коду ошибки и обычно начинаю свой рассказ с такого типа диагностических правил.
http://www.viva64.com/ru/d/0090/ 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 |
Небольшой пример того, как вредно называть переменные коротко и неинформативно. Теперь из-за опечатки в букве 'b', часть условия никогда не выполнится. Таким образом, функция возвращает нулевой статус не всегда по назначению.
http://www.viva64.com/ru/d/0090/ 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 |
Одно из полей структуры сравнивается само с собой, следовательно, результат этой логической операции всегда будет равен False.
http://www.viva64.com/ru/d/0090/ 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_NOBUSRESET", что никак не влияет на результат. Скорее всего тут хотели использовать константу с другим значением, но забыли переименовать переменную.
http://www.viva64.com/ru/d/0112/ The 'then' statement is equivalent to the 'else' statement. saint.c 2023
| Код |
GLOBAL void siSMPRespRcvd(....) |
Две ветви условия подписаны разными комментариями: /* indirect mode */ и /* direct mode */, но при этом реализованы одинаковым способом, что очень подозрительно.
http://www.viva64.com/ru/d/0112/ The 'then' statement is equivalent to the 'else' statement. smsat.c 2848
| Код |
osGLOBAL void |
Этот пример ещё более подозрительный, чем предыдущей. Скопирован такой большой фрагмент кода, но потом не сделано никаких изменений.
http://www.viva64.com/ru/d/0137/ Expression is always true. Probably the '&&' operator should be used here. qla_hw.c 799
| Код |
static int |
Здесь анализатор обнаружил, что условие "(*(tcp_opt + 2) != 0x08) || (*(tcp_opt + 2) != 10)" всегда истинно и это действительно так, если построить таблицу истинности. Но, скорее всего, здесь не нужен оператор '&&', а просто сделали опечатку в смещении адреса. Возможно, код функции должен был быть таким:
| Код |
static int |
http://www.viva64.com/ru/d/0169/ Recurring check. This condition was already verified in line 1946. sahw.c 1949
| Код |
GLOBAL |
Очень странный код, если его условно упростить, то увидим следующее:
| Код |
if( A > B ) |
Два раза подряд проверяется одно и тоже условие. Скорее всего, тут хотели написать другой код.
Ещё похожее место:
- V571 Recurring check. This condition was already verified in line 1940. if_rl.c 1941
Опасные макросы
http://www.viva64.com/ru/d/0112/ The 'then' statement is equivalent to the 'else' statement. agtiapi.c 829
| Код |
if (osti_strncmp(buffer, "0x", 2) == 0) |
Это предупреждение анализатора я сначала пропустил, решив, что это ложное срабатывания. Но после проверки проекта ложные срабатывания надо изучать и улучшать анализатор. Чем я и занялся, после чего встретил такой макрос:
| Код |
#define osti_strtoul(nptr, endptr, base) \ |
Параметр 'base' вообще не используется, а в функцию "strtoul" последним параметром всегда передаётся значение '0'. Хотя в макрос передают значения '0' и '10'. В препроцессированном файле все макросы раскрылись, и код стал одинаковым. Этот макрос используется таким способом несколько десятков раз. Весь список таких мест я отправил разработчикам.
http://www.viva64.com/ru/d/0380 It is possible that macro expansion resulted in incorrect evaluation order. Check expression: chan - 1 * 20. isp.c 2301
| Код |
static void |
На первый взгляд в этом фрагменте кода нет ничего подозрительного. Иногда используется значение 'chan', иногда на единицу меньше: 'chan - 1', но посмотрим на определение макроса:
| Код |
#define ICB2400_VPOPT_WRITE_SIZE 20 |
При передаче в макрос бинарного выражения, там кардинально меняется логика вычислений. Предполагаемое выражение "(chan - 1) * 20" превращается в "chan - 1 *20", т.е. в "chan - 20", и далее в программе используется неверно вычисленный размер.
О приоритетах операций
В этом разделе я расскажу, насколько важно знать приоритеты операций, использовать лишние скобки, если не уверен, и иногда проверять себя с помощью построения таблицы истинности логического выражения.
http://www.viva64.com/ru/d/0091/ 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) |
Приоритет оператора '?:' ниже побитового ИЛИ '|'. В итоге, в битовых операциях, кроме числовых констант, участвует и результат выражения "(ctlr->chip->cfg1 == SWKS_100)", что неожиданно меняет логику вычислений. Возможно, в этом месте часто получается результат, похожий на правду, поэтому такую ошибку ещё не заметили.
http://www.viva64.com/ru/d/0091/ 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 |
В другом файле тоже нашлось место с похожей ошибкой с тернарным оператором.
http://www.viva64.com/ru/d/0137/ Expression 'cdb[0] != 0x28 || cdb[0] != 0x2A' is always true. Probably the '&&' operator should be used here. mfi_tbolt.c 1110
| Код |
int |
Тут первое условное выражение всегда истинно, из-за чего ветвь 'else' никогда не получает управления. Для доказательства ошибки в этом и следующих примерах я буду приводить таблицу истинности для спорных логических выражений. Пример для этого случая:
http://www.viva64.com/ru/d/0194/ Consider inspecting the 'error == 0 || error != - 1' expression. The expression is excessive or contains a misprint. nd6.c 2119
| Код |
int |
Проблема этого фрагмента кода заключается в том, что условное выражение не зависит от результата "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
| Код |
enum uni_ieact { |
Тут результат всего условного выражения не зависит от вычисления значения "maxact != UNI_IEACT_CLEAR". Вот как это выглядит в таблице:
В этой главе я привёл целых 3 способа, как можно ошибиться в, казалось бы, простых формулах. Задумайтесь...
http://www.viva64.com/ru/d/0197/ 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 */ |
В этой функции портится код ошибки, когда выполняется присваивание в операторе '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