Примеры ошибок, которые может обнаружить PVS-Studio в коде LLVM 15.0

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

Назначение статьи

Иногда программисты задаются вопросом, а есть ли смысл пробовать какие-то дополнительные инструменты контроля качества кода, помимо тех, которые предоставляют компиляторы/среды разработки. Есть ли преимущества у таких инструментов перед «условным -Wall»? Стоит ли внедрять в процесс разработки дополнительный инструмент, такой как PVS-Studio?

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

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

Однако это теоретический ответ. А как дела обстоят на практике?

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

• 2021 — LLVM 13.0;
• 2020 — GCC 10;
• 2020 — LLVM 11;
• 2019 — Roslyn;
• 2019 — LLVM 8.

Проверку LLVM 14 мы пропустили, но решили, что стоит написать заметку, посвященную проверке LLVM 15.

Справка из Wikipedia. LLVM — проект программной инфраструктуры для создания компиляторов и сопутствующих им утилит. Состоит из набора компиляторов из языков высокого уровня, системы оптимизации, интерпретации и компиляции в машинный код. Написан на C, C++ и ассемблере. Сайт проекта: llvm.org.

При разработке проекта LLVM для предотвращения ошибок используются диагностические возможности компилятора Clang. LLVM проверяется с помощью Clang Static Analyzer и clang-tidy. Более того, проект LLVM дополнительно проверяется с помощью проприетарного анализатора Coverity.

Найти даже несколько ошибок в LLVM с помощью статического анализатора — это уже большое достижение. И анализатор PVS-Studio на это способен!

Анализ проекта, предупреждения, ложные срабатывания

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

Это не является правильным сценарием работы с анализатором. Надо понимать, что моей целью было только написать эту статью :). И с этой задачей я успешно справился. О том, как правильно начать использовать анализатор, я писал в статье «Как внедрить статический анализатор кода в legacy проект и не демотивировать команду«.

Итак в отчёте встретились ложно-положительные срабатывания. С этими всё понятно, любой статический анализатор, к сожалению, их выдаёт. Интересен другой момент. На проекте LLVM я увидел очень большое количество предупреждений, которые нельзя назвать ложными. Анализатор прав. Но и полезными эти сообщения назвать сложно. Давайте чуть подробнее остановимся на этой теме.

Очень много неоднозначных предупреждений связано с условиями, которые всегда истинны или ложны. Из-за этого я даже не захотел изучать предупреждения V547 — Expression is always true/false. Это утомительно и неинтересно. Давайте на паре примеров посмотрим, что я имею в виду.

int L = -1, V = -1, T = -1; if (L != -1 && V != -1 && T != -1 && Name.empty()) {  // <=   if (!Strict) {     Buffer.append("HANGUL SYLLABLE ");     if (L != -1)                                      // <=       Buffer.append(HangulSyllables[L][0]);     if (V != -1)                                      // <=       Buffer.append(HangulSyllables[V][1]);     if (T != -1)                                      // <=       Buffer.append(HangulSyllables[T][2]);   }

Здесь анализатор выдаёт сразу 3 предупреждения:

• V547 [CWE-571] Expression ‘L != — 1’ is always true. UnicodeNameToCodepoint.cpp 306
• V547 [CWE-571] Expression ‘V != — 1’ is always true. UnicodeNameToCodepoint.cpp 308
• V547 [CWE-571] Expression ‘T != — 1’ is always true. UnicodeNameToCodepoint.cpp 310

И действительно, переменные L, V, T проверяются дважды. Повторные проверки избыточны, но и ошибкой это назвать нельзя.

Другой пример:

while (top_reader_sp) {   if (!top_reader_sp)     break;   top_reader_sp->Run();

Предупреждение: V547 [CWE-570] Expression ‘!top_reader_sp’ is always false. Debugger.cpp 1042

Условный оператор действительно не имеет смысла. Это похоже на последствия множественных правок и рефакторинга. Код менялся, менялся и в результате стал вот таким. Да, это можно назвать атавизмом, но на ошибку всё равно не тянет.

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

42 ошибки в коде LLVM

Почему 42? Потому, что «42 — ответ на главный вопрос жизни, вселенной и всего такого» :). Выписывать больше примеров ошибок мне было утомительно и неинтересно. Да и всё равно читать здоровый отчёт будет никому неинтересно. Ну разве что разработчикам LLVM… Но им будет полезней проверить проект, настроить анализатор PVS-Studio и самостоятельно изучить предупреждения.

Фрагмент N1

Классическая ошибка, похожая на множество тех, что я рассматривал в статье «Зло живёт в функциях сравнения«. Конечно, перед нами не совсем функция сравнения. Однако суть в том же. Есть два объекта, над которыми выполняется ряд проверок. Эта функция скучна, и её не интересно внимательно проверять на code review. И именно поэтому в ней есть эта ошибка, которую, к счастью, может заметить статический анализатор кода. Анализатор никогда не устаёт и не ленится :).

