Geekly articles weekly

Rechercher des bogues dans LLVM 8 à l'aide de l'analyseur PVS-Studio

PVS-Studio et LLVM 8.0.0

Plus de deux ans se sont écoulés depuis la dernière vérification du code de projet LLVM à l'aide de notre analyseur PVS-Studio. Assurons-nous que l'analyseur PVS-Studio est toujours le principal outil de détection des erreurs et des vulnérabilités potentielles. Pour ce faire, vérifiez et recherchez de nouvelles erreurs dans la version LLVM 8.0.0.

Article à rédiger


Pour être honnête, je ne voulais pas écrire cet article. Il n'est pas intéressant d'écrire sur un projet que nous avons déjà testé à plusieurs reprises ( 1 , 2 , 3 ). Il vaut mieux écrire sur quelque chose de nouveau, mais je n'ai pas le choix.

Chaque fois qu'une nouvelle version de LLVM est publiée ou que Clang Static Analyzer est mis à jour, les questions suivantes apparaissent dans notre courrier:

Regardez, la nouvelle version de Clang Static Analyzer a appris à trouver de nouveaux bugs! Il me semble que la pertinence d'utiliser PVS-Studio diminue. Clang trouve plus d'erreurs qu'auparavant et rattrape les capacités de PVS-Studio. Qu'en pensez-vous?

À cela, je veux toujours répondre à quelque chose dans l'esprit:

Nous ne sommes pas non plus restés inactifs! 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 en elle. Et c'est pourquoi j'écris cet article maintenant. Ainsi, le projet LLVM a de nouveau été testé et diverses erreurs y ont été trouvées. Celles qui m'ont paru intéressantes, je vais maintenant les démontrer. Ces erreurs ne peuvent pas être trouvées par l'analyseur statique Clang (ou il est extrêmement gênant de le faire). Et nous le pouvons. Et j'ai trouvé et écrit toutes ces erreurs en une soirée.

Mais la rédaction de l'article a duré plusieurs semaines. Je ne pouvais pas me forcer à organiser tout cela sous forme de texte :).

Soit dit en passant, si vous êtes intéressé par les technologies utilisées dans l'analyseur PVS-Studio pour détecter les erreurs et les vulnérabilités potentielles, je vous suggère de vous familiariser avec cette note .

Diagnostics nouveaux et anciens


Comme déjà indiqué, il y a environ deux ans, le projet LLVM a été une fois de plus vérifié et les erreurs trouvées ont été corrigées. Maintenant, dans cet article, une nouvelle partie des erreurs sera présentée. Pourquoi de nouveaux bogues ont-ils été trouvés? Il y a 3 raisons à cela:

  1. Le projet LLVM se développe, l'ancien code y change et un nouveau apparaît. Naturellement, il y a de nouvelles erreurs dans le code modifié et écrit. Cela montre bien que l'analyse statique doit être appliquée régulièrement, et non au cas par cas. 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 la correction des erreurs. Utilisez régulièrement un analyseur de code statique!
  2. Nous finalisons et améliorons les diagnostics existants. Par conséquent, l'analyseur peut détecter des erreurs qui n'ont pas été détectées lors des vérifications précédentes.
  3. PVS-Studio a introduit de nouveaux diagnostics, qui n'étaient pas il y a 2 ans. J'ai décidé de les séparer dans une section distincte pour démontrer clairement le développement de PVS-Studio.

Défauts identifiés par des diagnostics qui existaient 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: V501 [CWE-570] Il existe des sous-expressions identiques 'Name.startswith ("avx512.mask.permvar.")' À gauche et à droite de '||' opérateur. AutoUpgrade.cpp 73

Il est vérifié que le nom commence par la sous-chaîne «avx512.mask.permvar». Dans le deuxième test, ils voulaient clairement écrire autre chose, mais 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; .... }

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

En raison d'une faute de frappe, la même constante nommée CXNameRange_WantQualifier est utilisée deux fois .

Extrait N3: confusion sur les priorités 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: 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

À mon avis, c'est une très belle erreur. Oui, je sais que j'ai des idées étranges sur la beauté :).

