r326249 - [analyzer] MallocChecker: Suppress false positives in shared pointers.
Artem Dergachev via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 27 13:19:33 PST 2018
Author: dergachev
Date: Tue Feb 27 13:19:33 2018
New Revision: 326249
URL: http://llvm.org/viewvc/llvm-project?rev=326249&view=rev
Log:
[analyzer] MallocChecker: Suppress false positives in shared pointers.
Throw away MallocChecker warnings that occur after releasing a pointer within a
destructor (or its callees) after performing C11 atomic fetch_add or fetch_sub
within that destructor (or its callees).
This is an indication that the destructor's class is likely a
reference-counting pointer. The analyzer is not able to understand that the
original reference count is usually large enough to avoid most use-after-frees.
Even when the smart pointer is a local variable, we still have these false
positives that this patch suppresses, because the analyzer doesn't currently
support atomics well enough.
Differential Revision: https://reviews.llvm.org/D43791
Added:
cfe/trunk/test/Analysis/NewDelete-atomics.cpp
Modified:
cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp?rev=326249&r1=326248&r2=326249&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp Tue Feb 27 13:19:33 2018
@@ -446,15 +446,24 @@ private:
// 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);
}
@@ -2822,6 +2831,32 @@ static SymbolRef findFailedReallocSymbol
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;
+
+ const LocationContext *CurrentLC = N->getLocationContext();
+
+ // If we find an atomic fetch_add or fetch_sub within the destructor in which
+ // the pointer was released (before the release), 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.
+ if (ReleaseDestructorLC) {
+ if (const auto *AE = dyn_cast<AtomicExpr>(S)) {
+ AtomicExpr::AtomicOp Op = AE->getOp();
+ if (Op == AtomicExpr::AO__c11_atomic_fetch_add ||
+ Op == AtomicExpr::AO__c11_atomic_fetch_sub) {
+ if (ReleaseDestructorLC == CurrentLC ||
+ ReleaseDestructorLC->isParentOf(CurrentLC)) {
+ BR.markInvalid(getTag(), S);
+ }
+ }
+ }
+ }
+
ProgramStateRef state = N->getState();
ProgramStateRef statePrev = PrevN->getState();
@@ -2830,10 +2865,6 @@ std::shared_ptr<PathDiagnosticPiece> Mal
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 +2880,24 @@ std::shared_ptr<PathDiagnosticPiece> Mal
Msg = "Memory is released";
StackHint = new StackHintGeneratorForSymbol(Sym,
"Returning; memory was released");
+
+ // See if we're releasing memory while inlining a destructor (or one of
+ // its callees). If so, enable the atomic-related suppression within that
+ // destructor (and all of its callees), which would kick in while visiting
+ // other nodes (the visit order is from the bug to the graph root).
+ 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();
+ // It is unlikely that releasing memory is delegated to a destructor
+ // inside a destructor of a shared pointer, because it's fairly hard
+ // to pass the information that the pointer indeed needs to be
+ // released into it. So we're only interested in the innermost
+ // destructor.
+ break;
+ }
+ }
} else if (isRelinquished(RS, RSPrev, S)) {
Msg = "Memory ownership is transferred";
StackHint = new StackHintGeneratorForSymbol(Sym, "");
Added: cfe/trunk/test/Analysis/NewDelete-atomics.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/NewDelete-atomics.cpp?rev=326249&view=auto
==============================================================================
--- cfe/trunk/test/Analysis/NewDelete-atomics.cpp (added)
+++ cfe/trunk/test/Analysis/NewDelete-atomics.cpp Tue Feb 27 13:19:33 2018
@@ -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
+}
More information about the cfe-commits
mailing list