[clang] [analyzer] [MallocChecker] suspect all release functions as candite for supression (PR #104599)
Pavel Skripkin via cfe-commits
cfe-commits at lists.llvm.org
Sat Aug 31 09:51:43 PDT 2024
https://github.com/pskrgag updated https://github.com/llvm/llvm-project/pull/104599
>From 913036ab795d6b91d6bb74d82aa2d329fe689535 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin <paskripkin at gmail.com>
Date: Fri, 16 Aug 2024 17:45:57 +0300
Subject: [PATCH 1/4] clang/csa: suspect all functions as those that may do
refcount release
---
.../StaticAnalyzer/Checkers/MallocChecker.cpp | 57 ++++++-------
clang/test/Analysis/malloc-refcounted.c | 80 +++++++++++++++++++
2 files changed, 110 insertions(+), 27 deletions(-)
create mode 100644 clang/test/Analysis/malloc-refcounted.c
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 3ddcb7e94ae5d6..77b9913a904700 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -904,16 +904,16 @@ class MallocBugVisitor final : public BugReporterVisitor {
// 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
+ // A release function stack frame in which memory was released. Used for
// miscellaneous false positive suppression.
- const StackFrameContext *ReleaseDestructorLC;
+ const StackFrameContext *ReleaseFunctionLC;
bool IsLeak;
public:
MallocBugVisitor(SymbolRef S, bool isLeak = false)
: Sym(S), Mode(Normal), FailedReallocSymbol(nullptr),
- ReleaseDestructorLC(nullptr), IsLeak(isLeak) {}
+ ReleaseFunctionLC(nullptr), IsLeak(isLeak) {}
static void *getTag() {
static int Tag = 0;
@@ -3558,8 +3558,8 @@ 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 && (ReleaseDestructorLC == CurrentLC ||
- ReleaseDestructorLC->isParentOf(CurrentLC))) {
+ if (ReleaseFunctionLC && (ReleaseFunctionLC == CurrentLC ||
+ ReleaseFunctionLC->isParentOf(CurrentLC))) {
if (const auto *AE = dyn_cast<AtomicExpr>(S)) {
// Check for manual use of atomic builtins.
AtomicExpr::AtomicOp Op = AE->getOp();
@@ -3648,35 +3648,38 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const ExplodedNode *N,
return nullptr;
}
- // 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 (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 &&
+ // See if we're releasing memory while inlining a destructor or
+ // functions that decrement reference counters (or one of its callees).
+ // This turns on various common false positive suppressions.
+ bool FoundAnyReleaseFunction = false;
+ for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
+ 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);
+ continue;
+ }
+ }
+
+ if (!FoundAnyReleaseFunction) {
+ assert(!ReleaseFunctionLC &&
"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->getStackFrame();
+ // Suspect that it's a reference counting pointer
+ // destructor/function. 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.
+ ReleaseFunctionLC = LC->getStackFrame();
// 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;
+ // destructor or function.
+ FoundAnyReleaseFunction = true;
}
}
- }
} else if (isRelinquished(RSCurr, RSPrev, S)) {
Msg = "Memory ownership is transferred";
StackHint = std::make_unique<StackHintGeneratorForSymbol>(Sym, "");
diff --git a/clang/test/Analysis/malloc-refcounted.c b/clang/test/Analysis/malloc-refcounted.c
new file mode 100644
index 00000000000000..5897b6c3186bd5
--- /dev/null
+++ b/clang/test/Analysis/malloc-refcounted.c
@@ -0,0 +1,80 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -verify %s
+//
+
+typedef unsigned long size_t;
+
+typedef enum memory_order {
+ memory_order_relaxed = __ATOMIC_RELAXED,
+} memory_order;
+
+void *calloc(size_t, size_t);
+void free(void *);
+
+struct SomeData {
+ int i;
+ _Atomic int ref;
+};
+
+static struct SomeData *alloc_data(void)
+{
+ struct SomeData *data = calloc(sizeof(*data), 1);
+
+ __c11_atomic_store(&data->ref, 2, memory_order_relaxed);
+ return data;
+}
+
+static void put_data(struct SomeData *data)
+{
+ if (__c11_atomic_fetch_sub(&data->ref, 1, memory_order_relaxed) == 1)
+ free(data);
+}
+
+static int dec_refcounter(struct SomeData *data)
+{
+ return __c11_atomic_fetch_sub(&data->ref, 1, memory_order_relaxed) == 1;
+}
+
+static void put_data_nested(struct SomeData *data)
+{
+ if (dec_refcounter(data))
+ free(data);
+}
+
+static void put_data_uncond(struct SomeData *data)
+{
+ free(data);
+}
+
+static void put_data_unrelated_atomic(struct SomeData *data)
+{
+ free(data);
+ __c11_atomic_fetch_sub(&data->ref, 1, memory_order_relaxed);
+}
+
+void test_no_uaf(void)
+{
+ struct SomeData *data = alloc_data();
+ put_data(data);
+ data->i += 1; // no warning
+}
+
+void test_no_uaf_nested(void)
+{
+ struct SomeData *data = alloc_data();
+ put_data_nested(data);
+ data->i += 1; // no warning
+}
+
+void test_uaf(void)
+{
+ struct SomeData *data = alloc_data();
+ put_data_uncond(data);
+ data->i += 1; // expected-warning{{Use of memory after it is freed}}
+}
+
+void test_no_uaf_atomic_after(void)
+{
+ struct SomeData *data = alloc_data();
+ put_data_unrelated_atomic(data);
+ data->i += 1; // expected-warning{{Use of memory after it is freed}}
+}
>From a9d23cfb446c45c1b85e9a881ec2102b549b5f1b Mon Sep 17 00:00:00 2001
From: Pavel Skripkin <paskripkin at gmail.com>
Date: Fri, 16 Aug 2024 19:02:57 +0300
Subject: [PATCH 2/4] fix windows CI
---
clang/test/Analysis/malloc-refcounted.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/test/Analysis/malloc-refcounted.c b/clang/test/Analysis/malloc-refcounted.c
index 5897b6c3186bd5..bfbe91d6ecaf92 100644
--- a/clang/test/Analysis/malloc-refcounted.c
+++ b/clang/test/Analysis/malloc-refcounted.c
@@ -1,7 +1,7 @@
// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc -verify %s
//
-typedef unsigned long size_t;
+typedef __SIZE_TYPE__ size_t;
typedef enum memory_order {
memory_order_relaxed = __ATOMIC_RELAXED,
>From 0ab7a5a7ee72a60b3a478a7c508779458348f993 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin <paskripkin at gmail.com>
Date: Sat, 31 Aug 2024 19:41:32 +0300
Subject: [PATCH 3/4] rework ReleaseFunctionLC logic
---
.../StaticAnalyzer/Checkers/MallocChecker.cpp | 62 ++++++++++++-------
clang/test/Analysis/NewDelete-atomics.cpp | 46 ++++++++++++++
2 files changed, 85 insertions(+), 23 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 77b9913a904700..f1194eb20fe564 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -3551,12 +3551,13 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const ExplodedNode *N,
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.
+ // If we find an atomic fetch_add or fetch_sub within the function in which
+ // the pointer was released (before the release), this is likely a release point
+ // of reference-counted object (like 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
+ // on objects deleted in such functions. This can probably be improved
// through better shared pointer modeling.
if (ReleaseFunctionLC && (ReleaseFunctionLC == CurrentLC ||
ReleaseFunctionLC->isParentOf(CurrentLC))) {
@@ -3566,6 +3567,8 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const ExplodedNode *N,
if (Op == AtomicExpr::AO__c11_atomic_fetch_add ||
Op == AtomicExpr::AO__c11_atomic_fetch_sub) {
BR.markInvalid(getTag(), S);
+ // After report is considered invalid there is no need to proceed futher.
+ return nullptr;
}
} else if (const auto *CE = dyn_cast<CallExpr>(S)) {
// Check for `std::atomic` and such. This covers both regular method calls
@@ -3648,10 +3651,13 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const ExplodedNode *N,
return nullptr;
}
- // See if we're releasing memory while inlining a destructor or
- // functions that decrement reference counters (or one of its callees).
+ // Save the first destructor/function as release point.
+ assert(!ReleaseFunctionLC && "There should be only one release point");
+ ReleaseFunctionLC = CurrentLC->getStackFrame();
+
+ // See if we're releasing memory while inlining a destructor that
+ // decrement reference counters (or one of its callees).
// This turns on various common false positive suppressions.
- bool FoundAnyReleaseFunction = false;
for (const LocationContext *LC = CurrentLC; LC; LC = LC->getParent()) {
if (const auto *DD = dyn_cast<CXXDestructorDecl>(LC->getDecl())) {
if (isReferenceCountingPointerDestructor(DD)) {
@@ -3659,27 +3665,37 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const ExplodedNode *N,
// We're bad at guessing the original reference count of the
// object, so suppress the report for now.
BR.markInvalid(getTag(), DD);
- continue;
+
+ // After report is considered invalid there is no need to proceed futher.
+ return nullptr;
}
- }
- if (!FoundAnyReleaseFunction) {
- assert(!ReleaseFunctionLC &&
- "There can be only one release point!");
- // Suspect that it's a reference counting pointer
- // destructor/function. 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.
+ // Switch suspection to outer destructor to catch patterns like:
+ //
+ // SmartPointr::~SmartPointr() {
+ // if (__c11_atomic_fetch_sub(refcount, 1, memory_order_relaxed) == 1)
+ // release_resources();
+ // }
+ // void SmartPointr::release_resources() {
+ // free(buffer);
+ // }
+ //
+ // This way ReleaseFunctionLC will point to outermost destructor and
+ // it would be possible to catch wider range of FP.
+ //
+ // NOTE: it would be great to support smth like that in C, since currently
+ // patterns like following won't be supressed:
+ //
+ // void doFree(struct Data *data) { free(data); }
+ // void putData(struct Data *data)
+ // {
+ // if (refPut(data))
+ // doFree(data);
+ // }
ReleaseFunctionLC = LC->getStackFrame();
- // 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 or function.
- FoundAnyReleaseFunction = true;
}
}
+
} else if (isRelinquished(RSCurr, RSPrev, S)) {
Msg = "Memory ownership is transferred";
StackHint = std::make_unique<StackHintGeneratorForSymbol>(Sym, "");
diff --git a/clang/test/Analysis/NewDelete-atomics.cpp b/clang/test/Analysis/NewDelete-atomics.cpp
index 1425acab7489ba..cb0d5aaf1644b6 100644
--- a/clang/test/Analysis/NewDelete-atomics.cpp
+++ b/clang/test/Analysis/NewDelete-atomics.cpp
@@ -97,6 +97,32 @@ class DifferentlyNamed {
T *getPtr() const { return Ptr; } // no-warning
};
+// Also IntrusivePtr with different name and outline release in destructor
+template <typename T>
+class DifferentlyNamedOutlineRelease {
+ T *Ptr;
+
+public:
+ DifferentlyNamedOutlineRelease(T *Ptr) : Ptr(Ptr) {
+ Ptr->incRef();
+ }
+
+ DifferentlyNamedOutlineRelease(const DifferentlyNamedOutlineRelease &Other) : Ptr(Other.Ptr) {
+ Ptr->incRef();
+ }
+
+ void releasePtr(void) {
+ delete Ptr;
+ }
+
+ ~DifferentlyNamedOutlineRelease() {
+ if (Ptr->decRef() == 1)
+ releasePtr();
+ }
+
+ T *getPtr() const { return Ptr; } // no-warning
+};
+
void testDestroyLocalRefPtr() {
IntrusivePtr<RawObj> p1(new RawObj());
{
@@ -176,3 +202,23 @@ void testDestroyLocalRefPtrWithAtomicsDifferentlyNamed(
// p1 still maintains ownership. The object is not deleted.
p1.getPtr()->foo(); // no-warning
}
+
+void testDestroyLocalRefPtrWithOutlineRelease() {
+ DifferentlyNamedOutlineRelease <RawObj> p1(new RawObj());
+ {
+ DifferentlyNamedOutlineRelease <RawObj> p2(p1);
+ }
+
+ // p1 still maintains ownership. The object is not deleted.
+ p1.getPtr()->foo(); // no-warning
+}
+
+void testDestroySymbolicRefPtrWithOutlineRelease(
+ const DifferentlyNamedOutlineRelease<RawObj> &p1) {
+ {
+ DifferentlyNamedOutlineRelease <RawObj> p2(p1);
+ }
+
+ // p1 still maintains ownership. The object is not deleted.
+ p1.getPtr()->foo(); // no-warning
+}
>From 6c1467504616690ac4c627decb29f0df868b6d02 Mon Sep 17 00:00:00 2001
From: Pavel Skripkin <paskripkin at gmail.com>
Date: Sat, 31 Aug 2024 19:51:31 +0300
Subject: [PATCH 4/4] fix style
---
.../StaticAnalyzer/Checkers/MallocChecker.cpp | 31 ++++++++++---------
1 file changed, 17 insertions(+), 14 deletions(-)
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index f1194eb20fe564..1f2dd42ed915ca 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -3552,8 +3552,8 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const ExplodedNode *N,
const LocationContext *CurrentLC = N->getLocationContext();
// If we find an atomic fetch_add or fetch_sub within the function in which
- // the pointer was released (before the release), this is likely a release point
- // of reference-counted object (like shared pointer).
+ // the pointer was released (before the release), this is likely a release
+ // point of reference-counted object (like 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
@@ -3567,7 +3567,8 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const ExplodedNode *N,
if (Op == AtomicExpr::AO__c11_atomic_fetch_add ||
Op == AtomicExpr::AO__c11_atomic_fetch_sub) {
BR.markInvalid(getTag(), S);
- // After report is considered invalid there is no need to proceed futher.
+ // After report is considered invalid there is no need to proceed
+ // futher.
return nullptr;
}
} else if (const auto *CE = dyn_cast<CallExpr>(S)) {
@@ -3666,14 +3667,16 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const ExplodedNode *N,
// object, so suppress the report for now.
BR.markInvalid(getTag(), DD);
- // After report is considered invalid there is no need to proceed futher.
+ // After report is considered invalid there is no need to proceed
+ // futher.
return nullptr;
}
// Switch suspection to outer destructor to catch patterns like:
//
// SmartPointr::~SmartPointr() {
- // if (__c11_atomic_fetch_sub(refcount, 1, memory_order_relaxed) == 1)
+ // if (__c11_atomic_fetch_sub(refcount, 1, memory_order_relaxed) ==
+ // 1)
// release_resources();
// }
// void SmartPointr::release_resources() {
@@ -3683,15 +3686,15 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const ExplodedNode *N,
// This way ReleaseFunctionLC will point to outermost destructor and
// it would be possible to catch wider range of FP.
//
- // NOTE: it would be great to support smth like that in C, since currently
- // patterns like following won't be supressed:
- //
- // void doFree(struct Data *data) { free(data); }
- // void putData(struct Data *data)
- // {
- // if (refPut(data))
- // doFree(data);
- // }
+ // NOTE: it would be great to support smth like that in C, since
+ // currently patterns like following won't be supressed:
+ //
+ // void doFree(struct Data *data) { free(data); }
+ // void putData(struct Data *data)
+ // {
+ // if (refPut(data))
+ // doFree(data);
+ // }
ReleaseFunctionLC = LC->getStackFrame();
}
}
More information about the cfe-commits
mailing list