ProHoster > Блог > Администриране > Намираме бъгове в LLVM 8 с помощта на анализатора PVS-Studio

Намираме бъгове в LLVM 8 с помощта на анализатора PVS-Studio

Намираме бъгове в 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. Така че не се тревожете, ние продължаваме да водим, както и преди.

За съжаление, това е лош отговор. В него няма доказателства. И именно затова в момента пиша тази статия. И така, проектът LLVM отново е проверен и в него са открити разнообразни грешки. Тези, които ми се сториха интересни, ще демонстрирам сега. Тези грешки не може да бъдат открити от Clang Static Analyzer (или е изключително неудобно да се направи с него). А ние можем. Освен това намерих и записах всички тези грешки за един вечер.

А написването на статията отне няколко седмици. Никак не можех да се накарам да оформя всичко в текст :).

Между другото, ако ви интересува какви технологии се използват в анализатора PVS-Studio за откриване на грешки и потенциални уязвимости, предлагам да се запознаете с този запис.

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

Както вече беше отбелязано, около две години преди проекта LLVM отново беше проверен, а откритите грешки бяха коригирани. Сега в тази статия ще бъде представена нова партида грешки. Защо бяха намерени нови грешки? Има три причини:

  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" || // Добавено в 8.0
    ....
    Name == "avx512.mask.cvtps2pd.128" || // Добавено в 7.0
    Name == "avx512.mask.cvtps2pd.256" || // Добавено в 7.0
    Name == "avx512.cvtusi2sd" || // Добавено в 7.0
    Name.startswith("avx512.mask.permvar.") || // Добавено в 7.0     // <=
    Name.startswith("avx512.mask.permvar.") || // Добавено в 7.0     // <=
    Name == "sse2.pmulu.dq" || // Добавено в 7.0
    Name == "sse41.pmuldq" || // Добавено в 7.0
    Name == "avx2.pmulu.dq" || // Добавено в 7.0
  ....
}

Предупреждение PVS-Studio: V501 [CWE-570] Има идентични подизразления 'Name.startswith("avx512.mask.permvar.")' отляво и отдясно на оператора '||'. 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 Има идентични подизразления 'CXNameRange_WantQualifier' отляво и отдясно на оператора '|'. 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] Може би операторът '?:' действа по различен начин, отколкото се очаква. Операторът '?:' има по-нисък приоритет от оператора '=='. PPCTargetTransformInfo.cpp 404

На мен ми изглежда, че това е много красив грешка. Да, знам, че имам странни представи за красота :) .

Сега, в съответствие с приоритетите на операторите, изразът се изчислява по следния начин:

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

От практическа гледна точка, това условие няма смисъл, тъй като може да бъде опростено до:

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

Това е явна грешка. Най-вероятно, 0/1 е искало да се сравни с променливата Индекс. За да се поправи кодът, е необходимо да се добавят скоби около тернарния оператор:

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(Result);
  ....
  LHS = dyn_cast(
    UnOpInit::get(UnOpInit::CAST, LHS, StringRecTy::get())
      ->Fold(CurRec));
  if (!LHS) {
    Error(PasteLoc, Twine("не може да се преобразува '") + LHS->getAsString() +
                    "' в низ");
    return nullptr;
  }
  ....
}

Предупреждение PVS-Studio: V522 [CWE-476] Разыменованието на нулевия указател ‘LHS’ може да се извърши. TGParser.cpp 2152

Ако указателят LHS окажe нулев, трябва да бъде издадено предупреждение. Въпреки това, вместо това ще се извърши разыменованието на този нулев указател: LHS->getAsString().

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

Подобна грешка в обработката на указателя RHS е допусната в кода малко по-долу: V522 [CWE-476] Разыменованието на нулевия указател ‘RHS’ може да се извърши. TGParser.cpp 2186

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

static Expected
ExtractBlocks(....)
{
  ....
  std::unique_ptr ProgClone = CloneModule(BD.getProgram(), VMap);
  ....
  BD.setNewProgram(std::move(ProgClone));                                // getFunction(MisCompFunctions[i].first);  // <=
    assert(NewF && "Функцията не е намерена??");
    MiscompiledFunctions.push_back(NewF);
  }
  ....
}

