RedDocMD updated this revision to Diff 332538.
RedDocMD added a comment.
Changed approach to use visitor
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D97183/new/
https://reviews.llvm.org/D97183
Files:
clang/lib/StaticAnalyzer/Checkers/SmartPtr.h
clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
clang/test/Analysis/smart-ptr-text-output.cpp
Index: clang/test/Analysis/smart-ptr-text-output.cpp
===================================================================
--- clang/test/Analysis/smart-ptr-text-output.cpp
+++ 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<A> 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<A> P) {
A *a = P.get();
if (!P) { // expected-note {{Taking true branch}}
Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ 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 <string>
using namespace clang;
@@ -76,10 +82,14 @@
{{"release"}, &SmartPtrModeling::handleRelease},
{{"swap", 1}, &SmartPtrModeling::handleSwap},
{{"get"}, &SmartPtrModeling::handleGet}};
+ mutable llvm::ImmutableSet<const Expr *>::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<const Expr *>)
// Define the inter-checker API.
namespace clang {
@@ -106,6 +116,12 @@
return InnerPointVal &&
!State->assume(InnerPointVal->castAs<DefinedOrUnknownSVal>(), true);
}
+
+bool isExprObtainedFromGet(const ProgramStateRef State,
+ const MemRegion *ThisRegion, const Expr *E) {
+ const auto *ExprSet = State->get<ExprsFromGet>(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<TrackedRegionMap>(ThisRegion)) {
InnerPointerVal = *InnerValPtr;
} else {
- const auto *CallExpr = Call.getOriginExpr();
InnerPointerVal = C.getSValBuilder().conjureSymbolVal(
CallExpr, C.getLocationContext(), Call.getResultType(), C.blockCount());
State = State->set<TrackedRegionMap>(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<ExprsFromGet>(ThisRegion)) {
+ auto NewStmtSet = StmtSetFactory.add(*StmtSet, CallExpr);
+ State = State->set<ExprsFromGet>(ThisRegion, NewStmtSet);
+ } else {
+ auto EmptySet = StmtSetFactory.getEmptySet();
+ auto NewStmtSet = StmtSetFactory.add(EmptySet, CallExpr);
+ State = State->set<ExprsFromGet>(ThisRegion, NewStmtSet);
+ }
+
+ C.addTransition(State); // The note is added later by a visitor.
}
bool SmartPtrModeling::handleAssignOp(const CallEvent &Call,
Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp
+++ 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<const Expr *, 16> 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<DeclStmt>(S)) {
+ for (const Decl *D : DS->getDeclGroup()) {
+ if (const VarDecl *VD = llvm::dyn_cast<VarDecl>(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<PathDiagnosticEventPiece>(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<PathSensitiveBugReport>(NullDereferenceBugType,
OS.str(), ErrNode);
+ R->addVisitor(std::make_unique<ExprFromGetIsConstrainedVisitor>(
+ ErrNode->getState(), DerefRegion));
R->markInteresting(DerefRegion);
- const Expr *BugExpr = Call.getOriginExpr();
- bugreporter::trackExpressionValue(ErrNode, BugExpr, *R);
C.emitReport(std::move(R));
}
Index: clang/lib/StaticAnalyzer/Checkers/SmartPtr.h
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/SmartPtr.h
+++ 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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
