Находим баги в LLVM 8 с помощью анализатора PVS-Studio

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

Статья, которая должна быть написана

Если честно, мне не хотелось писать эту статью. Неинтересно писать про проект, который мы уже неоднократно проверяли (1, 2, 3). Лучше написать про что-то новое, но у меня нет выбора.

Каждый раз, когда выходит новая версия LLVM или обновляется Clang Static Analyzer, у нас в почте появляются вопросы следующего типа:

Смотрите, новая версия Clang Static Analyzer научилась находить новые ошибки! Мне кажется, актуальность использовать PVS-Studio уменьшается. Clang находит больше ошибок, чем раньше и догоняет по возможностям PVS-Studio. Что вы про это думаете?

На это мне всегда хочется ответить что-то в духе:

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

К сожалению, это плохой ответ. В нём нет proof-ов. И именно поэтому сейчас я пишу эту статью. Итак, проект LLVM в очередной раз проверен и в нём найдены разнообразнейшие ошибки. Те, которые мне показались интересными, я сейчас продемонстрирую. Эти ошибки не может найти Clang Static Analyzer (или это крайне неудобно делать с его помощью). А мы можем. Причём я нашел и выписал все эти ошибки за один вечер.

А вот написание статьи затянулось на несколько недель. Никак не мог себя заставить всё это оформить в виде текста :).

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

Новые и старые диагностики

Как уже было отмечено, около двух лет назад проект LLVM был в очередной раз проверен, а найденные ошибки исправлены. Теперь в этой статье будет представлена новая порция ошибок. Почему были найдены новые ошибки? На это есть 3 причины:

1. Проект LLVM развивается, в нём изменяется старый код, и появляется новый. Естественно в изменённом и написанном коде есть новые ошибки. Это хорошо демонстрирует, что статический анализ должен применяться регулярно, а не от случая к случаю. Наши статьи хорошо показывают возможности анализатора PVS-Studio, но это не имеет ничего общего с повышением качества кода и снижением стоимости исправления ошибок. Используйте статический анализатор кода регулярно!
2. Мы дорабатываем и усовершенствуем уже существующие диагностики. Поэтому анализатор может выявить ошибки, которые не замечал при предыдущих проверках.
3. В PVS-Studio появились новые диагностики, которых не было 2 года назад. Я решил выделить их в отдельный раздел, чтобы наглядно показать развитие PVS-Studio.

Дефекты, выявленные диагностиками, существовавшими 2 года назад

Фрагмент N1: Copy-Paste

