RedDocMD updated this revision to Diff 348165.
RedDocMD marked 10 inline comments as done.
RedDocMD added a comment.
More refactoring
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
@@ -306,10 +306,81 @@
};
void derefAfterBranchingOnUnknownInnerPtr(std::unique_ptr<A> 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 multpleDeclsWithGet(std::unique_ptr<A> P) {
+ A *dummy1 = nullptr, *RP = P.get(), *dummy2; // 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 multipleGetsShouldNotAllHaveNotes(std::unique_ptr<A> P) {
+ A *RP = P.get(); // expected-note {{Obtained null inner pointer from 'P'}}
+ A *dummy1 = P.get();
+ A *dummy2 = P.get();
+ 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<A> 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<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}}
+ // 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<A> 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<A> 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'}}
+ }
+}
+
+void getShouldLeaveNoteOnChaining(std::unique_ptr<A> P) {
+ A *praw = P.get(), *other; // expected-note {{Obtained null inner pointer from 'P'}}
+ other = praw; // expected-note {{Obtained null value here}}
+ if (!other) { // expected-note {{Assuming 'other' 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'}}
+ }
+}
\ No newline at end of file
Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -76,11 +76,18 @@
{{"release"}, &SmartPtrModeling::handleRelease},
{{"swap", 1}, &SmartPtrModeling::handleSwap},
{{"get"}, &SmartPtrModeling::handleGet}};
+
+ // TODO: Get rid after D97183
+ mutable llvm::ImmutableSet<const Expr *>::Factory StmtSetFactory;
};
} // end of anonymous namespace
REGISTER_MAP_WITH_PROGRAMSTATE(TrackedRegionMap, const MemRegion *, SVal)
+// TODO: Get rid of this onece D97183 is settled
+REGISTER_MAP_WITH_PROGRAMSTATE(ExprsFromGet, const MemRegion *,
+ llvm::ImmutableSet<const Expr *>)
+
// Define the inter-checker API.
namespace clang {
namespace ento {
@@ -101,11 +108,170 @@
return false;
}
+bool isStdSmartPtrCall(const CXXRecordDecl *Rec) {
+ if (!Rec || !Rec->getDeclContext()->isStdNamespace())
+ return false;
+ if (Rec->getDeclName().isIdentifier()) {
+ StringRef Name = Rec->getName();
+ return Name == "shared_ptr" || Name == "unique_ptr" || Name == "weak_ptr";
+ }
+ return false;
+}
+
bool isNullSmartPtr(const ProgramStateRef State, const MemRegion *ThisRegion) {
const auto *InnerPointVal = State->get<TrackedRegionMap>(ThisRegion);
return InnerPointVal &&
!State->assume(InnerPointVal->castAs<DefinedOrUnknownSVal>(), true);
}
+
+// TODO: Get rid after D97183
+bool isExprObtainedFromGet(const ProgramStateRef State,
+ const MemRegion *ThisRegion, const Expr *E) {
+ const auto *ExprSet = State->get<ExprsFromGet>(ThisRegion);
+ return ExprSet && ExprSet->contains(E);
+}
+
+PathDiagnosticPieceRef
+EmitNoteOnGetVisitor::VisitNode(const ExplodedNode *Node,
+ BugReporterContext &BRC,
+ PathSensitiveBugReport &BR) {
+
+ ProgramStateRef State = Node->getState();
+ const SVal *InnerPtr = State->get<TrackedRegionMap>(ThisRegion);
+ if (InnerPtr) {
+ SVal InnerPtrVal = *InnerPtr;
+ ProgramStateRef NullState, NonNullState;
+ std::tie(NullState, NonNullState) =
+ State->assume(InnerPtrVal.castAs<DefinedOrUnknownSVal>());
+
+ // Check whether we have a constraint on ThisRegion
+ if (NullState && NonNullState) {
+ if (const Stmt *S = Node->getStmtForDiagnostics()) {
+ // Skip if we already have covered this statement
+ auto ItInsertedPair = StmtsCovered.insert(S);
+ if (!ItInsertedPair.second)
+ return nullptr;
+
+ // If statement on raw pointer
+ visitIfStmt(Node, State, BRC, S, InnerPtrVal);
+
+ // Assignment operator
+ if (auto Report = visitAssgnStmt(Node, State, BRC, S, InnerPtrVal))
+ return Report;
+
+ // Variable declaration
+ if (auto Report = visitDeclStmt(Node, State, BRC, S, InnerPtrVal))
+ return Report;
+ }
+ }
+ }
+ return nullptr;
+}
+
+PathDiagnosticPieceRef EmitNoteOnGetVisitor::emitBugReportAtGet(
+ const ExplodedNode *Node, BugReporterContext &BRC, const Expr *E) {
+ if (const auto *MCE = llvm::dyn_cast<CXXMemberCallExpr>(E)) {
+ const auto *Method = MCE->getMethodDecl();
+ const auto *Record = MCE->getRecordDecl();
+ if (Method->getName() == "get" && isStdSmartPtrCall(Record)) {
+ 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";
+ }
+ const Stmt *S = Node->getStmtForDiagnostics();
+ PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
+ Node->getLocationContext());
+ return std::make_shared<PathDiagnosticEventPiece>(Pos, OS.str(), true);
+ }
+ }
+ return {};
+}
+
+void EmitNoteOnGetVisitor::visitIfStmt(const ExplodedNode *Node,
+ const ProgramStateRef State,
+ BugReporterContext &BRC, const Stmt *S,
+ const SVal InnerPtrVal) {
+ const auto *E = llvm::dyn_cast<ImplicitCastExpr>(S);
+ if (!E || E->getCastKind() != CastKind::CK_PointerToBoolean)
+ return;
+ // Check if we are tracking the expression being casted
+ const Expr *Sub = E->getSubExpr();
+ SVal ExprVal = State->getSVal(Sub, Node->getLocationContext());
+ if (ExprVal != InnerPtrVal)
+ return;
+ // So we have a pointer being used in an if statement
+ // And that pointer is being tracked to the same SVal as
+ // ThisRegion Also it is at this if statement that the
+ // constraining starts So we know that this pointer points to
+ // P.get()
+ if (const auto *Ptr = llvm::dyn_cast<DeclRefExpr>(Sub->IgnoreParenCasts()))
+ PtrsTrackedAndConstrained.insert(Ptr->getDecl());
+}
+
+PathDiagnosticPieceRef EmitNoteOnGetVisitor::visitAssgnStmt(
+ const ExplodedNode *Node, const ProgramStateRef State,
+ BugReporterContext &BRC, const Stmt *S, const SVal InnerPtrVal) {
+ const auto *BO = llvm::dyn_cast<BinaryOperator>(S);
+ if (!BO || BO->getOpcode() != BO_Assign)
+ return nullptr;
+ const Expr *LHS = BO->getLHS();
+ const auto *DeclRef = llvm::dyn_cast<DeclRefExpr>(LHS);
+ if (!DeclRef || !PtrsTrackedAndConstrained.contains(DeclRef->getDecl()))
+ return nullptr;
+ const Expr *RHS = BO->getRHS();
+ // So now we have an assignment of the form a = b,
+ // where a is known to be tracked and constrained
+
+ // If b is just a pointer, then we should add it to the set
+ if (const auto *ICE = llvm::dyn_cast<ImplicitCastExpr>(RHS)) {
+ const Expr *Sub = ICE->getSubExpr();
+
+ if (const auto *Ptr = llvm::dyn_cast<DeclRefExpr>(Sub)) {
+ PtrsTrackedAndConstrained.insert(Ptr->getDecl());
+ llvm::SmallString<128> Str;
+ llvm::raw_svector_ostream OS(Str);
+ OS << "Obtained null value here";
+ PathDiagnosticLocation Pos(S, BRC.getSourceManager(),
+ Node->getLocationContext());
+ return std::make_shared<PathDiagnosticEventPiece>(Pos, OS.str(), true);
+ }
+ }
+
+ // If b is a get() expression, then we can return a note
+ if (auto Report = emitBugReportAtGet(Node, BRC, RHS))
+ return Report;
+
+ return nullptr;
+}
+
+PathDiagnosticPieceRef EmitNoteOnGetVisitor::visitDeclStmt(
+ const ExplodedNode *Node, const ProgramStateRef State,
+ BugReporterContext &BRC, const Stmt *S, const SVal InnerPtrVal) {
+ const auto *DS = llvm::dyn_cast<DeclStmt>(S);
+ if (!DS)
+ return nullptr;
+ const Decl *D = DS->getSingleDecl();
+ assert(D && "DeclStmt should have at least one Decl");
+ const auto *VD = llvm::dyn_cast<VarDecl>(D);
+ if (!VD)
+ return nullptr;
+ for (const auto *I : PtrsTrackedAndConstrained) {
+ if (I->getName() == VD->getName()) {
+ const Expr *Init = VD->getAnyInitializer();
+ if (!Init)
+ continue;
+ if (auto Report = emitBugReportAtGet(Node, BRC, Init))
+ return Report;
+ break;
+ }
+ }
+ return nullptr;
+}
+
} // namespace smartptr
} // namespace ento
} // namespace clang
@@ -446,10 +612,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);
@@ -457,8 +623,16 @@
State = State->BindExpr(Call.getOriginExpr(), C.getLocationContext(),
InnerPointerVal);
- // TODO: Add NoteTag, for how the raw pointer got using 'get' method.
- C.addTransition(State);
+
+ // TODO: Get rid of this onece D97183 is settled
+ // Store the CallExpr as being obtained through get. It will be necessary in
+ // isExprObtainedFromGet().
+ const auto *ExistingSet = State->get<ExprsFromGet>(ThisRegion);
+ auto BaseSet = ExistingSet ? *ExistingSet : StmtSetFactory.getEmptySet();
+ auto NewStmtSet = StmtSetFactory.add(BaseSet, 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
@@ -86,6 +86,8 @@
explainDereference(OS, DerefRegion, Call);
auto R = std::make_unique<PathSensitiveBugReport>(NullDereferenceBugType,
OS.str(), ErrNode);
+ R->addVisitor(std::make_unique<smartptr::EmitNoteOnGetVisitor>(
+ DerefRegion, C.getSValBuilder()));
R->markInteresting(DerefRegion);
C.emitReport(std::move(R));
}
Index: clang/lib/StaticAnalyzer/Checkers/SmartPtr.h
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/SmartPtr.h
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtr.h
@@ -14,6 +14,7 @@
#ifndef LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_SMARTPTR_H
#define LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_SMARTPTR_H
+#include "clang/AST/DeclCXX.h"
#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
namespace clang {
@@ -22,12 +23,57 @@
/// Returns true if the event call is on smart pointer.
bool isStdSmartPtrCall(const CallEvent &Call);
+bool isStdSmartPtrCall(const CXXRecordDecl *Rec);
/// Returns whether the smart pointer is null or not.
bool isNullSmartPtr(const ProgramStateRef State, const MemRegion *ThisRegion);
const BugType *getNullDereferenceBugType();
+// TODO: Get rid after D97183
+// 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);
+
+class EmitNoteOnGetVisitor : public BugReporterVisitor {
+private:
+ const MemRegion *ThisRegion;
+ SValBuilder &Bldr;
+ llvm::SmallPtrSet<const ValueDecl *, 4> PtrsTrackedAndConstrained;
+ llvm::SmallPtrSet<const Stmt *, 16> StmtsCovered;
+
+public:
+ EmitNoteOnGetVisitor(const MemRegion *ThisRegion, SValBuilder &Bldr)
+ : ThisRegion{ThisRegion}, Bldr{Bldr}, PtrsTrackedAndConstrained{},
+ StmtsCovered{} {}
+ PathDiagnosticPieceRef VisitNode(const ExplodedNode *Node,
+ BugReporterContext &BRC,
+ PathSensitiveBugReport &BR) override;
+
+ PathDiagnosticPieceRef emitBugReportAtGet(const ExplodedNode *Node,
+ BugReporterContext &BRC,
+ const Expr *E);
+
+ void visitIfStmt(const ExplodedNode *Node, const ProgramStateRef State,
+ BugReporterContext &BRC, const Stmt *S,
+ const SVal InnerPtrVal);
+
+ PathDiagnosticPieceRef visitAssgnStmt(const ExplodedNode *Node,
+ const ProgramStateRef State,
+ BugReporterContext &BRC, const Stmt *S,
+ const SVal InnerPtrVal);
+
+ PathDiagnosticPieceRef visitDeclStmt(const ExplodedNode *Node,
+ const ProgramStateRef State,
+ BugReporterContext &BRC, const Stmt *S,
+ const SVal InnerPtrVal);
+
+ void Profile(llvm::FoldingSetNodeID &ID) const override {
+ ID.Add(ThisRegion);
+ }
+};
+
} // 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
