Leiame vigu LLVM 8 analüsaatoriga PVS-Studio abil

Kaks aastat on möödunud projekti LLVM koodi viimase kontrollimise hetkest meie analüsaatori PVS-Studio abil. Veendugem, et PVS-Studio on endiselt juhtiv tööriist vigade ja võimalike turvaaukude tuvastamiseks. Selleks kontrollime ja leidke uusi vigu versioonis LLVM 8.0.0.

Artikkel, mis tuleb kirjutada

Ausalt öeldes ei olnud mul soovi seda artiklit kirjutada. Pole huvitav kirjutada projektist, mida oleme juba korduvalt kontrollinud (1, 2, 3). Paremini kirjutada millestki uuest, kuid mul pole valikut.

Iga kord, kui ilmub uus versioon LLVM või uuendatakse Clang Static Analyzer, saadame meile järgmiste tüüpi küsimusi:

Vaadake, uus versioon Clang Static Analyzer oskab tuvastada uusi vigu! Tundub, et PVS-Studio kasutamise asjakohasus väheneb. Clang leiab rohkem vigu kui varem ja jõuab PVS-Studio võimalustele järele. Mida teie sellest arvate?

Sellele tahan ma alati vastata millegagi sellise vaimuga:

Me ei istu ka käed rüpes! Oleme PVS-Studio analüsaatori võimalusi oluliselt parandanud. Seega ärge muretsege, me jätkame juhtivana nagu varem.

Kahjuks on see halb vastus. Selles pole tõendeid. Ja just seetõttu ma nüüd seda artiklit kirjutan. Nii et projekt LLVM on jälle kontrollitud ja seal on leitud erinevaid vigu. Need, mis tundusid mulle huvitavad, demonstreerin praegu. Need vead ei suuda tuvastada Clang Static Analyzer (või on see äärmiselt vaevaline tema abil teha). Aga meie saame. Ja ma leidisin ja üles märkisin kõik need vead ühe õhtuga.

Kuid artikli kirjutamine venis mitme nädala peale. Ei suutnud ennast sundida kõike tekstiks vormima :).

Muide, kui teid huvitab, milliseid tehnoloogiaid kasutatakse PVS-Studio analüsaatoris vigade ja võimalike turvaaukude tuvastamiseks, siis soovitan tutvuda selle märkmega.

Uued ja vanad diagnostikad

Nagu juba mainitud, oli projekt LLVM umbes kaks aastat tagasi uuesti kontrollitud ja leitud vead parandatud. Nüüd esitatakse selles artiklis uus hulk vigu. Miks leiti uusi vigu? Selleks on kolm põhjust:

1. LLVM projekt areneb, kus vanad koodid muudetakse ja uut koodi lisatakse. Loomulikult toob muudetud ja uue koodi kirjutamine esile uusi vigu. See demonstreerib hästi, et staatilist analüüsi tuleks rakendada regulaarselt, mitte juhuslikult. Meie artiklid näitavad hästi PVS-Studio analüsaatori võimalusi, kuid see ei ole seotud koodi kvaliteedi tõstmise ja vigade parandamise kulude vähendamisega. Kasutage staatilist koodi analüsaatorit regulaarselt!
2. Töötame olemasolevate diagnostikate täiustamise ja arendamise kallal. Seetõttu suudab analüsaator tuvastada vigu, mida ei tuvastatud eelmistel kontrollimisel.
3. PVS-Studio's on ilmunud uusi diagnostikaid, mida ei olnud kaks aastat tagasi. Otsustasin need eraldi ossa välja tuua, et selgelt näidata PVS-Studio arengut.

Vigade tuvastamine diagnostikatega, mis olid olemas kaks aastat tagasi

Fragment N1: Kopeerimine ja kleepimine

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

PVS-Studio hoiatus: V501 [CWE-570] Vasakul ja paremal '||' operaatori kõrval on identsed al-expressionid 'Name.startswith("avx512.mask.permvar.")'. AutoUpgrade.cpp 73

Kui kaks korda kontrollitakse, et nimi algab alamtäist "avx512.mask.permvar.". Teisel kontrollimisel kavatses ilmselt kirjutada midagi muud, kuid unustati kopeeritud tekst parandada.

