
Prošlo je više od dvije godine od posljednjeg pregleda koda projekta LLVM uz pomoć našeg analizatora PVS-Studio. Provjerimo je li analizator PVS-Studio i dalje vodeći alat za otkrivanje grešaka i potencijalnih ranjivosti. U tu svrhu provjerit ćemo i pronaći nove greške u izdanju LLVM 8.0.0.
Članak koji treba biti napisan
Iskreno, nisam želio pisati ovaj članak. Nije zanimljivo pisati o projektu koji smo već više puta provjeravali (, , ). Radije bih pisao o nečemu novom, ali nemam izbora.
Svaki put kada izađe nova verzija LLVM ili se ažurira , u našoj pošti pojavljuju se pitanja sljedećeg tipa:
Pogledajte, nova verzija Clang Static Analyzer je naučila pronalaziti nove greške! Čini mi se da je relevantnost korištenja PVS-Studio smanjena. Clang pronalazi više grešaka nego prije i sustiže mogućnosti PVS-Studio. Što vi mislite o tome?
Na to uvijek želim odgovoriti nešto u stilu:
Mi također ne sjedimo skrštenih ruku! Značajno smo poboljšali mogućnosti analizatora PVS-Studio. Tako da se ne brinite, nastavljamo biti vodeći, kao i prije.
Nažalost, ovo je loš odgovor. U njemu nema dokaza. I upravo zato sada pišem ovaj članak. Dakle, projekt LLVM je ponovno pregledan i pronađene su razne greške. One koje su mi se činile zanimljivima, sada ću demonstrirati. Ove greške Clang Static Analyzer ne može pronaći (ili ih je iznimno nezgodno pronaći s njim). A mi možemo. Osim toga, sve sam te greške pronašao i zabilježio u jednom poslijepodnevu.
No, pisanje članka se odužilo na nekoliko tjedana. Nalog me nije mogao natjerati da sve to uredim u tekst.
Usput, ako vas zanima koje tehnologije se koriste u analizatoru PVS-Studio za otkrivanje grešaka i potencijalnih ranjivosti, predlažem da se upoznate s ovom .
Nove i stare dijagnostike
Kao što je već spomenuto, prije otprilike dvije godine projekt LLVM je ponovno pregledan, a pronađene greške su ispravljene. Sada će u ovom članku biti predstavljena nova serija grešaka. Zašto su pronađene nove greške? Razlog tome su 3 razloga:
- LLVM projekt se razvija, stari kod se mijenja i novi se pojavljuje. Naravno, u izmijenjenom i novom kodu pojavljuju se nove greške. To dobro pokazuje da se statička analiza mora provoditi redovito, a ne samo povremeno. Naši članci dobro prikazuju mogućnosti analitičara PVS-Studio, no to nema veze sa poboljšanjem kvalitete koda i smanjenjem troškova ispravljanja grešaka. Redovito koristite statički analitičar koda!
- Dorađujemo i unapređujemo već postojeće dijagnostike. Stoga analitičar može otkriti greške koje nisu bile primijećene tijekom prethodnih provjera.
- PVS-Studio je dobio nove dijagnostike koje nisu postojale prije 2 godine. Odlučio sam ih izdvojiti u posebnu sekciju kako bih jasno pokazao razvoj PVS-Studio.
Nedostaci koje su otkrile dijagnostike koje su postojale 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
....
}Upozorenje PVS-Studio: [CWE-570] Postoje identični podizrazi 'Name.startswith(«avx512.mask.permvar.»)' s lijeve i desne strane operatora '||'. AutoUpgrade.cpp 73
Dvostruko se provjerava da ime počinje s podstringom «avx512.mask.permvar.». U drugoj provjeri očito se htjelo napisati nešto drugo, ali su zaboravili ispraviti kopirani tekst.
Fragment N2: Typo
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’ s lijeve i desne strane operatora ‘|’. CIndex.cpp 7245
Zbog tipfeler dva puta se koristi ista imenovana konstanta CXNameRange_WantQualifier.
Fragment N3: Zbunjujući operatori
int PPCTTIImpl::getVectorInstrCost(unsigned Opcode, Type *Val, unsigned Index) {
....
if (ISD == ISD::EXTRACT_VECTOR_ELT && Index == ST->isLittleEndian() ? 1 : 0)
return 0;
....
}Upozorenje PVS-Studio: [CWE-783] Možda operator ‘?:’ radi na drugačiji način nego što se očekivalo. Operator ‘?:’ ima niži prioritet od operatora ‘==’. PPCTargetTransformInfo.cpp 404
Po mom mišljenju, to je vrlo lijepa greška. Da, znam da imam čudne ideje o ljepoti:).
Trenutno, prema , izraz se izračunava na sljedeći način:
(ISD == ISD::EXTRACT_VECTOR_ELT && (Index == ST->isLittleEndian())) ? 1 : 0Sa praktične tačke gledišta, takvo stanje nema smisla, jer se može pojednostaviti na:
(ISD == ISD::EXTRACT_VECTOR_ELT && Index == ST->isLittleEndian())Ovo je očigledna greška. Verovatno ste hteli uporediti 0/1 sa promenljivom Index. Da biste ispravili kod, morate dodati zagrade oko ternarnog operatora:
if (ISD == ISD::EXTRACT_VECTOR_ELT && Index == (ST->isLittleEndian() ? 1 : 0))Uostalom, ternarni operator je veoma opasan i može izazvati logičke greške. Budite veoma oprezni s njim i nemojte se ustručavati da stavite okrugle zagrade. O ovoj temi sam detaljnije govorio , u poglavlju 'Bojte se operatora ?: i obavezno ga stavite u okrugle zagrade'.
Fragment N4, N5: Null pointer
Init *TGParser::ParseValue(Record *CurRec, RecTy *ItemType, IDParseMode Mode) {
....
TypedInit *LHS = dyn_cast(Result);
....
LHS = dyn_cast(
UnOpInit::get(UnOpInit::CAST, LHS, StringRecTy::get())
->Fold(CurRec));
if (!LHS) {
Error(PasteLoc, Twine("can't cast '") + LHS->getAsString() +
"' to string");
return nullptr;
}
....
}Upozorenje PVS-Studio: [CWE-476] Dereferenciranje null pokazivača ‘LHS’ može se dogoditi. TGParser.cpp 2152
Ako pokazivač LHS bude null, trebalo bi da se izda upozorenje. Međutim, umesto toga doći će do dereferenciranja tog null pokazivača: LHS->getAsString().
Ovo je prilično tipična situacija u kojoj se greška skriva u obradi grešaka, jer ih niko ne testira. Statistički analizatori proveravaju sav dostupni kod, bez obzira koliko često se koristi. Ovo je veoma dobar primer kako statička analiza dopunjava druge metode testiranja i zaštite od grešaka.
Slična greška u obradi pokazivača RHS dogodila se u kodu malo niže: V522 [CWE-476] Dereferenciranje null pokazivača ‘RHS’ može se dogoditi. TGParser.cpp 2186
Fragment N6: Korišćenje pokazivača nakon premještanja
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);
}
....
}Upozorenje PVS-Studio: V522 [CWE-476] Dereferenciranje null pokazivača ‘ProgClone’ može se dogoditi. Miscompilation.cpp 601
Na početku pametni pokazivač ProgClone prestaje da poseduje objekat:
BD.setNewProgram(std::move(ProgClone));U suštini, sada ProgClone — to je nulti pokazivač. Stoga bi malo niže trebalo doći do dereferenciranja nultog pokazivača:
Function *NewF = ProgClone->getFunction(MisCompFunctions[i].first);No, zapravo se to neće dogoditi! Obratite pažnju da se petlja zapravo ne izvodi.
Na početku kontejnera MiscompiledFunctions se čisti:
MiscompiledFunctions.clear();Nakon toga, veličina ovog kontejnera koristi se u uvjetu petlje:
for (unsigned i = 0, e = MisCompFunctions.size(); i != e; ++i) {Lako je uočiti da se petlja ne pokreće. Mislim da je to također greška i da bi kod trebao biti napisan drugačije.
Čini se da smo naišli na onu poznatu parnost grešaka! Jedna greška prikriva drugu :).
Fragment N7: Korištenje pokazivača nakon premještanja
static Expected TestOptimizer(BugDriver &BD, std::unique_ptr Test,
std::unique_ptr Safe) {
outs() << " Optimiziranje funkcija koje se testiraju: ";
std::unique_ptr Optimized =
BD.runPassesOn(Test.get(), BD.getPassesToRun());
if (!Optimized) {
errs() << " Greška pri izvođenju ove sekvence prolaza"
<< " na ulaznom programu!n";
BD.setNewProgram(std::move(Test)); // <=
BD.EmitProgressBitcode(*Test, "pass-error", false); // <=
if (Error E = BD.debugOptimizerCrash())
return std::move(E);
return false;
}
....
}Upozorenje PVS-Studio: V522 [CWE-476] Dereferenciranje nultog pokazivača 'Test' može se dogoditi. Miscompilation.cpp 709
Opet ista situacija. Na početku se sadržaj objekta premješta, a zatim se koristi kao da se ništa nije dogodilo. Sve više se susrećem s ovom situacijom u kodu programa, nakon što je u C++ uvedena semantika premještanja. Zbog toga volim C++! Stalno se pojavljuju novi načini kako si samootkočiti nogu. Analizatoru PVS-Studio uvijek će biti posla :).
Fragment N8: Nulti pokazivač
void FunctionDumper::dump(const PDBSymbolTypeFunctionArg &Symbol) {
uint32_t TypeId = Symbol.getTypeId();
auto Type = Symbol.getSession().getSymbolById(TypeId);
if (Type)
Printer << "";
else
Type->dump(*this);
}Upozorenje PVS-Studio: V522 [CWE-476] Dereferenciranje nultog pokazivača 'Type' može se dogoditi. PrettyFunctionDumper.cpp 233
Osim obradnika grešaka, obično se ne testiraju ni funkcije za debagiranje podataka. Pred nama je baš takav slučaj. Funkcija očekuje korisnika koji će umjesto rješavanja vlastitih problema biti prisiljen ispraviti je.
Ispravno:
if (Type)
Type->dump(*this);
else
Printer << "";Fragment N9: Nulti pokazivač
void SearchableTableEmitter::collectTableEntries(
GenericTable &Table, const std::vector &Items) {
....
RecTy *Ty = resolveTypes(Field.RecType, TI->getType());
if (!Ty) // getAsString() + " vs. " + // getType()->getAsString());
....
}Upozorenje PVS-Studio: V522 [CWE-476] Može se dogoditi dereferenciranje nule ‘Ty’. SearchableTableEmitter.cpp 614
Mislim da je sve to jasno i ne treba dodatnih objašnjenja.
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;
}Upozorenje PVS-Studio: Varijabla 'Identifier->Type' se dodjeljuje sama sebi. FormatTokenLexer.cpp 249
Nema smisla dodjeljivati varijablu samoj sebi. Vjerojatno su htjeli napisati:
Identifier->Type = Question->Type;Fragment N11: Sumnjivi 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;
....
}Upozorenje PVS-Studio: [CWE-478] Razmotrite inspekciju ‘switch’ izjave. Moguće je da nedostaje prvi ‘case’ operator. SystemZAsmParser.cpp 652
Na početku se nalazi vrlo sumnjiv operator break. Jesu li ovdje zaboravili nešto napisati?
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");
....
}Upozorenje PVS-Studio: [CWE-476] Pokazivač ‘Callee’ je korišten prije nego što je provjeren na nullptr. Provjerite linije: 172, 174. AMDGPUInline.cpp 172
Pokazivač Callee u početku se dereferencira pri pozivu funkcije getTTI.
A onda se ispostavlja da treba provjeravati ovaj pokazivač na jednakost nullptr:
if (!Callee || Callee->isDeclaration())Ali već je prekasno…
Fragment N13 — N…: Provjera pokazivača nakon dereferenciranja
Situacija o kojoj se raspravlja u prethodnom fragmentu koda nije jedinstvena. Susreće se ovdje:
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()) { // <=
....
}Upozorenje PVS-Studio: V595 [CWE-476] Pokazivač ‘CalleeFn’ korišten je prije nego što je provjeren 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(New);
CXXRecordDecl *ThisContext =
dyn_cast_or_null(ND->getDeclContext()); // isCXXInstanceMember()); // <=
....
}Upozorenje PVS-Studio: V595 [CWE-476] Pokazivač ‘ND’ je korišten prije nego što je provjeren na nullptr. Provjerite redove: 532, 534. SemaTemplateInstantiateDecl.cpp 532
I ovdje:
- V595 [CWE-476] Pokazivač ‘U’ je korišten prije nego što je provjeren na nullptr. Provjerite redove: 404, 407. DWARFFormValue.cpp 404
- V595 [CWE-476] Pokazivač ‘ND’ je korišten prije nego što je provjeren na nullptr. Provjerite redove: 2149, 2151. SemaTemplateInstantiate.cpp 2149
A nakon toga mi je postalo dosadno proučavati upozorenja s brojem V595. Tako da ne znam ima li još sličnih grešaka, osim onih navedenih ovdje. Vjerovatno ih ima.
Fragment N17, N18: Sumnjivi pomak
static inline bool processLogicalImmediate(uint64_t Imm, unsigned RegSize,
uint64_t &Encoding) {
....
unsigned Size = RegSize;
....
uint64_t NImms = ~(Size-1) << 1;
....
}Upozorenje PVS-Studio: [CWE-190] Razmislite o provjeri izraza ‘~(Size — 1) << 1’. Pomak bitova 32-bitne vrijednosti s naknadnim proširenjem na 64-bitni tip. AArch64AddressingModes.h 260
Moguće je da to nije greška, i da kod radi kako je i zamišljeno. No, ovo je očito vrlo sumnjivo mjesto i potrebno ga je provjeriti.
Pretpostavimo da je varijabla Size jednaka 16, i tada je autor koda planirao dobiti vrijednost u varijabli NImms vrijednost:
1111111111111111111111111111111111111111111111111111111111100000
Međutim, zapravo će ispasti vrijednost:
0000000000000000000000000000000011111111111111111111111111100000
Stvar je u tome da se sva izračunavanja vrše korištenjem 32-bitnog tipa unsigned. I tek poslije toga, ovaj 32-bitni bezsudni tip će biti neizravno proširen na uint64_t. Pri tome će viši bitovi biti nulti.
Situaciju možete popraviti ovako:
uint64_t NImms = ~static_cast(Size-1) << 1;Slična situacija: V629 [CWE-190] Razmislite o provjeri izraza ‘Immr << 6’. Pomak bitova 32-bitne vrijednosti s naknadnim proširenjem na 64-bitni tip. AArch64AddressingModes.h 269
Fragment N19: Propušten je ključna riječ else?
void AMDGPUAsmParser::cvtDPP(MCInst &Inst, const OperandVector &Operands) {
....
if (Op.isReg() && Op.Reg.RegNo == AMDGPU::VCC) {
// VOP2b (v_add_u32, v_sub_u32 ...) dpp koristi "vcc" token.
// Preskoči to.
continue;
} if (isRegOrImmWithInputMods(Desc, Inst.getNumOperands())) { // <=
Op.addRegWithFPInputModsOperands(Inst, 2);
} else if (Op.isDPPCtrl()) {
Op.addImmOperands(Inst, 1);
} else if (Op.isImm()) {
// Obradi opcionalne argumente
OptionalIdx[Op.getImmTy()] = I;
} else {
llvm_unreachable("Nevažeća vrsta operanda");
}
....
}Upozorenje PVS-Studio: [CWE-670] Razmislite o provjeri logike aplikacije. Moguće je da nedostaje ključna riječ ‘else’. AMDGPUAsmParser.cpp 5655
Ovdje nema greške. Budući da then-blok prvog if se završava na continue, nije važno, postoji li ključna riječ else ili ne. U svakom slučaju, kod će raditi jednako. Međutim, propušteni else čini kod manje jasnim i opasnim. Ako u budućnosti continue nestane, kod će početi raditi potpuno drugačije. Po mom mišljenju, bolje je dodati else.
Fragment N20: Četiri slične tipografske greške
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;
}Upozorenja PVS-Studio:
- V655 [CWE-480] Stringovi su spojeni, ali se ne koriste. Razmislite o provjeri izraza ‘Result + Name.str()’. Symbol.cpp 32
- V655 [CWE-480] Stringovi su spojeni, ali se ne koriste. Razmislite o provjeri izraza ‘Result + «(ObjC Class) » + Name.str()’. Symbol.cpp 35
- V655 [CWE-480] Stringovi su spojeni, ali se ne koriste. Razmislite o provjeri izraza ‘Result + «(ObjC Class EH) » + Name.str()’. Symbol.cpp 38
- V655 [CWE-480] Stringovi su spojeni, ali se ne koriste. Razmislite o provjeri izraza ‘Result + «(ObjC IVar) » + Name.str()’. Symbol.cpp 41
Umjesto operatora += slučajno se koristi operator +. Kao rezultat toga, nastaju konstrukcije bez smisla.
Fragment N21: Nepredvidivo ponašanje
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 ne može biti prazno");
for (auto &Op : Ops) {
assert(!Op.empty() && "Prazan operator");
if (FeaturesMap.find(Op) == FeaturesMap.end())
FeaturesMap[Op] = FeaturesMap.size();
}
}
}Pokušajte sami pronaći opasan kod. A ovo je slika za skretanje pažnje, da ne pogledate odmah odgovor:

