[clang] [analyzer] Suppress NewDeleteLeaks FP in protobuf code (PR #162124)

[Donát Nagy via cfe-commits]

https://github.com/NagyDonat updated https://github.com/llvm/llvm-project/pull/162124

>From 41bab25ef65afe274d2f57db1f640d4ee3987f15 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Mon, 6 Oct 2025 19:13:37 +0200
Subject: [PATCH 1/2] [analyzer] Suppress NewDeleteLeaks FP in protobuf code

Code automatically generated by protobuf can include a pattern where it
allocates memory with `new` and then passes it to a function named
`GetOwnedMessageInternal` which takes ownership of the allocated memory.
This caused a large amount of false positives on a system where the
protobuf header was included as a system header, so the analyzer assumed
that `GetOwnedMessageInternal` won't escape memory.

As we already individually recognize a dozen functions that can be
declared in system headers but can escape memory, this commit just adds
`GetOwnedMessageInternal` to that list.

On a longer term perhaps we should distinguish the standard library
headers (where the analyzer can assume that it recognizes all the
functions that can free/escape memory) and other system headers (where
the analyzer shouldn't make this assumption).

Change-Id: I63ab80a64f379405f27d6c94473a92d2bd2a45e4
---
 .../StaticAnalyzer/Checkers/MallocChecker.cpp |  9 +++++
 .../system-header-simulator-for-protobuf.h    | 16 +++++++++
 clang/test/Analysis/NewDeleteLeaks.cpp        | 34 +++++++++++++++++++
 3 files changed, 59 insertions(+)
 create mode 100644 clang/test/Analysis/Inputs/system-header-simulator-for-protobuf.h

diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 83d79b43afe9f..70baab54df563 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -3812,6 +3812,15 @@ bool MallocChecker::mayFreeAnyEscapedMemoryOrIsModeledExplicitly(
     return true;
   }
 
+  // Protobuf function declared in `generated_message_util.h` that takes
+  // ownership of the second argument. As the first and third arguments are
+  // allocation arenas and won't be tracked by this checker, there is no reason
+  // to set `EscapingSymbol`. (Also, this is an implementation detail of
+  // Protobuf, so it's better to be a bit more permissive.)
+  if (FName == "GetOwnedMessageInternal") {
+    return true;
+  }
+
   // Handle cases where we know a buffer's /address/ can escape.
   // Note that the above checks handle some special cases where we know that
   // even though the address escapes, it's still our responsibility to free the
diff --git a/clang/test/Analysis/Inputs/system-header-simulator-for-protobuf.h b/clang/test/Analysis/Inputs/system-header-simulator-for-protobuf.h
new file mode 100644
index 0000000000000..093255cda3580
--- /dev/null
+++ b/clang/test/Analysis/Inputs/system-header-simulator-for-protobuf.h
@@ -0,0 +1,16 @@
+// Like the compiler, the static analyzer treats some functions differently if
+// they come from a system header -- for example, it is assumed that system
+// functions do not arbitrarily free() their parameters, and that some bugs
+// found in system headers cannot be fixed by the user and should be
+// suppressed.
+#pragma clang system_header
+
+class Arena;
+class MessageLite;
+
+// Originally declared in generated_message_util.h
+MessageLite *GetOwnedMessageInternal(Arena *, MessageLite *, Arena *);
+
+// Not a real protobuf function -- just introduced to validate that this file
+// is handled as a system header.
+void SomeOtherFunction(MessageLite *);
diff --git a/clang/test/Analysis/NewDeleteLeaks.cpp b/clang/test/Analysis/NewDeleteLeaks.cpp
index b2bad7e76fad0..41cbb1ddb1705 100644
--- a/clang/test/Analysis/NewDeleteLeaks.cpp
+++ b/clang/test/Analysis/NewDeleteLeaks.cpp
@@ -218,3 +218,37 @@ void caller() {
   (void)n;
 } // no-warning: No potential memory leak here, because that's been already reported.
 } // namespace symbol_reaper_lifetime
+
+// Check that we do not report false positives in automaticall generated
+// protobuf code that passes dynamically allocated memory to a certain function
+// named GetOwnedMessageInternal.
+namespace protobuf_leak {
+#include "Inputs/system-header-simulator-for-protobuf.h"
+
+class MessageLite { int SomeField; }; // Sufficient for our purposes.
+Arena *some_arena, *some_submessage_arena;
+
+MessageLite *protobuf_leak() {
+  MessageLite *p = new MessageLite(); // Real protobuf code instantiates a
+                                      // subclass of MessageLite, but that's
+                                      // not relevant for the bug.
+  MessageLite *q = GetOwnedMessageInternal(some_arena, p, some_submessage_arena);
+  return q;
+  // No leak at end of function -- the pointer escapes in GetOwnedMessageInternal.
+}
+
+void validate_system_header() {
+  // The case protobuf_leak would also pass if GetOwnedMessageInternal wasn't
+  // declared in a system header. This test verifies that another function
+  // declared in the same header behaves differently (doesn't escape memory) to
+  // demonstrate that GetOwnedMessageInternal is indeed explicitly recognized
+  // by the analyzer.
+
+  // expected-note at +1 {{Memory is allocated}}
+  MessageLite *p = new MessageLite();
+  SomeOtherFunction(p);
+  // expected-warning at +2 {{Potential leak of memory pointed to by 'p'}}
+  // expected-note at +1    {{Potential leak of memory pointed to by 'p'}}
+}
+
+} // namespace protobuf_leak

>From ac4b57f5399af3d1a30ec42233434d6294093d49 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.nagy at ericsson.com>
Date: Tue, 7 Oct 2025 12:56:42 +0200
Subject: [PATCH 2/2] Move #include out of namespace

---
 .../Analysis/Inputs/system-header-simulator-for-protobuf.h   | 4 +++-
 clang/test/Analysis/NewDeleteLeaks.cpp                       | 5 ++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/clang/test/Analysis/Inputs/system-header-simulator-for-protobuf.h b/clang/test/Analysis/Inputs/system-header-simulator-for-protobuf.h
index 093255cda3580..cb12b55a00925 100644
--- a/clang/test/Analysis/Inputs/system-header-simulator-for-protobuf.h
+++ b/clang/test/Analysis/Inputs/system-header-simulator-for-protobuf.h
@@ -6,7 +6,9 @@
 #pragma clang system_header
 
 class Arena;
-class MessageLite;
+class MessageLite {
+  int SomeArbitraryField;
+};
 
 // Originally declared in generated_message_util.h
 MessageLite *GetOwnedMessageInternal(Arena *, MessageLite *, Arena *);
diff --git a/clang/test/Analysis/NewDeleteLeaks.cpp b/clang/test/Analysis/NewDeleteLeaks.cpp
index 41cbb1ddb1705..5c982ecbe93b5 100644
--- a/clang/test/Analysis/NewDeleteLeaks.cpp
+++ b/clang/test/Analysis/NewDeleteLeaks.cpp
@@ -13,6 +13,8 @@
 // RUN:     unix.DynamicMemoryModeling:AddNoOwnershipChangeNotes=true
 
 #include "Inputs/system-header-simulator-for-malloc.h"
+// For the tests in namespace protobuf_leak:
+#include "Inputs/system-header-simulator-for-protobuf.h"
 
 //===----------------------------------------------------------------------===//
 // Report for which we expect NoOwnershipChangeVisitor to add a new note.
@@ -223,9 +225,6 @@ void caller() {
 // protobuf code that passes dynamically allocated memory to a certain function
 // named GetOwnedMessageInternal.
 namespace protobuf_leak {
-#include "Inputs/system-header-simulator-for-protobuf.h"
-
-class MessageLite { int SomeField; }; // Sufficient for our purposes.
 Arena *some_arena, *some_submessage_arena;
 
 MessageLite *protobuf_leak() {