Предупреждение PVS-Studio: V522 [CWE-476] Разыменованието на нулевия указател ‘ProgClone’ може да се извърши. 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 TestOptimizer(BugDriver &BD, std::unique_ptr Test,
                                    std::unique_ptr Safe) {
  outs() << "  Оптимизиране на тестваните функции: ";
  std::unique_ptr Optimized =
      BD.runPassesOn(Test.get(), BD.getPassesToRun());
  if (!Optimized) {
    errs() << " Грешка при изпълнение на тази последователност от пропуски"
           << " върху входната програма!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] Може да се извърши разименуване на нулевия указател ‘Test’. Miscompilation.cpp 709

Отново същата ситуация. В началото съдържанието на обекта се премества, а след това той се използва, сякаш нищо не е било. Все по-често срещам тази ситуация в кода на програмите, след като в C++ беше въведена семантиката на преместването. Заради това обичам C++! Появяват се все нови и нови начини да си простреляте крака. Анализаторът PVS-Studio винаги ще има работа :).

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

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

Предупреждение PVS-Studio: V522 [CWE-476] Може да се извърши разименуване на нулевия указател ‘Type’. PrettyFunctionDumper.cpp 233

Освен обработчиците на грешки, обикновено не се тестват и функции за отстраняване на грешки. Пред нас е точно такъв случай. Функцията очаква потребител, който, вместо да решава проблемите си, ще трябва да се заеме с нейното коригиране.

Правилно:

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

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

void SearchableTableEmitter::collectTableEntries(
    GenericTable &Table, const std::vector &Items) {
  ....
  RecTy *Ty = resolveTypes(Field.RecType, TI->getType());
  if (!Ty)                                                              // getAsString() + " vs. " +                       // getType()->getAsString());
   ....
}

Предупреждение PVS-Studio: V522 [CWE-476] Разыменование нулевого указателя 'Ty' может произойти. 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 Переменная 'Identifier->Type' присваивается самой себе. 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] Рассмотрите возможность проверки оператора 'switch'. Возможно, отсутствует первый оператор 'case'. 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("неопределённый вызываемый");
  ....
}

Предупреждение PVS-Studio: V595 [CWE-476] Указатель 'Callee' был использован до его проверки на nullptr. Проверьте строки: 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();                 // getAttributes();
  if (CalleeFn && !CalleeFn->isIntrinsic()) {               // <=
  ....
}

Предупреждение PVS-Studio: V595 [CWE-476] Указатель 'CalleeFn' был использован до его проверки на nullptr. Проверьте строки: 1079, 1081. SimplifyLibCalls.cpp 1079

И здесь:

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

Предупреждение PVS-Studio: V595 [CWE-476] Указатель ‘ND’ был использован до того, как был проверен на nullptr. Проверьте строки: 532, 534. SemaTemplateInstantiateDecl.cpp 532

И здесь:

  • V595 [CWE-476] Указатель ‘U’ был использован до того, как был проверен на nullptr. Проверьте строки: 404, 407. DWARFFormValue.cpp 404
  • V595 [CWE-476] Указатель ‘ND’ был использован до того, как был проверен на nullptr. Проверьте строки: 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] Рассмотрите возможность проверки выражения ‘~(Size — 1) << 1’. Сдвиг битов 32-битного значения с последующим расширением до 64-битного типа. AArch64AddressingModes.h 260

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

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

1111111111111111111111111111111111111111111111111111111111100000

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

0000000000000000000000000000000011111111111111111111111111100000

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

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

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

Аналогичная ситуация: V629 [CWE-190] Рассмотрите возможность проверки выражения ‘Immr << 6’. Сдвиг битов 32-битного значения с последующим расширением до 64-битного типа. AArch64AddressingModes.h 269

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

void AMDGPUAsmParser::cvtDPP(MCInst &Inst, const OperandVector &Operands) {
  ....
  if (Op.isReg() && Op.Reg.RegNo == AMDGPU::VCC) {
    // VOP2b (v_add_u32, v_sub_u32 ...) dpp использует токен "vcc".
    // Пропуск.
    continue;
  } if (isRegOrImmWithInputMods(Desc, Inst.getNumOperands())) {    // <=
    Op.addRegWithFPInputModsOperands(Inst, 2);
  } else if (Op.isDPPCtrl()) {
    Op.addImmOperands(Inst, 1);
  } else if (Op.isImm()) {
    // Обработка необязательных аргументов
    OptionalIdx[Op.getImmTy()] = I;
  } else {
    llvm_unreachable("Недопустимый тип операнда");
  }
  ....
}

