Время от времени мы повторно анализируем проекты, которые уже проверяли с помощью PVS-Studio. Причин несколько. Например, хочется посмотреть, удалось ли избавиться от определенных ложных срабатываний.  Но самое интересное, это посмотреть, как работают новые диагностические правила и какие ошибки они находят. Очень интересно наблюдать, как в уже казалось бы очищенном проекте находятся всё новые и новые дефекты. Очередным перепроверенным проектом стал Clang.

Clang очень интересный для нас проект. Во-первых, он очень качественный. А значит найти в нём новую ошибку, это уже целое достижение. Во-вторых, на нем очень хорошо видны различные недоделки в PVS-Studio, из-за которых возникают ложные срабатывания.

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

Статический анализ следует применять регулярно, так как:

• В проекте появляется новый код. Если его не проверить сразу, многие ошибки будут исправляться медленно. О них будет сообщать отдел тестирования или клиенты.
• Инструменты учатся выявлять всё новые и новые паттерны ошибок.

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

Предыдущий раз проект Clang мы проверяли приблизительно год назад. За это время у нас появились новые диагностические правила, которые позволили выявить очередные места с подозрительным кодом. Правда, таких мест совсем немного. Это не удивительно, так как проект Clang сам включает в себя статический анализатор  и разрабатывается высококвалифицированными разработчиками.  Странно, что вообще удается найти хоть что-то подозрительное.

Посмотрим, что удалось заметить интересного в коде. В основном подозрительные фрагменты связаны с операторами сдвига.



PVS-Studio: V629 Consider inspecting the '1 << shift' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. dataextractor.cpp 171

Судя по проверке "shift < 64", число 1 может быть сдвинуто влево на [0..63] позиций. Однако этот код может привести к неопределенному поведению (undefined behaviour). Подробнее, почему здесь возможно неопределенное поведение, можно познакомиться в статье "Не зная брода, не лезь в воду. Часть третья". Коварство подобных дефектов заключается в том, что программа может долгое время делать вид, что корректно работает. Сбои могут возникать при смене версий компилятора, при изменении ключей оптимизации и после рефакторинга кода.

Код станет безопасным, если число 1 будет представлено 64-битным беззнаковым типом данных. Тогда его можно будет смело сдвигать на 63 разряда. Безопасный код:



К сожалению, я не соображу, как лучше поступить со знаком минус.

Рассмотрим другой пример, содержащий подозрительную операцию сдвига:



PVS-Studio: V629 Consider inspecting the '1U << (NumBits - 1)' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. bitstreamwriter.h 173

Если аргумент 'NumBits' может быть больше 32, то функция будет работать некорректно. Как и в предыдущем примере при сдвиге '1U' на большие значения, будет возникать неопределенное поведение программы. На практике неопределенное поведение, скорее всего, будет проявлять себя как помещение в переменную 'Threshold' бессмысленных значений.

Безопасный вариант:



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



PVS-Studio: V610 Undefined behavior. Check the shift operator '<<. The left operand '~0L' is negative. bitvector.h 175

Так делать небезопасно. Проект Clang собирается для различных платформ. Нужно быть аккуратным с использованием таких конструкций. Сложно предвидеть, как где-то поведёт себя сдвиг отрицательных чисел. 

Есть и другие потенциально опасные операции сдвига. Они однотипны, поэтому не будем их рассматривать подробнее. Просто перечислю их расположение в коде:

• V610 Undefined behavior. Check the shift operator '<<=. The left operand 'Val' is negative. pointerintpair.h 139
• V610 Undefined behavior. Check the shift operator '<<. The left operand '~0L' is negative. bitvector.h 454
• V610 Undefined behavior. Check the shift operator '<<. The left operand '~0L' is negative. sparsebitvector.h 161
• V610 Undefined behavior. Check the shift operator '<<=. The left operand 'Val' is negative. pointerintpair.h 144
• V610 Undefined behavior. Check the shift operator '<<=. The left operand 'Val' is negative. densemapinfo.h 35
• V610 Undefined behavior. Check the shift operator '<<=. The left operand 'Val' is negative. densemapinfo.h 40
• V629 Consider inspecting the '1U << (NumBits - 1)' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. bitstreamreader.h 362
• V629 Consider inspecting the 'Bit->getValue() << i' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. record.cpp 248

Помимо подозрительных сдвигов, было найдено несколько подозрительных циклов. Дело в том, что они выполняются только один раз.



PVS-Studio: V612 An unconditional 'break' within a loop. objcarc.cpp 2763

Обратите внимание на последний оператор 'break'. Перед ним нет никакого условия, и он всегда останавливает цикл. Таким образом, выполняется только одна итерация цикла.

Аналогичные странные фрагменты кода:

• V612 An unconditional 'break' within a loop. objcarc.cpp 2948
• V612 An unconditional 'break' within a loop. undefinedassignmentchecker.cpp 75
• V612 An unconditional 'break' within a loop. bugreporter.cpp 1095

Заключение

Диагностики V610, V612, V629 являются новыми и поэтому, позволили найти что-то новое и интересное. Если вы проверяли свой проект год назад, это ничего не значит. Вообще ничего. Вы написали новый непроверенный код. В анализаторе появились новые диагностические возможности. И продолжают появляться каждый месяц. Начните использовать статический анализ регулярно, и вы существенно сократите усилия на поиск и устранения многих ошибок.