Nalezení chyb v LLVM 8 pomocí analyzátoru PVS-Studio

Uplynuly více než dva roky od poslední revize kódu projektu LLVM naším analyzátorem PVS-Studio. Pojďme se ujistit, že analyzátor PVS-Studio je stále vedoucím nástrojem pro odhalování chyb a potenciálních zranitelností. Za tímto účelem zkontrolujeme a najdeme nové chyby v vydání LLVM 8.0.0.

Článek, který má být napsán

Upřímně, nechtělo se mi psát tento článek. Není zábavné psát o projektu, který jsme již několikrát kontrolovali (1, 2, 3). Raději bych napsal něco nového, ale nemám na výběr.

Každýkrát, když vyjde nová verze LLVM nebo se aktualizuje Clang Static Analyzer, přicházejí nám do e-mailu otázky následujícího typu:

Podívejte, nová verze Clang Static Analyzer se naučila nacházet nové chyby! Myslím, že relevance používání PVS-Studio klesá. Clang nachází více chyb než dříve a přibližuje se možnostem PVS-Studio. Co si o tom myslíte?

Na to mám vždy chuť odpovědět něco v duchu:

Také nezahálíme! Výrazně jsme zlepšili možnosti analyzátoru PVS-Studio. Takže se nebojte, stále vedeme, jako dříve.

Bohužel, to je špatná odpověď. Nemá to žádné důkazy. A právě proto nyní píšu tento článek. Takže projekt LLVM byl znovu zkontrolován a našly se různé chyby. Ty, které mi přišly zajímavé, nyní ukážu. Tyto chyby nemůže najít Clang Static Analyzer (nebo je velmi obtížné to s ním dělat). A my je můžeme. A já jsem našel a popsal všechny tyto chyby za jeden večer.

Ale psaní článku zabralo několik týdnů. Nemohl jsem se donutit to vše uspořádat do textu :).

Mimochodem, pokud vás zajímá, jaké technologie se používají v analyzátoru PVS-Studio k odhalování chyb a potenciálních zranitelností, pak doporučuji se seznámit s touto poznámkou.

Nové a staré diagnostiky

Jak již bylo zmíněno, přibližně před dvěma lety byl projekt LLVM znovu zkontrolován a nalezené chyby byly opraveny. Nyní bude v tomto článku představena nová porce chyb. Proč byly nalezeny nové chyby? Na to jsou 3 důvody:

1. Projekt LLVM se vyvíjí, starý kód se mění a vzniká nový. Je přirozené, že v upraveném a novém kódu jsou nové chyby. To dobře ukazuje, že statická analýza by měla být aplikována pravidelně, nikoli jen náhodně. Naše články dobře ilustrují možnosti analyzátoru PVS-Studio, ale to nemá nic společného se zvyšováním kvality kódu a snižováním nákladů na opravy chyb. Používejte statický analyzátor kódu pravidelně!
2. O vícezlepšujeme a zdokonalujeme již existující diagnostiky. Proto může analyzátor odhalit chyby, které nebyly zjištěny při předchozích kontrolách.
3. V PVS-Studio se objevily nové diagnostiky, které zde nebyly před 2 lety. Rozhodl jsem se je vyčlenit do samostatné sekce, abych jasně ukázal vývoj PVS-Studio.

Defekty zjištěné diagnostikami, které existovaly před 2 lety

Fragment N1: Copy-Paste

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

Upozornění PVS-Studio: V501 [CWE-570] Existují identické podvýrazy ‘Name.startswith(„avx512.mask.permvar.“)’ nalevo a napravo od operátoru ‘||’. AutoUpgrade.cpp 73

Dvakrát se kontroluje, že jméno začíná podřetězcem „avx512.mask.permvar.“. Při druhé kontrole se zjevně chtělo napsat něco jiného, ale zapomnělo se opravit zkopírovaný text.

Fragment N2: Překlep

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;
  ....
}

Upozornění PVS-Studio: V501 Existují identické podvýrazy ‘CXNameRange_WantQualifier’ nalevo a napravo od operátoru ‘|’. CIndex.cpp 7245

Z důvodu překlepu je dvakrát použita stejná pojmenovaná konstanta CXNameRange_WantQualifier.

Fragment N3: Zmatek priorit operátorů

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

Upozornění PVS-Studio: V502 [CWE-783] Možná že operátor ‘?:’ funguje jiným způsobem, než se očekávalo. Operátor ‘?:’ má nižší prioritu než operátor ‘==’. PPCTargetTransformInfo.cpp 404

Považuji to za velmi krásnou chybu. Ano, vím, že mám zvláštní představy o kráse :).