Предупреждение PVS-Studio: V646 [CWE-670] Рассмотрите возможность проверки логики приложения. Возможно, отсутствует ключевое слово 'else'. AMDGPUAsmParser.cpp 5655

Ошибки здесь нет. Так как then-блок первого ако оканчивается на продължи, но не е важно, има ли ключова дума иначе или не. Във всеки случай кодът ще работи по един и същи начин. Въпреки това, пропуснатото иначе прави кода по-малко разбираем и по-опасен. Ако в последствие продължи изчезне, кодът ще започне да работи съвсем по различен начин. Според мен е по-добре да добавим иначе.

Фрагмент 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] Стрингите са съединени, но не се използват. Помислете за проверка на израза 'Result + Name.str()'. Symbol.cpp 32
  • V655 [CWE-480] Стрингите са съединени, но не се използват. Помислете за проверка на израза 'Result + "(ObjC Class) " + Name.str()'. Symbol.cpp 35
  • V655 [CWE-480] Стрингите са съединени, но не се използват. Помислете за проверка на израза 'Result + "(ObjC Class EH) " + Name.str()'. Symbol.cpp 38
  • V655 [CWE-480] Стрингите са съединени, но не се използват. Помислете за проверка на израза 'Result + "(ObjC IVar) " + Name.str()'. Symbol.cpp 41

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

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

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

    SmallVector Ops;
    SplitString(AsmCondString, Ops, ",");
    assert(!Ops.empty() && "AssemblerCondString не може да бъде празен");

    for (auto &Op : Ops) {
      assert(!Op.empty() && "Празен оператор");
      if (FeaturesMap.find(Op) == FeaturesMap.end())
        FeaturesMap[Op] = FeaturesMap.size();
    }
  }
}

Опитайте сами да намерите опасен код. А това е картинка за отвличане на вниманието, за да не погледнете веднага отговора:

Намираме бъгове в LLVM 8 с помощта на анализатора PVS-Studio

Предупреждение PVS-Studio: V708 [CWE-758] Използвана е опасна конструкция: 'FeaturesMap[Op] = FeaturesMap.size()', където 'FeaturesMap' е от клас 'map'. Това може да доведе до неопределено поведение. RISCVCompressInstEmitter.cpp 490

Проблемният ред:

FeaturesMap[Op] = FeaturesMap.size();

Ако елементът Op не бъде намерен, нов елемент се създава в картата и там се записва броят на елементите в тази карта. Само че не е ясно дали функцията размер ще бъде извикана преди или след добавяне на новия елемент.

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

Грешка MachOObjectFile::checkSymbolTable() const {
  ....
  } иначе {
    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] Променливата ‘NType’ е присвоена стойности два пъти последователно. Вероятно е грешка. Проверете редове: 1663, 1664. MachOObjectFile.cpp 1664

Мисля, че тук няма истинска грешка. Просто е излишно повтарящо се присвояване. Но все пак е недомислица.

Същото:

  • V519 [CWE-563] Променливата ‘B.NDesc’ е присвоена стойности два пъти последователно. Вероятно е грешка. Проверете редове: 1488, 1489. llvm-nm.cpp 1489
  • V519 [CWE-563] Променливата е присвоена стойности два пъти последователно. Вероятно е грешка. Проверете редове: 59, 61. coff2yaml.cpp 61

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

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

bool Vectorizer::vectorizeLoadChain(
    ArrayRef Chain,
    SmallPtrSet *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] Променливата ‘Alignment’ е присвоена стойности два пъти последователно. Вероятно е грешка. Проверете редове: 1158, 1160. LoadStoreVectorizer.cpp 1160

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