Maintenant, selon les priorités des opérateurs , l'expression est calculée comme suit:

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

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

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

C'est une erreur évidente. Très probablement, 0/1 voulait comparer avec la variable Index . Pour corriger le code, ajoutez des crochets autour de l'opérateur ternaire:

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

Soit dit en passant, l'opérateur ternaire est très dangereux et provoque des erreurs logiques. Soyez très prudent avec elle et ne soyez pas avide de mettre des parenthèses. J'ai examiné ce sujet plus en détail ici dans le chapitre «Craignez l'opérateur?: Et mettez-le entre parenthèses».

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: V522 [CWE-476] Le déréférencement du pointeur nul 'LHS' peut avoir lieu. TGParser.cpp 2152

Si le pointeur LHS est nul, un avertissement doit être émis. Cependant, au lieu de cela, cela déréférencera ce pointeur très nul: LHS-> getAsString () .

Il s'agit d'une situation très typique lorsque l'erreur est masquée dans le gestionnaire d'erreurs, car personne ne les teste. Les analyseurs statiques vérifient tout le code accessible, quelle que soit sa fréquence d'utilisation. Il s'agit d'un très bon exemple de la façon dont l'analyse statique complète d'autres techniques de test et de protection contre les erreurs.

Une erreur similaire lors du traitement du pointeur RHS est générée dans le code ci-dessous: V522 [CWE-476] Le déréférencement du pointeur nul "RHS" peut avoir lieu. TGParser.cpp 2186

Fragment N6: utilisation du curseur après le 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] Le déréférencement du pointeur nul 'ProgClone' peut avoir lieu. Miscompilation.cpp 601

Au début, le pointeur intelligent ProgClone cesse d'être propriétaire de l'objet:

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

En fait, ProgClone est désormais un pointeur nul. Par conséquent, un déréférencement du pointeur nul doit se produire juste en dessous:

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

Mais, en fait, cela n'arrivera pas! Notez que la boucle n'est pas réellement en cours d'exécution.

Au début, le conteneur MiscompiledFunctions est 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 voir que la boucle ne démarre pas. Je pense que c'est aussi une erreur, et le code devrait être écrit différemment.

Il semble que nous ayons rencontré cette très célèbre parité d'erreurs! Une erreur en masque une autre :).

Fragment N7: utilisation du curseur après le 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] Le déréférencement du pointeur nul "Test" peut avoir lieu. Miscompilation.cpp 709

Encore une fois la même situation. Au début, le contenu de l'objet est déplacé, puis il est utilisé comme si de rien n'était. Je vois de plus en plus cette situation dans le code du programme, après l'apparition de la sémantique du mouvement en C ++. Pour cela j'adore le langage C ++! Il existe de plus en plus de nouvelles façons de tirer sur sa propre jambe. L'analyseur PVS-Studio fonctionnera toujours :).

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] Le déréférencement du pointeur nul 'Type' peut avoir lieu. PrettyFunctionDumper.cpp 233

Outre les gestionnaires d'erreurs, les fonctions de débogage des impressions ne sont généralement pas testées. Un tel cas est devant nous. La fonction attend un utilisateur qui, au lieu de résoudre ses problèmes, sera obligé de le corriger.

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] Le déréférencement du pointeur nul 'Ty' peut avoir lieu. SearchableTableEmitter.cpp 614

Je pense, et donc 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; }

PVS-Studio Warning: V570 La variable 'Identifier-> Type' est assignée à elle-même. FormatTokenLexer.cpp 249

Cela n'a aucun sens 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: V622 [CWE-478] Envisagez d'inspecter l'instruction 'switch'. Il est possible que le premier opérateur «case» soit manquant. SystemZAsmParser.cpp 652

Au début, il y a une déclaration de rupture très suspecte. Avez-vous oublié d'écrire autre chose ici?

Fragment N12: vérification du 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: 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é au début est déréférencé lorsque la fonction getTTI est appelée .

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

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

Mais c'est trop tard ...

Fragment N13 - N ...: Vérification du pointeur après déréférencement

