Geekly articles weekly

Recherche de bogues dans LLVM 8 avec PVS-Studio

PVS-Studio et LLVM 8.0.0

Cela fait deux ans que nous n'avons pas vérifié le code du projet LLVM avec PVS-Studio, alors voyons si PVS-Studio est toujours le leader parmi les outils pour détecter les bugs et les failles de sécurité. Nous le ferons en analysant la version LLVM 8.0.0 pour détecter de nouveaux bogues.

L'article qui doit être écrit


Franchement, je n'avais pas envie d'écrire cet article. Ce n'est pas très amusant de parler du projet que nous avons déjà vérifié plus d'une fois ( 1 , 2 , 3 ). Je préférerais quelque chose de nouveau à la place, mais je n'avais pas le choix.

Chaque fois qu'une nouvelle version de LLVM est lancée ou que Clang Static Analyzer est mis à jour, nous recevons des e-mails lisant ces lignes:

Hé, la nouvelle version de Clang Static Analyzer a obtenu de nouveaux diagnostics! PVS-Studio semble devenir moins pertinent. Clang peut détecter plus de bugs qu'auparavant et rattrape maintenant PVS-Studio. Qu'avez-vous dit?

À cela, je répondrais avec plaisir:

Nous n'avons pas paressé non plus! Nous avons considérablement augmenté les capacités de PVS-Studio, donc pas de soucis - nous sommes toujours les meilleurs.

Mais c'est une mauvaise réponse, je le crains. Il n'offre aucune preuve, et c'est la raison pour laquelle j'écris cet article. J'ai donc vérifié LLVM une fois de plus et trouvé des tonnes de bugs de toutes sortes. Ceux que j'aimais le plus seront discutés plus loin. Clang Static Analyzer ne peut pas détecter ces bogues (ou rend le processus très gênant) - et nous le pouvons. Et, en passant, il ne m'a fallu qu'une seule soirée pour écrire tous ces bugs.

Cependant, l'article m'a pris plusieurs semaines. Je ne pouvais tout simplement pas me résoudre à mettre le matériel rassemblé dans le texte :).

Soit dit en passant, si vous vous demandez quelles techniques PVS-Studio utilise pour détecter les bogues et les vulnérabilités, jetez un œil à ce billet .

Diagnostics nouveaux et existants


Comme je l'ai déjà dit, la dernière des nombreuses vérifications de LLVM a été effectuée il y a deux ans, et les bogues trouvés à l'époque ont été corrigés par les auteurs. Cet article montrera une nouvelle portion d'erreurs. Comment se fait-il qu'il y ait de nouveaux bugs? Il y a trois raisons:

  1. Le projet LLVM évolue; les auteurs modifient le code existant et ajoutent un nouveau code. Les parties modifiées et nouvelles ont naturellement de nouveaux bugs. Ce fait est un argument solide pour exécuter une analyse statique régulièrement plutôt que de temps en temps. Le format de nos articles est parfait pour présenter les capacités de PVS-Studio, mais cela n'a rien à voir avec l'amélioration de la qualité du code ou la réduction des bogues moins coûteuse. Utilisez régulièrement l'analyse statique!
  2. Nous modifions et améliorons les diagnostics existants, permettant à l'analyseur de détecter les bogues qu'il n'a pas pu détecter auparavant.
  3. PVS-Studio a été amélioré avec de nouveaux diagnostics, qui n'existaient pas il y a deux ans. J'ai regroupé ces avertissements dans une section distincte afin que la progression de PVS-Studio soit vue plus distinctement.

Défauts détectés par les diagnostics existants


Extrait no. 1: 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 .... }

Message de diagnostic PVS-Studio: V501 [CWE-570] Il existe des sous-expressions identiques 'Name.startswith ("avx512.mask.permvar.")' À gauche et à droite de '||' opérateur. AutoUpgrade.cpp 73

L'occurrence du "avx512.mask.permvar". la sous-chaîne est vérifiée deux fois. La deuxième condition était évidemment de vérifier autre chose, mais le programmeur a oublié de changer la ligne copiée.

Extrait no. 2: 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; .... }

Message de diagnostic PVS-Studio: V501 Il existe des sous-expressions identiques «CXNameRange_WantQualifier» à gauche et à droite de «|» opérateur. CIndex.cpp 7245

La constante nommée CXNameRange_WantQualifier est utilisée deux fois en raison d'une faute de frappe.