Аналогични ситуации могат да се видят тук:

  • V519 [CWE-563] Променливата ‘Effects’ е присвоена стойности два пъти последователно. Вероятно е грешка. Проверете редове: 152, 165. WebAssemblyRegStackify.cpp 165
  • V519 [CWE-563] Променливата ‘ExpectNoDerefChunk’ е присвоена стойности два пъти последователно. Вероятно е грешка. Проверете редове: 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             // <=
      break;
  }
  ....
}

Предупреждение PVS-Studio: V547 [CWE-571] Изразът ‘nextByte != 0x90’ винаги е верен. 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] Част от условно изразяване винаги е вярно: 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] Изразът ‘!HasError’ винаги е грешен. 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] Некондиционен ‘return’ в цикъл. R600OptimizeVectorRegisters.cpp 63

Това или е грешка, или специфична техника, която е предназначена да обясни нещо на програмистите, четящи кода. Тази конструкция не ми обяснява нищо и изглежда много подозрителна. По-добре е да не се пише така :).

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

Намираме бъгове в LLVM 8 с помощта на анализатора PVS-Studio

Дефекти, установени от новите диагностики

Мисля, че 30 срабатываний на стари диагностики са достатъчни. Нека сега да видим какво интересно можем да открием с новите диагностики, които вече са добавени в анализатора след предишната проверка. През това време в C++ анализатора бяха добавени 66 диагностични инструмента за общо предназначение.

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

Грешка 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] Засечен недостижим код. Възможно е да има грешка. ExecutionUtils.cpp 146

Както виждате, и двете клонове на оператора ако завършват с извикване на оператора 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(), "неочакван вид резюме");
  }
  Lex.setIgnoreColonInIdentifiers(false);                      
  return false;
}

Предупреждение PVS-Studio: V779 [CWE-561] Засечен недостижим код. Възможно е да има грешка. 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
RuntimeDyldImpl::emitSection(const ObjectFile &Obj,
                             const SectionRef &Section,
                             bool IsCode) {
  ....
  uint64_t DataSize = Section.getSize();
  ....
  if (StubBufSize > 0)
    DataSize &= ~(getStubAlignment() - 1);
  ....
}

Предупреждение PVS-Studio: V784 Размерът на битовата маска е по-малък от размера на първия операнд. Това ще доведе до загуба на висши битове. RuntimeDyld.cpp 815

Обърнете внимание, че функцията getStubAlignment върща тип unsigned. Нека изчислим стойността на израза, ако приемем, че функцията ще върне стойност 8:

~(getStubAlignment() — 1)

~(8u-1)

0xFFFFFFF8‬u

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

За да поправите грешката, трябва да напишете така:

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

Или така:

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

Фрагмент N34: Неуспешно явно преобразуване на типа

template <typename T>
void scaleShuffleMask(int Scale, ArrayRef<T> Mask,
                      SmallVectorImpl<T> &ScaledMask) {
  assert(0 < Scale && "Неочакван фактор на мащабиране");
  int NumElts = Mask.size();
  ScaledMask.assign(static_cast<size_t>(NumElts * Scale), -1);
  ....
}

Предупреждение PVS-Studio: V1028 [CWE-190] Възможно преливане. Обмислете да преобразувате операнденти на оператора ‘NumElts * Scale’ към типа ‘size_t’, а не резултата. 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] Намерени са два сходни кода. Може би, това е грешка и променливата ‘Op1’ трябва да бъде използвана вместо ‘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] Променливата ‘Mode’ е присвоена, но не е използвана до края на функцията. 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(Sym));
  Size += this->EntrySize;
}

Предупреждение PVS-Studio: V1001 [CWE-563] Променливата 'Size' е присвоена, но не е използвана до края на функцията. Object.cpp 424

Ситуацията е аналогична на предишната. Трябва да бъде написано:

this->Size += this->EntrySize;

Фрагмент N38-N47: Указателят не е проверен.

По-рано разгледахме примери за диагностика. V595. Същността му е, че указателят първо се разименува, а след това се проверява. Млада диагностика. V1004 има обратен смисъл, но също така открива много грешки. Тя идентифицира ситуации, в които указателят първо е проверен, а след това забравен. Нека разгледаме такива случаи, открити в LLVM.

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

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

