Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp =================================================================== --- lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -2828,6 +2828,21 @@ return nullptr; } +static bool isReferenceCountingPointerDestructor(const CXXDestructorDecl *DD) { + if (const IdentifierInfo *II = DD->getParent()->getIdentifier()) { + StringRef N = II->getName(); + // FIXME: Use regular expressions when they get marked as acceptable + // in the LLVM coding standard? + if (N.contains_lower("ptr") || N.contains_lower("pointer")) { + if (N.contains_lower("ref") || N.contains_lower("cnt") || + N.contains_lower("intrusive") || N.contains_lower("shared")) { + return true; + } + } + } + return false; +} + std::shared_ptr MallocChecker::MallocBugVisitor::VisitNode( const ExplodedNode *N, const ExplodedNode *PrevN, BugReporterContext &BRC, BugReport &BR) { @@ -2881,21 +2896,34 @@ 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). + // See if we're releasing memory while inlining a destructor + // (or one of its callees). This turns on various common + // false positive suppressions. + bool FoundAnyDestructor = false; for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) { - if (isa(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; + if (const CXXDestructorDecl *DD = + dyn_cast(LC->getDecl())) { + if (isReferenceCountingPointerDestructor(DD)) { + // This immediately looks like a reference-counting destructor. + // We're bad at guessing the original reference count of the object, + // so suppress the report for now. + BR.markInvalid(getTag(), DD); + } else if (!FoundAnyDestructor) { + assert(!ReleaseDestructorLC && + "There can be only one release point!"); + // Suspect that it's a reference counting pointer destructor. + // On one of the next nodes might find out that it has atomic + // reference counting operations within it (see the code above), + // and if so, we'd conclude that it likely is a reference counting + // pointer destructor. + 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. + FoundAnyDestructor = true; + } } } } else if (isRelinquished(RS, RSPrev, S)) { Index: test/Analysis/NewDelete-atomics.cpp =================================================================== --- test/Analysis/NewDelete-atomics.cpp +++ test/Analysis/NewDelete-atomics.cpp @@ -2,6 +2,10 @@ // 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 +// RUN: %clang_analyze_cc1 -analyzer-inline-max-stack-depth 2 -analyzer-config ipa-always-inline-size=2 -analyzer-checker=core,cplusplus.NewDelete -std=c++11 -verify %s +// RUN: %clang_analyze_cc1 -analyzer-inline-max-stack-depth 2 -analyzer-config ipa-always-inline-size=2 -analyzer-checker=core,cplusplus.NewDeleteLeaks -DLEAKS -std=c++11 -verify %s +// RUN: %clang_analyze_cc1 -analyzer-inline-max-stack-depth 2 -analyzer-config ipa-always-inline-size=2 -analyzer-checker=core,cplusplus.NewDelete -std=c++11 -DTEST_INLINABLE_ALLOCATORS -verify %s +// RUN: %clang_analyze_cc1 -analyzer-inline-max-stack-depth 2 -analyzer-config ipa-always-inline-size=2 -analyzer-checker=core,cplusplus.NewDeleteLeaks -DLEAKS -std=c++11 -DTEST_INLINABLE_ALLOCATORS -verify %s // expected-no-diagnostics @@ -51,7 +55,7 @@ delete Ptr; } - Obj *getPtr() const { return Ptr; } + Obj *getPtr() const { return Ptr; } // no-warning }; void testDestroyLocalRefPtr() { Index: test/Analysis/NewDelete-refptr.cpp =================================================================== --- /dev/null +++ test/Analysis/NewDelete-refptr.cpp @@ -0,0 +1,60 @@ +// 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" + +class Obj { + int RefCnt; + +public: + // Implementation is intentionally missing to emulate the situation in which + // we are unable to inline these methods for any irrelevant reason. + int incRef(); + int decRef(); + + 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; } // no-warning +}; + +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 +}