Remeve memset(0) from arena allocated memory.

Callers are updated to properly initialize the memory.

This behavior can be overridden using GRPC_ARENA_INIT_STRATEGY
environment variable.

6 files changed, 166 insertions, 123 deletions

diff --git a/src/core/lib/security/context/security_context.cc b/src/core/lib/security/context/security_context.cc
index 94c9c69fcd..16f40b4f55 100644
--- a/src/core/lib/security/context/security_context.cc
+++ b/src/core/lib/security/context/security_context.cc
@@ -81,38 +81,45 @@ void grpc_auth_context_release(grpc_auth_context* context) {
 }
 
 /* --- grpc_client_security_context --- */
+grpc_client_security_context::~grpc_client_security_context() {
+  grpc_call_credentials_unref(creds);
+  GRPC_AUTH_CONTEXT_UNREF(auth_context, "client_security_context");
+  if (extension.instance != nullptr && extension.destroy != nullptr) {
+    extension.destroy(extension.instance);
+  }
+}
 
 grpc_client_security_context* grpc_client_security_context_create(
     gpr_arena* arena) {
-  return static_cast<grpc_client_security_context*>(
-      gpr_arena_alloc(arena, sizeof(grpc_client_security_context)));
+  return new (gpr_arena_alloc(arena, sizeof(grpc_client_security_context)))
+      grpc_client_security_context();
 }
 
 void grpc_client_security_context_destroy(void* ctx) {
   grpc_core::ExecCtx exec_ctx;
   grpc_client_security_context* c =
       static_cast<grpc_client_security_context*>(ctx);
-  grpc_call_credentials_unref(c->creds);
-  GRPC_AUTH_CONTEXT_UNREF(c->auth_context, "client_security_context");
-  if (c->extension.instance != nullptr && c->extension.destroy != nullptr) {
-    c->extension.destroy(c->extension.instance);
-  }
+  c->~grpc_client_security_context();
 }
 
 /* --- grpc_server_security_context --- */
+grpc_server_security_context::~grpc_server_security_context() {
+  GRPC_AUTH_CONTEXT_UNREF(auth_context, "server_security_context");
+  if (extension.instance != nullptr && extension.destroy != nullptr) {
+    extension.destroy(extension.instance);
+  }
+}
+
 grpc_server_security_context* grpc_server_security_context_create(
     gpr_arena* arena) {
-  return static_cast<grpc_server_security_context*>(
-      gpr_arena_alloc(arena, sizeof(grpc_server_security_context)));
+  return new (gpr_arena_alloc(arena, sizeof(grpc_server_security_context)))
+      grpc_server_security_context();
 }
 
 void grpc_server_security_context_destroy(void* ctx) {
   grpc_server_security_context* c =
       static_cast<grpc_server_security_context*>(ctx);
-  GRPC_AUTH_CONTEXT_UNREF(c->auth_context, "server_security_context");
-  if (c->extension.instance != nullptr && c->extension.destroy != nullptr) {
-    c->extension.destroy(c->extension.instance);
-  }
+  c->~grpc_server_security_context();
 }
 
 /* --- grpc_auth_context --- */
diff --git a/src/core/lib/security/context/security_context.h b/src/core/lib/security/context/security_context.h
index a8e1c3fd64..e45415f63b 100644
--- a/src/core/lib/security/context/security_context.h
+++ b/src/core/lib/security/context/security_context.h
@@ -34,18 +34,20 @@ struct gpr_arena;
 
 /* Property names are always NULL terminated. */
 
-typedef struct {
-  grpc_auth_property* array;
-  size_t count;
-  size_t capacity;
-} grpc_auth_property_array;
+struct grpc_auth_property_array {
+  grpc_auth_property* array = nullptr;
+  size_t count = 0;
+  size_t capacity = 0;
+};
 
 struct grpc_auth_context {
-  struct grpc_auth_context* chained;
+  grpc_auth_context() { gpr_ref_init(&refcount, 0); }
+
+  struct grpc_auth_context* chained = nullptr;
   grpc_auth_property_array properties;
   gpr_refcount refcount;
-  const char* peer_identity_property_name;
-  grpc_pollset* pollset;
+  const char* peer_identity_property_name = nullptr;
+  grpc_pollset* pollset = nullptr;
 };
 
 /* Creation. */
@@ -76,20 +78,23 @@ void grpc_auth_property_reset(grpc_auth_property* property);
    Extension to the security context that may be set in a filter and accessed
    later by a higher level method on a grpc_call object. */
 
