From f712ca5d1c4e6caefcdf9f0ee553b4c84b98df27 Mon Sep 17 00:00:00 2001 From: Adam Cozzette Date: Tue, 16 Jul 2024 12:57:33 -0700 Subject: [PATCH] Rust protobuf: fix memory leaks Our ASAN test runs have not had the heap checker enabled, so this has allowed a few memory leaks to slip in. This CL fixes all of them so that we can turn on the heap checker. The first one takes place whenever we add an entry into a string-valued map using the C++ kernel. The problem is that `InnerProtoString::into_raw()` gives up ownership of the raw `std::string` pointer it holds, but then we never delete that pointer. This CL fixes the problem by deleting the pointer in C++ right after we perform the map insertion. To simplify things, I created a `MakeCleanup()` helper function that we always call in our map insertion thunks, but it's a no-op in the cases where we don't need to free anything. There were a couple similar memory leaks related to repeated field accessors in the C++ kernel, and those were simple to fix just by adding the necessary `delete` call. Finally, there were two benign memory leaks in the upb kernel involving global variables used for empty repeated fields and maps. It turned out that we did not need to use `Box` at all here, so removing that simplified things and fixed the leaks. PiperOrigin-RevId: 652947042 --- rust/cpp_kernel/map.h | 26 +++++++++++++++++++ rust/cpp_kernel/repeated.cc | 1 + rust/upb.rs | 17 +++--------- .../compiler/rust/accessors/repeated_field.cc | 4 ++- 4 files changed, 34 insertions(+), 14 deletions(-) diff --git a/rust/cpp_kernel/map.h b/rust/cpp_kernel/map.h index 2f0a8a82ab..3a471b18d4 100644 --- a/rust/cpp_kernel/map.h +++ b/rust/cpp_kernel/map.h @@ -1,6 +1,30 @@ #ifndef GOOGLE_PROTOBUF_RUST_CPP_KERNEL_MAP_H__ #define GOOGLE_PROTOBUF_RUST_CPP_KERNEL_MAP_H__ +#include + +namespace google { +namespace protobuf { +namespace rust { + +// String and bytes values are passed across the FFI boundary as owned raw +// pointers when we do map insertions. Unlike other types, they have to be +// explicitly deleted. This MakeCleanup() helper does nothing by default, but +// for std::string pointers it returns a std::unique_ptr to take ownership of +// the raw pointer. +template +int MakeCleanup(T value) { + return 0; +} + +inline std::unique_ptr MakeCleanup(std::string* value) { + return std::unique_ptr(value); +} + +} // namespace rust +} // namespace protobuf +} // namespace google + // Defines concrete thunks to access typed map methods from Rust. #define __PB_RUST_EXPOSE_SCALAR_MAP_METHODS( \ key_ty, rust_key_ty, ffi_key_ty, to_cpp_key, to_ffi_key, value_ty, \ @@ -23,6 +47,8 @@ } \ bool proto2_rust_thunk_Map_##rust_key_ty##_##rust_value_ty##_insert( \ google::protobuf::Map* m, ffi_key_ty key, ffi_value_ty value) { \ + auto cleanup = google::protobuf::rust::MakeCleanup(value); \ + (void)cleanup; \ auto iter_and_inserted = m->try_emplace(to_cpp_key, to_cpp_value); \ if (!iter_and_inserted.second) { \ iter_and_inserted.first->second = to_cpp_value; \ diff --git a/rust/cpp_kernel/repeated.cc b/rust/cpp_kernel/repeated.cc index 79654fbad2..2ff2248156 100644 --- a/rust/cpp_kernel/repeated.cc +++ b/rust/cpp_kernel/repeated.cc @@ -83,6 +83,7 @@ expose_repeated_field_methods(int64_t, i64); google::protobuf::RepeatedPtrField* r, size_t index, \ std::string* val) { \ *r->Mutable(index) = std::move(*val); \ + delete val; \ } \ void proto2_rust_RepeatedField_##ty##_copy_from( \ const google::protobuf::RepeatedPtrField* src, \ diff --git a/rust/upb.rs b/rust/upb.rs index 23f7563e78..a9c5581214 100644 --- a/rust/upb.rs +++ b/rust/upb.rs @@ -467,18 +467,15 @@ pub fn empty_array() -> RepeatedView<'static, T> // TODO: Consider creating a static empty array in C. // Use `i32` for a shared empty repeated for all repeated types in the program. - static EMPTY_REPEATED_VIEW: OnceLock> = OnceLock::new(); + static EMPTY_REPEATED_VIEW: OnceLock> = OnceLock::new(); // SAFETY: // - Because the repeated is never mutated, the repeated type is unused and // therefore valid for `T`. - // - The view is leaked for `'static`. unsafe { RepeatedView::from_raw( Private, - EMPTY_REPEATED_VIEW - .get_or_init(|| Box::leak(Box::new(Repeated::new())).as_mut().into_view()) - .as_raw(Private), + EMPTY_REPEATED_VIEW.get_or_init(Repeated::new).as_view().as_raw(Private), ) } } @@ -502,7 +499,7 @@ where // // If we used a larger key, then UPB would hash more bytes of the key than Rust // initialized. - static EMPTY_MAP_VIEW: OnceLock> = OnceLock::new(); + static EMPTY_MAP_VIEW: OnceLock> = OnceLock::new(); // SAFETY: // - The map is empty and never mutated. @@ -511,14 +508,8 @@ where // The map is empty, therefore it doesn't matter what hash is computed, but we // have to use `bool` type as the smallest key possible (otherwise UPB would // read more bytes than Rust allocated). - // - The view is leaked for `'static`. unsafe { - MapView::from_raw( - Private, - EMPTY_MAP_VIEW - .get_or_init(|| Box::leak(Box::new(Map::new())).as_mut().into_view()) - .as_raw(Private), - ) + MapView::from_raw(Private, EMPTY_MAP_VIEW.get_or_init(Map::new).as_view().as_raw(Private)) } } diff --git a/src/google/protobuf/compiler/rust/accessors/repeated_field.cc b/src/google/protobuf/compiler/rust/accessors/repeated_field.cc index 75435d19c4..966f835608 100644 --- a/src/google/protobuf/compiler/rust/accessors/repeated_field.cc +++ b/src/google/protobuf/compiler/rust/accessors/repeated_field.cc @@ -232,7 +232,8 @@ void RepeatedField::InThunkCc(Context& ctx, [&] { ctx.Emit( R"cc( - $ContainerType$<$ElementType$>* $getter_mut_thunk$($QualifiedMsg$* msg) { + $ContainerType$<$ElementType$>* $getter_mut_thunk$( + $QualifiedMsg$* msg) { return msg->mutable_$field$(); } const $ContainerType$<$ElementType$>* $getter_thunk$( @@ -243,6 +244,7 @@ void RepeatedField::InThunkCc(Context& ctx, $QualifiedMsg$* msg, $ContainerType$<$ElementType$>* value) { *msg->mutable_$field$() = std::move(*value); + delete value; } )cc"); }}},