Hitta buggar i LLVM 8 med PVS-Studio-analysatorn

Mer än två år har gått sedan den senaste koden för LLVM-projektet granskades med vår PVS-Studio-analysator. Låt oss se till att PVS-Studio-analysatorn fortfarande är det ledande verktyget för att upptäcka fel och potentiella sårbarheter. För att göra detta kommer vi att kontrollera och hitta nya fel i releaseversionen LLVM 8.0.0.

En artikel som behöver skrivas

Ärligt talat, jag ville inte skriva den här artikeln. Det är inte intressant att skriva om ett projekt som vi redan har granskat flera gånger (1, 2, 3). Det är bättre att skriva om något nytt, men jag har inget val.

Varje gång en ny version av LLVM släpps eller uppdateras Clang Static Analyzer, får vi frågor av följande typ i vår e-post:

Titta, den nya versionen av Clang Static Analyzer har lärt sig att hitta nya fel! Jag tycker att relevansen av att använda PVS-Studio minskar. Clang hittar fler fel än tidigare och närmar sig PVS-Studios kapabiliteter. Vad tycker du om detta?

Till detta vill jag alltid svara något i stil med:

Vi sitter inte heller stilla! Vi har avsevärt förbättrat funktionerna i PVS-Studio-analysatorn. Så oroa dig inte, vi fortsätter att vara ledande som tidigare.

Tyvärr är det ett dåligt svar. Det finns inga bevis för det. Och just därför skriver jag den här artikeln nu. Så, LLVM-projektet har återigen granskats och olika fel har hittats. De som jag tyckte var intressanta kommer jag nu att visa. Dessa fel kan Clang Static Analyzer inte identifiera (eller det är mycket besvärligt att göra det med dess hjälp). Men vi kan göra det. Dessutom har jag funnit och noterat alla dessa fel på en kväll.

Men skrivandet av artikeln har dragit ut på tiden i flera veckor. Jag kunde verkligen inte tvinga mig att formulera allt i textform :).

Förresten, om du är intresserad av vilka teknologier som används i PVS-Studio-analysatorn för att identifiera fel och potentiella sårbarheter, så föreslår jag att du bekantar dig med detta inlägg.

Nya och gamla diagnoser

Som redan nämnts, för cirka två år sedan granskades LLVM-projektet ännu en gång och de funna felen åtgärdades. Nu kommer en ny mängd fel att presenteras i denna artikel. Varför har nya fel hittats? Det finns tre skäl till detta:

1. LLVM-projektet utvecklas, den gamla koden förändras och ny kod tillkommer. Naturligtvis finns det nya fel i den ändrade och nyss skrivna koden. Detta visar tydligt att statisk analys bör tillämpas regelbundet, inte bara sporadiskt. Våra artiklar visar väl möjligheterna med PVS-Studio-analyzern, men det har inget att göra med att förbättra kodkvaliteten och minska kostnaderna för att rätta till buggar. Använd statisk kodanalysator regelbundet!
2. Vi förbättrar och förfinar redan existerande diagnoser. Därför kan analysatorn upptäcka fel som inte observerades vid tidigare kontroller.
3. PVS-Studio har fått nya diagnoser som inte fanns för två år sedan. Jag har valt att lyfta fram dem i en separat sektion för att tydligt visa utvecklingen av PVS-Studio.

Defekter som upptäcktes av diagnoser som fanns för två år sedan.

Fragment 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 varning: V501 [CWE-570] Det finns identiska sub-uttryck ‘Name.startswith(«avx512.mask.permvar.»)’ till vänster och höger om ‘||’ operatorn. AutoUpgrade.cpp 73

Det kontrolleras två gånger om namnet börjar med strängen «avx512.mask.permvar.». I den andra kontrollen ville man tydligt skriva något annat, men glömde att rätta det kopierade texten.

Fragment N2: Stavfel

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 varning: V501 Det finns identiska sub-uttryck ‘CXNameRange_WantQualifier’ till vänster och höger om ‘|’ operatorn. CIndex.cpp 7245

På grund av ett stavfel används samma namngivna konstant två gånger. CXNameRange_WantQualifier.

Fragment N3: Förvirring med operatorprioriteringar

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

