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,6 +165,8 @@ explainDereference(OS, DerefRegion, Call); auto R = std::make_unique(NullDereferenceBugType, OS.str(), ErrNode); + R->addVisitor(std::make_unique( + ErrNode->getState(), DerefRegion)); R->markInteresting(DerefRegion); 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 @@ -29,6 +29,7 @@ #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 +77,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 +111,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 +457,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); @@ -457,8 +468,19 @@ State = State->BindExpr(Call.getOriginExpr(), C.getLocationContext(), InnerPointerVal); - // TODO: Add NoteTag, for how the raw pointer got using 'get' method. - C.addTransition(State); + + // 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 @@ -306,10 +306,51 @@ }; void derefAfterBranchingOnUnknownInnerPtr(std::unique_ptr P) { - A *RP = P.get(); + A *RP = P.get(); // expected-note {{Obtained null inner pointer from 'P'}} if (!RP) { // expected-note {{Assuming 'RP' is null}} // expected-note@-1 {{Taking true branch}} P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}} // expected-note@-1{{Dereference of null smart pointer 'P'}} } } + +void getShouldNotAlwaysLeaveANote() { + std::unique_ptr P; // expected-note {{Default constructed smart pointer 'P' is null}} + A *a = P.get(); + P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}} + // 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}} + // expected-note@-1 {{Assuming smart pointer 'P' is null}} + P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}} + // expected-note@-1{{Dereference of null smart pointer 'P'}} + } +} + +void getShouldLeaveANoteWithWhileLoop(std::unique_ptr P) { + A *RP = P.get(); // expected-note {{Obtained null inner pointer from 'P'}} + while (!RP) { // expected-note {{Assuming 'RP' is null}} + // expected-note@-1 {{Loop condition is true. Entering loop body}} + P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}} + // expected-note@-1{{Dereference of null smart pointer 'P'}} + } +} + +void getShouldLeaveANoteWithForLoop(std::unique_ptr P) { + for (A *RP = P.get(); !RP;) { // expected-note {{Assuming 'RP' is null}} + // expected-note@-1 {{Loop condition is true. Entering loop body}} + // expected-note@-2 {{Obtained null inner pointer from 'P'}} + P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}} + // expected-note@-1{{Dereference of null smart pointer 'P'}} + } +} \ No newline at end of file