diff --git a/clang/lib/StaticAnalyzer/Checkers/SmartPtr.h b/clang/lib/StaticAnalyzer/Checkers/SmartPtr.h --- a/clang/lib/StaticAnalyzer/Checkers/SmartPtr.h +++ b/clang/lib/StaticAnalyzer/Checkers/SmartPtr.h @@ -28,6 +28,11 @@ const BugType *getNullDereferenceBugType(); +// Returns whether E is of the form a.get(), where the MemRegion corresponding +// to the smart-ptr a is ThisRegion. +bool isExprObtainedFromGet(const ProgramStateRef State, + const MemRegion *ThisRegion, const Expr *E); + } // namespace smartptr } // namespace ento } // namespace clang diff --git a/clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp --- a/clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp @@ -76,6 +76,85 @@ } } +// This visitor checks for raw pointers obtained from get() method on a specific +// smart-pointer. After identifying such expressions, it checks whether the SVal +// corresponding to it is constrained to null in the State where the +// null-smart-pointer dereference bug is detected. +class ExprFromGetIsConstrainedVisitor : public BugReporterVisitor { +private: + const MemRegion *ThisRegion; + const ProgramStateRef BugState; + llvm::SmallPtrSet DoneExprs; + +public: + ExprFromGetIsConstrainedVisitor(const ProgramStateRef BugState, + const MemRegion *ThisRegion) + : ThisRegion{ThisRegion}, BugState{BugState}, DoneExprs{} {} + PathDiagnosticPieceRef VisitNode(const ExplodedNode *Node, + BugReporterContext &BRC, + PathSensitiveBugReport &BR) override; + void Profile(llvm::FoldingSetNodeID &ID) const override; +}; + +PathDiagnosticPieceRef +ExprFromGetIsConstrainedVisitor::VisitNode(const ExplodedNode *Node, + BugReporterContext &BRC, + PathSensitiveBugReport &BR) { + const Stmt *S = Node->getStmtForDiagnostics(); + const ProgramStateRef CurrState = Node->getState(); + if (S) { + if (const auto *DS = llvm::dyn_cast(S)) { + for (const Decl *D : DS->getDeclGroup()) { + if (const VarDecl *VD = llvm::dyn_cast(D)) { + const Expr *Init = VD->getInit(); + + // Check whether we are looking at the right expr. + if (!smartptr::isExprObtainedFromGet(BugState, ThisRegion, Init)) + return nullptr; + + const LocationContext *Loc = Node->getLocationContext(); + // We need the original SVal, since get() returns a pointer, which may + // be reassigned to later. + const SVal ExprVal = CurrState->getSVal(Init, Loc); + // Check is ExprVal is null (constrained or otherwise) in the + // BugState. + const ConditionTruthVal IsNull = BugState->isNull(ExprVal); + if (!IsNull.isConstrainedTrue()) + return nullptr; + // If ExprVal was null to begin with, that means the smart-pointer was + // null previous to the call to get(). In that case, we don't need a + // note. + if (ExprVal.isZeroConstant()) + return nullptr; + + // Don't create the note multiple times. + if (DoneExprs.contains(Init)) + return nullptr; + DoneExprs.insert(Init); + + llvm::SmallString<128> Str; + llvm::raw_svector_ostream OS(Str); + if (ThisRegion->canPrintPretty()) { + OS << "Obtained null inner pointer from "; + ThisRegion->printPretty(OS); + } else { + OS << "Obtained null inner pointer here"; + } + PathDiagnosticLocation Pos(S, BRC.getSourceManager(), Loc); + return std::make_shared(Pos, OS.str(), + true); + } + } + } + } + return nullptr; +} + +void ExprFromGetIsConstrainedVisitor::Profile( + llvm::FoldingSetNodeID &ID) const { + ID.Add(ThisRegion); +} + void SmartPtrChecker::reportBug(CheckerContext &C, const MemRegion *DerefRegion, const CallEvent &Call) const { ExplodedNode *ErrNode = C.generateErrorNode(); @@ -86,9 +165,9 @@ explainDereference(OS, DerefRegion, Call); auto R = std::make_unique(NullDereferenceBugType, OS.str(), ErrNode); + R->addVisitor(std::make_unique( + ErrNode->getState(), DerefRegion)); R->markInteresting(DerefRegion); - const Expr *BugExpr = Call.getOriginExpr(); - bugreporter::trackExpressionValue(ErrNode, BugExpr, *R); C.emitReport(std::move(R)); } diff --git a/clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp b/clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp --- a/clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp @@ -18,6 +18,7 @@ #include "clang/AST/DeclarationName.h" #include "clang/AST/ExprCXX.h" #include "clang/AST/Type.h" +#include "clang/Analysis/AnalysisDeclContext.h" #include "clang/Basic/LLVM.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" @@ -25,10 +26,15 @@ #include "clang/StaticAnalyzer/Core/CheckerManager.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/Environment.h" #include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h" #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h" #include "clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h" #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h" +#include "llvm/ADT/ImmutableSet.h" #include using namespace clang; @@ -76,10 +82,14 @@ {{"release"}, &SmartPtrModeling::handleRelease}, {{"swap", 1}, &SmartPtrModeling::handleSwap}, {{"get"}, &SmartPtrModeling::handleGet}}; + mutable llvm::ImmutableSet::Factory StmtSetFactory; }; } // end of anonymous namespace REGISTER_MAP_WITH_PROGRAMSTATE(TrackedRegionMap, const MemRegion *, SVal) +// REGISTER_SET_WITH_PROGRAMSTATE(ExprsFromGet, const Stmt *) +REGISTER_MAP_WITH_PROGRAMSTATE(ExprsFromGet, const MemRegion *, + llvm::ImmutableSet) // Define the inter-checker API. namespace clang { @@ -106,6 +116,12 @@ return InnerPointVal && !State->assume(InnerPointVal->castAs(), true); } + +bool isExprObtainedFromGet(const ProgramStateRef State, + const MemRegion *ThisRegion, const Expr *E) { + const auto *ExprSet = State->get(ThisRegion); + return ExprSet && ExprSet->contains(E); +} } // namespace smartptr } // namespace ento } // namespace clang @@ -446,10 +462,10 @@ return; SVal InnerPointerVal; + const auto *CallExpr = Call.getOriginExpr(); if (const auto *InnerValPtr = State->get(ThisRegion)) { InnerPointerVal = *InnerValPtr; } else { - const auto *CallExpr = Call.getOriginExpr(); InnerPointerVal = C.getSValBuilder().conjureSymbolVal( CallExpr, C.getLocationContext(), Call.getResultType(), C.blockCount()); State = State->set(ThisRegion, InnerPointerVal); @@ -458,21 +474,18 @@ State = State->BindExpr(Call.getOriginExpr(), C.getLocationContext(), InnerPointerVal); - C.addTransition(State, C.getNoteTag([ThisRegion, InnerPointerVal, - State](PathSensitiveBugReport &BR, - llvm::raw_ostream &OS) { - if (&BR.getBugType() != smartptr::getNullDereferenceBugType() || - !BR.isInteresting(ThisRegion)) - return; - if (!BR.isInteresting(InnerPointerVal) || !BR.isInteresting(InnerPointerVal.getAsSymbol())) - return; - if (ThisRegion->canPrintPretty()) { - OS << "Obtained null inner pointer from"; - checkAndPrettyPrintRegion(OS, ThisRegion); - } else { - OS << "Obtained null inner pointer here"; - } - })); + // Store the CallExpr as being obtained through get. It will be necessary in + // isExprObtainedFromGet(). + if (const auto *StmtSet = State->get(ThisRegion)) { + auto NewStmtSet = StmtSetFactory.add(*StmtSet, CallExpr); + State = State->set(ThisRegion, NewStmtSet); + } else { + auto EmptySet = StmtSetFactory.getEmptySet(); + auto NewStmtSet = StmtSetFactory.add(EmptySet, CallExpr); + State = State->set(ThisRegion, NewStmtSet); + } + + C.addTransition(State); // The note is added later by a visitor. } bool SmartPtrModeling::handleAssignOp(const CallEvent &Call, diff --git a/clang/test/Analysis/smart-ptr-text-output.cpp b/clang/test/Analysis/smart-ptr-text-output.cpp --- a/clang/test/Analysis/smart-ptr-text-output.cpp +++ b/clang/test/Analysis/smart-ptr-text-output.cpp @@ -321,6 +321,13 @@ // expected-note@-1{{Dereference of null smart pointer 'P'}} } +void getShouldNotLeaveANoteAfterReset(std::unique_ptr P) { + A *a = P.get(); + P.reset(); // expected-note {{Smart pointer 'P' reset using a null value}} + P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}} + // expected-note@-1{{Dereference of null smart pointer 'P'}} +} + void getShouldNotLeaveNoteWhenPtrNotUsed(std::unique_ptr P) { A *a = P.get(); if (!P) { // expected-note {{Taking true branch}}