V současnosti, podle priorit operátorů, se výraz počítá následovně:

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

Z praktického hlediska tato podmínka nedává smysl, protože ji lze zjednodušit na:

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

To je zřejmá chyba. Pravděpodobně se chtěli porovnat 0/1 s proměnnou Index. Aby se kód opravil, je třeba přidat závorky kolem ternárního operátoru:

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

Mimochodem, ternární operator je velmi nebezpečný a vyvolává logické chyby. Buďte velmi opatrní s ním a nešetřete na kulatých závorkách. Toto téma jsem podrobněji probíral zde, v kapitole „Bojte se operátoru ?: a uzavírejte ho do kulatých závorek“.

Fragment N4, N5: Null pointer

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("can't cast '") + LHS->getAsString() +
                    "' to string");
    return nullptr;
  }
  ....
}

Upozornění PVS-Studio: V522 [CWE-476] Dereferencing of the null pointer ‘LHS’ might take place. TGParser.cpp 2152

Pokud ukazatel LHS bude nulový, mělo by být vyvoláno varování. Místo toho dojde k dereferencování tohoto nulového ukazatele: LHS->getAsString().

To je velmi typická situace, kdy se chyba skrývá v zpracovatelské části, protože ji nikdo netestuje. Statické analyzátory kontrolují veškerý dosažitelný kód, bez ohledu na to, jak často se používá. To je velmi dobrý příklad, jak statická analýza doplňuje jiné metody testování a ochrany proti chybám.

Podobná chyba v zpracování ukazatele RHS se vyskytuje v kódu o něco níže: V522 [CWE-476] Dereferencing of the null pointer ‘RHS’ might take place. TGParser.cpp 2186

Fragment N6: Použití ukazatele po přesunutí

static Expected
ExtractBlocks(....)
{
  ....
  std::unique_ptr ProgClone = CloneModule(BD.getProgram(), VMap);
  ....
  BD.setNewProgram(std::move(ProgClone));                                // getFunction(MisCompFunctions[i].first);  // <=
    assert(NewF && "Function not found??");
    MiscompiledFunctions.push_back(NewF);
  }
  ....
}

Varování PVS-Studio: V522 [CWE-476] Dereferencing of the null pointer ‘ProgClone’ might take place. Miscompilation.cpp 601

Na začátku chytrý ukazatel ProgClone přestává vlastnit objekt:

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

V podstatě, nyní ProgClone — je to nulový ukazatel. Proto by se o něco níže mělo provést dereferencování nulového ukazatele:

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

Ale ve skutečnosti k tomu nedojde! Všimněte si, že smyčka se ve skutečnosti neprovede.

Na začátku kontejneru MiscompiledFunctions se vyčistí:

MiscompiledFunctions.clear();

Následně je velikost tohoto kontejneru použita v podmínce smyčky:

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

Je snadné vidět, že smyčka se nespustí. Myslím, že to je také chyba a kód by měl být napsán jinak.

Zdá se, že jsme narazili na onu slavnou paritu chyb! Jedna chyba zakrývá druhou :).

Fragment N7: Použití ukazatele po přesunutí

static Expected TestOptimizer(BugDriver &BD, std::unique_ptr Test,
                                    std::unique_ptr Safe) {
  outs() << "  Optimalizace funkcí, které se testují: ";
  std::unique_ptr Optimized =
      BD.runPassesOn(Test.get(), BD.getPassesToRun());
  if (!Optimized) {
    errs() << " Chyba při provádění této sekvence přechodů"
           << " na vstupním programu!n";
    BD.setNewProgram(std::move(Test));                       // <=
    BD.EmitProgressBitcode(*Test, "pass-error", false);      // <=
    if (Error E = BD.debugOptimizerCrash())
      return std::move(E);
    return false;
  }
  ....
}

Upozornění PVS-Studio: V522 [CWE-476] Může dojít k dereferencování nulového ukazatele 'Test'. Miscompilation.cpp 709

Opět stejná situace. Na začátku se obsah objektu přesune, a pak je použit jako by se nic nestalo. Čím dál tím častěji se s touto situací setkávám v kódu programů, poté, co v C++ byla zavedena semantika přesunutí. Za to miluji jazyk C++! Objevují se stále nové a nové způsoby, jak si na sebe šlápnout. Analytik PVS-Studio bude mít stále co dělat :).

Fragment N8: Nulový ukazatel

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

Upozornění PVS-Studio: V522 [CWE-476] Může dojít k dereferencování nulového ukazatele 'Type'. PrettyFunctionDumper.cpp 233

Kromě zpracovatelů chyb se obvykle netestují ani funkce pro ladění výstupních dat. To je právě tento případ. Funkce čeká na uživatele, který místo toho, aby vyřešil své problémy, bude muset řešit její chybu.