static bool ShouldUpgradeX86Intrinsic(Function *F, StringRef Name) {
  if (Name == "addcarryx.u32" || // Added in 8.0
    ....
    Name == "avx512.mask.cvtps2pd.128" || // Added in 7.0
    Name == "avx512.mask.cvtps2pd.256" || // Added in 7.0
    Name == "avx512.cvtusi2sd" || // Added in 7.0
    Name.startswith("avx512.mask.permvar.") || // Added in 7.0     // <=
    Name.startswith("avx512.mask.permvar.") || // Added in 7.0     // <=
    Name == "sse2.pmulu.dq" || // Added in 7.0
    Name == "sse41.pmuldq" || // Added in 7.0
    Name == "avx2.pmulu.dq" || // Added in 7.0
  ....
}

Предупреждение PVS-Studio: V501 [CWE-570] There are identical sub-expressions ‘Name.startswith(«avx512.mask.permvar.»)’ to the left and to the right of the ‘||’ operator. AutoUpgrade.cpp 73

Дважды проверяется, что имя начинается с подстроки «avx512.mask.permvar.». Во второй проверке явно хотели написать что-то ещё, но забыли исправить скопированный текст.

Фрагмент N2: Опечатка

enum CXNameRefFlags {
  CXNameRange_WantQualifier = 0x1,
  CXNameRange_WantTemplateArgs = 0x2,
  CXNameRange_WantSinglePiece = 0x4
};

void AnnotateTokensWorker::HandlePostPonedChildCursor(
    CXCursor Cursor, unsigned StartTokenIndex) {
  const auto flags = CXNameRange_WantQualifier | CXNameRange_WantQualifier;
  ....
}

Предупреждение PVS-Studio: V501 There are identical sub-expressions ‘CXNameRange_WantQualifier’ to the left and to the right of the ‘|’ operator. CIndex.cpp 7245

Из-за опечатки дважды используется одна и та же именованная константа CXNameRange_WantQualifier.

Фрагмент N3: Путаница с приоритетами операторов

int PPCTTIImpl::getVectorInstrCost(unsigned Opcode, Type *Val, unsigned Index) {
  ....
  if (ISD == ISD::EXTRACT_VECTOR_ELT && Index == ST->isLittleEndian() ? 1 : 0)
    return 0;
  ....
}

Предупреждение PVS-Studio: V502 [CWE-783] Perhaps the ‘?:’ operator works in a different way than it was expected. The ‘?:’ operator has a lower priority than the ‘==’ operator. PPCTargetTransformInfo.cpp 404

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

Сейчас, согласно приоритетам операторов, выражение вычисляется следующим образом:

(ISD == ISD::EXTRACT_VECTOR_ELT && (Index == ST->isLittleEndian())) ? 1 : 0

С практической точки зрения такое условие не имеет смысла, так как его можно сократить до:

(ISD == ISD::EXTRACT_VECTOR_ELT && Index == ST->isLittleEndian())

Это явная ошибка. Скорее всего, 0/1 хотели сравнить с переменной Index. Чтобы исправить код необходимо добавить скобки вокруг тернарного оператора:

if (ISD == ISD::EXTRACT_VECTOR_ELT && Index == (ST->isLittleEndian() ? 1 : 0))

Кстати, тернарный оператор очень опасный и провоцирует логические ошибки. Будьте очень аккуратны с ним и не жадничайте ставить круглые скобки. Подробнее эту тему я рассматривал здесь, в главе «Бойтесь оператора ?: и заключайте его в круглые скобки».

Фрагмент N4, N5: Нулевой указатель

Init *TGParser::ParseValue(Record *CurRec, RecTy *ItemType, IDParseMode Mode) {
  ....
  TypedInit *LHS = dyn_cast<TypedInit>(Result);
  ....
  LHS = dyn_cast<TypedInit>(
    UnOpInit::get(UnOpInit::CAST, LHS, StringRecTy::get())
      ->Fold(CurRec));
  if (!LHS) {
    Error(PasteLoc, Twine("can't cast '") + LHS->getAsString() +
                    "' to string");
    return nullptr;
  }
  ....
}

Предупреждение PVS-Studio: V522 [CWE-476] Dereferencing of the null pointer ‘LHS’ might take place. TGParser.cpp 2152

Если указатель LHS окажется нулевым, то должно быть выдано предупреждение. Однако вместо этого произойдёт разыменование этого самого нулевого указателя: LHS->getAsString().

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

Аналогичная ошибка обработки указателя RHS допущена в коде чуть ниже: V522 [CWE-476] Dereferencing of the null pointer ‘RHS’ might take place. TGParser.cpp 2186

Фрагмент N6: Использование указателя после перемещения

static Expected<bool>
ExtractBlocks(....)
{
  ....
  std::unique_ptr<Module> ProgClone = CloneModule(BD.getProgram(), VMap);
  ....
  BD.setNewProgram(std::move(ProgClone));                                // <=
  MiscompiledFunctions.clear();

  for (unsigned i = 0, e = MisCompFunctions.size(); i != e; ++i) {
    Function *NewF = ProgClone->getFunction(MisCompFunctions[i].first);  // <=
    assert(NewF && "Function not found??");
    MiscompiledFunctions.push_back(NewF);
  }
  ....
}

Предупреждение PVS-Studio: V522 [CWE-476] Dereferencing of the null pointer ‘ProgClone’ might take place. Miscompilation.cpp 601

В начале умный указатель ProgClone перестаёт владеть объектом:

BD.setNewProgram(std::move(ProgClone));

Фактически, теперь ProgClone — это нулевой указатель. Поэтому чуть ниже должно произойти разыменование нулевого указателя:

Function *NewF = ProgClone->getFunction(MisCompFunctions[i].first);

Но, на самом деле, этого не произойдёт! Обратите внимание, что цикл на самом деле не выполняется.

В начале контейнер MiscompiledFunctions очищается:

MiscompiledFunctions.clear();

Далее размер этого контейнера используется в условии цикла:

for (unsigned i = 0, e = MisCompFunctions.size(); i != e; ++i) {

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

Кажется, мы встретили ту самую знаменитую чётность ошибок! Одна ошибка маскирует другую :).

Фрагмент N7: Использование указателя после перемещения

static Expected<bool> TestOptimizer(BugDriver &BD, std::unique_ptr<Module> Test,
                                    std::unique_ptr<Module> Safe) {
  outs() << "  Optimizing functions being tested: ";
  std::unique_ptr<Module> Optimized =
      BD.runPassesOn(Test.get(), BD.getPassesToRun());
  if (!Optimized) {
    errs() << " Error running this sequence of passes"
           << " on the input program!n";
    BD.setNewProgram(std::move(Test));                       // <=
    BD.EmitProgressBitcode(*Test, "pass-error", false);      // <=
    if (Error E = BD.debugOptimizerCrash())
      return std::move(E);
    return false;
  }
  ....
}

Предупреждение PVS-Studio: V522 [CWE-476] Dereferencing of the null pointer ‘Test’ might take place. Miscompilation.cpp 709

Вновь та же самая ситуация. В начале содержимое объекта перемещается, а затем он используется как ни в чём не бывало. Я всё чаще встречаю эту ситуацию в коде программ, после того, как в С++ появилась семантика перемещения. За это я и люблю язык C++! Появляются всё новые и новые способы отстрелить себе ногу. Анализатору PVS-Studio всегда будет работа :).

Фрагмент N8: Нулевой указатель

void FunctionDumper::dump(const PDBSymbolTypeFunctionArg &Symbol) {
  uint32_t TypeId = Symbol.getTypeId();
  auto Type = Symbol.getSession().getSymbolById(TypeId);
  if (Type)
    Printer << "<unknown-type>";
  else
    Type->dump(*this);
}

Предупреждение PVS-Studio: V522 [CWE-476] Dereferencing of the null pointer ‘Type’ might take place. PrettyFunctionDumper.cpp 233

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

Правильно:

if (Type)
  Type->dump(*this);
else
  Printer << "<unknown-type>";

Фрагмент N9: Нулевой указатель

void SearchableTableEmitter::collectTableEntries(
    GenericTable &Table, const std::vector<Record *> &Items) {
  ....
  RecTy *Ty = resolveTypes(Field.RecType, TI->getType());
  if (!Ty)                                                              // <=
    PrintFatalError(Twine("Field '") + Field.Name + "' of table '" +
                    Table.Name + "' has incompatible type: " +
                    Ty->getAsString() + " vs. " +                       // <=
                    TI->getType()->getAsString());
   ....
}