La situation décrite dans l'extrait de code précédent n'est pas unique. Elle se trouve 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. 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

Et puis il m'est devenu inutile d'étudier les avertissements avec le numéro V595. Je ne sais donc pas s'il existe des erreurs similaires à celles répertoriées ici. Il y en a très probablement.

Fragment N17, N18: changement 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: 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 n'est peut-être pas une erreur et le code fonctionne exactement comme prévu. Mais c'est clairement un endroit très suspect, et cela doit être vérifié.

Supposons que la variable Size soit 16, puis l'auteur du code a prévu d'obtenir la valeur dans la variable NImms :

11111111111111111111111111111111111111111111111111111111111111100000000

Cependant, en fait, le résultat est:

0000000000000000000000000000000000001111111111111111111111111111100000

Le fait est que tous les calculs se produisent en utilisant le type non signé 32 bits. Et seulement alors, ce type non signé 32 bits sera implicitement étendu à uint64_t . Dans ce cas, les bits les plus significatifs seront nuls.

Vous pouvez corriger la situation comme ceci:

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

Situation similaire: 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 N19: Mot - clé else manquant ?

 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: V646 [CWE-670] Envisagez d'inspecter la logique de l'application. Il est possible que le mot clé "else" soit manquant. AMDGPUAsmParser.cpp 5655

Il n'y a pas d'erreur ici. Étant donné que le bloc alors du premier if se termine par continue , peu importe que le mot-clé else existe ou non. Dans tous les cas, le code fonctionnera de la même manière. Cependant, manquer un autre rend le code plus obscur et dangereux. Si à l'avenir la suite disparaît, alors le code commencera à fonctionner d'une manière complètement différente. À mon avis, il vaut mieux ajouter autre chose .

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

Par hasard, l'opérateur + est utilisé à la place de l'opérateur + =. Le résultat est des conceptions dénuées de sens.

Fragment N21: 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 de trouver le code dangereux vous-même. Et ceci est une image de distraction, afin de ne pas regarder immédiatement la réponse:

Hmmm ...


Avertissement 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

Ligne de problème:

 FeaturesMap[Op] = FeaturesMap.size();

Si l'élément Op n'est pas trouvé, un nouvel élément est créé dans la carte et le nombre d'éléments dans cette carte y est écrit. On ne sait tout simplement pas si la fonction size sera appelée avant ou après l'ajout d'un nouvel élément.

Fragment N22-N24: réaffectations

 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: 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 pense qu'il n'y a pas vraiment d'erreur ici. Affectation répétée juste superflue. Mais toujours une erreur.

De même:

  • 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

Fragment N25-N27: plus de réaffectations

Considérons maintenant une option de réaffectation légèrement différente.

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

PVS-Studio Warning: V519 [CWE-563] La variable 'Alignment' reçoit des valeurs successives deux fois. 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 semble contenir une erreur logique. Au début, la variable d' alignement se voit attribuer une valeur en fonction de la condition. Et puis l'affectation se produit à nouveau, mais maintenant sans aucune vérification.

Des situations similaires peuvent être vues ici:

  • 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

Fragment N28: toujours une vraie condition

 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: V547 [CWE-571] L'expression 'nextByte! = 0x90' est toujours vraie. X86DisassemblerDecoder.cpp 379

La vérification n'a pas de sens. La variable nextByte n'est pas toujours égale à 0x90 , qui découle de la vérification précédente. C'est une sorte d'erreur logique.

Fragment N29 - N ...: conditions toujours vraies / fausses

L'analyseur donne de nombreux avertissements que la condition entière ( V547 ) ou une partie de celle-ci ( V560 ) est toujours vraie ou fausse. Souvent, ce ne sont pas de vraies erreurs, mais simplement du code inexact, résultant du déploiement de macros et autres. Néanmoins, il est logique de regarder tous ces avertissements, car de temps en temps il y a de vraies erreurs logiques. Par exemple, ce morceau de code est suspect:

 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: V560 [CWE-570] Une partie de l'expression conditionnelle est toujours fausse: RegNo == 0xe. ARMDisassembler.cpp 939

