Mer än två år har gått sedan den senaste kodkontrollen av LLVM-projektet med vår PVS-Studio-analysator. Låt oss se till att PVS-Studio-analysatorn fortfarande är ett ledande verktyg för att identifiera fel och potentiella sårbarheter. För att göra detta kommer vi att kontrollera och hitta nya fel i LLVM 8.0.0-versionen.
Artikel som ska skrivas
För att vara ärlig så ville jag inte skriva den här artikeln. Det är inte intressant att skriva om ett projekt som vi redan har kollat flera gånger (, , ). 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 , får vi frågor av följande typ i vårt mail:
Titta, den nya versionen av Clang Static Analyzer har lärt sig att hitta nya fel! Det verkar för mig som att relevansen av att använda PVS-Studio minskar. Clang hittar fler fel än tidigare och kommer ikapp PVS-Studios funktioner. Vad tycker du om detta?
På detta vill jag alltid svara något i stil med:
Vi sitter inte sysslolösa heller! Vi har avsevärt förbättrat funktionerna hos PVS-Studio-analysatorn. Så oroa dig inte, vi fortsätter att leda som tidigare.
Tyvärr är detta ett dåligt svar. Det finns inga bevis i det. Och det är därför jag skriver den här artikeln nu. Så LLVM-projektet har återigen kontrollerats och en mängd olika fel har hittats i det. Jag ska nu visa de som verkade intressanta för mig. Clang Static Analyzer kan inte hitta dessa fel (eller det är extremt obekvämt att göra det med dess hjälp). Men vi kan. Dessutom hittade och skrev jag ner alla dessa fel på en kväll.
Men att skriva artikeln tog flera veckor. Jag kunde bara inte förmå mig att skriva allt detta i text :).
Förresten, om du är intresserad av vilka tekniker som används i PVS-Studio-analysatorn för att identifiera fel och potentiella sårbarheter, föreslår jag att du bekantar dig med detta .
Ny och gammal diagnostik
Som redan nämnts kontrollerades LLVM-projektet för cirka två år sedan igen, och de fel som hittats korrigerades. Nu kommer den här artikeln att presentera en ny grupp fel. Varför hittades nya buggar? Det finns 3 anledningar till detta:
- LLVM-projektet utvecklas, ändrar gammal kod och lägger till ny kod. Naturligtvis finns det nya fel i den modifierade och skrivna koden. Detta visar tydligt att statisk analys bör användas regelbundet och inte ibland. Våra artiklar visar väl kapaciteten hos PVS-Studio-analysatorn, men detta har ingenting att göra med att förbättra kodkvaliteten och minska kostnaden för att åtgärda fel. Använd en statisk kodanalysator regelbundet!
- Vi håller på att färdigställa och förbättra befintlig diagnostik. Därför kan analysatorn identifiera fel som den inte märkte under tidigare skanningar.
- Ny diagnostik har dykt upp i PVS-Studio som inte fanns för 2 år sedan. Jag bestämde mig för att lyfta fram dem i ett separat avsnitt för att tydligt visa utvecklingen av PVS-Studio.
Defekter identifierade av diagnostik som fanns för 2 å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: [CWE-570] Det finns identiska underuttryck 'Name.startswith("avx512.mask.permvar.")' till vänster och till höger om '||' operatör. AutoUpgrade.cpp 73
Det är dubbelkollat att namnet börjar med delsträngen "avx512.mask.permvar.". I den andra kontrollen ville de uppenbarligen skriva något annat, men glömde att rätta till den kopierade texten.
Fragment N2: Skrivfel
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;
....
}Varning PVS-Studio: V501 Det finns identiska underuttryck 'CXNameRange_WantQualifier' till vänster och till höger om '|' operatör. CIndex.cpp 7245
På grund av ett stavfel används samma namngivna konstant två gånger CXNameRange_WantQualifier.
Fragment N3: Förvirring med operatörsföreträde
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: [CWE-783] Kanske fungerar '?:'-operatören på ett annat sätt än det förväntades. Operatorn '?:' har lägre prioritet än operatorn '=='. PPCTargetTransformInfo.cpp 404
Enligt min mening är detta ett mycket vackert misstag. Ja, jag vet att jag har konstiga idéer om skönhet :).
Nu, enligt , utvärderas uttrycket enligt följande:
(ISD == ISD::EXTRACT_VECTOR_ELT && (Index == ST->isLittleEndian())) ? 1 : 0Ur praktisk synvinkel är ett sådant villkor inte vettigt, eftersom det kan reduceras till:
(ISD == ISD::EXTRACT_VECTOR_ELT && Index == ST->isLittleEndian())Detta är ett tydligt misstag. Troligtvis ville de jämföra 0/1 med en variabel index. För att fixa koden måste du lägga till parenteser runt den ternära operatorn:
if (ISD == ISD::EXTRACT_VECTOR_ELT && Index == (ST->isLittleEndian() ? 1 : 0))Förresten, den ternära operatören är mycket farlig och provocerar logiska fel. Var väldigt försiktig med det och var inte girig med parenteser. Jag tittade på detta ämne mer i detalj , i kapitlet "Se upp för ?: Operatör och skriv den inom parentes."
Fragment N4, N5: Nollpekare
Init *TGParser::ParseValue(Record *CurRec, RecTy *ItemType, IDParseMode Mode) {
....
TypedInit *LHS = dyn_cast<TypedInit>(Result);
....
LHS = dyn_cast<TypedInit>(
UnOpInit::get(UnOpInit::CAST, LHS, StringRecTy::get())
->Fold(CurRec));
if (!LHS) {
Error(PasteLoc, Twine("can't cast '") + LHS->getAsString() +
"' to string");
return nullptr;
}
....
}PVS-Studio varning: [CWE-476] Bortreferensering av nollpekaren 'LHS' kan ske. TGParser.cpp 2152
Om pekaren LHS är null bör en varning utfärdas. Men i stället kommer samma nollpekare att avreferens: LHS->getAsString().
Detta är en mycket typisk situation när ett fel är dolt i en felhanterare, eftersom ingen testar dem. Statiska analysatorer kontrollerar all nåbar kod, oavsett hur ofta den används. Detta är ett mycket bra exempel på hur statisk analys kompletterar andra test- och felskyddstekniker.
Liknande pekarhanteringsfel RHS tillåtet i koden precis nedan: V522 [CWE-476] Avreferensering av nollpekaren 'RHS' kan ske. TGParser.cpp 2186
Fragment N6: Använda pekaren efter att ha flyttat
static Expected<bool>
ExtractBlocks(....)
{
....
std::unique_ptr<Module> ProgClone = CloneModule(BD.getProgram(), VMap);
....
BD.setNewProgram(std::move(ProgClone)); // <=
MiscompiledFunctions.clear();
for (unsigned i = 0, e = MisCompFunctions.size(); i != e; ++i) {
Function *NewF = ProgClone->getFunction(MisCompFunctions[i].first); // <=
assert(NewF && "Function not found??");
MiscompiledFunctions.push_back(NewF);
}
....
}PVS-Studio Varning: V522 [CWE-476] Omreferensering av nollpekaren 'ProgClone' kan ske. Miscompilation.cpp 601
I början en smart pekare ProgClone upphör att äga objektet:
BD.setNewProgram(std::move(ProgClone));Faktiskt nu ProgClone är en nollpekare. Därför bör en nollpekardereferens ske precis nedanför:
Function *NewF = ProgClone->getFunction(MisCompFunctions[i].first);Men i verkligheten kommer detta inte att hända! Observera att slingan faktiskt inte exekveras.
I början av behållaren Felkompilerade funktioner rensat:
MiscompiledFunctions.clear();Därefter används storleken på denna behållare i looptillståndet:
for (unsigned i = 0, e = MisCompFunctions.size(); i != e; ++i) {Det är lätt att se att slingan inte startar. Jag tror att detta också är en bugg och koden bör skrivas annorlunda.
Det verkar som att vi har stött på den berömda pariteten av fel! Ett misstag döljer ett annat :).
Fragment N7: Använda pekaren efter att ha flyttat
static Expected<bool> TestOptimizer(BugDriver &BD, std::unique_ptr<Module> Test,
std::unique_ptr<Module> Safe) {
outs() << " Optimizing functions being tested: ";
std::unique_ptr<Module> Optimized =
BD.runPassesOn(Test.get(), BD.getPassesToRun());
if (!Optimized) {
errs() << " Error running this sequence of passes"
<< " on the input program!n";
BD.setNewProgram(std::move(Test)); // <=
BD.EmitProgressBitcode(*Test, "pass-error", false); // <=
if (Error E = BD.debugOptimizerCrash())
return std::move(E);
return false;
}
....
}PVS-Studio-varning: V522 [CWE-476] Omreferensering av nollpekaren 'Test' kan ske. Miscompilation.cpp 709
Samma situation igen. Först flyttas föremålets innehåll och sedan används det som om ingenting hade hänt. Jag ser denna situation allt oftare i programkod efter att rörelsesemantik dök upp i C++. Det är därför jag älskar C++-språket! Det finns fler och fler nya sätt att skjuta sitt eget ben. PVS-Studio-analysatorn kommer alltid att fungera :).
Fragment N8: Nollpekare
void FunctionDumper::dump(const PDBSymbolTypeFunctionArg &Symbol) {
uint32_t TypeId = Symbol.getTypeId();
auto Type = Symbol.getSession().getSymbolById(TypeId);
if (Type)
Printer << "<unknown-type>";
else
Type->dump(*this);
}PVS-Studio-varning: V522 [CWE-476] Avreferensering av nollpekaren 'Typ' kan ske. PrettyFunctionDumper.cpp 233
Förutom felhanterare testas vanligtvis inte funktioner för felsökning av utskrifter. Vi har just ett sådant fall framför oss. Funktionen väntar på användaren, som istället för att lösa sina problem kommer att tvingas fixa det.
korrigera:
if (Type)
Type->dump(*this);
else
Printer << "<unknown-type>";Fragment N9: Nollpekare
void SearchableTableEmitter::collectTableEntries(
GenericTable &Table, const std::vector<Record *> &Items) {
....
RecTy *Ty = resolveTypes(Field.RecType, TI->getType());
if (!Ty) // <=
PrintFatalError(Twine("Field '") + Field.Name + "' of table '" +
Table.Name + "' has incompatible type: " +
Ty->getAsString() + " vs. " + // <=
TI->getType()->getAsString());
....
}PVS-Studio-varning: V522 [CWE-476] Omreferensering av nollpekaren 'Ty' kan ske. SearchableTableEmitter.cpp 614
Jag tycker att allt är klart och kräver ingen förklaring.
Fragment N10: Skrivfel
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: Variabeln 'Identifier->Typ' tilldelas sig själv. FormatTokenLexer.cpp 249
Det är ingen idé att tilldela en variabel till sig själv. Troligtvis ville de skriva:
Identifier->Type = Question->Type;Fragment N11: Misstänkt brott
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: [CWE-478] Överväg att inspektera "switch"-satsen. Det är möjligt att den första "case"-operatören saknas. SystemZAsmParser.cpp 652
Det finns en mycket misstänksam operatör i början bryta. Har du glömt att skriva något mer här?
Fragment N12: Kontrollera en pekare efter referens
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: [CWE-476] 'Callee'-pekaren användes innan den verifierades mot nullptr. Kontrollera rader: 172, 174. AMDGPUInline.cpp 172
pekaren Callee i början avreferens vid den tidpunkt då funktionen anropas getTTI.
Och så visar det sig att den här pekaren ska kontrolleras för jämställdhet nullptr:
if (!Callee || Callee->isDeclaration())Men det är för sent...
Fragment N13 - N...: Kontrollera en pekare efter referens
Situationen som diskuterades i det föregående kodfragmentet är inte unik. Det visas här:
static Value *optimizeDoubleFP(CallInst *CI, IRBuilder<> &B,
bool isBinary, bool isPrecise = false) {
....
Function *CalleeFn = CI->getCalledFunction();
StringRef CalleeNm = CalleeFn->getName(); // <=
AttributeList CalleeAt = CalleeFn->getAttributes();
if (CalleeFn && !CalleeFn->isIntrinsic()) { // <=
....
}PVS-Studio varning: V595 [CWE-476] 'CalleeFn'-pekaren 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<NamedDecl>(New);
CXXRecordDecl *ThisContext =
dyn_cast_or_null<CXXRecordDecl>(ND->getDeclContext()); // <=
CXXThisScopeRAII ThisScope(*this, ThisContext, Qualifiers(),
ND && ND->isCXXInstanceMember()); // <=
....
}PVS-Studio varning: V595 [CWE-476] 'ND'-pekaren användes innan den verifierades mot nullptr. Kontrollera rader: 532, 534. SemaTemplateInstantiateDecl.cpp 532
Och här:
- V595 [CWE-476] 'U'-pekaren användes innan den verifierades mot nullptr. Kontrollera rader: 404, 407. DWARFormValue.cpp 404
- V595 [CWE-476] 'ND'-pekaren användes innan den verifierades mot nullptr. Kontrollera rader: 2149, 2151. SemaTemplateInstantiate.cpp 2149
Och så blev jag ointresserad av att studera varningarna med nummer V595. Så jag vet inte om det finns fler liknande fel än de som listas här. Med största sannolikhet finns det.
Fragment N17, N18: Misstänkt skift
static inline bool processLogicalImmediate(uint64_t Imm, unsigned RegSize,
uint64_t &Encoding) {
....
unsigned Size = RegSize;
....
uint64_t NImms = ~(Size-1) << 1;
....
}PVS-Studio varning: [CWE-190] Överväg att inspektera uttrycket '~(Size - 1) << 1'. Bitförskjutning av 32-bitarsvärdet med en efterföljande expansion till 64-bitarstyp. AArch64AddressingModes.h 260
Det kanske inte är en bugg och koden fungerar precis som den är tänkt. Men detta är helt klart en mycket misstänkt plats och måste kontrolleras.
Låt oss säga variabeln Storlek är lika med 16, och sedan planerade författaren till koden att få den i en variabel NImms värde:
1111111111111111111111111111111111111111111111111111111111100000
Men i verkligheten blir resultatet:
0000000000000000000000000000000011111111111111111111111111100000
Faktum är att alla beräkningar sker med 32-bitars osignerad typ. Och först då kommer denna 32-bitars osignerade typ att implicit utökas till uint64_t. I detta fall kommer de mest signifikanta bitarna att vara noll.
Du kan fixa situationen så här:
uint64_t NImms = ~static_cast<uint64_t>(Size-1) << 1;Liknande situation: V629 [CWE-190] Överväg att inspektera uttrycket 'Immr << 6'. Bitförskjutning av 32-bitarsvärdet med en efterföljande expansion till 64-bitarstyp. AArch64AddressingModes.h 269
Fragment N19: Sökord saknas annars?
void AMDGPUAsmParser::cvtDPP(MCInst &Inst, const OperandVector &Operands) {
....
if (Op.isReg() && Op.Reg.RegNo == AMDGPU::VCC) {
// VOP2b (v_add_u32, v_sub_u32 ...) dpp use "vcc" token.
// Skip it.
continue;
} if (isRegOrImmWithInputMods(Desc, Inst.getNumOperands())) { // <=
Op.addRegWithFPInputModsOperands(Inst, 2);
} else if (Op.isDPPCtrl()) {
Op.addImmOperands(Inst, 1);
} else if (Op.isImm()) {
// Handle optional arguments
OptionalIdx[Op.getImmTy()] = I;
} else {
llvm_unreachable("Invalid operand type");
}
....
}PVS-Studio varning: [CWE-670] Överväg att inspektera programmets logik. Det är möjligt att sökordet "annat" saknas. AMDGPAUsmParser.cpp 5655
Det är inget fel här. Sedan det dåvarande blocket av den första if slutar med fortsätta, då spelar det ingen roll, det finns ett nyckelord annars eller inte. Hur som helst kommer koden att fungera likadant. Fortfarande saknad annars gör koden mer otydlig och farlig. Om i framtiden fortsätta försvinner kommer koden att börja fungera helt annorlunda. Enligt min mening är det bättre att lägga till annars.
Fragment N20: Fyra stavfel av samma typ
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 varningar:
- V655 [CWE-480] Strängarna var sammanlänkade men används inte. Överväg att inspektera uttrycket 'Result + Name.str()'. Symbol.cpp 32
- V655 [CWE-480] Strängarna var sammanlänkade men används inte. Överväg att inspektera uttrycket 'Result + "(ObjC Class)" + Name.str()'. Symbol.cpp 35
- V655 [CWE-480] Strängarna var sammanlänkade men används inte. Överväg att inspektera uttrycket 'Result + "(ObjC Class EH) " + Name.str()'. Symbol.cpp 38
- V655 [CWE-480] Strängarna var sammanlänkade men används inte. Överväg att inspektera uttrycket 'Result + "(ObjC IVar)" + Name.str()'. Symbol.cpp 41
Av misstag används operatorn + istället för operatorn +=. Resultatet är design 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 cannot be empty");
for (auto &Op : Ops) {
assert(!Op.empty() && "Empty operator");
if (FeaturesMap.find(Op) == FeaturesMap.end())
FeaturesMap[Op] = FeaturesMap.size();
}
}
}Försök att hitta den farliga koden själv. Och det här är en bild för att distrahera uppmärksamheten för att inte direkt titta på svaret:
PVS-Studio varning: [CWE-758] Farlig konstruktion används: 'FeaturesMap[Op] = FeaturesMap.size()', där 'FeaturesMap' är av klassen 'map'. Detta kan leda till odefinierat beteende. RISCVCompressInstEmitter.cpp 490
Problemrad:
FeaturesMap[Op] = FeaturesMap.size();Om element Op inte hittas skapas ett nytt element i kartan och antalet element i denna karta skrivs där. Det är bara okänt om funktionen kommer att anropas Storlek före eller efter att ett nytt element lagts till.
Fragment N22-N24: Upprepade uppdrag
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 varning: [CWE-563] Variabeln 'NType' tilldelas värden två gånger i följd. Kanske är detta ett misstag. Kontrollera rader: 1663, 1664. MachOObjectFile.cpp 1664
Jag tror inte att det är ett riktigt misstag här. Bara ett onödigt upprepat uppdrag. Men fortfarande en blunder.
Liknande:
- V519 [CWE-563] Variabeln 'B.NDesc' tilldelas värden två gånger i följd. Kanske är detta ett misstag. Kontrollera rader: 1488, 1489. llvm-nm.cpp 1489
- V519 [CWE-563] Variabeln tilldelas värden två gånger i följd. Kanske är detta ett misstag. Kontrollera rader: 59, 61. coff2yaml.cpp 61
Fragment N25-N27: Fler omplaceringar
Låt oss nu titta på en lite annan version av omplacering.
bool Vectorizer::vectorizeLoadChain(
ArrayRef<Instruction *> Chain,
SmallPtrSet<Instruction *, 16> *InstructionsProcessed) {
....
unsigned Alignment = getAlignment(L0);
....
unsigned NewAlign = getOrEnforceKnownAlignment(L0->getPointerOperand(),
StackAdjustedAlignment,
DL, L0, nullptr, &DT);
if (NewAlign != 0)
Alignment = NewAlign;
Alignment = NewAlign;
....
}PVS-Studio varning: V519 [CWE-563] Variabeln 'Alignment' tilldelas värden två gånger i följd. Kanske är detta ett misstag. Kontrollera rader: 1158, 1160. LoadStoreVectorizer.cpp 1160
Detta är en mycket konstig kod som tydligen innehåller ett logiskt fel. I början, variabel Justering ett värde tilldelas beroende på villkoret. Och så inträffar uppdraget igen, men nu utan någon kontroll.
Liknande situationer kan ses här:
- V519 [CWE-563] Variabeln 'Effekter' tilldelas värden två gånger i följd. Kanske är detta ett misstag. Kontrollera rader: 152, 165. WebAssemblyRegStackify.cpp 165
- V519 [CWE-563] Variabeln 'ExpectNoDerefChunk' tilldelas värden två gånger i följd. Kanske är detta ett misstag. Kontrollera rader: 4970, 4973. SemaType.cpp 4973
Fragment N28: Alltid sant skick
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 varning: [CWE-571] Uttrycket 'nextByte != 0x90' är alltid sant. X86DisassemblerDecoder.cpp 379
Att kolla är meningslöst. Variabel nästaByte alltid inte lika med värdet 0x90, vilket följer av föregående kontroll. Detta är något slags logiskt fel.
Fragment N29 - N...: Alltid sanna/falska förhållanden
Analysatorn utfärdar många varningar om att hela tillståndet () eller del därav () är alltid sant eller falskt. Ofta är dessa inte riktiga fel, utan helt enkelt slarvig kod, resultatet av makroexpansion och liknande. Det är dock meningsfullt att titta på alla dessa varningar, eftersom äkta logiska fel uppstår då och då. Till exempel är det här avsnittet av koden 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: [CWE-570] En del av villkorligt uttryck är alltid falskt: RegNo == 0xe. ARMDisassembler.cpp 939
Konstanten 0xE är värdet 14 i decimal. Undersökning RegNo == 0xe är inte vettigt för om RegNo > 13, då kommer funktionen att slutföra sin körning.
Det fanns många andra varningar med ID V547 och V560, men som med , jag var inte intresserad av att studera dessa varningar. Det stod redan klart att jag hade tillräckligt med material för att skriva en artikel :). Därför är det okänt hur många fel av denna typ som kan identifieras i LLVM med PVS-Studio.
Jag ska ge dig ett exempel på varför det är tråkigt att studera dessa triggers. Analysatorn har helt rätt i att utfärda en varning för följande kod. Men detta är inte ett misstag.
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 retur
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: [CWE-670] En ovillkorlig "retur" inom en loop. R600OptimizeVectorRegisters.cpp 63
Detta är antingen ett fel eller en specifik teknik som är avsedd att förklara något för programmerare som läser koden. Den här designen förklarar ingenting för mig och ser väldigt misstänksam ut. Det är bättre att inte skriva så :).
Trött? Sedan är det dags att göra te eller kaffe.
Defekter identifierade av ny diagnostik
Jag tror att det räcker med 30 aktiveringar av gammal diagnostik. Låt oss nu se vilka intressanta saker som kan hittas med den nya diagnostiken som dök upp i analysatorn efter kontroller. Under denna tid lades totalt 66 allmändiagnostik till C++-analysatorn.
Fragment N31: Otillgänglig kod
Error CtorDtorRunner::run() {
....
if (auto CtorDtorMap =
ES.lookup(JITDylibSearchList({{&JD, true}}), std::move(Names),
NoDependenciesToRegister, true))
{
....
return Error::success();
} else
return CtorDtorMap.takeError();
CtorDtorsByPriority.clear();
return Error::success();
}PVS-Studio varning: [CWE-561] Otillgänglig kod upptäckt. Det är möjligt att ett fel föreligger. ExecutionUtils.cpp 146
Som du kan se, båda grenarna av operatören if avslutas med ett samtal till operatören avkastning. Följaktligen behållaren CtorDtorsByPriority kommer aldrig att rensas.
Fragment N32: Otillgänglig 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(), "unexpected summary kind");
}
Lex.setIgnoreColonInIdentifiers(false); // <=
return false;
}PVS-Studio varning: V779 [CWE-561] Otillgänglig kod upptäckt. Det är möjligt att ett fel föreligger. LLParser.cpp 835
Intressant situation. Låt oss först titta på denna plats:
return ParseTypeIdEntry(SummaryID);
break;Vid första anblicken verkar det som att det inte finns något fel här. Det ser ut som operatören bryta det finns en extra här, och du kan helt enkelt ta bort den. Dock inte allt så enkelt.
Analysatorn utfärdar en varning på raderna:
Lex.setIgnoreColonInIdentifiers(false);
return false;Och verkligen, den här koden går inte att nå. Alla fall i strömbrytare avslutas med ett samtal från operatören avkastning. Och nu sanslös ensam bryta ser inte så ofarligt ut! Kanske en av grenarna ska sluta med brytamen inte på avkastning?
Fragment N33: Slumpmässig återställning av höga bitar
unsigned getStubAlignment() override {
if (Arch == Triple::systemz)
return 8;
else
return 1;
}
Expected<unsigned>
RuntimeDyldImpl::emitSection(const ObjectFile &Obj,
const SectionRef &Section,
bool IsCode) {
....
uint64_t DataSize = Section.getSize();
....
if (StubBufSize > 0)
DataSize &= ~(getStubAlignment() - 1);
....
}PVS-Studio varning: Storleken på bitmasken är mindre än storleken på den första operanden. Detta kommer att orsaka förlust av högre bitar. RuntimeDyld.cpp 815
Observera att funktionen getStubAlignment returtyp unsigned. Låt oss beräkna värdet på uttrycket, förutsatt att funktionen returnerar värdet 8:
~(getStubAlignment() - 1)
~(8u-1)
0xFFFFFFFF8u
Lägg nu märke till att variabeln DataSize har en 64-bitars osignerad typ. Det visar sig att när du utför DataSize & 0xFFFFFFF8u operationen kommer alla 0 bitar av hög ordning att återställas till noll. Troligtvis är detta inte vad programmeraren ville ha. Jag misstänker att han ville räkna: DataSize & 8xFFFFFFFFFFFFFFFXNUMXu.
För att åtgärda felet bör du skriva detta:
DataSize &= ~(static_cast<uint64_t>(getStubAlignment()) - 1);Eller så:
DataSize &= ~(getStubAlignment() - 1ULL);Fragment N34: Misslyckad gjutning av explicit typ
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 varning: [CWE-190] Möjligt spill. Överväg att casta operander av operatorn 'NumElts * Scale' till typen 'size_t', inte resultatet. X86ISelLowering.h 1577
Explicit typgjutning används för att undvika spill när typvariabler multipliceras int. Explicit typgjutning här skyddar dock inte mot översvämning. Först kommer variablerna att multipliceras, och först då kommer 32-bitars resultatet av multiplikationen att utökas till typen .
Fragment N35: Misslyckades med att kopiera och klistra in
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;
}
....
}[CWE-682] Två liknande kodfragment hittades. Kanske är detta ett stavfel och variabeln 'Op1' bör användas istället för 'Op0'. InstCombineCompares.cpp 5507
Denna nya intressanta diagnostik identifierar situationer där en kodbit har kopierats och några namn i den har börjat ändras, men på ett ställe har de inte korrigerat det.
Observera att de ändrades i det andra blocket Op0 på Op1. Men på ett ställe fixade de det inte. Troligtvis borde det ha skrivits så här:
if (!match(Op1, m_PosZeroFP()) && isKnownNeverNaN(Op1, &TLI)) {
I.setOperand(1, ConstantFP::getNullValue(Op1->getType()));
return &I;
}Fragment N36: Variabel förvirring
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: [CWE-563] Variabeln 'Mode' är tilldelad men används inte i slutet av funktionen. SIModeRegister.cpp 48
Det är mycket farligt att ge funktionsargument samma namn som klassmedlemmar. Det är väldigt lätt att bli förvirrad. Vi har just ett sådant fall framför oss. Det här uttrycket är inte vettigt:
Mode &= Mask;Funktionsargumentet ändras. Det är allt. Detta argument används inte längre. Troligtvis borde du ha skrivit det så här:
Status(unsigned Mask, unsigned Mode) : Mask(Mask), Mode(Mode) {
this->Mode &= Mask;
};Fragment N37: Variabel förvirring
class SectionBase {
....
uint64_t Size = 0;
....
};
class SymbolTableSection : public SectionBase {
....
};
void SymbolTableSection::addSymbol(Twine Name, uint8_t Bind, uint8_t Type,
SectionBase *DefinedIn, uint64_t Value,
uint8_t Visibility, uint16_t Shndx,
uint64_t Size) {
....
Sym.Value = Value;
Sym.Visibility = Visibility;
Sym.Size = Size;
Sym.Index = Symbols.size();
Symbols.emplace_back(llvm::make_unique<Symbol>(Sym));
Size += this->EntrySize;
}Varning PVS-Studio: V1001 [CWE-563] Variabeln 'Storlek' är tilldelad men används inte i slutet av funktionen. Object.cpp 424
Situationen liknar den föregående. Det ska skrivas:
this->Size += this->EntrySize;Fragment N38-N47: De glömde kolla indexet
Tidigare har vi tittat på exempel på diagnostisk triggning . Dess väsen är att pekaren är bortreferens i början, och först då kontrolleras. Ung diagnostik är den motsatta i betydelsen, men avslöjar också en hel del fel. Den identifierar situationer där pekaren kontrollerades i början och sedan glömdes bort att göra det. Låt oss titta på sådana fall som finns i LLVM.
int getGEPCost(Type *PointeeType, const Value *Ptr,
ArrayRef<const Value *> Operands) {
....
if (Ptr != nullptr) { // <=
assert(....);
BaseGV = dyn_cast<GlobalValue>(Ptr->stripPointerCasts());
}
bool HasBaseReg = (BaseGV == nullptr);
auto PtrSizeBits = DL.getPointerTypeSizeInBits(Ptr->getType()); // <=
....
}PVS-Studio varning: V1004 [CWE-476] 'Ptr'-pekaren användes på ett osäkert sätt efter att den verifierats mot nullptr. Kontrollera rader: 729, 738. TargetTransformInfoImpl.h 738
Variabel PTR kan vara lika nullptr, vilket framgår av kontrollen:
if (Ptr != nullptr)Men nedanför denna pekare hänvisas utan preliminär kontroll:
auto PtrSizeBits = DL.getPointerTypeSizeInBits(Ptr->getType());Låt oss överväga ett annat liknande fall.
llvm::DISubprogram *CGDebugInfo::getFunctionFwdDeclOrStub(GlobalDecl GD,
bool Stub) {
....
auto *FD = dyn_cast<FunctionDecl>(GD.getDecl());
SmallVector<QualType, 16> ArgTypes;
if (FD) // <=
for (const ParmVarDecl *Parm : FD->parameters())
ArgTypes.push_back(Parm->getType());
CallingConv CC = FD->getType()->castAs<FunctionType>()->getCallConv(); // <=
....
}PVS-Studio varning: V1004 [CWE-476] 'FD'-pekaren användes på ett osäkert sätt efter att den verifierats mot nullptr. Kontrollera rader: 3228, 3231. CGDebugInfo.cpp 3231
Var uppmärksam på skylten FD. Jag är säker på att problemet är tydligt och ingen speciell förklaring krävs.
Och vidare:
static void computePolynomialFromPointer(Value &Ptr, Polynomial &Result,
Value *&BasePtr,
const DataLayout &DL) {
PointerType *PtrTy = dyn_cast<PointerType>(Ptr.getType());
if (!PtrTy) { // <=
Result = Polynomial();
BasePtr = nullptr;
}
unsigned PointerBits =
DL.getIndexSizeInBits(PtrTy->getPointerAddressSpace()); // <=
....
}PVS-Studio-varning: V1004 [CWE-476] 'PtrTy'-pekaren användes på ett osäkert sätt efter att den verifierats mot nullptr. Kontrollera rader: 960, 965. InterleavedLoadCombinePass.cpp 965
Hur skyddar man sig från sådana fel? Var mer uppmärksam på Code-Review och använd den statiska analysatorn PVS-Studio för att regelbundet kontrollera din kod.
Det är ingen idé att citera andra kodfragment med fel av denna typ. Jag lämnar bara en lista med varningar i artikeln:
- V1004 [CWE-476] 'Expr'-pekaren användes på ett osäkert sätt efter att den verifierats mot nullptr. Kontrollera rader: 1049, 1078. DebugInfoMetadata.cpp 1078
- V1004 [CWE-476] 'PI'-pekaren användes på ett osäkert sätt efter att den verifierats mot nullptr. Kontrollera rader: 733, 753. LegacyPassManager.cpp 753
- V1004 [CWE-476] 'StatepointCall'-pekaren användes på ett osäkert sätt efter att den verifierats mot nullptr. Kontrollera rader: 4371, 4379. Verifier.cpp 4379
- V1004 [CWE-476] 'RV'-pekaren användes på ett osäkert sätt efter att den verifierats mot nullptr. Kontrollera rader: 2263, 2268. TGParser.cpp 2268
- V1004 [CWE-476] 'CalleeFn'-pekaren användes på ett osäkert sätt efter att den verifierats mot nullptr. Kontrollera rader: 1081, 1096. SimplifyLibCalls.cpp 1096
- V1004 [CWE-476] 'TC'-pekaren användes på ett osäkert sätt efter att den verifierats mot nullptr. Kontrollera rader: 1819, 1824. Driver.cpp 1824
Fragment N48-N60: Inte kritiskt, men en defekt (möjlig minnesläcka)
std::unique_ptr<IRMutator> createISelMutator() {
....
std::vector<std::unique_ptr<IRMutationStrategy>> Strategies;
Strategies.emplace_back(
new InjectorIRStrategy(InjectorIRStrategy::getDefaultOps()));
....
}PVS-Studio varning: [CWE-460] En pekare utan ägare läggs till i 'Strategies'-behållaren med 'emplace_back'-metoden. En minnesläcka kommer att inträffa vid ett undantag. llvm-isel-fuzzer.cpp 58
För att lägga till ett element i slutet av en behållare som std::vektor > du kan inte bara skriva xxx.push_back(nytt X), eftersom det inte finns någon implicit konvertering från X* в std::unique_ptr.
En vanlig lösning är att skriva xxx.emplace_back(nytt X)eftersom den kompilerar: metod emplace_back konstruerar ett element direkt från dess argument och kan därför använda explicita konstruktorer.
Det är inte säkert. Om vektorn är full, återallokeras minnet. Åtgärden för omfördelning av minne kan misslyckas, vilket resulterar i att ett undantag skapas std::bad_alloc. I det här fallet kommer pekaren att gå förlorad och det skapade objektet kommer aldrig att raderas.
En säker lösning är att skapa unik_ptrsom kommer att äga pekaren innan vektorn försöker omfördela minne:
xxx.push_back(std::unique_ptr<X>(new X))Sedan C++14 kan du använda 'std::make_unique':
xxx.push_back(std::make_unique<X>())Denna typ av defekt är inte kritisk för LLVM. Om minne inte kan allokeras kommer kompilatorn helt enkelt att stoppa. Dock för applikationer med långa , som inte bara kan avslutas om minnesallokering misslyckas, detta kan vara ett riktigt otäckt fel.
Så även om den här koden inte utgör ett praktiskt hot mot LLVM, fann jag det användbart att prata om detta felmönster och att PVS-Studio-analysatorn har lärt sig att identifiera det.
Andra varningar av denna typ:
- V1023 [CWE-460] En pekare utan ägare läggs till i 'Passes'-behållaren med 'emplace_back'-metoden. En minnesläcka kommer att inträffa vid ett undantag. PassManager.h 546
- V1023 [CWE-460] En pekare utan ägare läggs till i 'AAs'-behållaren med metoden 'emplace_back'. En minnesläcka kommer att inträffa vid ett undantag. AliasAnalysis.h 324
- V1023 [CWE-460] En pekare utan ägare läggs till i 'Entries'-behållaren med 'emplace_back'-metoden. En minnesläcka kommer att inträffa vid ett undantag. DWARFDebugFrame.cpp 519
- V1023 [CWE-460] En pekare utan ägare läggs till i 'AllEdges'-behållaren med metoden 'emplace_back'. En minnesläcka kommer att inträffa vid ett undantag. CFGMST.h 268
- V1023 [CWE-460] En pekare utan ägare läggs till i 'VMaps'-behållaren med metoden 'emplace_back'. En minnesläcka kommer att inträffa vid ett undantag. SimpleLoopUswitch.cpp 2012
- V1023 [CWE-460] En pekare utan ägare läggs till i 'Records'-behållaren med 'emplace_back'-metoden. En minnesläcka kommer att inträffa vid ett undantag. FDRLogBuilder.h 30
- V1023 [CWE-460] En pekare utan ägare läggs till i 'PendingSubmodules'-behållaren med metoden 'emplace_back'. En minnesläcka kommer att inträffa vid ett undantag. ModuleMap.cpp 810
- V1023 [CWE-460] En pekare utan ägare läggs till i 'Objekt'-behållaren med metoden 'emplace_back'. En minnesläcka kommer att inträffa vid ett undantag. DebugMap.cpp 88
- V1023 [CWE-460] En pekare utan ägare läggs till i 'Strategies'-behållaren med 'emplace_back'-metoden. En minnesläcka kommer att inträffa vid ett undantag. llvm-isel-fuzzer.cpp 60
- V1023 [CWE-460] En pekare utan ägare läggs till i 'Modifiers'-behållaren med 'emplace_back'-metoden. En minnesläcka kommer att inträffa vid ett undantag. llvm-stress.cpp 685
- V1023 [CWE-460] En pekare utan ägare läggs till i 'Modifiers'-behållaren med 'emplace_back'-metoden. En minnesläcka kommer att inträffa vid ett undantag. llvm-stress.cpp 686
- V1023 [CWE-460] En pekare utan ägare läggs till i 'Modifiers'-behållaren med 'emplace_back'-metoden. En minnesläcka kommer att inträffa vid ett undantag. llvm-stress.cpp 688
- V1023 [CWE-460] En pekare utan ägare läggs till i 'Modifiers'-behållaren med 'emplace_back'-metoden. En minnesläcka kommer att inträffa vid ett undantag. llvm-stress.cpp 689
- V1023 [CWE-460] En pekare utan ägare läggs till i 'Modifiers'-behållaren med 'emplace_back'-metoden. En minnesläcka kommer att inträffa vid ett undantag. llvm-stress.cpp 690
- V1023 [CWE-460] En pekare utan ägare läggs till i 'Modifiers'-behållaren med 'emplace_back'-metoden. En minnesläcka kommer att inträffa vid ett undantag. llvm-stress.cpp 691
- V1023 [CWE-460] En pekare utan ägare läggs till i 'Modifiers'-behållaren med 'emplace_back'-metoden. En minnesläcka kommer att inträffa vid ett undantag. llvm-stress.cpp 692
- V1023 [CWE-460] En pekare utan ägare läggs till i 'Modifiers'-behållaren med 'emplace_back'-metoden. En minnesläcka kommer att inträffa vid ett undantag. llvm-stress.cpp 693
- V1023 [CWE-460] En pekare utan ägare läggs till i 'Modifiers'-behållaren med 'emplace_back'-metoden. En minnesläcka kommer att inträffa vid ett undantag. llvm-stress.cpp 694
- V1023 [CWE-460] En pekare utan ägare läggs till i 'Operander'-behållaren med metoden 'emplace_back'. En minnesläcka kommer att inträffa vid ett undantag. GlobalISelEmitter.cpp 1911
- V1023 [CWE-460] En pekare utan ägare läggs till i 'Stash'-behållaren med metoden 'emplace_back'. En minnesläcka kommer att inträffa vid ett undantag. GlobalISelEmitter.cpp 2100
- V1023 [CWE-460] En pekare utan ägare läggs till i 'Matchers'-behållaren med metoden 'emplace_back'. En minnesläcka kommer att inträffa vid ett undantag. GlobalISelEmitter.cpp 2702
Slutsats
Jag utfärdade 60 varningar totalt och slutade sedan. Finns det andra defekter som PVS-Studio-analysatorn upptäcker i LLVM? Ja det har jag. Men när jag skrev ut kodfragment till artikeln var det sen kväll, eller snarare till och med natt, och jag bestämde mig för att det var dags att kalla det en dag.
Jag hoppas att du tyckte att det var intressant och att du vill prova analysatorn PVS-Studio.
Du kan ladda ner analysatorn och få minröjningsnyckeln på .
Viktigast av allt, använd statisk analys regelbundet. Engångskontroller, utförda av oss för att popularisera metodiken för statisk analys och PVS-Studio är inte ett normalt scenario.
Lycka till med att förbättra kvaliteten och tillförlitligheten för din kod!
Om du vill dela den här artikeln med en engelsktalande publik, använd gärna översättningslänken: Andrey Karpov. .
Källa: will.com