Prošlo je više od dvije godine od posljednje provjere koda LLVM projekta pomoću našeg PVS-Studio analizatora. Pobrinimo se da PVS-Studio analizator i dalje bude vodeći alat za identifikaciju grešaka i potencijalnih ranjivosti. Da bismo to učinili, provjerit ćemo i pronaći nove greške u izdanju LLVM 8.0.0.
Članak treba napisati
Da budem iskren, nisam želio da pišem ovaj članak. Nije zanimljivo pisati o projektu koji smo već nekoliko puta provjerili (
Svaki put kada se objavi ili ažurira nova verzija LLVM-a
Gledajte, nova verzija Clang Static Analyzer je naučila da pronađe nove greške! Čini mi se da je relevantnost korištenja PVS-Studio sve manja. Clang pronalazi više grešaka nego ranije i sustiže mogućnosti PVS-Studio. Šta mislite o ovome?
Na ovo uvek želim da odgovorim nešto poput:
Ni mi ne sedimo besposleni! Značajno smo poboljšali mogućnosti PVS-Studio analizatora. Zato ne brinite, nastavljamo da vodimo kao i do sada.
Nažalost, ovo je loš odgovor. U tome nema dokaza. I zato sada pišem ovaj članak. Dakle, LLVM projekat je još jednom provjeren i u njemu su pronađene razne greške. Sada ću pokazati one koji su mi se činili zanimljivi. Clang Static Analyzer ne može pronaći ove greške (ili je krajnje nezgodno to učiniti uz njegovu pomoć). Ali možemo. Štaviše, sve ove greške sam pronašao i zapisao u jednoj večeri.
Ali pisanje članka je trajalo nekoliko sedmica. Jednostavno se nisam mogao natjerati da sve ovo stavim u tekst :).
Usput, ako vas zanima koje se tehnologije koriste u analizatoru PVS-Studio za identifikaciju grešaka i potencijalnih ranjivosti, onda predlažem da se upoznate s ovim
Nova i stara dijagnostika
Kao što je već navedeno, prije otprilike dvije godine LLVM projekat je još jednom provjeren, a pronađene greške su ispravljene. Sada će ovaj članak predstaviti novu grupu grešaka. Zašto su pronađene nove greške? Za to postoje 3 razloga:
- LLVM projekat se razvija, mijenja stari kod i dodaje novi kod. Naravno, ima novih grešaka u modifikovanom i pisanom kodu. Ovo jasno pokazuje da statičku analizu treba koristiti redovno, a ne povremeno. Naši članci dobro pokazuju mogućnosti analizatora PVS-Studio, ali to nema nikakve veze sa poboljšanjem kvaliteta koda i smanjenjem troškova ispravljanja grešaka. Redovno koristite statički analizator koda!
- Dovršavamo i unapređujemo postojeću dijagnostiku. Stoga analizator može identificirati greške koje nije primijetio tokom prethodnih skeniranja.
- U PVS-Studiju se pojavila nova dijagnostika koja nije postojala prije 2 godine. Odlučio sam da ih istaknem u posebnom dijelu kako bih jasno prikazao razvoj PVS-Studija.
Defekti utvrđeni dijagnostikom koji su postojali prije 2 godine
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 upozorenje:
Dvostruko se provjerava da ime počinje podnizom "avx512.mask.permvar.". U drugoj provjeri očito su htjeli nešto drugo napisati, ali su zaboravili ispraviti kopirani tekst.
Fragment N2: Tipska greška
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;
....
}
Upozorenje PVS-Studio: V501 Postoje identični podizrazi 'CXNameRange_WantQualifier' lijevo i desno od '|' operater. CIndex.cpp 7245
Zbog greške u kucanju ista imenovana konstanta se koristi dva puta CXNameRange_WantQualifier.
Fragment N3: Zabuna s prioritetom operatora
int PPCTTIImpl::getVectorInstrCost(unsigned Opcode, Type *Val, unsigned Index) {
....
if (ISD == ISD::EXTRACT_VECTOR_ELT && Index == ST->isLittleEndian() ? 1 : 0)
return 0;
....
}
PVS-Studio upozorenje:
Po mom mišljenju, ovo je veoma lepa greška. Da, znam da imam čudne ideje o lepoti :).
Sada, prema
(ISD == ISD::EXTRACT_VECTOR_ELT && (Index == ST->isLittleEndian())) ? 1 : 0
Sa praktične tačke gledišta, takav uslov nema smisla, jer se može svesti na:
(ISD == ISD::EXTRACT_VECTOR_ELT && Index == ST->isLittleEndian())
Ovo je jasna greška. Najvjerovatnije su htjeli uporediti 0/1 sa varijablom indeks. Da popravite kod, morate dodati zagrade oko ternarnog operatora:
if (ISD == ISD::EXTRACT_VECTOR_ELT && Index == (ST->isLittleEndian() ? 1 : 0))
Inače, ternarni operator je vrlo opasan i izaziva logičke greške. Budite veoma oprezni s tim i nemojte biti pohlepni sa zagradama. Pogledao sam ovu temu detaljnije
Fragment N4, N5: Null pokazivač
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 upozorenje:
Ako je pokazivač LHS je null, treba izdati upozorenje. Međutim, umjesto toga, ovaj isti null pokazivač će biti dereferenciran: LHS->getAsString().
Ovo je vrlo tipična situacija kada je greška skrivena u rukovaocu greškama, jer ih niko ne testira. Statički analizatori provjeravaju sav dostupan kod, bez obzira koliko se često koristi. Ovo je vrlo dobar primjer kako statička analiza nadopunjuje druge tehnike testiranja i zaštite od grešaka.
Slična greška u rukovanju pokazivačem RHS dozvoljeno u kodu ispod: V522 [CWE-476] Dereferenciranje nulte pokazivača 'RHS' može se dogoditi. TGParser.cpp 2186
Fragment N6: Upotreba pokazivača nakon kretanja
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 Upozorenje: V522 [CWE-476] Dereferenciranje nulte pokazivača 'ProgClone' može se dogoditi. Miscompilation.cpp 601
Na početku pametan pokazivač ProgClone prestaje vlasništvo nad objektom:
BD.setNewProgram(std::move(ProgClone));
U stvari, sada ProgClone je nulti pokazivač. Stoga bi se nulta dereferenca pokazivača trebala pojaviti odmah ispod:
Function *NewF = ProgClone->getFunction(MisCompFunctions[i].first);
Ali, u stvarnosti, to se neće dogoditi! Imajte na umu da se petlja zapravo ne izvršava.
Na početku kontejnera MiscompiledFunctions očišćeno:
MiscompiledFunctions.clear();
Zatim se veličina ovog kontejnera koristi u stanju petlje:
for (unsigned i = 0, e = MisCompFunctions.size(); i != e; ++i) {
Lako je vidjeti da petlja ne počinje. Mislim da je i ovo greška i kod bi trebao biti napisan drugačije.
Čini se da smo naišli na taj famozni paritet grešaka! Jedna greška maskira drugu :).
Fragment N7: Upotreba pokazivača nakon kretanja
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 upozorenje: V522 [CWE-476] Dereferenciranje nultog pokazivača 'Test' može se dogoditi. Miscompilation.cpp 709
Opet ista situacija. Prvo se pomera sadržaj objekta, a zatim se koristi kao da se ništa nije dogodilo. Ovu situaciju sve češće viđam u programskom kodu nakon što se u C++ pojavila semantika pokreta. Zbog toga volim C++ jezik! Sve je više novih načina da sebi odbijete nogu. PVS-Studio analizator će uvijek imati posla :).
Fragment N8: Null pokazivač
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 upozorenje: V522 [CWE-476] Dereferenciranje nulte pokazivača 'Type' može se dogoditi. PrettyFunctionDumper.cpp 233
Osim rukovatelja greškama, funkcije ispisa za otklanjanje grešaka obično se ne testiraju. Pred nama je upravo takav slučaj. Funkcija čeka korisnika, koji će, umjesto da rješava svoje probleme, biti primoran da je popravi.
Tačno:
if (Type)
Type->dump(*this);
else
Printer << "<unknown-type>";
Fragment N9: Null pokazivač
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 upozorenje: V522 [CWE-476] Može doći do dereferenciranja nulte pokazivača 'Ty'. SearchableTableEmitter.cpp 614
Mislim da je sve jasno i ne zahteva objašnjenje.
Fragment N10: Tipska greška
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 upozorenje:
Nema smisla dodijeliti varijablu samoj sebi. Najvjerovatnije su htjeli napisati:
Identifier->Type = Question->Type;
Fragment N11: Sumnjiv prekid
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 upozorenje:
Na početku je vrlo sumnjiv operater pauza. Da li ste zaboravili da napišete još nešto ovde?
Fragment N12: Provjera pokazivača nakon dereferenciranja
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 upozorenje:
Pointer Callee na početku je dereferenciran u trenutku kada je funkcija pozvana getTTI.
A onda se ispostavlja da ovaj pokazivač treba provjeriti na jednakost nullptr:
if (!Callee || Callee->isDeclaration())
Ali prekasno je…
Fragment N13 - N...: Provjera pokazivača nakon dereferenciranja
Situacija o kojoj se govori u prethodnom fragmentu koda nije jedinstvena. Ovdje se pojavljuje:
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 upozorenje: V595 [CWE-476] 'CalleeFn' pokazivač je korišten prije nego što je verifikovan u odnosu na nullptr. Provjerite linije: 1079, 1081. SimplifyLibCalls.cpp 1079
i ovdje:
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 upozorenje: V595 [CWE-476] 'ND' pokazivač je korišten prije nego što je verifikovan u odnosu na nullptr. Kontrolni redovi: 532, 534. SemaTemplateInstantiateDecl.cpp 532
i ovdje:
- V595 [CWE-476] 'U' pokazivač je korišten prije nego što je verificiran u odnosu na nullptr. Kontrolni redovi: 404, 407. DWARFormValue.cpp 404
- V595 [CWE-476] 'ND' pokazivač je korišten prije nego što je verificiran prema nullptr. Kontrolni redovi: 2149, 2151. SemaTemplateInstantiate.cpp 2149
A onda sam postao nezainteresovan za proučavanje upozorenja sa brojem V595. Tako da ne znam ima li još sličnih grešaka osim ovdje navedenih. Najvjerovatnije postoji.
Fragment N17, N18: Sumnjiv pomak
static inline bool processLogicalImmediate(uint64_t Imm, unsigned RegSize,
uint64_t &Encoding) {
....
unsigned Size = RegSize;
....
uint64_t NImms = ~(Size-1) << 1;
....
}
PVS-Studio upozorenje:
Možda nije greška i kod radi tačno kako je predviđeno. Ali ovo je očigledno vrlo sumnjivo mjesto i treba ga provjeriti.
Recimo varijabla veličina je jednak 16, a onda je autor koda planirao da ga dobije u promenljivoj NImms vrednost:
1111111111111111111111111111111111111111111111111111111111100000
Međutim, u stvarnosti rezultat će biti:
0000000000000000000000000000000011111111111111111111111111100000
Činjenica je da se svi proračuni odvijaju koristeći 32-bitni neoznačeni tip. I tek tada će ovaj 32-bitni nepotpisani tip biti implicitno proširen na uint64_t. U ovom slučaju najvažniji bitovi će biti nula.
Situaciju možete popraviti ovako:
uint64_t NImms = ~static_cast<uint64_t>(Size-1) << 1;
Slična situacija: V629 [CWE-190] Razmislite o pregledu izraza 'Immr << 6'. Pomeranje bita 32-bitne vrednosti sa naknadnim proširenjem na 64-bitni tip. AArch64AddressingModes.h 269
Fragment N19: Nedostaje ključna riječ drugo?
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 upozorenje:
Ovdje nema greške. Od tada-bloka prvog if završava sa nastaviti, onda nije bitno, postoji ključna riječ drugo ili ne. U svakom slučaju, kod će raditi isto. Još promašen drugo čini kod nejasnijim i opasnijim. Ako u budućnosti nastaviti nestane, kod će početi raditi potpuno drugačije. Po mom mišljenju bolje je dodati drugo.
Fragment N20: Četiri greške u kucanju iste vrste
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 upozorenja:
- V655 [CWE-480] Nizovi su spojeni, ali se ne koriste. Razmislite o pregledu izraza 'Rezultat + Name.str()'. Symbol.cpp 32
- V655 [CWE-480] Nizovi su spojeni, ali se ne koriste. Razmislite o pregledu izraza 'Rezultat + "(ObjC Class)" + Name.str()'. Symbol.cpp 35
- V655 [CWE-480] Nizovi su spojeni, ali se ne koriste. Razmislite o pregledu izraza 'Rezultat + "(ObjC Class EH) " + Name.str()'. Symbol.cpp 38
- V655 [CWE-480] Nizovi su spojeni, ali se ne koriste. Razmislite o pregledu izraza 'Rezultat + "(ObjC IVar)" + Name.str()'. Symbol.cpp 41
Slučajno se koristi operator + umjesto operatora +=. Rezultat su dizajni koji su lišeni značenja.
Fragment N21: Nedefinirano ponašanje
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();
}
}
}
Pokušajte sami pronaći opasnu šifru. A ovo je slika za odvlačenje pažnje kako ne biste odmah pogledali odgovor:
PVS-Studio upozorenje:
Linija problema:
FeaturesMap[Op] = FeaturesMap.size();
If element Op nije pronađen, tada se kreira novi element u mapi i tamo se upisuje broj elemenata u ovoj mapi. Samo je nepoznato da li će funkcija biti pozvana veličina prije ili nakon dodavanja novog elementa.
Fragment N22-N24: Ponovljeni zadaci
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 upozorenje:
Mislim da ovde nema prave greške. Samo nepotreban ponovljeni zadatak. Ali ipak greška.
Slično tome:
- V519 [CWE-563] Varijabli 'B.NDesc' se dodjeljuju vrijednosti dvaput uzastopno. Možda je ovo greška. Kontrolni redovi: 1488, 1489. llvm-nm.cpp 1489
- V519 [CWE-563] Varijabli se dodjeljuju vrijednosti dvaput uzastopno. Možda je ovo greška. Kontrolni redovi: 59, 61. coff2yaml.cpp 61
Fragment N25-N27: Još preimenovanja
Pogledajmo sada malo drugačiju verziju preraspodjele.
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;
....
}
Upozorenje PVS-Studio: V519 [CWE-563] Varijabli 'Alignment' se dodjeljuju vrijednosti dvaput uzastopno. Možda je ovo greška. Kontrolne linije: 1158, 1160. LoadStoreVectorizer.cpp 1160
Ovo je vrlo čudan kod koji očigledno sadrži logičku grešku. Na početku, varijabilna Poravnanje vrijednost se dodjeljuje ovisno o stanju. A onda se dodjela ponovo javlja, ali sada bez ikakve provjere.
Slične situacije možete vidjeti ovdje:
- V519 [CWE-563] Varijabli 'Effects' se dodjeljuju vrijednosti dvaput uzastopno. Možda je ovo greška. Kontrolni redovi: 152, 165. WebAssemblyRegStackify.cpp 165
- V519 [CWE-563] Varijabli 'ExpectNoDerefChunk' se dodjeljuju vrijednosti dvaput uzastopno. Možda je ovo greška. Kontrolni redovi: 4970, 4973. SemaType.cpp 4973
Fragment N28: Uvek istinito stanje
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 upozorenje:
Provjeravanje nema smisla. Varijabilna nextByte uvijek nije jednak vrijednosti 0x90, što slijedi iz prethodne provjere. Ovo je neka vrsta logičke greške.
Fragment N29 - N...: Uvek tačni/netačni uslovi
Analizator izdaje mnoga upozorenja da je cijelo stanje (
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 upozorenje:
Konstanta 0xE je vrijednost 14 u decimalnom obliku. Ispitivanje RegNo == 0xe nema smisla jer ako RegNo > 13, tada će funkcija završiti svoje izvršavanje.
Bilo je mnogo drugih upozorenja sa ID-ovima V547 i V560, ali kao i sa
Dat ću vam primjer zašto je proučavanje ovih okidača dosadno. Analizator je potpuno u pravu kada je izdao upozorenje za sljedeći kod. Ali ovo nije greška.
bool UnwrappedLineParser::parseBracedList(bool ContinueOnSemicolons,
tok::TokenKind ClosingBraceKind) {
bool HasError = false;
....
HasError = true;
if (!ContinueOnSemicolons)
return !HasError;
....
}
Upozorenje PVS-Studio: V547 [CWE-570] Izraz '!HasError' je uvijek lažan. UnwrappedLineParser.cpp 1635
Fragment N30: Sumnjiv povratak
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 upozorenje:
Ovo je ili greška ili specifična tehnika koja ima za cilj da objasni nešto programerima koji čitaju kod. Ovaj dizajn mi ništa ne objašnjava i izgleda vrlo sumnjivo. Bolje je ne pisati tako :).
Umoran? Onda je vrijeme za skuhanje čaja ili kafe.
Defekti otkriveni novom dijagnostikom
Mislim da je dovoljno 30 aktivacija stare dijagnostike. Pogledajmo sada koje se zanimljive stvari mogu pronaći s novom dijagnostikom koja se kasnije pojavila u analizatoru
Fragment N31: Nedostupan 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 upozorenje:
Kao što vidite, obje grane operatora if završava pozivom operateru povratak. Prema tome, kontejner CtorDtorsByPriority nikada neće biti očišćena.
Fragment N32: Nedostupan 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 upozorenje: V779 [CWE-561] Otkriven nedostupan kod. Moguće je da postoji greška. LLParser.cpp 835
Zanimljiva situacija. Pogledajmo prvo ovo mjesto:
return ParseTypeIdEntry(SummaryID);
break;
Na prvi pogled se čini da tu nema greške. Izgleda kao operater pauza postoji još jedan ovdje i možete ga jednostavno izbrisati. Međutim, nije sve tako jednostavno.
Analizator izdaje upozorenje na redovima:
Lex.setIgnoreColonInIdentifiers(false);
return false;
I zaista, ovaj kod je nedostižan. Svi slučajevi u prekidač završava pozivom operatera povratak. A sada besmisleno sama pauza ne izgleda tako bezazleno! Možda bi jedna od grana trebala završiti sa pauzane na povratak?
Fragment N33: Nasumično resetovanje visokih bitova
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 upozorenje:
Imajte na umu da je funkcija getStubAlignment vraća tip nepotpisan. Izračunajmo vrijednost izraza, pod pretpostavkom da funkcija vraća vrijednost 8:
~(getStubAlignment() - 1)
~(8u-1)
0xFFFFFFFF8u
Sada primijetite da je varijabla DataSize ima 64-bitni neoznačeni tip. Ispostavilo se da će prilikom izvođenja operacije DataSize & 0xFFFFFFF8u, sva trideset dva bita visokog reda biti vraćena na nulu. Najvjerovatnije to nije ono što je programer želio. Pretpostavljam da je htio izračunati: DataSize & 0xFFFFFFFFFFFFFFF8u.
Da biste ispravili grešku, trebali biste napisati ovo:
DataSize &= ~(static_cast<uint64_t>(getStubAlignment()) - 1);
Ili tako:
DataSize &= ~(getStubAlignment() - 1ULL);
Fragment N34: Neuspješno prebacivanje eksplicitnog tipa
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 upozorenje:
Eksplicitno uvođenje tipova se koristi da bi se izbjeglo prelijevanje pri množenju varijabli tipa Int. Međutim, eksplicitno uvođenje tipa ovdje ne štiti od prelijevanja. Prvo će se varijable pomnožiti, a tek onda će se 32-bitni rezultat množenja proširiti na tip
Fragment N35: Neuspješno kopiranje i lijepljenje
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;
}
....
}
Ova nova zanimljiva dijagnostika identifikuje situacije u kojima je deo koda kopiran i neka imena u njemu su počela da se menjaju, ali na jednom mestu to nisu ispravili.
Napominjemo da su se u drugom bloku promijenili Op0 na Op1. Ali na jednom mjestu to nisu popravili. Najvjerovatnije je trebalo ovako napisati:
if (!match(Op1, m_PosZeroFP()) && isKnownNeverNaN(Op1, &TLI)) {
I.setOperand(1, ConstantFP::getNullValue(Op1->getType()));
return &I;
}
Fragment N36: Varijabilna konfuzija
struct Status {
unsigned Mask;
unsigned Mode;
Status() : Mask(0), Mode(0){};
Status(unsigned Mask, unsigned Mode) : Mask(Mask), Mode(Mode) {
Mode &= Mask;
};
....
};
PVS-Studio upozorenje:
Vrlo je opasno argumentima funkcije dati ista imena kao i članovima klase. Lako se zbuniti. Pred nama je upravo takav slučaj. Ovaj izraz nema smisla:
Mode &= Mask;
Argument funkcije se mijenja. To je sve. Ovaj argument se više ne koristi. Najvjerovatnije ste to trebali napisati ovako:
Status(unsigned Mask, unsigned Mode) : Mask(Mask), Mode(Mode) {
this->Mode &= Mask;
};
Fragment N37: Varijabilna konfuzija
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;
}
Upozorenje PVS-Studio: V1001 [CWE-563] Varijabla 'Size' je dodijeljena, ali se ne koristi do kraja funkcije. Object.cpp 424
Situacija je slična prethodnoj. Trebalo bi da bude napisano:
this->Size += this->EntrySize;
Fragment N38-N47: Zaboravili su provjeriti indeks
Prethodno smo pogledali primjere dijagnostičkog pokretanja
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 upozorenje: V1004 [CWE-476] 'Ptr' pokazivač je korišten nesigurno nakon što je verifikovan u odnosu na nullptr. Kontrolni redovi: 729, 738. TargetTransformInfoImpl.h 738
Promjenjivo Ptr mogu biti jednaki nullptr, o čemu svjedoči provjera:
if (Ptr != nullptr)
Međutim, ispod ovog pokazivača se dereferencira bez preliminarne provjere:
auto PtrSizeBits = DL.getPointerTypeSizeInBits(Ptr->getType());
Razmotrimo još jedan sličan slučaj.
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 upozorenje: V1004 [CWE-476] 'FD' pokazivač je korišćen nesigurno nakon što je verifikovan u odnosu na nullptr. Kontrolni redovi: 3228, 3231. CGDebugInfo.cpp 3231
Obratite pažnju na znak FD. Siguran sam da je problem jasno vidljiv i nije potrebno posebno objašnjenje.
I dalje:
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 upozorenje: V1004 [CWE-476] 'PtrTy' pokazivač je nebezbedno korišćen nakon što je verifikovan u odnosu na nullptr. Kontrolni redovi: 960, 965. InterleavedLoadCombinePass.cpp 965
Kako se zaštititi od ovakvih grešaka? Budite pažljiviji na Code-Review i koristite PVS-Studio statički analizator da redovno provjeravate svoj kod.
Nema smisla citirati druge fragmente koda s greškama ovog tipa. U članku ću ostaviti samo listu upozorenja:
- V1004 [CWE-476] 'Expr' pokazivač je korišten nesigurno nakon što je provjeren u odnosu na nullptr. Provjerite linije: 1049, 1078. DebugInfoMetadata.cpp 1078
- V1004 [CWE-476] 'PI' pokazivač je korišten nesigurno nakon što je verificiran prema nullptr. Kontrolne linije: 733, 753. LegacyPassManager.cpp 753
- V1004 [CWE-476] Pokazivač 'StatepointCall' korišten je nesigurno nakon što je verificiran prema nullptr. Kontrolni redovi: 4371, 4379. Verifier.cpp 4379
- V1004 [CWE-476] 'RV' pokazivač je korišten nesigurno nakon što je provjeren u odnosu na nullptr. Kontrolne linije: 2263, 2268. TGParser.cpp 2268
- V1004 [CWE-476] 'CalleeFn' pokazivač je korišten nesigurno nakon što je provjeren u odnosu na nullptr. Kontrolne linije: 1081, 1096. SimplifyLibCalls.cpp 1096
- V1004 [CWE-476] 'TC' pokazivač je korišten nesigurno nakon što je provjeren u odnosu na nullptr. Kontrolni redovi: 1819, 1824. Driver.cpp 1824
Fragment N48-N60: Nije kritično, ali je kvar (moguće curenje memorije)
std::unique_ptr<IRMutator> createISelMutator() {
....
std::vector<std::unique_ptr<IRMutationStrategy>> Strategies;
Strategies.emplace_back(
new InjectorIRStrategy(InjectorIRStrategy::getDefaultOps()));
....
}
PVS-Studio upozorenje:
Da biste dodali element na kraj kontejnera kao što je std::vector > ne možeš samo da pišeš xxx.push_back(novi X), budući da ne postoji implicitna konverzija iz X* в std::unique_ptr.
Uobičajeno rješenje je pisanje xxx.emplace_back (novi X)pošto kompajlira: metod emplace_back konstruiše element direktno iz njegovih argumenata i stoga može koristiti eksplicitne konstruktore.
Nije sigurno. Ako je vektor pun, tada se memorija ponovo dodjeljuje. Operacija preraspodjele memorije možda neće uspjeti, što rezultira izbacivanjem izuzetka std::bad_alloc. U tom slučaju, pokazivač će biti izgubljen i kreirani objekt nikada neće biti obrisan.
Sigurno rješenje je stvaranje unique_ptrkoji će posjedovati pokazivač prije nego što vektor pokuša ponovno dodijeliti memoriju:
xxx.push_back(std::unique_ptr<X>(new X))
Od C++14, možete koristiti 'std::make_unique':
xxx.push_back(std::make_unique<X>())
Ova vrsta kvara nije kritična za LLVM. Ako se memorija ne može dodijeliti, kompajler će jednostavno stati. Međutim, za aplikacije sa dugim
Dakle, iako ovaj kod ne predstavlja praktičnu prijetnju LLVM-u, smatrao sam korisnim govoriti o ovom obrascu greške i da je PVS-Studio analizator naučio da ga identificira.
Ostala upozorenja ove vrste:
- V1023 [CWE-460] Pokazivač bez vlasnika se dodaje u kontejner 'Prolazi' metodom 'emplace_back'. Doći će do curenja memorije u slučaju izuzetka. PassManager.h 546
- V1023 [CWE-460] Pokazivač bez vlasnika se dodaje u 'AAs' kontejner metodom 'emplace_back'. Doći će do curenja memorije u slučaju izuzetka. AliasAnalysis.h 324
- V1023 [CWE-460] Pokazivač bez vlasnika se dodaje u kontejner 'Entries' metodom 'emplace_back'. Doći će do curenja memorije u slučaju izuzetka. DWARFDebugFrame.cpp 519
- V1023 [CWE-460] Pokazivač bez vlasnika se dodaje u 'AllEdges' kontejner metodom 'emplace_back'. Doći će do curenja memorije u slučaju izuzetka. CFGMST.h 268
- V1023 [CWE-460] Pokazivač bez vlasnika se dodaje u 'VMaps' kontejner metodom 'emplace_back'. Doći će do curenja memorije u slučaju izuzetka. SimpleLoopUnswitch.cpp 2012
- V1023 [CWE-460] Pokazivač bez vlasnika se dodaje u 'Records' kontejner metodom 'emplace_back'. Doći će do curenja memorije u slučaju izuzetka. FDRLogBuilder.h 30
- V1023 [CWE-460] Pointer bez vlasnika se dodaje u 'PendingSubmodules' kontejner metodom 'emplace_back'. Doći će do curenja memorije u slučaju izuzetka. ModuleMap.cpp 810
- V1023 [CWE-460] Pointer bez vlasnika se dodaje u kontejner 'Objekti' metodom 'emplace_back'. Doći će do curenja memorije u slučaju izuzetka. DebugMap.cpp 88
- V1023 [CWE-460] Pokazivač bez vlasnika se dodaje u kontejner 'Strategije' metodom 'emplace_back'. Doći će do curenja memorije u slučaju izuzetka. llvm-isel-fuzzer.cpp 60
- V1023 [CWE-460] Pokazivač bez vlasnika se dodaje u kontejner 'Modifikatori' metodom 'emplace_back'. Doći će do curenja memorije u slučaju izuzetka. llvm-stress.cpp 685
- V1023 [CWE-460] Pokazivač bez vlasnika se dodaje u kontejner 'Modifikatori' metodom 'emplace_back'. Doći će do curenja memorije u slučaju izuzetka. llvm-stress.cpp 686
- V1023 [CWE-460] Pokazivač bez vlasnika se dodaje u kontejner 'Modifikatori' metodom 'emplace_back'. Doći će do curenja memorije u slučaju izuzetka. llvm-stress.cpp 688
- V1023 [CWE-460] Pokazivač bez vlasnika se dodaje u kontejner 'Modifikatori' metodom 'emplace_back'. Doći će do curenja memorije u slučaju izuzetka. llvm-stress.cpp 689
- V1023 [CWE-460] Pokazivač bez vlasnika se dodaje u kontejner 'Modifikatori' metodom 'emplace_back'. Doći će do curenja memorije u slučaju izuzetka. llvm-stress.cpp 690
- V1023 [CWE-460] Pokazivač bez vlasnika se dodaje u kontejner 'Modifikatori' metodom 'emplace_back'. Doći će do curenja memorije u slučaju izuzetka. llvm-stress.cpp 691
- V1023 [CWE-460] Pokazivač bez vlasnika se dodaje u kontejner 'Modifikatori' metodom 'emplace_back'. Doći će do curenja memorije u slučaju izuzetka. llvm-stress.cpp 692
- V1023 [CWE-460] Pokazivač bez vlasnika se dodaje u kontejner 'Modifikatori' metodom 'emplace_back'. Doći će do curenja memorije u slučaju izuzetka. llvm-stress.cpp 693
- V1023 [CWE-460] Pokazivač bez vlasnika se dodaje u kontejner 'Modifikatori' metodom 'emplace_back'. Doći će do curenja memorije u slučaju izuzetka. llvm-stress.cpp 694
- V1023 [CWE-460] Pokazivač bez vlasnika se dodaje u 'Operands' kontejner metodom 'emplace_back'. Doći će do curenja memorije u slučaju izuzetka. GlobalISelEmitter.cpp 1911
- V1023 [CWE-460] Pokazivač bez vlasnika se dodaje u 'Stash' kontejner metodom 'emplace_back'. Doći će do curenja memorije u slučaju izuzetka. GlobalISelEmitter.cpp 2100
- V1023 [CWE-460] Pokazivač bez vlasnika se dodaje u 'Matchers' kontejner metodom 'emplace_back'. Doći će do curenja memorije u slučaju izuzetka. GlobalISelEmitter.cpp 2702
zaključak
Izdao sam ukupno 60 upozorenja i onda prestao. Postoje li drugi nedostaci koje PVS-Studio analizator otkriva u LLVM-u? Da imam. Međutim, kada sam ispisivao fragmente koda za članak, bilo je kasno navečer, odnosno čak noć, i odlučio sam da je vrijeme da to nazovem danom.
Nadam se da vam je bilo zanimljivo i da ćete htjeti isprobati PVS-Studio analizator.
Možete preuzeti analizator i dobiti ključ minolovca na
Ono što je najvažnije, redovno koristite statičku analizu. Jednokratne provjere, koje sprovodimo u cilju popularizacije metodologije statičke analize i PVS-Studio nisu normalan scenario.
Sretno u poboljšanju kvalitete i pouzdanosti vašeg koda!
Ako želite da podijelite ovaj članak sa publikom koja govori engleski, koristite link za prijevod: Andrey Karpov.
izvor: www.habr.com