Sono passati più di due anni dall'ultima revisione del codice del progetto LLVM utilizzando il nostro analizzatore PVS-Studio. Assicuriamoci che l'analizzatore PVS-Studio sia ancora uno strumento leader nell'individuazione di errori e vulnerabilità potenziali. Per questo motivo, controlleremo e cercheremo nuovi errori nella versione LLVM 8.0.0.
Articolo che deve essere scritto
Se devo essere sincero, non avevo voglia di scrivere questo articolo. Non è interessante scrivere di un progetto che abbiamo già verificato più volte (, , ). È meglio scrivere di qualcosa di nuovo, ma non ho scelta.
Ogni volta che esce una nuova versione di LLVM o viene aggiornata , riceviamo via email domande di questo tipo:
Guarda, la nuova versione di Clang Static Analyzer ha imparato a trovare nuovi errori! Mi sembra che la rilevanza di utilizzare PVS-Studio stia diminuendo. Clang trova più errori di prima e sta raggiungendo le capacità di PVS-Studio. Cosa ne pensi?
A questo punto, mi viene sempre voglia di rispondere qualcosa del tipo:
Anche noi non siamo stati fermi! Abbiamo migliorato notevolmente le capacità dell'analizzatore PVS-Studio. Quindi non preoccuparti, continuiamo a mantenere la leadership come prima.
Sfortunatamente, questa è una risposta deludente. Non contiene prove. Ed è per questo che sto scrivendo questo articolo. Così, il progetto LLVM è stato nuovamente esaminato e sono stati trovati vari errori. Quelli che mi sono sembrati interessanti li dimostrerò ora. Questi errori non possono essere trovati da Clang Static Analyzer (o è estremamente scomodo farlo con lui). Ma noi possiamo. Anzi, ho trovato e annotato tutti questi errori in una sola serata.
Scrivere l'articolo, invece, ha richiesto alcune settimane. Non riuscivo a costringermi a mettere tutto questo in forma di testo :).
A proposito, se sei interessato a quali tecnologie vengono utilizzate nell'analizzatore PVS-Studio per individuare errori e vulnerabilità potenziali, ti invito a dare un'occhiata a questo .
Diagnostiche nuove e vecchie
Come già accennato, circa due anni fa il progetto LLVM è stato nuovamente controllato e gli errori trovati sono stati corretti. Ora in questo articolo verrà presentato un nuovo insieme di errori. Perché sono stati trovati nuovi errori? Ci sono 3 motivi:
- Il progetto LLVM si evolve, il codice vecchio viene modificato e ne appare di nuovo. È evidente che nel codice modificato e scritto ci sono nuovi errori. Questo dimostra bene che l'analisi statica deve essere applicata regolarmente e non di tanto in tanto. I nostri articoli mostrano bene le potenzialità dell'analizzatore PVS-Studio, ma ciò non ha nulla a che fare con il miglioramento della qualità del codice e la riduzione dei costi di correzione degli errori. Utilizzate l'analizzatore statico di codice regolarmente!
- Stiamo perfezionando e migliorando le diagnosi già esistenti. Pertanto, l'analizzatore può identificare errori che non erano stati notati durante i controlli precedenti.
- In PVS-Studio sono state aggiunte nuove diagnosi che non esistevano due anni fa. Ho deciso di evidenziarle in una sezione separata per mostrare concretamente lo sviluppo di PVS-Studio.
Difetti evidenziati dalle diagnosi esistenti due anni fa
Frammento N1: Copia-Incolla
static bool ShouldUpgradeX86Intrinsic(Function *F, StringRef Name) {
if (Name == "addcarryx.u32" || // Aggiunto in 8.0
....
Name == "avx512.mask.cvtps2pd.128" || // Aggiunto in 7.0
Name == "avx512.mask.cvtps2pd.256" || // Aggiunto in 7.0
Name == "avx512.cvtusi2sd" || // Aggiunto in 7.0
Name.startswith("avx512.mask.permvar.") || // Aggiunto in 7.0 // <=
Name.startswith("avx512.mask.permvar.") || // Aggiunto in 7.0 // <=
Name == "sse2.pmulu.dq" || // Aggiunto in 7.0
Name == "sse41.pmuldq" || // Aggiunto in 7.0
Name == "avx2.pmulu.dq" || // Aggiunto in 7.0
....
}Avviso PVS-Studio: [CWE-570] Ci sono sottoespressioni identiche ‘Name.startswith(«avx512.mask.permvar.»)’ a sinistra e a destra dell'operatore ‘||’. AutoUpgrade.cpp 73
Si verifica due volte che il nome inizia con la sottostringa «avx512.mask.permvar.». Nella seconda verifica si voleva chiaramente scrivere qualcos'altro, ma si è dimenticati di correggere il testo copiato.
Frammento N2: Errore di battitura
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;
....
}Avviso PVS-Studio: V501 Ci sono sottoespressioni identiche ‘CXNameRange_WantQualifier’ a sinistra e a destra dell'operatore ‘|’. CIndex.cpp 7245
A causa di un errore di battitura viene utilizzata due volte la stessa costante denominata. CXNameRange_WantQualifier.
Frammento N3: Confusione con le priorità degli operatori
int PPCTTIImpl::getVectorInstrCost(unsigned Opcode, Type *Val, unsigned Index) {
....
if (ISD == ISD::EXTRACT_VECTOR_ELT && Index == ST->isLittleEndian() ? 1 : 0)
return 0;
....
}Avviso PVS-Studio: [CWE-783] Forse l'operatore ‘?:’ funziona in modo diverso da quanto ci si aspettava. L'operatore ‘?:’ ha una priorità inferiore rispetto all'operatore ‘==’. PPCTargetTransformInfo.cpp 404
A mio avviso, questo è un errore molto bello. Sì, so che ho idee strane sulla bellezza :).
Adesso, secondo , l'espressione viene calcolata come segue:
(ISD == ISD::EXTRACT_VECTOR_ELT && (Index == ST->isLittleEndian())) ? 1 : 0Da un punto di vista pratico, questa condizione non ha senso, poiché può essere semplificata a:
(ISD == ISD::EXTRACT_VECTOR_ELT && Index == ST->isLittleEndian())Si tratta di un errore evidente. Probabilmente, 0/1 si voleva confrontare con la variabile Index. Per correggere il codice occorre aggiungere parentesi attorno all'operatore ternario:
if (ISD == ISD::EXTRACT_VECTOR_ELT && Index == (ST->isLittleEndian() ? 1 : 0))A proposito, l'operatore ternario è molto pericoloso e provoca errori logici. Fate attenzione ad usarlo e non siate avari nel mettere le parentesi. Ho trattato questo argomento , nel capitolo «Fate attenzione all'operatore ?: e racchiudetelo tra parentesi».
Frammento N4, N5: Puntatore nullo
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("impossibile convertire '") + LHS->getAsString() +
"' in stringa");
return nullptr;
}
....
}Avviso PVS-Studio: [CWE-476] La dereferenziazione del puntatore nullo ‘LHS’ potrebbe avvenire. TGParser.cpp 2152
Se il puntatore LHS risulta nullo, deve essere emesso un avviso. Tuttavia, invece di ciò, si verificherà la dereferenziazione di questo puntatore nullo: LHS->getAsString().
Questa è una situazione piuttosto comune, in cui l'errore si nasconde nel gestore degli errori, poiché nessuno lo testa. Gli analizzatori statici esaminano tutto il codice raggiungibile, indipendentemente dalla frequenza con cui viene utilizzato. Questo è un ottimo esempio di come l'analisi statica integri altre metodologie di test e protezione dagli errori.
Un errore analogo nella gestione del puntatore RHS è presente nel codice poco sotto: V522 [CWE-476] La dereferenziazione del puntatore nullo ‘RHS’ potrebbe avvenire. TGParser.cpp 2186
Frammento N6: Utilizzo del puntatore dopo lo spostamento
static Expected
ExtractBlocks(....)
{
....
std::unique_ptr ProgClone = CloneModule(BD.getProgram(), VMap);
....
BD.setNewProgram(std::move(ProgClone)); // getFunction(MisCompFunctions[i].first); // <=
assert(NewF && "Funzione non trovata??");
MiscompiledFunctions.push_back(NewF);
}
....
}Avviso PVS-Studio: V522 [CWE-476] La dereferenziazione del puntatore nullo ‘ProgClone’ potrebbe avvenire. Miscompilation.cpp 601
All'inizio, il puntatore intelligente ProgClone smette di possedere l'oggetto:
BD.setNewProgram(std::move(ProgClone));Di fatto, ora ProgClone — è un puntatore nullo. Pertanto, un po' più in basso dovrebbe avvenire il dereferencing del puntatore nullo:
Function *NewF = ProgClone->getFunction(MisCompFunctions[i].first);Ma in realtà, ciò non avverrà! Si noti che il ciclo in realtà non viene eseguito.
All'inizio del contenitore MiscompiledFunctions viene svuotato:
MiscompiledFunctions.clear();Successivamente, la dimensione di questo contenitore è utilizzata nella condizione del ciclo:
for (unsigned i = 0, e = MisCompFunctions.size(); i != e; ++i) {È facile vedere che il ciclo non parte. Penso che sia anche questo un errore e il codice dovrebbe essere scritto in modo diverso.
Sembra che abbiamo incontrato proprio quella famosa parità di errori! Un errore ne maschera un altro :).
Frammento N7: Utilizzo di un puntatore dopo la movimentazione
static Expected TestOptimizer(BugDriver &BD, std::unique_ptr Test,
std::unique_ptr Safe) {
outs() << " Ottimizzando le funzioni in fase di test: ";
std::unique_ptr Optimized =
BD.runPassesOn(Test.get(), BD.getPassesToRun());
if (!Optimized) {
errs() << " Errore durante l'esecuzione di questa sequenza di passaggi"
<< " sul programma di input!n";
BD.setNewProgram(std::move(Test)); // <=
BD.EmitProgressBitcode(*Test, "pass-error", false); // <=
if (Error E = BD.debugOptimizerCrash())
return std::move(E);
return false;
}
....
}Avviso PVS-Studio: V522 [CWE-476] Potrebbe verificarsi dereferenziazione del puntatore nullo 'Test'. Miscompilation.cpp 709
Ancora la stessa situazione. All'inizio il contenuto dell'oggetto viene spostato e poi viene utilizzato come se nulla fosse. Incontro sempre più spesso questa situazione nel codice dei programmi, dopo che in C++ è stata introdotta la semantica di movimentazione. È per questo che amo il linguaggio C++! Sono in arrivo sempre nuovi modi per spararsi nei piedi. L'analizzatore PVS-Studio avrà sempre lavoro :).
Frammento N8: Puntatore nullo
void FunctionDumper::dump(const PDBSymbolTypeFunctionArg &Symbol) {
uint32_t TypeId = Symbol.getTypeId();
auto Type = Symbol.getSession().getSymbolById(TypeId);
if (Type)
Printer << "";
else
Type->dump(*this);
}Avviso PVS-Studio: V522 [CWE-476] Potrebbe verificarsi dereferenziazione del puntatore nullo 'Type'. PrettyFunctionDumper.cpp 233
Oltre ai gestori di errori, di solito non vengono testate nemmeno le funzioni di debug per la stampa dei dati. Questo è proprio un caso del genere. La funzione attende l'utente, che invece di risolvere i propri problemi sarà costretto a occuparsi della sua correzione.
Corretto:
if (Type)
Type->dump(*this);
else
Printer << "";Frammento N9: Puntatore nullo
void SearchableTableEmitter::collectTableEntries(
GenericTable &Table, const std::vector &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());
....
}Avviso PVS-Studio: V522 [CWE-476] Dereferenziazione del puntatore nullo ‘Ty’ potrebbe avere luogo. SearchableTableEmitter.cpp 614
Credo che sia tutto chiaro e non richieda spiegazioni.
Frammento N10: Errore di battitura
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;
}Avviso PVS-Studio: La variabile ‘Identifier->Type’ è assegnata a se stessa. FormatTokenLexer.cpp 249
Non ha senso assegnare a una variabile se stessa. Probabilmente si voleva scrivere:
Identifier->Type = Question->Type;Frammento N11: Break sospetto
void SystemZOperand::print(raw_ostream &OS) const {
switch (Kind) {
break;
case KindToken:
OS << "Token:" << getToken();
break;
case KindReg:
OS << "Reg:" << SystemZInstPrinter::getRegisterName(getReg());
break;
....
}Avviso PVS-Studio: [CWE-478] Si consiglia di ispezionare l’istruzione ‘switch’. È possibile che il primo operatore ‘case’ manchi. SystemZAsmParser.cpp 652
All'inizio è presente un operatore molto sospetto break. Non è stato dimenticato di scrivere qualcos'altro qui?
Frammento N12: Controllo del puntatore dopo la dereferenziazione
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");
....
}Avviso PVS-Studio: [CWE-476] Il puntatore ‘Callee’ è stato utilizzato prima di essere verificato rispetto a nullptr. Controlla le righe: 172, 174. AMDGPUInline.cpp 172
Puntatore Callee viene dereferenziato all'inizio al momento della chiamata della funzione getTTI.
E poi si scopre che questo puntatore deve essere controllato per l'uguaglianza nullptr:
if (!Callee || Callee->isDeclaration())Ma è già tardi…
Frammento N13 — N…: Controllo del puntatore dopo la dereferenziazione
La situazione considerata nel precedente frammento di codice non è unica. Si verifica qui:
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()) {
....
}Avviso PVS-Studio: V595 [CWE-476] Il puntatore ‘CalleeFn’ è stato utilizzato prima di essere verificato rispetto a nullptr. Controlla le righe: 1079, 1081. SimplifyLibCalls.cpp 1079
E qui:
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()); // <=
....
}Avviso PVS-Studio: V595 [CWE-476] Il puntatore ‘ND’ è stato utilizzato prima di essere verificato contro nullptr. Controlla le righe: 532, 534. SemaTemplateInstantiateDecl.cpp 532
E qui:
- V595 [CWE-476] Il puntatore ‘U’ è stato utilizzato prima di essere verificato contro nullptr. Controlla le righe: 404, 407. DWARFFormValue.cpp 404
- V595 [CWE-476] Il puntatore ‘ND’ è stato utilizzato prima di essere verificato contro nullptr. Controlla le righe: 2149, 2151. SemaTemplateInstantiate.cpp 2149
E dopo non mi è più interessato studiare gli avvisi con numero V595. Quindi non so se ci siano altri errori simili, oltre a quelli elencati qui. Probabilmente ci sono.
Frammento N17, N18: Spostamento sospetto
static inline bool processLogicalImmediate(uint64_t Imm, unsigned RegSize,
uint64_t &Encoding) {
....
unsigned Size = RegSize;
....
uint64_t NImms = ~(Size-1) << 1;
....
}Avviso PVS-Studio: [CWE-190] Considera di ispezionare l'espressione ‘~(Size — 1) << 1’. Spostamento dei bit del valore a 32 bit con un successivo ampliamento al tipo a 64 bit. AArch64AddressingModes.h 260
Forse non è un errore e il codice funziona esattamente come previsto. Ma questo è chiaramente un punto molto sospetto e deve essere controllato.
Supponiamo che la variabile Dimensione sia uguale a 16, e quindi l'autore del codice prevedeva di ottenere nella variabile NImms il valore:
1111111111111111111111111111111111111111111111111111111111100000
Tuttavia, in realtà, otterremo il valore:
0000000000000000000000000000000011111111111111111111111111100000
Il fatto è che tutti i calcoli sono effettuati utilizzando il tipo unsigned a 32 bit. E solo successivamente, questo tipo unsigned a 32 bit sarà espanso implicitamente a uint64_t. In questo caso, i bit più significativi risulteranno essere zero.
Si può correggere la situazione in questo modo:
uint64_t NImms = ~static_cast(Size-1) << 1;Situazione analoga: V629 [CWE-190] Considera di ispezionare l'espressione ‘Immr << 6’. Spostamento dei bit del valore a 32 bit con un successivo ampliamento al tipo a 64 bit. AArch64AddressingModes.h 269
Frammento N19: Parola chiave mancante 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 utilizza il token "vcc".
// Salta.
continue;
} if (isRegOrImmWithInputMods(Desc, Inst.getNumOperands())) { // <=
Op.addRegWithFPInputModsOperands(Inst, 2);
} else if (Op.isDPPCtrl()) {
Op.addImmOperands(Inst, 1);
} else if (Op.isImm()) {
// Gestisci argomenti opzionali
OptionalIdx[Op.getImmTy()] = I;
} else {
llvm_unreachable("Tipo di operando non valido");
}
....
}Avviso PVS-Studio: [CWE-670] Considera di ispezionare la logica dell'applicazione. È possibile che manchi la parola chiave ‘else’. AMDGPUAsmParser.cpp 5655
Non ci sono errori qui. Poiché il blocco then del primo if termina su continua, quindi non importa se c'è una parola chiave else o meno. In ogni caso, il codice funzionerà allo stesso modo. Tuttavia, un codice mancante else rende il codice più difficile da comprendere e pericoloso. Se in seguito continua scompare, il codice inizierà a funzionare in modo completamente diverso. A mio avviso, è meglio aggiungere else.
Fragmento N20: Quattro errori simili
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;
}Avvisi PVS-Studio:
- V655 [CWE-480] Le stringhe sono state concatenate ma non sono utilizzate. Considera di esaminare l'espressione ‘Result + Name.str()’. Symbol.cpp 32
- V655 [CWE-480] Le stringhe sono state concatenate ma non sono utilizzate. Considera di esaminare l'espressione ‘Result + «(ObjC Class) » + Name.str()’. Symbol.cpp 35
- V655 [CWE-480] Le stringhe sono state concatenate ma non sono utilizzate. Considera di esaminare l'espressione ‘Result + «(ObjC Class EH) » + Name.str()’. Symbol.cpp 38
- V655 [CWE-480] Le stringhe sono state concatenate ma non sono utilizzate. Considera di esaminare l'espressione ‘Result + «(ObjC IVar) » + Name.str()’. Symbol.cpp 41
Per errore, viene utilizzato l'operatore + invece dell'operatore +=. Di conseguenza, si creano costrutti privi di senso.
Fragmento N21: Comportamento indefinito
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 non può essere vuoto");
for (auto &Op : Ops) {
assert(!Op.empty() && "Operatore vuoto");
if (FeaturesMap.find(Op) == FeaturesMap.end())
FeaturesMap[Op] = FeaturesMap.size();
}
}
}Prova a trovare il codice pericoloso da solo. Ecco un'immagine per distrarti, così non guardi subito la risposta:
Avviso PVS-Studio: [CWE-758] Si utilizza una costruzione pericolosa: ‘FeaturesMap[Op] = FeaturesMap.size()’, dove ‘FeaturesMap’ è di classe ‘map’. Questo può portare a comportamenti indefiniti. RISCVCompressInstEmitter.cpp 490
Riga problematica:
FeaturesMap[Op] = FeaturesMap.size();Se l'elemento Op non viene trovato, viene creato un nuovo elemento nella mappa e lì viene registrato il numero di elementi in questa mappa. Peccato che non si sappia se verrà chiamata la funzione size prima o dopo l'aggiunta di un nuovo elemento.
Fragmento N22-N24: Assegnazioni ripetute
Errore 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;
}
....
}Avviso PVS-Studio: [CWE-563] La variabile ‘NType’ è assegnata valori due volte consecutivamente. Forse è un errore. Controlla le righe: 1663, 1664. MachOObjectFile.cpp 1664
Non credo ci sia un errore reale qui. È solo una assegnazione ripetuta superflua. Ma è comunque un errore.
Analogamente:
- V519 [CWE-563] La variabile ‘B.NDesc’ è assegnata valori due volte consecutivamente. Forse è un errore. Controlla le righe: 1488, 1489. llvm-nm.cpp 1489
- V519 [CWE-563] La variabile è assegnata valori due volte consecutivamente. Forse è un errore. Controlla le righe: 59, 61. coff2yaml.cpp 61
Fragmento N25-N27: Altri assegnamenti ripetuti
Ora consideriamo un'opzione leggermente diversa di assegnazione ripetuta.
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;
....
}Avviso PVS-Studio: V519 [CWE-563] La variabile ‘Alignment’ è assegnata valori due volte consecutivamente. Forse è un errore. Controlla le righe: 1158, 1160. LoadStoreVectorizer.cpp 1160
Questo codice è molto strano e sembra contenere un errore logico. All'inizio, alla variabile Alignment viene assegnato un valore in base a una condizione. Poi avviene una seconda assegnazione, ma questa volta senza alcun controllo.
Situazioni simili possono essere viste qui:
- V519 [CWE-563] La variabile ‘Effects’ è assegnata valori due volte consecutivamente. Forse è un errore. Controlla le righe: 152, 165. WebAssemblyRegStackify.cpp 165
- V519 [CWE-563] La variabile ‘ExpectNoDerefChunk’ è assegnata valori due volte consecutivamente. Forse è un errore. Controlla le righe: 4970, 4973. SemaType.cpp 4973
Fragmento N28: Condizione sempre vera
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) // Supporto per l'istruzione PAUSE // <=
break;
}
....
}Avviso PVS-Studio: [CWE-571] L'espressione ‘nextByte != 0x90’ è sempre vera. X86DisassemblerDecoder.cpp 379
Il controllo non ha senso. La variabile nextByte è sempre diversa dal valore 0x90, il che risulta dal controllo precedente. È un errore logico.
Fragmento N29 — N…: Condizioni sempre vere/false
L'analizzatore fornisce molti avvisi riguardo al fatto che l'intera condizione () o parte di essa () è sempre vero o falso. Spesso non si tratta di veri errori, ma solo di codice poco curato, risultato di macro espanse e simili. Tuttavia, ha senso esaminare tutti questi avvisi, poiché di tanto in tanto si possono incontrare veri errori logici. Per esempio, questo pezzo di codice è sospetto:
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;
....
}Avviso PVS-Studio: [CWE-570] Una parte dell'espressione condizionale è sempre falsa: RegNo == 0xe. ARMDisassembler.cpp 939
La costante 0xE è il valore 14 in decimale. Controllo RegNo == 0xe non ha senso, poiché se RegNo > 13, la funzione terminerà la sua esecuzione.
Ci sono stati molti altri avvisi con identificativi V547 e V560, ma, come nel caso di , studiare questi avvisi non mi interessava. Era già chiaro che avevo materiale sufficiente per scrivere un articolo :). Pertanto, non si sa quante di queste tipologie di errori possano essere rilevate in LLVM utilizzando PVS-Studio.
Ecco un esempio del perché studiare queste attivazioni sia noioso. L'analizzatore ha perfettamente ragione nell'emissione di un avviso sul seguente codice. Ma non si tratta neppure di un errore.
bool UnwrappedLineParser::parseBracedList(bool ContinueOnSemicolons,
tok::TokenKind ClosingBraceKind) {
bool HasError = false;
....
HasError = true;
if (!ContinueOnSemicolons)
return !HasError;
....
}Avviso PVS-Studio: V547 [CWE-570] L'espressione ‘!HasError’ è sempre falsa. UnwrappedLineParser.cpp 1635
Fragmento N30: Return sospetto
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();
}
....
}Avviso PVS-Studio: [CWE-670] Un ‘return’ incondizionato all'interno di un ciclo. R600OptimizeVectorRegisters.cpp 63
Questo è o un errore, o un espediente specifico che intende chiarire qualcosa ai programmatori che leggono il codice. Una costruzione del genere non chiarisce nulla per me e appare molto sospetta. È meglio evitare di scrivere in questo modo :).
Stanco? Allora è il momento di preparare un tè o un caffè.
Difetti rivelati da nuove diagnosi
Penso che 30 attivazioni delle vecchie diagnosi siano sufficienti. Ora vediamo quali aspetti interessanti possiamo scoprire con le nuove diagnosi, che sono state aggiunte all'analizzatore dopo la verifica. In totale, nel frattempo, sono state aggiunte 66 diagnosi generali all'analizzatore C++.
Fragmento N31: Codice irraggiungibile
Errore 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();
}Avviso PVS-Studio: [CWE-561] Codice inaccessibile rilevato. È possibile che sia presente un errore. ExecutionUtils.cpp 146
Come potete vedere, entrambi i rami dell'operatore if terminano con una chiamata all'operatore return. Di conseguenza, il contenitore CtorDtorsByPriority non verrà mai svuotato.
Frammento N32: Codice inaccessibile
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(), "tipo di sommario inaspettato");
}
Lex.setIgnoreColonInIdentifiers(false); // <=
return false;
}Avviso PVS-Studio: V779 [CWE-561] Codice inaccessibile rilevato. È possibile che sia presente un errore. LLParser.cpp 835
Situazione interessante. Iniziamo a considerare questo punto:
return ParseTypeIdEntry(SummaryID);
break;A prima vista sembra che qui non ci siano errori. Sembra che l'operatore break sia superfluo e possa essere semplicemente rimosso. Tuttavia, non è così semplice.
L'analizzatore genera un avviso sulle righe:
Lex.setIgnoreColonInIdentifiers(false);
return false;E infatti, questo codice è inaccessibile. Tutti i casi in switch terminano con una chiamata all'operatore return. E ora un singolo break non sembra così innocuo! Forse uno dei rami dovrebbe terminare con break, invece di return?
Frammento N33: Azzeramento casuale dei bit superiori
unsigned getStubAlignment() override {
if (Arch == Triple::systemz)
return 8;
else
return 1;
}
Expected
RuntimeDyldImpl::emitSection(const ObjectFile &Obj,
const SectionRef &Section,
bool IsCode) {
....
uint64_t DataSize = Section.getSize();
....
if (StubBufSize > 0)
DataSize &= ~(getStubAlignment() - 1);
....
}Avviso PVS-Studio: La dimensione della maschera di bit è inferiore alla dimensione del primo operando. Questo causerà la perdita dei bit superiori. RuntimeDyld.cpp 815
Si noti che la funzione getStubAlignment restituisce il tipo unsigned. Calcoliamo il valore dell'espressione, assumendo che la funzione restituisca il valore 8:
~(getStubAlignment() - 1)
~(8u-1)
0xFFFFFFF8u
Ora si noti che la variabile DataSize ha un tipo unsigned a 64 bit. Di conseguenza, durante l'esecuzione dell'operazione DataSize & 0xFFFFFFF8u, tutti e trenta due i bit superiori saranno azzerati. Probabilmente, questo non è ciò che voleva il programmatore. Suspetto che volesse calcolare: DataSize & 0xFFFFFFFFFFFFFFF8u.
Per correggere l'errore, si dovrebbe scrivere così:
DataSize &= ~(static_cast(getStubAlignment()) - 1);Oppure così:
DataSize &= ~(getStubAlignment() - 1ULL);Frammento N34: Cast di tipo esplicito fallito
template <typename T>
void scaleShuffleMask(int Scale, ArrayRef<T> Mask,
SmallVectorImpl<T> &ScaledMask) {
assert(0 < Scale && "Fattore di scala inaspettato");
int NumElts = Mask.size();
ScaledMask.assign(static_cast<size_t>(NumElts * Scale), -1);
....
}Avviso PVS-Studio: [CWE-190] Possibile overflow. Considera di castare gli operandi dell'operatore ‘NumElts * Scale’ al tipo ‘size_t’, non al risultato. X86ISelLowering.h 1577
Il cast di tipo esplicito è usato per evitare overflow quando si moltiplicano variabili di tipo int. Tuttavia, qui il cast di tipo esplicito non protegge dall'overflow. All'inizio, le variabili saranno moltiplicate e solo successivamente il risultato della moltiplicazione a 32 bit sarà esteso al tipo .
Frammento N35: Copia-Incolla fallita
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] Sono stati trovati due frammenti di codice simili. Forse, è un errore di battitura e la variabile ‘Op1’ dovrebbe essere usata invece di ‘Op0’. InstCombineCompares.cpp 5507
Questa nuova e interessante diagnosi identifica situazioni in cui un frammento di codice è stato copiato, e sono stati modificati alcuni nomi, ma in un punto non è stata fatta la correzione.
Si noti che nel secondo blocco è stato modificato Op0 in Op1. Ma in un punto non è stata fatta la correzione. È probabile che dovesse essere scritto così:
if (!match(Op1, m_PosZeroFP()) && isKnownNeverNaN(Op1, &TLI)) {
I.setOperand(1, ConstantFP::getNullValue(Op1->getType()));
return &I;
}Frammento N36: Confusione nelle variabili
struct Status {
unsigned Mask;
unsigned Mode;
Status() : Mask(0), Mode(0){};
Status(unsigned Mask, unsigned Mode) : Mask(Mask), Mode(Mode) {
Mode &= Mask;
};
....
};Avviso PVS-Studio: [CWE-563] La variabile ‘Mode’ è assegnata ma non viene utilizzata entro la fine della funzione. SIModeRegister.cpp 48
È molto pericoloso dare agli argomenti delle funzioni gli stessi nomi dei membri della classe. È molto facile confondersi. Questo è proprio un caso del genere. Questa espressione non ha senso:
Mode &= Mask;L'argomento della funzione viene modificato. E basta. Questo argomento non viene più utilizzato. È probabile che dovesse essere scritto così:
Status(unsigned Mask, unsigned Mode) : Mask(Mask), Mode(Mode) {
this->Mode &= Mask;
};Frammento N37: Confusione nelle variabili
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;
}Avviso PVS-Studio: V1001 [CWE-563] La variabile ‘Size’ viene assegnata ma non viene utilizzata entro la fine della funzione. Object.cpp 424
La situazione è simile alla precedente. Dovrebbe essere scritto:
this->Size += this->EntrySize;Frammento N38-N47: Il puntatore non è stato verificato
In precedenza abbiamo esaminato esempi di attivazione della diagnosi . Il suo scopo è che il puntatore venga dereferenziato all'inizio e poi verificato. La diagnosi giovane è concettualmente opposta, ma rileva anche molti errori. Identifica situazioni in cui il puntatore è stato controllato all'inizio, per poi dimenticare di farlo successivamente. Consideriamo tali casi trovati all'interno di 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()); // <=
....
}Avviso PVS-Studio: V1004 [CWE-476] Il puntatore ‘Ptr’ è stato utilizzato in modo non sicuro dopo essere stato verificato contro nullptr. Controllare le righe: 729, 738. TargetTransformInfoImpl.h 738
Variabile Ptr può essere uguale a nullptr, come evidenziato dalla verifica:
if (Ptr != nullptr)Tuttavia, più in basso questo puntatore viene dereferenziato senza una verifica preliminare:
auto PtrSizeBits = DL.getPointerTypeSizeInBits(Ptr->getType());Consideriamo un altro caso analogo.
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(); // <=
....
}Avviso PVS-Studio: V1004 [CWE-476] Il puntatore ‘FD’ è stato utilizzato in modo non sicuro dopo essere stato verificato contro nullptr. Controllare le righe: 3228, 3231. CGDebugInfo.cpp 3231
Si noti il puntatore FD. Sono sicuro che il problema sia evidente e non sia necessaria alcuna spiegazione speciale.
E ancora:
static void computePolynomialFromPointer(Value &Ptr, Polynomial &Result,
Value *&BasePtr,
const DataLayout &DL) {
PointerType *PtrTy = dyn_cast(Ptr.getType());
if (!PtrTy) { // getPointerAddressSpace()); // <=
....
}Avviso PVS-Studio: V1004 [CWE-476] Il puntatore ‘PtrTy’ è stato usato in modo non sicuro dopo essere stato verificato contro nullptr. Controllare le righe: 960, 965. InterleavedLoadCombinePass.cpp 965
Come proteggersi da tali errori? Siate più attenti durante il Code Review e utilizzate l'analizzatore statico PVS-Studio per controlli regolari del codice.
Non ha senso presentare altri frammenti di codice con errori di questo tipo. Lascio nell'articolo solo l'elenco degli avvisi:
- V1004 [CWE-476] Il puntatore ‘Expr’ è stato usato in modo non sicuro dopo essere stato verificato contro nullptr. Controllare le righe: 1049, 1078. DebugInfoMetadata.cpp 1078
- V1004 [CWE-476] Il puntatore ‘PI’ è stato usato in modo non sicuro dopo essere stato verificato contro nullptr. Controllare le righe: 733, 753. LegacyPassManager.cpp 753
- V1004 [CWE-476] Il puntatore ‘StatepointCall’ è stato usato in modo non sicuro dopo essere stato verificato contro nullptr. Controllare le righe: 4371, 4379. Verifier.cpp 4379
- V1004 [CWE-476] Il puntatore ‘RV’ è stato usato in modo non sicuro dopo essere stato verificato contro nullptr. Controllare le righe: 2263, 2268. TGParser.cpp 2268
- V1004 [CWE-476] Il puntatore ‘CalleeFn’ è stato usato in modo non sicuro dopo essere stato verificato contro nullptr. Controllare le righe: 1081, 1096. SimplifyLibCalls.cpp 1096
- V1004 [CWE-476] Il puntatore ‘TC’ è stato usato in modo non sicuro dopo essere stato verificato contro nullptr. Controllare le righe: 1819, 1824. Driver.cpp 1824
Frammento N48-N60: Non critico, ma difetto (possibile perdita di memoria)
std::unique_ptr createISelMutator() {
....
std::vector<std::unique_ptr> Strategies;
Strategies.emplace_back(
new InjectorIRStrategy(InjectorIRStrategy::getDefaultOps()));
....
}Avviso PVS-Studio: [CWE-460] Un puntatore senza proprietario viene aggiunto al contenitore ‘Strategies’ dal metodo ’emplace_back’. Si verificherà una perdita di memoria in caso di eccezione. llvm-isel-fuzzer.cpp 58
Per aggiungere un elemento alla fine di un contenitore di tipo std::vector<std::unique_ptr> non è possibile semplicemente scrivere xxx.push_back(new X), poiché non c'è una conversione implicita da X* in std::unique_ptr.
Una soluzione comune è scrivere xxx.emplace_back(new X), poiché si compila: il metodo emplace_back costruisce l'elemento direttamente dagli argomenti e quindi può utilizzare costruttori espliciti.
Questo non è sicuro. Se il vettore è pieno, avviene il riallocamento della memoria. L'operazione di riallocazione della memoria può fallire, risultando in un'eccezione std::bad_alloc. In tal caso, il puntatore andrà perso e l'oggetto creato non verrà mai eliminato.
Una soluzione sicura è creare unique_ptr, che possiederà il puntatore fino a quando il vettore non tenterà di riallocare la memoria:
xxx.push_back(std::unique_ptr(new X))A partire da C++14, è possibile utilizzare 'std::make_unique':
xxx.push_back(std::make_unique())Questo tipo di difetto non è critico per LLVM. Se non riesce a allocare memoria, il funzionamento del compilatore verrà semplicemente interrotto. Tuttavia, per le applicazioni con un lungo , che non possono semplicemente terminare se l'allocazione della memoria non riesce, questo può rivelarsi un errore davvero sgradevole.
Quindi, sebbene questo codice non rappresenti un pericolo pratico per LLVM, ho ritenuto utile parlare di questo modello di errori e di come l'analizzatore PVS-Studio abbia imparato a rilevarlo.
Altri avvisi di questo tipo:
- V1023 [CWE-460] Un puntatore senza proprietario viene aggiunto al contenitore 'Passes' tramite il metodo 'emplace_back'. Si verificherà una perdita di memoria in caso di eccezione. PassManager.h 546
- V1023 [CWE-460] Un puntatore senza proprietario viene aggiunto al contenitore 'AAs' tramite il metodo 'emplace_back'. Si verificherà una perdita di memoria in caso di eccezione. AliasAnalysis.h 324
- V1023 [CWE-460] Un puntatore senza proprietario viene aggiunto al contenitore 'Entries' tramite il metodo 'emplace_back'. Si verificherà una perdita di memoria in caso di eccezione. DWARFDebugFrame.cpp 519
- V1023 [CWE-460] Un puntatore senza proprietario viene aggiunto al contenitore 'AllEdges' tramite il metodo 'emplace_back'. Si verificherà una perdita di memoria in caso di eccezione. CFGMST.h 268
- V1023 [CWE-460] Un puntatore senza proprietario viene aggiunto al contenitore 'VMaps' tramite il metodo 'emplace_back'. Si verificherà una perdita di memoria in caso di eccezione. SimpleLoopUnswitch.cpp 2012
- V1023 [CWE-460] Un puntatore senza proprietario viene aggiunto al contenitore 'Records' tramite il metodo 'emplace_back'. Si verificherà una perdita di memoria in caso di eccezione. FDRLogBuilder.h 30
- V1023 [CWE-460] Un puntatore senza proprietario viene aggiunto al contenitore 'PendingSubmodules' tramite il metodo 'emplace_back'. Si verificherà una perdita di memoria in caso di eccezione. ModuleMap.cpp 810
- V1023 [CWE-460] Un puntatore senza proprietario viene aggiunto al contenitore 'Objects' tramite il metodo 'emplace_back'. Si verificherà una perdita di memoria in caso di eccezione. DebugMap.cpp 88
- V1023 [CWE-460] Un puntatore senza proprietario viene aggiunto al contenitore 'Strategies' tramite il metodo 'emplace_back'. Si verificherà una perdita di memoria in caso di eccezione. llvm-isel-fuzzer.cpp 60
- V1023 [CWE-460] Un puntatore senza proprietario viene aggiunto al contenitore 'Modifiers' tramite il metodo 'emplace_back'. Si verificherà una perdita di memoria in caso di eccezione. llvm-stress.cpp 685
- V1023 [CWE-460] Un puntatore senza proprietario viene aggiunto al contenitore 'Modifiers' tramite il metodo 'emplace_back'. Si verificherà una perdita di memoria in caso di eccezione. llvm-stress.cpp 686
- V1023 [CWE-460] Un puntatore senza proprietario viene aggiunto al contenitore 'Modifiers' tramite il metodo 'emplace_back'. Si verificherà una perdita di memoria in caso di eccezione. llvm-stress.cpp 688
- V1023 [CWE-460] Un puntatore senza proprietario viene aggiunto al contenitore 'Modifiers' tramite il metodo 'emplace_back'. Si verificherà una perdita di memoria in caso di eccezione. llvm-stress.cpp 689
- V1023 [CWE-460] Un puntatore senza proprietario viene aggiunto al contenitore 'Modifiers' tramite il metodo 'emplace_back'. Si verificherà una perdita di memoria in caso di eccezione. llvm-stress.cpp 690
- V1023 [CWE-460] Un puntatore senza proprietario viene aggiunto al contenitore 'Modifiers' tramite il metodo 'emplace_back'. Si verificherà una perdita di memoria in caso di eccezione. llvm-stress.cpp 691
- V1023 [CWE-460] Un puntatore senza proprietario è stato aggiunto al contenitore 'Modifiers' dal metodo 'emplace_back'. Si verificherà una perdita di memoria in caso di eccezione. llvm-stress.cpp 692
- V1023 [CWE-460] Un puntatore senza proprietario è stato aggiunto al contenitore 'Modifiers' dal metodo 'emplace_back'. Si verificherà una perdita di memoria in caso di eccezione. llvm-stress.cpp 693
- V1023 [CWE-460] Un puntatore senza proprietario è stato aggiunto al contenitore 'Modifiers' dal metodo 'emplace_back'. Si verificherà una perdita di memoria in caso di eccezione. llvm-stress.cpp 694
- V1023 [CWE-460] Un puntatore senza proprietario è stato aggiunto al contenitore 'Operands' dal metodo 'emplace_back'. Si verificherà una perdita di memoria in caso di eccezione. GlobalISelEmitter.cpp 1911
- V1023 [CWE-460] Un puntatore senza proprietario è stato aggiunto al contenitore 'Stash' dal metodo 'emplace_back'. Si verificherà una perdita di memoria in caso di eccezione. GlobalISelEmitter.cpp 2100
- V1023 [CWE-460] Un puntatore senza proprietario è stato aggiunto al contenitore 'Matchers' dal metodo 'emplace_back'. Si verificherà una perdita di memoria in caso di eccezione. GlobalISelEmitter.cpp 2702
Conclusione
In totale ho registrato 60 avvisi, dopodiché mi sono fermato. Ci sono altri difetti che l'analizzatore PVS-Studio rileva in LLVM? Sì, ci sono. Tuttavia, quando stavo scrivendo i frammenti di codice per l'articolo, era già tardi, anzi, era notte, e ho deciso che era il momento di concludere.
Spero che vi sia piaciuto e che vogliate provare l'analizzatore PVS-Studio.
Potete scaricare l'analizzatore e ottenere una chiave di attivazione su .
La cosa più importante è utilizzare l'analisi statica regolarmente. Controlli sporadici, realizzati da noi per promuovere la metodologia dell'analisi statica e PVS-Studio non sono uno scenario normale.
Buona fortuna per migliorare la qualità e l'affidabilità del codice!
Se volete condividere questo articolo con un pubblico anglofono, vi prego di usare il link alla traduzione: Andrey Karpov. .
Fonte: habr.com