Предупреждение PVS-Studio: V522 [CWE-476] Dereferencing of the null pointer ‘Ty’ might take place. SearchableTableEmitter.cpp 614

Думаю, и так всё понятно и не требует пояснений.

Фрагмент N10: Опечатка

bool FormatTokenLexer::tryMergeCSharpNullConditionals() {
  ....
  auto &Identifier = *(Tokens.end() - 2);
  auto &Question = *(Tokens.end() - 1);
  ....
  Identifier->ColumnWidth += Question->ColumnWidth;
  Identifier->Type = Identifier->Type;                    // <=
  Tokens.erase(Tokens.end() - 1);
  return true;
}

Предупреждение PVS-Studio: V570 The ‘Identifier->Type’ variable is assigned to itself. FormatTokenLexer.cpp 249

Нет смысла присваивать переменную саму себе. Скорее всего хотели написать:

Identifier->Type = Question->Type;

Фрагмент N11: Подозрительный break

void SystemZOperand::print(raw_ostream &OS) const {
  switch (Kind) {
    break;
  case KindToken:
    OS << "Token:" << getToken();
    break;
  case KindReg:
    OS << "Reg:" << SystemZInstPrinter::getRegisterName(getReg());
    break;
  ....
}

Предупреждение PVS-Studio: V622 [CWE-478] Consider inspecting the ‘switch’ statement. It’s possible that the first ‘case’ operator is missing. SystemZAsmParser.cpp 652

В начале присутствует очень подозрительный оператор break. Не забыли ли здесь написать что-то ещё?

Фрагмент N12: Проверка указателя после разыменования

InlineCost AMDGPUInliner::getInlineCost(CallSite CS) {
  Function *Callee = CS.getCalledFunction();
  Function *Caller = CS.getCaller();
  TargetTransformInfo &TTI = TTIWP->getTTI(*Callee);

  if (!Callee || Callee->isDeclaration())
    return llvm::InlineCost::getNever("undefined callee");
  ....
}

Предупреждение PVS-Studio: V595 [CWE-476] The ‘Callee’ pointer was utilized before it was verified against nullptr. Check lines: 172, 174. AMDGPUInline.cpp 172

Указатель Callee в начале разыменовывается в момент вызова функции getTTI.

А затем оказывается, что этот указатель следует проверять на равенство nullptr:

if (!Callee || Callee->isDeclaration())

Но уже поздно…

Фрагмент N13 — N…: Проверка указателя после разыменования

Ситуация, рассмотренная в предыдущем фрагменте кода не уникальна. Она встречается здесь:

static Value *optimizeDoubleFP(CallInst *CI, IRBuilder<> &B,
                               bool isBinary, bool isPrecise = false) {
  ....
  Function *CalleeFn = CI->getCalledFunction();
  StringRef CalleeNm = CalleeFn->getName();                 // <=
  AttributeList CalleeAt = CalleeFn->getAttributes();
  if (CalleeFn && !CalleeFn->isIntrinsic()) {               // <=
  ....
}

Предупреждение PVS-Studio: V595 [CWE-476] The ‘CalleeFn’ pointer was utilized before it was verified against nullptr. Check lines: 1079, 1081. SimplifyLibCalls.cpp 1079

И здесь:

void Sema::InstantiateAttrs(const MultiLevelTemplateArgumentList &TemplateArgs,
                            const Decl *Tmpl, Decl *New,
                            LateInstantiatedAttrVec *LateAttrs,
                            LocalInstantiationScope *OuterMostScope) {
  ....
  NamedDecl *ND = dyn_cast<NamedDecl>(New);
  CXXRecordDecl *ThisContext =
    dyn_cast_or_null<CXXRecordDecl>(ND->getDeclContext());         // <=
  CXXThisScopeRAII ThisScope(*this, ThisContext, Qualifiers(),
                             ND && ND->isCXXInstanceMember());     // <=
  ....
}

Предупреждение PVS-Studio: V595 [CWE-476] The ‘ND’ pointer was utilized before it was verified against nullptr. Check lines: 532, 534. SemaTemplateInstantiateDecl.cpp 532

И здесь:

• V595 [CWE-476] The ‘U’ pointer was utilized before it was verified against nullptr. Check lines: 404, 407. DWARFFormValue.cpp 404
• V595 [CWE-476] The ‘ND’ pointer was utilized before it was verified against nullptr. Check lines: 2149, 2151. SemaTemplateInstantiate.cpp 2149

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

Фрагмент N17, N18: Подозрительный сдвиг

static inline bool processLogicalImmediate(uint64_t Imm, unsigned RegSize,
                                           uint64_t &Encoding) {
  ....
  unsigned Size = RegSize;
  ....
  uint64_t NImms = ~(Size-1) << 1;
  ....
}

Предупреждение PVS-Studio: V629 [CWE-190] Consider inspecting the ‘~(Size — 1) << 1’ expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. AArch64AddressingModes.h 260

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

Допустим, переменная Size равна 16, и тогда автор кода планировал получить в переменной NImms значение:

1111111111111111111111111111111111111111111111111111111111100000

Однако, на самом деле, получится значение:

0000000000000000000000000000000011111111111111111111111111100000

Дело в том, что все вычисления происходят с использованием 32-битного типа unsigned. И только затем, этот 32-битный беззнаковый тип будет неявно расширен до uint64_t. При этом старшие биты окажется нулевыми.

Исправить ситуацию можно так:

uint64_t NImms = ~static_cast<uint64_t>(Size-1) << 1;

Аналогичная ситуация: V629 [CWE-190] Consider inspecting the ‘Immr << 6’ expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. AArch64AddressingModes.h 269

Фрагмент N19: Пропущено ключевое слово else?

void AMDGPUAsmParser::cvtDPP(MCInst &Inst, const OperandVector &Operands) {
  ....
  if (Op.isReg() && Op.Reg.RegNo == AMDGPU::VCC) {
    // VOP2b (v_add_u32, v_sub_u32 ...) dpp use "vcc" token.
    // Skip it.
    continue;
  } if (isRegOrImmWithInputMods(Desc, Inst.getNumOperands())) {    // <=
    Op.addRegWithFPInputModsOperands(Inst, 2);
  } else if (Op.isDPPCtrl()) {
    Op.addImmOperands(Inst, 1);
  } else if (Op.isImm()) {
    // Handle optional arguments
    OptionalIdx[Op.getImmTy()] = I;
  } else {
    llvm_unreachable("Invalid operand type");
  }
  ....
}

Предупреждение PVS-Studio: V646 [CWE-670] Consider inspecting the application’s logic. It’s possible that ‘else’ keyword is missing. AMDGPUAsmParser.cpp 5655

Ошибки здесь нет. Так как then-блок первого if оканчивается на continue, то не важно, есть ключевое слово else или нет. В любом случае код будет работать одинаково. Тем не менее, пропущенный else делает код более непонятным и опасным. Если в дальнейшем continue исчезнет, то код начнёт работать совсем по-другому. На мой взгляд, лучше добавить else.

Фрагмент N20: Четыре однотипных опечатки

LLVM_DUMP_METHOD void Symbol::dump(raw_ostream &OS) const {
  std::string Result;
  if (isUndefined())
    Result += "(undef) ";
  if (isWeakDefined())
    Result += "(weak-def) ";
  if (isWeakReferenced())
    Result += "(weak-ref) ";
  if (isThreadLocalValue())
    Result += "(tlv) ";
  switch (Kind) {
  case SymbolKind::GlobalSymbol:
    Result + Name.str();                        // <=
    break;
  case SymbolKind::ObjectiveCClass:
    Result + "(ObjC Class) " + Name.str();      // <=
    break;
  case SymbolKind::ObjectiveCClassEHType:
    Result + "(ObjC Class EH) " + Name.str();   // <=
    break;
  case SymbolKind::ObjectiveCInstanceVariable:
    Result + "(ObjC IVar) " + Name.str();       // <=
    break;
  }
  OS << Result;
}

Предупреждения PVS-Studio:

• V655 [CWE-480] The strings were concatenated but are not utilized. Consider inspecting the ‘Result + Name.str()’ expression. Symbol.cpp 32
• V655 [CWE-480] The strings were concatenated but are not utilized. Consider inspecting the ‘Result + «(ObjC Class) » + Name.str()’ expression. Symbol.cpp 35
• V655 [CWE-480] The strings were concatenated but are not utilized. Consider inspecting the ‘Result + «(ObjC Class EH) » + Name.str()’ expression. Symbol.cpp 38
• V655 [CWE-480] The strings were concatenated but are not utilized. Consider inspecting the ‘Result + «(ObjC IVar) » + Name.str()’ expression. Symbol.cpp 41

Случайно вместо оператора += используется оператор +. В результате получаются конструкции, лишённые смысла.

Фрагмент N21: Неопределённое поведение

static void getReqFeatures(std::map<StringRef, int> &FeaturesMap,
                           const std::vector<Record *> &ReqFeatures) {
  for (auto &R : ReqFeatures) {
    StringRef AsmCondString = R->getValueAsString("AssemblerCondString");

    SmallVector<StringRef, 4> Ops;
    SplitString(AsmCondString, Ops, ",");
    assert(!Ops.empty() && "AssemblerCondString cannot be empty");

    for (auto &Op : Ops) {
      assert(!Op.empty() && "Empty operator");
      if (FeaturesMap.find(Op) == FeaturesMap.end())
        FeaturesMap[Op] = FeaturesMap.size();
    }
  }
}

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

Предупреждение PVS-Studio: V708 [CWE-758] Dangerous construction is used: ‘FeaturesMap[Op] = FeaturesMap.size()’, where ‘FeaturesMap’ is of ‘map’ class. This may lead to undefined behavior. RISCVCompressInstEmitter.cpp 490

Проблемная строчка:

FeaturesMap[Op] = FeaturesMap.size();

Если элемент Op не найден, то создаётся новый элемент в карте и туда записывается количество элементов в этой карте. Вот только неизвестно, будет вызвана функция size до или после добавления нового элемента.

Фрагмент N22-N24: Повторные присваивания

Error MachOObjectFile::checkSymbolTable() const {
  ....
  } else {
    MachO::nlist STE = getSymbolTableEntry(SymDRI);
    NType = STE.n_type;                              // <=
    NType = STE.n_type;                              // <=
    NSect = STE.n_sect;
    NDesc = STE.n_desc;
    NStrx = STE.n_strx;
    NValue = STE.n_value;
  }
  ....
}

Предупреждение PVS-Studio: V519 [CWE-563] The ‘NType’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1663, 1664. MachOObjectFile.cpp 1664

Думаю, настоящей ошибки здесь нет. Просто лишнее повторяющееся присваивание. Но всё равно ляп.

Аналогично:

• V519 [CWE-563] The ‘B.NDesc’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1488, 1489. llvm-nm.cpp 1489
• V519 [CWE-563] The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 59, 61. coff2yaml.cpp 61

Фрагмент N25-N27: Ещё повторные присваивания

Теперь рассмотрим немного другой вариант повторного присваивания.

bool Vectorizer::vectorizeLoadChain(
    ArrayRef<Instruction *> Chain,
    SmallPtrSet<Instruction *, 16> *InstructionsProcessed) {
  ....
  unsigned Alignment = getAlignment(L0);
  ....
  unsigned NewAlign = getOrEnforceKnownAlignment(L0->getPointerOperand(),
                                                 StackAdjustedAlignment,
                                                 DL, L0, nullptr, &DT);
  if (NewAlign != 0)
    Alignment = NewAlign;
  Alignment = NewAlign;
  ....
}

Предупреждение PVS-Studio: V519 [CWE-563] The ‘Alignment’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1158, 1160. LoadStoreVectorizer.cpp 1160

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

Аналогичные ситуации можно увидеть здесь:

• V519 [CWE-563] The ‘Effects’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 152, 165. WebAssemblyRegStackify.cpp 165
• V519 [CWE-563] The ‘ExpectNoDerefChunk’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 4970, 4973. SemaType.cpp 4973

Фрагмент N28: Всегда истинное условие

static int readPrefixes(struct InternalInstruction* insn) {
  ....
  uint8_t byte = 0;
  uint8_t nextByte;
  ....
  if (byte == 0xf3 && (nextByte == 0x88 || nextByte == 0x89 ||
                       nextByte == 0xc6 || nextByte == 0xc7)) {
    insn->xAcquireRelease = true;
    if (nextByte != 0x90) // PAUSE instruction support             // <=
      break;
  }
  ....
}

Предупреждение PVS-Studio: V547 [CWE-571] Expression ‘nextByte != 0x90’ is always true. X86DisassemblerDecoder.cpp 379

Проверка не имеет смысла. Переменная nextByte всегда не равна значению 0x90, что вытекает из предыдущей проверки. Это какая-то логическая ошибка.

Фрагмент N29 — N…: Всегда истинные/ложные условия

Анализатор выдаёт много предупреждений о том, что всё условие (V547) или его часть (V560) всегда истинно или ложно. Часто это не настоящие ошибки, а просто неаккуратный код, результат развертывания макросов и тому подобное. Тем не менее, есть смысл посмотреть все эти предупреждения, так как время от времени встречаются настоящие логические ошибки. Например, подозрителен вот этот участок кода:

static DecodeStatus DecodeGPRPairRegisterClass(MCInst &Inst, unsigned RegNo,
                                   uint64_t Address, const void *Decoder) {
  DecodeStatus S = MCDisassembler::Success;

  if (RegNo > 13)
    return MCDisassembler::Fail;

  if ((RegNo & 1) || RegNo == 0xe)
     S = MCDisassembler::SoftFail;
  ....
}

Предупреждение PVS-Studio: V560 [CWE-570] A part of conditional expression is always false: RegNo == 0xe. ARMDisassembler.cpp 939

Константа 0xE это значение 14 в десятичной системе. Проверка RegNo == 0xe не имеет смысла, так как если RegNo > 13, то функция завершит своё выполнение.

Было множество других предупреждений с идентификатором V547 и V560, но, как и в случае с V595, изучать эти предупреждения мне было неинтересно. Было и так понятно, что мне хватит материала для написания статьи :). Поэтому неизвестно, сколько всего можно выявить ошибок этого типа в LLVM с помощью PVS-Studio.

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

