# Статический анализ следует применять регулярно Время от времени мы повторно анализируем проекты, которые уже проверяли с помощью PVS\-Studio\. Причин несколько\. Например, хочется посмотреть, удалось ли избавиться от определенных ложных срабатываний\. Но самое интересное, это посмотреть, как работают новые диагностические правила и какие ошибки они находят\. Очень интересно наблюдать, как в уже казалось бы очищенном проекте находятся всё новые и новые дефекты\. Очередным перепроверенным проектом стал Clang\. [Clang](http://clang.llvm.org/) очень интересный для нас проект\. Во\-первых, он очень качественный\. А значит найти в нём новую ошибку, это уже целое достижение\. Во\-вторых, на нем очень хорошо видны различные недоделки в PVS\-Studio, из\-за которых возникают ложные срабатывания\. К сожалению, с момента очередной проверки проекта и написанием этой заметки прошло более месяца\. Причина тому мой отпуск\. Есть вероятность, что описанный здесь подозрительный код к моменту публикации заметки уже будет исправлен\. Но я думаю это не страшно\. Главное я смогу напомнить читателям, что статический анализ это инструмент постоянного, а не разового применения\. Статический анализ следует применять регулярно, так как: * В проекте появляется новый код\. Если его не проверить сразу, многие ошибки будут исправляться медленно\. О них будет сообщать отдел тестирования или клиенты\. * Инструменты учатся выявлять всё новые и новые паттерны ошибок\. Всё это звучит очень просто и даже банально\. К сожалению, разработчики ленятся интегрировать статический анализ в процесс разработки\. Вновь и вновь приходится подталкивать их к этому полезному шагу\. [Предыдущий раз](https://pvs-studio.ru/ru/blog/posts/0108/) проект Clang мы проверяли приблизительно год назад\. За это время у нас появились новые диагностические правила, которые позволили выявить очередные места с подозрительным кодом\. Правда, таких мест совсем немного\. Это не удивительно, так как проект Clang сам включает в себя статический анализатор и разрабатывается высококвалифицированными разработчиками\. Странно, что вообще удается найти хоть что\-то подозрительное\. Посмотрим, что удалось заметить интересного в коде\. В основном подозрительные фрагменты связаны с операторами сдвига\. ```cpp 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\] позиций\. Однако этот код может привести к [неопределенному поведению](https://pvs-studio.ru/ru/blog/terms/0066/) \(undefined behaviour\)\. Подробнее, почему здесь возможно неопределенное поведение, можно познакомиться в статье "[Не зная брода, не лезь в воду\. Часть третья](https://pvs-studio.ru/ru/blog/posts/cpp/0142/)"\. Коварство подобных дефектов заключается в том, что программа может долгое время делать вид, что корректно работает\. Сбои могут возникать при смене версий компилятора, при изменении ключей оптимизации и после рефакторинга кода\. Код станет безопасным, если число 1 будет представлено 64\-битным беззнаковым типом данных\. Тогда его можно будет смело сдвигать на 63 разряда\. Безопасный код: ```cpp result |= -(1ui64 << shift); ``` К сожалению, я не соображу, как лучше поступить со знаком минус\. Рассмотрим другой пример, содержащий подозрительную операцию сдвига: ```cpp 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' бессмысленных значений\. Безопасный вариант: ```cpp uint64_t Threshold = 1UI64 << (NumBits-1); ``` Рассмотренные выше примеры будут приводить к ошибкам, только в том случае, если сдвиг осуществляется на большое количество бит\. Но есть фрагменты, которые всегда приводят к неопределенному поведению\. Например, это сдвиг отрицательных чисел\. ```cpp 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 собирается для различных платформ\. Нужно быть аккуратным с использованием таких конструкций\. Сложно предвидеть, как где\-то поведёт себя сдвиг отрицательных чисел\. Есть и другие потенциально опасные операции сдвига\. Они однотипны, поэтому не будем их рассматривать подробнее\. Просто перечислю их расположение в коде: * 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 Помимо подозрительных сдвигов, было найдено несколько подозрительных циклов\. Дело в том, что они выполняются только один раз\. ```cpp bool ObjCARCOpt::VisitBottomUp(....) { ... for (BBState::edge_iterator SI(MyStates.succ_begin()), SE(MyStates.succ_end()); SI != SE; ++SI) { const BasicBlock *Succ = *SI; DenseMap::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'\. Перед ним нет никакого условия, и он всегда останавливает цикл\. Таким образом, выполняется только одна итерация цикла\. Аналогичные странные фрагменты кода: * 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 являются новыми и поэтому, позволили найти что\-то новое и интересное\. Если вы проверяли свой проект год назад, это ничего не значит\. Вообще ничего\. Вы написали новый непроверенный код\. В анализаторе появились новые диагностические возможности\. И [продолжают появляться](https://pvs-studio.ru/ru/docs/manual/0010/) каждый месяц\. Начните использовать статический анализ регулярно, и вы существенно сократите усилия на поиск и устранения многих ошибок\.