PVS-Studio varning: V502 [CWE-783] Kanske fungerar ‘?:’ operatorn på ett annat sätt än förväntat. ‘?:’ operatorn har lägre prioritet än ‘==’ operatorn. PPCTargetTransformInfo.cpp 404

Jag tycker att detta är ett mycket vackert fel. Ja, jag vet att jag har konstiga uppfattningar om skönhet :).

För närvarande, enligt operatörernas prioriteringar, beräknas uttrycket på följande sätt:

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

Ur ett praktiskt perspektiv har detta villkor inget syfte, eftersom det kan förenklas till:

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

Detta är ett uppenbart fel. Troligtvis ville man jämföra 0/1 med variabeln Index. För att åtgärda koden måste man lägga till parenteser runt den ternära operatorn:

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

För övrigt, den ternära operatorn är mycket farlig och kan orsaka logiska fel. Var mycket försiktig med den och försök att alltid sätta parenteser. Jag har diskuterat detta ämne här, i kapitlet "Frukta operatorn ?: och omge den med parenteser."

Fragment N4, N5: Nullpekare

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("kan inte kasta '") + LHS->getAsString() +
                    "' till sträng");
    return nullptr;
  }
  ....
}

PVS-Studio varning: V522 [CWE-476] Derreferensiering av nullpekaren ‘LHS’ kan äga rum. TGParser.cpp 2152

Om pekaren LHS visar sig vara null, ska en varning ges. Men istället kommer dereferensieringen av denna nullpekare att inträffa: LHS->getAsString().

Detta är en typisk situation där felet gömmer sig i felhanteringskoden, eftersom ingen testar dem. Statisk analys kontrollerar hela den åtkomliga koden, oavsett hur ofta den används. Detta är ett utmärkt exempel på hur statisk analys kompletterar andra testningstekniker och skydd mot fel.

En liknande felhantering av pekaren RHS har begåtts i koden lite längre ner: V522 [CWE-476] Derreferensiering av nullpekaren ‘RHS’ kan äga rum. TGParser.cpp 2186

Fragment N6: Användning av pekare efter flyttning

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

Varning PVS-Studio: V522 [CWE-476] Derreferensiering av nullpekaren ‘ProgClone’ kan äga rum. Miscompilation.cpp 601

I början slutar den smarta pekaren ProgClone att äga objektet:

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

Faktum är att den nu ProgClone — är en nullpekare. Därför ska det strax nedan ske en dereferens av nullpekaren:

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

Men i verkligheten kommer detta inte att hända! Observera att loopen egentligen inte körs.

I början av containern MiscompiledFunctions rensar:

MiscompiledFunctions.clear();

Därefter används storleken på denna container i villkoret för loopen:

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

Det är lätt att se att loopen inte startar. Jag tror att detta också är ett fel, och koden borde skrivas på ett annat sätt.

Det verkar som om vi har stött på den berömda pariteten av fel! Ett fel döljer ett annat :).

Fragment N7: Användning av pekare efter flyttning

static Expected TestOptimizer(BugDriver &BD, std::unique_ptr Test,
                                    std::unique_ptr Safe) {
  outs() << "  Optimizing functions being tested: ";
  std::unique_ptr 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;
  }
  ....
}

Varning PVS-Studio: V522 [CWE-476] Dereferens av nullpekaren 'Test' kan förekomma. Miscompilation.cpp 709

Återigen samma situation. I början flyttas innehållet i objektet, och sedan används det som om inget har hänt. Jag stöter allt oftare på denna situation i programkod efter att C++ fick flyttsemantic. Det är just därför jag älskar C++! Det dyker upp allt fler sätt att skjuta sig själv i foten. PVS-Studio-analyzern kommer alltid att ha arbete :).

Fragment N8: Nullpekare

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

Varning PVS-Studio: V522 [CWE-476] Dereferens av nullpekaren 'Type' kan förekomma. PrettyFunctionDumper.cpp 233

Utöver felhanterare testas vanligtvis inte funktioner för avlusning av data. Det här är just ett sådant fall. Funktionen förväntar sig en användare som, istället för att lösa sina problem, kommer att tvingas fixa den.

Korrekt:

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

Fragment N9: Nullpekare

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

Varning PVS-Studio: V522 [CWE-476] Dereferering av nullpekaren 'Ty' kan inträffa. SearchableTableEmitter.cpp 614

Jag tror att allt är tydligt och inte behöver förklaring.

