From f88b97bb2a67b9852e9f603a9bf9592f20b948b0 Mon Sep 17 00:00:00 2001 From: Kristof Umann Date: Wed, 14 Aug 2019 00:48:57 +0000 Subject: [PATCH] [analyzer][NFC] Prepare visitors for different tracking kinds When we're tracking a variable that is responsible for a null pointer dereference or some other sinister programming error, we of course would like to gather as much information why we think that the variable has that specific value as possible. However, the newly introduced condition tracking shows that tracking all values this thoroughly could easily cause an intolerable growth in the bug report's length. There are a variety of heuristics we discussed on the mailing list[1] to combat this, all of them requiring to differentiate in between tracking a "regular value" and a "condition". This patch introduces the new `bugreporter::TrackingKind` enum, adds it to several visitors as a non-optional argument, and moves some functions around to make the code a little more coherent. [1] http://lists.llvm.org/pipermail/cfe-dev/2019-June/062613.html Differential Revision: https://reviews.llvm.org/D64270 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@368777 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../Core/BugReporter/BugReporterVisitors.h | 76 +++++++---- lib/StaticAnalyzer/Checkers/MIGChecker.cpp | 3 +- .../Checkers/ObjCContainersChecker.cpp | 4 +- .../Checkers/UndefCapturedBlockVarChecker.cpp | 3 +- .../Core/BugReporterVisitors.cpp | 123 +++++++++--------- 5 files changed, 121 insertions(+), 88 deletions(-) diff --git a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h index b2927a08b6..4ca38079d0 100644 --- a/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h +++ b/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h @@ -85,6 +85,40 @@ public: const BugReport &BR); }; +namespace bugreporter { + +/// Specifies the type of tracking for an expression. +enum class TrackingKind { + /// Default tracking kind -- specifies that as much information should be + /// gathered about the tracked expression value as possible. + Thorough, + /// Specifies that a more moderate tracking should be used for the expression + /// value. This will essentially make sure that functions relevant to the it + /// aren't pruned, but otherwise relies on the user reading the code or + /// following the arrows. + Condition +}; + +/// Attempts to add visitors to track expression value back to its point of +/// origin. +/// +/// \param N A node "downstream" from the evaluation of the statement. +/// \param E The expression value which we are tracking +/// \param R The bug report to which visitors should be attached. +/// \param EnableNullFPSuppression Whether we should employ false positive +/// suppression (inlined defensive checks, returned null). +/// +/// \return Whether or not the function was able to add visitors for this +/// statement. Note that returning \c true does not actually imply +/// that any visitors were added. +bool trackExpressionValue(const ExplodedNode *N, const Expr *E, BugReport &R, + TrackingKind TKind = TrackingKind::Thorough, + bool EnableNullFPSuppression = true); + +const Expr *getDerefExpr(const Stmt *S); + +} // namespace bugreporter + /// Finds last store into the given region, /// which is different from a given symbolic value. class FindLastStoreBRVisitor final : public BugReporterVisitor { @@ -96,15 +130,28 @@ class FindLastStoreBRVisitor final : public BugReporterVisitor { /// bug, we are going to employ false positive suppression. bool EnableNullFPSuppression; + using TrackingKind = bugreporter::TrackingKind; + TrackingKind TKind; + public: /// Creates a visitor for every VarDecl inside a Stmt and registers it with /// the BugReport. static void registerStatementVarDecls(BugReport &BR, const Stmt *S, - bool EnableNullFPSuppression); - + bool EnableNullFPSuppression, + TrackingKind TKind); + + /// \param V We're searching for the store where \c R received this value. + /// \param R The region we're tracking. + /// \param EnableNullFPSuppression Whether we should employ false positive + /// suppression (inlined defensive checks, returned null). + /// \param TKind May limit the amount of notes added to the bug report. FindLastStoreBRVisitor(KnownSVal V, const MemRegion *R, - bool InEnableNullFPSuppression) - : R(R), V(V), EnableNullFPSuppression(InEnableNullFPSuppression) {} + bool InEnableNullFPSuppression, + TrackingKind TKind) + : R(R), V(V), EnableNullFPSuppression(InEnableNullFPSuppression), + TKind(TKind) { + assert(R); + } void Profile(llvm::FoldingSetNodeID &ID) const override; @@ -347,27 +394,6 @@ public: BugReport &R) override; }; -namespace bugreporter { - -/// Attempts to add visitors to track expression value back to its point of -/// origin. -/// -/// \param N A node "downstream" from the evaluation of the statement. -/// \param E The expression value which we are tracking -/// \param R The bug report to which visitors should be attached. -/// \param EnableNullFPSuppression Whether we should employ false positive -/// suppression (inlined defensive checks, returned null). -/// -/// \return Whether or not the function was able to add visitors for this -/// statement. Note that returning \c true does not actually imply -/// that any visitors were added. -bool trackExpressionValue(const ExplodedNode *N, const Expr *E, BugReport &R, - bool EnableNullFPSuppression = true); - -const Expr *getDerefExpr(const Stmt *S); - -} // namespace bugreporter - } // namespace ento } // namespace clang diff --git a/lib/StaticAnalyzer/Checkers/MIGChecker.cpp b/lib/StaticAnalyzer/Checkers/MIGChecker.cpp index 6e7776bb48..9cdc7612b2 100644 --- a/lib/StaticAnalyzer/Checkers/MIGChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/MIGChecker.cpp @@ -282,7 +282,8 @@ void MIGChecker::checkReturnAux(const ReturnStmt *RS, CheckerContext &C) const { N); R->addRange(RS->getSourceRange()); - bugreporter::trackExpressionValue(N, RS->getRetValue(), *R, false); + bugreporter::trackExpressionValue(N, RS->getRetValue(), *R, + bugreporter::TrackingKind::Thorough, false); C.emitReport(std::move(R)); } diff --git a/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp b/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp index f69a3944a5..98d2a9941d 100644 --- a/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp @@ -146,8 +146,8 @@ void ObjCContainersChecker::checkPreStmt(const CallExpr *CE, initBugType(); auto R = llvm::make_unique(*BT, "Index is out of bounds", N); R->addRange(IdxExpr->getSourceRange()); - bugreporter::trackExpressionValue(N, IdxExpr, *R, - /*EnableNullFPSuppression=*/false); + bugreporter::trackExpressionValue( + N, IdxExpr, *R, bugreporter::TrackingKind::Thorough, false); C.emitReport(std::move(R)); return; } diff --git a/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp b/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp index c787ef5866..1b6f19c01c 100644 --- a/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp +++ b/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp @@ -87,7 +87,8 @@ UndefCapturedBlockVarChecker::checkPostStmt(const BlockExpr *BE, if (const Expr *Ex = FindBlockDeclRefExpr(BE->getBody(), VD)) R->addRange(Ex->getSourceRange()); R->addVisitor(llvm::make_unique( - *V, VR, /*EnableNullFPSuppression*/ false)); + *V, VR, /*EnableNullFPSuppression*/ false, + bugreporter::TrackingKind::Thorough)); R->disablePathPruning(); // need location of block C.emitReport(std::move(R)); diff --git a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp index bd3296f7ea..21e488ff85 100644 --- a/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -851,12 +851,13 @@ class ReturnVisitor : public BugReporterVisitor { bool EnableNullFPSuppression; bool ShouldInvalidate = true; AnalyzerOptions& Options; + bugreporter::TrackingKind TKind; public: ReturnVisitor(const StackFrameContext *Frame, bool Suppressed, - AnalyzerOptions &Options) + AnalyzerOptions &Options, bugreporter::TrackingKind TKind) : CalleeSFC(Frame), EnableNullFPSuppression(Suppressed), - Options(Options) {} + Options(Options), TKind(TKind) {} static void *getTag() { static int Tag = 0; @@ -878,7 +879,8 @@ public: /// bug report, so it can print a note later. static void addVisitorIfNecessary(const ExplodedNode *Node, const Stmt *S, BugReport &BR, - bool InEnableNullFPSuppression) { + bool InEnableNullFPSuppression, + bugreporter::TrackingKind TKind) { if (!CallEvent::isCallStmt(S)) return; @@ -951,7 +953,7 @@ public: BR.addVisitor(llvm::make_unique(CalleeContext, EnableNullFPSuppression, - Options)); + Options, TKind)); } PathDiagnosticPieceRef visitNodeInitial(const ExplodedNode *N, @@ -999,8 +1001,9 @@ public: RetE = RetE->IgnoreParenCasts(); - // If we're returning 0, we should track where that 0 came from. - bugreporter::trackExpressionValue(N, RetE, BR, EnableNullFPSuppression); + // Let's track the return value. + bugreporter::trackExpressionValue( + N, RetE, BR, TKind, EnableNullFPSuppression); // Build an appropriate message based on the return value. SmallString<64> Msg; @@ -1115,7 +1118,7 @@ public: if (!State->isNull(*ArgV).isConstrainedTrue()) continue; - if (bugreporter::trackExpressionValue(N, ArgE, BR, EnableNullFPSuppression)) + if (trackExpressionValue(N, ArgE, BR, TKind, EnableNullFPSuppression)) ShouldInvalidate = false; // If we /can't/ track the null pointer, we should err on the side of @@ -1159,9 +1162,45 @@ void FindLastStoreBRVisitor::Profile(llvm::FoldingSetNodeID &ID) const { ID.AddPointer(&tag); ID.AddPointer(R); ID.Add(V); + ID.AddInteger(static_cast(TKind)); ID.AddBoolean(EnableNullFPSuppression); } +void FindLastStoreBRVisitor::registerStatementVarDecls( + BugReport &BR, const Stmt *S, bool EnableNullFPSuppression, + TrackingKind TKind) { + + const ExplodedNode *N = BR.getErrorNode(); + std::deque WorkList; + WorkList.push_back(S); + + while (!WorkList.empty()) { + const Stmt *Head = WorkList.front(); + WorkList.pop_front(); + + ProgramStateManager &StateMgr = N->getState()->getStateManager(); + + if (const auto *DR = dyn_cast(Head)) { + if (const auto *VD = dyn_cast(DR->getDecl())) { + const VarRegion *R = + StateMgr.getRegionManager().getVarRegion(VD, N->getLocationContext()); + + // What did we load? + SVal V = N->getSVal(S); + + if (V.getAs() || V.getAs()) { + // Register a new visitor with the BugReport. + BR.addVisitor(llvm::make_unique( + V.castAs(), R, EnableNullFPSuppression, TKind)); + } + } + } + + for (const Stmt *SubStmt : Head->children()) + WorkList.push_back(SubStmt); + } +} + /// Returns true if \p N represents the DeclStmt declaring and initializing /// \p VR. static bool isInitializationOfVar(const ExplodedNode *N, const VarRegion *VR) { @@ -1332,7 +1371,7 @@ FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ, // should track the initializer expression. if (Optional PIP = Pred->getLocationAs()) { const MemRegion *FieldReg = (const MemRegion *)PIP->getLocationValue(); - if (FieldReg && FieldReg == R) { + if (FieldReg == R) { StoreSite = Pred; InitE = PIP->getInitializer()->getInit(); } @@ -1397,8 +1436,8 @@ FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ, if (!IsParam) InitE = InitE->IgnoreParenCasts(); - bugreporter::trackExpressionValue(StoreSite, InitE, BR, - EnableNullFPSuppression); + bugreporter::trackExpressionValue( + StoreSite, InitE, BR, TKind, EnableNullFPSuppression); } // Okay, we've found the binding. Emit an appropriate message. @@ -1426,7 +1465,7 @@ FindLastStoreBRVisitor::VisitNode(const ExplodedNode *Succ, if (const VarRegion *OriginalR = BDR->getOriginalRegion(VR)) { if (auto KV = State->getSVal(OriginalR).getAs()) BR.addVisitor(llvm::make_unique( - *KV, OriginalR, EnableNullFPSuppression)); + *KV, OriginalR, EnableNullFPSuppression, TKind)); } } } @@ -1724,7 +1763,8 @@ PathDiagnosticPieceRef TrackControlDependencyCondBRVisitor::VisitNode( // expression, hence the BugReport level set. if (BR.addTrackedCondition(N)) { bugreporter::trackExpressionValue( - N, Condition, BR, /*EnableNullFPSuppression=*/false); + N, Condition, BR, bugreporter::TrackingKind::Condition, + /*EnableNullFPSuppression=*/false); return constructDebugPieceForTrackedCondition(Condition, N, BRC); } } @@ -1838,7 +1878,9 @@ static const ExplodedNode* findNodeForExpression(const ExplodedNode *N, bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode, const Expr *E, BugReport &report, + bugreporter::TrackingKind TKind, bool EnableNullFPSuppression) { + if (!E || !InputNode) return false; @@ -1862,12 +1904,13 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode, // At this point in the path, the receiver should be live since we are at the // message send expr. If it is nil, start tracking it. if (const Expr *Receiver = NilReceiverBRVisitor::getNilReceiver(Inner, LVNode)) - trackExpressionValue(LVNode, Receiver, report, EnableNullFPSuppression); + trackExpressionValue( + LVNode, Receiver, report, TKind, EnableNullFPSuppression); // Track the index if this is an array subscript. if (const auto *Arr = dyn_cast(Inner)) trackExpressionValue( - LVNode, Arr->getIdx(), report, /*EnableNullFPSuppression*/ false); + LVNode, Arr->getIdx(), report, TKind, /*EnableNullFPSuppression*/false); // See if the expression we're interested refers to a variable. // If so, we can track both its contents and constraints on its value. @@ -1883,7 +1926,7 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode, if (RR && !LVIsNull) if (auto KV = LVal.getAs()) report.addVisitor(llvm::make_unique( - *KV, RR, EnableNullFPSuppression)); + *KV, RR, EnableNullFPSuppression, TKind)); // In case of C++ references, we want to differentiate between a null // reference and reference to null pointer. @@ -1920,7 +1963,7 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode, if (auto KV = V.getAs()) report.addVisitor(llvm::make_unique( - *KV, R, EnableNullFPSuppression)); + *KV, R, EnableNullFPSuppression, TKind)); return true; } } @@ -1930,7 +1973,7 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode, SVal V = LVState->getSValAsScalarOrLoc(Inner, LVNode->getLocationContext()); ReturnVisitor::addVisitorIfNecessary( - LVNode, Inner, report, EnableNullFPSuppression); + LVNode, Inner, report, EnableNullFPSuppression, TKind); // Is it a symbolic value? if (auto L = V.getAs()) { @@ -1959,7 +2002,7 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode, if (CanDereference) if (auto KV = RVal.getAs()) report.addVisitor(llvm::make_unique( - *KV, L->getRegion(), EnableNullFPSuppression)); + *KV, L->getRegion(), EnableNullFPSuppression, TKind)); const MemRegion *RegionRVal = RVal.getAsRegion(); if (RegionRVal && isa(RegionRVal)) { @@ -2017,53 +2060,15 @@ PathDiagnosticPieceRef NilReceiverBRVisitor::VisitNode(const ExplodedNode *N, // The receiver was nil, and hence the method was skipped. // Register a BugReporterVisitor to issue a message telling us how // the receiver was null. - bugreporter::trackExpressionValue(N, Receiver, BR, - /*EnableNullFPSuppression*/ false); + bugreporter::trackExpressionValue( + N, Receiver, BR, bugreporter::TrackingKind::Thorough, + /*EnableNullFPSuppression*/ false); // Issue a message saying that the method was skipped. PathDiagnosticLocation L(Receiver, BRC.getSourceManager(), N->getLocationContext()); return std::make_shared(L, OS.str()); } -//===----------------------------------------------------------------------===// -// Implementation of FindLastStoreBRVisitor. -//===----------------------------------------------------------------------===// - -// Registers every VarDecl inside a Stmt with a last store visitor. -void FindLastStoreBRVisitor::registerStatementVarDecls(BugReport &BR, - const Stmt *S, - bool EnableNullFPSuppression) { - const ExplodedNode *N = BR.getErrorNode(); - std::deque WorkList; - WorkList.push_back(S); - - while (!WorkList.empty()) { - const Stmt *Head = WorkList.front(); - WorkList.pop_front(); - - ProgramStateManager &StateMgr = N->getState()->getStateManager(); - - if (const auto *DR = dyn_cast(Head)) { - if (const auto *VD = dyn_cast(DR->getDecl())) { - const VarRegion *R = - StateMgr.getRegionManager().getVarRegion(VD, N->getLocationContext()); - - // What did we load? - SVal V = N->getSVal(S); - - if (V.getAs() || V.getAs()) { - // Register a new visitor with the BugReport. - BR.addVisitor(llvm::make_unique( - V.castAs(), R, EnableNullFPSuppression)); - } - } - } - - for (const Stmt *SubStmt : Head->children()) - WorkList.push_back(SubStmt); - } -} - //===----------------------------------------------------------------------===// // Visitor that tries to report interesting diagnostics from conditions. //===----------------------------------------------------------------------===// -- 2.40.0