Fragment N2: Trüki viga

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 hoiatus: V501 Vasakul ja paremal '|' operaatori kõrval on identsed al-expressionid 'CXNameRange_WantQualifier'. CIndex.cpp 7245

Trüki vea tõttu kasutatakse kaks korda sama nimelist konstanti CXNameRange_WantQualifier.

Fragment N3: Operaatorite prioriteedi segadus

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

PVS-Studio hoiatus: V502 [CWE-783] Võib-olla töötab ‘?:’ operaator oodatust teisiti. ‘?:’ operaatoril on madalam prioriteet kui ‘==’ operaatoril. PPCTargetTransformInfo.cpp 404

Minu arvates on see väga ilus viga. Jah, ma tean, et mul on kummalised arusaamad ilust :) .

Praegu, vastavalt operaatorite prioriteetidele, arvutus toimub järgmiselt:

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

Praktilisest vaatenurgast ei oma see tingimus mõtet, kuna seda saab lühendada:

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

See on ilmne viga. Tõenäoliselt soovisid 0/1 võrrelda muutuja Index. Koodi parandamiseks tuleb ternaarse operaatori ümber lisada sulud:

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

Muide, ternaarne operaator on väga ohtlik ja soodustab loogilisi vigu. Olge selle kasutamisel väga ettevaatlik ja ärge kartke lisada ümmarguseid sulgusid. Selle teema käsitlesin siin, peatükis „Kartke operaatorit ?: ja pange see ümmargustesse sulgudesse“.

Fragmendid N4, N5: Nullviit

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

PVS-Studio hoiatus: V522 [CWE-476] Nullviidiku 'LHS' dereferentseerimine võib toimuda. TGParser.cpp 2152

Kui viit LHS osutub nulliks, siis peaks toimuma hoiatus. Siiski toimuvad selle nullviidiku dereferentseerimine: LHS->getAsString().

See on üsna tüüpiline olukord, kus viga peitub veahalduri sees, kuna neid ei testita. Staatilised analüsaatorid kontrollivad kogu saavutatavat koodi, sõltumata sellest, kui tihti seda kasutatakse. See on väga hea näide, kuidas staatiline analüüs täiendab muid testimise ja vigade kaitse meetodeid.

Sarnane viitprotsessi viga RHS tekib koodis veidi allpool: V522 [CWE-476] Nullviidiku 'RHS' dereferentseerimine võib toimuda. TGParser.cpp 2186

Fragment N6: Viit pärast edastamist

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

PVS-Studio hoiatus: V522 [CWE-476] Nullviidiku 'ProgClone' dereferentseerimine võib toimuda. Miscompilation.cpp 601

Alguses lõpetab nutikas viit ProgClone omandi objekti:

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

Tegelikult, nüüd ProgClone — see null pointer. Therefore, dereferencing of the null pointer should occur just below:

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

But, in reality, this will not happen! Note that the loop does not actually execute.

At the beginning of the container MiscompiledFunctions is cleared:

MiscompiledFunctions.clear();

Next, the size of this container is used in the loop condition:

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

It's easy to see that the loop does not start. I think this is also a mistake, and the code should be written differently.

It seems we have encountered that famous parity of errors! One error masks another :).

Fragment N7: Using a pointer after moving

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

Warning PVS-Studio: V522 [CWE-476] Dereferencing of the null pointer ‘Test’ might take place. Miscompilation.cpp 709

Once again the same situation. Initially, the content of the object is moved, and then it is used as if nothing happened. I increasingly encounter this situation in the code of programs after C++ acquired move semantics. This is what I love about C++! New ways to shoot oneself in the foot keep appearing. PVS-Studio will always have work :).

Fragment N8: Null pointer

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

Warning PVS-Studio: V522 [CWE-476] Dereferencing of the null pointer ‘Type’ might take place. PrettyFunctionDumper.cpp 233

In addition to error handlers, debugging data print functions are usually not tested. Here we have just such a case. The function awaits a user who instead of solving their issues will be forced to fix it.

Correct:

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

Fragment N9: Null pointer

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

PVS-Studio Warning: V522 [CWE-476] Dereferencing of the null pointer 'Ty' might take place. SearchableTableEmitter.cpp 614

I think everything is clear and doesn't require further explanation.

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

PVS-Studio hoiatus: V570 The 'Identifier->Type' variable is assigned to itself. FormatTokenLexer.cpp 249