Správně:

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

Fragment N9: Nulový ukazatel

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

Upozornění PVS-Studio: V522 [CWE-476] Dereferencing of the null pointer ‘Ty’ might take place. SearchableTableEmitter.cpp 614

Myslím, že to je jasné a nevyžaduje vysvětlení.

Fragment N10: Typo

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;
}

Upozornění PVS-Studio: V570 Proměnná ‘Identifier->Type’ je přiřazena sama sobě. FormatTokenLexer.cpp 249

Není smysluplné přiřazovat proměnnou samo sobě. Pravděpodobně mělo být napsáno:

Identifier->Type = Question->Type;

Fragment N11: Podezřelý 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;
  ....
}

Upozornění PVS-Studio: V622 [CWE-478] Zvažte prozkoumání příkazu ‘switch’. Je možné, že první operátor ‘case’ chybí. SystemZAsmParser.cpp 652

Na začátku je velmi podezřelý operátor break. Nezapomněli zde něco napsat?

Fragment N12: Kontrola ukazatele po dereferenci

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");
  ....
}

Upozornění PVS-Studio: V595 [CWE-476] Ukazatel ‘Callee’ byl použit před tím, než byl ověřen proti nullptr. Zkontrolujte řádky: 172, 174. AMDGPUInline.cpp 172

Ukazatel Callee na začátku dereferencován v okamžiku volání funkce getTTI.

A pak se ukáže, že tento ukazatel by měl být ověřen na rovnost nullptr:

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

Ale už je pozdě...

Fragment N13 — N…: Kontrola ukazatele po dereferenci

Situace uvedená v předchozím fragmentu kódu není unikátní. Objevuje se zde:

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()) {               // <=
  ....
}

Upozornění PVS-Studio: V595 [CWE-476] Ukazatel ‘CalleeFn’ byl použit před tím, než byl ověřen proti nullptr. Zkontrolujte řádky: 1079, 1081. SimplifyLibCalls.cpp 1079

A zde:

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());     // <=
  ....
}

Upozornění PVS-Studio: V595 [CWE-476] Pointers ‘ND’ byly použity před kontrolou proti nullptr. Zkontrolujte řádky: 532, 534. SemaTemplateInstantiateDecl.cpp 532

A zde:

• V595 [CWE-476] Pointers ‘U’ byly použity před kontrolou proti nullptr. Zkontrolujte řádky: 404, 407. DWARFFormValue.cpp 404
• V595 [CWE-476] Pointers ‘ND’ byly použity před kontrolou proti nullptr. Zkontrolujte řádky: 2149, 2151. SemaTemplateInstantiate.cpp 2149

A pak mě přestalo bavit studovat varování s číslem V595. Takže nevím, zda existují další podobné chyby mimo tyto uvedené.

Fragment N17, N18: Podezřelý posun

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

Upozornění PVS-Studio: V629 [CWE-190] Zvažte prozkoumání výrazu ‘~(Size — 1) << 1’. Bitový posun 32bitové hodnoty s následným rozšířením na 64bitový typ. AArch64AddressingModes.h 260

Možná to není chyba a kód funguje přesně tak, jak bylo zamýšleno. Ale toto je očividně velmi podezřelé místo a mělo by být zkontrolováno.

Předpokládejme, že proměnná Velikost je rovna 16, a autor kódu plánoval získat v proměnné NImms hodnotu:

1111111111111111111111111111111111111111111111111111111111100000

Ale ve skutečnosti dostaneme hodnotu:

0000000000000000000000000000000011111111111111111111111111100000

Jde o to, že všechna výpočty probíhají s použitím 32bitového typu unsigned. A teprve poté bude tento 32bitový bez znaménka typ implicitně rozšířen na uint64_t. Při tom budou vyšší bity nulové.

Situaci lze opravit takto:

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

Podobná situace: V629 [CWE-190] Zvažte prozkoumání výrazu ‘Immr << 6’. Bitový posun 32bitové hodnoty s následným rozšířením na 64bitový typ. AArch64AddressingModes.h 269

Fragment N19: Chybí klíčové slovo 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 používá token "vcc".
    // Přeskočit.
    continue;
  } if (isRegOrImmWithInputMods(Desc, Inst.getNumOperands())) {    // <=
    Op.addRegWithFPInputModsOperands(Inst, 2);
  } else if (Op.isDPPCtrl()) {
    Op.addImmOperands(Inst, 1);
  } else if (Op.isImm()) {
    // Zpracovávat volitelné argumenty
    OptionalIdx[Op.getImmTy()] = I;
  } else {
    llvm_unreachable("Neplatný typ operandu");
  }
  ....
}

