Author: dergachev Date: Tue Mar 20 17:49:47 2018 New Revision: 328066 URL: http://llvm.org/viewvc/llvm-project?rev=328066&view=rev Log: [analyzer] Suppress more MallocChecker positives in smart pointer destructors.
r326249 wasn't quite enough because we often run out of inlining stack depth limit and for that reason fail to see the atomics we're looking for. Add a more straightforward false positive suppression that is based on the name of the class. I.e. if we're releasing a pointer in a destructor of a "something shared/intrusive/reference/counting something ptr/pointer something", then any use-after-free or double-free that occurs later would likely be a false positive. rdar://problem/38013606 Differential Revision: https://reviews.llvm.org/D44281 Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp cfe/trunk/test/Analysis/NewDelete-atomics.cpp Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp?rev=328066&r1=328065&r2=328066&view=diff ============================================================================== --- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (original) +++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp Tue Mar 20 17:49:47 2018 @@ -2829,6 +2829,19 @@ static SymbolRef findFailedReallocSymbol return nullptr; } +static bool isReferenceCountingPointerDestructor(const CXXDestructorDecl *DD) { + if (const IdentifierInfo *II = DD->getParent()->getIdentifier()) { + StringRef N = II->getName(); + 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<PathDiagnosticPiece> MallocChecker::MallocBugVisitor::VisitNode( const ExplodedNode *N, const ExplodedNode *PrevN, BugReporterContext &BRC, BugReport &BR) { @@ -2882,21 +2895,33 @@ std::shared_ptr<PathDiagnosticPiece> Mal 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<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; + if (const auto *DD = dyn_cast<CXXDestructorDecl>(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)) { Modified: cfe/trunk/test/Analysis/NewDelete-atomics.cpp URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/NewDelete-atomics.cpp?rev=328066&r1=328065&r2=328066&view=diff ============================================================================== --- cfe/trunk/test/Analysis/NewDelete-atomics.cpp (original) +++ cfe/trunk/test/Analysis/NewDelete-atomics.cpp Tue Mar 20 17:49:47 2018 @@ -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 @@ public: delete Ptr; } - Obj *getPtr() const { return Ptr; } + Obj *getPtr() const { return Ptr; } // no-warning }; void testDestroyLocalRefPtr() { _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
