[llvm-branch-commits] [clang] c82ee13 - [analyzer] Fix crash in MallocChecker when a function has both ownership_returns and ownership_takes (#183583)

[Douglas Yung via llvm-branch-commits]

Author: Balázs Benics
Date: 2026-02-28T09:20:09Z
New Revision: c82ee13efefd12a60c8f14e2363ca0ab4dbb072c

URL: https://github.com/llvm/llvm-project/commit/c82ee13efefd12a60c8f14e2363ca0ab4dbb072c
DIFF: https://github.com/llvm/llvm-project/commit/c82ee13efefd12a60c8f14e2363ca0ab4dbb072c.diff

LOG: [analyzer] Fix crash in MallocChecker when a function has both ownership_returns and ownership_takes (#183583)

When a function was annotated with both `ownership_returns` and
`ownership_takes` (or `ownership_holds`), MallocChecker::evalCall would
fall into the freeing-only branch (isFreeingOwnershipAttrCall) and call
checkOwnershipAttr without first calling MallocBindRetVal. That meant no
heap symbol had been conjured for the return value, so
checkOwnershipAttr later dereferenced a null/invalid symbol and crashed.

Fix: merge the two dispatch branches so that MallocBindRetVal is always
called first whenever ownership_returns is present, regardless of
whether the function also carries ownership_takes/ownership_holds.

The crash was introduced in #106081
339282d49f5310a2837da45c0ccc19da15675554.
Released in clang-20, and crashing ever since.

Fixes #183344.

Assisted-By: claude
(cherry picked from commit 2f4624613d05b2982dccc99ac0a06e87e6f3efd0)

Added: 
    

Modified: 
    clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
    clang/test/Analysis/malloc-annotations.c

Removed: 
    


################################################################################
diff  --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index ec7ef237b7c31..68369f8e81eb2 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1705,13 +1705,9 @@ bool MallocChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
     return true;
   }
 
-  if (isFreeingOwnershipAttrCall(Call)) {
-    checkOwnershipAttr(State, Call, C);
-    return true;
-  }
-
-  if (isAllocatingOwnershipAttrCall(Call)) {
-    State = MallocBindRetVal(C, Call, State, false);
+  if (isFreeingOwnershipAttrCall(Call) || isAllocatingOwnershipAttrCall(Call)) {
+    if (isAllocatingOwnershipAttrCall(Call))
+      State = MallocBindRetVal(C, Call, State, false);
     checkOwnershipAttr(State, Call, C);
     return true;
   }

diff  --git a/clang/test/Analysis/malloc-annotations.c b/clang/test/Analysis/malloc-annotations.c
index fdfd1d014ded4..ce02e39e04cfb 100644
--- a/clang/test/Analysis/malloc-annotations.c
+++ b/clang/test/Analysis/malloc-annotations.c
@@ -278,3 +278,96 @@ void testNoUninitAttr(void) {
   user_free(p);
 }
 
+// Regression test for GH#183344 — crash when a function has both
+// ownership_returns and ownership_takes attributes.
+typedef struct GH183344_X GH183344_X;
+typedef struct GH183344_Y GH183344_Y;
+
+GH183344_Y *GH183344_X_to_Y(GH183344_X *x)
+    __attribute__((ownership_returns(GH183344_Y)))
+    __attribute__((ownership_takes(GH183344_X, 1)));
+
+void testGH183344(void) {
+  GH183344_Y *y = GH183344_X_to_Y(0); // no-crash
+  (void)y;
+} // expected-warning{{Potential leak of memory pointed to by 'y'}}
+
+// Extended regression tests for GH#183344 — additional combinations of
+// ownership_returns, ownership_takes, and ownership_holds.
+
+GH183344_X *GH183344_alloc_X(void)
+    __attribute__((ownership_returns(GH183344_X)));
+void GH183344_free_X(GH183344_X *)
+    __attribute__((ownership_takes(GH183344_X, 1)));
+void GH183344_free_Y(GH183344_Y *)
+    __attribute__((ownership_takes(GH183344_Y, 1)));
+
+// Returns + Holds variant: Y is allocated, X is held (not consumed) by callee.
+GH183344_Y *GH183344_X_to_Y_hold(GH183344_X *x)
+    __attribute__((ownership_returns(GH183344_Y)))
+    __attribute__((ownership_holds(GH183344_X, 1)));
+
+// Returns + two Takes (same pool): both X arguments are consumed, Y is
+// returned. Multiple arg indices in one attribute (same pool) is valid;
+// two ownership_takes attributes with *
diff erent* pool names are not.
+GH183344_Y *GH183344_combine_XX(GH183344_X *x1, GH183344_X *x2)
+    __attribute__((ownership_returns(GH183344_Y)))
+    __attribute__((ownership_takes(GH183344_X, 1, 2)));
+
+// No-crash for Returns+Holds with null input — same crash pattern as the
+// original GH183344 bug but with ownership_holds instead of ownership_takes.
+void testGH183344_ReturnsHolds_NullInput(void) {
+  GH183344_Y *y = GH183344_X_to_Y_hold(0); // no-crash
+  (void)y;
+} // expected-warning{{Potential leak of memory pointed to by 'y'}}
+
+// Returns+Takes: allocate X, convert to Y (X consumed), free Y — no warnings.
+void testGH183344_ReturnsTakes_CleanUse(void) {
+  GH183344_X *x = GH183344_alloc_X();
+  GH183344_Y *y = GH183344_X_to_Y(x);
+  GH183344_free_Y(y);
+} // no-warning
+
+// Returns+Takes: after the conversion X is consumed; freeing it again is a
+// double-free.
+void testGH183344_ReturnsTakes_DoubleFreeInput(void) {
+  GH183344_X *x = GH183344_alloc_X();
+  GH183344_Y *y = GH183344_X_to_Y(x);
+  GH183344_free_X(x); // expected-warning{{Attempt to release already released memory}}
+  GH183344_free_Y(y);
+}
+
+// Returns+Takes: X is consumed but Y is never freed — leak on Y.
+void testGH183344_ReturnsTakes_LeakRetval(void) {
+  GH183344_X *x = GH183344_alloc_X();
+  GH183344_Y *y = GH183344_X_to_Y(x);
+  (void)y;
+} // expected-warning{{Potential leak of memory pointed to by 'y'}}
+
+// Returns+Holds: after the hold, X is non-owned by the caller; freeing it
+// produces a "non-owned memory" warning (analogous to af3).
+void testGH183344_ReturnsHolds_FreeHeldInput(void) {
+  GH183344_X *x = GH183344_alloc_X();
+  GH183344_Y *y = GH183344_X_to_Y_hold(x);
+  GH183344_free_X(x); // expected-warning{{Attempt to release non-owned memory}}
+  GH183344_free_Y(y);
+}
+
+// Multiple Takes (same pool) + Returns: both X inputs are consumed, Y is
+// returned and freed — no warnings.
+void testGH183344_CombineXX_CleanUse(void) {
+  GH183344_X *x1 = GH183344_alloc_X();
+  GH183344_X *x2 = GH183344_alloc_X();
+  GH183344_Y *y = GH183344_combine_XX(x1, x2);
+  GH183344_free_Y(y);
+} // no-warning
+
+// Multiple Takes (same pool): after the combine, x2 is already consumed;
+// freeing it again is a double-free.
+void testGH183344_CombineXX_DoubleFreeSecondInput(void) {
+  GH183344_X *x1 = GH183344_alloc_X();
+  GH183344_X *x2 = GH183344_alloc_X();
+  GH183344_Y *y = GH183344_combine_XX(x1, x2);
+  GH183344_free_X(x2); // expected-warning{{Attempt to release already released memory}}
+  GH183344_free_Y(y);
+}