Plus de deux ans se sont écoulés depuis la dernière vérification du code du projet LLVM à l'aide de notre analyseur PVS-Studio. Assurons-nous que l'analyseur PVS-Studio reste un outil leader pour identifier les erreurs et les vulnérabilités potentielles. Pour ce faire, nous vérifierons et trouverons de nouvelles erreurs dans la version LLVM 8.0.0.
Article à écrire
Pour être honnête, je ne voulais pas écrire cet article. Ce n’est pas intéressant d’écrire sur un projet que l’on a déjà vérifié plusieurs fois (
Chaque fois qu'une nouvelle version de LLVM est publiée ou mise à jour
Regardez, la nouvelle version de Clang Static Analyzer a appris à trouver de nouvelles erreurs ! Il me semble que la pertinence d'utiliser PVS-Studio diminue. Clang trouve plus d'erreurs qu'avant et rattrape les capacités de PVS-Studio. Que penses-tu de cela?
À cela, je veux toujours répondre quelque chose comme :
Nous ne restons pas les bras croisés non plus ! Nous avons considérablement amélioré les capacités de l'analyseur PVS-Studio. Alors ne vous inquiétez pas, nous continuons à diriger comme avant.
Malheureusement, c'est une mauvaise réponse. Il n’y a aucune preuve là-dedans. Et c'est pourquoi j'écris cet article maintenant. Ainsi, le projet LLVM a été à nouveau vérifié et diverses erreurs y ont été trouvées. Je vais maintenant vous démontrer ceux qui m'ont semblé intéressants. Clang Static Analyzer ne peut pas trouver ces erreurs (ou il est extrêmement gênant de le faire avec son aide). Mais nous pouvons. De plus, j'ai trouvé et noté toutes ces erreurs en une soirée.
Mais la rédaction de l’article a pris plusieurs semaines. Je ne pouvais tout simplement pas me résoudre à mettre tout cela dans un texte :).
À propos, si vous êtes intéressé par les technologies utilisées dans l'analyseur PVS-Studio pour identifier les erreurs et les vulnérabilités potentielles, je vous suggère de vous familiariser avec ceci.
Diagnostics nouveaux et anciens
Comme nous l'avons déjà indiqué, il y a environ deux ans, le projet LLVM a été à nouveau vérifié et les erreurs trouvées ont été corrigées. Cet article présentera désormais un nouveau lot d'erreurs. Pourquoi de nouveaux bugs ont-ils été trouvés ? Il y a 3 raisons à cela :
- Le projet LLVM évolue, modifie l'ancien code et ajoute du nouveau code. Naturellement, de nouvelles erreurs apparaissent dans le code modifié et écrit. Cela démontre clairement que l’analyse statique doit être utilisée régulièrement et non occasionnellement. Nos articles montrent bien les capacités de l'analyseur PVS-Studio, mais cela n'a rien à voir avec l'amélioration de la qualité du code et la réduction du coût de correction des erreurs. Utilisez régulièrement un analyseur de code statique !
- Nous finalisons et améliorons les diagnostics existants. Par conséquent, l’analyseur peut identifier les erreurs qu’il n’a pas remarquées lors des analyses précédentes.
- De nouveaux diagnostics sont apparus dans PVS-Studio qui n'existaient pas il y a 2 ans. J'ai décidé de les mettre en évidence dans une section séparée pour montrer clairement le développement de PVS-Studio.
Défauts identifiés par des diagnostics existant il y a 2 ans
Fragment N1 : Copier-Coller
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
....
}
Avertissement PVS-Studio :
Il est vérifié que le nom commence par la sous-chaîne "avx512.mask.permvar.". Lors du deuxième contrôle, ils voulaient évidemment écrire autre chose, mais ils ont oublié de corriger le texte copié.
Fragment N2 : Faute de frappe
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;
....
}
Attention PVS-Studio : V501 Il y a des sous-expressions identiques 'CXNameRange_WantQualifier' à gauche et à droite du '|' opérateur. CIndex.cpp 7245
En raison d'une faute de frappe, la même constante nommée est utilisée deux fois CXNameRange_WantQualifier.
Fragment N3 : Confusion avec la priorité des opérateurs
int PPCTTIImpl::getVectorInstrCost(unsigned Opcode, Type *Val, unsigned Index) {
....
if (ISD == ISD::EXTRACT_VECTOR_ELT && Index == ST->isLittleEndian() ? 1 : 0)
return 0;
....
}
Avertissement PVS-Studio :
À mon avis, c'est une très belle erreur. Oui, je sais que j'ai des idées étranges sur la beauté :).
Maintenant, selon
(ISD == ISD::EXTRACT_VECTOR_ELT && (Index == ST->isLittleEndian())) ? 1 : 0
D'un point de vue pratique, une telle condition n'a pas de sens, puisqu'elle peut se réduire à :
(ISD == ISD::EXTRACT_VECTOR_ELT && Index == ST->isLittleEndian())
C'est une erreur évidente. Très probablement, ils voulaient comparer 0/1 avec une variable Sommaire. Pour corriger le code, vous devez ajouter des parenthèses autour de l'opérateur ternaire :
if (ISD == ISD::EXTRACT_VECTOR_ELT && Index == (ST->isLittleEndian() ? 1 : 0))
D’ailleurs, l’opérateur ternaire est très dangereux et provoque des erreurs logiques. Soyez très prudent et ne soyez pas gourmand en parenthèses. J'ai regardé ce sujet plus en détail
Fragment N4, N5 : pointeur nul
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;
}
....
}
Avertissement PVS-Studio :
Si le pointeur LHS est nul, un avertissement doit être émis. Cependant, ce même pointeur nul sera déréférencé : LHS->getAsString().
Il s'agit d'une situation très typique où une erreur est cachée dans un gestionnaire d'erreurs, puisque personne ne les teste. Les analyseurs statiques vérifient tout le code accessible, quelle que soit la fréquence d'utilisation. Il s'agit d'un très bon exemple de la manière dont l'analyse statique complète d'autres techniques de test et de protection contre les erreurs.
Erreur de gestion de pointeur similaire RHS autorisé dans le code juste en dessous : V522 [CWE-476] Un déréférencement du pointeur nul 'RHS' pourrait avoir lieu. TGParser.cpp 2186
Fragment N6 : Utilisation du pointeur après un déplacement
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);
}
....
}
Avertissement PVS-Studio : V522 [CWE-476] Un déréférencement du pointeur nul 'ProgClone' peut avoir lieu. Mauvaise compilation.cpp 601
Au début un pointeur intelligent CloneProg cesse de posséder l'objet :
BD.setNewProgram(std::move(ProgClone));
En fait, maintenant CloneProg est un pointeur nul. Par conséquent, un déréférencement de pointeur nul devrait se produire juste en dessous :
Function *NewF = ProgClone->getFunction(MisCompFunctions[i].first);
Mais en réalité, cela n’arrivera pas ! Notez que la boucle n'est pas réellement exécutée.
Au début du conteneur Fonctions mal compilées effacé :
MiscompiledFunctions.clear();
Ensuite, la taille de ce conteneur est utilisée dans la condition de boucle :
for (unsigned i = 0, e = MisCompFunctions.size(); i != e; ++i) {
Il est facile de constater que la boucle ne démarre pas. Je pense que c'est aussi un bug et que le code devrait être écrit différemment.
Il semblerait que nous ayons rencontré cette fameuse parité d’erreurs ! Une erreur en masque une autre :).
Fragment N7 : Utilisation du pointeur après un déplacement
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;
}
....
}
Avertissement PVS-Studio : V522 [CWE-476] Un déréférencement du pointeur nul 'Test' peut avoir lieu. Mauvaise compilation.cpp 709
Encore la même situation. Dans un premier temps, le contenu de l'objet est déplacé, puis il est utilisé comme si de rien n'était. Je vois cette situation de plus en plus souvent dans le code de programme après l'apparition de la sémantique du mouvement en C++. C'est pourquoi j'aime le langage C++ ! Il existe de plus en plus de nouvelles façons de se tirer une balle dans la jambe. L'analyseur PVS-Studio aura toujours du travail :).
Fragment N8 : pointeur nul
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);
}
Avertissement PVS-Studio : V522 [CWE-476] Un déréférencement du pointeur nul 'Type' peut avoir lieu. PrettyFunctionDumper.cpp 233
En plus des gestionnaires d'erreurs, les fonctions d'impression de débogage ne sont généralement pas testées. Nous avons justement un cas de ce genre devant nous. La fonction attend l'utilisateur qui, au lieu de résoudre ses problèmes, sera obligé de le résoudre.
Correctement:
if (Type)
Type->dump(*this);
else
Printer << "<unknown-type>";
Fragment N9 : pointeur nul
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());
....
}
Avertissement PVS-Studio : V522 [CWE-476] Un déréférencement du pointeur nul 'Ty' peut avoir lieu. SearchableTableEmitter.cpp 614
Je pense que tout est clair et ne nécessite aucune explication.
Fragment N10 : Faute de frappe
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;
}
Avertissement PVS-Studio :
Cela ne sert à rien de s’attribuer une variable à elle-même. Très probablement, ils voulaient écrire :
Identifier->Type = Question->Type;
Fragment N11 : Rupture suspecte
void SystemZOperand::print(raw_ostream &OS) const {
switch (Kind) {
break;
case KindToken:
OS << "Token:" << getToken();
break;
case KindReg:
OS << "Reg:" << SystemZInstPrinter::getRegisterName(getReg());
break;
....
}
Avertissement PVS-Studio :
Il y a un opérateur très suspect au début pause. Avez-vous oublié d'écrire autre chose ici ?
Fragment N12 : Vérification d'un pointeur après déréférencement
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");
....
}
Avertissement PVS-Studio :
Указатель Appelé au début est déréférencé au moment où la fonction est appelée obtenirTTI.
Et puis il s'avère que l'égalité de ce pointeur doit être vérifiée nullptr:
if (!Callee || Callee->isDeclaration())
Mais c'est trop tard…
Fragment N13 - N... : Vérification d'un pointeur après déréférencement
La situation évoquée dans le fragment de code précédent n’est pas unique. Il apparaît ici :
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()) { // <=
....
}
Avertissement PVS-Studio : V595 [CWE-476] Le pointeur 'CalleeFn' a été utilisé avant d'être vérifié par rapport à nullptr. Vérifiez les lignes : 1079, 1081. SimplifyLibCalls.cpp 1079
Et ici:
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()); // <=
....
}
Avertissement PVS-Studio : V595 [CWE-476] Le pointeur 'ND' a été utilisé avant d'être vérifié par rapport à nullptr. Vérifiez les lignes : 532, 534. SemaTemplateInstantiateDecl.cpp 532
Et ici:
- V595 [CWE-476] Le pointeur 'U' a été utilisé avant d'être vérifié par rapport à nullptr. Vérifiez les lignes : 404, 407. DWARFormValue.cpp 404
- V595 [CWE-476] Le pointeur 'ND' a été utilisé avant d'être vérifié par rapport à nullptr. Lignes de contrôle : 2149, 2151. SemaTemplateInstantiate.cpp 2149
Et puis je ne me suis plus intéressé à l’étude des avertissements portant le numéro V595. Je ne sais donc pas s'il existe d'autres erreurs similaires en plus de celles répertoriées ici. Il est fort probable que ce soit le cas.
Fragment N17, N18 : décalage suspect
static inline bool processLogicalImmediate(uint64_t Imm, unsigned RegSize,
uint64_t &Encoding) {
....
unsigned Size = RegSize;
....
uint64_t NImms = ~(Size-1) << 1;
....
}
Avertissement PVS-Studio :
Il ne s'agit peut-être pas d'un bug et le code fonctionne exactement comme prévu. Mais il s’agit clairement d’un endroit très suspect et qui doit être vérifié.
Disons la variable Taille est égal à 16, puis l'auteur du code a prévu de le récupérer dans une variable NImms valeur:
1111111111111111111111111111111111111111111111111111111111100000
Cependant, en réalité, le résultat sera :
0000000000000000000000000000000011111111111111111111111111100000
Le fait est que tous les calculs sont effectués en utilisant le type non signé 32 bits. Et alors seulement, ce type non signé 32 bits sera implicitement étendu à uint64_t. Dans ce cas, les bits de poids fort seront nuls.
Vous pouvez résoudre la situation comme ceci :
uint64_t NImms = ~static_cast<uint64_t>(Size-1) << 1;
Situation similaire : V629 [CWE-190] Pensez à inspecter l'expression 'Immr << 6'. Décalage de bits de la valeur 32 bits avec extension ultérieure au type 64 bits. AArch64AddressingModes.h 269
Fragment N19 : Mot-clé manquant d'autre?
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");
}
....
}
Avertissement PVS-Studio :
Il n’y a aucune erreur ici. Depuis le bloc d'alors du premier if se termine par continuer, alors ce n'est pas grave, il y a un mot-clé d'autre ou non. Quoi qu'il en soit, le code fonctionnera de la même manière. Encore manqué d'autre rend le code plus flou et dangereux. Si à l'avenir continuer disparaît, le code commencera à fonctionner complètement différemment. A mon avis il vaut mieux ajouter d'autre.
Fragment N20 : Quatre fautes de frappe du même type
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;
}
Avertissements PVS-Studio :
- V655 [CWE-480] Les chaînes ont été concaténées mais ne sont pas utilisées. Pensez à inspecter l'expression 'Result + Name.str()'. Symbole.cpp 32
- V655 [CWE-480] Les chaînes ont été concaténées mais ne sont pas utilisées. Pensez à inspecter l'expression 'Result + "(ObjC Class)" + Name.str()'. Symbole.cpp 35
- V655 [CWE-480] Les chaînes ont été concaténées mais ne sont pas utilisées. Pensez à inspecter l'expression 'Result + "(ObjC Class EH) " + Name.str()'. Symbole.cpp 38
- V655 [CWE-480] Les chaînes ont été concaténées mais ne sont pas utilisées. Pensez à inspecter l'expression 'Result + "(ObjC IVar)" + Name.str()'. Symbole.cpp 41
Par accident, l'opérateur + est utilisé à la place de l'opérateur +=. Le résultat est des designs dénués de sens.
Fragment N21 : comportement non défini
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();
}
}
}
Essayez de trouver vous-même le code dangereux. Et voici une image pour détourner l'attention afin de ne pas regarder immédiatement la réponse :
Avertissement PVS-Studio :
Ligne problématique :
FeaturesMap[Op] = FeaturesMap.size();
Si élément Op n'est pas trouvé, alors un nouvel élément est créé dans la carte et le nombre d'éléments dans cette carte y est écrit. On ne sait juste pas si la fonction sera appelée taille avant ou après l'ajout d'un nouvel élément.
Fragment N22-N24 : Affectations répétées
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;
}
....
}
Avertissement PVS-Studio :
Je ne pense pas qu'il y ait une véritable erreur ici. Juste une mission répétée et inutile. Mais c'est quand même une erreur.
Similaire à:
- V519 [CWE-563] La variable 'B.NDesc' se voit attribuer des valeurs deux fois successivement. C'est peut-être une erreur. Lignes de contrôle : 1488, 1489. llvm-nm.cpp 1489
- V519 [CWE-563] La variable reçoit des valeurs deux fois successivement. C'est peut-être une erreur. Lignes de contrôle : 59, 61. coff2yaml.cpp 61
Fragment N25-N27 : Plus de réaffectations
Examinons maintenant une version légèrement différente de la réaffectation.
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;
....
}
Avertissement PVS-Studio : V519 [CWE-563] La variable 'Alignement' reçoit des valeurs deux fois successivement. C'est peut-être une erreur. Vérifiez les lignes : 1158, 1160. LoadStoreVectorizer.cpp 1160
Il s'agit d'un code très étrange qui contient apparemment une erreur logique. Au début, variable Alignement une valeur est attribuée en fonction de la condition. Et puis la mission se reproduit, mais maintenant sans aucun contrôle.
Des situations similaires peuvent être observées ici :
- V519 [CWE-563] La variable 'Effets' reçoit des valeurs deux fois successivement. C'est peut-être une erreur. Lignes de contrôle : 152, 165. WebAssemblyRegStackify.cpp 165
- V519 [CWE-563] La variable 'ExpectNoDerefChunk' reçoit des valeurs deux fois successivement. C'est peut-être une erreur. Lignes de contrôle : 4970, 4973. SemaType.cpp 4973
Fragment N28 : Condition toujours vraie
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;
}
....
}
Avertissement PVS-Studio :
Vérifier n'a pas de sens. Variable octet suivant toujours pas égal à la valeur Assistance , qui découle du contrôle précédent. C'est une sorte d'erreur logique.
Fragment N29 - N... : Conditions toujours vraies/fausses
L'analyseur émet de nombreux avertissements indiquant que l'ensemble de la condition (
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;
....
}
Avertissement PVS-Studio :
La constante 0xE est la valeur 14 en décimal. Examen N° d'enregistrement == 0xe cela n'a pas de sens parce que si N° d'enregistrement > 13, alors la fonction terminera son exécution.
Il y avait de nombreux autres avertissements avec les ID V547 et V560, mais comme pour
Je vais vous donner un exemple de la raison pour laquelle étudier ces déclencheurs est ennuyeux. L'analyseur a tout à fait raison d'émettre un avertissement pour le code suivant. Mais ce n'est pas une erreur.
bool UnwrappedLineParser::parseBracedList(bool ContinueOnSemicolons,
tok::TokenKind ClosingBraceKind) {
bool HasError = false;
....
HasError = true;
if (!ContinueOnSemicolons)
return !HasError;
....
}
Avertissement PVS-Studio : V547 [CWE-570] L'expression '!HasError' est toujours fausse. UnwrappedLineParser.cpp 1635
Fragment N30 : Retour suspect
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();
}
....
}
Avertissement PVS-Studio :
Il s'agit soit d'une erreur, soit d'une technique spécifique destinée à expliquer quelque chose aux programmeurs qui lisent le code. Ce design ne m’explique rien et semble très suspect. Il vaut mieux ne pas écrire comme ça :).
Fatigué? Il est ensuite temps de préparer du thé ou du café.
Défauts identifiés par de nouveaux diagnostics
Je pense que 30 activations d'anciens diagnostics suffisent. Voyons maintenant quelles choses intéressantes peuvent être trouvées avec les nouveaux diagnostics apparus dans l'analyseur après
Fragment N31 : Code inaccessible
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();
}
Avertissement PVS-Studio :
Comme vous pouvez le constater, les deux branches de l'opérateur if se termine par un appel à l'opérateur retourner. En conséquence, le conteneur CtorDtorsByPriority ne sera jamais effacé.
Fragment N32 : Code inaccessible
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;
}
Avertissement PVS-Studio : V779 [CWE-561] Code inaccessible détecté. Il est possible qu'une erreur soit présente. LLParser.cpp 835
Situation intéressante. Regardons d'abord cet endroit :
return ParseTypeIdEntry(SummaryID);
break;
À première vue, il semble qu’il n’y ait aucune erreur ici. On dirait que l'opérateur pause il y en a un supplémentaire ici, et vous pouvez simplement le supprimer. Cependant, tout n’est pas si simple.
L'analyseur émet un avertissement sur les lignes :
Lex.setIgnoreColonInIdentifiers(false);
return false;
Et effectivement, ce code est inaccessible. Tous les cas dans interrupteur se termine par un appel de l'opérateur retourner. Et maintenant, seul, insensé pause ça n'a pas l'air si inoffensif ! Peut-être qu'une des branches devrait se terminer par pause, pas sur retourner?
Fragment N33 : Réinitialisation aléatoire des bits hauts
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);
....
}
Avertissement PVS-Studio :
Veuillez noter que la fonction getStubAlignment type de retour non signé. Calculons la valeur de l'expression, en supposant que la fonction renvoie la valeur 8 :
~(getStubAlignment() - 1)
~(8u-1)
0xFFFFFFFF8u
Notez maintenant que la variable Taille des données a un type non signé de 64 bits. Il s'avère que lors de l'exécution de l'opération DataSize & 0xFFFFFFF8u, les trente-deux bits de poids fort seront réinitialisés à zéro. Très probablement, ce n'est pas ce que voulait le programmeur. Je soupçonne qu'il voulait calculer : DataSize & 0xFFFFFFFFFFFFFFF8u.
Pour corriger l'erreur, vous devez écrire ceci :
DataSize &= ~(static_cast<uint64_t>(getStubAlignment()) - 1);
Ou alors:
DataSize &= ~(getStubAlignment() - 1ULL);
Fragment N34 : échec de la conversion de type explicite
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);
....
}
Avertissement PVS-Studio :
Le transtypage de type explicite est utilisé pour éviter le débordement lors de la multiplication des variables de type int. Cependant, le transtypage explicite ici ne protège pas contre le débordement. Tout d'abord, les variables seront multipliées, et ensuite seulement le résultat 32 bits de la multiplication sera étendu au type
Fragment N35 : échec du copier-coller
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;
}
....
}
Ce nouveau diagnostic intéressant identifie les situations dans lesquelles un morceau de code a été copié et certains noms qu'il contient ont commencé à être modifiés, mais à un endroit, ils ne l'ont pas corrigé.
Veuillez noter que dans le deuxième bloc, ils ont changé Op0 sur Op1. Mais à un endroit, ils ne l’ont pas réparé. Il aurait probablement dû être écrit ainsi :
if (!match(Op1, m_PosZeroFP()) && isKnownNeverNaN(Op1, &TLI)) {
I.setOperand(1, ConstantFP::getNullValue(Op1->getType()));
return &I;
}
Fragment N36 : Confusion variable
struct Status {
unsigned Mask;
unsigned Mode;
Status() : Mask(0), Mode(0){};
Status(unsigned Mask, unsigned Mode) : Mask(Mask), Mode(Mode) {
Mode &= Mask;
};
....
};
Avertissement PVS-Studio :
Il est très dangereux de donner aux arguments d’une fonction les mêmes noms qu’aux membres de la classe. Il est très facile de se tromper. Nous avons justement un cas de ce genre devant nous. Cette expression n'a pas de sens :
Mode &= Mask;
L'argument de la fonction change. C'est tout. Cet argument n'est plus utilisé. Très probablement, vous auriez dû l'écrire comme ceci :
Status(unsigned Mask, unsigned Mode) : Mask(Mask), Mode(Mode) {
this->Mode &= Mask;
};
Fragment N37 : Confusion variable
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;
}
Attention PVS-Studio : V1001 [CWE-563] La variable 'Taille' est affectée mais n'est pas utilisée à la fin de la fonction. Objet.cpp 424
La situation est similaire à la précédente. Il faut écrire :
this->Size += this->EntrySize;
Fragment N38-N47 : Ils ont oublié de vérifier l'index
Précédemment, nous avons examiné des exemples de déclenchement de diagnostic
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()); // <=
....
}
Avertissement PVS-Studio : V1004 [CWE-476] Le pointeur 'Ptr' a été utilisé de manière non sécurisée après avoir été vérifié par rapport à nullptr. Vérifiez les lignes : 729, 738. TargetTransformInfoImpl.h 738
Variable Ptr peut être égal nullptr, comme en témoigne le chèque :
if (Ptr != nullptr)
Cependant, en dessous ce pointeur est déréférencé sans vérification préalable :
auto PtrSizeBits = DL.getPointerTypeSizeInBits(Ptr->getType());
Considérons un autre cas similaire.
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(); // <=
....
}
Avertissement PVS-Studio : V1004 [CWE-476] Le pointeur 'FD' a été utilisé de manière dangereuse après avoir été vérifié par rapport à nullptr. Vérifiez les lignes : 3228, 3231. CGDebugInfo.cpp 3231
Faites attention au signe FD. Je suis sûr que le problème est clairement visible et qu'aucune explication particulière n'est requise.
Et plus:
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()); // <=
....
}
Avertissement PVS-Studio : V1004 [CWE-476] Le pointeur 'PtrTy' a été utilisé de manière non sécurisée après avoir été vérifié par rapport à nullptr. Vérifiez les lignes : 960, 965. InterleavedLoadCombinePass.cpp 965
Comment se protéger de telles erreurs ? Soyez plus attentif sur Code-Review et utilisez l'analyseur statique PVS-Studio pour vérifier régulièrement votre code.
Cela n'a aucun sens de citer d'autres fragments de code contenant des erreurs de ce type. Je ne laisserai qu'une liste d'avertissements dans l'article :
- V1004 [CWE-476] Le pointeur 'Expr' a été utilisé de manière non sécurisée après avoir été vérifié par rapport à nullptr. Lignes de contrôle : 1049, 1078. DebugInfoMetadata.cpp 1078
- V1004 [CWE-476] Le pointeur 'PI' a été utilisé de manière non sécurisée après avoir été vérifié par rapport à nullptr. Vérifiez les lignes : 733, 753. LegacyPassManager.cpp 753
- V1004 [CWE-476] Le pointeur 'StatepointCall' a été utilisé de manière non sécurisée après avoir été vérifié par rapport à nullptr. Lignes de contrôle : 4371, 4379. Verifier.cpp 4379
- V1004 [CWE-476] Le pointeur 'RV' a été utilisé de manière non sécurisée après avoir été vérifié par rapport à nullptr. Lignes de contrôle : 2263, 2268. TTGParser.cpp 2268
- V1004 [CWE-476] Le pointeur 'CalleeFn' a été utilisé de manière non sécurisée après avoir été vérifié par rapport à nullptr. Vérifiez les lignes : 1081, 1096. SimplifyLibCalls.cpp 1096
- V1004 [CWE-476] Le pointeur 'TC' a été utilisé de manière non sécurisée après avoir été vérifié par rapport à nullptr. Lignes de contrôle : 1819, 1824. Driver.cpp 1824
Fragment N48-N60 : Pas critique, mais un défaut (fuite mémoire possible)
std::unique_ptr<IRMutator> createISelMutator() {
....
std::vector<std::unique_ptr<IRMutationStrategy>> Strategies;
Strategies.emplace_back(
new InjectorIRStrategy(InjectorIRStrategy::getDefaultOps()));
....
}
Avertissement PVS-Studio :
Pour ajouter un élément à la fin d'un conteneur comme std :: vecteur > tu ne peux pas simplement écrire xxx.push_back (nouveau X), puisqu'il n'y a pas de conversion implicite de X* в std :: unique_ptr.
Une solution courante consiste à écrire xxx.emplace_back(nouveau X)puisqu'il compile : méthode replace_back construit un élément directement à partir de ses arguments et peut donc utiliser des constructeurs explicites.
Ce n'est pas sûr. Si le vecteur est plein, la mémoire est réaffectée. L'opération de réallocation de mémoire peut échouer, entraînant la levée d'une exception std :: bad_alloc. Dans ce cas, le pointeur sera perdu et l'objet créé ne sera jamais supprimé.
Une solution sûre consiste à créer unique_ptrqui détiendra le pointeur avant que le vecteur ne tente de réallouer la mémoire :
xxx.push_back(std::unique_ptr<X>(new X))
Depuis C++14, vous pouvez utiliser 'std::make_unique' :
xxx.push_back(std::make_unique<X>())
Ce type de défaut n'est pas critique pour LLVM. Si la mémoire ne peut pas être allouée, le compilateur s'arrêtera simplement. Cependant, pour les applications à longue durée
Ainsi, bien que ce code ne constitue pas une menace pratique pour LLVM, j'ai trouvé utile de parler de ce modèle d'erreur et du fait que l'analyseur PVS-Studio a appris à l'identifier.
Autres avertissements de ce type :
- V1023 [CWE-460] Un pointeur sans propriétaire est ajouté au conteneur 'Passes' par la méthode 'emplace_back'. Une fuite de mémoire se produira en cas d'exception. PassManager.h 546
- V1023 [CWE-460] Un pointeur sans propriétaire est ajouté au conteneur 'AAs' par la méthode 'emplace_back'. Une fuite de mémoire se produira en cas d'exception. AliasAnalysis.h 324
- V1023 [CWE-460] Un pointeur sans propriétaire est ajouté au conteneur 'Entries' par la méthode 'emplace_back'. Une fuite de mémoire se produira en cas d'exception. DWARFDebugFrame.cpp 519
- V1023 [CWE-460] Un pointeur sans propriétaire est ajouté au conteneur 'AllEdges' par la méthode 'emplace_back'. Une fuite de mémoire se produira en cas d'exception. CFGMST.h 268
- V1023 [CWE-460] Un pointeur sans propriétaire est ajouté au conteneur 'VMaps' par la méthode 'emplace_back'. Une fuite de mémoire se produira en cas d'exception. SimpleLoopUnswitch.cpp 2012
- V1023 [CWE-460] Un pointeur sans propriétaire est ajouté au conteneur 'Records' par la méthode 'emplace_back'. Une fuite de mémoire se produira en cas d'exception. FDRLogBuilder.h 30
- V1023 [CWE-460] Un pointeur sans propriétaire est ajouté au conteneur 'PendingSubmodules' par la méthode 'emplace_back'. Une fuite de mémoire se produira en cas d'exception. ModuleMap.cpp 810
- V1023 [CWE-460] Un pointeur sans propriétaire est ajouté au conteneur 'Objects' par la méthode 'emplace_back'. Une fuite de mémoire se produira en cas d'exception. DebugMap.cpp 88
- V1023 [CWE-460] Un pointeur sans propriétaire est ajouté au conteneur 'Stratégies' par la méthode 'emplace_back'. Une fuite de mémoire se produira en cas d'exception. llvm-isel-fuzzer.cpp 60
- V1023 [CWE-460] Un pointeur sans propriétaire est ajouté au conteneur 'Modifiers' par la méthode 'emplace_back'. Une fuite de mémoire se produira en cas d'exception. llvm-stress.cpp 685
- V1023 [CWE-460] Un pointeur sans propriétaire est ajouté au conteneur 'Modifiers' par la méthode 'emplace_back'. Une fuite de mémoire se produira en cas d'exception. llvm-stress.cpp 686
- V1023 [CWE-460] Un pointeur sans propriétaire est ajouté au conteneur 'Modifiers' par la méthode 'emplace_back'. Une fuite de mémoire se produira en cas d'exception. llvm-stress.cpp 688
- V1023 [CWE-460] Un pointeur sans propriétaire est ajouté au conteneur 'Modifiers' par la méthode 'emplace_back'. Une fuite de mémoire se produira en cas d'exception. llvm-stress.cpp 689
- V1023 [CWE-460] Un pointeur sans propriétaire est ajouté au conteneur 'Modifiers' par la méthode 'emplace_back'. Une fuite de mémoire se produira en cas d'exception. llvm-stress.cpp 690
- V1023 [CWE-460] Un pointeur sans propriétaire est ajouté au conteneur 'Modifiers' par la méthode 'emplace_back'. Une fuite de mémoire se produira en cas d'exception. llvm-stress.cpp 691
- V1023 [CWE-460] Un pointeur sans propriétaire est ajouté au conteneur 'Modifiers' par la méthode 'emplace_back'. Une fuite de mémoire se produira en cas d'exception. llvm-stress.cpp 692
- V1023 [CWE-460] Un pointeur sans propriétaire est ajouté au conteneur 'Modifiers' par la méthode 'emplace_back'. Une fuite de mémoire se produira en cas d'exception. llvm-stress.cpp 693
- V1023 [CWE-460] Un pointeur sans propriétaire est ajouté au conteneur 'Modifiers' par la méthode 'emplace_back'. Une fuite de mémoire se produira en cas d'exception. llvm-stress.cpp 694
- V1023 [CWE-460] Un pointeur sans propriétaire est ajouté au conteneur 'Opérandes' par la méthode 'emplace_back'. Une fuite de mémoire se produira en cas d'exception. GlobalISelEmitter.cpp 1911
- V1023 [CWE-460] Un pointeur sans propriétaire est ajouté au conteneur 'Stash' par la méthode 'emplace_back'. Une fuite de mémoire se produira en cas d'exception. GlobalISelEmitter.cpp 2100
- V1023 [CWE-460] Un pointeur sans propriétaire est ajouté au conteneur 'Matchers' par la méthode 'emplace_back'. Une fuite de mémoire se produira en cas d'exception. GlobalISelEmitter.cpp 2702
Conclusion
J'ai émis 60 avertissements au total, puis j'ai arrêté. Existe-t-il d'autres défauts détectés par l'analyseur PVS-Studio dans LLVM ? Oui j'ai. Cependant, lorsque j'écrivais des fragments de code pour l'article, il était tard dans la soirée, ou plutôt même dans la nuit, et j'ai décidé qu'il était temps d'en finir avec cette journée.
J'espère que vous l'avez trouvé intéressant et que vous voudrez essayer l'analyseur PVS-Studio.
Vous pouvez télécharger l'analyseur et obtenir la clé du dragueur de mines sur
Plus important encore, utilisez régulièrement l’analyse statique. Chèques uniques, réalisés par nos soins afin de vulgariser la méthodologie de l'analyse statique et PVS-Studio ne sont pas un scénario normal.
Bonne chance pour améliorer la qualité et la fiabilité de votre code !
Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien de traduction : Andrey Karpov.
Source: habr.com