Upozornění PVS-Studio: V646 [CWE-670] Zvažte prozkoumání logiky aplikace. Je možné, že chybí klíčové slovo ‘else’. AMDGPUAsmParser.cpp 5655

Zde není chyba. Protože then-blok první if končí na continue, tak nezáleží na tom, zda je klíčové slovo else nebo ne. Každopádně kód bude fungovat stejně. Nicméně, chybějící else činí kód méně srozumitelným a nebezpečným. Pokud v budoucnu continue zmizí, kód začne fungovat úplně jinak. Podle mě je lepší přidat else.

Fragment N20: Čtyři stejné překlepy

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;
}

Upozornění PVS-Studio:

• V655 [CWE-480] Řetězce byly sloučeny, ale nejsou využity. Zvažte prozkoumání výrazu 'Result + Name.str()'. Symbol.cpp 32
• V655 [CWE-480] Řetězce byly sloučeny, ale nejsou využity. Zvažte prozkoumání výrazu 'Result + "(ObjC Class) " + Name.str()'. Symbol.cpp 35
• V655 [CWE-480] Řetězce byly sloučeny, ale nejsou využity. Zvažte prozkoumání výrazu 'Result + "(ObjC Class EH) " + Name.str()'. Symbol.cpp 38
• V655 [CWE-480] Řetězce byly sloučeny, ale nejsou využity. Zvažte prozkoumání výrazu 'Result + "(ObjC IVar) " + Name.str()'. Symbol.cpp 41

Náhodou je místo operátoru += použito operátor +. V důsledku toho vznikají konstrukce, které postrádají smysl.

Fragment N21: Neurčité chování

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 cannot be empty");

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

Zkuste najít nebezpečný kód sami. A tohle je obrázek pro rozptýlení, abyste se hned nebyli podívat na odpověď:

Upozornění PVS-Studio: V708 [CWE-758] Používá se nebezpečná konstrukce: ‘FeaturesMap[Op] = FeaturesMap.size()’, kde ‘FeaturesMap’ je třída ‘map’. To může vést k neurčitému chování. RISCVCompressInstEmitter.cpp 490

Problémový řádek:

FeaturesMap[Op] = FeaturesMap.size();

Pokud prvek Op není nalezen, je vytvořen nový prvek v mapě a do něj se zapíše počet prvků v této mapě. Jenže není známo, zda bude funkce size vyvolána před nebo po přidání nového prvku.

Fragment N22-N24: Opakovaná přiřazení

Chyba 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;
  }
  ....
}

Upozornění PVS-Studio: V519 [CWE-563] Proměnné ‘NType’ je přiřazena hodnota dvakrát za sebou. Možná se jedná o chybu. Zkontrolujte řádky: 1663, 1664. MachOObjectFile.cpp 1664

Myslím, že zde chyba není. Jen se jedná o nadbytečné opakované přiřazení. I tak je to blbost.

Podobně:

• V519 [CWE-563] Proměnné ‘B.NDesc’ je přiřazena hodnota dvakrát za sebou. Možná se jedná o chybu. Zkontrolujte řádky: 1488, 1489. llvm-nm.cpp 1489
• V519 [CWE-563] Proměnné je přiřazena hodnota dvakrát za sebou. Možná se jedná o chybu. Zkontrolujte řádky: 59, 61. coff2yaml.cpp 61

Fragment N25-N27: Další opakovaná přiřazení

Nyní se podíváme na poněkud jinou variantu opakovaného přiřazení.

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;
  ....
}

Varování PVS-Studio: V519 [CWE-563] Proměnné ‘Alignment’ je přiřazena hodnota dvakrát za sebou. Možná se jedná o chybu. Zkontrolujte řádky: 1158, 1160. LoadStoreVectorizer.cpp 1160

Toto je velmi podivný kód, který pravděpodobně obsahuje logickou chybu. Zpočátku je proměnné Alignment přiřazena hodnota v závislosti na podmínce. Poté však dochází k dalšímu přiřazení, ale již bez jakékoli kontroly.

Podobné situace lze vidět zde:

• V519 [CWE-563] Proměnné ‘Effects’ je přiřazena hodnota dvakrát za sebou. Možná se jedná o chybu. Zkontrolujte řádky: 152, 165. WebAssemblyRegStackify.cpp 165
• V519 [CWE-563] Proměnné ‘ExpectNoDerefChunk’ je přiřazena hodnota dvakrát za sebou. Možná se jedná o chybu. Zkontrolujte řádky: 4970, 4973. SemaType.cpp 4973