There is no point in assigning a variable to itself. Most likely, you wanted to write:

Identifier->Type = Question->Type;

Fragment N11: Suspicious 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 hoiatus: V622 [CWE-478] Consider inspecting the 'switch' statement. It's possible that the first 'case' operator is missing. SystemZAsmParser.cpp 652

There is a very suspicious operator present at the beginning break. Did we forget to write something else here?

Fragment N12: Pointer check after dereferencing

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 hoiatus: V595 [CWE-476] The 'Callee' pointer was utilized before it was verified against nullptr. Check lines: 172, 174. AMDGPUInline.cpp 172

Pointer Callee is dereferenced at the moment of the function call getTTI.

And then it turns out that this pointer should be checked for equality nullptr:

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

But it's already too late...

Fragment N13 - N…: Pointer check after dereferencing

The situation described in the previous code fragment is not unique. It occurs here:

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 Warning: V595 [CWE-476] The 'CalleeFn' pointer was utilized before it was verified against nullptr. Check lines: 1079, 1081. SimplifyLibCalls.cpp 1079

And here:

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

Hoiatus PVS-Studio: V595 [CWE-476] ‘ND’ viidatud pointer kasutati enne, kui see kontrolliti nullptr vastu. Kontrollige ridu: 532, 534. SemaTemplateInstantiateDecl.cpp 532

And here:

• V595 [CWE-476] ‘U’ viidatud pointer kasutati enne, kui see kontrolliti nullptr vastu. Kontrollige ridu: 404, 407. DWARFFormValue.cpp 404
• V595 [CWE-476] ‘ND’ viidatud pointer kasutati enne, kui see kontrolliti nullptr vastu. Kontrollige ridu: 2149, 2151. SemaTemplateInstantiate.cpp 2149

Edasi ei olnud mul enam huvi V595 numbriga hoiatusi uurida. Seega ei tea ma, kas selliseid vigu on veel, peale siin loetletud. Tõenäoliselt on.

Fragment N17, N18: Kahtlane nihke

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

PVS-Studio hoiatus: V629 [CWE-190] Kaaluge ‘~(Size — 1) << 1’ väljendi kontrollimist. Bit nihutamine 32-bitise väärtusega koos sellele järgnenud laiendusega 64-bitise tüübini. AArch64AddressingModes.h 260

Võib-olla see ei ole viga ja kood töötab just nii, nagu kavandatud. Aga see on kindlasti kahtlane koht ning seda tuleb kontrollida.

Oletame, et muutuja Suurus on 16, ja siis kavandas koodi autor, et muutuja NImms saab väärtuse:

1111111111111111111111111111111111111111111111111111111111100000

Kuid tegelikult saab see väärtuseks:

0000000000000000000000000000000011111111111111111111111111100000

Asi on selles, et kõik arvutused toimuvad kasutades 32-bitist unsigned tüüpi. Ja alles seejärel laieneb see 32-bitine märgitu tüüp vaikselt uint64_t. Selle käigus jäävad kõrged bitid nullideks.

Situatsiooni saab parandada järgmiselt:

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

Sarnane olukord: V629 [CWE-190] Kaaluge ‘Immr << 6’ väljendi kontrollimist. Bit nihutamine 32-bitise väärtusega koos sellele järgnenud laiendusega 64-bitise tüübini. AArch64AddressingModes.h 269

Fragment N19: Puudub võtmesõna muul juhul?

void AMDGPUAsmParser::cvtDPP(MCInst &Inst, const OperandVector &Operands) {
  ....
  if (Op.isReg() && Op.Reg.RegNo == AMDGPU::VCC) {
    // VOP2b (v_add_u32, v_sub_u32 ...) dpp kasutab "vcc" tokenit.
    // Jätame selle vahele.
    continue;
  } if (isRegOrImmWithInputMods(Desc, Inst.getNumOperands())) {    // <=
    Op.addRegWithFPInputModsOperands(Inst, 2);
  } else if (Op.isDPPCtrl()) {
    Op.addImmOperands(Inst, 1);
  } else if (Op.isImm()) {
    // Käsitleme valikulisi argumente
    OptionalIdx[Op.getImmTy()] = I;
  } else {
    llvm_unreachable("Kehtetu operanditüüp");
  }
  ....
}

