From 717f6400a5293fa8e80802acabd21db49afd520f Mon Sep 17 00:00:00 2001 From: Jose Ulises Nino Rivera Date: Mon, 19 Aug 2019 10:20:35 -0700 Subject: [PATCH] core: clean up memory management for headers (#342) Description: this PR completes memory management for headers being passed from the platform to C to Cpp and back. Risk Level: med - changes memory management of headers. Testing: Added unit tests to make sure that headers are being freed when appropriate. Signed-off-by: Jose Nino Signed-off-by: JP Simard --- mobile/library/common/http/dispatcher.cc | 6 +-- mobile/library/common/http/header_utility.cc | 20 ++++----- mobile/library/common/http/header_utility.h | 4 +- mobile/library/common/include/BUILD | 11 ++--- mobile/library/common/include/c_types.c | 41 +++++++++++++++++++ mobile/library/common/include/c_types.cc | 16 -------- mobile/library/common/include/c_types.h | 24 +++++++++++ mobile/library/objective-c/EnvoyEngine.m | 2 + mobile/test/common/http/dispatcher_test.cc | 30 +++++++------- .../test/common/http/header_utility_test.cc | 38 +++++++++++------ 10 files changed, 123 insertions(+), 69 deletions(-) create mode 100644 mobile/library/common/include/c_types.c delete mode 100644 mobile/library/common/include/c_types.cc diff --git a/mobile/library/common/http/dispatcher.cc b/mobile/library/common/http/dispatcher.cc index d8f7edaf0efd..078a1d84c884 100644 --- a/mobile/library/common/http/dispatcher.cc +++ b/mobile/library/common/http/dispatcher.cc @@ -14,7 +14,7 @@ Dispatcher::DirectStreamCallbacks::DirectStreamCallbacks(envoy_stream_t stream, void Dispatcher::DirectStreamCallbacks::onHeaders(HeaderMapPtr&& headers, bool end_stream) { ENVOY_LOG(debug, "[S{}] response headers for stream (end_stream={}):\n{}", stream_handle_, end_stream, *headers); - observer_.on_headers(Utility::transformHeaders(*headers), end_stream, observer_.context); + observer_.on_headers(Utility::toBridgeHeaders(*headers), end_stream, observer_.context); } void Dispatcher::DirectStreamCallbacks::onData(Buffer::Instance& data, bool end_stream) { @@ -25,7 +25,7 @@ void Dispatcher::DirectStreamCallbacks::onData(Buffer::Instance& data, bool end_ void Dispatcher::DirectStreamCallbacks::onTrailers(HeaderMapPtr&& trailers) { ENVOY_LOG(debug, "[S{}] response trailers for stream:\n{}", stream_handle_, *trailers); - observer_.on_trailers(Utility::transformHeaders(*trailers), observer_.context); + observer_.on_trailers(Utility::toBridgeHeaders(*trailers), observer_.context); } void Dispatcher::DirectStreamCallbacks::onComplete() { @@ -83,7 +83,7 @@ envoy_status_t Dispatcher::sendHeaders(envoy_stream_t stream, envoy_headers head // from the caller. // https://github.com/lyft/envoy-mobile/issues/301 if (direct_stream != nullptr) { - direct_stream->headers_ = Utility::transformHeaders(headers); + direct_stream->headers_ = Utility::toInternalHeaders(headers); ENVOY_LOG(debug, "[S{}] request headers for stream (end_stream={}):\n{}", stream, end_stream, *direct_stream->headers_); direct_stream->underlying_stream_.sendHeaders(*direct_stream->headers_, end_stream); diff --git a/mobile/library/common/http/header_utility.cc b/mobile/library/common/http/header_utility.cc index 5334b8492063..f0053f6743b2 100644 --- a/mobile/library/common/http/header_utility.cc +++ b/mobile/library/common/http/header_utility.cc @@ -6,28 +6,22 @@ namespace Envoy { namespace Http { namespace Utility { -static inline envoy_data copyEnvoyData(size_t length, const uint8_t* source) { - uint8_t* destination = static_cast(malloc(sizeof(uint8_t) * length)); - memcpy(destination, source, length); - return {length, destination, nullptr, nullptr}; -} - std::string convertToString(envoy_data s) { return std::string(reinterpret_cast(const_cast(s.bytes)), s.length); } -HeaderMapPtr transformHeaders(envoy_headers headers) { +HeaderMapPtr toInternalHeaders(envoy_headers headers) { Http::HeaderMapPtr transformed_headers = std::make_unique(); for (envoy_header_size_t i = 0; i < headers.length; i++) { transformed_headers->addCopy(LowerCaseString(convertToString(headers.headers[i].key)), convertToString(headers.headers[i].value)); } + // The C envoy_headers struct can be released now because the headers have been copied. + release_envoy_headers(headers); return transformed_headers; } -envoy_headers transformHeaders(const HeaderMap& header_map) { - // TODO: provide utility for the caller to free allocated memory - // https://github.com/lyft/envoy-mobile/issues/280 +envoy_headers toBridgeHeaders(const HeaderMap& header_map) { envoy_header* headers = static_cast(malloc(sizeof(envoy_header) * header_map.size())); envoy_headers transformed_headers; @@ -42,9 +36,9 @@ envoy_headers transformHeaders(const HeaderMap& header_map) { const absl::string_view header_value = header.value().getStringView(); envoy_data key = - copyEnvoyData(header_key.size(), reinterpret_cast(header_key.data())); - envoy_data value = copyEnvoyData(header_value.size(), - reinterpret_cast(header_value.data())); + copy_envoy_data(header_key.size(), reinterpret_cast(header_key.data())); + envoy_data value = copy_envoy_data(header_value.size(), + reinterpret_cast(header_value.data())); transformed_headers->headers[transformed_headers->length] = {key, value}; transformed_headers->length++; diff --git a/mobile/library/common/http/header_utility.h b/mobile/library/common/http/header_utility.h index 8cd362700616..c4c52b015cd7 100644 --- a/mobile/library/common/http/header_utility.h +++ b/mobile/library/common/http/header_utility.h @@ -22,7 +22,7 @@ std::string convertToString(envoy_data s); * @param headers, the envoy_headers to transform. * @return HeaderMapPtr, the HeaderMap 1:1 transformation of the headers param. */ -HeaderMapPtr transformHeaders(envoy_headers headers); +HeaderMapPtr toInternalHeaders(envoy_headers headers); /** * Transform envoy_headers to HeaderMap. @@ -31,7 +31,7 @@ HeaderMapPtr transformHeaders(envoy_headers headers); * @param headers, the HeaderMap to transform. * @return envoy_headers, the HeaderMap 1:1 transformation of the headers param. */ -envoy_headers transformHeaders(const HeaderMap& headers); +envoy_headers toBridgeHeaders(const HeaderMap& headers); } // namespace Utility } // namespace Http diff --git a/mobile/library/common/include/BUILD b/mobile/library/common/include/BUILD index 8615443c0b83..8e815db3b4ed 100644 --- a/mobile/library/common/include/BUILD +++ b/mobile/library/common/include/BUILD @@ -1,16 +1,13 @@ licenses(["notice"]) # Apache 2 -load("@envoy//bazel:envoy_build_system.bzl", "envoy_cc_library", "envoy_package") - -envoy_package() - -envoy_cc_library( +# This is a plain cc rule to ensure that the file is getting compiled as a plain C file, and provides plain C types. +cc_library( name = "c_types_interface", srcs = [ - "c_types.cc", + "c_types.c", ], hdrs = [ "c_types.h", ], - repository = "@envoy", + visibility = ["//visibility:public"], ) diff --git a/mobile/library/common/include/c_types.c b/mobile/library/common/include/c_types.c new file mode 100644 index 000000000000..22b5f71fd1f1 --- /dev/null +++ b/mobile/library/common/include/c_types.c @@ -0,0 +1,41 @@ +#include "library/common/include/c_types.h" + +#include + +// NOLINT(namespace-envoy) +// Although this file is in an envoy_cc_library rule it should be C compatible with old style casts. +#pragma GCC diagnostic ignored "-Wold-style-cast" + +void envoy_noop_release(void* context) { (void)context; } + +void release_envoy_headers(envoy_headers headers) { + for (envoy_header_size_t i = 0; i < headers.length; i++) { + envoy_header header = headers.headers[i]; + header.key.release(header.key.context); + header.value.release(header.value.context); + } + free(headers.headers); +} + +envoy_headers copy_envoy_headers(envoy_headers src) { + envoy_header* dst_header_array = (envoy_header*)malloc(sizeof(envoy_header) * src.length); + for (envoy_header_size_t i = 0; i < src.length; i++) { + envoy_header new_header = { + copy_envoy_data(src.headers[i].key.length, src.headers[i].key.bytes), + copy_envoy_data(src.headers[i].value.length, src.headers[i].value.bytes)}; + dst_header_array[i] = new_header; + } + envoy_headers dst = {src.length, dst_header_array}; + return dst; +} + +envoy_data copy_envoy_data(size_t length, const uint8_t* src_bytes) { + uint8_t* dst_bytes = (uint8_t*)malloc(sizeof(uint8_t) * length); + memcpy(dst_bytes, src_bytes, length); + // Note: since this function is copying the bytes over to freshly allocated memory, free is an + // appropriate release function and dst_bytes is an appropriate context. + envoy_data dst = {length, dst_bytes, free, dst_bytes}; + return dst; +} + +const envoy_data envoy_nodata = {0, NULL, envoy_noop_release, NULL}; diff --git a/mobile/library/common/include/c_types.cc b/mobile/library/common/include/c_types.cc deleted file mode 100644 index 43692d163f08..000000000000 --- a/mobile/library/common/include/c_types.cc +++ /dev/null @@ -1,16 +0,0 @@ -#include "library/common/include/c_types.h" - -// NOLINT(namespace-envoy) - -void envoy_noop_release(void* context) { (void)context; } - -void release_envoy_headers(envoy_headers headers) { - for (envoy_header_size_t i = 0; i < headers.length; i++) { - envoy_header header = headers.headers[i]; - header.key.release(header.key.context); - header.value.release(header.value.context); - } - free(headers.headers); -} - -const envoy_data envoy_nodata = {0, NULL, envoy_noop_release, NULL}; diff --git a/mobile/library/common/include/c_types.h b/mobile/library/common/include/c_types.h index 7b7f91c1c9f2..3f9a300c3769 100644 --- a/mobile/library/common/include/c_types.h +++ b/mobile/library/common/include/c_types.h @@ -80,11 +80,35 @@ typedef struct { envoy_header* headers; } envoy_headers; +#ifdef __cplusplus +extern "C" { // utility functions +#endif + /** * Helper function to free/release memory associated with underlying headers. + * @param headers, envoy_headers to release. */ void release_envoy_headers(envoy_headers headers); +/** + * Helper function to copy envoy_headers. + * @param src, the envoy_headers to copy from. + * @param envoy_headers, copied headers. + */ +envoy_headers copy_envoy_headers(envoy_headers src); + +/** + * Helper function to copy envoy_data. + * @param length, the length of the data to copy. + * @param src_bytes, the byte array to copy from. + * @return envoy_data, the envoy_data copied from the src. + */ +envoy_data copy_envoy_data(size_t length, const uint8_t* src_bytes); + +#ifdef __cplusplus +} // utility functions +#endif + // Convenience constant to pass to function calls with no data. // For example when sending a headers-only request. extern const envoy_data envoy_nodata; diff --git a/mobile/library/objective-c/EnvoyEngine.m b/mobile/library/objective-c/EnvoyEngine.m index ecb37c082196..09e3494a43cd 100644 --- a/mobile/library/objective-c/EnvoyEngine.m +++ b/mobile/library/objective-c/EnvoyEngine.m @@ -131,6 +131,8 @@ static envoy_headers toNativeHeaders(EnvoyHeaders *headers) { } [headerValueList addObject:headerValue]; } + // The C envoy_headers struct can be released now because the headers have been copied. + release_envoy_headers(headers); return headerDict; } diff --git a/mobile/test/common/http/dispatcher_test.cc b/mobile/test/common/http/dispatcher_test.cc index 79676b0f7e5c..f06ad4924e99 100644 --- a/mobile/test/common/http/dispatcher_test.cc +++ b/mobile/test/common/http/dispatcher_test.cc @@ -66,7 +66,7 @@ TEST_F(DispatcherTest, BasicStreamHeadersOnly) { observer.context = &cc; observer.on_headers = [](envoy_headers c_headers, bool end_stream, void* context) -> void { ASSERT_TRUE(end_stream); - HeaderMapPtr response_headers = Utility::transformHeaders(c_headers); + HeaderMapPtr response_headers = Utility::toInternalHeaders(c_headers); EXPECT_EQ(response_headers->Status()->value().getStringView(), "200"); callbacks_called* cc = static_cast(context); cc->on_headers = true; @@ -88,7 +88,7 @@ TEST_F(DispatcherTest, BasicStreamHeadersOnly) { // Build a set of request headers. TestHeaderMapImpl headers; HttpTestUtility::addDefaultHeaders(headers); - envoy_headers c_headers = Utility::transformHeaders(headers); + envoy_headers c_headers = Utility::toBridgeHeaders(headers); // Create a stream. EXPECT_CALL(cm_, httpAsyncClientForCluster("egress_cluster")) @@ -177,7 +177,7 @@ TEST_F(DispatcherTest, MultipleStreams) { observer.context = &cc; observer.on_headers = [](envoy_headers c_headers, bool end_stream, void* context) -> void { ASSERT_TRUE(end_stream); - HeaderMapPtr response_headers = Utility::transformHeaders(c_headers); + HeaderMapPtr response_headers = Utility::toInternalHeaders(c_headers); EXPECT_EQ(response_headers->Status()->value().getStringView(), "200"); callbacks_called* cc = static_cast(context); cc->on_headers = true; @@ -199,7 +199,7 @@ TEST_F(DispatcherTest, MultipleStreams) { // Build a set of request headers. TestHeaderMapImpl headers; HttpTestUtility::addDefaultHeaders(headers); - envoy_headers c_headers = Utility::transformHeaders(headers); + envoy_headers c_headers = Utility::toBridgeHeaders(headers); // Create a stream. EXPECT_CALL(cm_, httpAsyncClientForCluster("egress_cluster")) @@ -229,7 +229,7 @@ TEST_F(DispatcherTest, MultipleStreams) { observer2.context = &cc2; observer2.on_headers = [](envoy_headers c_headers, bool end_stream, void* context) -> void { ASSERT_TRUE(end_stream); - HeaderMapPtr response_headers = Utility::transformHeaders(c_headers); + HeaderMapPtr response_headers = Utility::toInternalHeaders(c_headers); EXPECT_EQ(response_headers->Status()->value().getStringView(), "503"); bool* on_headers_called2 = static_cast(context); *on_headers_called2 = true; @@ -251,7 +251,7 @@ TEST_F(DispatcherTest, MultipleStreams) { // Build a set of request headers. TestHeaderMapImpl headers2; HttpTestUtility::addDefaultHeaders(headers2); - envoy_headers c_headers2 = Utility::transformHeaders(headers2); + envoy_headers c_headers2 = Utility::toBridgeHeaders(headers2); // Create a stream. EXPECT_CALL(cm_, httpAsyncClientForCluster("egress_cluster")) @@ -303,7 +303,7 @@ TEST_F(DispatcherTest, LocalResetAfterStreamStart) { }; observer.on_headers = [](envoy_headers c_headers, bool end_stream, void* context) -> void { ASSERT_FALSE(end_stream); - HeaderMapPtr response_headers = Utility::transformHeaders(c_headers); + HeaderMapPtr response_headers = Utility::toInternalHeaders(c_headers); EXPECT_EQ(response_headers->Status()->value().getStringView(), "200"); callbacks_called* cc = static_cast(context); cc->on_headers = true; @@ -325,7 +325,7 @@ TEST_F(DispatcherTest, LocalResetAfterStreamStart) { // Build a set of request headers. TestHeaderMapImpl headers; HttpTestUtility::addDefaultHeaders(headers); - envoy_headers c_headers = Utility::transformHeaders(headers); + envoy_headers c_headers = Utility::toBridgeHeaders(headers); // Create a stream. EXPECT_CALL(cm_, httpAsyncClientForCluster("egress_cluster")) @@ -376,7 +376,7 @@ TEST_F(DispatcherTest, RemoteResetAfterStreamStart) { }; observer.on_headers = [](envoy_headers c_headers, bool end_stream, void* context) -> void { ASSERT_FALSE(end_stream); - HeaderMapPtr response_headers = Utility::transformHeaders(c_headers); + HeaderMapPtr response_headers = Utility::toInternalHeaders(c_headers); EXPECT_EQ(response_headers->Status()->value().getStringView(), "200"); callbacks_called* cc = static_cast(context); cc->on_headers = true; @@ -398,7 +398,7 @@ TEST_F(DispatcherTest, RemoteResetAfterStreamStart) { // Build a set of request headers. TestHeaderMapImpl headers; HttpTestUtility::addDefaultHeaders(headers); - envoy_headers c_headers = Utility::transformHeaders(headers); + envoy_headers c_headers = Utility::toBridgeHeaders(headers); // Create a stream. EXPECT_CALL(cm_, httpAsyncClientForCluster("egress_cluster")) @@ -443,7 +443,7 @@ TEST_F(DispatcherTest, DestroyWithActiveStream) { // Build a set of request headers. TestHeaderMapImpl headers; HttpTestUtility::addDefaultHeaders(headers); - envoy_headers c_headers = Utility::transformHeaders(headers); + envoy_headers c_headers = Utility::toBridgeHeaders(headers); // Create a stream. EXPECT_CALL(cm_, httpAsyncClientForCluster("egress_cluster")) @@ -474,7 +474,7 @@ TEST_F(DispatcherTest, ResetInOnHeaders) { // Build a set of request headers. TestHeaderMapImpl headers; HttpTestUtility::addDefaultHeaders(headers); - envoy_headers c_headers = Utility::transformHeaders(headers); + envoy_headers c_headers = Utility::toBridgeHeaders(headers); // Create a stream. EXPECT_CALL(cm_, httpAsyncClientForCluster("egress_cluster")) @@ -517,7 +517,7 @@ TEST_F(DispatcherTest, StreamTimeout) { // Build a set of request headers. TestHeaderMapImpl headers; HttpTestUtility::addDefaultHeaders(headers); - envoy_headers c_headers = Utility::transformHeaders(headers); + envoy_headers c_headers = Utility::toBridgeHeaders(headers); EXPECT_CALL(cm_, httpAsyncClientForCluster("egress_cluster")) .WillOnce(ReturnRef(cm_.async_client_)); @@ -566,7 +566,7 @@ TEST_F(DispatcherTest, StreamTimeoutHeadReply) { // Build a set of request headers. TestHeaderMapImpl headers; HttpTestUtility::addDefaultHeaders(headers, "HEAD"); - envoy_headers c_headers = Utility::transformHeaders(headers); + envoy_headers c_headers = Utility::toBridgeHeaders(headers); EXPECT_CALL(cm_, httpAsyncClientForCluster("egress_cluster")) .WillOnce(ReturnRef(cm_.async_client_)); @@ -605,7 +605,7 @@ TEST_F(DispatcherTest, DisableTimerWithStream) { TestHeaderMapImpl headers; HttpTestUtility::addDefaultHeaders(headers, "HEAD"); - envoy_headers c_headers = Utility::transformHeaders(headers); + envoy_headers c_headers = Utility::toBridgeHeaders(headers); EXPECT_CALL(cm_, httpAsyncClientForCluster("egress_cluster")) .WillOnce(ReturnRef(cm_.async_client_)); diff --git a/mobile/test/common/http/header_utility_test.cc b/mobile/test/common/http/header_utility_test.cc index da539ebfb6e8..3a0e2c366c96 100644 --- a/mobile/test/common/http/header_utility_test.cc +++ b/mobile/test/common/http/header_utility_test.cc @@ -7,18 +7,22 @@ namespace Envoy { namespace Http { -envoy_data envoyString(std::string& s) { - return {s.size(), reinterpret_cast(s.c_str()), envoy_noop_release, nullptr}; +void envoy_test_release(void* context) { + uint32_t* counter = static_cast(context); + *counter = *counter + 1; +} + +envoy_data envoyTestString(std::string& s, uint32_t* sentinel) { + return {s.size(), reinterpret_cast(s.c_str()), envoy_test_release, sentinel}; } TEST(HeaderDataConstructorTest, FromCToCppEmpty) { envoy_header* header_array = new envoy_header[0]; envoy_headers empty_headers = {0, header_array}; - HeaderMapPtr cpp_headers = Utility::transformHeaders(empty_headers); + HeaderMapPtr cpp_headers = Utility::toInternalHeaders(empty_headers); ASSERT_TRUE(cpp_headers->empty()); - delete[] header_array; } TEST(HeaderDataConstructorTest, FromCToCpp) { @@ -28,22 +32,30 @@ TEST(HeaderDataConstructorTest, FromCToCpp) { envoy_header* header_array = new envoy_header[headers.size()]; + uint32_t* sentinel = new uint32_t; + *sentinel = 0; for (size_t i = 0; i < headers.size(); i++) { header_array[i] = { - envoyString(headers[i].first), - envoyString(headers[i].second), + envoyTestString(headers[i].first, sentinel), + envoyTestString(headers[i].second, sentinel), }; } envoy_headers c_headers = {static_cast(headers.size()), header_array}; + // This copy is used for assertions given that envoy_headers are released when toInternalHeaders + // is called. + envoy_headers c_headers_copy = copy_envoy_headers(c_headers); - HeaderMapPtr cpp_headers = Utility::transformHeaders(c_headers); + HeaderMapPtr cpp_headers = Utility::toInternalHeaders(c_headers); - ASSERT_EQ(cpp_headers->size(), c_headers.length); + // Check that the sentinel was advance due to c_headers being released; + ASSERT_EQ(*sentinel, 2 * c_headers_copy.length); - for (envoy_header_size_t i = 0; i < c_headers.length; i++) { - auto expected_key = LowerCaseString(Utility::convertToString(c_headers.headers[i].key)); - auto expected_value = Utility::convertToString(c_headers.headers[i].value); + ASSERT_EQ(cpp_headers->size(), c_headers_copy.length); + + for (envoy_header_size_t i = 0; i < c_headers_copy.length; i++) { + auto expected_key = LowerCaseString(Utility::convertToString(c_headers_copy.headers[i].key)); + auto expected_value = Utility::convertToString(c_headers_copy.headers[i].value); // Key is present. EXPECT_NE(cpp_headers->get(expected_key), nullptr); @@ -54,7 +66,7 @@ TEST(HeaderDataConstructorTest, FromCToCpp) { TEST(HeaderDataConstructorTest, FromCppToCEmpty) { HeaderMapImpl empty_headers; - envoy_headers c_headers = Utility::transformHeaders(std::move(empty_headers)); + envoy_headers c_headers = Utility::toBridgeHeaders(std::move(empty_headers)); ASSERT_EQ(0, c_headers.length); delete[] c_headers.headers; } @@ -66,7 +78,7 @@ TEST(HeaderDataConstructorTest, FromCppToC) { cpp_headers.addCopy(LowerCaseString(std::string(":authority")), std::string("api.lyft.com")); cpp_headers.addCopy(LowerCaseString(std::string(":path")), std::string("/ping")); - envoy_headers c_headers = Utility::transformHeaders(std::move(cpp_headers)); + envoy_headers c_headers = Utility::toBridgeHeaders(std::move(cpp_headers)); ASSERT_EQ(c_headers.length, static_cast(cpp_headers.size()));