Extrait no. 3: Confusion sur la priorité de l'opérateur

 int PPCTTIImpl::getVectorInstrCost(unsigned Opcode, Type *Val, unsigned Index) { .... if (ISD == ISD::EXTRACT_VECTOR_ELT && Index == ST->isLittleEndian() ? 1 : 0) return 0; .... }

Message de diagnostic PVS-Studio: V502 [CWE-783] L'opérateur '?:' Fonctionne peut -être d'une manière différente de celle attendue. L'opérateur '?:' A une priorité inférieure à l'opérateur '=='. PPCTargetTransformInfo.cpp 404

Je trouve ce bug très mignon. Oui, je sais que j'ai un goût étrange :).

Comme dicté par la priorité de l'opérateur , l'expression d'origine est évaluée comme suit:

 (ISD == ISD::EXTRACT_VECTOR_ELT && (Index == ST->isLittleEndian())) ? 1 : 0

D'un point de vue pratique, cependant, cette condition n'a pas de sens car elle peut être réduite à:

 (ISD == ISD::EXTRACT_VECTOR_ELT && Index == ST->isLittleEndian())

C'est évidemment un bug. Ce devait être la variable Index que le programmeur voulait vérifier pour 0/1. Pour corriger le code, l'opérateur ternaire doit être placé entre parenthèses:

 if (ISD == ISD::EXTRACT_VECTOR_ELT && Index == (ST->isLittleEndian() ? 1 : 0))

L'opérateur ternaire est en fait très délicat et peut conduire à des erreurs logiques. Utilisez-le soigneusement et n'hésitez pas à mettre des parenthèses supplémentaires autour de lui. Ce sujet est abordé plus en détail ici , dans la section "Attention à l'opérateur?: Et mettez-le entre parenthèses".

Extraits non. 4, 5: 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; } .... }

Message de diagnostic PVS-Studio: V522 [CWE-476] Le déréférencement du pointeur nul "LHS" peut avoir lieu. TGParser.cpp 2152

Si le pointeur LHS se trouve être nul, le programme devrait générer un avertissement. Au lieu de cela, il déréférencera ce pointeur très nul: LHS-> getAsString () .

C'est une situation assez typique pour les gestionnaires d'erreurs de contenir des bogues car les développeurs ne les testent pas correctement. Les analyseurs statiques vérifient tout le code accessible, quelle que soit la fréquence à laquelle il est réellement exécuté. C'est un bon exemple de la façon dont l'analyse statique complète d'autres moyens de test et de protection du code.

Un gestionnaire défectueux similaire pour le pointeur RHS se trouve un peu plus loin: V522 [CWE-476] Le déréférencement du pointeur nul 'RHS' pourrait avoir lieu. TGParser.cpp 2186

Extrait no. 6: Utilisation d'un 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); } .... }

Message de diagnostic PVS-Studio: V522 [CWE-476] Le déréférencement du pointeur nul 'ProgClone' peut avoir lieu. Miscompilation.cpp 601

Le pointeur intelligent ProgClone libère d'abord la propriété de l'objet:

 BD.setNewProgram(std::move(ProgClone));

En fait, ProgClone est devenu un pointeur nul - donc, techniquement, un pointeur nul est déréférencé un peu plus loin:

 Function *NewF = ProgClone->getFunction(MisCompFunctions[i].first);

Mais cela n'arrivera pas! Notez que la boucle ne s'exécute pas du tout.

Le conteneur MiscompiledFunctions est d'abord effacé:

 MiscompiledFunctions.clear();

Et puis sa taille est utilisée dans la condition de boucle:

 for (unsigned i = 0, e = MisCompFunctions.size(); i != e; ++i) {

De toute évidence, la boucle ne démarre tout simplement pas. Je pense que c'est aussi un bug, et le code était censé avoir une apparence différente.

Je suppose que ce que nous voyons ici est cette parité d'erreur notoire, où un bug agit comme un déguisement pour un autre :).

Extrait no. 7: Utilisation d'un 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; } .... }

Message de diagnostic PVS-Studio: V522 [CWE-476] Le déréférencement du pointeur nul "Test" peut avoir lieu. Miscompilation.cpp 709

Celui-ci est similaire au cas précédent. Le contenu de l'objet est d'abord déplacé puis utilisé comme si de rien n'était. Cette erreur est devenue de plus en plus courante après l'ajout de la sémantique de déplacement à C ++. C'est ce que j'aime dans cette langue! Vous avez de nouvelles façons de vous tirer une balle dans le pied, ce qui signifie que PVS-Studio aura toujours du travail à faire :).

