From bed585bdcb5cbbbfb407b4e869665d405b04adca Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Tue, 28 Sep 2021 21:44:38 -0700 Subject: [PATCH] fix memory leak (#27510) --- .../chttp2/transport/chttp2_transport.cc | 3 ++- .../chttp2/transport/incoming_metadata.cc | 3 +-- .../chttp2/transport/incoming_metadata.h | 1 + src/core/lib/transport/metadata_batch.h | 6 ++++-- test/core/transport/metadata_test.cc | 18 ++++++++++++++++++ 5 files changed, 26 insertions(+), 5 deletions(-) diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index 8ec6ed1000d..0a26c7d6d4b 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -2114,7 +2114,8 @@ void grpc_chttp2_fake_status(grpc_chttp2_transport* t, grpc_chttp2_stream* s, GRPC_LOG_IF_ERROR( "add_status_message", grpc_chttp2_incoming_metadata_buffer_replace_or_add( - &s->trailing_metadata_buffer, GRPC_MDSTR_GRPC_MESSAGE, slice)); + &s->trailing_metadata_buffer, GRPC_MDSTR_GRPC_MESSAGE, + grpc_slice_ref_internal(slice))); } s->published_metadata[1] = GRPC_METADATA_SYNTHESIZED_FROM_FAKE; grpc_chttp2_maybe_complete_recv_trailing_metadata(t, s); diff --git a/src/core/ext/transport/chttp2/transport/incoming_metadata.cc b/src/core/ext/transport/chttp2/transport/incoming_metadata.cc index 65725ef255d..34d0b775abc 100644 --- a/src/core/ext/transport/chttp2/transport/incoming_metadata.cc +++ b/src/core/ext/transport/chttp2/transport/incoming_metadata.cc @@ -47,8 +47,7 @@ grpc_error_handle grpc_chttp2_incoming_metadata_buffer_replace_or_add( grpc_slice value) { if (buffer->batch.ReplaceIfExists(key, value)) return GRPC_ERROR_NONE; return grpc_chttp2_incoming_metadata_buffer_add( - buffer, grpc_mdelem_from_slices(grpc_slice_ref_internal(key), - grpc_slice_ref_internal(value))); + buffer, grpc_mdelem_from_slices(key, value)); } void grpc_chttp2_incoming_metadata_buffer_set_deadline( diff --git a/src/core/ext/transport/chttp2/transport/incoming_metadata.h b/src/core/ext/transport/chttp2/transport/incoming_metadata.h index 2a32ebc600a..5da24c80165 100644 --- a/src/core/ext/transport/chttp2/transport/incoming_metadata.h +++ b/src/core/ext/transport/chttp2/transport/incoming_metadata.h @@ -44,6 +44,7 @@ void grpc_chttp2_incoming_metadata_buffer_publish( grpc_error_handle grpc_chttp2_incoming_metadata_buffer_add( grpc_chttp2_incoming_metadata_buffer* buffer, grpc_mdelem elem) GRPC_MUST_USE_RESULT; +// key & value ownership are transferred to this function. grpc_error_handle grpc_chttp2_incoming_metadata_buffer_replace_or_add( grpc_chttp2_incoming_metadata_buffer* buffer, grpc_slice key, grpc_slice value) GRPC_MUST_USE_RESULT; diff --git a/src/core/lib/transport/metadata_batch.h b/src/core/lib/transport/metadata_batch.h index d7bd53435a7..db6f1155d6a 100644 --- a/src/core/lib/transport/metadata_batch.h +++ b/src/core/lib/transport/metadata_batch.h @@ -209,6 +209,9 @@ class MetadataMap { } // Set key to value if it exists and return true, otherwise return false. + // If this function returns true, it takes ownership of key and value. + // If this function returns false, it does not take ownership of key nor + // value. bool ReplaceIfExists(grpc_slice key, grpc_slice value); void Clear(); @@ -652,8 +655,7 @@ bool MetadataMap::ReplaceIfExists(grpc_slice key, grpc_slice value) { AssertValidCallouts(); for (grpc_linked_mdelem* l = list_.head; l != nullptr; l = l->next) { if (grpc_slice_eq(GRPC_MDKEY(l->md), key)) { - auto new_mdelem = grpc_mdelem_from_slices(grpc_slice_ref_internal(key), - grpc_slice_ref_internal(value)); + auto new_mdelem = grpc_mdelem_from_slices(key, value); GRPC_MDELEM_UNREF(l->md); l->md = new_mdelem; AssertValidCallouts(); diff --git a/test/core/transport/metadata_test.cc b/test/core/transport/metadata_test.cc index a3f7015d0b4..6935e188e7f 100644 --- a/test/core/transport/metadata_test.cc +++ b/test/core/transport/metadata_test.cc @@ -32,7 +32,9 @@ #include "src/core/ext/transport/chttp2/transport/bin_encoder.h" #include "src/core/ext/transport/chttp2/transport/hpack_utils.h" +#include "src/core/ext/transport/chttp2/transport/incoming_metadata.h" #include "src/core/lib/gpr/string.h" +#include "src/core/lib/iomgr/error.h" #include "src/core/lib/iomgr/exec_ctx.h" #include "src/core/lib/slice/slice_internal.h" #include "src/core/lib/transport/metadata_batch.h" @@ -407,6 +409,21 @@ static void test_grpc_metadata_batch_get_value_returns_multiple_values(void) { grpc_shutdown(); } +static void test_grpc_chttp2_incoming_metadata_replace_or_add_works(void) { + grpc_core::Arena* arena = grpc_core::Arena::Create(1024); + grpc_chttp2_incoming_metadata_buffer buffer(arena); + GRPC_LOG_IF_ERROR("incoming_buffer_add", + grpc_chttp2_incoming_metadata_buffer_add( + &buffer, grpc_mdelem_from_slices( + grpc_slice_from_static_string("a"), + grpc_slice_from_static_string("b")))); + GRPC_LOG_IF_ERROR("incoming_buffer_replace_or_add", + grpc_chttp2_incoming_metadata_buffer_replace_or_add( + &buffer, grpc_slice_from_static_string("a"), + grpc_slice_malloc(1024 * 1024 * 1024))); + arena->Destroy(); +} + int main(int argc, char** argv) { grpc::testing::TestEnvironment env(argc, argv); grpc_init(); @@ -427,6 +444,7 @@ int main(int argc, char** argv) { test_grpc_metadata_batch_get_value_with_absent_key(); test_grpc_metadata_batch_get_value_returns_one_value(); test_grpc_metadata_batch_get_value_returns_multiple_values(); + test_grpc_chttp2_incoming_metadata_replace_or_add_works(); grpc_shutdown(); return 0; }