Fragment N10: Stavfel

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 varning: V570 Variabeln 'Identifier->Type' tilldelas sig själv. FormatTokenLexer.cpp 249

Det finns ingen mening med att tilldela en variabel sig själv. Troligtvis ville man skriva:

Identifier->Type = Question->Type;

Fragment N11: Misstänkt 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 varning: V622 [CWE-478] Överväg att inspektera 'switch'-satsen. Det är möjligt att den första 'case'-operatören saknas. SystemZAsmParser.cpp 652

I början finns det en mycket misstänkt operator break. Glömde man kanske att skriva något mer här?

Fragment N12: Kontroll av pekare efter dereferering

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 varning: V595 [CWE-476] Pekaren 'Callee' användes innan den verifierades mot nullptr. Kontrollera rader: 172, 174. AMDGPUInline.cpp 172

Pekare Callee deregistreras i början under funktionsanropet getTTI.

Och sedan visar det sig att denna pekare bör kontrolleras för likhet nullptr:

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

Men det är redan för sent...

Fragment N13 – N…: Kontroll av pekare efter dereferering

Situationen som beskrivs i det föregående kodfragmentet är inte unik. Den förekommer här:

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

Varning PVS-Studio: V595 [CWE-476] Pekaren 'CalleeFn' användes innan den verifierades mot nullptr. Kontrollera rader: 1079, 1081. SimplifyLibCalls.cpp 1079

Och här:

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

Varningsmeddelande från PVS-Studio: V595 [CWE-476] Pekaren 'ND' användes innan den verifierades mot nullptr. Kontrollera rader: 532, 534. SemaTemplateInstantiateDecl.cpp 532

Och här:

• V595 [CWE-476] Pekaren 'U' användes innan den verifierades mot nullptr. Kontrollera rader: 404, 407. DWARFFormValue.cpp 404
• V595 [CWE-476] Pekaren 'ND' användes innan den verifierades mot nullptr. Kontrollera rader: 2149, 2151. SemaTemplateInstantiate.cpp 2149

Jag blev sedan inte intresserad av att studera varningar med nummer V595. Så jag vet inte om det finns några andra liknande fel, förmodligen finns det.

Fragment N17, N18: Misstänkt förskjutning

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

PVS-Studio varning: V629 [CWE-190] Överväg att inspektera uttrycket ' ~(Size — 1) << 1'. Bitförskjutning av det 32-bitars värdet med efterföljande expansion till 64-bitars typ. AArch64AddressingModes.h 260

Det kanske inte är ett fel, och koden fungerar precis som avsett. Men det är uppenbart en mycket misstänkt plats och den bör kontrolleras.

Anta att variabeln Size är lika med 16, och då planerade kodförfattaren att få i variabeln NImms värdet:

1111111111111111111111111111111111111111111111111111111111100000

Men i verkligheten kommer värdet att bli:

0000000000000000000000000000000011111111111111111111111111100000

Detta beror på att alla beräkningar görs med hjälp av 32-bitars unsigned typ. Och först därefter kommer denna 32-bitars osignerade typ att implicit utvidgas till uint64_t. Under processen kommer de övre bitarna att vara noll.

Situationen kan åtgärdas så här:

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

Liknande situation: V629 [CWE-190] Överväg att inspektera uttrycket 'Immr << 6'. Bitförskjutning av det 32-bitars värdet med efterföljande expansion till 64-bitars typ. AArch64AddressingModes.h 269

Fragment N19: Saknat nyckelord 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 använder "vcc" token.
    // Hoppa över det.
    continue;
  } if (isRegOrImmWithInputMods(Desc, Inst.getNumOperands())) {    // <=
    Op.addRegWithFPInputModsOperands(Inst, 2);
  } else if (Op.isDPPCtrl()) {
    Op.addImmOperands(Inst, 1);
  } else if (Op.isImm()) {
    // Hantera valfria argument
    OptionalIdx[Op.getImmTy()] = I;
  } else {
    llvm_unreachable("Ogiltig operandtyp");
  }
  ....
}

PVS-Studio varning: V646 [CWE-670] Överväg att inspektera programmets logik. Det är möjligt att 'else' nyckelordet saknas. AMDGPUAsmParser.cpp 5655