Предупреждение PVS-Studio: V1004 [CWE-476] Указателят 'Ptr' е използван безопасно след проверка за nullptr. Проверете редовете: 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(GD.getDecl());
  SmallVector ArgTypes;
  if (FD)                                                                // parameters())
      ArgTypes.push_back(Parm->getType());
  CallingConv CC = FD->getType()->castAs()->getCallConv(); // <=
  ....
}

Предупреждение PVS-Studio: V1004 [CWE-476] Указателят 'FD' е използван безопасно след проверка за nullptr. Проверете редовете: 3228, 3231. CGDebugInfo.cpp 3231

Обърнете внимание на указателя FD. Сигурен съм, че проблемът е добре видим и не изисква специални обяснения.

И още:

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

Предупреждение PVS-Studio: V1004 [CWE-476] Указатель ‘PtrTy’ использовался небезопасно после проверки на nullptr. Проверьте строки: 960, 965. InterleavedLoadCombinePass.cpp 965

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

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

  • V1004 [CWE-476] Указатель ‘Expr’ использовался небезопасно после проверки на nullptr. Проверьте строки: 1049, 1078. DebugInfoMetadata.cpp 1078
  • V1004 [CWE-476] Указатель ‘PI’ использовался небезопасно после проверки на nullptr. Проверьте строки: 733, 753. LegacyPassManager.cpp 753
  • V1004 [CWE-476] Указатель ‘StatepointCall’ использовался небезопасно после проверки на nullptr. Проверьте строки: 4371, 4379. Verifier.cpp 4379
  • V1004 [CWE-476] Указатель ‘RV’ использовался небезопасно после проверки на nullptr. Проверьте строки: 2263, 2268. TGParser.cpp 2268
  • V1004 [CWE-476] Указатель ‘CalleeFn’ использовался небезопасно после проверки на nullptr. Проверьте строки: 1081, 1096. SimplifyLibCalls.cpp 1096
  • V1004 [CWE-476] Указатель ‘TC’ использовался небезопасно после проверки на nullptr. Проверьте строки: 1819, 1824. Driver.cpp 1824

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

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

Предупреждение PVS-Studio: V1023 [CWE-460] Указатель без владельца добавляется в контейнер ‘Strategies’ с помощью метода ’emplace_back’. Произойдет утечка памяти в случае исключения. llvm-isel-fuzzer.cpp 58

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

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

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

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

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

Започвайки от C++14, можете да използвате 'std::make_unique':

xxx.push_back(std::make_unique())