Upozorenje PVS-Studio: [CWE-758] Opasna konstrukcija se koristi: ‘FeaturesMap[Op] = FeaturesMap.size()’, gdje je ‘FeaturesMap’ klase ‘map’. To može dovesti do nepredvidivog ponašanja. RISCVCompressInstEmitter.cpp 490
Problematična linija:
FeaturesMap[Op] = FeaturesMap.size();Ako element Op nije pronađen, tada se stvara novi element u mapi i tamo se zapisuje broj elemenata u toj mapi. Samo nije jasno hoće li funkcija size biti pozvana prije ili nakon dodavanja novog elementa.
Fragment N22-N24: Ponovna dodjeljivanja
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;
}
....
}Upozorenje PVS-Studio: [CWE-563] Varijabla ‘NType’ se dva puta zadaje sukcesivno. Možda je ovo greška. Provjerite redove: 1663, 1664. MachOObjectFile.cpp 1664
Mislim da ovdje nije stvarna greška. Samo je suvišna ponavljajuća dodjeljivanje. Ali opet, to je lapsus.
Slično:
- V519 [CWE-563] Varijabla ‘B.NDesc’ se dva puta zadaje sukcesivno. Možda je ovo greška. Provjerite redove: 1488, 1489. llvm-nm.cpp 1489
- V519 [CWE-563] Varijabla se dva puta zadaje sukcesivno. Možda je ovo greška. Provjerite redove: 59, 61. coff2yaml.cpp 61
Fragment N25-N27: Još ponavljajućih dodjeljivanja
Sada ćemo razmotriti malo drugačiju varijantu ponavljajućeg dodjeljivanja.
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 upozorenje: V519 [CWE-563] Varijabla ‘Alignment’ se dva puta zadaje sukcesivno. Možda je ovo greška. Provjerite redove: 1158, 1160. LoadStoreVectorizer.cpp 1160
To je vrlo čudan kod koji očigledno sadrži logičku grešku. U početku, varijabli Alignment se dodjeljuje vrijednost ovisno o uvjetu. A onda ponovo dolazi do dodjeljivanja, ali sada bez ikakve provjere.
Slične situacije se mogu vidjeti ovdje:
- V519 [CWE-563] Varijabla ‘Effects’ se dva puta zadaje sukcesivno. Možda je ovo greška. Provjerite redove: 152, 165. WebAssemblyRegStackify.cpp 165
- V519 [CWE-563] Varijabla ‘ExpectNoDerefChunk’ se dva puta zadaje sukcesivno. Možda je ovo greška. Provjerite redove: 4970, 4973. SemaType.cpp 4973
Fragment N28: Uvijek istinita uvjet
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;
}
....
}Upozorenje PVS-Studio: [CWE-571] Izraz ‘nextByte != 0x90’ je uvijek istinit. X86DisassemblerDecoder.cpp 379
Provjera nije smislen. Varijabla nextByte uvijek nije jednaka vrijednosti 0x90, što proizađe iz prethodne provjere. Ovo je neka logička greška.
Fragment N29 — N…: Uvijek istiniti/lažni uvjeti
Analizator izbacuje mnogo upozorenja o tome da je cijeli uvjet () ili njegov dio () uvijek je istinito ili lažno. Često to nisu stvarne greške, već samo neuredan kod, rezultat provale makronaredbi i tome slično. Ipak, ima smisla razmotriti sve te upozorenja, jer se s vremena na vrijeme susreću stvarne logičke greške. Na primjer, sumnjiv je sljedeći dio koda:
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;
....
}Upozorenje PVS-Studio: [CWE-570] Dio uvjetnog izraza je uvijek lažno: RegNo == 0xe. ARMDisassembler.cpp 939
Konstanta 0xE je vrijednost 14 u decimalnom sustavu. Provjera RegNo == 0xe nema smisla, jer ako RegNo > 13, funkcija će završiti svoje izvršavanje.
Bilo je mnogo drugih upozorenja s identifikatorima V547 i V560, ali, kao i u slučaju s , proučavanje tih upozorenja mi nije bilo zanimljivo. Tako je bilo jasno da imam dovoljno materijala za pisanje članka :). Stoga nije poznato koliko se sveukupno može otkriti grešaka ove vrste u LLVM pomoću PVS-Studio.
Ovdje je primjer zašto je proučavanje ovih aktivacija dosadno. Analizator je potpuno u pravu, izdajući upozorenje na sljedeći kod. Ali to 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žno. UnwrappedLineParser.cpp 1635
Fragment N30: Sumnjivi 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();
}
....
}Upozorenje PVS-Studio: [CWE-670] Bezuvjetni ‘return’ unutar petlje. R600OptimizeVectorRegisters.cpp 63
To je ili greška, ili specifična tehnika koja ima za cilj nešto objasniti programerima koji čitaju kod. Takva konstrukcija meni ne objašnjava ništa i izgleda vrlo sumnjivo. Bolje je ne pisati tako : ).
Umorni? Onda je vrijeme za čaj ili kavu.