PVS-Studio hoiatus: V646 [CWE-670] Kaaluge rakenduse loogika kontrollimist. Võimalik, et ‘else’ võtmesõna on puudu. AMDGPUAsmParser.cpp 5655

Siin pole viga. Kuna then-ploki esimene if lõpeb continue, siis pole oluline, kas on märksõna muul juhul või mitte. Igal juhul töötab kood sama moodi. Siiski, puuduv muul juhul teeb koodi arusaamatuks ja ohtlikuks. Kui hiljem continue kaob, töötab kood täiesti teistmoodi. Minu arvates on parem lisada muul juhul.

Fragment N20: Neli sarnast trükiviga

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 hoiatused:

• V655 [CWE-480] Stringid on ühendatud, kuid ei ole kasutatud. Soovitame uurida 'Result + Name.str()' väljendit. Symbol.cpp 32
• V655 [CWE-480] Stringid on ühendatud, kuid ei ole kasutatud. Soovitame uurida 'Result + "(ObjC Class) " + Name.str()' väljendit. Symbol.cpp 35
• V655 [CWE-480] Stringid on ühendatud, kuid ei ole kasutatud. Soovitame uurida 'Result + "(ObjC Class EH) " + Name.str()' väljendit. Symbol.cpp 38
• V655 [CWE-480] Stringid on ühendatud, kuid ei ole kasutatud. Soovitame uurida 'Result + "(ObjC IVar) " + Name.str()' väljendit. Symbol.cpp 41

T juhuslikult kasutatakse operatori += asemel operatorit +. Selle tulemusena tekivad tähendust kaotavad konstruktsioonid.

Fragment N21: Määratlemata käitumine

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 ei tohi olla tühi");

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

Proovige ise leida ohtlik kood. Siin on pilt tähelepanu kõrvalejuhtimiseks, et te kohe vastusele ei vaataks:

PVS-Studio hoiatus: V708 [CWE-758] Kasutatakse ohtlikku konstruktsiooni: 'FeaturesMap[Op] = FeaturesMap.size()', kus 'FeaturesMap' on 'map' klass. See võib viia määratlemata käitumiseni. RISCVCompressInstEmitter.cpp 490

Probleemne rida:

FeaturesMap[Op] = FeaturesMap.size();

Kui elementi Op ei leita, luuakse kaardis uus element ja sinna salvestatakse elementide arv. Siiski ei ole teada, kas funktsioon size kutsutakse välja enne või pärast uue elemendi lisamist.

Fragment N22-N24: Korduvad määramised

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

PVS-Studio hoiatus: V519 [CWE-563] Muut ‘NType’ muutuja on määratud väärtused järjestikku kaks korda. See võib olla viga. Kontrollige ridu: 1663, 1664. MachOObjectFile.cpp 1664

Ma arvan, et siin ei ole tõelist viga. Lihtsalt liigsed korduvad määramised. Kuid siiski on see vale.

Sarnaselt:

• V519 [CWE-563] Muut ‘B.NDesc’ muutuja on määratud väärtused järjestikku kaks korda. See võib olla viga. Kontrollige ridu: 1488, 1489. llvm-nm.cpp 1489
• V519 [CWE-563] Muut on määratud väärtused järjestikku kaks korda. See võib olla viga. Kontrollige ridu: 59, 61. coff2yaml.cpp 61

Fragments N25-N27: Veel korduvad määramised

Nüüd vaatame veidi teistsugust korduva määramise varianti.

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 hoiatus: V519 [CWE-563] Muut ‘Alignment’ muutuja on määratud väärtused järjestikku kaks korda. See võib olla viga. Kontrollige ridu: 1158, 1160. LoadStoreVectorizer.cpp 1160

See on väga kummaline kood, mis tundub sisaldavat loogilist viga. Alguses, muutuja Alignment määra väärtus olenevalt tingimusest. Seejärel toimub uus määramine, kuid nüüd juba ilma igasuguse kontrollita.

Sarnaseid olukordi võib näha siin:

• V519 [CWE-563] Muut ‘Effects’ muutuja on määratud väärtused järjestikku kaks korda. See võib olla viga. Kontrollige ridu: 152, 165. WebAssemblyRegStackify.cpp 165
• V519 [CWE-563] Muut ‘ExpectNoDerefChunk’ muutuja on määratud väärtused järjestikku kaks korda. See võib olla viga. Kontrollige ridu: 4970, 4973. SemaType.cpp 4973

