[clang] [analyzer] [MallocChecker] suspect all release functions as candite for supression (PR #104599)
Pavel Skripkin via cfe-commits
cfe-commits at lists.llvm.org
Fri Aug 16 07:52:53 PDT 2024
https://github.com/pskrgag created https://github.com/llvm/llvm-project/pull/104599
Current MalloChecker logic suppresses FP caused by refcounting only for C++ destructors. The same pattern occurs a lot in C in objects with intrusive refcounting. See #104229 for code example.
To extend current logic to C, suspect all release functions as candidate for suppression.
Closes: #104229
>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] 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}}
+}
More information about the cfe-commits
mailing list