Defekti otkriveni novim dijagnostikama
Mislim da je 30 aktivacija starih dijagnostika dovoljno. Pogledajmo sada što zanimljivo možemo pronaći novim dijagnostikama koje su se pojavile u analizatoru nakon provjere. Tijekom tog vremena u C++ analizatoru dodano je 66 dijagnostika opće namjene.
Fragment N31: Nedostižan 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();
}Upozorenje PVS-Studio: [CWE-561] Nepostojani kod otkriven. Moguće je da je prisutna greška. ExecutionUtils.cpp 146
Kao što vidite, oba ogranka operatora if završavaju pozivom operatora return. Shodno tome, kontejner CtorDtorsByPriority nikada neće biti očišćen.
Fragment N32: Nepostojani 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(), "neočekivana vrsta sažetka");
}
Lex.setIgnoreColonInIdentifiers(false); \/\/ <=
return false;
}Upozorenje PVS-Studio: V779 [CWE-561] Nepostojani kod otkriven. Moguće je da je prisutna greška. LLParser.cpp 835
Zanimljiva situacija. Pogledajmo prvo ovo mjesto:
return ParseTypeIdEntry(SummaryID);
break;Na prvi pogled čini se da nema greške ovdje. Izgleda da je operator break ovdje suvišan i može se jednostavno ukloniti. Međutim, nije sve tako jednostavno.
Analizator izbacuje upozorenje na redove:
Lex.setIgnoreColonInIdentifiers(false);
return false;I zaista, ovaj kod je nedostižan. Svi slučajevi u switch završavaju pozivom operatora return. I sada besmisleni osamljeni break ne izgleda tako bezopasno! Možda jedan od ogranka treba završavati na break, a ne na return?
Fragment N33: Nasumično nula viših 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);
....
}Upozorenje PVS-Studio: Veličina bitne maske je manja od veličine prvog operanda. To će uzrokovati gubitak viših bitova. RuntimeDyld.cpp 815
Obratite pažnju da funkcija getStubAlignment vraća tip unsigned. Izračunajmo vrijednost izraza, pod pretpostavkom da funkcija vrati vrijednost 8:
~(getStubAlignment() - 1)
~(8u-1)
0xFFFFFFF8u
Sada obratite pažnju da varijabla DataSize ima 64-bitni bezznakodni tip. Dakle, kada se izvrši operacija DataSize & 0xFFFFFFF8u, svi trideset dva viša bita će biti nula. Verovatno to nije ono što je programer želeo. Sumnjam da je želeo da izračuna: DataSize & 0xFFFFFFFFFFFFFFF8u.
Da biste ispravili grešku, treba napisati ovako:
DataSize &= ~(static_cast<uint64_t>(getStubAlignment()) - 1);Ili ovako:
DataSize &= ~(getStubAlignment() - 1ULL);Fragment N34: Neuspješno eksplicitno pretvaranje tipa
template
void scaleShuffleMask(int Scale, ArrayRef Mask,
SmallVectorImpl &ScaledMask) {
assert(0 < Scale && "Neočekivani faktor skaliranja");
int NumElts = Mask.size();
ScaledMask.assign(static_cast(NumElts * Scale), -1);
....
}Upozorenje PVS-Studio: [CWE-190] Moguće preljevanje. Razmotrite pretvaranje operanada operatora ‘NumElts * Scale’ u tip ‘size_t’, a ne rezultat. X86ISelLowering.h 1577
Eksplicitno pretvaranje tipa se koristi kako bi se izbjeglo preljevanje pri množenju varijabli tipa int. Međutim, ovdje eksplicitno pretvaranje tipa ne štiti od preljevanja. Na početku će se varijable pomnožiti, a tek nakon toga će 32-bitni rezultat množenja biti proširen na tip .
Fragment N35: Neuspješni 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;
}
....
}[CWE-682] Pronađena su dva slična fragmentsa koda. Možda je to tipfeler i varijabla ‘Op1’ bi trebala biti korištena umjesto ‘Op0’. InstCombineCompares.cpp 5507
Ova nova zanimljiva dijagnostika otkriva situacije kada je fragment koda kopiran, a neka imena su počela mijenjati, ali u jednom dijelu nisu ispravljena.
Obratite pažnju da je u drugom bloku promijenjeno Op0 na Op1. Ali u jednom dijelu nije ispravljeno. Vjerojatno je trebalo biti napisano ovako:
if (!match(Op1, m_PosZeroFP()) && isKnownNeverNaN(Op1, &TLI)) {
I.setOperand(1, ConstantFP::getNullValue(Op1->getType()));
return &I;
}Fragment N36: Nejasnoće u varijablama
struct Status {
unsigned Mask;
unsigned Mode;
Status() : Mask(0), Mode(0){};
Status(unsigned Mask, unsigned Mode) : Mask(Mask), Mode(Mode) {
Mode &= Mask;
};
....
};Upozorenje PVS-Studio: [CWE-563] Varijabla ‘Mode’ se dodjeljuje, ali se ne koristi do kraja funkcije. SIModeRegister.cpp 48
Vrlo je opasno davati argumentima funkcija ista imena kao i članovima klase. Lako se može zbuniti. Ovo je jedan takav slučaj. Ova izjava nema smisla:
Mode &= Mask;Mijenja se argument funkcije. I to je to. Ovaj argument više ne koristi. Vjerojatno je trebalo biti napisano ovako:
Status(unsigned Mask, unsigned Mode) : Mask(Mask), Mode(Mode) {
this->Mode &= Mask;
};Fragment N37: Nejasnoće u varijablama
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;
}Upozorenje PVS-Studio: V1001 [CWE-563] Varijabla ‘Size’ je dodijeljena, ali nije korištena do kraja funkcije. Object.cpp 424
Situacija je slična prethodnoj. Trebalo bi napisati:
this->Size += this->EntrySize;Fragment N38-N47: Pokazivač nije provjeren
Ranije smo razmatrali primjere aktivacije dijagnostike . Njena suština je da se pokazivač prvo dereferencira, a zatim provjerava. Mladi dijagnostika je suprotno njoj po značenju, ali također otkriva mnogo grešaka. Otkazuje situacije kada je pokazivač najprije provjeren, a zatim je to zaboravljeno. Razmotrićemo takve slučajeve, pronađene unutar LLVM.
int getGEPCost(Type *PointeeType, const Value *Ptr,
ArrayRef Operands) {
....
if (Ptr != nullptr) { // <=
assert(....);
BaseGV = dyn_cast(Ptr->stripPointerCasts());
}
bool HasBaseReg = (BaseGV == nullptr);
auto PtrSizeBits = DL.getPointerTypeSizeInBits(Ptr->getType()); // <=
....
}Upozorenje PVS-Studio: V1004 [CWE-476] Pokazivač ‘Ptr’ je korišten nesigurno nakon što je provjeren protiv nullptr. Provjerite linije: 729, 738. TargetTransformInfoImpl.h 738
Varijabla Ptr može biti jednaka nullptr, što potvrđuje provjera:
if (Ptr != nullptr)Međutim, ispod taj pokazivač se dereferencira već bez prethodne provjere:
auto PtrSizeBits = DL.getPointerTypeSizeInBits(Ptr->getType());Razmotrite drugi sličan slučaj.
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(); // <=
....
}Upozorenje PVS-Studio: V1004 [CWE-476] Pokazivač ‘FD’ je korišten nesigurno nakon što je provjeren protiv nullptr. Provjerite linije: 3228, 3231. CGDebugInfo.cpp 3231
Obratite pažnju na pokazivač FD. Siguran sam da je problem dobro vidljiv i da posebna objašnjenja nisu potrebna.
I još:
static void computePolynomialFromPointer(Value &Ptr, Polynomial &Result,
Value *&BasePtr,
const DataLayout &DL) {
PointerType *PtrTy = dyn_cast(Ptr.getType());
if (!PtrTy) { // getPointerAddressSpace()); // <=
....
}PVS-Studio upozorenje: V1004 [CWE-476] Pokazivač 'PtrTy' korišten je nesigurno nakon što je verifikovan protiv nullptr. Provjerite linije: 960, 965. InterleavedLoadCombinePass.cpp 965
Kako se zaštititi od takvih grešaka? Budite pažljiviji tijekom Code-Review-a i koristite statički analizator PVS-Studio za redovno provjeravanje koda.
Nema smisla davati druge isječke koda s greškama ovog tipa. Ostavit ću u članku samo popis upozorenja:
- V1004 [CWE-476] Pokazivač 'Expr' korišten je nesigurno nakon što je verifikovan protiv nullptr. Provjerite linije: 1049, 1078. DebugInfoMetadata.cpp 1078
- V1004 [CWE-476] Pokazivač 'PI' korišten je nesigurno nakon što je verifikovan protiv nullptr. Provjerite linije: 733, 753. LegacyPassManager.cpp 753
- V1004 [CWE-476] Pokazivač 'StatepointCall' korišten je nesigurno nakon što je verifikovan protiv nullptr. Provjerite linije: 4371, 4379. Verifier.cpp 4379
- V1004 [CWE-476] Pokazivač 'RV' korišten je nesigurno nakon što je verifikovan protiv nullptr. Provjerite linije: 2263, 2268. TGParser.cpp 2268
- V1004 [CWE-476] Pokazivač 'CalleeFn' korišten je nesigurno nakon što je verifikovan protiv nullptr. Provjerite linije: 1081, 1096. SimplifyLibCalls.cpp 1096
- V1004 [CWE-476] Pokazivač 'TC' korišten je nesigurno nakon što je verifikovan protiv nullptr. Provjerite linije: 1819, 1824. Driver.cpp 1824
Isječak N48-N60: Nije kritično, ali postoji greška (moguća curenja memorije)
std::unique_ptr createISelMutator() {
....
std::vector<std::unique_ptr> Strategies;
Strategies.emplace_back(
new InjectorIRStrategy(InjectorIRStrategy::getDefaultOps()));
....
}Upozorenje PVS-Studio: [CWE-460] Pokazivač bez vlasnika dodan je u kontejner 'Strategies' metodom 'emplace_back'. Doći će do curenja memorije u slučaju izuzetka. llvm-isel-fuzzer.cpp 58
Za dodavanje elementa na kraj kontejnera tipa std::vector<std::unique_ptr> ne može se jednostavno napisati xxx.push_back(new X), jer nema implicitne konverzije iz X* u std::unique_ptr.
Uobičajeno rješenje je napisati xxx.emplace_back(new X), jer se kompilira: metoda emplace_back konstruira element izravno iz argumenata i zato može koristiti eksplicitne konstruktore.
To nije sigurno. Ako je vektor pun, doći će do ponovne alokacije memorije. Operacija ponovne alokacije memorije može završiti neuspjehom, što može dovesti do izuzetka std::bad_alloc. U tom slučaju pokazivač će biti izgubljen, i stvoreni objekt nikada neće biti uklonjen.
Sigurno rješenje je kreirati unique_ptr, koji će posjedovati pokazivač dok vektor ne pokuša ponovo alocirati memoriju:
xxx.push_back(std::unique_ptr(new X))Od C++14 možete koristiti 'std::make_unique':
xxx.push_back(std::make_unique())Ova vrsta pogreške nije kritična za LLVM. Ako ne uspije alocirati memoriju, rad prevoditelja će jednostavno biti prekinut. Međutim, za aplikacije s dugim , koje ne mogu lako završiti ako nije uspjela alokacija memorije, to može predstavljati stvarni problem.
Dakle, iako ovaj kod ne predstavlja praktičnu opasnost za LLVM, smatrao sam korisnim ispričati o ovom uzorku pogrešaka i kako je analizator PVS-Studio naučio prepoznavati ga.
Druga upozorenja ovog tipa:
- V1023 [CWE-460] Pokazivač bez vlasnika dodan je u 'Passes' kontejner korištenjem metode 'emplace_back'. Doći će do curenja memorije u slučaju iznimke. PassManager.h 546
- V1023 [CWE-460] Pokazivač bez vlasnika dodan je u 'AAs' kontejner korištenjem metode 'emplace_back'. Doći će do curenja memorije u slučaju iznimke. AliasAnalysis.h 324
- V1023 [CWE-460] Pokazivač bez vlasnika dodan je u 'Entries' kontejner korištenjem metode 'emplace_back'. Doći će do curenja memorije u slučaju iznimke. DWARFDebugFrame.cpp 519
- V1023 [CWE-460] Pokazivač bez vlasnika dodan je u 'AllEdges' kontejner korištenjem metode 'emplace_back'. Doći će do curenja memorije u slučaju iznimke. CFGMST.h 268
- V1023 [CWE-460] Pokazivač bez vlasnika dodan je u 'VMaps' kontejner korištenjem metode 'emplace_back'. Doći će do curenja memorije u slučaju iznimke. SimpleLoopUnswitch.cpp 2012
- V1023 [CWE-460] Pokazivač bez vlasnika dodan je u 'Records' kontejner korištenjem metode 'emplace_back'. Doći će do curenja memorije u slučaju iznimke. FDRLogBuilder.h 30
- V1023 [CWE-460] Pokazivač bez vlasnika dodan je u 'PendingSubmodules' kontejner korištenjem metode 'emplace_back'. Doći će do curenja memorije u slučaju iznimke. ModuleMap.cpp 810
- V1023 [CWE-460] Pokazivač bez vlasnika dodan je u 'Objects' kontejner korištenjem metode 'emplace_back'. Doći će do curenja memorije u slučaju iznimke. DebugMap.cpp 88
- V1023 [CWE-460] Pokazivač bez vlasnika dodan je u 'Strategies' kontejner korištenjem metode 'emplace_back'. Doći će do curenja memorije u slučaju iznimke. llvm-isel-fuzzer.cpp 60
- V1023 [CWE-460] Pokazivač bez vlasnika dodan je u 'Modifiers' kontejner korištenjem metode 'emplace_back'. Doći će do curenja memorije u slučaju iznimke. llvm-stress.cpp 685
- V1023 [CWE-460] Pokazivač bez vlasnika dodan je u 'Modifiers' kontejner korištenjem metode 'emplace_back'. Doći će do curenja memorije u slučaju iznimke. llvm-stress.cpp 686
- V1023 [CWE-460] Pokazivač bez vlasnika dodan je u 'Modifiers' kontejner korištenjem metode 'emplace_back'. Doći će do curenja memorije u slučaju iznimke. llvm-stress.cpp 688
- V1023 [CWE-460] Pokazivač bez vlasnika dodan je u 'Modifiers' kontejner korištenjem metode 'emplace_back'. Doći će do curenja memorije u slučaju iznimke. llvm-stress.cpp 689
- V1023 [CWE-460] Pokazivač bez vlasnika dodan je u 'Modifiers' kontejner korištenjem metode 'emplace_back'. Doći će do curenja memorije u slučaju iznimke. llvm-stress.cpp 690
- V1023 [CWE-460] Pokazivač bez vlasnika dodan je u 'Modifiers' kontejner korištenjem metode 'emplace_back'. Doći će do curenja memorije u slučaju iznimke. llvm-stress.cpp 691
- V1023 [CWE-460] Pokazivač bez vlasnika je dodan u ‘Modifiers’ spremnik metodom ’emplace_back’. Doći će do curenja memorije u slučaju iznimke. llvm-stress.cpp 692
- V1023 [CWE-460] Pokazivač bez vlasnika je dodan u ‘Modifiers’ spremnik metodom ’emplace_back’. Doći će do curenja memorije u slučaju iznimke. llvm-stress.cpp 693
- V1023 [CWE-460] Pokazivač bez vlasnika je dodan u ‘Modifiers’ spremnik metodom ’emplace_back’. Doći će do curenja memorije u slučaju iznimke. llvm-stress.cpp 694
- V1023 [CWE-460] Pokazivač bez vlasnika je dodan u ‘Operands’ spremnik metodom ’emplace_back’. Doći će do curenja memorije u slučaju iznimke. GlobalISelEmitter.cpp 1911
- V1023 [CWE-460] Pokazivač bez vlasnika je dodan u ‘Stash’ spremnik metodom ’emplace_back’. Doći će do curenja memorije u slučaju iznimke. GlobalISelEmitter.cpp 2100
- V1023 [CWE-460] Pokazivač bez vlasnika je dodan u ‘Matchers’ spremnik metodom ’emplace_back’. Doći će do curenja memorije u slučaju iznimke. GlobalISelEmitter.cpp 2702
Zaključak
Ukupno sam izdao 60 upozorenja, nakon čega sam se zaustavio. Ima li drugih grešaka koje PVS-Studio analizer otkriva u LLVM-u? Da, ima. Međutim, kada sam izdavao dijelove koda za članak, bila je kasna večer, ili bolje rečeno, čak i noć, i odlučio sam da je vrijeme za završiti.
Nadam se da vam je bilo zanimljivo i da ćete probati PVS-Studio analizer.
Možete preuzeti analizer i dobiti probni ključ na .
Najvažnije, redovito koristite statičku analizu. Jednokratne provjere, koje mi provodimo s ciljem popularizacije metodologije statičke analize i PVS-Studio, nisu normalan scenarij.
Sretno u poboljšanju kvalitete i pouzdanosti koda!
Ako želite podijeliti ovaj članak s anglofonom publikom, molim vas da koristite vezu na prijevod: Andrey Karpov. .
Izvor: habr.com