La constante 0xE est la valeur 14 dans le système décimal. Vérifier RegNo == 0xe n'a pas de sens, car si RegNo> 13 , la fonction terminera son exécution.

Il y avait beaucoup d'autres avertissements avec les identifiants V547 et V560, mais, comme dans le cas de V595 , je n'étais pas intéressé à étudier ces avertissements. Il était déjà clair que j'avais suffisamment de matériel pour écrire un article :). Par conséquent, on ne sait pas combien d'erreurs de ce type peuvent être détectées dans LLVM à l'aide de PVS-Studio.

Je vais vous donner un exemple pourquoi il est ennuyeux d'étudier ces réponses. 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: V612 [CWE-670] Un "retour" inconditionnel dans une boucle. R600OptimizeVectorRegisters.cpp 63

Il s'agit soit d'une erreur, soit d'une astuce spécifique destinée à expliquer quelque chose aux programmeurs lisant le code. Cette conception ne m'explique rien et semble très suspecte. Il vaut mieux ne pas écrire comme ça :).

Êtes-vous fatigué? Puis il est temps de faire du thé ou du café.

Le café


Défauts détectés par de nouveaux diagnostics


Je pense que 30 déclenchements d'anciens diagnostics suffisent. Voyons maintenant ce qui est intéressant avec les nouveaux diagnostics apparus dans l'analyseur après une vérification précédente . Au total, 66 diagnostics à usage général ont été ajoutés à l'analyseur C ++ pendant cette période.

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: 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 un appel à l' instruction return . Par conséquent, le conteneur CtorDtorsByPriority ne sera jamais vidé .

Extrait 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

Une situation intéressante. Regardons le début de ce lieu:

 return ParseTypeIdEntry(SummaryID); break;

À première vue, il semble qu'il n'y ait pas d'erreur. Il semble que l'instruction break soit superflue ici, et vous pouvez simplement la supprimer. Cependant, tout n'est pas si simple.

L'analyseur génère un avertissement sur les lignes:

 Lex.setIgnoreColonInIdentifiers(false); return false;

En effet, ce code est inaccessible. Tous les cas dans le commutateur se terminent par un appel à l' instruction de retour . Et maintenant, la pause solitaire inutile ne semble pas si inoffensive! Peut-être que l'une des branches devrait se terminer par une rupture , et non par un retour ?

Fragment N33: Accidentalisation de 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: 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 un type non signé . Nous calculons la valeur de l'expression si nous supposons que la fonction renvoie la valeur 8:

~ (getStubAlignment () - 1)

~ (8u-1)

0xFFFFFFF8u

Notez maintenant que la variable DataSize a un type non signé 64 bits. Il s'avère que lors de l'opération DataSize & 0xFFFFFFF88, les trente-deux bits de poids fort seront réinitialisés. Ce n'est probablement pas ce que le programmeur voulait. Je soupçonne qu'il voulait calculer: DataSize & 0xFFFFFFFFFFFFFFFFF8u.

Pour corriger l'erreur, vous devez écrire comme ceci:

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

Ou alors:

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

Fragment N34: transtypage explicite échoué

 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: 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 le débordement lors de la multiplication de variables de type int . Cependant, la conversion explicite ici ne protège pas contre le débordement.Au début, les variables seront multipliées, et ce n'est qu'alors que le résultat de la multiplication 32 bits sera étendu au type size_t .

Fragment N35: copier-coller infructueux

 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 intéressant révèle les situations où un morceau de code a été copié et certains noms ont commencé à changer, mais n'ont pas été corrigés en un seul endroit.

Veuillez noter que dans le deuxième bloc, ils ont changé Op0 en Op1 . Mais à un endroit, ils ne l'ont pas résolu. Très probablement, il aurait dû être écrit comme ceci:

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

Fragment N36: confusion dans les variables

 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: 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 de donner aux arguments de fonction les mêmes noms que les membres de la classe. Très facile à confondre. Un tel cas est devant nous. Cette expression n'a pas de sens:

 Mode &= Mask;