bool UnwrappedLineParser::parseBracedList(bool ContinueOnSemicolons,
                                          tok::TokenKind ClosingBraceKind) {
  bool HasError = false;
  ....
  HasError = true;
  if (!ContinueOnSemicolons)
    return !HasError;
  ....
}

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

Фрагмент N30: Подозрительный return

static bool
isImplicitlyDef(MachineRegisterInfo &MRI, unsigned Reg) {
  for (MachineRegisterInfo::def_instr_iterator It = MRI.def_instr_begin(Reg),
      E = MRI.def_instr_end(); It != E; ++It) {
    return (*It).isImplicitDef();
  }
  ....
}

Предупреждение PVS-Studio: V612 [CWE-670] An unconditional ‘return’ within a loop. R600OptimizeVectorRegisters.cpp 63

Это или ошибка, или специфический приём, который призван что-то пояснить программистам, читающим код. Мне такая конструкция ничего не поясняет и выглядит очень подозрительной. Лучше так не писать :).

Устали? Тогда время заварить чай или кофе.

Дефекты, выявленные новыми диагностиками

Думаю, 30 срабатываний старых диагностик достаточно. Давайте теперь посмотрим, что интересного можно находить новыми диагностиками, которые появились в анализаторе уже после предыдущей проверки. Всего за это время в C++ анализаторе добавилось 66 диагностик общего назначения.

