Время от времени мы повторно анализируем проекты, которые уже проверяли с помощью PVS-Studio. Причин несколько. Например, хочется посмотреть, удалось ли избавиться от определенных ложных срабатываний. Но самое интересное, это посмотреть, как работают новые диагностические правила и какие ошибки они находят. Очень интересно наблюдать, как в уже казалось бы очищенном проекте находятся всё новые и новые дефекты. Очередным перепроверенным проектом стал Clang.
Clang очень интересный для нас проект. Во-первых, он очень качественный. А значит найти в нём новую ошибку, это уже целое достижение. Во-вторых, на нем очень хорошо видны различные недоделки в PVS-Studio, из-за которых возникают ложные срабатывания.
К сожалению, с момента очередной проверки проекта и написанием этой заметки прошло более месяца. Причина тому мой отпуск. Есть вероятность, что описанный здесь подозрительный код к моменту публикации заметки уже будет исправлен. Но я думаю это не страшно. Главное я смогу напомнить читателям, что статический анализ это инструмент постоянного, а не разового применения.
Статический анализ следует применять регулярно, так как:
Всё это звучит очень просто и даже банально. К сожалению, разработчики ленятся интегрировать статический анализ в процесс разработки. Вновь и вновь приходится подталкивать их к этому полезному шагу.
Предыдущий раз проект Clang мы проверяли приблизительно год назад. За это время у нас появились новые диагностические правила, которые позволили выявить очередные места с подозрительным кодом. Правда, таких мест совсем немного. Это не удивительно, так как проект Clang сам включает в себя статический анализатор и разрабатывается высококвалифицированными разработчиками. Странно, что вообще удается найти хоть что-то подозрительное.
Посмотрим, что удалось заметить интересного в коде. В основном подозрительные фрагменты связаны с операторами сдвига.
int64_t DataExtractor::getSLEB128(....) const {
int64_t result = 0;
...
// Sign bit of byte is 2nd high order bit (0x40)
if (shift < 64 && (byte & 0x40))
result |= -(1 << shift);
...
}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 разряда. Безопасный код:
result |= -(1ui64 << shift);К сожалению, я не соображу, как лучше поступить со знаком минус.
Рассмотрим другой пример, содержащий подозрительную операцию сдвига:
void EmitVBR64(uint64_t Val, unsigned NumBits) {
if ((uint32_t)Val == Val)
return EmitVBR((uint32_t)Val, NumBits);
uint64_t Threshold = 1U << (NumBits-1);
...
}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' бессмысленных значений.
Безопасный вариант:
uint64_t Threshold = 1UI64 << (NumBits-1);Рассмотренные выше примеры будут приводить к ошибкам, только в том случае, если сдвиг осуществляется на большое количество бит. Но есть фрагменты, которые всегда приводят к неопределенному поведению. Например, это сдвиг отрицательных чисел.
int find_next(unsigned Prev) const {
...
// Mask off previous bits.
Copy &= ~0L << BitPos;
...
}PVS-Studio: V610 Undefined behavior. Check the shift operator '<<. The left operand '~0L' is negative. bitvector.h 175
Так делать небезопасно. Проект Clang собирается для различных платформ. Нужно быть аккуратным с использованием таких конструкций. Сложно предвидеть, как где-то поведёт себя сдвиг отрицательных чисел.
Есть и другие потенциально опасные операции сдвига. Они однотипны, поэтому не будем их рассматривать подробнее. Просто перечислю их расположение в коде:
Помимо подозрительных сдвигов, было найдено несколько подозрительных циклов. Дело в том, что они выполняются только один раз.
bool ObjCARCOpt::VisitBottomUp(....) {
...
for (BBState::edge_iterator SI(MyStates.succ_begin()),
SE(MyStates.succ_end()); SI != SE; ++SI)
{
const BasicBlock *Succ = *SI;
DenseMap<const BasicBlock *, BBState>::iterator I =
BBStates.find(Succ);
assert(I != BBStates.end());
MyStates.InitFromSucc(I->second);
++SI;
for (; SI != SE; ++SI) {
Succ = *SI;
I = BBStates.find(Succ);
assert(I != BBStates.end());
MyStates.MergeSucc(I->second);
}
break;
}
...
}PVS-Studio: V612 An unconditional 'break' within a loop. objcarc.cpp 2763
Обратите внимание на последний оператор 'break'. Перед ним нет никакого условия, и он всегда останавливает цикл. Таким образом, выполняется только одна итерация цикла.
Аналогичные странные фрагменты кода:
Диагностики V610, V612, V629 являются новыми и поэтому, позволили найти что-то новое и интересное. Если вы проверяли свой проект год назад, это ничего не значит. Вообще ничего. Вы написали новый непроверенный код. В анализаторе появились новые диагностические возможности. И продолжают появляться каждый месяц. Начните использовать статический анализ регулярно, и вы существенно сократите усилия на поиск и устранения многих ошибок.