About Teletype
Join
O
@onehellsus
October 11, 2021

Выявляем ошибки в релизе LLVM 13.0.0

Проверка LLVM

Предупреждения PVS-Studio удобнее всего смотреть в среде разработки. У меня на машине из сред разработки оказалась доступна Visual Studio 2019. Ей я и воспользовался. Процесс крайне прост:

  • скачиваем исходные коды LLVM 13.0.0;
  • создаём проект для VS2019: cmake -S llvm -B build -G "Visual Studio 16 2019";
  • компилируем. Этот шаг необходим, чтобы были сгенерированы различные inc-файлы. Без них невозможно препроцессировать, а следовательно, и анализировать многие cpp-файлы;
  • удивляемся, что создалось разных файлов более чем на 100 Gb;
  • в меню Visual Studio указываем плагину PVS-Studio проверить проект;
  • profit.

На самом деле, не всё так радужно. Без предварительной настройки анализатора вы, скорее всего, столкнётесь с большим количеством ложных или неинтересных (в рамках данного проекта) предупреждений. Для меня лишние предупреждения не являются проблемой, так как моя задача найти некоторое количество интересных багов, достаточных для написания статьи. Не более того.

Если вы хотите начать использовать анализатор регулярно, то требуется его настройка. Помимо этого, на начальном этапе будет полезно объявить все предупреждения техническим долгом и спрятать их. После чего начать работать только с новыми предупреждениями, постепенно ликвидируя технический долг. Более подробно этот подход описан здесь.

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

Я потратил на просмотр лога один вечер и выписал то, что показалось мне интересным. Уверен, можно найти гораздо больше ошибок. Однако даже то, что, просто бегло пробежавшись по отчёту, можно исправить 20 ошибок, демонстрирует анализатор в выгодном свете.

Опечатки

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

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

Баг N1, ошибка конструирования 64-битного значения из двух 32-битных значений

uint64_t uval;
....
bool DWARFFormValue::extractValue(const DWARFDataExtractor &Data,
                                  uint64_t *OffsetPtr, dwarf::FormParams FP,
                                  const DWARFContext *Ctx,
                                  const DWARFUnit *CU) {
  ....
  case DW_FORM_LLVM_addrx_offset:
    Value.uval = Data.getULEB128(OffsetPtr, &Err) << 32;
    Value.uval = Data.getU32(OffsetPtr, &Err);
    break;
  ....
}

Предупреждение PVS-Studio: V519 [CWE-563, CERT-MSC13-C] The 'Value.uval' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 334, 335. DWARFFormValue.cpp 335

Нет смысла в одну и ту же переменную записывать поочередно разные значения. Об этом и предупреждает анализатор. Скорее всего, автор кода немного поспешил и допустил опечатку, забыв добавить '|'. Данный код должен создавать из двух 32-битных значений одно 64-битное. Корректный вариант этого кода:

case DW_FORM_LLVM_addrx_offset:
  Value.uval = Data.getULEB128(OffsetPtr, &Err) << 32;
  Value.uval |= Data.getU32(OffsetPtr, &Err);
  break;

Баг N2, поспешный copy-paste кода

В классе ExecutorAddress потребовалось реализовать набор однотипных операторов. Почти наверняка это делалось с помощью copy-paste. Согласитесь, достаточно скучно писать следующий код, не копируя строки.

class ExecutorAddress {
  ....
  ExecutorAddress &operator++() {
    ++Addr;
    return *this;
  }
  ExecutorAddress &operator--() {
    --Addr;
    return *this;
  }
  ExecutorAddress operator++(int) { return ExecutorAddress(Addr++); }
  ExecutorAddress operator--(int) { return ExecutorAddress(Addr++); }

  ExecutorAddress &operator+=(const ExecutorAddrDiff Delta) {
    Addr += Delta.getValue();
    return *this;
  }

  ExecutorAddress &operator-=(const ExecutorAddrDiff Delta) {
    Addr -= Delta.getValue();
    return *this;
  }
  ....
private:
  uint64_t Addr = 0;
}

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

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