Extrait no. 8: 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); }

Message de diagnostic PVS-Studio: V522 [CWE-476] Le déréférencement du pointeur nul "Type" peut avoir lieu. PrettyFunctionDumper.cpp 233

Tout comme les gestionnaires d'erreurs, les fonctions de test qui impriment des données de débogage n'ont généralement pas non plus une couverture de test adéquate, et ceci en est un exemple. Au lieu d'aider l'utilisateur à résoudre ses problèmes, la fonction attend qu'il soit résolu.

Code fixe:

 if (Type) Type->dump(*this); else Printer << "<unknown-type>";

Extrait no. 9: 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()); .... }

Message de diagnostic PVS-Studio: V522 [CWE-476] Le déréférencement du pointeur nul «Ty» peut avoir lieu. SearchableTableEmitter.cpp 614

Je ne pense pas que vous ayez besoin de commentaires à ce sujet.

Extrait no. 10: 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; }

Message de diagnostic PVS-Studio: V570 La variable 'Identifier-> Type' est affectée à elle-même. FormatTokenLexer.cpp 249

L'attribution d'une variable à elle-même est une opération dénuée de sens. Le programmeur doit avoir voulu faire ce qui suit:

 Identifier->Type = Question->Type;

Extrait no. 11: Pause 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; .... }

Message de diagnostic PVS-Studio: V622 [CWE-478] Envisagez d'inspecter l'instruction 'switch'. Il est possible que le premier opérateur «case» soit manquant. SystemZAsmParser.cpp 652

Il y a une déclaration de rupture très suspecte au début. Ne devrait-il pas y avoir autre chose ici?

Extrait no. 12: 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"); .... }

Message de diagnostic PVS-Studio: V595 [CWE-476] Le pointeur "Appelé" a été utilisé avant d'être vérifié par rapport à nullptr. Vérifier les lignes: 172, 174. AMDGPUInline.cpp 172

Le pointeur de l' appelé est d'abord déréférencé lorsque la fonction getTTI est appelée.

Et puis il s'avère que le pointeur doit être vérifié pour nullptr :

 if (!Callee || Callee->isDeclaration())

Trop tard ...

Extraits non. 13 - Non ....: Vérification d'un pointeur après déréférencement

L'exemple précédent n'est pas unique. Le même problème se trouve dans cet extrait:

 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()) { // <= .... }

Message de diagnostic 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 celui-ci:

 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()); // <= .... }

Message de diagnostic 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. DWARFFormValue.cpp 404
  • V595 [CWE-476] Le pointeur 'ND' a été utilisé avant d'être vérifié par rapport à nullptr. Vérifiez les lignes: 2149, 2151. SemaTemplateInstantiate.cpp 2149

Ensuite, je me suis désintéressé du suivi des avertissements V595, donc je ne peux pas vous dire s'il existe d'autres bogues de ce type en plus de ceux indiqués ci-dessus. Je parie qu'il y en a.

Extraits non. 17, 18: Changement suspect

 static inline bool processLogicalImmediate(uint64_t Imm, unsigned RegSize, uint64_t &Encoding) { .... unsigned Size = RegSize; .... uint64_t NImms = ~(Size-1) << 1; .... }

Message de diagnostic PVS-Studio: V629 [CWE-190] Envisagez d'inspecter l'expression '~ (Taille - 1) << 1'. Décalage binaire de la valeur 32 bits avec une extension ultérieure au type 64 bits. AArch64AddressingModes.h 260

Ce code peut être correct, mais il semble étrange et doit être examiné.

Supposons que la variable Size ait la valeur 16; alors la variable NImms devrait obtenir la valeur suivante:

11111111111111111111111111111111111111111111111111111111111111100000000

Mais en réalité, il obtiendra la valeur:

0000000000000000000000000000000000001111111111111111111111111111100000

Cela se produit car tous les calculs sont effectués sur le type non signé 32 bits, et ce n'est qu'ensuite qu'il est implicitement promu en uint64_t , avec les bits les plus significatifs mis à zéro.

