diff --git a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h --- a/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h +++ b/clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h @@ -19,6 +19,7 @@ #include "clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h" #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h" #include "llvm/ADT/FoldingSet.h" +#include "llvm/ADT/IntrusiveRefCntPtr.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/StringRef.h" #include @@ -147,6 +148,9 @@ const MemRegion *Dest, *Origin; }; +class Tracker; +using TrackerRef = llvm::IntrusiveRefCntPtr; + class ExpressionHandler; class StoreHandler; @@ -155,7 +159,7 @@ /// Tracker aimes at providing a sensible set of default behaviors that can be /// used by any checker, while providing mechanisms to hook into any part of the /// tracking process and insert checker-specific logic. -class Tracker { +class Tracker : public llvm::RefCountedBase { private: using ExpressionHandlerPtr = std::unique_ptr; using StoreHandlerPtr = std::unique_ptr; @@ -164,11 +168,17 @@ std::list ExpressionHandlers; std::list StoreHandlers; -public: +protected: /// \param Report The bug report to which visitors should be attached. Tracker(PathSensitiveBugReport &Report); + +public: virtual ~Tracker() = default; + static TrackerRef create(PathSensitiveBugReport &Report) { + return new Tracker(Report); + } + PathSensitiveBugReport &getReport() { return Report; } /// Describes a tracking result with the most basic information of what was @@ -318,6 +328,18 @@ Tracker &getParentTracker() { return ParentTracker; } }; +/// Visitor that tracks expressions and values. +class TrackingBugReporterVisitor : public BugReporterVisitor { +private: + TrackerRef ParentTracker; + +public: + TrackingBugReporterVisitor(TrackerRef ParentTracker) + : ParentTracker(ParentTracker) {} + + Tracker &getParentTracker() { return *ParentTracker; } +}; + /// Attempts to add visitors to track expression value back to its point of /// origin. /// @@ -335,13 +357,31 @@ TrackingKind TKind = TrackingKind::Thorough, bool EnableNullFPSuppression = true); +/// Track how the value got stored into the given region and where it came +/// from. +/// +/// \param V We're searching for the store where \c R received this value. +/// \param R The region we're tracking. +/// \param Opts Tracking options specifying how we want to track the value. +/// \param Origin Only adds notes when the last store happened in a +/// different stackframe to this one. Disregarded if the tracking kind +/// is thorough. +/// This is useful, because for non-tracked regions, notes about +/// changes to its value in a nested stackframe could be pruned, and +/// this visitor can prevent that without polluting the bugpath too +/// much. +void trackStoredValue(SVal V, const MemRegion *R, + PathSensitiveBugReport &Report, TrackingOptions Opts = {}, + const StackFrameContext *Origin = nullptr); + 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 { +class FindLastStoreBRVisitor final + : public bugreporter::TrackingBugReporterVisitor { const MemRegion *R; SVal V; bool Satisfied = false; @@ -365,11 +405,13 @@ /// changes to its value in a nested stackframe could be pruned, and /// this visitor can prevent that without polluting the bugpath too /// much. - FindLastStoreBRVisitor(KnownSVal V, const MemRegion *R, - bool InEnableNullFPSuppression, TrackingKind TKind, + FindLastStoreBRVisitor(bugreporter::TrackerRef ParentTracker, KnownSVal V, + const MemRegion *R, bool InEnableNullFPSuppression, + TrackingKind TKind, const StackFrameContext *OriginSFC = nullptr) - : R(R), V(V), EnableNullFPSuppression(InEnableNullFPSuppression), - TKind(TKind), OriginSFC(OriginSFC) { + : TrackingBugReporterVisitor(ParentTracker), R(R), V(V), + EnableNullFPSuppression(InEnableNullFPSuppression), TKind(TKind), + OriginSFC(OriginSFC) { assert(R); } diff --git a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp --- a/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp @@ -980,13 +980,12 @@ // got from one to another. // // NOTE: We use the actual SVal stored in AllocBindingToReport here because - // FindLastStoreBRVisitor compares SVal's and it can get trickier for + // trackStoredValue compares SVal's and it can get trickier for // something like derived regions if we want to construct SVal from // Sym. Instead, we take the value that is definitely stored in that - // region, thus guaranteeing that FindLastStoreBRVisitor will work. - addVisitor( - AllVarBindings[0].second.castAs(), AllocBindingToReport, - false, bugreporter::TrackingKind::Thorough); + // region, thus guaranteeing that trackStoredValue will work. + bugreporter::trackStoredValue(AllVarBindings[0].second, + AllocBindingToReport, *this); } else { AllocBindingToReport = AllocFirstBinding; } diff --git a/clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp --- a/clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp @@ -86,9 +86,9 @@ auto R = std::make_unique(*BT, os.str(), N); if (const Expr *Ex = FindBlockDeclRefExpr(BE->getBody(), VD)) R->addRange(Ex->getSourceRange()); - R->addVisitor(std::make_unique( - *V, VR, /*EnableNullFPSuppression*/ false, - bugreporter::TrackingKind::Thorough)); + bugreporter::trackStoredValue(*V, VR, *R, + {bugreporter::TrackingKind::Thorough, + /*EnableNullFPSuppression*/ false}); R->disablePathPruning(); // need location of block C.emitReport(std::move(R)); diff --git a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp --- a/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp +++ b/clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp @@ -1488,8 +1488,8 @@ if (!IsParam) InitE = InitE->IgnoreParenCasts(); - bugreporter::trackExpressionValue(StoreSite, InitE, BR, TKind, - EnableNullFPSuppression); + getParentTracker().track(InitE, StoreSite, + {TKind, EnableNullFPSuppression}); } // Let's try to find the region where the value came from. @@ -1588,8 +1588,8 @@ dyn_cast_or_null(V.getAsRegion())) { if (const VarRegion *OriginalR = BDR->getOriginalRegion(VR)) { if (auto KV = State->getSVal(OriginalR).getAs()) - BR.addVisitor( - *KV, OriginalR, EnableNullFPSuppression, TKind, OriginSFC); + getParentTracker().track( + *KV, OriginalR, {TKind, EnableNullFPSuppression}, OriginSFC); } } } @@ -2050,7 +2050,166 @@ // Tracker implementation //===----------------------------------------------------------------------===// +class DefaultExpressionHandler final : public ExpressionHandler { +public: + using ExpressionHandler::ExpressionHandler; + + Tracker::Result handle(const Expr *Inner, const ExplodedNode *InputNode, + const ExplodedNode *LVNode, + TrackingOptions Opts) override { + ProgramStateRef LVState = LVNode->getState(); + const StackFrameContext *SFC = LVNode->getStackFrame(); + PathSensitiveBugReport &Report = getParentTracker().getReport(); + Tracker::Result Result; + + // We only track expressions if we believe that they are important. Chances + // are good that control dependencies to the tracking point are also + // important because of this, let's explain why we believe control reached + // this point. + // TODO: Shouldn't we track control dependencies of every bug location, + // rather than only tracked expressions? + if (LVState->getAnalysisManager() + .getAnalyzerOptions() + .ShouldTrackConditions) { + Report.addVisitor(InputNode); + Result.FoundSomethingToTrack = true; + } + + // The message send could be nil due to the receiver being nil. + // 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)) + Result.combineWith(getParentTracker().track(Receiver, LVNode, Opts)); + + // Track the index if this is an array subscript. + if (const auto *Arr = dyn_cast(Inner)) + Result.combineWith(getParentTracker().track( + Arr->getIdx(), LVNode, + {Opts.Kind, /*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. + if (ExplodedGraph::isInterestingLValueExpr(Inner)) { + SVal LVal = LVNode->getSVal(Inner); + + const MemRegion *RR = getLocationRegionIfReference(Inner, LVNode); + bool LVIsNull = LVState->isNull(LVal).isConstrainedTrue(); + + // If this is a C++ reference to a null pointer, we are tracking the + // pointer. In addition, we should find the store at which the reference + // got initialized. + if (RR && !LVIsNull) + Result.combineWith(getParentTracker().track(LVal, RR, Opts, SFC)); + + // In case of C++ references, we want to differentiate between a null + // reference and reference to null pointer. + // If the LVal is null, check if we are dealing with null reference. + // For those, we want to track the location of the reference. + const MemRegion *R = + (RR && LVIsNull) ? RR : LVNode->getSVal(Inner).getAsRegion(); + + if (R) { + + // Mark both the variable region and its contents as interesting. + SVal V = LVState->getRawSVal(loc::MemRegionVal(R)); + Report.addVisitor(cast(R), Opts.Kind); + + // When we got here, we do have something to track, and we will + // interrupt. + Result.FoundSomethingToTrack = true; + Result.WasInterrupted = true; + + MacroNullReturnSuppressionVisitor::addMacroVisitorIfNecessary( + LVNode, R, Opts.EnableNullFPSuppression, Report, V); + + Report.markInteresting(V, Opts.Kind); + Report.addVisitor(R); + + // If the contents are symbolic and null, find out when they became + // null. + if (V.getAsLocSymbol(/*IncludeBaseRegions=*/true)) + if (LVState->isNull(V).isConstrainedTrue()) + Report.addVisitor(V.castAs(), + false); + + // Add visitor, which will suppress inline defensive checks. + if (auto DV = V.getAs()) + if (!DV->isZeroConstant() && Opts.EnableNullFPSuppression) + // Note that LVNode may be too late (i.e., too far from the + // InputNode) because the lvalue may have been computed before the + // inlined call was evaluated. InputNode may as well be too early + // here, because the symbol is already dead; this, however, is fine + // because we can still find the node in which it collapsed to null + // previously. + Report.addVisitor(*DV, + InputNode); + + getParentTracker().track(V, R, Opts, SFC); + + return Result; + } + } + + // If the expression is not an "lvalue expression", we can still + // track the constraints on its contents. + SVal V = LVState->getSValAsScalarOrLoc(Inner, LVNode->getLocationContext()); + + ReturnVisitor::addVisitorIfNecessary( + LVNode, Inner, Report, Opts.EnableNullFPSuppression, Opts.Kind); + + // Is it a symbolic value? + if (auto L = V.getAs()) { + // FIXME: this is a hack for fixing a later crash when attempting to + // dereference a void* pointer. + // We should not try to dereference pointers at all when we don't care + // what is written inside the pointer. + bool CanDereference = true; + if (const auto *SR = L->getRegionAs()) { + if (SR->getSymbol()->getType()->getPointeeType()->isVoidType()) + CanDereference = false; + } else if (L->getRegionAs()) + CanDereference = false; + + // At this point we are dealing with the region's LValue. + // However, if the rvalue is a symbolic region, we should track it as + // well. Try to use the correct type when looking up the value. + SVal RVal; + if (ExplodedGraph::isInterestingLValueExpr(Inner)) + RVal = LVState->getRawSVal(L.getValue(), Inner->getType()); + else if (CanDereference) + RVal = LVState->getSVal(L->getRegion()); + + if (CanDereference) { + Report.addVisitor(L->getRegion()); + Result.FoundSomethingToTrack = true; + + if (auto KV = RVal.getAs()) + Result.combineWith( + getParentTracker().track(*KV, L->getRegion(), Opts, SFC)); + } + + const MemRegion *RegionRVal = RVal.getAsRegion(); + if (isa_and_nonnull(RegionRVal)) { + Report.markInteresting(RegionRVal, Opts.Kind); + Report.addVisitor( + loc::MemRegionVal(RegionRVal), + /*assumption=*/false); + Result.FoundSomethingToTrack = true; + } + } + + if (Inner->isRValue()) + // TODO: Incorporate as Handler + trackRValueExpression(LVNode, Inner, Report, Opts.Kind, + Opts.EnableNullFPSuppression); + + return Result; + } +}; + Tracker::Tracker(PathSensitiveBugReport &Report) : Report(Report) { + addHighPriorityHandler(); // TODO: split trackExpressionValue and FindLastStoreBRVisitor into handlers // and add them here. } @@ -2078,7 +2237,11 @@ Tracker::Result Tracker::track(SVal V, const MemRegion *R, TrackingOptions Opts, const StackFrameContext *Origin) { - // TODO: support this operation after dismantling FindLastStoreBRVisitor + if (auto KV = V.getAs()) { + Report.addVisitor( + this, *KV, R, Opts.EnableNullFPSuppression, Opts.Kind, Origin); + return {true}; + } return {}; } @@ -2099,147 +2262,16 @@ bugreporter::TrackingKind TKind, bool EnableNullFPSuppression) { - if (!E || !InputNode) - return false; - - const Expr *Inner = peelOffOuterExpr(E, InputNode); - const ExplodedNode *LVNode = findNodeForExpression(InputNode, Inner); - if (!LVNode) - return false; - - ProgramStateRef LVState = LVNode->getState(); - const StackFrameContext *SFC = LVNode->getStackFrame(); - - // We only track expressions if we believe that they are important. Chances - // are good that control dependencies to the tracking point are also important - // because of this, let's explain why we believe control reached this point. - // TODO: Shouldn't we track control dependencies of every bug location, rather - // than only tracked expressions? - if (LVState->getAnalysisManager().getAnalyzerOptions().ShouldTrackConditions) - report.addVisitor(InputNode); - - // The message send could be nil due to the receiver being nil. - // 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, TKind, EnableNullFPSuppression); - - // Track the index if this is an array subscript. - if (const auto *Arr = dyn_cast(Inner)) - trackExpressionValue( - 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. - if (ExplodedGraph::isInterestingLValueExpr(Inner)) { - SVal LVal = LVNode->getSVal(Inner); - - const MemRegion *RR = getLocationRegionIfReference(Inner, LVNode); - bool LVIsNull = LVState->isNull(LVal).isConstrainedTrue(); - - // If this is a C++ reference to a null pointer, we are tracking the - // pointer. In addition, we should find the store at which the reference - // got initialized. - if (RR && !LVIsNull) - if (auto KV = LVal.getAs()) - report.addVisitor( - *KV, RR, EnableNullFPSuppression, TKind, SFC); - - // In case of C++ references, we want to differentiate between a null - // reference and reference to null pointer. - // If the LVal is null, check if we are dealing with null reference. - // For those, we want to track the location of the reference. - const MemRegion *R = (RR && LVIsNull) ? RR : - LVNode->getSVal(Inner).getAsRegion(); - - if (R) { - - // Mark both the variable region and its contents as interesting. - SVal V = LVState->getRawSVal(loc::MemRegionVal(R)); - report.addVisitor(cast(R), TKind); - - MacroNullReturnSuppressionVisitor::addMacroVisitorIfNecessary( - LVNode, R, EnableNullFPSuppression, report, V); - - report.markInteresting(V, TKind); - report.addVisitor(R); - - // If the contents are symbolic and null, find out when they became null. - if (V.getAsLocSymbol(/*IncludeBaseRegions=*/true)) - if (LVState->isNull(V).isConstrainedTrue()) - report.addVisitor(V.castAs(), - false); - - // Add visitor, which will suppress inline defensive checks. - if (auto DV = V.getAs()) - if (!DV->isZeroConstant() && EnableNullFPSuppression) { - // Note that LVNode may be too late (i.e., too far from the InputNode) - // because the lvalue may have been computed before the inlined call - // was evaluated. InputNode may as well be too early here, because - // the symbol is already dead; this, however, is fine because we can - // still find the node in which it collapsed to null previously. - report.addVisitor(*DV, - InputNode); - } - - if (auto KV = V.getAs()) - report.addVisitor( - *KV, R, EnableNullFPSuppression, TKind, SFC); - return true; - } - } - - // If the expression is not an "lvalue expression", we can still - // track the constraints on its contents. - SVal V = LVState->getSValAsScalarOrLoc(Inner, LVNode->getLocationContext()); - - ReturnVisitor::addVisitorIfNecessary( - LVNode, Inner, report, EnableNullFPSuppression, TKind); - - // Is it a symbolic value? - if (auto L = V.getAs()) { - // FIXME: this is a hack for fixing a later crash when attempting to - // dereference a void* pointer. - // We should not try to dereference pointers at all when we don't care - // what is written inside the pointer. - bool CanDereference = true; - if (const auto *SR = L->getRegionAs()) { - if (SR->getSymbol()->getType()->getPointeeType()->isVoidType()) - CanDereference = false; - } else if (L->getRegionAs()) - CanDereference = false; - - // At this point we are dealing with the region's LValue. - // However, if the rvalue is a symbolic region, we should track it as well. - // Try to use the correct type when looking up the value. - SVal RVal; - if (ExplodedGraph::isInterestingLValueExpr(Inner)) - RVal = LVState->getRawSVal(L.getValue(), Inner->getType()); - else if (CanDereference) - RVal = LVState->getSVal(L->getRegion()); - - if (CanDereference) { - report.addVisitor(L->getRegion()); - - if (auto KV = RVal.getAs()) - report.addVisitor( - *KV, L->getRegion(), EnableNullFPSuppression, TKind, SFC); - } - - const MemRegion *RegionRVal = RVal.getAsRegion(); - if (isa_and_nonnull(RegionRVal)) { - report.markInteresting(RegionRVal, TKind); - report.addVisitor(loc::MemRegionVal(RegionRVal), - /*assumption=*/false); - } - } - - if (Inner->isRValue()) - trackRValueExpression(LVNode, Inner, report, TKind, - EnableNullFPSuppression); + return Tracker::create(report) + ->track(E, InputNode, {TKind, EnableNullFPSuppression}) + .FoundSomethingToTrack; +} - return true; +void bugreporter::trackStoredValue(SVal V, const MemRegion *R, + PathSensitiveBugReport &Report, + TrackingOptions Opts, + const StackFrameContext *Origin) { + Tracker::create(Report)->track(V, R, Opts, Origin); } //===----------------------------------------------------------------------===//