Този тип дефект не е критичен за LLVM. Ако не може да бъде заделена памет, работата на компилятора просто ще бъде спряна. Обаче, за приложения с дълъг аптайм, които не могат просто да приключат, ако не е успяно заделена памет, това може да бъде наистина неприятна грешка.

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

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

  • V1023 [CWE-460] Указател без собственик е добавен в контейнера 'Passes' чрез метода 'emplace_back'. Ще възникне изтичане на памет в случай на изключение. PassManager.h 546
  • V1023 [CWE-460] Указател без собственик е добавен в контейнера 'AAs' чрез метода 'emplace_back'. Ще възникне изтичане на памет в случай на изключение. AliasAnalysis.h 324
  • V1023 [CWE-460] Указател без собственик е добавен в контейнера 'Entries' чрез метода 'emplace_back'. Ще възникне изтичане на памет в случай на изключение. DWARFDebugFrame.cpp 519
  • V1023 [CWE-460] Указател без собственик е добавен в контейнера 'AllEdges' чрез метода 'emplace_back'. Ще възникне изтичане на памет в случай на изключение. CFGMST.h 268
  • V1023 [CWE-460] Указател без собственик е добавен в контейнера 'VMaps' чрез метода 'emplace_back'. Ще възникне изтичане на памет в случай на изключение. SimpleLoopUnswitch.cpp 2012
  • V1023 [CWE-460] Указател без собственик е добавен в контейнера 'Records' чрез метода 'emplace_back'. Ще възникне изтичане на памет в случай на изключение. FDRLogBuilder.h 30
  • V1023 [CWE-460] Указател без собственик е добавен в контейнера 'PendingSubmodules' чрез метода 'emplace_back'. Ще възникне изтичане на памет в случай на изключение. ModuleMap.cpp 810
  • V1023 [CWE-460] Указател без собственик е добавен в контейнера 'Objects' чрез метода 'emplace_back'. Ще възникне изтичане на памет в случай на изключение. DebugMap.cpp 88
  • V1023 [CWE-460] Указател без собственик е добавен в контейнера 'Strategies' чрез метода 'emplace_back'. Ще възникне изтичане на памет в случай на изключение. llvm-isel-fuzzer.cpp 60
  • V1023 [CWE-460] Указател без собственик е добавен в контейнера 'Modifiers' чрез метода 'emplace_back'. Ще възникне изтичане на памет в случай на изключение. llvm-stress.cpp 685
  • V1023 [CWE-460] Указател без собственик е добавен в контейнера 'Modifiers' чрез метода 'emplace_back'. Ще възникне изтичане на памет в случай на изключение. llvm-stress.cpp 686
  • V1023 [CWE-460] Указател без собственик е добавен в контейнера 'Modifiers' чрез метода 'emplace_back'. Ще възникне изтичане на памет в случай на изключение. llvm-stress.cpp 688
  • V1023 [CWE-460] Указател без собственик е добавен в контейнера 'Modifiers' чрез метода 'emplace_back'. Ще възникне изтичане на памет в случай на изключение. llvm-stress.cpp 689
  • V1023 [CWE-460] Указател без собственик е добавен в контейнера 'Modifiers' чрез метода 'emplace_back'. Ще възникне изтичане на памет в случай на изключение. llvm-stress.cpp 690
  • V1023 [CWE-460] Указател без собственик е добавен в контейнера 'Modifiers' чрез метода 'emplace_back'. Ще възникне изтичане на памет в случай на изключение. llvm-stress.cpp 691
  • V1023 [CWE-460] Показател без собственик е добавен в контейнера ‘Modifiers’ чрез метода ’emplace_back’. Ще настъпи изтичане на памет в случай на изключение. llvm-stress.cpp 692
  • V1023 [CWE-460] Показател без собственик е добавен в контейнера ‘Modifiers’ чрез метода ’emplace_back’. Ще настъпи изтичане на памет в случай на изключение. llvm-stress.cpp 693
  • V1023 [CWE-460] Показател без собственик е добавен в контейнера ‘Modifiers’ чрез метода ’emplace_back’. Ще настъпи изтичане на памет в случай на изключение. llvm-stress.cpp 694
  • V1023 [CWE-460] Показател без собственик е добавен в контейнера ‘Operands’ чрез метода ’emplace_back’. Ще настъпи изтичане на памет в случай на изключение. GlobalISelEmitter.cpp 1911
  • V1023 [CWE-460] Показател без собственик е добавен в контейнера ‘Stash’ чрез метода ’emplace_back’. Ще настъпи изтичане на памет в случай на изключение. GlobalISelEmitter.cpp 2100
  • V1023 [CWE-460] Показател без собственик е добавен в контейнера ‘Matchers’ чрез метода ’emplace_back’. Ще настъпи изтичане на памет в случай на изключение. GlobalISelEmitter.cpp 2702

Заключение

Общо съм записал 60 предупреждения, след което спрях. Има ли други дефекти, които анализаторът PVS-Studio открива в LLVM? Да, има. Въпреки това, когато записвах фрагменти от кода за статията, вече беше късно вечерта, даже нощ, и реших, че е време да приключа.

Надявам се, да ви е било интересно и ще искате да опитате анализатора PVS-Studio.

Можете да изтеглите анализатора и да получите лицензионен ключ на на тази страница.

Най-важното е, че използвайте статичен анализ редовно. Еднократни проверки, извършвани от нас с цел популяризиране на методологията на статичния анализ и PVS-Studio не са нормален сценарий.

Успех в подобряването на качеството и надеждността на кода!

Намираме бъгове в LLVM 8 с помощта на анализатора PVS-Studio

Ако искате да споделите тази статия с англоговореща аудитория, моля, използвайте линка за превода: Andrey Karpov. Намиране на бъгове в LLVM 8 с помощта на PVS-Studio.

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

Купете надежден хостинг за сайтове със защита от DDoS, VPS и VDS сървъри 🔥 Купете надежден хостинг за сайтове със защита от DDoS, VPS и VDS сървъри | ProHoster