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 <jnino@lyft.com>
Signed-off-by: JP Simard <jp@jpsim.com>

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

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<uint8_t*>(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<char*>(const_cast<uint8_t*>(s.bytes)), s.length);
 }
 
-HeaderMapPtr transformHeaders(envoy_headers headers) {
+HeaderMapPtr toInternalHeaders(envoy_headers headers) {
   Http::HeaderMapPtr transformed_headers = std::make_unique<HeaderMapImpl>();
   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<envoy_header*>(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<const uint8_t*>(header_key.data()));
-        envoy_data value = copyEnvoyData(header_value.size(),
-                                         reinterpret_cast<const uint8_t*>(header_value.data()));
+            copy_envoy_data(header_key.size(), reinterpret_cast<const uint8_t*>(header_key.data()));
+        envoy_data value = copy_envoy_data(header_value.size(),
+                                           reinterpret_cast<const uint8_t*>(header_value.data()));
 
         transformed_headers->headers[transformed_headers->length] = {key, value};
         transformed_headers->length++;

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

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"],
 )

mobile/library/common/include/c_types.c

@@ -0,0 +1,41 @@
+#include "library/common/include/c_types.h"
+
+#include <string.h>
+
+// 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};

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;

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;
 }
 

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<callbacks_called*>(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<callbacks_called*>(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<bool*>(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<callbacks_called*>(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<callbacks_called*>(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_));