Фрагмент N31: Недостижимый код

Error CtorDtorRunner::run() {
  ....
  if (auto CtorDtorMap =
          ES.lookup(JITDylibSearchList({{&JD, true}}), std::move(Names),
                    NoDependenciesToRegister, true))
  {
    ....
    return Error::success();
  } else
    return CtorDtorMap.takeError();

  CtorDtorsByPriority.clear();

  return Error::success();
}

Предупреждение PVS-Studio: V779 [CWE-561] Unreachable code detected. It is possible that an error is present. ExecutionUtils.cpp 146

Как видите, обе ветки оператора if заканчиваются вызовом оператора return. Соответственно, контейнер CtorDtorsByPriority никогда не будет очищен.

Фрагмент N32: Недостижимый код

bool LLParser::ParseSummaryEntry() {
  ....
  switch (Lex.getKind()) {
  case lltok::kw_gv:
    return ParseGVEntry(SummaryID);
  case lltok::kw_module:
    return ParseModuleEntry(SummaryID);
  case lltok::kw_typeid:
    return ParseTypeIdEntry(SummaryID);                        // <=
    break;                                                     // <=
  default:
    return Error(Lex.getLoc(), "unexpected summary kind");
  }
  Lex.setIgnoreColonInIdentifiers(false);                      // <=
  return false;
}

