Index: clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h =================================================================== --- clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h +++ clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h @@ -83,6 +83,40 @@ 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 { @@ -94,15 +128,28 @@ /// 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; @@ -345,27 +392,6 @@ 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 Index: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp @@ -282,7 +282,8 @@ 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)); } Index: clang/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/ObjCContainersChecker.cpp @@ -146,8 +146,8 @@ 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; } Index: clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp =================================================================== --- clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp +++ clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp @@ -87,7 +87,8 @@ 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)); Index: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp =================================================================== --- clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -851,13 +851,13 @@ bool EnableNullFPSuppression; bool ShouldInvalidate = true; AnalyzerOptions& Options; + bugreporter::TrackingKind TKind; public: - ReturnVisitor(const StackFrameContext *Frame, - bool Suppressed, - AnalyzerOptions &Options) + ReturnVisitor(const StackFrameContext *Frame, bool Suppressed, + AnalyzerOptions &Options, bugreporter::TrackingKind TKind) : CalleeSFC(Frame), EnableNullFPSuppression(Suppressed), - Options(Options) {} + Options(Options), TKind(TKind) {} static void *getTag() { static int Tag = 0; @@ -879,7 +879,8 @@ /// 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; @@ -952,7 +953,7 @@ BR.addVisitor(llvm::make_unique(CalleeContext, EnableNullFPSuppression, - Options)); + Options, TKind)); } std::shared_ptr @@ -1000,8 +1001,9 @@ 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; @@ -1114,7 +1116,7 @@ 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 @@ -1158,9 +1160,45 @@ 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) { @@ -1331,7 +1369,7 @@ // 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(); } @@ -1396,8 +1434,8 @@ 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. @@ -1425,7 +1463,7 @@ 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)); } } } @@ -1727,7 +1765,8 @@ // 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); } } @@ -1841,7 +1880,9 @@ bool bugreporter::trackExpressionValue(const ExplodedNode *InputNode, const Expr *E, BugReport &report, + bugreporter::TrackingKind TKind, bool EnableNullFPSuppression) { + if (!E || !InputNode) return false; @@ -1865,12 +1906,13 @@ // 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. @@ -1886,7 +1928,7 @@ 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. @@ -1923,7 +1965,7 @@ if (auto KV = V.getAs()) report.addVisitor(llvm::make_unique( - *KV, R, EnableNullFPSuppression)); + *KV, R, EnableNullFPSuppression, TKind)); return true; } } @@ -1933,7 +1975,7 @@ 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()) { @@ -1963,7 +2005,7 @@ 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)) { @@ -2021,8 +2063,9 @@ // 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()); @@ -2030,45 +2073,6 @@ } //===----------------------------------------------------------------------===// -// 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. //===----------------------------------------------------------------------===//