llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Donát Nagy (NagyDonat) <details> <summary>Changes</summary> 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 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 `````````` </details> https://github.com/llvm/llvm-project/pull/162124 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