Предупреждение PVS-Studio: V779 [CWE-561] Unreachable code detected. It is possible that an error is present. LLParser.cpp 835

Интересная ситуация. Давайте рассмотрим в начале вот это место:

return ParseTypeIdEntry(SummaryID);
break;

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

Анализатор выдаёт предупреждение на строчки:

Lex.setIgnoreColonInIdentifiers(false);
return false;

И действительно, этот код недостижим. Все случаи в switch заканчиваются вызовом оператором return. И теперь бессмысленный одинокий break не выглядит таким безобидным! Быть может одна из веток должна заканчиваться на break, а не на return?

Фрагмент N33: Случайное обнуление старших бит

unsigned getStubAlignment() override {
  if (Arch == Triple::systemz)
    return 8;
  else
    return 1;
}

Expected<unsigned>
RuntimeDyldImpl::emitSection(const ObjectFile &Obj,
                             const SectionRef &Section,
                             bool IsCode) {
  ....
  uint64_t DataSize = Section.getSize();
  ....
  if (StubBufSize > 0)
    DataSize &= ~(getStubAlignment() - 1);
  ....
}

Предупреждение PVS-Studio: V784 The size of the bit mask is less than the size of the first operand. This will cause the loss of higher bits. RuntimeDyld.cpp 815

Обратите внимание, что функция getStubAlignment возвращает тип unsigned. Вычислим значение выражения, если предположить, что функция вернёт значение 8:

~(getStubAlignment() — 1)

~(8u-1)

0xFFFFFFF8‬u

Теперь обратите внимание, что переменная DataSize имеет 64-битный беззнаковый тип. Получается, что при выполнении операции DataSize & 0xFFFFFFF8‬u все тридцать два старших бита будут обнулены. Скорее всего, это не то, что хотел программист. Подозреваю, что он хотел вычислить: DataSize & 0xFFFFFFFFFFFFFFF8‬u.

Чтобы исправить ошибку, следует написать так:

DataSize &= ~(static_cast<uint64_t>(getStubAlignment()) - 1);

Или так:

DataSize &= ~(getStubAlignment() - 1ULL);

Фрагмент N34: Неудачное явное приведение типа

template <typename T>
void scaleShuffleMask(int Scale, ArrayRef<T> Mask,
                      SmallVectorImpl<T> &ScaledMask) {
  assert(0 < Scale && "Unexpected scaling factor");
  int NumElts = Mask.size();
  ScaledMask.assign(static_cast<size_t>(NumElts * Scale), -1);
  ....
}

Предупреждение PVS-Studio: V1028 [CWE-190] Possible overflow. Consider casting operands of the ‘NumElts * Scale’ operator to the ‘size_t’ type, not the result. X86ISelLowering.h 1577

Явное приведение типа используется для того, чтобы не возникло переполнение при перемножении переменных типа int. Однако, здесь явное приведение типа не защищает от переполнения. В начале переменные будут перемножены, и только потом 32-битный результат умножения будет расширен до типа size_t.

Фрагмент N35: Неудачный Copy-Paste

Instruction *InstCombiner::visitFCmpInst(FCmpInst &I) {
  ....
  if (!match(Op0, m_PosZeroFP()) && isKnownNeverNaN(Op0, &TLI)) {
    I.setOperand(0, ConstantFP::getNullValue(Op0->getType()));
    return &I;
  }
  if (!match(Op1, m_PosZeroFP()) && isKnownNeverNaN(Op1, &TLI)) {
    I.setOperand(1, ConstantFP::getNullValue(Op0->getType()));        // <=
    return &I;
  }
  ....
}

V778 [CWE-682] Two similar code fragments were found. Perhaps, this is a typo and ‘Op1’ variable should be used instead of ‘Op0’. InstCombineCompares.cpp 5507

Эта новая интересная диагностика выявляет ситуации, когда фрагмент кода был скопирован, и в нём начали менять некоторые имена, но в одном месте не поправили.

Обратите внимание, что во втором блоке меняли Op0 на Op1. Но в одном месте не поправили. Скорее всего, должно было быть написано так:

if (!match(Op1, m_PosZeroFP()) && isKnownNeverNaN(Op1, &TLI)) {
  I.setOperand(1, ConstantFP::getNullValue(Op1->getType()));
  return &I;
}

Фрагмент N36: Путаница в переменных

struct Status {
  unsigned Mask;
  unsigned Mode;

  Status() : Mask(0), Mode(0){};

  Status(unsigned Mask, unsigned Mode) : Mask(Mask), Mode(Mode) {
    Mode &= Mask;
  };
  ....
};

Предупреждение PVS-Studio: V1001 [CWE-563] The ‘Mode’ variable is assigned but is not used by the end of the function. SIModeRegister.cpp 48

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

Mode &= Mask;

Меняется аргумент функции. И всё. Этот аргумент больше никак не используется. Скорее всего, надо было написать так:

Status(unsigned Mask, unsigned Mode) : Mask(Mask), Mode(Mode) {
  this->Mode &= Mask;
};

Фрагмент N37: Путаница в переменных

class SectionBase {
  ....
  uint64_t Size = 0;
  ....
};

class SymbolTableSection : public SectionBase {
  ....
};

