NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs.
This patch is a targeted suppression heuristic for false positives
`MallocChecker` produces when a shared / reference-counting pointer is copied
(including, but not limited to, `llvm::IntrusiveRefCntPtr`). The program
increments the reference count through an atomic `fetch_add` (which is often
the C++11 `std::atomic<T>::fetch_add()` that executes the respective C11 atomic
when inlined), then decrements it via `fetch_sub` (or via `fetch_add` while
adding -1), then sees if the reference count is zero by comparing the value
returned by `fetch_sub` to 1, then deletes the object if the reference count is
indeed zero.
These false positives get amplified by inlining temporary destructors, but even
in code without any temporary destructors these positives are reproducible, as
the tests suggest.
We cannot easily model the comparison to 1 correctly: even if we model all
atomic expressions precisely, the original reference count may still have been
symbolic to begin with. And if we tried to assume that no overflows occur on
reference counts, this would still require pattern-matching heuristics to
figure out if a certain variable is a reference counter.
The proposed fix is to suppress `MallocChecker` positives that are caused by
releasing memory in a destructor stack frame (or its children stack frames)
after performing any C11 atomic `fetch_add` or `fetch_sub` in that destructor's
stack frame (or its children stack frames). This is done in a visitor
suppression.
Repository:
rC Clang
https://reviews.llvm.org/D43791
Files:
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
test/Analysis/NewDelete-atomics.cpp
Index: test/Analysis/NewDelete-atomics.cpp
===================================================================
--- /dev/null
+++ test/Analysis/NewDelete-atomics.cpp
@@ -0,0 +1,74 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDeleteLeaks -DLEAKS -std=c++11 -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDelete -std=c++11 -DTEST_INLINABLE_ALLOCATORS -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus.NewDeleteLeaks -DLEAKS -std=c++11 -DTEST_INLINABLE_ALLOCATORS -verify %s
+
+// expected-no-diagnostics
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+typedef enum memory_order {
+ memory_order_relaxed = __ATOMIC_RELAXED,
+ memory_order_consume = __ATOMIC_CONSUME,
+ memory_order_acquire = __ATOMIC_ACQUIRE,
+ memory_order_release = __ATOMIC_RELEASE,
+ memory_order_acq_rel = __ATOMIC_ACQ_REL,
+ memory_order_seq_cst = __ATOMIC_SEQ_CST
+} memory_order;
+
+class Obj {
+ int RefCnt;
+
+public:
+ int incRef() {
+ return __c11_atomic_fetch_add((volatile _Atomic(int) *)&RefCnt, 1,
+ memory_order_relaxed);
+ }
+
+ int decRef() {
+ return __c11_atomic_fetch_sub((volatile _Atomic(int) *)&RefCnt, 1,
+ memory_order_relaxed);
+ }
+
+ void foo();
+};
+
+class IntrusivePtr {
+ Obj *Ptr;
+
+public:
+ IntrusivePtr(Obj *Ptr) : Ptr(Ptr) {
+ Ptr->incRef();
+ }
+
+ IntrusivePtr(const IntrusivePtr &Other) : Ptr(Other.Ptr) {
+ Ptr->incRef();
+ }
+
+ ~IntrusivePtr() {
+ // We should not take the path on which the object is deleted.
+ if (Ptr->decRef() == 1)
+ delete Ptr;
+ }
+
+ Obj *getPtr() const { return Ptr; }
+};
+
+void testDestroyLocalRefPtr() {
+ IntrusivePtr p1(new Obj());
+ {
+ IntrusivePtr p2(p1);
+ }
+
+ // p1 still maintains ownership. The object is not deleted.
+ p1.getPtr()->foo(); // no-warning
+}
+
+void testDestroySymbolicRefPtr(const IntrusivePtr &p1) {
+ {
+ IntrusivePtr p2(p1);
+ }
+
+ // p1 still maintains ownership. The object is not deleted.
+ p1.getPtr()->foo(); // no-warning
+}
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===================================================================
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -446,15 +446,24 @@
// A symbol from when the primary region should have been reallocated.
SymbolRef FailedReallocSymbol;
+ // A C++ destructor stack frame in which memory was released. Used for
+ // miscellaneous false positive suppression.
+ const StackFrameContext *ReleaseDestructorLC;
+
bool IsLeak;
public:
MallocBugVisitor(SymbolRef S, bool isLeak = false)
- : Sym(S), Mode(Normal), FailedReallocSymbol(nullptr), IsLeak(isLeak) {}
+ : Sym(S), Mode(Normal), FailedReallocSymbol(nullptr),
+ ReleaseDestructorLC(nullptr), IsLeak(isLeak) {}
+
+ static void *getTag() {
+ static int Tag = 0;
+ return &Tag;
+ }
void Profile(llvm::FoldingSetNodeID &ID) const override {
- static int X = 0;
- ID.AddPointer(&X);
+ ID.AddPointer(getTag());
ID.AddPointer(Sym);
}
@@ -515,6 +524,9 @@
}
private:
+ bool shouldSuppressOnAtomicReferenceCountingPointers(
+ const Stmt *S, const LocationContext *CurrentLC);
+
class StackHintGeneratorForReallocationFailed
: public StackHintGeneratorForSymbol {
public:
@@ -2819,21 +2831,53 @@
return nullptr;
}
+bool MallocChecker::MallocBugVisitor::
+ shouldSuppressOnAtomicReferenceCountingPointers(
+ const Stmt *S, const LocationContext *CurrentLC) {
+ // If we find an atomic fetch_add or fetch_sub within the destructor in which
+ // the pointer was released, this is likely a destructor of a shared pointer.
+ // Because we don't model atomics, and also because we don't know that the
+ // original reference count is positive, we should not report use-after-frees
+ // on objects deleted in such destructors. This can probably be improved
+ // through better shared pointer modeling.
+ const auto *AE = dyn_cast<AtomicExpr>(S);
+ if (!AE)
+ return false;
+
+ AtomicExpr::AtomicOp Op = AE->getOp();
+ if (Op != AtomicExpr::AO__c11_atomic_fetch_add &&
+ Op != AtomicExpr::AO__c11_atomic_fetch_sub) {
+ return false;
+ }
+
+ for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent())
+ if (LC == ReleaseDestructorLC)
+ return true;
+
+ return false;
+}
+
std::shared_ptr<PathDiagnosticPiece> MallocChecker::MallocBugVisitor::VisitNode(
const ExplodedNode *N, const ExplodedNode *PrevN, BugReporterContext &BRC,
BugReport &BR) {
+ const Stmt *S = PathDiagnosticLocation::getStmt(N);
+ if (!S)
+ return nullptr;
+
+ // Suppression for common use-after-free false positives that are caused
+ // by inlining shared pointer destructors.
+ const LocationContext *CurrentLC = N->getLocationContext();
+ if (shouldSuppressOnAtomicReferenceCountingPointers(S, CurrentLC))
+ BR.markInvalid(getTag(), S);
+
ProgramStateRef state = N->getState();
ProgramStateRef statePrev = PrevN->getState();
const RefState *RS = state->get<RegionState>(Sym);
const RefState *RSPrev = statePrev->get<RegionState>(Sym);
if (!RS)
return nullptr;
- const Stmt *S = PathDiagnosticLocation::getStmt(N);
- if (!S)
- return nullptr;
-
// FIXME: We will eventually need to handle non-statement-based events
// (__attribute__((cleanup))).
@@ -2849,6 +2893,17 @@
Msg = "Memory is released";
StackHint = new StackHintGeneratorForSymbol(Sym,
"Returning; memory was released");
+
+ // See if memory is released in a destructor. If so, store the destructor
+ // to find out if a likely-false-positive suppression should kick in.
+ for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
+ if (isa<CXXDestructorDecl>(LC->getDecl())) {
+ assert(!ReleaseDestructorLC &&
+ "There can be only one release point!");
+ ReleaseDestructorLC = LC->getCurrentStackFrame();
+ break;
+ }
+ }
} else if (isRelinquished(RS, RSPrev, S)) {
Msg = "Memory ownership is transferred";
StackHint = new StackHintGeneratorForSymbol(Sym, "");
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