Fragment N28: Vždy pravdivá podmínka

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) // Podpora instrukce PAUSE             // <=
      break;
  }
  ....
}

Upozornění PVS-Studio: V547 [CWE-571] Výraz ‘nextByte != 0x90’ je vždy pravdivý. X86DisassemblerDecoder.cpp 379

Kontrola nemá smysl. Proměnná nextByte je vždy nerovná hodnotě 0x90, což vyplývá z předchozí kontroly. To je nějaká logická chyba.

Fragment N29 — N…: Vždy pravdivé/nepravdivé podmínky

Analyzátor vydává mnoho varování o tom, že celé podmínky (V547) nebo jejich část (V560) vždy pravda nebo nepravda. Často to není skutečná chyba, ale pouze nedbalý kód, výsledek nasazení makra a podobně. Nicméně má smysl podívat se na všechna tato upozornění, protože občas se objeví pravé logické chyby. Například podezřelý je tento kód:

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;
  ....
}

Upozornění PVS-Studio: V560 [CWE-570] Část podmínkového výrazu je vždy nepravdivá: RegNo == 0xe. ARMDisassembler.cpp 939

Konstanta 0xE je hodnota 14 v desítkové soustavě. Kontrola RegNo == 0xe není smysluplná, protože pokud RegNo > 13, funkce se ukončí.

Bylo mnoho dalších upozornění s identifikátory V547 a V560, ale, jako v případě s V595, zkoumat tato upozornění mě nenadchlo. Bylo jasné, že mám dostatek materiálu pro napsání článku :). Proto není známo, kolik chyb tohoto typu lze v LLVM odhalit pomocí PVS-Studio.

Uvedu příklad, proč studium těchto upozornění je nudné. Analyzátor má pravdu, když upozorňuje na následující kód. Ale není to chyba.

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

Upozornění PVS-Studio: V547 [CWE-570] Výraz ‘!HasError’ je vždy nepravdivý. UnwrappedLineParser.cpp 1635

Fragment N30: Podezřelý 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();
  }
  ....
}

Upozornění PVS-Studio: V612 [CWE-670] Nezávislý ‘return’ uvnitř smyčky. R600OptimizeVectorRegisters.cpp 63

To je buď chyba, nebo specifický trik, který má objasnit něco programátorům čtoucím kód. Mně taková konstrukce nic neobjasňuje a vypadá velmi podezřele. Lepší to tak nepřepisovat :).

Jste unaveni? Tak je čas si uvařit čaj nebo kávu.

Defekty odhalené novými diagnostikami

Myslím, že 30 výskytů starých diagnostik stačí. Pojďme se teď podívat, co zajímavého lze najít novými diagnostikami, které se v analyzátoru objevily po předchozí části, že v takovém systému se používají speciální hlavní odbočovače: kontrole. Během této doby bylo v C++ analyzátoru přidáno 66 diagnostik obecného určení.

Fragment N31: Nedosažitelný kód

Chyba 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();
}

Upozornění PVS-Studio: V779 [CWE-561] Detectován nedosažitelný kód. Je možné, že se vyskytla chyba. ExecutionUtils.cpp 146

Jak vidíte, obě větve operátoru if končí voláním operátoru vrátit. Proto kontejner CtorDtorsByPriority nikdy nebude vymazán.

Fragment N32: Nedosažitelný kód

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(), "neočekávaný typ shrnutí");
  }
  Lex.setIgnoreColonInIdentifiers(false);                      \/\/ <=
  return false;
}

Upozornění PVS-Studio: V779 [CWE-561] Detectován nedosažitelný kód. Je možné, že se vyskytla chyba. LLParser.cpp 835

Zajímavá situace. Podívejme se nejprve na toto místo:

return ParseTypeIdEntry(SummaryID);
break;

Na první pohled se zdá, že zde není chyba. Vypadá to, že operátor break je zde nadbytečný a může být prostě odstraněn. Nicméně, všechno není tak jednoduché.

Analizátor vydává varování na řádcích:

Lex.setIgnoreColonInIdentifiers(false);
return false;

A skutečně, tento kód je nedosažitelný. Všechny případy v switch končí voláním operátoru vrátit. A nyní osamělý bezvýznamný break už nevypadá tak nevinně! Možná jedna z větví měla končit na break, a ne na vrátit?

Fragment N33: Náhodné vynulování vyšších bitů

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);
  ....
}

Upozornění PVS-Studio: V784 Velikost bitové masky je menší než velikost prvního operandu. To způsobí ztrátu vyšších bitů. RuntimeDyld.cpp 815

Všimněte si, že funkce getStubAlignment vrací typ unsigned. Vypočítáme hodnotu výrazu, pokud předpokládáme, že funkce vrátí hodnotu 8:

~(getStubAlignment() - 1)

~(8u-1)

0xFFFFFFF8‬u

Nyní si uvědomte, že proměnná DataSize má 64bitový bezznaménkový typ. Pokud se tedy provedou operace DataSize & 0xFFFFFFF8‬u, všechny třiadvacet vyšších bitů budou vynulovány. To pravděpodobně není to, co programátor chtěl. Podezřívám, že chtěl vypočítat: DataSize & 0xFFFFFFFFFFFFFFF8‬u.

Chcete-li chybu opravit, měli byste napsat takto:

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

Nebo takto:

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

Fragment N34: Neúspěšné explicitní přetypování

template <typename T>
void scaleShuffleMask(int Scale, ArrayRef<T> Mask,
                      SmallVectorImpl<T> &ScaledMask) {
  assert(0 < Scale && "Neočekávaný faktor škálování");
  int NumElts = Mask.size();
  ScaledMask.assign(static_cast<size_t>(NumElts * Scale), -1);
  ....
}

Upozornění PVS-Studio: V1028 [CWE-190] Možný přetečení. Zvažte přetypování operandů operátoru 'NumElts * Scale' na typ 'size_t', ne jeho výsledek. X86ISelLowering.h 1577

Explicitní přetypování se používá, aby nedošlo k přetečení při násobení proměnných typu int. Avšak zde explicitní přetypování nechrání před přetečením. Nejprve budou proměnné vynásobeny a až poté bude 32bitový výsledek násobení rozšířen na typ size_t.

Fragment N35: Neúspěšný 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] Byly nalezeny dva podobné úryvky kódu. Možná je to překlep a měla by být použita proměnná 'Op1' místo 'Op0'. InstCombineCompares.cpp 5507

Tato nová zajímavá diagnostika odhaluje situace, kdy byl úryvek kódu zkopírován a byla začata změna některých jmen, ale na jednom místě nebyla úprava provedena.

Všimněte si, že v druhém bloku bylo změněno Op0 na Op1. Ale na jednom místě nebyla úprava provedena. Pravděpodobně mělo být napsáno takto:

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

Fragment N36: Zmatek v proměnných

struct Status {
  unsigned Mask;
  unsigned Mode;

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

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

Upozornění PVS-Studio: V1001 [CWE-563] Proměnná 'Mode' je přiřazena, ale na konci funkce není použita. SIModeRegister.cpp 48

Je velmi nebezpečné dávat argumentům funkcí stejná jména jako členům třídy. Velmi snadno se můžete zmýlit. Toto je takový případ. Tento výraz nedává smysl:

Mode &= Mask;

Mění se argument funkce. A to je vše. Tento argument již není dále používán. Pravděpodobně mělo být napsáno takto:

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

Fragment N37: Zmatek v proměnných

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;
}

Upozornění PVS-Studio: V1001 [CWE-563] Proměnná 'Size' je přiřazena, ale na konci funkce není použita. Object.cpp 424

Situace je analogická předchozí. Mělo by být napsáno:

this->Size += this->EntrySize;

Fragment N38-N47: Ukazatel nebyl zkontrolován

Dříve jsme zkoumali příklady vyvolání diagnostiky V595. Její podstata spočívá v tom, že ukazatel se na začátku dereferencuje a až poté se kontroluje. Nová diagnostika V1004 je významně opačná, ale také odhaluje velmi mnoho chyb. Zjistí situace, kdy byl ukazatel na začátku zkontrolován, ale poté se na tuto kontrolu zapomnělo. Podívejme se na takové případy nalezené uvnitř 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());  // <=
  ....
}

Upozornění PVS-Studio: V1004 [CWE-476] Ukazatel 'Ptr' byl použit nebezpečně poté, co byl ověřen na nullptr. Zkontrolujte řádky: 729, 738. TargetTransformInfoImpl.h 738

Proměnná Ptr může být roven nullptr, o čemž svědčí kontrola:

if (Ptr != nullptr)

Avšak níže je tento ukazatel dereferencován již bez předchozí kontroly:

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

Podívejme se na jiný podobný případ.

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(); // <=
  ....
}

Upozornění PVS-Studio: V1004 [CWE-476] Ukazatel 'FD' byl použit nebezpečně poté, co byl ověřen na nullptr. Zkontrolujte řádky: 3228, 3231. CGDebugInfo.cpp 3231

Všimněte si ukazatele FD. Jsem si jistý, že problém je dobře viditelný a není potřeba speciálních vysvětlení.

A ještě:

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