Le problème peut être résolu comme suit:

 uint64_t NImms = ~static_cast<uint64_t>(Size-1) << 1;

Un autre bogue de ce type: V629 [CWE-190] Envisagez d'inspecter l'expression 'Immr << 6'. Décalage binaire de la valeur 32 bits avec une extension ultérieure au type 64 bits. AArch64AddressingModes.h 269

Extrait no. 19: 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"); } .... }

Message de diagnostic PVS-Studio: V646 [CWE-670] Envisagez d'inspecter la logique de l'application. Il est possible que le mot clé "else" soit manquant. AMDGPUAsmParser.cpp 5655

Celui-ci n'est pas un bug. Étant donné que le bloc then de la première instruction if se termine par continue , peu importe qu'il ait ou non le mot-clé else . Le comportement sera le même dans tous les cas. Cependant, le reste manquant rend le code moins lisible et, par conséquent, potentiellement dangereux. Si continue disparaît un jour, le comportement changera radicalement. Je recommande fortement d'ajouter autre chose .

Extrait no. 20: Quatre fautes de frappe identiques

 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; }

Messages de diagnostic 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 ()». Symbol.cpp 32
  • V655 [CWE-480] Les chaînes ont été concaténées mais ne sont pas utilisées. Pensez à inspecter l'expression 'Résultat + "(Classe ObjC)" + Name.str ()'. Symbol.cpp 35
  • V655 [CWE-480] Les chaînes ont été concaténées mais ne sont pas utilisées. Pensez à inspecter l'expression 'Résultat + "(ObjC Classe EH)" + Name.str ()'. Symbol.cpp 38
  • V655 [CWE-480] Les chaînes ont été concaténées mais ne sont pas utilisées. Pensez à inspecter l'expression 'Résultat + "(ObjC IVar)" + Name.str ()'. Symbol.cpp 41

Le programmeur a accidentellement utilisé l'opérateur + au lieu de + = et s'est retrouvé avec quatre constructions dénuées de sens.

Extrait no. 21: Comportement indé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 d'abord de repérer le bug par vous-même. J'ai ajouté l'image pour ne pas jeter un œil à la réponse tout de suite:

???


Message de diagnostic PVS-Studio: V708 [CWE-758] Une construction dangereuse est utilisée: 'FeaturesMap [Op] = FeaturesMap.size ()', où 'FeaturesMap' est de la classe 'map'. Cela peut conduire à un comportement indéfini. RISCVCompressInstEmitter.cpp 490

La ligne défectueuse est celle-ci:

 FeaturesMap[Op] = FeaturesMap.size();

Si l'élément Op n'a pas été trouvé, le programme crée un nouvel élément dans la carte et lui attribue le nombre total d'éléments dans cette carte. Vous ne savez tout simplement pas si la fonction size sera appelée avant ou après l'ajout du nouvel élément.

Extraits non. 22 - Non. 24: affectations en double

 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; } .... }

Message de diagnostic PVS-Studio: V519 [CWE-563] La variable 'NType' reçoit des valeurs successives deux fois. C'est peut-être une erreur. Vérifiez les lignes: 1663, 1664. MachOObjectFile.cpp 1664

Je ne pense pas que ce soit une vraie erreur - plutôt une tâche en double. Mais c'est toujours un défaut.

Deux autres cas:

  • V519 [CWE-563] La variable 'B.NDesc' reçoit des valeurs successives deux fois. C'est peut-être une erreur. Vérifiez les lignes: 1488, 1489. llvm-nm.cpp 1489
  • V519 [CWE-563] La variable se voit attribuer des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 59, 61. coff2yaml.cpp 61

Extraits non. 25 - Non. 27: Plus d'affectations en double

Ceux-ci traitent de versions légèrement différentes des affectations en double.

 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; .... }

Message de diagnostic PVS-Studio: V519 [CWE-563] La variable 'Alignement' se voit attribuer des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 1158, 1160. LoadStoreVectorizer.cpp 1160

Il s'agit d'un extrait très étrange, et il contient probablement une erreur logique. La variable d' alignement se voit d'abord attribuer la valeur en fonction de la condition, puis elle lui est attribuée à nouveau, mais sans aucun contrôle préalable.