Här finns inget fel. Eftersom då-blocket av den första if slutar på continue, så spelar det ingen roll om nyckelordet finns else eller inte. Oavsett kommer koden att fungera på samma sätt. Men en bortglömd else gör koden mer otydlig och farlig. Om detta skulle försvinna continue kommer koden att börja fungera helt annorlunda. Jag tycker det är bättre att lägga till else.

Fragment N20: Fyra likartade skrivfel

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

Varningar från PVS-Studio:

• V655 [CWE-480] Strängarna har sammanfogats men används inte. Överväg att inspektera uttrycket 'Result + Name.str()'. Symbol.cpp 32
• V655 [CWE-480] Strängarna har sammanfogats men används inte. Överväg att inspektera uttrycket 'Result + "(ObjC Class) " + Name.str()'. Symbol.cpp 35
• V655 [CWE-480] Strängarna har sammanfogats men används inte. Överväg att inspektera uttrycket 'Result + "(ObjC Class EH) " + Name.str()'. Symbol.cpp 38
• V655 [CWE-480] Strängarna har sammanfogats men används inte. Överväg att inspektera uttrycket 'Result + "(ObjC IVar) " + Name.str()'. Symbol.cpp 41

Av misstag används operator + istället för operator +=. Detta resulterar i konstruktioner som saknar mening.

Fragment N21: Odefinierat beteende

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 kan inte vara tom");

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

Försök själv att hitta farlig kod. Och här är en bild för att avleda uppmärksamheten så att du inte ser svaret direkt:

PVS-Studio varning: V708 [CWE-758] Farlig konstruktion används: 'FeaturesMap[Op] = FeaturesMap.size()', där 'FeaturesMap' är av klass 'map'. Detta kan leda till odefinierat beteende. RISCVCompressInstEmitter.cpp 490

Problematiska raden:

FeaturesMap[Op] = FeaturesMap.size();

Om elementet Op inte hittas, skapas ett nytt element i kartan och därtill skrivs antalet element i denna karta. Men det råder osäkerhet om funktionen size kommer att anropas före eller efter att det nya elementet har lagts till.

Fragment N22-N24: Återkommande tilldelningar

Fel 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 varning: V519 [CWE-563] Variabeln ‘NType’ tilldelas värden två gånger i rad. Kanske är detta ett misstag. Kontrollera rader: 1663, 1664. MachOObjectFile.cpp 1664

Jag tror inte att det finns något verkligt fel här. Bara en onödig upprepning av tilldelningen. Men det är ändå slarv.

På liknande sätt:

• V519 [CWE-563] Variabeln ‘B.NDesc’ tilldelas värden två gånger i rad. Kanske är detta ett misstag. Kontrollera rader: 1488, 1489. llvm-nm.cpp 1489
• V519 [CWE-563] Variabeln tilldelas värden två gånger i rad. Kanske är detta ett misstag. Kontrollera rader: 59, 61. coff2yaml.cpp 61

Fragment N25-N27: Ytterligare upprepade tilldelningar

Låt oss nu överväga en något annan variant av upprepade tilldelningar.

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

Varningsmeddelande från PVS-Studio: V519 [CWE-563] Variabeln ‘Alignment’ tilldelas värden två gånger i rad. Kanske är detta ett misstag. Kontrollera rader: 1158, 1160. LoadStoreVectorizer.cpp 1160

Detta är mycket konstig kod som uppenbarligen innehåller ett logiskt fel. I början tilldelas variabeln Alignment ett värde beroende på ett villkor. Sedan sker det en ny tilldelning, men nu utan någon kontroll.

Liknande situationer kan ses här:

• V519 [CWE-563] Variabeln ‘Effects’ tilldelas värden två gånger i rad. Kanske är detta ett misstag. Kontrollera rader: 152, 165. WebAssemblyRegStackify.cpp 165
• V519 [CWE-563] Variabeln ‘ExpectNoDerefChunk’ tilldelas värden två gånger i rad. Kanske är detta ett misstag. Kontrollera rader: 4970, 4973. SemaType.cpp 4973

Fragment N28: Alltid sann villkor

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-instruktion stöds             // <=
      break;
  }
  ....
}

PVS-Studio varning: V547 [CWE-571] Uttrycket ‘nextByte != 0x90’ är alltid sant. X86DisassemblerDecoder.cpp 379