Varování PVS-Studio: V1004 [CWE-476] Ukazatel „PtrTy“ byl použit nebezpečně poté, co byl ověřen proti nullptr. Zkontrolujte řádky: 960, 965. InterleavedLoadCombinePass.cpp 965

Jak se chránit před takovými chybami? Buďte pozornější při kontrole kódu a používejte pro pravidelnou kontrolu kódu statický analyzátor PVS-Studio.

Nemá smysl uvádět další fragmenty kódu s chybami tohoto druhu. V článku nechám pouze seznam varování:

• V1004 [CWE-476] Ukazatel „Expr“ byl použit nebezpečně poté, co byl ověřen proti nullptr. Zkontrolujte řádky: 1049, 1078. DebugInfoMetadata.cpp 1078
• V1004 [CWE-476] Ukazatel „PI“ byl použit nebezpečně poté, co byl ověřen proti nullptr. Zkontrolujte řádky: 733, 753. LegacyPassManager.cpp 753
• V1004 [CWE-476] Ukazatel „StatepointCall“ byl použit nebezpečně poté, co byl ověřen proti nullptr. Zkontrolujte řádky: 4371, 4379. Verifier.cpp 4379
• V1004 [CWE-476] Ukazatel „RV“ byl použit nebezpečně poté, co byl ověřen proti nullptr. Zkontrolujte řádky: 2263, 2268. TGParser.cpp 2268
• V1004 [CWE-476] Ukazatel „CalleeFn“ byl použit nebezpečně poté, co byl ověřen proti nullptr. Zkontrolujte řádky: 1081, 1096. SimplifyLibCalls.cpp 1096
• V1004 [CWE-476] Ukazatel „TC“ byl použit nebezpečně poté, co byl ověřen proti nullptr. Zkontrolujte řádky: 1819, 1824. Driver.cpp 1824

Fragment N48-N60: Není kritické, ale jde o chybu (možná únik paměti)

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

Upozornění PVS-Studio: V1023 [CWE-460] Ukazatel bez vlastníka je přidán do kontejneru „Strategies“ metodou „emplace_back“. K úniku paměti dojde v případě výjimky. llvm-isel-fuzzer.cpp 58

Pro přidání prvku na konec kontejneru typu std::vector<std::unique_ptr> nelze jednoduše napsat xxx.push_back(new X), protože neexistuje implicitní převod z X* v std::unique_ptr.

Obvyklým řešením je napsání xxx.emplace_back(new X), protože se kompiluje: metoda emplace_back konstruuje prvek přímo z argumentů a proto může používat explicitní konstruktory.

To není bezpečné. Pokud je vektor plný, dojde k přealokaci paměti. Operace přealokace paměti může selhat, což povede k vyhození výjimky std::bad_alloc. V tomto případě bude ukazatel ztracen a vytvořený objekt nikdy nebude odstraněn.

Bezpečným řešením je vytvoření unique_ptr, který bude vlastnit ukazatel, dokud se vektor nepokusí přealokovat paměť:

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

Od verze C++14 lze použít ‘std::make_unique’:

xxx.push_back(std::make_unique())

Tento typ chyby není kritický pro LLVM. Pokud se nepodaří alokovat paměť, práce překladače se jednoduše zastaví. Nicméně, pro aplikace s dlouhou dostupností, které se nemohou jen tak ukončit, pokud se nepodaří alokovat paměť, může to být skutečně nepříjemná chyba.

Takže i když tento kód nepředstavuje praktické nebezpečí pro LLVM, považoval jsem za užitečné říct o tomto vzoru chyb a že analyzátor PVS-Studio se jej naučil odhalovat.

Další varování tohoto typu:

