[clang] 13b3629 - [analyzer] Suppress NewDeleteLeaks FP in protobuf code (#162124)

[via cfe-commits]

Author: Donát Nagy
Date: 2025-10-07T14:48:31+02:00
New Revision: 13b362911e553cc5e676d95d59343c74e8920fa1

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

LOG: [analyzer] Suppress NewDeleteLeaks FP in protobuf code (#162124)

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 large amounts of false positives on a system where the
protobuf header was included as a system header and therefore 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).

Added: 
    clang/test/Analysis/Inputs/system-header-simulator-for-protobuf.h

Modified: 
    clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
    clang/test/Analysis/NewDeleteLeaks.cpp

Removed: 
    


################################################################################
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..cb12b55a00925
--- /dev/null
+++ b/clang/test/Analysis/Inputs/system-header-simulator-for-protobuf.h
@@ -0,0 +1,18 @@
+// Like the compiler, the static analyzer treats some functions 
diff erently 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 {
+  int SomeArbitraryField;
+};
+
+// 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..d9c4b77d1e6fc 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.
@@ -218,3 +220,34 @@ 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 automatically generated
+// protobuf code that passes dynamically allocated memory to a certain function
+// named GetOwnedMessageInternal.
+namespace protobuf_leak {
+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 
diff erently (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