Fragment N28: Alati tõene tingimus

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

PVS-Studio hoiatus: V547 [CWE-571] Väljend ‘nextByte != 0x90’ on alati tõene. X86DisassemblerDecoder.cpp 379

Kontroll ei oma mõtet. Muutuja nextByte on alati erinev väärtusest 0x90, mis tuleneb eelnevast kontrollist. See on mingi loogiline viga.

Fragment N29 — N…: Alati tõesed/vale tingimused

Analüsaator annab palju hoiatusteateid selle kohta, et kogu tingimus (V547) või selle osa (V560) on alati tõsi või vale. Sageli ei ole need tõelised vead, vaid lihtsalt hooletu kood, makrode lahtiharutamise tulemus jne. Siiski on mõttekas vaadata kõiki neid hoiatusi, kuna aeg-ajalt võib tõeliselt esineda loogika vigu. Näiteks on kahtlane see koodijupp:

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 hoiatus: V560 [CWE-570] Tingimusavalduse osa on alati vale: RegNo == 0xe. ARMDisassembler.cpp 939

Konstant 0xE on kümnendsüsteemis väärtus 14. Kontroll RegNo == 0xe pole mõtet, kuna kui RegNo > 13, siis funktsioon lõpetab oma täitmise.

Oli palju teisi hoiatusi identifikaatoritega V547 ja V560, kuid nagu juba mainitud, ei huvita mind nende uurimine. Oli niigi selge, et mul on piisavalt materjali artikli kirjutamiseks :). Seetõttu ei ole teada, kui palju selliseid vigu on LLVM-is võimalik PVS-Studio abil tuvastada. V595Toon näite, miks nende tundmine on igav. Analüsaator on täiesti õige, andes hoiatuse järgmise koodi kohta. Kuid see ei ole viga.

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

PVS-Studio hoiatus: V547 [CWE-570] Avalduse ‘!HasError’ väärtus on alati vale. UnwrappedLineParser.cpp 1635

Fragment N30: Kahtlane 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(); } .... }

V612

PVS-Studio hoiatus: [CWE-670] Tingimusteta ‘return’ tsüklis. R600OptimizeVectorRegisters.cpp 63 See on kas viga või spetsiifiline tehnika, mis on mõeldud arendajatele, kes koodi loevad, midagi selgitama. Selline konstruktsioon ei selgita mulle midagi ja näeb väga kahtlane välja. Selliselt ei tohiks kirjutada :).

Oled tüdinud? Siis on aeg teed või kohvi valmistada.

Defektid, mille uued diagnostikad tuvastasid

Arvan, et 30 vanade diagnostikate juhtumit on piisavalt. Vaadakem nüüd, milliseid huvitavaid leide saavad uued diagnostikad, mis on analüsaatorisse lisatud pärast

eelmist kontrolli. Sel ajal on C++ analüsaatorisse lisatud kokku 66 üldotstarbelist diagnoosi. Fragment N31: Jõudmata kood

Fragment N31: Kättesaamatu kood

Viga 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 hoiatus: V779 [CWE-561] Tuvastamata kood tuvastatud. Võimalik, et viga on olemas. ExecutionUtils.cpp 146

Nagu näha, mõlemad harud operaatoris if lõppevad operaatori kutsumisega return. Seega konteiner CtorDtorsByPriority ei saa kunagi puhastatud.

Fragment N32: Tuvastamata kood

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(), "ootamatu kokkuvõtte tüüp");
  }
  Lex.setIgnoreColonInIdentifiers(false);                      
  return false;
}

Hoiatus PVS-Studio: V779 [CWE-561] Tuvastamata kood tuvastatud. Võimalik, et viga on olemas. LLParser.cpp 835

Huvitav olukord. Vaatame kõigepealt seda kohta:

return ParseTypeIdEntry(SummaryID);
break;

Esmapilgul tundub, et siin pole viga. Tundub, et operaator break on siin üleliigne ja selle võib lihtsalt eemaldada. Ent mitte kõik ei ole nii lihtne.

Analüsaator annab hoiatuse ridade kohta:

Lex.setIgnoreColonInIdentifiers(false);
return false;

Ja tõepoolest, see kood on tuvastamata. Kõik juhtumid switch lõppevad operaatori kutsumisega return. Ja nüüd mõttetu üksildane break ei tundu enam nii kahjutu! Võib-olla peaks üks haru lõppema break, mitte return?