L'argument de la fonction change. Et c'est tout. Cet argument n'est plus utilisé. Très probablement, il fallait écrire comme ceci:

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

Fragment N37: confusion dans les variables

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

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

La situation est similaire à la précédente. Il devrait être écrit:

 this->Size += this->EntrySize;

Fragment N38-N47: le pointeur a oublié de vérifier.

Plus tôt, nous avons examiné des exemples de déclenchement de diagnostic V595 . Son essence est que le pointeur est déréférencé au début, puis seulement vérifié. Le jeune diagnostic du V1004 est l'inverse de sa signification, mais il détecte également beaucoup d'erreurs. Il identifie les situations où le pointeur a été vérifié au début, puis a oublié de le faire. Considérez de tels cas trouvés dans 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()); // <= .... }

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

La variable Ptr peut être nullptr , comme en témoigne la vérification:

 if (Ptr != nullptr)

Cependant, sous ce pointeur est déréférencé sans vérification préalable:

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

Prenons 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 non sécurisée après avoir été vérifié par rapport à nullptr. Vérifiez les lignes: 3228, 3231. CGDebugInfo.cpp 3231

Faites attention au pointeur FD . Je suis sûr que le problème est clairement visible et aucune explication particulière n'est requise.

Et aussi:

 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 vous protéger contre de telles erreurs? Soyez prudent lors de la révision de code et utilisez l'analyseur statique PVS-Studio pour vérifier régulièrement votre code.

Cela n'a aucun sens d'apporter d'autres fragments de code avec 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. 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] The 'StatepointCall' pointer was used unsafely after it was verified against nullptr. Check lines: 4371, 4379. Verifier.cpp 4379
  • V1004 [CWE-476] The 'RV' pointer was used unsafely after it was verified against nullptr. Check lines: 2263, 2268. TGParser.cpp 2268
  • V1004 [CWE-476] The 'CalleeFn' pointer was used unsafely after it was verified against nullptr. Check lines: 1081, 1096. SimplifyLibCalls.cpp 1096
  • V1004 [CWE-476] The 'TC' pointer was used unsafely after it was verified against nullptr. Check lines: 1819, 1824. Driver.cpp 1824

N48-N60: , ( )

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

Avertissement 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

Pour ajouter un élément à la fin d'un conteneur comme std :: vector <std :: unique_ptr <X>>, vous ne pouvez pas simplement écrire xxx.push_back (nouveau X) , car il n'y a pas de conversion implicite de X * en std: : unique_ptr <X> .

Une solution courante consiste à écrire xxx.emplace_back (nouveau X) , car il compile: la méthode emplace_back construit un élément directement à partir des arguments et peut donc utiliser des constructeurs explicites.

Ce n'est pas sûr. Si le vecteur est plein, la mémoire est allouée. Une 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_ptr , qui possédera le pointeur avant que le vecteur essaie de réallouer la mémoire:

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

À partir de 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 cessera simplement de fonctionner. Cependant, pour les applications avec un temps de fonctionnement long qui ne peuvent pas se terminer si l'allocation de mémoire échoue, cela peut être un vrai bogue.

Donc, bien que ce code ne présente pas de danger pratique pour LLVM, j'ai trouvé utile de parler de ce modèle d'erreur et que l'analyseur PVS-Studio a appris à le détecter.

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] 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


Au total, j'ai écrit 60 avertissements, après quoi je me suis arrêté. Y a-t-il d'autres défauts que l'analyseur PVS-Studio détecte dans LLVM? Oui. Cependant, quand j'ai écrit les extraits de code de l'article, c'était tard le soir, ou plutôt même la nuit, et j'ai décidé qu'il était temps d'arrêter.

J'espère que vous étiez intéressé et que vous voudrez essayer l'analyseur PVS-Studio.

Vous pouvez télécharger l'analyseur et obtenir une clé d'essai sur cette page .

Plus important encore, utilisez régulièrement l'analyse statique. Les contrôles ponctuels effectué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é du code!



Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: Andrey Karpov. Recherche de bogues dans LLVM 8 avec PVS-Studio .

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

More articles:


All Articles