Défauts similaires:

  • V519 [CWE-563] La variable 'Effets' se voit attribuer des valeurs deux fois de suite. C'est peut-être une erreur. Vérifiez les lignes: 152, 165. WebAssemblyRegStackify.cpp 165
  • V519 [CWE-563] La variable 'ExpectNoDerefChunk' reçoit des valeurs successives deux fois. C'est peut-être une erreur. Vérifiez les lignes: 4970, 4973. SemaType.cpp 4973

Extrait no. 28: 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; } .... }

Message de diagnostic PVS-Studio: V547 [CWE-571] L'expression 'nextByte! = 0x90' est toujours vraie. X86DisassemblerDecoder.cpp 379

Le chèque n'a pas de sens. La variable nextByte n'est jamais égale à 0x90 : elle découle logiquement de la vérification précédente. Ce doit être une erreur logique.

Extraits non. 29 - Non ....: conditions toujours vraies / fausses

Il existe de nombreux avertissements indiquant qu'une condition entière ( V547 ) ou une partie d'une condition ( V560 ) est toujours vraie ou fausse. Plutôt que de véritables bogues, il s'agit souvent simplement de mauvais code, des effets de l'expansion des macros, etc. Cela dit, tous ces avertissements doivent toujours être vérifiés car certains d'entre eux peuvent signaler de véritables erreurs logiques. Par exemple, l'extrait de code suivant ne semble pas correct:

 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; .... }

Message de diagnostic PVS-Studio: V560 [CWE-570] Une partie de l'expression conditionnelle est toujours fausse: RegNo == 0xe. ARMDisassembler.cpp 939

La constante 0xE est le nombre décimal 14. La vérification RegNo == 0xe n'a pas de sens car si RegNo> 13 , la fonction reviendra.

J'ai vu beaucoup d'autres avertissements V547 et V560, mais, comme avec V595 , je ne me sentais pas enthousiaste à l'idée de les vérifier car j'avais déjà assez de matériel pour un article :). Donc, pas de chiffres pour le nombre total de bogues de ce type dans LLVM.

Voici un exemple pour illustrer pourquoi la vérification de ces avertissements est ennuyeuse. L'analyseur est totalement correct lors de l'émission d'un avertissement sur le code suivant. Mais ce n'est toujours pas un bug.

 bool UnwrappedLineParser::parseBracedList(bool ContinueOnSemicolons, tok::TokenKind ClosingBraceKind) { bool HasError = false; .... HasError = true; if (!ContinueOnSemicolons) return !HasError; .... }

Message de diagnostic PVS-Studio: V547 [CWE-570] L'expression '! HasError' est toujours fausse. UnwrappedLineParser.cpp 1635

Extrait no. 30: 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(); } .... }

Message de diagnostic PVS-Studio: V612 [CWE-670] Un «retour» inconditionnel dans une boucle. R600OptimizeVectorRegisters.cpp 63

Il s'agit soit d'un bug, soit d'une technique de codage spécifique destinée à communiquer une idée aux autres programmeurs. Pour moi, cela ne dit rien, sauf que c'est un morceau de code très suspect. Veuillez ne pas écrire de code comme ça :).

Vous vous sentez fatigué? OK, il est temps de faire du thé ou du café.

café


Défauts découverts par de nouveaux diagnostics


Je pense que 30 exemples suffisent pour les diagnostics existants. Voyons maintenant si nous pouvons trouver quelque chose d'intéressant avec les nouveaux diagnostics, qui ont été ajoutés après la vérification précédente . Au cours des deux dernières années, le module d'analyseur C ++ a été étendu avec 66 nouveaux diagnostics.

Extrait no. 31: 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(); }

Message de diagnostic PVS-Studio: V779 [CWE-561] Code inaccessible détecté. Il est possible qu'une erreur soit présente. ExecutionUtils.cpp 146

Comme vous pouvez le voir, les deux branches de l'instruction if se terminent par une instruction return , ce qui signifie que le conteneur CtorDtorsByPriority ne sera jamais effacé.

Extrait no. 32: 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; }

Message de diagnostic PVS-Studio: V779 [CWE-561] Code inaccessible détecté. Il est possible qu'une erreur soit présente. LLParser.cpp 835

Celui-ci est intéressant. Jetez d'abord un œil à cette partie:

 return ParseTypeIdEntry(SummaryID); break;

Il ne semble y avoir rien d'étrange dans ce code; l'instruction break n'est pas nécessaire et peut être supprimée en toute sécurité. Mais ce n'est pas si simple.