static bool isCopyCompatibleType(LLT SrcTy, LLT DstTy) {   if (SrcTy == DstTy)     return true;    if (SrcTy.getSizeInBits() != DstTy.getSizeInBits())     return false;    SrcTy = SrcTy.getScalarType();   DstTy = DstTy.getScalarType();    return (SrcTy.isPointer() && DstTy.isScalar()) ||          (DstTy.isScalar() && SrcTy.isPointer()); }

Предупреждение PVS-Studio: V501 [CWE-570] There are identical sub-expressions ‘(SrcTy.isPointer() && DstTy.isScalar())’ to the left and to the right of the ‘||’ operator. CallLowering.cpp 1198

Ошибка здесь:

(SrcTy.isPointer() && DstTy.isScalar()) || (DstTy.isScalar()  && SrcTy.isPointer())

Программист одновременно поменял во второй строчке Src на Dst и Pointer на Scalar. В результате получается, что два раза проверяется одно и то же! Код выше эквивалентен:

(SrcTy.isPointer() && DstTy.isScalar()) || (SrcTy.isPointer() && DstTy.isScalar())

Правильный вариант:

(SrcTy.isPointer() && DstTy.isScalar()) || (SrcTy.isScalar()  && DstTy.isPointer())

Фрагмент N2

И сразу ещё одна родственная ошибка.