-typedef struct {
-  void* instance;
-  void (*destroy)(void*);
-} grpc_security_context_extension;
+struct grpc_security_context_extension {
+  void* instance = nullptr;
+  void (*destroy)(void*) = nullptr;
+};
 
 /* --- grpc_client_security_context ---
 
    Internal client-side security context. */
 
-typedef struct {
-  grpc_call_credentials* creds;
-  grpc_auth_context* auth_context;
+struct grpc_client_security_context {
+  grpc_client_security_context() = default;
+  ~grpc_client_security_context();
+
+  grpc_call_credentials* creds = nullptr;
+  grpc_auth_context* auth_context = nullptr;
   grpc_security_context_extension extension;
-} grpc_client_security_context;
+};
 
 grpc_client_security_context* grpc_client_security_context_create(
     gpr_arena* arena);
@@ -99,10 +104,13 @@ void grpc_client_security_context_destroy(void* ctx);
 
    Internal server-side security context. */
 
-typedef struct {
-  grpc_auth_context* auth_context;
+struct grpc_server_security_context {
+  grpc_server_security_context() = default;
+  ~grpc_server_security_context();
+
+  grpc_auth_context* auth_context = nullptr;
   grpc_security_context_extension extension;
-} grpc_server_security_context;
+};
 
 grpc_server_security_context* grpc_server_security_context_create(
     gpr_arena* arena);