ExecutorAddress operator++(int) { return ExecutorAddress(Addr++); }
ExecutorAddress operator--(int) { return ExecutorAddress(Addr++); }

Предупреждение PVS-Studio: V524 It is odd that the body of '--' function is fully equivalent to the body of '++' function. ExecutorAddress.h 104

Явная ошибка: забыли заменить в правой части скопированной строки оператор ++ на --.

Баг N3, никто не умеет писать функции сравнения

bool operator==(const BDVState &Other) const {
  return OriginalValue == OriginalValue && BaseValue == Other.BaseValue &&
    Status == Other.Status;
}

V501 [CWE-571] There are identical sub-expressions to the left and to the right of the '==' operator: OriginalValue == OriginalValue RewriteStatepointsForGC.cpp 758

Классика! Я даже отдельную большую статью писал на эту тему — "Зло живёт в функциях сравнения".

Чтобы снизить количество таких ошибок, рекомендую всегда оформлять однотипные операции табличным методом. Я бы оформил эту функцию так:

bool operator==(const BDVState &Other) const {
  return
       OriginalValue == OriginalValue
    && BaseValue == Other.BaseValue
    && Status == Other.Status;
}

Так длиннее, но зато больше вероятность, что опечатка будет замечена на code review. Впрочем, даже так ошибку можно просмотреть и лучше дополнительно подстраховаться, используя профессиональный анализатор.

Баг N4, никто не умеет писать функции сравнения (я серьезно)

Возможно, рассматривая предыдущий пример, вы подумали, что я преувеличиваю и перед нами одиночный случайный ляп. К сожалению, функции сравнения действительно тяготеют к опечаткам. Рассмотрим ещё один пример.

bool TypeInfer::EnforceSmallerThan(TypeSetByHwMode &Small,
                                   TypeSetByHwMode &Big) {
  ....
  if (Small.empty())
    Changed |= EnforceAny(Small);
  if (Big.empty())
    Changed |= EnforceAny(Big);

  assert(Small.hasDefault() && Big.hasDefault());

  SmallVector<unsigned, 4> Modes;
  union_modes(Small, Big, Modes);

  for (unsigned M : Modes) {
    TypeSetByHwMode::SetType &S = Small.get(M);
    TypeSetByHwMode::SetType &B = Big.get(M);

    if (any_of(S, isIntegerOrPtr) && any_of(S, isIntegerOrPtr)) {
      auto NotInt = [](MVT VT) { return !isIntegerOrPtr(VT); };
      Changed |= berase_if(S, NotInt);
      Changed |= berase_if(B, NotInt);
    } else if (any_of(S, isFloatingPoint) && any_of(B, isFloatingPoint)) {
      auto NotFP = [](MVT VT) { return !isFloatingPoint(VT); };
      Changed |= berase_if(S, NotFP);
      Changed |= berase_if(B, NotFP);
    } else if (S.empty() || B.empty()) {
      Changed = !S.empty() || !B.empty();
      S.clear();
      B.clear();
    } else {
      TP.error("Incompatible types");
      return Changed;
    }
  ....
}

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

Проблема здесь:

if (any_of(S, isIntegerOrPtr) && any_of(S, isIntegerOrPtr))

Предупреждение PVS-Studio: V501 [CWE-571] There are identical sub-expressions 'any_of(S, isIntegerOrPtr)' to the left and to the right of the '&&' operator. CodeGenDAGPatterns.cpp 479

Правильно:

if (any_of(S, isIntegerOrPtr) && any_of(B, isIntegerOrPtr))

Баг N5, форматирование таблицей не всегда помогает

LegalizerHelper::LegalizeResult LegalizerHelper::lowerRotate(MachineInstr &MI) {
  Register Dst = MI.getOperand(0).getReg();
  Register Src = MI.getOperand(1).getReg();
  Register Amt = MI.getOperand(2).getReg();
  LLT DstTy = MRI.getType(Dst);
  LLT SrcTy = MRI.getType(Dst);
  LLT AmtTy = MRI.getType(Amt);
  ....
}

Предупреждение PVS-Studio: V656 [CWE-665] Variables 'DstTy', 'SrcTy' are initialized through the call to the same function. It's probably an error or un-optimized code. Consider inspecting the 'MRI.getType(Dst)' expression. Check lines: 5953, 5954. LegalizerHelper.cpp 5954

Ранее я писал, что от опечаток помогает защититься форматирование кода таблицей. Да, помогает, но гарантии не даёт. Перед нами красиво оформленный код, похожий на таблицу. Но ошибка всё равно там есть.

Скорее всего, код писался копированием. Скопировали строчку

LLT DstTy = MRI.getType(Dst);

Но только в одном месте заменили Dst на Src:

LLT SrcTy = MRI.getType(Dst);

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

LLT DstTy = MRI.getType(Dst);
LLT SrcTy = MRI.getType(Src);
LLT AmtTy = MRI.getType(Amt);

Нулевые указатели

Писать код на C или C++ и случайно не разыменовать где-то нулевой указатель – это научная фантастика :). Есть такие места и в LLVM. Изучать предупреждения про нулевые указатели скучно и утомительно. Я просмотрел такие предупреждения очень бегло и подозреваю, что при желании таких ошибок будет выявлено намного больше.

