[analyzer] Suppress NewDeleteLeaks FP in protobuf code

[NagyDonat]

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).

[llvmbot]

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: Donát Nagy (NagyDonat)

Changes

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).

Change-Id: I63ab80a64f379405f27d6c94473a92d2bd2a45e4

---

Full diff: https://github.com/llvm/llvm-project/pull/162124.diff

3 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (+9)
• (added) clang/test/Analysis/Inputs/system-header-simulator-for-protobuf.h (+16)
• (modified) clang/test/Analysis/NewDeleteLeaks.cpp (+34)

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@+1 {{Memory is allocated}}
+  MessageLite *p = new MessageLite();
+  SomeOtherFunction(p);
+  // expected-warning@+2 {{Potential leak of memory pointed to by 'p'}}
+  // expected-note@+1    {{Potential leak of memory pointed to by 'p'}}
+}
+
+} // namespace protobuf_leak

[steakhal]

LGTM with nits. Thanks.

[steakhal]

I'd just include this at the top, or create a separate test file.
Including into a namespace is pretty evil.

[NagyDonat]

Ok, I moved the include to the top. (I didn't want to create a separate test file because I feel that it would be too small.)