• V1023 [CWE-460] Ukazatel bez vlastníka je přidán do kontejneru ‘Passes’ metodou ’emplace_back’. Dojde k úniku paměti v případě výjimky. PassManager.h 546
• V1023 [CWE-460] Ukazatel bez vlastníka je přidán do kontejneru ‘AAs’ metodou ’emplace_back’. Dojde k úniku paměti v případě výjimky. AliasAnalysis.h 324
• V1023 [CWE-460] Ukazatel bez vlastníka je přidán do kontejneru ‘Entries’ metodou ’emplace_back’. Dojde k úniku paměti v případě výjimky. DWARFDebugFrame.cpp 519
• V1023 [CWE-460] Ukazatel bez vlastníka je přidán do kontejneru ‘AllEdges’ metodou ’emplace_back’. Dojde k úniku paměti v případě výjimky. CFGMST.h 268
• V1023 [CWE-460] Ukazatel bez vlastníka je přidán do kontejneru ‘VMaps’ metodou ’emplace_back’. Dojde k úniku paměti v případě výjimky. SimpleLoopUnswitch.cpp 2012
• V1023 [CWE-460] Ukazatel bez vlastníka je přidán do kontejneru ‘Records’ metodou ’emplace_back’. Dojde k úniku paměti v případě výjimky. FDRLogBuilder.h 30
• V1023 [CWE-460] Ukazatel bez vlastníka je přidán do kontejneru ‘PendingSubmodules’ metodou ’emplace_back’. Dojde k úniku paměti v případě výjimky. ModuleMap.cpp 810
• V1023 [CWE-460] Ukazatel bez vlastníka je přidán do kontejneru ‘Objects’ metodou ’emplace_back’. Dojde k úniku paměti v případě výjimky. DebugMap.cpp 88
• V1023 [CWE-460] Ukazatel bez vlastníka je přidán do kontejneru ‘Strategies’ metodou ’emplace_back’. Dojde k úniku paměti v případě výjimky. llvm-isel-fuzzer.cpp 60
• V1023 [CWE-460] Ukazatel bez vlastníka je přidán do kontejneru ‘Modifiers’ metodou ’emplace_back’. Dojde k úniku paměti v případě výjimky. llvm-stress.cpp 685
• V1023 [CWE-460] Ukazatel bez vlastníka je přidán do kontejneru ‘Modifiers’ metodou ’emplace_back’. Dojde k úniku paměti v případě výjimky. llvm-stress.cpp 686
• V1023 [CWE-460] Ukazatel bez vlastníka je přidán do kontejneru ‘Modifiers’ metodou ’emplace_back’. Dojde k úniku paměti v případě výjimky. llvm-stress.cpp 688
• V1023 [CWE-460] Ukazatel bez vlastníka je přidán do kontejneru ‘Modifiers’ metodou ’emplace_back’. Dojde k úniku paměti v případě výjimky. llvm-stress.cpp 689
• V1023 [CWE-460] Ukazatel bez vlastníka je přidán do kontejneru ‘Modifiers’ metodou ’emplace_back’. Dojde k úniku paměti v případě výjimky. llvm-stress.cpp 690
• V1023 [CWE-460] Ukazatel bez vlastníka je přidán do kontejneru ‘Modifiers’ metodou ’emplace_back’. Dojde k úniku paměti v případě výjimky. llvm-stress.cpp 691
• V1023 [CWE-460] Do kontejneru „Modifiers” je přidán ukazatel bez vlastníka pomocí metody „emplace_back”. Při výskytu výjimky dojde k úniku paměti. llvm-stress.cpp 692
• V1023 [CWE-460] Do kontejneru „Modifiers” je přidán ukazatel bez vlastníka pomocí metody „emplace_back”. Při výskytu výjimky dojde k úniku paměti. llvm-stress.cpp 693
• V1023 [CWE-460] Do kontejneru „Modifiers” je přidán ukazatel bez vlastníka pomocí metody „emplace_back”. Při výskytu výjimky dojde k úniku paměti. llvm-stress.cpp 694
• V1023 [CWE-460] Do kontejneru „Operands” je přidán ukazatel bez vlastníka pomocí metody „emplace_back”. Při výskytu výjimky dojde k úniku paměti. GlobalISelEmitter.cpp 1911
• V1023 [CWE-460] Do kontejneru „Stash” je přidán ukazatel bez vlastníka pomocí metody „emplace_back”. Při výskytu výjimky dojde k úniku paměti. GlobalISelEmitter.cpp 2100
• V1023 [CWE-460] Do kontejneru „Matchers” je přidán ukazatel bez vlastníka pomocí metody „emplace_back”. Při výskytu výjimky dojde k úniku paměti. GlobalISelEmitter.cpp 2702

Závěr

Celkem jsem vypisoval 60 upozornění, po čemž jsem přestal. Existují další chyby, které analyzátor PVS-Studio odhaluje v LLVM? Ano, existují. Když jsem však připravoval úryvky kódu pro článek, byla už pozdní večer, vlastně dokonce noc, a rozhodl jsem se, že je čas skončit.

Doufám, že vás to bavilo a budete chtít vyzkoušet analyzátor PVS-Studio.

Můžete stáhnout analyzátor a získat trvalý klíč na tato stránka.

Nejdůležitější je používat statickou analýzu pravidelně. Jednorázové kontroly, které provádíme za účelem popularizace metodologie statické analýzy a PVS-Studio, nejsou normálním scénářem.

Hodně štěstí při zlepšování kvality a spolehlivosti kódu!

Pokud chcete sdílet tento článek s anglicky mluvícím publikem, prosím, použijte odkaz na překlad: Andrey Karpov. Hledání chyb v LLVM 8 s PVS-Studio.

Zdroj: habr.com