vrnithinkumar updated this revision to Diff 273568.
vrnithinkumar marked 16 inline comments as done.
vrnithinkumar added a comment.
Addressing review comments
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81315/new/
https://reviews.llvm.org/D81315
Files:
clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
clang/test/Analysis/Inputs/system-header-simulator-cxx.h
clang/test/Analysis/analyzer-config.c
clang/test/Analysis/smart-ptr.cpp
Index: clang/test/Analysis/smart-ptr.cpp
===================================================================
--- clang/test/Analysis/smart-ptr.cpp
+++ clang/test/Analysis/smart-ptr.cpp
@@ -1,5 +1,6 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection\
// RUN: -analyzer-checker cplusplus.Move,cplusplus.SmartPtr\
+// RUN: -analyzer-config cplusplus.SmartPtr:CheckSmartPtrDereference=true\
// RUN: -std=c++11 -verify %s
#include "Inputs/system-header-simulator-cxx.h"
@@ -10,7 +11,7 @@
std::unique_ptr<int> Q = std::move(P);
if (Q)
clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
- *Q.get() = 1; // no-warning
+ *Q.get() = 1; // no-warning
if (P)
clang_analyzer_warnIfReached(); // no-warning
// TODO: Report a null dereference (instead).
@@ -26,3 +27,70 @@
(s->*func)(); // no-crash
}
} // namespace testUnknownCallee
+
+class A {
+public:
+ A(){};
+ void foo();
+};
+
+A *return_null() {
+ return nullptr;
+}
+
+void derefAfterValidCtr() {
+ std::unique_ptr<A> P(new A());
+ P->foo(); // No warning.
+}
+
+void derefAfterDefaultCtr() {
+ std::unique_ptr<A> P;
+ P->foo(); // expected-warning {{Dereference of null smart pointer [cplusplus.SmartPtr]}}
+}
+
+void derefAfterCtrWithNull() {
+ std::unique_ptr<A> P(nullptr);
+ *P; // expected-warning {{Dereference of null smart pointer [cplusplus.SmartPtr]}}
+}
+
+void derefAfterCtrWithNullReturnMethod() {
+ std::unique_ptr<A> P(return_null());
+ P->foo(); // expected-warning {{Dereference of null smart pointer [cplusplus.SmartPtr]}}
+}
+
+void derefAfterRelease() {
+ std::unique_ptr<A> P(new A());
+ P.release();
+ P->foo(); // expected-warning {{Dereference of null smart pointer [cplusplus.SmartPtr]}}
+}
+
+void derefAfterReset() {
+ std::unique_ptr<A> P(new A());
+ P.reset();
+ P->foo(); // expected-warning {{Dereference of null smart pointer [cplusplus.SmartPtr]}}
+}
+
+void derefAfterResetWithNull() {
+ std::unique_ptr<A> P(new A());
+ P.reset(nullptr);
+ P->foo(); // expected-warning {{Dereference of null smart pointer [cplusplus.SmartPtr]}}
+}
+
+void derefAfterResetWithNonNull() {
+ std::unique_ptr<A> P;
+ P.reset(new A());
+ P->foo(); // No warning.
+}
+
+void derefAfterReleaseAndResetWithNonNull() {
+ std::unique_ptr<A> P(new A());
+ P.release();
+ P.reset(new A());
+ P->foo(); // No warning.
+}
+
+void derefOnReleasedNullRawPtr() {
+ std::unique_ptr<A> P;
+ A *AP = P.release();
+ AP->foo(); // expected-warning {{Called C++ object pointer is null [core.CallAndMessage]}}
+}
Index: clang/test/Analysis/analyzer-config.c
===================================================================
--- clang/test/Analysis/analyzer-config.c
+++ clang/test/Analysis/analyzer-config.c
@@ -39,6 +39,7 @@
// CHECK-NEXT: core.CallAndMessage:ParameterCount = true
// CHECK-NEXT: core.CallAndMessage:UndefReceiver = true
// CHECK-NEXT: cplusplus.Move:WarnOn = KnownsAndLocals
+// CHECK-NEXT: cplusplus.SmartPtr:CheckSmartPtrDereference = false
// CHECK-NEXT: crosscheck-with-z3 = false
// CHECK-NEXT: ctu-dir = ""
// CHECK-NEXT: ctu-import-threshold = 100
Index: clang/test/Analysis/Inputs/system-header-simulator-cxx.h
===================================================================
--- clang/test/Analysis/Inputs/system-header-simulator-cxx.h
+++ clang/test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -946,10 +946,15 @@
template <typename T> // TODO: Implement the stub for deleter.
class unique_ptr {
public:
+ unique_ptr() {}
+ unique_ptr(T *) {}
unique_ptr(const unique_ptr &) = delete;
unique_ptr(unique_ptr &&);
T *get() const;
+ T *release() const;
+ void reset(T *p = nullptr) const;
+ void swap(unique_ptr<T> &p) const;
typename std::add_lvalue_reference<T>::type operator*() const;
T *operator->() const;
Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -13,26 +13,60 @@
#include "Move.h"
+#include "clang/AST/DeclCXX.h"
#include "clang/AST/ExprCXX.h"
+#include "clang/AST/Type.h"
#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
#include "clang/StaticAnalyzer/Core/Checker.h"
#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/SVals.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h"
using namespace clang;
using namespace ento;
namespace {
-class SmartPtrModeling : public Checker<eval::Call> {
+class SmartPtrModeling : public Checker<eval::Call, check::DeadSymbols> {
+ // Set of STL smart pointer class which we are trying to model.
+ const llvm::StringSet<> StdSmartPtrs = {
+ "shared_ptr",
+ "unique_ptr",
+ "weak_ptr",
+ };
bool isNullAfterMoveMethod(const CallEvent &Call) const;
+ BugType NullDereferenceBugType{this, "Null SmartPtr dereference",
+ "C++ Smart Pointer"};
public:
+ // Whether the checker should check for null dereferences of smart pointers.
+ DefaultBool ShouldCheckSmartPtrDereference;
bool evalCall(const CallEvent &Call, CheckerContext &C) const;
+ void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
+
+private:
+ void reportBug(CheckerContext &C, const CallEvent &Call) const;
+ bool isStdSmartPointerClass(const CallEvent &Call) const;
+ void updateTrackedRegion(const CallEvent &Call, CheckerContext &C,
+ const MemRegion *Region) const;
+ void handleReset(const CallEvent &Call, CheckerContext &C) const;
+ void handleRelease(const CallEvent &Call, CheckerContext &C) const;
+ void handleSwap(const CallEvent &Call, CheckerContext &C) const;
+ bool checkDeferenceOps(const CallEvent &Call, CheckerContext &C) const;
+
+ using SmartPtrMethodHandlerFn =
+ void (SmartPtrModeling::*)(const CallEvent &Call, CheckerContext &) const;
+ CallDescriptionMap<SmartPtrMethodHandlerFn> SmartPtrMethodHandlers{
+ {{"reset"}, &SmartPtrModeling::handleReset},
+ {{"release"}, &SmartPtrModeling::handleRelease},
+ {{"swap", 1}, &SmartPtrModeling::handleSwap}};
};
} // end of anonymous namespace
+REGISTER_MAP_WITH_PROGRAMSTATE(TrackedRegionMap, const MemRegion *, SVal)
+
bool SmartPtrModeling::isNullAfterMoveMethod(const CallEvent &Call) const {
// TODO: Update CallDescription to support anonymous calls?
// TODO: Handle other methods, such as .get() or .release().
@@ -44,27 +78,185 @@
bool SmartPtrModeling::evalCall(const CallEvent &Call,
CheckerContext &C) const {
- if (!isNullAfterMoveMethod(Call))
+
+ if (!isStdSmartPointerClass(Call))
return false;
- ProgramStateRef State = C.getState();
- const MemRegion *ThisR =
- cast<CXXInstanceCall>(&Call)->getCXXThisVal().getAsRegion();
+ if (isNullAfterMoveMethod(Call)) {
+ ProgramStateRef State = C.getState();
+ const MemRegion *ThisR =
+ cast<CXXInstanceCall>(&Call)->getCXXThisVal().getAsRegion();
+
+ if (!move::isMovedFrom(State, ThisR)) {
+ // TODO: Model this case as well. At least, avoid invalidation of globals.
+ return false;
+ }
+
+ // TODO: Add a note to bug reports describing this decision.
+ C.addTransition(
+ State->BindExpr(Call.getOriginExpr(), C.getLocationContext(),
+ C.getSValBuilder().makeZeroVal(Call.getResultType())));
+ return true;
+ }
- if (!move::isMovedFrom(State, ThisR)) {
- // TODO: Model this case as well. At least, avoid invalidation of globals.
+ if (!ShouldCheckSmartPtrDereference)
return false;
+
+ if (const auto *CC = dyn_cast<CXXConstructorCall>(&Call)) {
+ if (CC->getDecl()->isCopyOrMoveConstructor())
+ return false;
+
+ const MemRegion *ThisValRegion = CC->getCXXThisVal().getAsRegion();
+ if (!ThisValRegion)
+ return false;
+
+ updateTrackedRegion(Call, C, ThisValRegion);
+ return true;
}
- // TODO: Add a note to bug reports describing this decision.
- C.addTransition(
- State->BindExpr(Call.getOriginExpr(), C.getLocationContext(),
- C.getSValBuilder().makeZeroVal(Call.getResultType())));
+ if (checkDeferenceOps(Call, C))
+ return true;
+
+ const SmartPtrMethodHandlerFn *Handler = SmartPtrMethodHandlers.lookup(Call);
+ if (!Handler)
+ return false;
+ (this->**Handler)(Call, C);
+
+ return C.isDifferent();
+}
+
+void SmartPtrModeling::checkDeadSymbols(SymbolReaper &SymReaper,
+ CheckerContext &C) const {
+ ProgramStateRef State = C.getState();
+ // Clean up dead regions from the region map.
+ TrackedRegionMapTy TrackedRegions = State->get<TrackedRegionMap>();
+ for (auto E : TrackedRegions) {
+ const MemRegion *Region = E.first;
+ bool IsRegDead = !SymReaper.isLiveRegion(Region);
+
+ if (IsRegDead)
+ State = State->remove<TrackedRegionMap>(Region);
+ }
+ C.addTransition(State);
+}
+
+bool SmartPtrModeling::checkDeferenceOps(const CallEvent &Call,
+ CheckerContext &C) const {
+ ProgramStateRef State = C.getState();
+ const auto *OC = dyn_cast<CXXMemberOperatorCall>(&Call);
+ if (!OC)
+ return false;
+ const MemRegion *ThisRegion = OC->getCXXThisVal().getAsRegion();
+ if (!ThisRegion)
+ return false;
+
+ const auto *MethodDecl = dyn_cast_or_null<CXXMethodDecl>(OC->getDecl());
+
+ if (!MethodDecl || !MethodDecl->isOverloadedOperator())
+ return false;
+
+ OverloadedOperatorKind OOK = MethodDecl->getOverloadedOperator();
+ if (OOK == OO_Star || OOK == OO_Arrow) {
+ const auto *InnerPointVal = State->get<TrackedRegionMap>(ThisRegion);
+ if (InnerPointVal && InnerPointVal->isZeroConstant()) {
+ reportBug(C, Call);
+ }
+ }
return true;
}
+void SmartPtrModeling::handleReset(const CallEvent &Call,
+ CheckerContext &C) const {
+ const auto *IC = dyn_cast<CXXInstanceCall>(&Call);
+ if (!IC)
+ return;
+
+ const MemRegion *ThisValRegion = IC->getCXXThisVal().getAsRegion();
+ if (!ThisValRegion)
+ return;
+ updateTrackedRegion(Call, C, ThisValRegion);
+ // TODO: Make sure to ivalidate the the region in the Store if we don't have
+ // time to model all methods.
+}
+
+void SmartPtrModeling::handleRelease(const CallEvent &Call,
+ CheckerContext &C) const {
+ const auto *IC = dyn_cast<CXXInstanceCall>(&Call);
+ if (!IC)
+ return;
+
+ const MemRegion *ThisValRegion = IC->getCXXThisVal().getAsRegion();
+ if (!ThisValRegion)
+ return;
+ updateTrackedRegion(Call, C, ThisValRegion);
+
+ auto State = C.getState();
+ const auto *InnerPointVal = State->get<TrackedRegionMap>(ThisValRegion);
+ if (InnerPointVal) {
+ State = State->BindExpr(Call.getOriginExpr(), C.getLocationContext(),
+ *InnerPointVal);
+ C.addTransition(State);
+ }
+ // TODO: Add support to enable MallocChecker to start tracking the raw
+ // pointer.
+}
+
+void SmartPtrModeling::handleSwap(const CallEvent &Call,
+ CheckerContext &C) const {
+ // TODO: Add support to handle swap method.
+}
+
+bool SmartPtrModeling::isStdSmartPointerClass(const CallEvent &Call) const {
+ const auto *MethodDecl = dyn_cast_or_null<CXXMethodDecl>(Call.getDecl());
+ if (!MethodDecl || !MethodDecl->getParent())
+ return false;
+
+ const auto *RecordDecl = MethodDecl->getParent();
+ if (!RecordDecl || !RecordDecl->getDeclContext()->isStdNamespace())
+ return false;
+
+ if (RecordDecl->getDeclName().isIdentifier()) {
+ return StdSmartPtrs.count(RecordDecl->getName().lower());
+ }
+ return false;
+}
+
+void SmartPtrModeling::reportBug(CheckerContext &C,
+ const CallEvent &Call) const {
+ ExplodedNode *ErrNode = C.generateErrorNode();
+ if (!ErrNode)
+ return;
+
+ auto R = std::make_unique<PathSensitiveBugReport>(
+ NullDereferenceBugType, "Dereference of null smart pointer", ErrNode);
+ R->addRange(Call.getSourceRange());
+ C.emitReport(std::move(R));
+}
+
+void SmartPtrModeling::updateTrackedRegion(
+ const CallEvent &Call, CheckerContext &C,
+ const MemRegion *ThisValRegion) const {
+ auto State = C.getState();
+ auto NumArgs = Call.getNumArgs();
+
+ if (NumArgs == 0) {
+ auto NullSVal = C.getSValBuilder().makeNull();
+ State = State->set<TrackedRegionMap>(ThisValRegion, NullSVal);
+ } else if (NumArgs == 1) {
+ auto ArgVal = Call.getArgSVal(0);
+ assert(Call.getArgExpr(0)->getType()->isPointerType() &&
+ "Adding a non pointer value to TrackedRegionMap");
+ State = State->set<TrackedRegionMap>(ThisValRegion, ArgVal);
+ }
+
+ C.addTransition(State);
+}
+
void ento::registerSmartPtrModeling(CheckerManager &Mgr) {
- Mgr.registerChecker<SmartPtrModeling>();
+ auto *Checker = Mgr.registerChecker<SmartPtrModeling>();
+ Checker->ShouldCheckSmartPtrDereference =
+ Mgr.getAnalyzerOptions().getCheckerBooleanOption(
+ Checker, "CheckSmartPtrDereference");
}
bool ento::shouldRegisterSmartPtrModeling(const CheckerManager &mgr) {
Index: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
===================================================================
--- clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
+++ clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
@@ -574,6 +574,13 @@
def SmartPtrModeling: Checker<"SmartPtr">,
HelpText<"Model behavior of C++ smart pointers">,
Documentation<NotDocumented>,
+ CheckerOptions<[
+ CmdLineOption<Boolean,
+ "CheckSmartPtrDereference",
+ "Enable check for SmartPtr null dereferences",
+ "false",
+ InAlpha>,
+ ]>,
Hidden;
def MoveChecker: Checker<"Move">,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
