diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index 11651fd491f7438..3d89d1e63434902 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -3447,7 +3447,7 @@ static bool isReferenceCountingPointerDestructor(const CXXDestructorDecl *DD) { if (N.contains_insensitive("ptr") || N.contains_insensitive("pointer")) { if (N.contains_insensitive("ref") || N.contains_insensitive("cnt") || N.contains_insensitive("intrusive") || - N.contains_insensitive("shared")) { + N.contains_insensitive("shared") || N.ends_with_insensitive("rc")) { return true; } } @@ -3479,13 +3479,24 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const ExplodedNode *N, // 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 (ReleaseDestructorLC && (ReleaseDestructorLC == CurrentLC || + ReleaseDestructorLC->isParentOf(CurrentLC))) { if (const auto *AE = dyn_cast(S)) { + // Check for manual use of atomic builtins. 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); + } + } else if (const auto *CE = dyn_cast(S)) { + // Check for `std::atomic` and such. This covers both regular method calls + // and operator calls. + if (const auto *MD = + dyn_cast_or_null(CE->getDirectCallee())) { + const CXXRecordDecl *RD = MD->getParent(); + // A bit wobbly with ".contains()" because it may be like + // "__atomic_base" or something. + if (StringRef(RD->getNameAsString()).contains("atomic")) { BR.markInvalid(getTag(), S); } } diff --git a/clang/test/Analysis/Inputs/system-header-simulator-cxx.h b/clang/test/Analysis/Inputs/system-header-simulator-cxx.h index 1c2be322f83c20a..29326ec1f9280db 100644 --- a/clang/test/Analysis/Inputs/system-header-simulator-cxx.h +++ b/clang/test/Analysis/Inputs/system-header-simulator-cxx.h @@ -1260,6 +1260,13 @@ template< iterator end() const { return iterator(val + 1); } }; +template +class atomic { +public: + T operator++(); + T operator--(); +}; + namespace execution { class sequenced_policy {}; } diff --git a/clang/test/Analysis/NewDelete-atomics.cpp b/clang/test/Analysis/NewDelete-atomics.cpp index 54fce17ea7bd22e..1425acab7489ba9 100644 --- a/clang/test/Analysis/NewDelete-atomics.cpp +++ b/clang/test/Analysis/NewDelete-atomics.cpp @@ -20,7 +20,7 @@ typedef enum memory_order { memory_order_seq_cst = __ATOMIC_SEQ_CST } memory_order; -class Obj { +class RawObj { int RefCnt; public: @@ -37,11 +37,27 @@ class Obj { void foo(); }; +class StdAtomicObj { + std::atomic RefCnt; + +public: + int incRef() { + return ++RefCnt; + } + + int decRef() { + return --RefCnt; + } + + void foo(); +}; + +template class IntrusivePtr { - Obj *Ptr; + T *Ptr; public: - IntrusivePtr(Obj *Ptr) : Ptr(Ptr) { + IntrusivePtr(T *Ptr) : Ptr(Ptr) { Ptr->incRef(); } @@ -55,22 +71,106 @@ class IntrusivePtr { delete Ptr; } - Obj *getPtr() const { return Ptr; } // no-warning + T *getPtr() const { return Ptr; } // no-warning +}; + +// Also IntrusivePtr but let's dodge name-based heuristics. +template +class DifferentlyNamed { + T *Ptr; + +public: + DifferentlyNamed(T *Ptr) : Ptr(Ptr) { + Ptr->incRef(); + } + + DifferentlyNamed(const DifferentlyNamed &Other) : Ptr(Other.Ptr) { + Ptr->incRef(); + } + + ~DifferentlyNamed() { + // We should not take the path on which the object is deleted. + if (Ptr->decRef() == 1) + delete Ptr; + } + + T *getPtr() const { return Ptr; } // no-warning }; void testDestroyLocalRefPtr() { - IntrusivePtr p1(new Obj()); + IntrusivePtr p1(new RawObj()); + { + 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 +} + +void testDestroyLocalRefPtrWithAtomics() { + IntrusivePtr p1(new StdAtomicObj()); + { + IntrusivePtr p2(p1); + } + + // p1 still maintains ownership. The object is not deleted. + p1.getPtr()->foo(); // no-warning +} + + +void testDestroyLocalRefPtrWithAtomics(const IntrusivePtr &p1) { { - IntrusivePtr p2(p1); + IntrusivePtr p2(p1); } // p1 still maintains ownership. The object is not deleted. p1.getPtr()->foo(); // no-warning } -void testDestroySymbolicRefPtr(const IntrusivePtr &p1) { +void testDestroyLocalRefPtrDifferentlyNamed() { + DifferentlyNamed p1(new RawObj()); + { + DifferentlyNamed p2(p1); + } + + // p1 still maintains ownership. The object is not deleted. + p1.getPtr()->foo(); // no-warning +} + +void testDestroySymbolicRefPtrDifferentlyNamed( + const DifferentlyNamed &p1) { + { + DifferentlyNamed p2(p1); + } + + // p1 still maintains ownership. The object is not deleted. + p1.getPtr()->foo(); // no-warning +} + +void testDestroyLocalRefPtrWithAtomicsDifferentlyNamed() { + DifferentlyNamed p1(new StdAtomicObj()); + { + DifferentlyNamed p2(p1); + } + + // p1 still maintains ownership. The object is not deleted. + p1.getPtr()->foo(); // no-warning +} + + +void testDestroyLocalRefPtrWithAtomicsDifferentlyNamed( + const DifferentlyNamed &p1) { { - IntrusivePtr p2(p1); + DifferentlyNamed p2(p1); } // p1 still maintains ownership. The object is not deleted.