Баг N6, разыменование указателя, который может быть нулевым

void DwarfCompileUnit::addLabelAddress(DIE &Die, dwarf::Attribute Attribute,
                                       const MCSymbol *Label) {
  ....
  if (Label)
    DD->addArangeLabel(SymbolCU(this, Label));

  bool UseAddrOffsetFormOrExpressions =
      DD->useAddrOffsetForm() || DD->useAddrOffsetExpressions();

  const MCSymbol *Base = nullptr;
  if (Label->isInSection() && UseAddrOffsetFormOrExpressions)
    Base = DD->getSectionLabel(&Label->getSection());
  ....
}

Предупреждение PVS-Studio: V1004 [CWE-476, CERT-EXP34-C] The 'Label' pointer was used unsafely after it was verified against nullptr. Check lines: 74, 81. DwarfCompileUnit.cpp 81

Проверка "if (Label)" говорит нам и анализатору, что указатель Label может быть нулевым. Но ниже этот указатель смело разыменовывается без всякой проверки:

if (Label->isInSection() && UseAddrOffsetFormOrExpressions)

Так явно не стоит делать.

Баг N7-N9, разыменование указателя, который может быть нулевым

static bool HandleUse(....)
{
  ....
  if (Pat->isLeaf()) {
    DefInit *DI = dyn_cast<DefInit>(Pat->getLeafValue());
    if (!DI)
      I.error("Input quot; + Pat->getName() + " must be an identifier!");
    Rec = DI->getDef();
  }
  ....
}

Предупреждение PVS-Studio: V1004 [CWE-476, CERT-EXP34-C] The 'DI' pointer was used unsafely after it was verified against nullptr. Check lines: 3349, 3351. CodeGenDAGPatterns.cpp 3351

Указатель DI проверятся, но далее сразу разыменовывается уже без проверки. Обосновано задаться вопросом, а является ли этой ошибкой? Ведь если указатель DI нулевой, то вызывается некая функция error, которая может генерировать исключение. Давайте заглянем в эту функцию:

void TreePattern::error(const Twine &Msg) {
  if (HasError)
    return;
  dump();
  PrintError(TheRecord->getLoc(), "In " + TheRecord->getName() + ": " + Msg);
  HasError = true;
}

Нет, эта функция не генерирует исключение и не останавливает выполнение программы каким-либо способом.

Получается, что сразу после фиксации ошибочного состояния (записи в лог) произойдёт разыменование нулевого указателя.

Как минимум, есть ещё пара аналогичных ошибок, которые нет смысла рассматривать отдельно:

  • V1004 [CWE-476, CERT-EXP34-C] The 'OpDef' pointer was used unsafely after it was verified against nullptr. Check lines: 2843, 2844. CodeGenDAGPatterns.cpp 2844
  • V1004 [CWE-476, CERT-EXP34-C] The 'Val' pointer was used unsafely after it was verified against nullptr. Check lines: 3418, 3420. CodeGenDAGPatterns.cpp 3420

Баг N10, недостаточная защита от нулевого указателя

Error DWARFDebugLine::LineTable::parse(...., raw_ostream *OS, bool Verbose) {
  assert((OS || !Verbose) && "cannot have verbose output without stream");
  ....
  auto EmitRow = [&] {
    if (!TombstonedAddress) {
      if (Verbose) {
        *OS << "\n";
        OS->indent(12);
      }
      if (OS)
        State.Row.dump(*OS);
      State.appendRowToMatrix();
    }
  };
  ....
}

Предупреждение PVS-Studio: V595 [CWE-476, CERT-EXP12-C] The 'OS' pointer was utilized before it was verified against nullptr. Check lines: 791, 793. DWARFDebugLine.cpp 791

Указатель OS может быть нулевым, о чём свидетельствует проверка "if (OS)". Однако ранее этот указатель может быть разыменован без предварительной проверки.

В начале есть assert, который защищает от нулевых указателей. Однако этого явно недостаточно, так как в релизной сборке макрос assert раскрывается в ничто.

Рационально сделать код более безопасным:

auto EmitRow = [&] {
  if (!TombstonedAddress) {
    if (OS)
    {
      if (Verbose) {
        *OS << "\n";
        OS->indent(12);
      }
      State.Row.dump(*OS);
    }
    State.appendRowToMatrix();
  }
};

Проблемы с перечислениями (enum)

Разработчики LLVM иногда полагаются на то, что небольшие перечисления (enum) будут представлены одним байтом. То есть sizeof(enum) == sizeof(char). Такое предположение опасно. Например, компилятор Visual C++ предпочитает по умолчанию делать размер перечисления, равным размеру int.

Баг N11, опасный индекс

enum class FunctionKinds { ENTRY, EXIT, TAIL, LOG_ARGS_ENTER, CUSTOM_EVENT };
....
static Error loadObj(....) {
  ....
  auto Kind = Extractor.getU8(&OffsetPtr);
  static constexpr SledEntry::FunctionKinds Kinds[] = {
      SledEntry::FunctionKinds::ENTRY, SledEntry::FunctionKinds::EXIT,
      SledEntry::FunctionKinds::TAIL,
      SledEntry::FunctionKinds::LOG_ARGS_ENTER,
      SledEntry::FunctionKinds::CUSTOM_EVENT};
  if (Kind >= sizeof(Kinds))
    return errorCodeToError(
        std::make_error_code(std::errc::executable_format_error));
  Entry.Kind = Kinds[Kind];
  ....
}

Предупреждение PVS-Studio: V557 [CWE-125, CERT-ARR30-C] Array overrun is possible. The value of 'Kind' index could reach 19. InstrumentationMap.cpp 196

Текст сообщения требует пояснения. Механизм анализа потока данных обрабатывает этот код:

if (Kind >= sizeof(Kinds))
  return errorCodeToError(...);

И приходит к выводу, что если условие не выполняется, то переменная Kind имеет далее значение [0..19].

Почему 19, а не 4? Я осуществлял проверку проекта с помощью плагина для Visual Studio 2019. Соответственно, анализатор знает, что использовался компилятор Visual C++ и что перечисление будет представлено четырьмя байтами. В этом можно убедиться, написав следующую тестовую программу:

int main()
{
  enum class FunctionKinds { ENTRY, EXIT, TAIL, LOG_ARGS_ENTER, CUSTOM_EVENT };
  static constexpr FunctionKinds Kinds[] = {
    FunctionKinds::ENTRY, FunctionKinds::EXIT, FunctionKinds::TAIL,
    FunctionKinds::LOG_ARGS_ENTER, FunctionKinds::CUSTOM_EVENT
  };
  std::cout << sizeof(Kinds) << std::endl;
  return 0;
}

Собираем с помощью компилятора Visual C++, запускаем и видим число "20".

Получается, что защита от выхода за границу массива не работает. Чтобы исправить код, нужно сравнивать Kind не с размером массива в байтах, а с количеством элементом массива.

Корректная проверка:

if (Kind >= sizeof(Kinds) / sizeof(Kinds[0]))
  return errorCodeToError(....);

Баг N12, ошибка инициализации массива

enum CondCode {
  // Opcode       N U L G E       Intuitive operation
  SETFALSE, //      0 0 0 0       Always false (always folded)
  SETOEQ,   //      0 0 0 1       True if ordered and equal
  ....
  SETCC_INVALID // Marker value.
};

static void InitCmpLibcallCCs(ISD::CondCode *CCs) {
  memset(CCs, ISD::SETCC_INVALID, sizeof(ISD::CondCode)*RTLIB::UNKNOWN_LIBCALL);
  ....
}

Предупреждение PVS-Studio: V575 [CWE-628, CERT-EXP37-C] The 'memset' function processes the pointer to enum type. Inspect the first argument. TargetLoweringBase.cpp 662

Код будет работать только в том случае, если повезёт и элементы перечисления CondCode будут представлены одним байтом.

Функция memset заполняет массив байт. В каждый байт записывается значение SETCC_INVALID. Если enum представлен 4 байтами, как это происходит в случае сборки с помощью Visual C++, массив будет заполнен бессмысленными значениями. Эти значения будут равны результату повторения константы в каждом из 4 байт:

SETCC_INVALID << 24 | SETCC_INVALID << 16 | SETCC_INVALID << 8 | SETCC_INVALID

Правильный вариант заполнения массива:

std::fill(CCs, CCs + RTLIB::UNKNOWN_LIBCALL, ISD::SETCC_INVALID);

Ошибки в потоке управления

Баг N13-N14, неинициализированная переменная

Expected<std::pair<JITTargetAddress, Edge::Kind>>
EHFrameEdgeFixer::readEncodedPointer(uint8_t PointerEncoding,
                                     JITTargetAddress PointerFieldAddress,
                                     BinaryStreamReader &RecordReader) {
  .....
  Edge::Kind PointerEdgeKind;

  switch (EffectiveType) {
  case DW_EH_PE_udata4: {
    ....
    PointerEdgeKind = Delta32;
    break;
  }
  case DW_EH_PE_udata8: {
    ....
    PointerEdgeKind = Delta64;
    break;
  }
  case DW_EH_PE_sdata4: {
    ....
    PointerEdgeKind = Delta32;
    break;
  }
  case DW_EH_PE_sdata8: {
    ....
    PointerEdgeKind = Delta64;
    break;
  }
  }

  if (PointerEdgeKind == Edge::Invalid)
    return make_error<JITLinkError>(
        "Unspported edge kind for encoded pointer at " +
        formatv("{0:x}", PointerFieldAddress));

  return std::make_pair(Addr, Delta64);
}

Предупреждение PVS-Studio: V614 [CWE-457, CERT-EXP53-CPP] Potentially uninitialized variable 'PointerEdgeKind' used. EHFrameSupport.cpp 704

Переменная PointerEdgeKind может остаться неинициализированной после выполнения switch-блока. Однако если переменная ничем не инициализировалась, то ожидается, что она равна именованной константе Edge::Invalid.

Следует сразу при объявлении переменной инициализировать её этой константой:

Edge::Kind PointerEdgeKind = Edge::Invalid;

Ещё одна такая ошибка: V614 [CWE-457, CERT-EXP53-CPP] Potentially uninitialized variable 'Result' used. llvm-rtdyld.cpp 998

Баг N15, недостижимый код

В начале рассмотрим вспомогательную функцию report_fatal_error:

void llvm::report_fatal_error(const Twine &Reason, bool GenCrashDiag) {
  ....
  abort();
}

Для нас важно то, что она прекращает выполнение программы с помощью вызова функции abort. Т. е. функция report_fatal_error никогда не возвращает управление.

Ещё есть вот такая промежуточная функция, вызов которой мы рассмотрим далее:

void llvm::report_fatal_error(const char *Reason, bool GenCrashDiag) {
  report_fatal_error(Twine(Reason), GenCrashDiag);
}

Примечание. Аргумент GenCrashDiag является необязательным:

__declspec(noreturn) void report_fatal_error(const char *reason, 
                                                bool gen_crash_diag = true);

Кстати, сейчас подумал, что тело функции можно было и не рассматривать. Уже из аннотации функции __declspec(noreturn) понятно, что она не возвращает управление. Но решил оставить как есть, чтобы объяснить ситуацию максимально подробно.

Теперь к сути. Рассмотрим вот этот фрагмент кода:

int AMDGPUCFGStructurizer::improveSimpleJumpintoIf(....)
{
  ....
  if (LandBlkHasOtherPred) {
    report_fatal_error("Extra register needed to handle CFG");
    Register CmpResReg =
        HeadMBB->getParent()->getRegInfo().createVirtualRegister(I32RC);
    report_fatal_error("Extra compare instruction needed to handle CFG");
    insertCondBranchBefore(LandBlk, I, R600::IF_PREDICATE_SET,
        CmpResReg, DebugLoc());
  }
  ....
}

Предупреждение PVS-Studio: V779 [CWE-561, CERT-MSC12-C] Unreachable code detected. It is possible that an error is present. AMDILCFGStructurizer.cpp 1286

Обратите внимание, что после вызова функции report_fatal_error программа ещё пытается что-то делать. Все эти действия уже не имеют смысла.

Есть подозрение, что автор кода вовсе не планировал полностью прекратить выполнение программы, а просто хотел сообщить об ошибке. Возможно, нужно было использовать какую-то другую функцию для логирования информации о проблеме.

Баг N16-N17, утечка памяти

uint64_t WinCOFFObjectWriter::writeObject(MCAssembler &Asm,
                                          const MCAsmLayout &Layout) {
  ....
  if (EmitAddrsigSection) {
    auto Frag = new MCDataFragment(AddrsigSection);
    Frag->setLayoutOrder(0);
    raw_svector_ostream OS(Frag->getContents());
    for (const MCSymbol *S : AddrsigSyms) {
      if (!S->isTemporary()) {
        encodeULEB128(S->getIndex(), OS);
        continue;
      }

      MCSection *TargetSection = &S->getSection();
      assert(SectionMap.find(TargetSection) != SectionMap.end() &&
             "Section must already have been defined in "
             "executePostLayoutBinding!");
      encodeULEB128(SectionMap[TargetSection]->Symbol->getIndex(), OS);
    }
  }
  ....
}

Предупреждение PVS-Studio: V773 [CWE-401, CERT-MEM31-C, CERT-MEM51-CPP] Visibility scope of the 'Frag' pointer was exited without releasing the memory. A memory leak is possible. WinCOFFObjectWriter.cpp 1116

Возможно, я чего-то не понимаю и это не ошибка. Но я не вижу, где и как может быть удалён объект, на который ссылается указатель Frag. Я согласен с анализатором: это очень похоже на утечку памяти.

Аналогичная ситуация: V773 [CWE-401, CERT-MEM31-C, CERT-MEM51-CPP] Visibility scope of the 'Frag' pointer was exited without releasing the memory. A memory leak is possible. WinCOFFObjectWriter.cpp 1130

Код с запахом

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

Код с запахом N1, дубликаты строк

static uint16_t toSecMapFlags(uint32_t Flags) {
  uint16_t Ret = 0;
  if (Flags & COFF::IMAGE_SCN_MEM_READ)
    Ret |= static_cast<uint16_t>(OMFSegDescFlags::Read);
  if (Flags & COFF::IMAGE_SCN_MEM_WRITE)
    Ret |= static_cast<uint16_t>(OMFSegDescFlags::Write);
  if (Flags & COFF::IMAGE_SCN_MEM_EXECUTE)
    Ret |= static_cast<uint16_t>(OMFSegDescFlags::Execute);
  if (Flags & COFF::IMAGE_SCN_MEM_EXECUTE)
    Ret |= static_cast<uint16_t>(OMFSegDescFlags::Execute);
  if (!(Flags & COFF::IMAGE_SCN_MEM_16BIT))
    Ret |= static_cast<uint16_t>(OMFSegDescFlags::AddressIs32Bit);
  ....
}

Предупреждение PVS-Studio: V581 [CWE-670] The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 335, 337. DbiStreamBuilder.cpp 337

Вот этот фрагмент повторяется два раза:

if (Flags & COFF::IMAGE_SCN_MEM_EXECUTE)
  Ret |= static_cast<uint16_t>(OMFSegDescFlags::Execute);

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

Код с запахом N2, атавизм

std::string pathname_;
....
void FilePath::Normalize() {
  if (pathname_.c_str() == nullptr) {
    pathname_ = "";
    return;
  }
....
}

Предупреждение PVS-Studio: V547 [CWE-570] Expression 'pathname_.c_str() == nullptr' is always false. gtest-filepath.cc 349

Если удалить всё содержимое функции, то ничего не изменится. Она всё равно ничего не делает. Похоже на артефакт нескольких последовательных рефакторингов.

Код с запахом N3, не там поставлена скобка

raw_ostream &raw_ostream::write_escaped(StringRef Str,
                                        bool UseHexEscapes) {
  ....
  *this << hexdigit((c >> 4 & 0xF));
  *this << hexdigit((c >> 0) & 0xF);
  ....
}

Предупреждение PVS-Studio: V592 The expression was enclosed by parentheses twice: '((c >> 4 & 0xF))'. One pair of parentheses is unnecessary or misprint is present. raw_ostream.cpp 188

В первой строчке используются двойные скобки. Эта избыточность скобок говорит, что хотели написать выражение как-то по-другому. И действительно, как хотели написать на самом деле видно в следующей строке. Становится понятно, что скобки хотели использовать для упрощения чтения выражения.

Задумывалось написать такой код:

*this << hexdigit((c >> 4) & 0xF);
*this << hexdigit((c >> 0) & 0xF);

Хотя скобка поставлена не там, это не ошибка. Всё равно приоритет сдвига (>>) выше, чем приоритет двоичного И (&). Всё вычисляется корректно.

Код с запахом N4-N6, неудачное слияние кода?

template <class ELFT>
void ELFState<ELFT>::writeSectionContent(
    Elf_Shdr &SHeader, const ELFYAML::StackSizesSection &Section,
    ContiguousBlobAccumulator &CBA) {
  if (!Section.Entries)
    return;

  if (!Section.Entries)
    return;
  ....
}

Предупреждение PVS-Studio: V581 [CWE-670] The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 1380, 1383. ELFEmitter.cpp 1383

Это похоже на какое-то неудачное слияние двух веток кода, из-за которого возникли дублирующиеся строки. Не ошибка, но стоит удалить дубликат.

Вот ещё схожие фрагменты с дубликатами кода:

  • V581 [CWE-670] The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 1488, 1491. ELFEmitter.cpp 1491
  • V581 [CWE-670] The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 1663, 1666. ELFEmitter.cpp 1666

Заключение

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

Раз PVS-Studio способен находить ошибки даже в таких хорошо оттестированных приложениях, как компиляторы, то есть смысл посмотреть, что он сможет найти в ваших проектах :). Предлагаю, не откладывая попробовать триальную версию анализатора. Спасибо за внимание.

@onehellsus
October 11, 2021, 06:05
0 reactions
0 views