PVS-Studio: потому, что могу
Компиляторы развиваются и выдают всё больше предупреждений. Остаются ли преимущества от использования статических анализаторов кода, таких как 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 предупреждения:


  • 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


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


1003_LLVM_15_ru/image2.webp


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.


Но как минимум есть ещё пяток таких ошибок
  • V595 [CWE-476, CERT-EXP12-C] The 'sc.symbol' pointer was utilized before it was verified against nullptr. Check lines: 899, 900. Module.cpp 899
  • V595 [CWE-476, CERT-EXP12-C] The 'process' pointer was utilized before it was verified against nullptr. Check lines: 159, 184. IRExecutionUnit.cpp 159
  • V595 [CWE-476, CERT-EXP12-C] The 'localVarCst' pointer was utilized before it was verified against nullptr. Check lines: 77, 96. AffineStructures.cpp 77
  • V595 [CWE-476, CERT-EXP12-C] The 'Class' pointer was utilized before it was verified against nullptr. Check lines: 58, 60. Transforms.cpp 58
  • V595 [CWE-476, CERT-EXP12-C] The 'CurPPLexer' pointer was utilized before it was verified against nullptr. Check lines: 739, 743. PPDirectives.cpp 739
  • Возможно, ошибок больше. Я не всматривался. Их неинтересно изучать и тем более — писать про них, так как они однотипные.

Фрагмент 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


Схожий код можно увидеть здесь
  • 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: 381, 382. LibCxx.cpp 381
  • 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: 551, 552. LibCxx.cpp 551
  • 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: 635, 636. LibCxx.cpp 635
  • 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: 49, 50. LibCxxInitializerList.cpp 49
  • 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: 70, 71. LibCxxSpan.cpp 70
  • 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: 311, 312. LibCxxList.cpp 311
  • 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: 207, 208. LibCxxMap.cpp 207
  • 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: 71, 72. LibCxxVector.cpp 71
  • 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: 174, 176. LibCxxVector.cpp 174
  • 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: 57, 59. LibCxxUnorderedMap.cpp 57
  • 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: 80, 82. LibStdcpp.cpp 80
  • 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: 183, 185. LibStdcpp.cpp 183
  • 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: 354, 355. LibStdcpp.cpp 354
  • 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: 464, 465. NSArray.cpp 464
  • 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: 608, 610. NSArray.cpp 608
  • 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: 404, 405. NSSet.cpp 404
  • 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: 671, 673. NSSet.cpp 671

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


Въедливый читатель может возразить, что, даже если передать нулевой указатель, это не обязательно будет проблемой. В конструкторе 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.

Комментарии (8)


  1. Demacr
    25.10.2022 18:50

    Всегда интересовало: а после проверки вы открываете PR или issue c найденными проблемами?


    1. Andrey2008 Автор
      25.10.2022 18:53
      +9

      Мы всегда уведомляем разработчиков о найденных ошибках. Почему мы сами не делаем Pull Requests для исправления найденных ошибок? Если кратко, то есть 3 причины:

      1. Мы постоянно что-то проверяем. Количество найденных ошибок в открытых проектах давно перевалило за 15000. Если мы будем все их пытаться править, то это надо на постоянную основу брать пару человек, которые только и будут этим заниматься. Пока мы не можем позволить себе такое баловство.

      2. Мы не знакомы с проектом и часто просто не знаем, как править ошибки. Т.е. то что это ошибка, это понятно, а как править — нет.

      3. При написании статей у нас не полноценный, а поверхностный анализ, цель которого популяризация статического анализа кода. Да, какие-то ошибки будут поправлены, но вовсе не все, которые можно обнаружить. Если уж править, то надо подойти к этому серьезно. Это лучше и качественнее сделают разработчики проекта, а не мы. Более того, статический анализ надо использовать регулярно, а не от случая к случаю. Подробнее эта мысль изложена здесь: "Ошибки, которые не находит статический анализ кода, потому что он не используется".


      1. alan008
        25.10.2022 23:48
        +2

        Интересно, исправят ли они хоть одну.)


  1. catBasilio
    25.10.2022 19:41

    А наоборот пробовали?
    Тем же CSA проверить код PVS И опубликовать результаты.


    1. Andrey2008 Автор
      25.10.2022 20:00

      Проверка PVS-Studio с помощью Clang (правда это 2014 год, а потом мы это регулярно делаем).


  1. Kohelet
    26.10.2022 09:18
    +1

    Фрагмент N1. Возможно, правильный вариант не тот, что вы предложили, а «два скаляра или два указателя».
    Фрагмент N3. Если, по вашему предложению, заменить константу на какую-то другую, то выражение станет всегда истинным. Если kind равно ImportKind::Only, то точно не равно чему-то другому.


    1. Andrey2008 Автор
      26.10.2022 09:35
      +1

  1. IkaR49
    26.10.2022 12:42
    +1

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

    Не факт. В нашем проекте кастомный assert, и в релизе срабатывание assert'а вызывает падение всей программы.