Fragment N33: Juhuslik kõrgemate bittide nullimine

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

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

PVS-Studio hoiatus: V784 Bittmaski suurus on väiksem kui esimese operaatori suurus. See põhjustab kõrgemate bittide kaotuse. RuntimeDyld.cpp 815

Pöörake tähelepanu, et funktsioon getStubAlignment tagastab tüübi unsigned. Arvutame väljendi väärtuse, eeldades, et funktsioon tagastab väärtuse 8:

~(getStubAlignment() - 1)

~(8u-1)

0xFFFFFFF8‬u

Nüüd pöörake tähelepanu, et muutuja DataSize on 64-bitine märketa tüüp. Tulemuseks on, et tehes tehte DataSize & 0xFFFFFFF8‬u kõik kolmkümmend kaks kõrgemat bitti nullitakse. Tõenäoliselt ei olnud see programmija soov. Kahtlustan, et ta soovis arvutada: DataSize & 0xFFFFFFFFFFFFFFF8‬u.

Probleemi parandamiseks tuleks kirjutada nii:

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

Või nii:

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

Fragment N34: Eba, mille casta tüöt.

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

PVS-Studio hoiatus: V1028 [CWE-190] Võimalik ülevool. Kaaluge operandide 'NumElts * Scale' operaatori tüübiks 'size_t' castingut, mitte tulemust. X86ISelLowering.h 1577

Tüübi selgeks tegemist kasutatakse, et vältida ülevoolu, kui muudetakse muutujaid. int. Siiski ei kaitse siin selge tüübimuutus ülevoolu eest. Esiteks muudetakse muutujaid ja siis laieneb 32-bitine korrutise tulemus tüübiks. size_t.

Fragment N35: Ebaõnnestunud 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] Leiti kaks sarnast koodifragmenti. Võib-olla on see trükiviga ja 'Op1' muutuja tuleks kasutada 'Op0' asemel. InstCombineCompares.cpp 5507

See uus huvitav diagnoos tuvastab olukorrad, kus koodifragmendi on kopeeritud ja selle nimede muutmine on alustatud, kuid ühes kohas ei ole seda parandatud.

Pange tähele, et teises plokis muudeti Op0 . Tundub, et Op1. Kuid ühes kohas ei ole seda parandatud. Tõenäoliselt oleks pidanud olema kirjutatud nii:

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

Fragment N36: Muutujate segadus