L'avertissement est déclenché par les lignes suivantes:

 Lex.setIgnoreColonInIdentifiers(false); return false;

En effet, ce code est inaccessible. Toutes les étiquettes de cas de l'instruction switch se terminent par un retour , et la pause solitaire sans signification ne semble plus aussi inoffensive! Et si l'une des branches devait se terminer par une pause plutôt que par un retour ?

Extrait no. 33: Suppression accidentelle des bits les plus significatifs

 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); .... }

Message de diagnostic PVS-Studio: V784 La taille du masque de bits est inférieure à la taille du premier opérande. Cela entraînera la perte de bits supérieurs. RuntimeDyld.cpp 815

Notez que la fonction getStubAlignment renvoie une valeur non signée . Voyons comment l'expression va évaluer, en supposant que la fonction retournera la valeur 8:

~ (getStubAlignment () - 1)

~ (8u-1)

0xFFFFFFF8u

Notez maintenant que le type de la variable DataSize est non signé 64 bits. Il s'avère donc que l'exécution de l'opération DataSize & 0xFFFFFFF8 entraînera la suppression des 32 bits les plus significatifs de la valeur. Je ne pense pas que le programmeur ait voulu ça. Peut-être qu'ils voulaient que ce soit DataSize et 0xFFFFFFFFFFFFFFFFF8u.

Pour corriger l'erreur, le code doit être réécrit comme ceci:

 DataSize &= ~(static_cast<uint64_t>(getStubAlignment()) - 1);

Ou comme ça:

 DataSize &= ~(getStubAlignment() - 1ULL);

Extrait no. 34: Mauvaise 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); .... }

Message de diagnostic PVS-Studio: V1028 [CWE-190] Débordement possible. Envisagez de transposer les opérandes de l'opérateur 'NumElts * Scale' en type 'size_t', pas le résultat. X86ISelLowering.h 1577

Une conversion de type explicite est utilisée pour éviter un débordement lors de la multiplication de variables de type int . Dans ce cas, cependant, cela ne fonctionne pas car la multiplication se produira en premier et ce n'est qu'ensuite que le résultat 32 bits sera promu au type size_t .

Extrait no. 35: Copier-coller incorrect

 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; } .... }

V778 [CWE-682] Deux fragments de code similaires ont été trouvés. Il s'agit peut-être d'une faute de frappe et la variable «Op1» devrait être utilisée à la place de «Op0». InstCombineCompares.cpp 5507

Ce nouveau diagnostic cool détecte les situations où un fragment de code est écrit à l'aide de copier-coller, avec tous les noms modifiés sauf un.

Notez que tous les Op0 sauf un ont été remplacés par Op1 dans le deuxième bloc. Le code devrait probablement ressembler à ceci:

 if (!match(Op1, m_PosZeroFP()) && isKnownNeverNaN(Op1, &TLI)) { I.setOperand(1, ConstantFP::getNullValue(Op1->getType())); return &I; }

Extrait no. 36: Variables mélangées

 struct Status { unsigned Mask; unsigned Mode; Status() : Mask(0), Mode(0){}; Status(unsigned Mask, unsigned Mode) : Mask(Mask), Mode(Mode) { Mode &= Mask; }; .... };

Message de diagnostic PVS-Studio: V1001 [CWE-563] La variable 'Mode' est affectée mais n'est pas utilisée à la fin de la fonction. SIModeRegister.cpp 48

Il est très dangereux d'avoir les mêmes noms pour les arguments de fonction que pour les membres de la classe car vous risquez de les mélanger. Ce que vous voyez ici en est un exemple. L'expression suivante n'a pas de sens:

 Mode &= Mask;

L'argument est modifié mais n'est plus utilisé par la suite. Cet extrait devrait probablement ressembler à ceci:

 Status(unsigned Mask, unsigned Mode) : Mask(Mask), Mode(Mode) { this->Mode &= Mask; };

Extrait no. 37: Variables mélangées

 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; }

Message de diagnostic PVS-Studio: V1001 [CWE-563] La variable 'Taille' est affectée mais n'est pas utilisée à la fin de la fonction. Object.cpp 424

Celui-ci est similaire à l'exemple précédent. Version correcte:

 this->Size += this->EntrySize;

Extraits non. 38 - Non. 47: Vérification du pointeur manquante