Kontrollen är meningslös. Variabeln nextByte är alltid inte lika med värdet 0x90, vilket följer av den tidigare kontrollen. Det är ett logiskt fel.

Fragment N29 — N…: Alltid sanna/falska villkor

Analysatorn ger många varningar om att hela villkoret (V547) eller en del av det (V560) alltid sant eller falskt. Oftast är detta inte verkliga fel, utan bara slarvig kod, resultatet av makrodeployeringar och liknande. Ändå finns det anledning att se över alla dessa varningar, eftersom det ibland dyker upp verkliga logiska fel. Till exempel är denna kodavsnitt misstänkt:

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 varning: V560 [CWE-570] En del av det villkorliga uttrycket är alltid falskt: RegNo == 0xe. ARMDisassembler.cpp 939

Konstanten 0xE är värdet 14 i decimal. Kontrollera RegNo == 0xe är meningslöst, eftersom om RegNo > 13, så kommer funktionen att avsluta sin körning.

Det fanns många andra varningar med identifierarna V547 och V560, men, precis som i fallet med V595, var det ointressant för mig att studera dessa varningar. Jag förstod redan att det fanns tillräckligt med material för att skriva en artikel :). Därför är det oklart hur många fel av denna typ som kan upptäckas i LLVM med hjälp av PVS-Studio.

Jag ger ett exempel på varför det är tråkigt att studera dessa upplysningar. Analysatorn har helt rätt att utfärda en varning för följande kod. Men det är också ingen fel.

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

PVS-Studio varning: V547 [CWE-570] Uttrycket ‘!HasError’ är alltid falskt. UnwrappedLineParser.cpp 1635

Fragment N30: Misstänkt 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 varning: V612 [CWE-670] En ovillkorlig ‘return’ inom en loop. R600OptimizeVectorRegisters.cpp 63

Detta är antingen ett fel eller en specifik teknik som syftar till att förklara något för programmerare som läser koden. En sådan konstruktion förklarar inget för mig och ser mycket misstänkt ut. Bättre att inte skriva så :).

Är du trött? Då är det dags att brygga te eller kaffe.

Defekter upptäckta av nya diagnoser

Jag tror att 30 upplysningar från gamla diagnoser är tillräckligt. Låt oss nu se vad intressant som kan upptäckas av de nya diagnoser som har tillkommit i analysatorn efter den föregående kontrollen. Totalt har 66 diagnoser för allmänt bruk lagts till i C++-analysatorn under denna tid.

Fragment N31: Oåtkomlig kod

Fel 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 varning: V779 [CWE-561] Ouppnåelig kod upptäcktes. Det kan finnas ett fel.

Som ni kan se, slutar båda grenarna av operatorn if med ett anrop till operatorn återvända. Därmed kommer behållaren CtorDtorsByPriority aldrig att rensas.

Fragment N32: Ouppnåelig kod

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(), "oväntad sammanfattning typ");
  }
  Lex.setIgnoreColonInIdentifiers(false);                      // <=
  return false;
}

Varning PVS-Studio: V779 [CWE-561] Ouppnåelig kod upptäcktes. Det kan finnas ett fel.

En intressant situation. Låt oss börja med denna plats:

return ParseTypeIdEntry(SummaryID);
break;

Vid första anblick verkar det inte finnas något fel här. Det verkar som om operatorn break är överflödig, och den kan tas bort. Men allt är inte så enkelt.

Analysatorn ger en varning på raderna:

Lex.setIgnoreColonInIdentifiers(false);
return false;

Och i själva verket är denna kod ouppnåelig. Alla fall i switch slutar med anropet till operatorn återvända. Och nu ser den meningslösa ensamma break inte så harmlös ut! Kanske borde en av grenarna sluta på break, istället för på återvända?

Fragment N33: Slumptal av högre bitar

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

Förväntad
RuntimeDyldImpl::emitSection(const ObjectFile &Obj,
                             const SectionRef &Section,
                             bool IsCode) {
  ....
  uint64_t DataSize = Section.getSize();
  ....
  if (StubBufSize > 0)
    DataSize &= ~(getStubAlignment() - 1);
  ....
}

PVS-Studio varning: V784 Storleken på bitmasken är mindre än storleken på den första operanden. Detta kommer att orsaka förlust av högre bitar.

Observera att funktionen getStubAlignment returnerar typen unsigned.Låt oss beräkna värdet på uttrycket, om vi antar att funktionen returnerar värdet 8:

~(getStubAlignment() - 1)

~(8u-1)

0xFFFFFFF8‬u

Nu, observera att variabeln DataSize har en 64-bit unsigned typ. Så, vid utförandet av operationen DataSize & 0xFFFFFFF8‬u kommer alla trettiotvå högre bitar att nollställas. Sannolikt är detta inte vad programmeraren avsåg. Jag misstänker att han ville beräkna: DataSize & 0xFFFFFFFFFFFFFFF8‬u.

För att rätta till felet, bör det skrivas så här:

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

Eller så här:

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

Fragment N34: Misslyckad typkonvertering

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

PVS-Studio varning: V1028 [CWE-190] Möjlig överflöde. Överväg att konvertera operanderna av 'NumElts * Scale' operatorn till 'size_t' typ, inte resultatet. X86ISelLowering.h 1577

Typkonvertering används för att undvika överflöde vid multiplikation av variabler av typ int. Men här skyddar typkonverteringen inte mot överflöde. I början kommer variablerna att multipliceras, och först därefter kommer den 32-bitars produktresultatet att utvidgas till typen size_t.

Fragment N35: Misslyckad 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] Två liknande kodfragment hittades. Kanske är detta ett skrivfel och 'Op1' variabeln borde användas istället för 'Op0'. InstCombineCompares.cpp 5507

Denna nya intressanta diagnos identifierar situationer där en kodsnutt kopierades och vissa namn ändrades, men på ett ställe blev det inte korrigerat.

Observera att i den andra blocket ändrades Op0 till Op1. Men på ett ställe blev det inte korrigerat. Det borde troligtvis ha skrivits så här:

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

Fragment N36: Förvirring med variabler