void SymbolTableSection::addSymbol(Twine Name, uint8_t Bind, uint8_t Type,
                                   SectionBase *DefinedIn, uint64_t Value,
                                   uint8_t Visibility, uint16_t Shndx,
                                   uint64_t Size) {
  ....
  Sym.Value = Value;
  Sym.Visibility = Visibility;
  Sym.Size = Size;
  Sym.Index = Symbols.size();
  Symbols.emplace_back(llvm::make_unique<Symbol>(Sym));
  Size += this->EntrySize;
}

Предупреждение PVS-Studio: V1001 [CWE-563] The ‘Size’ variable is assigned but is not used by the end of the function. Object.cpp 424

Ситуация аналогична предыдущей. Должно быть написано:

this->Size += this->EntrySize;

Фрагмент N38-N47: Указатель забыли проверить

Ранее мы рассматривали примеры срабатывания диагностики V595. Её суть в том, что указатель в начале разыменовывается, а только потом проверяется. Молодая диагностика V1004 является обратной ей по смыслу, но также обнаруживает очень много ошибок. Она выявляет ситуации, когда указатель в начале проверяли, а затем забыли это сделать. Рассмотрим такие случаи, найденные внутри LLVM.

int getGEPCost(Type *PointeeType, const Value *Ptr,
               ArrayRef<const Value *> Operands) {
  ....
  if (Ptr != nullptr) {                                            // <=
    assert(....);
    BaseGV = dyn_cast<GlobalValue>(Ptr->stripPointerCasts());
  }
  bool HasBaseReg = (BaseGV == nullptr);

  auto PtrSizeBits = DL.getPointerTypeSizeInBits(Ptr->getType());  // <=
  ....
}

Предупреждение PVS-Studio: V1004 [CWE-476] The ‘Ptr’ pointer was used unsafely after it was verified against nullptr. Check lines: 729, 738. TargetTransformInfoImpl.h 738

Переменная Ptr может быть равна nullptr, о чём свидетельствует проверка:

if (Ptr != nullptr)

Однако, ниже этот указатель разыменовывается уже без предварительной проверки:

auto PtrSizeBits = DL.getPointerTypeSizeInBits(Ptr->getType());

Рассмотрим другой аналогичный случай.

llvm::DISubprogram *CGDebugInfo::getFunctionFwdDeclOrStub(GlobalDecl GD,
                                                          bool Stub) {
  ....
  auto *FD = dyn_cast<FunctionDecl>(GD.getDecl());
  SmallVector<QualType, 16> ArgTypes;
  if (FD)                                                                // <=
    for (const ParmVarDecl *Parm : FD->parameters())
      ArgTypes.push_back(Parm->getType());
  CallingConv CC = FD->getType()->castAs<FunctionType>()->getCallConv(); // <=
  ....
}

Предупреждение PVS-Studio: V1004 [CWE-476] The ‘FD’ pointer was used unsafely after it was verified against nullptr. Check lines: 3228, 3231. CGDebugInfo.cpp 3231

Обратите внимание на указатель FD. Уверен, проблема хорошо видна, и специальных пояснений не требуется.

И ещё:

static void computePolynomialFromPointer(Value &Ptr, Polynomial &Result,
                                         Value *&BasePtr,
                                         const DataLayout &DL) {
  PointerType *PtrTy = dyn_cast<PointerType>(Ptr.getType());
  if (!PtrTy) {                                                   // <=
    Result = Polynomial();
    BasePtr = nullptr;
  }
  unsigned PointerBits =
      DL.getIndexSizeInBits(PtrTy->getPointerAddressSpace());     // <=
  ....
}

Предупреждение PVS-Studio: V1004 [CWE-476] The ‘PtrTy’ pointer was used unsafely after it was verified against nullptr. Check lines: 960, 965. InterleavedLoadCombinePass.cpp 965

Как защититься от таких ошибок? Будьте внимательней на Code-Review и используйте для регулярной проверки кода статический анализатор PVS-Studio.

Приводить другие фрагменты кода с ошибками данного вида смысла нет. Оставлю в статье только список предупреждений:

• V1004 [CWE-476] The ‘Expr’ pointer was used unsafely after it was verified against nullptr. Check lines: 1049, 1078. DebugInfoMetadata.cpp 1078
• V1004 [CWE-476] The ‘PI’ pointer was used unsafely after it was verified against nullptr. Check lines: 733, 753. LegacyPassManager.cpp 753
• V1004 [CWE-476] The ‘StatepointCall’ pointer was used unsafely after it was verified against nullptr. Check lines: 4371, 4379. Verifier.cpp 4379
• V1004 [CWE-476] The ‘RV’ pointer was used unsafely after it was verified against nullptr. Check lines: 2263, 2268. TGParser.cpp 2268
• V1004 [CWE-476] The ‘CalleeFn’ pointer was used unsafely after it was verified against nullptr. Check lines: 1081, 1096. SimplifyLibCalls.cpp 1096
• V1004 [CWE-476] The ‘TC’ pointer was used unsafely after it was verified against nullptr. Check lines: 1819, 1824. Driver.cpp 1824

Фрагмент N48-N60: Не критично, но дефект (возможна утечка памяти)

std::unique_ptr<IRMutator> createISelMutator() {
  ....
  std::vector<std::unique_ptr<IRMutationStrategy>> Strategies;
  Strategies.emplace_back(
      new InjectorIRStrategy(InjectorIRStrategy::getDefaultOps()));
  ....
}

Предупреждение PVS-Studio: V1023 [CWE-460] A pointer without owner is added to the ‘Strategies’ container by the ’emplace_back’ method. A memory leak will occur in case of an exception. llvm-isel-fuzzer.cpp 58

