[clang] 4c6f313 - [analyzer] [MallocChecker] suspect all release functions as candidate for suppression (#104599)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Sep 16 09:44:16 PDT 2024
Author: Pavel Skripkin
Date: 2024-09-16T19:44:13+03:00
New Revision: 4c6f313cb340c435f47ac032857030768c81c341
URL: https://github.com/llvm/llvm-project/commit/4c6f313cb340c435f47ac032857030768c81c341
DIFF: https://github.com/llvm/llvm-project/commit/4c6f313cb340c435f47ac032857030768c81c341.diff
LOG: [analyzer] [MallocChecker] suspect all release functions as candidate for suppression (#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
Added:
clang/test/Analysis/malloc-refcounted.c
Modified:
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
clang/test/Analysis/NewDelete-atomics.cpp
Removed:
################################################################################
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 596a885f886e7e..81ec8e1b516986 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -940,16 +940,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;
@@ -3653,21 +3653,25 @@ 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 (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();
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
@@ -3679,6 +3683,9 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const ExplodedNode *N,
// "__atomic_base" or something.
if (StringRef(RD->getNameAsString()).contains("atomic")) {
BR.markInvalid(getTag(), S);
+ // After report is considered invalid there is no need to proceed
+ // futher.
+ return nullptr;
}
}
}
@@ -3750,35 +3757,54 @@ 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 &&
- "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();
- // 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;
+ // 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.
+ 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);
+
+ // After report is considered invalid there is no need to proceed
+ // futher.
+ return nullptr;
+ }
+
+ // Switch suspection to outer destructor to catch patterns like:
+ // (note that class name is distorted to bypass
+ // isReferenceCountingPointerDestructor() logic)
+ //
+ // SmartPointr::~SmartPointr() {
+ // if (refcount.fetch_sub(1) == 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();
}
}
- }
+
} 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
diff erent 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
+}
diff --git a/clang/test/Analysis/malloc-refcounted.c b/clang/test/Analysis/malloc-refcounted.c
new file mode 100644
index 00000000000000..bfbe91d6ecaf92
--- /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 __SIZE_TYPE__ 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