Компиляторы развиваются и выдают всё больше предупреждений. Остаются ли преимущества от использования статических анализаторов кода, таких как PVS-Studio? Да, так как анализаторы тоже развиваются. Перед вами статья о том, как PVS-Studio находит баги даже в компиляторе.
Иногда программисты задаются вопросом, а есть ли смысл пробовать какие-то дополнительные инструменты контроля качества кода, помимо тех, которые предоставляют компиляторы/среды разработки. Есть ли преимущества у таких инструментов перед "условным -Wall"? Стоит ли внедрять в процесс разработки дополнительный инструмент, такой как PVS-Studio?
Вопрос вполне логичен и обоснован. Компиляторы развиваются, учатся находить новые ошибки и постепенно делают всё больше того, что раньше делали только инструменты статического анализа.
Ответ: да. Есть смысл использовать дополнительные инструменты статического анализа кода. Эти инструменты тоже развиваются и учатся выполнять всё более глубокий анализ кода. Сторонние инструменты решают узкую задачу: поиск ошибок и потенциальных уязвимостей. А следовательно они решают эту задачу лучше, чем компиляторы, для которых это только одна из функций.
Однако это теоретический ответ. А как дела обстоят на практике?
Чтобы иметь возможность уверенно отвечать на этот вопрос, наша команда время от времени выполняет анализ кода компиляторов. Если мы можем найти в их коде ошибки, значит, PVS-Studio по-прежнему нужный и востребованный инструмент для тех, кто заботится о качестве и надёжности своего кода. Некоторые из таких статей:
Проверку 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 предупреждения:
И действительно, переменные 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? Потому, что "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)Разберём как работает этот код:
Возможно, перед нами опечатка в условии и, на самом деле, должно быть написано:
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.