Nous avons examiné quelques exemples de l'avertissement V595 un peu plus tôt. Ce qu'il détecte, c'est une situation où un pointeur est d'abord déréférencé et ensuite vérifié. Le nouveau diagnostic V1004 est l'opposé de cela, et il détecte également des tonnes d'erreurs. Il recherche les pointeurs déjà testés qui ne sont pas testés à nouveau lorsque cela est nécessaire. Voici quelques erreurs de ce type trouvées dans le code LLVM.

 int getGEPCost(Type *PointeeType, const Value *Ptr, ArrayRef<const Value *> Operands) { .... if (Ptr != nullptr) { // <= assert(....); BaseGV = dyn_cast<GlobalValue>(Ptr->stripPointerCasts()); } bool HasBaseReg = (BaseGV == nullptr); auto PtrSizeBits = DL.getPointerTypeSizeInBits(Ptr->getType()); // <= .... }

Message de diagnostic 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

Ptr peut être nullptr , ce qui est indiqué par la vérification:

 if (Ptr != nullptr)

Cependant, le même pointeur est déréférencé sans une telle vérification un peu plus loin:

 auto PtrSizeBits = DL.getPointerTypeSizeInBits(Ptr->getType());

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(); // <= .... }

Message de diagnostic PVS-Studio: V1004 [CWE-476] Le pointeur 'FD' a été utilisé de manière non sécurisée après avoir été vérifié par rapport à nullptr. Vérifiez les lignes: 3228, 3231. CGDebugInfo.cpp 3231

Notez le pointeur FD . Cette erreur est simple, donc pas de commentaires sur celui-ci.

Un de plus ici:

 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()); // <= .... }

Message de diagnostic 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 éviter des erreurs comme ça? Soyez très prudent lorsque vous examinez votre code et vérifiez-le régulièrement avec PVS-Studio.

Je ne pense pas que nous devrions examiner d'autres exemples de ce type, alors voici juste une liste des avertissements:
  • 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. Vérifiez les lignes: 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. Vérifiez les lignes: 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. Vérifiez les lignes: 2263, 2268. TGParser.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. Vérifiez les lignes: 1819, 1824. Driver.cpp 1824

Extraits non. 48 - Non. 60: Pas critique mais toujours un défaut (fuite de mémoire potentielle)

 std::unique_ptr<IRMutator> createISelMutator() { .... std::vector<std::unique_ptr<IRMutationStrategy>> Strategies; Strategies.emplace_back( new InjectorIRStrategy(InjectorIRStrategy::getDefaultOps())); .... }

Message de diagnostic PVS-Studio: 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 58

Vous ne pouvez pas simplement écrire xxx.push_back (nouveau X) pour ajouter un élément à un conteneur de type std :: vector <std :: unique_ptr <X>> car il n'y a pas de transtypage implicite de X * en std :: unique_ptr < X> .

La solution populaire consiste à écrire xxx.emplace_back (nouveau X) car il est compilable: la méthode emplace_back construit l'élément directement à partir des arguments et, par conséquent, peut utiliser des constructeurs explicites.

Mais cette solution n'est pas sûre. Si le vecteur est plein, la mémoire sera réallouée. Cette opération peut échouer et finir par déclencher une exception std :: bad_alloc . Dans ce cas, le pointeur sera perdu et le programme ne pourra pas supprimer l'objet créé.

Une solution plus sûre consiste à créer un unique_ptr , qui conservera le pointeur jusqu'à ce que le vecteur tente de réaffecter la mémoire:

 xxx.push_back(std::unique_ptr<X>(new X))

Le standard C ++ 14 vous permet d'utiliser 'std :: make_unique':

 xxx.push_back(std::make_unique<X>())

Ce type de défaut n'a aucun effet dans LLVM. La compilation s'arrêtera simplement si l'allocation de mémoire échoue. Cela dit, cela peut être assez critique dans les applications avec un temps de fonctionnement long, qui ne peuvent pas simplement se terminer en cas d'échec d'allocation de mémoire.

Donc, même si ce code n'est pas dangereux pour LLVM, j'ai pensé que je devrais quand même vous parler de ce modèle de bogue et du fait que PVS-Studio peut maintenant le détecter.

Autres cas similaires:

  • 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] A pointer without owner is added to the 'AAs' container by the 'emplace_back' method. A memory leak will occur in case of an exception. AliasAnalysis.h 324
  • V1023 [CWE-460] A pointer without owner is added to the 'Entries' container by the 'emplace_back' method. A memory leak will occur in case of an exception. DWARFDebugFrame.cpp 519
  • V1023 [CWE-460] A pointer without owner is added to the 'AllEdges' container by the 'emplace_back' method. A memory leak will occur in case of an exception. CFGMST.h 268
  • V1023 [CWE-460] A pointer without owner is added to the 'VMaps' container by the 'emplace_back' method. A memory leak will occur in case of an exception. SimpleLoopUnswitch.cpp 2012
  • V1023 [CWE-460] A pointer without owner is added to the 'Records' container by the 'emplace_back' method. A memory leak will occur in case of an exception. FDRLogBuilder.h 30
  • V1023 [CWE-460] A pointer without owner is added to the 'PendingSubmodules' container by the 'emplace_back' method. A memory leak will occur in case of an exception. ModuleMap.cpp 810
  • V1023 [CWE-460] A pointer without owner is added to the 'Objects' container by the 'emplace_back' method. A memory leak will occur in case of an exception. DebugMap.cpp 88
  • V1023 [CWE-460] A pointer without owner is added to the 'Strategies' container by the 'emplace_back' method. A memory leak will occur in case of an exception. llvm-isel-fuzzer.cpp 60
  • V1023 [CWE-460] A pointer without owner is added to the 'Modifiers' container by the 'emplace_back' method. A memory leak will occur in case of an exception. llvm-stress.cpp 685
  • V1023 [CWE-460] A pointer without owner is added to the 'Modifiers' container by the 'emplace_back' method. A memory leak will occur in case of an exception. llvm-stress.cpp 686
  • V1023 [CWE-460] A pointer without owner is added to the 'Modifiers' container by the 'emplace_back' method. A memory leak will occur in case of an exception. llvm-stress.cpp 688
  • V1023 [CWE-460] A pointer without owner is added to the 'Modifiers' container by the 'emplace_back' method. A memory leak will occur in case of an exception. llvm-stress.cpp 689
  • V1023 [CWE-460] A pointer without owner is added to the 'Modifiers' container by the 'emplace_back' method. A memory leak will occur in case of an exception. llvm-stress.cpp 690
  • V1023 [CWE-460] A pointer without owner is added to the 'Modifiers' container by the 'emplace_back' method. A memory leak will occur in case of an exception. llvm-stress.cpp 691
  • V1023 [CWE-460] A pointer without owner is added to the 'Modifiers' container by the 'emplace_back' method. A memory leak will occur in case of an exception. llvm-stress.cpp 692
  • V1023 [CWE-460] A pointer without owner is added to the 'Modifiers' container by the 'emplace_back' method. A memory leak will occur in case of an exception. llvm-stress.cpp 693
  • V1023 [CWE-460] A pointer without owner is added to the 'Modifiers' container by the 'emplace_back' method. A memory leak will occur in case of an exception. llvm-stress.cpp 694
  • V1023 [CWE-460] A pointer without owner is added to the 'Operands' container by the 'emplace_back' method. A memory leak will occur in case of an exception. GlobalISelEmitter.cpp 1911
  • V1023 [CWE-460] A pointer without owner is added to the 'Stash' container by the 'emplace_back' method. A memory leak will occur in case of an exception. GlobalISelEmitter.cpp 2100
  • V1023 [CWE-460] A pointer without owner is added to the 'Matchers' container by the 'emplace_back' method. A memory leak will occur in case of an exception. GlobalISelEmitter.cpp 2702

Conclusion


J'ai écrit 60 avertissements et je me suis arrêté là-dessus. PVS-Studio a-t-il trouvé d'autres bogues dans LLVM? Oui, c'est vrai. Mais alors que j'écrivais les exemples, la nuit est tombée, j'ai donc décidé de tomber.

J'espère que vous avez apprécié la lecture de cet article et cela vous a encouragé à essayer l'analyseur PVS-Studio par vous-même.

Visitez cette page pour télécharger l'analyseur et obtenir une clé d'essai.

Plus important encore, utilisez régulièrement l'analyse statique. Les vérifications ponctuelles , comme celles que nous faisons pour vulgariser l'analyse statique et promouvoir PVS-Studio, ne sont pas le scénario normal.

Bonne chance pour améliorer la qualité et la fiabilité de votre code!

Source: https://habr.com/ru/post/fr450002/

More articles:


All Articles