static ValueKnowledge meet(const ValueKnowledge &lhs,                            const ValueKnowledge &rhs) {   ValueKnowledge result = getPessimisticValueState();   result.hasError = true;    if (!rhs || !rhs || lhs.dtype != rhs.dtype)   // <=     return result;    result.hasError = false;   result.dtype = lhs.dtype;   .... }

Предупреждение PVS-Studio: V501 [CWE-570] There are identical sub-expressions to the left and to the right of the ‘||’ operator: !rhs ||!rhs ShapeUtils.h 141

Дважды проверяется объект rhs, а lhs не проверяется. Обыкновенная спешка при написании кода с отсутствием последующей внимательной проверки на этапе code review.

Фрагмент N3

std::optional<parser::MessageFixedText> Scope::SetImportKind(ImportKind kind) {   ....   } else if (kind != *importKind_ &&       (kind != ImportKind::Only || kind != ImportKind::Only)) {     return        "Every IMPORT must have ONLY specifier if one of them does"_err_en_US;   } else {   .... }

Предупреждение PVS-Studio: V501 [CWE-570] There are identical sub-expressions ‘kind != ImportKind::Only’ to the left and to the right of the ‘||’ operator. scope.cpp 275

Нет смысла два раза выполнять одну и ту же проверку. Перед нами явная опечатка. Должна быть использована ещё какая-то именованная константа, помимо ImportKind::Only.

Примечание. Если бы выражение было оформлено в виде «столбика», а не в одну строчку, то, возможно, ошибку было бы легче заметить. См. главу 13. Выравнивайте однотипный код «таблицей» 🙂 Я бы писал этот код так:

} else if (kind != *importKind_ &&     (   kind != ImportKind::Only      || kind != ImportKind::Only)) {   return ....;

Фрагмент N4

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

bool Merger::maybeZero(unsigned e) const {   if (tensorExps[e].kind == kInvariant) {     if (auto c = tensorExps[e].val.getDefiningOp<complex::ConstantOp>()) {       ArrayAttr arrayAttr = c.getValue();       return arrayAttr[0].cast<FloatAttr>().getValue().isZero() &&              arrayAttr[0].cast<FloatAttr>().getValue().isZero();     }     if (auto c = tensorExps[e].val.getDefiningOp<arith::ConstantIntOp>())       return c.value() == 0;     if (auto c = tensorExps[e].val.getDefiningOp<arith::ConstantFloatOp>())       return c.value().isZero();   }   return true; }

Предупреждение PVS-Studio: V501 [CWE-571] There are identical sub-expressions ‘arrayAttr[0].cast < FloatAttr > ().getValue().isZero()’ to the left and to the right of the ‘&&’ operator. Merger.cpp 812

Подозрительно это место:

return arrayAttr[0].cast<FloatAttr>().getValue().isZero() &&        arrayAttr[0].cast<FloatAttr>().getValue().isZero();

Фрагмент N5

И последняя ошибка, выявленная диагностикой V501.

template <int KIND> Expr<Type<TypeCategory::Logical, KIND>> FoldIntrinsicFunction(....) {   ....   } else if (name == "__builtin_ieee_support_datatype" ||       name == "__builtin_ieee_support_denormal" ||       name == "__builtin_ieee_support_divide" ||     // <=       name == "__builtin_ieee_support_divide" ||     // <=       name == "__builtin_ieee_support_inf" ||       name == "__builtin_ieee_support_io" ||       name == "__builtin_ieee_support_nan" ||       name == "__builtin_ieee_support_sqrt" ||       name == "__builtin_ieee_support_standard" ||       name == "__builtin_ieee_support_subnormal" ||       name == "__builtin_ieee_support_underflow_control") {     return Expr<T>{true};   }   .... }

Предупреждение PVS-Studio: V501 [CWE-570] There are identical sub-expressions ‘name == «__builtin_ieee_support_divide»‘ to the left and to the right of the ‘||’ operator. fold-logical.cpp 218

Я пометил комментарием две одинаковые проверки. Скорее всего, одна из них просто лишняя. Я думаю, что перед нами не настоящая ошибка, а избыточный код. Однако есть небольшая вероятность, что здесь забыли сравнить name с какой-то другой строковой константой.

Фрагмент N6

Для начала обратите внимание, что bmap — это указатель:

struct isl_coalesce_info {   isl_basic_map *bmap;   .... };

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

static isl_stat tab_insert_divs(struct isl_coalesce_info *info, ....) {   ....   if (any) {     if (isl_tab_rollback(info->tab, snap) < 0)       return isl_stat_error;     info->bmap = isl_basic_map_cow(info->bmap);     info->bmap = isl_basic_map_free_inequality(info->bmap, 2 * n);      if (info->bmap < 0)                 // <=       return isl_stat_error;      return fix_constant_divs(info, n, expanded);   }   .... }

Предупреждение PVS-Studio: V503 [CWE-697, CERT-EXP08-C] This is a nonsensical comparison: pointer < 0. isl_coalesce.c 3181

Нет практического смысла выполнять проверку «указатель < 0». Скорее всего, перед нами опечатка или код не дописан. Например, с 0 должен сравниваться какой-то из членов структуры isl_basic_map.

Фрагмент N7

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

void FunctionLoweringInfo::ComputePHILiveOutRegInfo(const PHINode *PN) {   ....   Register DestReg = It->second;   if (DestReg == 0)     return   assert(Register::isVirtualRegister(DestReg) &&          "Expected a virtual reg");   LiveOutRegInfo.grow(DestReg);   .... }

Предупреждение PVS-Studio: V504 [CWE-841] It is highly probable that the semicolon ‘;’ is missing after ‘return’ keyword. FunctionLoweringInfo.cpp 454

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

Кажется, что assert выполняется в том случае, если не выполняется условие DestReg == 0. Но на самом деле, assert выполнится только в том случае, если DestReg == 0.

Нестрашна ошибка только из-за везения. Собственно, assert не оказывает заметного влияния на выполнение программы. А в release-версии он вообще превращается в ничто.

Фрагмент N8, N9

Не изучал историю этого кода, но, скорее всего, это «калька с malloc«. Т.е. когда-то здесь использовалась функция malloc, а затем её грубо заменили на new и shared_ptr.

std::shared_ptr<uint8_t> RenderScriptRuntime::GetAllocationData(....) {   ....   const uint32_t size = *alloc->size.get();   std::shared_ptr<uint8_t> buffer(new uint8_t[size]);   if (!buffer) {     LLDB_LOGF(log, "%s - couldn't allocate a %" PRIu32 " byte buffer",               __FUNCTION__, size);     return nullptr;   }   ....   return buffer; }

Предупреждение PVS-Studio: V554 [CWE-762, CERT-MEM51-CPP] Incorrect use of shared_ptr. The memory allocated with ‘new []’ will be cleaned using ‘delete’. RenderScriptRuntime.cpp 2371

Умный указатель std::shared_ptr используется некорректно:

std::shared_ptr<uint8_t> buffer(new uint8_t[size]);

Память будет освобождаться с помощью delete, а не delete [], что приводит к неопределённому поведению. Подробности см. в статье «Почему в С++ массивы нужно удалять через delete[]«. Правильный вариант:

std::shared_ptr<uint8_t []> buffer(new uint8_t[size]);

Поясняю, почему я предполагаю, что раньше здесь использовался malloc. Обратите внимание на последующую проверку указателя на равенство nullptr. Она не имеет смысла, так как в случае ошибки выделения памяти будет брошено исключение std::bad_alloc. Проверка и вывод отладочного сообщения — атавизм.

Ещё точно такая же «калька с malloc«: V554 [CWE-762, CERT-MEM51-CPP] Incorrect use of shared_ptr. The memory allocated with ‘new []’ will be cleaned using ‘delete’. RenderScriptRuntime.cpp 2698

Фрагмент N10

Ой, я давно не находил столь примечательную и забавную ошибку! Однозначно жемчужина среди опечаток!

OwningMemRef &operator=(const OwningMemRef &&other) {   freeFunc = other.freeFunc;   descriptor = other.descriptor;   other.freeFunc = nullptr;   memset(0, &other.descriptor, sizeof(other.descriptor)); }

Предупреждение PVS-Studio: V575 [CWE-628, CERT-EXP37-C] The null pointer is passed into ‘memset’ function. Inspect the first argument. MemRefUtils.h 194

Я описал разнообразные способы, как можно ошибиться, используя функцию memset, в статье «Самая опасная функция в мире С/С++«. Но это что-то новенькое!

Перепутан первый и второй аргумент функции. Запись произойдёт по нулевому указателю.

Хотя на самом деле не произойдёт. Этот код просто не скомпилируется, так как в качестве второго фактического аргумента передаётся указатель, а функция ожидает int. Этот код находится в шаблонном классе. И пока функция не используется, она не инстанцируется, и компилятор не замечает, что код некорректен. В случае шаблонов компилятор может не выдавать ошибки на некорректный код, пока он не используется.

В общем ничего страшного, но тем не менее это ошибка, которую лучше поправить заранее.

Фрагмент N11-N17

Самым распространённым среди обнаруживаемых нашей командой в открытых проектах дефектов является разыменование указателя до его проверки. По крайней мере больше всего в базе ошибок выписано случаев именно такого типа. Исходные коды LLVM не исключение.

void LibCallSimplifier::classifyArgUse(....) {   CallInst *CI = dyn_cast<CallInst>(Val);   Module *M = CI->getModule();    if (!CI || CI->use_empty())     return;   .... }

Предупреждение PVS-Studio: V595 [CWE-476, CERT-EXP12-C] The ‘CI’ pointer was utilized before it was verified against nullptr. Check lines: 2515, 2517. SimplifyLibCalls.cpp 2515

Указатель CI в начале разыменовывается при вызове функции: CI->getModule(). И только затем проверяется на равенство nullptr.

Ещё один пример:

void Module::FindFunctions(....) {   ....   for (size_t i = 0; i < num_matches; ++i) {     sc.symbol = symtab->SymbolAtIndex(symbol_indexes[i]);     SymbolType sym_type = sc.symbol->GetType();     if (sc.symbol && (sym_type == eSymbolTypeCode ||                       sym_type == eSymbolTypeResolver))       sc_list.Append(sc);   }   .... }

Предупреждение PVS-Studio: V595 [CWE-476, CERT-EXP12-C] The ‘sc.symbol’ pointer was utilized before it was verified against nullptr. Check lines: 877, 878. Module.cpp 877

Чтобы сократить выражение, программист решил предварительно сохранить некий тип символа в переменной sym_type:

SymbolType sym_type = sc.symbol->GetType();

Проблема в том, что он сделал это, не учтя, что указатель sc.symbol может быть нулевым. Это видно из следующей проверки:

if (sc.symbol && (sym_type == eSymbolTypeCode ||                   sym_type == eSymbolTypeResolver))

Далее рассматривать подобные ошибки неинтересно. Более того, я не стал изучать все предупреждения V595.

Фрагмент N18

Optional<SmallVector<ReassociationIndices>> mlir::getReassociationIndicesForCollapse(ArrayRef<int64_t> sourceShape,                                          ArrayRef<int64_t> targetShape) {   unsigned sourceDim = 0;   ....   int64_t currTargetShape = targetShape[targetDim];   while (sourceShape[sourceDim] != ShapedType::kDynamicSize &&          prodOfCollapsedDims * sourceShape[sourceDim] < currTargetShape &&          sourceDim < sourceShape.size()) {     prodOfCollapsedDims *= sourceShape[sourceDim];     currIndices.push_back(sourceDim++);   }   .... }

Предупреждение PVS-Studio: V781 [CWE-20, CERT-API00-C] The value of the ‘sourceDim’ index is checked after it was used. Perhaps there is a mistake in program logic. ReshapeOpsUtils.cpp 49

Потенциальный выход за границу массива. Взглянем внимательно на это условие цикла:

sourceShape[sourceDim] != ShapedType::kDynamicSize && .... && sourceDim < sourceShape.size()

Проверка индекса выполняется до обращения к элементу массива. Более логичным и безопасным выглядит такой вариант кода:

sourceDim < sourceShape.size() && sourceShape[sourceDim] != ShapedType::kDynamicSize && ....

Фрагмент N19-N36

Неожиданно много опасных разыменований указателей встретилось в конструкторах классов. Указатель смело разыменовывается при инициализации членов класса, а затем вдруг проверяется на равенство nullptr в теле конструктора. Пример:

typedef std::shared_ptr<lldb_private::ValueObject> ValueObjectSP;  lldb_private::formatters::LibCxxMapIteratorSyntheticFrontEnd::     LibCxxMapIteratorSyntheticFrontEnd(lldb::ValueObjectSP valobj_sp)     : SyntheticChildrenFrontEnd(*valobj_sp), m_pair_ptr(), m_pair_sp() {   if (valobj_sp)     Update(); }

Предупреждение PVS-Studio: V664 [CWE-476, CERT-EXP34-C] The ‘valobj_sp’ pointer is being dereferenced on the initialization list before it is verified against null inside the body of the constructor function. Check lines: 211, 212. LibCxx.cpp 211

Этот код не обязательно приводит к проблемам. Возможно, в конструкторы просто никогда не передаются нулевые указатели. Однако хорошим и правильным код всё равно назвать нельзя.

Въедливый читатель может возразить, что, даже если передать нулевой указатель, это не обязательно будет проблемой. В конструкторе SyntheticChildrenFrontEnd сохраняется ссылка на объект. Если передать nullptr, то сохранится ссылка на несуществующий объект. Однако если никак далее не «трогать» эту ссылку, то и проблемы не будет. Некрасиво, но не критично.

Нет, на такое допущение закладываться нельзя. Разыменование нулевого указателя — это неопределённое поведение. И невозможно предсказать, как оно проявит себя. Например, компилятор видит, что указатель разыменовывается. Значит, он точно не нулевой. Следовательно, в целях оптимизации проверку «if (valobj_sp)» можно удалить. Подробнее эту тему я рассматривал в статье «Разыменовывание нулевого указателя приводит к неопределённому поведению«. И вот здесь тоже про это говорится «What Every C Programmer Should Know About Undefined Behavior #2/3«.

Фрагмент N37

uint64_t NullabilityPayload = 0;  static constexpr const unsigned NullabilityKindMask = 0x3;  void addTypeInfo(unsigned index, NullabilityKind kind) {   ....   NullabilityPayload &=     ~(NullabilityKindMask << (index * NullabilityKindSize));   .... }

Предупреждение PVS-Studio: V784 [CWE-197, CERT-INT31-C] The size of the bit mask is less than the size of the first operand. This will cause the loss of higher bits. Types.h 529

Старшие биты в переменной NullabilityPayload будут потеряны. Сейчас поясню, как это произойдёт.

Переменная NullabilityKindMask представлена 32-битным типом unsigned. Сколько не сдвигай биты в этой переменной, результат всё равно будет иметь тип unsigned. Оператор побитового НЕ (~) также не изменит тип выражения.

И только в момент выполнения оператора &= тип unsigned будет расширен до типа uint64_t. Так как тип был беззнаковый, то старшие биты 64-битного типа будут всегда нулевыми. В результате выполнится операция:

NullabilityPayload &= 0x00000000xxxxxxxx;

Что приведёт к сбросу старших битов в NullabilityPayload. Правильный вариант кода:

NullabilityPayload &=   ~(static_cast<uint64_t>(NullabilityKindMask)      << (index * NullabilityKindSize));

Другой вариант исправить код — это сразу сделать константу NullabilityKindMask 64-битной:

static constexpr const uint64_t NullabilityKindMask = 0x3; .... NullabilityPayload &=   ~(NullabilityKindMask << (index * NullabilityKindSize));

Фрагмент N38

Бывает интересно заметить ошибку в тестах и понять, что он тестирует не совсем то, что задумывалось. Это, кстати, хороший пример, когда статический анализ дополняет юнит-тесты и методологию TDD. Ведь никто не пишет тесты на тесты :). К счастью, статический анализатор помогает находить ошибки не только в основном коде, но и в тестах.

TEST(RegisterContextMinidump, ConvertMinidumpContext_x86_64) {   MinidumpContext_x86_64 Context;   ....   Context.rax = 0x0001020304050607;   Context.rbx = 0x08090a0b0c0d0e0f;   ....   Context.eflags = 0x88898a8b;   Context.cs = 0x8c8d;   Context.fs = 0x8e8f;   Context.gs = 0x9091;   Context.ss = 0x9293;   Context.ds = 0x9495;   Context.ss = 0x9697;   llvm::ArrayRef<uint8_t> ContextRef(reinterpret_cast<uint8_t *>(&Context),                                      sizeof(Context));   .... }

Предупреждение PVS-Studio: V519 [CWE-563, CERT-MSC13-C] The ‘Context.ss’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 110, 112. RegisterContextMinidumpTest.cpp 112

Обратите внимание на вот эти строчки:

Context.ss = 0x9293; .... Context.ss = 0x9697;

Опечатка. В одном месте, по всей видимости, следовало написать Context.es.

Фрагмент N39, N40

В начале статьи я говорил, что не стал разбираться с некоторыми предупреждениями, такими как V547. Но немного я всё-таки их посмотрел и заметил, например, вот такую ошибку. Её обнаружение возможно благодаря технологии data-flow анализа, который учитывает возможные значения переменных в разных точках программы. Если вам интересно, как вообще работает анализатор, предлагаю вашему вниманию заметку «Технологии статического анализа кода PVS-Studio«.

static std::unordered_multimap<char32_t, std::string> loadDataFiles(const std::string &NamesFile, const std::string &AliasesFile) {   ....   auto FirstSemiPos = Line.find(';');   if (FirstSemiPos == std::string::npos)                   // <=     continue;   auto SecondSemiPos = Line.find(';', FirstSemiPos + 1);   if (FirstSemiPos == std::string::npos)                   // <=     continue;   .... }

Предупреждение PVS-Studio: V547 [CWE-570] Expression ‘FirstSemiPos == std::string::npos’ is always false. UnicodeNameMappingGenerator.cpp 46

Скорее всего, этот код писался с помощью Copy-Paste. Был размножен этот фрагмент:

auto FirstSemiPos = Line.find(';'); if (FirstSemiPos == std::string::npos)   continue;

Затем поменяли вызов функции find. First заменили на Second, но не везде. В результате строчка

if (FirstSemiPos == std::string::npos)

осталась без изменений. Так как это условие уже проверялось выше, анализатор сообщает, что условие всегда ложно. Такие ошибки очень легко просмотреть на code review, но они хорошо находятся с помощью PVS-Studio.

И ещё одна прям точно такая же ошибка:

Status Host::ShellExpandArguments(ProcessLaunchInfo &launch_info) {   ....   auto data_sp = StructuredData::ParseJSON(output);   if (!data_sp) {                                        // <=     error.SetErrorString("invalid JSON");     return error;   }    auto dict_sp = data_sp->GetAsDictionary();   if (!data_sp) {                                        // <=     error.SetErrorString("invalid JSON");     return error;   }   .... }

Предупреждение PVS-Studio: V547 [CWE-570] Expression ‘!data_sp’ is always false. Host.cpp 248

Фрагмент N41

Для выявления двух предыдущих ошибок использовался анализ потока данных (data-flow анализ). Для следующей ошибки используется ещё более изощрённая техника: символьное выполнение (symbolic execution). Смысл в том, что необязательно знать возможные значения переменных. Чтобы определить, что условие истинно/ложно, можно решить уравнение.

void Sema::adjustMemberFunctionCC(QualType &T, bool IsStatic, bool IsCtorOrDtor,                                   SourceLocation Loc) {   ....   CallingConv CurCC = FT->getCallConv();   CallingConv ToCC = Context.getDefaultCallingConvention(IsVariadic, !IsStatic);    if (CurCC == ToCC)     return;   ....   CallingConv DefaultCC =      Context.getDefaultCallingConvention(IsVariadic, IsStatic);    if (CurCC != DefaultCC || DefaultCC == ToCC)     return;   .... }

Предупреждение PVS-Studio: V560 [CWE-570] A part of conditional expression is always false: DefaultCC == ToCC. SemaType.cpp 7856

Давайте разбираться, почему анализатор решил, что правая часть условия всегда ложна. Для удобства уберём всё лишнее и заменим имена:

A = ....; B = ....; if (A == B) return; C = ....; if (A != C || C == B)

Разберём как работает этот код:

• есть 3 переменные A, B, C, значения которых нам неизвестны;
• но мы знаем, что если A == B, то происходит выход из функции;
• следовательно, если функция продолжает выполняться, то A != B;
• если A != C, то, в силу вычисления по короткой схеме, правое подвыражение не вычисляется;
• если правое подвыражение «C == B» вычисляется, значит A == C;
• если A != B и A == C, то С никак не может быть равно B.

Возможно, перед нами опечатка в условии и, на самом деле, должно быть написано:

if (CurCC != DefaultCC || DefaultCC != ToCC)

Фрагмент N42

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

ObjectFileMachO::MachOCorefileAllImageInfos ObjectFileMachO::GetCorefileAllImageInfos() {   ....   lldb::offset_t offset = MachHeaderSizeFromMagic(m_header.magic);   ....   uint32_t imgcount = m_data.GetU32(&offset);   uint64_t entries_fileoff = m_data.GetU64(&offset);   offset += 4; // uint32_t entries_size;   offset += 4; // uint32_t unused;   offset = entries_fileoff;                  // <=   .... }

Предупреждение PVS-Studio: V519 [CWE-563, CERT-MSC13-C] The ‘offset’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 6888, 6890. ObjectFileMachO.cpp 6890

Очень странно в начале что-то считать в переменной offset, а потом взять и перетереть её значение. Возможно, здесь должно быть написано так:

offset += 4; // uint32_t entries_size; offset += 4; // uint32_t unused; offset += entries_fileoff;

Скачайте и попробуйте PVS-Studio

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

Заодно предлагаю подписаться на ежемесячную рассылку, чтобы не пропускать интересные публикации и узнавать о новых возможностях PVS-Studio.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Examples of errors that PVS-Studio found in LLVM 15.0.