struct Status {
  unsigned Mask;
  unsigned Mode;

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

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

PVS-Studio hoiatus: V1001 [CWE-563] Muutujat 'Mode' omistatakse, kuid funktsiooni lõpuks ei kasutata. SIModeRegister.cpp 48

On väga ohtlik anda funktsiooni argumentidele samu nimesid nagu klassi liikmetele. Väga kergesti võib segadusse sattuda. See on just selline juhtum. See väljend ei oma tähendust:

Mode &= Mask;

Funktsiooni argument muutub. Ja kõik. Seda argumenti ei kasutata enam kusagil. Tõenäoliselt oleks pidanud olema kirjutatud nii:

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

Fragment N37: Muutujate segadus

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 hoiatus: V1001 [CWE-563] Muutuja 'Size' omistatakse, kuid seda ei kasutata funktsiooni lõpus. Object.cpp 424

Situatsioon on sarnane eelmisele. Tuleks kirjutada:

this->Size += this->EntrySize;

Fragment N38-N47: Näitajat unustati kontrollida

Oleme varem vaadanud diagnostika aktiveerimise näiteid V595. Selle olemus on see, et näitaja dereferenseeritakse alguses ja alles seejärel kontrollitakse. Uus diagnostika V1004 on vastupidine, kuid tuvastab ka väga palju vigu. See tuvastab olukordi, kus näitaja kontrolliti alguses, kuid seejärel unustati seda teha. Vaadakem selliseid juhtumeid, mis leiti LLVM-seest.

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 hoiatus: V1004 [CWE-476] 'Ptr' näitajat kasutati ebaohutult pärast seda, kui see oli kontrollitud nullptr'i vastu. Kontrollige ridu: 729, 738. TargetTransformInfoImpl.h 738

Muutuja Ptr võib olla võrdne nullptr, mida tõendab kontroll:

if (Ptr != nullptr)

Kuid allpool dereferenseeritakse see näitaja juba ilma eelneva kontrollita:

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

Vaadakem teist sarnast juhtumit.

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 hoiatus: V1004 [CWE-476] 'FD' näitajat kasutati ebaohutult pärast seda, kui see oli kontrollitud nullptr'i vastu. Kontrollige ridu: 3228, 3231. CGDebugInfo.cpp 3231

Pöörake tähelepanu näitajale FD. Olen kindel, et probleem on selgelt nähtav ja erilisi selgitusi ei ole vaja.

Ja veel:

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

PVS-Studio Warning: V1004 [CWE-476] The 'PtrTy' pointer was used unsafely after it was verified against nullptr. Check lines: 960, 965. InterleavedLoadCombinePass.cpp 965

How to protect against such errors? Be more careful during Code Review and use the static analyzer PVS-Studio for regular code checks.

There is no sense in providing other code fragments with such errors. I will leave only the list of warnings in the article:

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

Fragment N48-N60: Not critical, but a defect (possible memory leak)

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

PVS-Studio hoiatus: V1023 [CWE-460] A pointer without an owner is added to the 'Strategies' container by the 'emplace_back' method. A memory leak will occur in case of an exception. llvm-isel-fuzzer.cpp 58

To add an element to the end of a container of type std::vector<std::unique_ptr> you cannot simply write xxx.push_back(new X), since there is no implicit conversion from X* ja std::unique_ptr.

A common solution is to write xxx.emplace_back(new X), since it compiles: the method emplace_back constructs the element directly from arguments and can thus use explicit constructors.

This is unsafe. If the vector is full, it will attempt to reallocate memory. The memory reallocation operation may fail, resulting in an exception being generated std::bad_alloc. In this case, the pointer will be lost, and the created object will never be deleted.

A safe solution is to create a unique_ptr, which will own the pointer until the vector attempts to reallocate memory:

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

Alates C++14 on võimalik kasutada 'std::make_unique':

xxx.push_back(std::make_unique())

See tüüpi defekt ei ole LLVM jaoks kriitiline. Kui mälu eraldamine ebaõnnestub, peatub kompilaatori töö lihtsalt. Kuid pikalt kasutusaega, millel ei ole võimalik lihtsalt lõpetada, kui mälu eraldamine ebaõnnestub, võib see olla tõeline probleem.

Nii et kuigi see kood ei kujuta LLVM-le praktilist ohtu, leidsin, et on kasulik rääkida sellest veamustrist ja kuidas PVS-Studio analüsaator on õppinud seda tuvastama.

Teised selle tüüpi hoiatused:

• V1023 [CWE-460] Omneta ilma omanikuta lisatakse 'Passes' konteinerisse meetodiga 'emplace_back'. Üksuse mäluleke toimub, kui tekib erand. PassManager.h 546
• V1023 [CWE-460] Omneta ilma omanikuta lisatakse 'AAs' konteinerisse meetodiga 'emplace_back'. Üksuse mäluleke toimub, kui tekib erand. AliasAnalysis.h 324
• V1023 [CWE-460] Omneta ilma omanikuta lisatakse 'Entries' konteinerisse meetodiga 'emplace_back'. Üksuse mäluleke toimub, kui tekib erand. DWARFDebugFrame.cpp 519
• V1023 [CWE-460] Omneta ilma omanikuta lisatakse 'AllEdges' konteinerisse meetodiga 'emplace_back'. Üksuse mäluleke toimub, kui tekib erand. CFGMST.h 268
• V1023 [CWE-460] Omneta ilma omanikuta lisatakse 'VMaps' konteinerisse meetodiga 'emplace_back'. Üksuse mäluleke toimub, kui tekib erand. SimpleLoopUnswitch.cpp 2012
• V1023 [CWE-460] Omneta ilma omanikuta lisatakse 'Records' konteinerisse meetodiga 'emplace_back'. Üksuse mäluleke toimub, kui tekib erand. FDRLogBuilder.h 30
• V1023 [CWE-460] Omneta ilma omanikuta lisatakse 'PendingSubmodules' konteinerisse meetodiga 'emplace_back'. Üksuse mäluleke toimub, kui tekib erand. ModuleMap.cpp 810
• V1023 [CWE-460] Omneta ilma omanikuta lisatakse 'Objects' konteinerisse meetodiga 'emplace_back'. Üksuse mäluleke toimub, kui tekib erand. DebugMap.cpp 88
• V1023 [CWE-460] Omneta ilma omanikuta lisatakse 'Strategies' konteinerisse meetodiga 'emplace_back'. Üksuse mäluleke toimub, kui tekib erand. llvm-isel-fuzzer.cpp 60
• V1023 [CWE-460] Omneta ilma omanikuta lisatakse 'Modifiers' konteinerisse meetodiga 'emplace_back'. Üksuse mäluleke toimub, kui tekib erand. llvm-stress.cpp 685
• V1023 [CWE-460] Omneta ilma omanikuta lisatakse 'Modifiers' konteinerisse meetodiga 'emplace_back'. Üksuse mäluleke toimub, kui tekib erand. llvm-stress.cpp 686
• V1023 [CWE-460] Omneta ilma omanikuta lisatakse 'Modifiers' konteinerisse meetodiga 'emplace_back'. Üksuse mäluleke toimub, kui tekib erand. llvm-stress.cpp 688
• V1023 [CWE-460] Omneta ilma omanikuta lisatakse 'Modifiers' konteinerisse meetodiga 'emplace_back'. Üksuse mäluleke toimub, kui tekib erand. llvm-stress.cpp 689
• V1023 [CWE-460] Omneta ilma omanikuta lisatakse 'Modifiers' konteinerisse meetodiga 'emplace_back'. Üksuse mäluleke toimub, kui tekib erand. llvm-stress.cpp 690
• V1023 [CWE-460] Omneta ilma omanikuta lisatakse 'Modifiers' konteinerisse meetodiga 'emplace_back'. Üksuse mäluleke toimub, kui tekib erand. llvm-stress.cpp 691
• V1023 [CWE-460] Omanik, kellel ei ole omanikku, lisatakse 'Modifitseerijate' konteinerisse meetodi 'emplace_back' kaudu. Mäluleke toimub erandi korral. llvm-stress.cpp 692
• V1023 [CWE-460] Omanikuta pointer lisatakse 'Modifitseerijate' konteinerisse meetodi 'emplace_back' kaudu. Mäluleke toimub erandi korral. llvm-stress.cpp 693
• V1023 [CWE-460] Omanikuta pointer lisatakse 'Modifitseerijate' konteinerisse meetodi 'emplace_back' kaudu. Mäluleke toimub erandi korral. llvm-stress.cpp 694
• V1023 [CWE-460] Omanikuta pointer lisatakse 'Operatsioonide' konteinerisse meetodi 'emplace_back' kaudu. Mäluleke toimub erandi korral. GlobalISelEmitter.cpp 1911
• V1023 [CWE-460] Omanikuta pointer lisatakse 'Stash' konteinerisse meetodi 'emplace_back' kaudu. Mäluleke toimub erandi korral. GlobalISelEmitter.cpp 2100
• V1023 [CWE-460] Omanikuta pointer lisatakse 'Matcherite' konteinerisse meetodi 'emplace_back' kaudu. Mäluleke toimub erandi korral. GlobalISelEmitter.cpp 2702

Kokkuvõte

Kokku olen ma välja kirjutanud 60 hoiatust, pärast mida peatusin. Kas on teisi defekte, mida LLVM analüüsija PVS-Studio tuvastab? Jah, on. Siiski, kui ma välja kirjutasin koodilõike artikli jaoks, oli juba hiline õhtu, pigem isegi öö, ja otsustasin, et on aeg lõpetada.

Loodan, et oli huvitav, ja soovite proovida PVS-Studio analüüsijat.

Saate analüüsija alla laadida ja saada võtmepäärise sellel leheküljel.

Peaasi, et kasutage staatilist analüüsi regulaarselt. Ühekordsed kontrollid, mida me teeme staatilise analüüsi ja PVS-Studio meetodoloogia populariseerimise eesmärgil, ei ole normaalne stsenaarium.

Edu koodi kvaliteedi ja usaldusväärsuse parandamisel!

Kui soovite seda artiklit ingliskeelse publikuga jagada, palun kasutage tõlke lingi: Andrey Karpov. Leidke vigu LLVM 8-s PVS-Studio abil.

Allikas: habr.com