[clang] [analyzer] [MallocChecker] suspect all release functions as candite for supression (PR #104599)

[Donát Nagy via cfe-commits]

================
@@ -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);
----------------
NagyDonat wrote:

No, `LocationContext::isParentOf` is not relevant here. (Its purpose is to handle cases like `put_data_nested` where the atomic operation is found within a location context that is a descendant of the `ReleaseFunctionLC`.)

The suppression logic is very simple -- `MallocBugVisitor::VisitNode` just visits _each_ exploded graph node along the execution path that led to creating the bug report, and performs the suppression when it finds an atomic add/sub that is (A) in the stack frame corresponding to `ReleaseFunctionLC` or (B) in a stack frame of a function that is a descendant of `ReleaseFunctionLC`. [Part (B) is facilitated by the `LocationContext::isParentOf` call.]

This means that the suppression can be triggered by
- atomic add/sub on totally unrelated operands;
- atomic add/sub whose result is ignored;
- atomic add/sub that happens _after_ the free call and is a clear use-after-free error.

To avoid suppressing TPs in cases like the one bothering you, it would be possible refine the suppression logic to
- only look for atomic operations that are performed in the condition of a conditional statement; and/or
- only look for atomic operations that happen before the `free()` call [because operations that happen after the `free()` are either use-after-free errors or they are almost surely unrelated to the current object].

(This could be a nice follow-up commit if you're interested and have time for it.)

https://github.com/llvm/llvm-project/pull/104599