struct Status {
  unsigned Mask;
  unsigned Mode;

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

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

PVS-Studio varning: V1001 [CWE-563] 'Mode' variabeln tilldelas men används inte i slutet av funktionen. SIModeRegister.cpp 48

Det är mycket farligt att ge funktionsargument samma namn som klassmedlemmar. Det är lätt att bli förvirrad. Detta är just ett sådant fall. Detta uttryck ger ingen mening:

Mode &= Mask;

Funktionens argument ändras. Och that's it. Detta argument används inte längre. Det borde troligtvis ha skrivits så här:

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

Fragment N37: Förvirring med variabler

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-varning: V1001 [CWE-563] Variabeln 'Size' tilldelas men används inte i slutet av funktionen. Object.cpp 424

Situationen är liknande den föregående. Det borde stå:

this->Size += this->EntrySize;

Fragment N38-N47: Pekaren glömdes att kontrolleras

Tidigare granskade vi exempel på hur diagnostik utlöses V595. Huvudinnehållet är att pekaren först avrefereras och sedan kontrolleras. Den nya diagnosen V1004 är motsatt i betydelse, men hittar också många fel. Den upptäcker situationer när pekaren först har kontrollerats, men sedan glömdes det. Låt oss titta på sådana fall som hittades inom 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-varning: V1004 [CWE-476] Pekaren 'Ptr' användes osäkert efter att den verifierades mot nullptr. Kontrollera rader: 729, 738. TargetTransformInfoImpl.h 738

Variabel Ptr kan vara lika med nullptr, vilket bevisas av kontrollen:

if (Ptr != nullptr)

Men nedan avrefereras denna pekare redan utan föregående kontroll:

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

Låt oss titta på ett annat liknande fall.

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-varning: V1004 [CWE-476] Pekaren 'FD' användes osäkert efter att den verifierades mot nullptr. Kontrollera rader: 3228, 3231. CGDebugInfo.cpp 3231

Observera pekaren FD. Jag är säker på att problemet är väl synligt, och några särskilda förklaringar behövs inte.

Och dessutom:

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

Varning PVS-Studio: V1004 [CWE-476] Pekaren ‘PtrTy’ användes osäkert efter att den bekräftades mot nullptr. Kontrollera rader: 960, 965. InterleavedLoadCombinePass.cpp 965

Hur skyddar man sig mot sådana fel? Var noggrannare vid kodgranskning och använd den statiska kodanalysatorn PVS-Studio för regelbundna kontroller.

Det är meningslöst att ta med andra kodfragment med denna typ av fel. Jag lämnar bara en lista med varningar i artikeln:

• V1004 [CWE-476] Pekaren ‘Expr’ användes osäkert efter att den bekräftades mot nullptr. Kontrollera rader: 1049, 1078. DebugInfoMetadata.cpp 1078
• V1004 [CWE-476] Pekaren ‘PI’ användes osäkert efter att den bekräftades mot nullptr. Kontrollera rader: 733, 753. LegacyPassManager.cpp 753
• V1004 [CWE-476] Pekaren ‘StatepointCall’ användes osäkert efter att den bekräftades mot nullptr. Kontrollera rader: 4371, 4379. Verifier.cpp 4379
• V1004 [CWE-476] Pekaren ‘RV’ användes osäkert efter att den bekräftades mot nullptr. Kontrollera rader: 2263, 2268. TGParser.cpp 2268
• V1004 [CWE-476] Pekaren ‘CalleeFn’ användes osäkert efter att den bekräftades mot nullptr. Kontrollera rader: 1081, 1096. SimplifyLibCalls.cpp 1096
• V1004 [CWE-476] Pekaren ‘TC’ användes osäkert efter att den bekräftades mot nullptr. Kontrollera rader: 1819, 1824. Driver.cpp 1824

Fragment N48-N60: Inte kritiskt, men en defekt (möjligt minnesläckage)

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

PVS-Studio varning: V1023 [CWE-460] En pekare utan ägare lades till i ‘Strategies’-containern via metoden ’emplace_back’. Ett minnesläckage kommer att inträffa vid ett undantag. llvm-isel-fuzzer.cpp 58

För att lägga till ett element i slutet av en container av typen std::vector<std::unique_ptr> kan man inte bara skriva xxx.push_back(new X), eftersom det inte finns någon implicit omvandling från X* på std::unique_ptr.

En vanlig lösning är att skriva xxx.emplace_back(new X), eftersom det kompilera: metoden emplace_back konstruerar elementet direkt från argumenten och kan därför använda uttryckliga konstruktörer.

Detta är osäkert. Om vektorn är full kan det bli en omallokering av minnet. Omallokeringen kan misslyckas, vilket resulterar i ett undantag std::bad_alloc. I så fall går pekaren förlorad, och det skapade objektet kommer aldrig att tas bort.

En säker lösning är att skapa en unique_ptr, som kommer att äga pekaren innan vektorn försöker omallokera minnet:

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

Sedan med C++14 kan man använda 'std::make_unique':

xxx.push_back(std::make_unique())

Den här typen av fel är inte kritisk för LLVM. Om minnesallokeringen misslyckas kommer kompilatorn bara att stoppas. Men för applikationer med lång drifttid, som inte bara kan avslutas om minnesallokeringen misslyckas, kan detta vara ett verkligt obehagligt fel.

Så även om denna kod inte utgör någon praktisk fara för LLVM, tyckte jag att det var nyttigt att berätta om detta felmönster och hur PVS-Studio har lärt sig att identifiera det.

Andra varningar av denna typ:

• V1023 [CWE-460] En pekare utan ägare läggs till 'Passes'-behållaren med 'emplace_back'-metoden. Ett minnesläckage kommer att inträffa vid ett undantag. PassManager.h 546
• V1023 [CWE-460] En pekare utan ägare läggs till 'AAs'-behållaren med 'emplace_back'-metoden. Ett minnesläckage kommer att inträffa vid ett undantag. AliasAnalysis.h 324
• V1023 [CWE-460] En pekare utan ägare läggs till 'Entries'-behållaren med 'emplace_back'-metoden. Ett minnesläckage kommer att inträffa vid ett undantag. DWARFDebugFrame.cpp 519
• V1023 [CWE-460] En pekare utan ägare läggs till 'AllEdges'-behållaren med 'emplace_back'-metoden. Ett minnesläckage kommer att inträffa vid ett undantag. CFGMST.h 268
• V1023 [CWE-460] En pekare utan ägare läggs till 'VMaps'-behållaren med 'emplace_back'-metoden. Ett minnesläckage kommer att inträffa vid ett undantag. SimpleLoopUnswitch.cpp 2012
• V1023 [CWE-460] En pekare utan ägare läggs till 'Records'-behållaren med 'emplace_back'-metoden. Ett minnesläckage kommer att inträffa vid ett undantag. FDRLogBuilder.h 30
• V1023 [CWE-460] En pekare utan ägare läggs till 'PendingSubmodules'-behållaren med 'emplace_back'-metoden. Ett minnesläckage kommer att inträffa vid ett undantag. ModuleMap.cpp 810
• V1023 [CWE-460] En pekare utan ägare läggs till 'Objects'-behållaren med 'emplace_back'-metoden. Ett minnesläckage kommer att inträffa vid ett undantag. DebugMap.cpp 88
• V1023 [CWE-460] En pekare utan ägare läggs till 'Strategies'-behållaren med 'emplace_back'-metoden. Ett minnesläckage kommer att inträffa vid ett undantag. llvm-isel-fuzzer.cpp 60
• V1023 [CWE-460] En pekare utan ägare läggs till 'Modifiers'-behållaren med 'emplace_back'-metoden. Ett minnesläckage kommer att inträffa vid ett undantag. llvm-stress.cpp 685
• V1023 [CWE-460] En pekare utan ägare läggs till 'Modifiers'-behållaren med 'emplace_back'-metoden. Ett minnesläckage kommer att inträffa vid ett undantag. llvm-stress.cpp 686
• V1023 [CWE-460] En pekare utan ägare läggs till 'Modifiers'-behållaren med 'emplace_back'-metoden. Ett minnesläckage kommer att inträffa vid ett undantag. llvm-stress.cpp 688
• V1023 [CWE-460] En pekare utan ägare läggs till 'Modifiers'-behållaren med 'emplace_back'-metoden. Ett minnesläckage kommer att inträffa vid ett undantag. llvm-stress.cpp 689
• V1023 [CWE-460] En pekare utan ägare läggs till 'Modifiers'-behållaren med 'emplace_back'-metoden. Ett minnesläckage kommer att inträffa vid ett undantag. llvm-stress.cpp 690
• V1023 [CWE-460] En pekare utan ägare läggs till 'Modifiers'-behållaren med 'emplace_back'-metoden. Ett minnesläckage kommer att inträffa vid ett undantag. llvm-stress.cpp 691
• V1023 [CWE-460] En pekare utan ägare läggs till i 'Modifiers'-containern av metoden 'emplace_back'. En minnesläcka uppstår vid ett undantag. llvm-stress.cpp 692
• V1023 [CWE-460] En pekare utan ägare läggs till i 'Modifiers'-containern av metoden 'emplace_back'. En minnesläcka uppstår vid ett undantag. llvm-stress.cpp 693
• V1023 [CWE-460] En pekare utan ägare läggs till i 'Modifiers'-containern av metoden 'emplace_back'. En minnesläcka uppstår vid ett undantag. llvm-stress.cpp 694
• V1023 [CWE-460] En pekare utan ägare läggs till i 'Operands'-containern av metoden 'emplace_back'. En minnesläcka uppstår vid ett undantag. GlobalISelEmitter.cpp 1911
• V1023 [CWE-460] En pekare utan ägare läggs till i 'Stash'-containern av metoden 'emplace_back'. En minnesläcka uppstår vid ett undantag. GlobalISelEmitter.cpp 2100
• V1023 [CWE-460] En pekare utan ägare läggs till i 'Matchers'-containern av metoden 'emplace_back'. En minnesläcka uppstår vid ett undantag. GlobalISelEmitter.cpp 2702

Slutsats

Totalt har jag skrivit ner 60 varningar, efter det stannade jag. Finns det andra defekter som PVS-Studio-analysatorn upptäcker i LLVM? Ja, det finns. När jag dokumenterade kodfragment för artikeln blev det sen kväll, snarare natt, och jag bestämde mig för att det var dags att avsluta.

Jag hoppas att du tyckte att det var intressant och att du vill prova PVS-Studio-analysatorn.

Du kan ladda ner analysatorn och få en provnyckel på denna sida.

Det viktigaste är att använda statisk analys regelbundet. Engångskontroller, som vi utför för att popularisera metodologin för statisk analys och PVS-Studio, är inte ett normalt scenario.

Lycka till med att förbättra kvaliteten och pålitligheten i koden!

Om du vill dela den här artikeln med en engelsktalande publik, vänligen använd länken till översättningen: Andrey Karpov. Hitta buggar i LLVM 8 med PVS-Studio.

Källa: habr.com