NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a.sidorin, george.karpenkov, szepet.
Herald added subscribers: cfe-commits, rnkovacs.
https://reviews.llvm.org/D43791 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.
Repository:
rC Clang
https://reviews.llvm.org/D44281
Files:
lib/StaticAnalyzer/Checkers/MallocChecker.cpp
test/Analysis/NewDelete-atomics.cpp
test/Analysis/NewDelete-refptr.cpp
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
+}
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: 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<PathDiagnosticPiece> 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<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 CXXDestructorDecl *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)) {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