Для добавлении элемента в конец контейнера типа std::vector<std::unique_ptr<X>> нельзя просто написать xxx.push_back(new X), так как нет неявного преобразования из X* в std::unique_ptr<X>.

Распространенным решением является написание xxx.emplace_back(new X), так как он компилируется: метод emplace_back конструирует элемент непосредственно из аргументов и поэтому может использовать явные конструкторы.

Это небезопасно. Если вектор полон, то происходит перевыделение памяти. Операция перевыделения памяти может закончиться неудачей, в результате чего будет сгенерировано исключение std::bad_alloc. В этом случае указатель будет потерян, и созданный объект никогда не будет удален.

Безопасным решением является создание unique_ptr, который будет владеть указателем до того, как вектор попытается перевыделить память:

xxx.push_back(std::unique_ptr<X>(new X))

Начиная с C++14, можно использовать ‘std::make_unique’:

xxx.push_back(std::make_unique<X>())

Данный вид дефекта не критичен для LLVM. Если не удастся выделить память, то работа компилятора будет просто остановлена. Однако, для приложений с долгим аптаймом, которые не могут просто так завершаться, если не удалось выделить память, это может быть настоящей неприятной ошибкой.

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

Другие предупреждения данного типа:

• V1023 [CWE-460] A pointer without owner is added to the ‘Passes’ container by the ’emplace_back’ method. A memory leak will occur in case of an exception. PassManager.h 546
• V1023 [CWE-460] A pointer without owner is added to the ‘AAs’ container by the ’emplace_back’ method. A memory leak will occur in case of an exception. AliasAnalysis.h 324
• V1023 [CWE-460] A pointer without owner is added to the ‘Entries’ container by the ’emplace_back’ method. A memory leak will occur in case of an exception. DWARFDebugFrame.cpp 519
• V1023 [CWE-460] A pointer without owner is added to the ‘AllEdges’ container by the ’emplace_back’ method. A memory leak will occur in case of an exception. CFGMST.h 268
• V1023 [CWE-460] A pointer without owner is added to the ‘VMaps’ container by the ’emplace_back’ method. A memory leak will occur in case of an exception. SimpleLoopUnswitch.cpp 2012
• V1023 [CWE-460] A pointer without owner is added to the ‘Records’ container by the ’emplace_back’ method. A memory leak will occur in case of an exception. FDRLogBuilder.h 30
• V1023 [CWE-460] A pointer without owner is added to the ‘PendingSubmodules’ container by the ’emplace_back’ method. A memory leak will occur in case of an exception. ModuleMap.cpp 810
• V1023 [CWE-460] A pointer without owner is added to the ‘Objects’ container by the ’emplace_back’ method. A memory leak will occur in case of an exception. DebugMap.cpp 88
• V1023 [CWE-460] A pointer without owner is added to the ‘Strategies’ container by the ’emplace_back’ method. A memory leak will occur in case of an exception. llvm-isel-fuzzer.cpp 60
• V1023 [CWE-460] A pointer without owner is added to the ‘Modifiers’ container by the ’emplace_back’ method. A memory leak will occur in case of an exception. llvm-stress.cpp 685
• V1023 [CWE-460] A pointer without owner is added to the ‘Modifiers’ container by the ’emplace_back’ method. A memory leak will occur in case of an exception. llvm-stress.cpp 686
• V1023 [CWE-460] A pointer without owner is added to the ‘Modifiers’ container by the ’emplace_back’ method. A memory leak will occur in case of an exception. llvm-stress.cpp 688
• V1023 [CWE-460] A pointer without owner is added to the ‘Modifiers’ container by the ’emplace_back’ method. A memory leak will occur in case of an exception. llvm-stress.cpp 689
• V1023 [CWE-460] A pointer without owner is added to the ‘Modifiers’ container by the ’emplace_back’ method. A memory leak will occur in case of an exception. llvm-stress.cpp 690
• V1023 [CWE-460] A pointer without owner is added to the ‘Modifiers’ container by the ’emplace_back’ method. A memory leak will occur in case of an exception. llvm-stress.cpp 691
• V1023 [CWE-460] A pointer without owner is added to the ‘Modifiers’ container by the ’emplace_back’ method. A memory leak will occur in case of an exception. llvm-stress.cpp 692
• V1023 [CWE-460] A pointer without owner is added to the ‘Modifiers’ container by the ’emplace_back’ method. A memory leak will occur in case of an exception. llvm-stress.cpp 693
• V1023 [CWE-460] A pointer without owner is added to the ‘Modifiers’ container by the ’emplace_back’ method. A memory leak will occur in case of an exception. llvm-stress.cpp 694
• V1023 [CWE-460] A pointer without owner is added to the ‘Operands’ container by the ’emplace_back’ method. A memory leak will occur in case of an exception. GlobalISelEmitter.cpp 1911
• V1023 [CWE-460] A pointer without owner is added to the ‘Stash’ container by the ’emplace_back’ method. A memory leak will occur in case of an exception. GlobalISelEmitter.cpp 2100
• V1023 [CWE-460] A pointer without owner is added to the ‘Matchers’ container by the ’emplace_back’ method. A memory leak will occur in case of an exception. GlobalISelEmitter.cpp 2702

Заключение

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

Надеюсь, вам было интересно, и вы захотите попробовать анализатор PVS-Studio.

Вы можете скачать анализатор и получить тральный ключ на этой странице.

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

Удачи в улучшении качества и надёжности кода!

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Finding Bugs in LLVM 8 with PVS-Studio.

Источник: habr.com