diff --git a/src/core/lib/security/credentials/credentials.h b/src/core/lib/security/credentials/credentials.h
index b486d25ab2..3878958b38 100644
--- a/src/core/lib/security/credentials/credentials.h
+++ b/src/core/lib/security/credentials/credentials.h
@@ -142,8 +142,8 @@ grpc_channel_credentials* grpc_channel_credentials_find_in_args(
 /* --- grpc_credentials_mdelem_array. --- */
 
 typedef struct {
-  grpc_mdelem* md;
-  size_t size;
+  grpc_mdelem* md = nullptr;
+  size_t size = 0;
 } grpc_credentials_mdelem_array;
 
 /// Takes a new ref to \a md.
diff --git a/src/core/lib/security/transport/client_auth_filter.cc b/src/core/lib/security/transport/client_auth_filter.cc
index e34eacc8d7..6955e8698e 100644
--- a/src/core/lib/security/transport/client_auth_filter.cc
+++ b/src/core/lib/security/transport/client_auth_filter.cc
@@ -43,20 +43,39 @@
 namespace {
 /* We can have a per-call credentials. */
 struct call_data {
+  call_data(grpc_call_element* elem, const grpc_call_element_args& args)
+      : arena(args.arena),
+        owning_call(args.call_stack),
+        call_combiner(args.call_combiner) {}
+
+  // This method is technically the dtor of this class. However, since
+  // `get_request_metadata_cancel_closure` can run in parallel to
+  // `destroy_call_elem`, we cannot call the dtor in them. Otherwise,
+  // fields will be accessed after calling dtor, and msan correctly complains
+  // that the memory is not initialized.
+  void destroy() {
+    grpc_credentials_mdelem_array_destroy(&md_array);
+    grpc_call_credentials_unref(creds);
+    grpc_slice_unref_internal(host);
+    grpc_slice_unref_internal(method);
+    grpc_auth_metadata_context_reset(&auth_md_context);
+  }
+
   gpr_arena* arena;
   grpc_call_stack* owning_call;
   grpc_call_combiner* call_combiner;
-  grpc_call_credentials* creds;
-  grpc_slice host;
-  grpc_slice method;
+  grpc_call_credentials* creds = nullptr;
+  grpc_slice host = grpc_empty_slice();
+  grpc_slice method = grpc_empty_slice();
   /* pollset{_set} bound to this call; if we need to make external
      network requests, they should be done under a pollset added to this
      pollset_set so that work can progress when this call wants work to progress
   */
-  grpc_polling_entity* pollent;
+  grpc_polling_entity* pollent = nullptr;
   grpc_credentials_mdelem_array md_array;
-  grpc_linked_mdelem md_links[MAX_CREDENTIALS_METADATA_COUNT];
-  grpc_auth_metadata_context auth_md_context;
+  grpc_linked_mdelem md_links[MAX_CREDENTIALS_METADATA_COUNT] = {};
+  grpc_auth_metadata_context auth_md_context =
+      grpc_auth_metadata_context();  // Zero-initialize the C struct.
   grpc_closure async_result_closure;
   grpc_closure check_call_host_cancel_closure;
   grpc_closure get_request_metadata_cancel_closure;
@@ -334,12 +353,7 @@ static void auth_start_transport_stream_op_batch(
 /* Constructor for call_data */
 static grpc_error* init_call_elem(grpc_call_element* elem,
                                   const grpc_call_element_args* args) {
-  call_data* calld = static_cast<call_data*>(elem->call_data);
-  calld->arena = args->arena;
-  calld->owning_call = args->call_stack;
-  calld->call_combiner = args->call_combiner;
-  calld->host = grpc_empty_slice();
-  calld->method = grpc_empty_slice();
+  new (elem->call_data) call_data(elem, *args);
   return GRPC_ERROR_NONE;
 }
 
@@ -354,11 +368,7 @@ static void destroy_call_elem(grpc_call_element* elem,
                               const grpc_call_final_info* final_info,
                               grpc_closure* ignored) {
   call_data* calld = static_cast<call_data*>(elem->call_data);
-  grpc_credentials_mdelem_array_destroy(&calld->md_array);
-  grpc_call_credentials_unref(calld->creds);
-  grpc_slice_unref_internal(calld->host);
-  grpc_slice_unref_internal(calld->method);
-  grpc_auth_metadata_context_reset(&calld->auth_md_context);
+  calld->destroy();
 }
 
 /* Constructor for channel_data */
diff --git a/src/core/lib/security/transport/secure_endpoint.cc b/src/core/lib/security/transport/secure_endpoint.cc
index f40f969bb7..34d8435907 100644
--- a/src/core/lib/security/transport/secure_endpoint.cc
+++ b/src/core/lib/security/transport/secure_endpoint.cc
@@ -22,6 +22,8 @@
    headers. Therefore, sockaddr.h must always be included first */
 #include <grpc/support/port_platform.h>
 
+#include <new>
+
 #include "src/core/lib/iomgr/sockaddr.h"
 
 #include <grpc/slice.h>
@@ -31,6 +33,7 @@
 #include <grpc/support/sync.h>
 #include "src/core/lib/debug/trace.h"
 #include "src/core/lib/gpr/string.h"
+#include "src/core/lib/gprpp/memory.h"
 #include "src/core/lib/profiling/timers.h"
 #include "src/core/lib/security/transport/secure_endpoint.h"
 #include "src/core/lib/security/transport/tsi_error.h"
@@ -40,44 +43,68 @@
 
 #define STAGING_BUFFER_SIZE 8192
 
-typedef struct {
+static void on_read(void* user_data, grpc_error* error);
+
+namespace {
+struct secure_endpoint {
+  secure_endpoint(const grpc_endpoint_vtable* vtable,
+                  tsi_frame_protector* protector,
+                  tsi_zero_copy_grpc_protector* zero_copy_protector,
+                  grpc_endpoint* transport, grpc_slice* leftover_slices,
+                  size_t leftover_nslices)
+      : wrapped_ep(transport),
+        protector(protector),
+        zero_copy_protector(zero_copy_protector) {
+    base.vtable = vtable;
+    gpr_mu_init(&protector_mu);
+    GRPC_CLOSURE_INIT(&on_read, ::on_read, this, grpc_schedule_on_exec_ctx);
+    grpc_slice_buffer_init(&source_buffer);
+    grpc_slice_buffer_init(&leftover_bytes);
+    for (size_t i = 0; i < leftover_nslices; i++) {
+      grpc_slice_buffer_add(&leftover_bytes,
+                            grpc_slice_ref_internal(leftover_slices[i]));
+    }
+    grpc_slice_buffer_init(&output_buffer);
+    gpr_ref_init(&ref, 1);
+  }
+
+  ~secure_endpoint() {
+    grpc_endpoint_destroy(wrapped_ep);
+    tsi_frame_protector_destroy(protector);
+    tsi_zero_copy_grpc_protector_destroy(zero_copy_protector);
+    grpc_slice_buffer_destroy_internal(&source_buffer);
+    grpc_slice_buffer_destroy_internal(&leftover_bytes);
+    grpc_slice_unref_internal(read_staging_buffer);
+    grpc_slice_unref_internal(write_staging_buffer);
+    grpc_slice_buffer_destroy_internal(&output_buffer);
+    gpr_mu_destroy(&protector_mu);
+  }
+
   grpc_endpoint base;
   grpc_endpoint* wrapped_ep;
   struct tsi_frame_protector* protector;
   struct tsi_zero_copy_grpc_protector* zero_copy_protector;
   gpr_mu protector_mu;
   /* saved upper level callbacks and user_data. */
-  grpc_closure* read_cb;
-  grpc_closure* write_cb;
+  grpc_closure* read_cb = nullptr;
+  grpc_closure* write_cb = nullptr;
   grpc_closure on_read;
-  grpc_slice_buffer* read_buffer;
+  grpc_slice_buffer* read_buffer = nullptr;
   grpc_slice_buffer source_buffer;
   /* saved handshaker leftover data to unprotect. */
   grpc_slice_buffer leftover_bytes;
   /* buffers for read and write */
-  grpc_slice read_staging_buffer;
-
-  grpc_slice write_staging_buffer;
+  grpc_slice read_staging_buffer = GRPC_SLICE_MALLOC(STAGING_BUFFER_SIZE);
+  grpc_slice write_staging_buffer = GRPC_SLICE_MALLOC(STAGING_BUFFER_SIZE);
   grpc_slice_buffer output_buffer;
 
   gpr_refcount ref;
-} secure_endpoint;
+};
+}  // namespace
 
 grpc_core::TraceFlag grpc_trace_secure_endpoint(false, "secure_endpoint");
 
-static void destroy(secure_endpoint* secure_ep) {
-  secure_endpoint* ep = secure_ep;
-  grpc_endpoint_destroy(ep->wrapped_ep);
-  tsi_frame_protector_destroy(ep->protector);
-  tsi_zero_copy_grpc_protector_destroy(ep->zero_copy_protector);
-  grpc_slice_buffer_destroy_internal(&ep->leftover_bytes);
-  grpc_slice_unref_internal(ep->read_staging_buffer);
-  grpc_slice_unref_internal(ep->write_staging_buffer);
-  grpc_slice_buffer_destroy_internal(&ep->output_buffer);
-  grpc_slice_buffer_destroy_internal(&ep->source_buffer);
-  gpr_mu_destroy(&ep->protector_mu);
-  gpr_free(ep);
-}
+static void destroy(secure_endpoint* ep) { grpc_core::Delete(ep); }
 
 #ifndef NDEBUG
 #define SECURE_ENDPOINT_UNREF(ep, reason) \
@@ -405,25 +432,8 @@ grpc_endpoint* grpc_secure_endpoint_create(
     struct tsi_zero_copy_grpc_protector* zero_copy_protector,
     grpc_endpoint* transport, grpc_slice* leftover_slices,
     size_t leftover_nslices) {
-  size_t i;
-  secure_endpoint* ep =
-      static_cast<secure_endpoint*>(gpr_malloc(sizeof(secure_endpoint)));
-  ep->base.vtable = &vtable;
-  ep->wrapped_ep = transport;
-  ep->protector = protector;
-  ep->zero_copy_protector = zero_copy_protector;
-  grpc_slice_buffer_init(&ep->leftover_bytes);
-  for (i = 0; i < leftover_nslices; i++) {
-    grpc_slice_buffer_add(&ep->leftover_bytes,
-                          grpc_slice_ref_internal(leftover_slices[i]));
-  }
-  ep->write_staging_buffer = GRPC_SLICE_MALLOC(STAGING_BUFFER_SIZE);
-  ep->read_staging_buffer = GRPC_SLICE_MALLOC(STAGING_BUFFER_SIZE);
-  grpc_slice_buffer_init(&ep->output_buffer);
-  grpc_slice_buffer_init(&ep->source_buffer);
-  ep->read_buffer = nullptr;
-  GRPC_CLOSURE_INIT(&ep->on_read, on_read, ep, grpc_schedule_on_exec_ctx);
-  gpr_mu_init(&ep->protector_mu);
-  gpr_ref_init(&ep->ref, 1);
+  secure_endpoint* ep = grpc_core::New<secure_endpoint>(
+      &vtable, protector, zero_copy_protector, transport, leftover_slices,
+      leftover_nslices);
   return &ep->base;
 }
diff --git a/src/core/lib/security/transport/server_auth_filter.cc b/src/core/lib/security/transport/server_auth_filter.cc
index b99fc5e178..362f49a584 100644
--- a/src/core/lib/security/transport/server_auth_filter.cc
+++ b/src/core/lib/security/transport/server_auth_filter.cc
@@ -28,6 +28,9 @@
 #include "src/core/lib/security/transport/auth_filters.h"
 #include "src/core/lib/slice/slice_internal.h"
 
+static void recv_initial_metadata_ready(void* arg, grpc_error* error);
+static void recv_trailing_metadata_ready(void* user_data, grpc_error* error);
+
 namespace {
 enum async_state {
   STATE_INIT = 0,
@@ -35,28 +38,55 @@ enum async_state {
   STATE_CANCELLED,
 };
 
+struct channel_data {
+  grpc_auth_context* auth_context;
+  grpc_server_credentials* creds;
+};
+
 struct call_data {
+  call_data(grpc_call_element* elem, const grpc_call_element_args& args)
+      : call_combiner(args.call_combiner), owning_call(args.call_stack) {
+    GRPC_CLOSURE_INIT(&recv_initial_metadata_ready,
+                      ::recv_initial_metadata_ready, elem,
+                      grpc_schedule_on_exec_ctx);
+    GRPC_CLOSURE_INIT(&recv_trailing_metadata_ready,
+                      ::recv_trailing_metadata_ready, elem,
+                      grpc_schedule_on_exec_ctx);
+    // Create server security context.  Set its auth context from channel
+    // data and save it in the call context.
+    grpc_server_security_context* server_ctx =
+        grpc_server_security_context_create(args.arena);
+    channel_data* chand = static_cast<channel_data*>(elem->channel_data);
+    server_ctx->auth_context =
+        GRPC_AUTH_CONTEXT_REF(chand->auth_context, "server_auth_filter");
+    if (args.context[GRPC_CONTEXT_SECURITY].value != nullptr) {
+      args.context[GRPC_CONTEXT_SECURITY].destroy(
+          args.context[GRPC_CONTEXT_SECURITY].value);
+    }
+    args.context[GRPC_CONTEXT_SECURITY].value = server_ctx;
+    args.context[GRPC_CONTEXT_SECURITY].destroy =
+        grpc_server_security_context_destroy;
+  }
+
+  ~call_data() { GRPC_ERROR_UNREF(recv_initial_metadata_error); }
+
   grpc_call_combiner* call_combiner;
   grpc_call_stack* owning_call;
   grpc_transport_stream_op_batch* recv_initial_metadata_batch;
   grpc_closure* original_recv_initial_metadata_ready;
   grpc_closure recv_initial_metadata_ready;
-  grpc_error* recv_initial_metadata_error;
+  grpc_error* recv_initial_metadata_error = GRPC_ERROR_NONE;
   grpc_closure recv_trailing_metadata_ready;
   grpc_closure* original_recv_trailing_metadata_ready;
   grpc_error* recv_trailing_metadata_error;
-  bool seen_recv_trailing_metadata_ready;
+  bool seen_recv_trailing_metadata_ready = false;
   grpc_metadata_array md;
   const grpc_metadata* consumed_md;
   size_t num_consumed_md;
   grpc_closure cancel_closure;
-  gpr_atm state;  // async_state
+  gpr_atm state = STATE_INIT;  // async_state
 };
 
-struct channel_data {
-  grpc_auth_context* auth_context;
-  grpc_server_credentials* creds;
-};
 }  // namespace
 
 static grpc_metadata_array metadata_batch_to_md_array(
@@ -244,29 +274,7 @@ static void auth_start_transport_stream_op_batch(
 /* Constructor for call_data */
 static grpc_error* init_call_elem(grpc_call_element* elem,
                                   const grpc_call_element_args* args) {
-  call_data* calld = static_cast<call_data*>(elem->call_data);
-  channel_data* chand = static_cast<channel_data*>(elem->channel_data);
-  calld->call_combiner = args->call_combiner;
-  calld->owning_call = args->call_stack;
-  GRPC_CLOSURE_INIT(&calld->recv_initial_metadata_ready,
-                    recv_initial_metadata_ready, elem,
-                    grpc_schedule_on_exec_ctx);
-  GRPC_CLOSURE_INIT(&calld->recv_trailing_metadata_ready,
-                    recv_trailing_metadata_ready, elem,
-                    grpc_schedule_on_exec_ctx);
-  // Create server security context.  Set its auth context from channel
-  // data and save it in the call context.
-  grpc_server_security_context* server_ctx =
-      grpc_server_security_context_create(args->arena);
-  server_ctx->auth_context =
-      GRPC_AUTH_CONTEXT_REF(chand->auth_context, "server_auth_filter");
-  if (args->context[GRPC_CONTEXT_SECURITY].value != nullptr) {
-    args->context[GRPC_CONTEXT_SECURITY].destroy(
-        args->context[GRPC_CONTEXT_SECURITY].value);
-  }
-  args->context[GRPC_CONTEXT_SECURITY].value = server_ctx;
-  args->context[GRPC_CONTEXT_SECURITY].destroy =
-      grpc_server_security_context_destroy;
+  new (elem->call_data) call_data(elem, *args);
   return GRPC_ERROR_NONE;
 }
 
@@ -275,7 +283,7 @@ static void destroy_call_elem(grpc_call_element* elem,
                               const grpc_call_final_info* final_info,
                               grpc_closure* ignored) {
   call_data* calld = static_cast<call_data*>(elem->call_data);
-  GRPC_ERROR_UNREF(calld->recv_initial_metadata_error);
+  calld->~call_data();
 }
 
 /* Constructor for channel_data */