Revert "Revert "Metadata handling rewrite""

This reverts commit 5e01e2ac977655aa074faf7fde0a74298f5e4c55.

4 files changed, 102 insertions, 86 deletions

diff --git a/src/core/lib/security/transport/client_auth_filter.c b/src/core/lib/security/transport/client_auth_filter.c
index b7f6fd23e3..cf056e8008 100644
--- a/src/core/lib/security/transport/client_auth_filter.c
+++ b/src/core/lib/security/transport/client_auth_filter.c
@@ -45,6 +45,7 @@
 #include "src/core/lib/security/credentials/credentials.h"
 #include "src/core/lib/security/transport/security_connector.h"
 #include "src/core/lib/slice/slice_internal.h"
+#include "src/core/lib/slice/slice_string_helpers.h"
 #include "src/core/lib/support/string.h"
 #include "src/core/lib/surface/call.h"
 #include "src/core/lib/transport/static_metadata.h"
@@ -54,8 +55,10 @@
 /* We can have a per-call credentials. */
 typedef struct {
   grpc_call_credentials *creds;
-  grpc_mdstr *host;
-  grpc_mdstr *method;
+  bool have_host;
+  bool have_method;
+  grpc_slice host;
+  grpc_slice method;
   /* 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
@@ -89,14 +92,12 @@ static void reset_auth_metadata_context(
   auth_md_context->channel_auth_context = NULL;
 }
 
-static void bubble_up_error(grpc_exec_ctx *exec_ctx, grpc_call_element *elem,
-                            grpc_status_code status, const char *error_msg) {
-  call_data *calld = elem->call_data;
-  gpr_log(GPR_ERROR, "Client side authentication failure: %s", error_msg);
-  grpc_slice error_slice = grpc_slice_from_copied_string(error_msg);
-  grpc_transport_stream_op_add_close(exec_ctx, &calld->op, status,
-                                     &error_slice);
-  grpc_call_next_op(exec_ctx, elem, &calld->op);
+static void add_error(grpc_error **combined, grpc_error *error) {
+  if (error == GRPC_ERROR_NONE) return;
+  if (*combined == GRPC_ERROR_NONE) {
+    *combined = GRPC_ERROR_CREATE("Client auth metadata plugin error");
+  }
+  *combined = grpc_error_add_child(*combined, error);
 }
 
 static void on_credentials_metadata(grpc_exec_ctx *exec_ctx, void *user_data,
@@ -110,30 +111,37 @@ static void on_credentials_metadata(grpc_exec_ctx *exec_ctx, void *user_data,
   grpc_metadata_batch *mdb;
   size_t i;
   reset_auth_metadata_context(&calld->auth_md_context);
+  grpc_error *error = GRPC_ERROR_NONE;
   if (status != GRPC_CREDENTIALS_OK) {
-    bubble_up_error(exec_ctx, elem, GRPC_STATUS_UNAUTHENTICATED,
-                    (error_details != NULL && strlen(error_details) > 0)
-                        ? error_details
-                        : "Credentials failed to get metadata.");
-    return;
+    error = grpc_error_set_int(
+        GRPC_ERROR_CREATE(error_details != NULL && strlen(error_details) > 0
+                              ? error_details
+                              : "Credentials failed to get metadata."),
+        GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_UNAUTHENTICATED);
+  } else {
+    GPR_ASSERT(num_md <= MAX_CREDENTIALS_METADATA_COUNT);
+    GPR_ASSERT(op->send_initial_metadata != NULL);
+    mdb = op->send_initial_metadata;
+    for (i = 0; i < num_md; i++) {
+      add_error(&error,
+                grpc_metadata_batch_add_tail(
+                    exec_ctx, mdb, &calld->md_links[i],
+                    grpc_mdelem_from_slices(
+                        exec_ctx, grpc_slice_ref_internal(md_elems[i].key),
+                        grpc_slice_ref_internal(md_elems[i].value))));
+    }
   }
-  GPR_ASSERT(num_md <= MAX_CREDENTIALS_METADATA_COUNT);
-  GPR_ASSERT(op->send_initial_metadata != NULL);
-  mdb = op->send_initial_metadata;
-  for (i = 0; i < num_md; i++) {
-    grpc_metadata_batch_add_tail(
-        mdb, &calld->md_links[i],
-        grpc_mdelem_from_slices(exec_ctx,
-                                grpc_slice_ref_internal(md_elems[i].key),
-                                grpc_slice_ref_internal(md_elems[i].value)));
+  if (error == GRPC_ERROR_NONE) {
+    grpc_call_next_op(exec_ctx, elem, op);
+  } else {
+    grpc_transport_stream_op_finish_with_failure(exec_ctx, op, error);
   }
-  grpc_call_next_op(exec_ctx, elem, op);
 }
 
 void build_auth_metadata_context(grpc_security_connector *sc,
                                  grpc_auth_context *auth_context,
                                  call_data *calld) {
-  char *service = gpr_strdup(grpc_mdstr_as_c_string(calld->method));
+  char *service = grpc_slice_to_c_string(calld->method);
   char *last_slash = strrchr(service, '/');
   char *method_name = NULL;
   char *service_url = NULL;
@@ -149,14 +157,15 @@ void build_auth_metadata_context(grpc_security_connector *sc,
     method_name = gpr_strdup(last_slash + 1);
   }
   if (method_name == NULL) method_name = gpr_strdup("");
+  char *host = grpc_slice_to_c_string(calld->host);
   gpr_asprintf(&service_url, "%s://%s%s",
-               sc->url_scheme == NULL ? "" : sc->url_scheme,
-               grpc_mdstr_as_c_string(calld->host), service);
+               sc->url_scheme == NULL ? "" : sc->url_scheme, host, service);
   calld->auth_md_context.service_url = service_url;
   calld->auth_md_context.method_name = method_name;
   calld->auth_md_context.channel_auth_context =
       GRPC_AUTH_CONTEXT_REF(auth_context, "grpc_auth_metadata_context");
   gpr_free(service);
+  gpr_free(host);
 }
 
 static void send_security_metadata(grpc_exec_ctx *exec_ctx,
@@ -180,8 +189,12 @@ static void send_security_metadata(grpc_exec_ctx *exec_ctx,
     calld->creds = grpc_composite_call_credentials_create(channel_call_creds,
                                                           ctx->creds, NULL);
     if (calld->creds == NULL) {
-      bubble_up_error(exec_ctx, elem, GRPC_STATUS_UNAUTHENTICATED,
-                      "Incompatible credentials set on channel and call.");
+      grpc_transport_stream_op_finish_with_failure(
+          exec_ctx, op,
+          grpc_error_set_int(
+              GRPC_ERROR_CREATE(
+                  "Incompatible credentials set on channel and call."),
+              GRPC_ERROR_INT_GRPC_STATUS, GRPC_STATUS_UNAUTHENTICATED));
       return;
     }
   } else {
@@ -207,9 +220,14 @@ static void on_host_checked(grpc_exec_ctx *exec_ctx, void *user_data,
     send_security_metadata(exec_ctx, elem, &calld->op);
   } else {
     char *error_msg;
+    char *host = grpc_slice_to_c_string(calld->host);
     gpr_asprintf(&error_msg, "Invalid host %s set in :authority metadata.",
-                 grpc_mdstr_as_c_string(calld->host));
-    bubble_up_error(exec_ctx, elem, GRPC_STATUS_UNAUTHENTICATED, error_msg);
+                 host);
+    gpr_free(host);
+    grpc_call_element_signal_error(
+        exec_ctx, elem, grpc_error_set_int(GRPC_ERROR_CREATE(error_msg),
+                                           GRPC_ERROR_INT_GRPC_STATUS,
+                                           GRPC_STATUS_UNAUTHENTICATED));
     gpr_free(error_msg);
   }
 }
@@ -247,23 +265,30 @@ static void auth_start_transport_op(grpc_exec_ctx *exec_ctx,
 
   if (op->send_initial_metadata != NULL) {
     for (l = op->send_initial_metadata->list.head; l != NULL; l = l->next) {
-      grpc_mdelem *md = l->md;
+      grpc_mdelem md = l->md;
       /* Pointer comparison is OK for md_elems created from the same context.
        */
-      if (md->key == GRPC_MDSTR_AUTHORITY) {
-        if (calld->host != NULL) GRPC_MDSTR_UNREF(exec_ctx, calld->host);
-        calld->host = GRPC_MDSTR_REF(md->value);
-      } else if (md->key == GRPC_MDSTR_PATH) {
-        if (calld->method != NULL) GRPC_MDSTR_UNREF(exec_ctx, calld->method);
-        calld->method = GRPC_MDSTR_REF(md->value);
+      if (grpc_slice_eq(GRPC_MDKEY(md), GRPC_MDSTR_AUTHORITY)) {
+        if (calld->have_host) {
+          grpc_slice_unref_internal(exec_ctx, calld->host);
+        }
+        calld->host = grpc_slice_ref_internal(GRPC_MDVALUE(md));
+        calld->have_host = true;
+      } else if (grpc_slice_eq(GRPC_MDKEY(md), GRPC_MDSTR_PATH)) {
+        if (calld->have_method) {
+          grpc_slice_unref_internal(exec_ctx, calld->method);
+        }
+        calld->method = grpc_slice_ref_internal(GRPC_MDVALUE(md));
+        calld->have_method = true;
       }
     }
-    if (calld->host != NULL) {
-      const char *call_host = grpc_mdstr_as_c_string(calld->host);
+    if (calld->have_host) {
+      char *call_host = grpc_slice_to_c_string(calld->host);
       calld->op = *op; /* Copy op (originates from the caller's stack). */
       grpc_channel_security_connector_check_call_host(
           exec_ctx, chand->security_connector, call_host, chand->auth_context,
           on_host_checked, elem);
+      gpr_free(call_host);
       GPR_TIMER_END("auth_start_transport_op", 0);
       return; /* early exit */
     }
@@ -296,11 +321,11 @@ static void destroy_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem,
                               void *ignored) {
   call_data *calld = elem->call_data;
   grpc_call_credentials_unref(exec_ctx, calld->creds);
-  if (calld->host != NULL) {
-    GRPC_MDSTR_UNREF(exec_ctx, calld->host);
+  if (calld->have_host) {
+    grpc_slice_unref_internal(exec_ctx, calld->host);
   }
-  if (calld->method != NULL) {
-    GRPC_MDSTR_UNREF(exec_ctx, calld->method);
+  if (calld->have_method) {
+    grpc_slice_unref_internal(exec_ctx, calld->method);
   }
   reset_auth_metadata_context(&calld->auth_md_context);
 }
diff --git a/src/core/lib/security/transport/security_connector.c b/src/core/lib/security/transport/security_connector.c
index 5aa26e0577..b09127811b 100644
--- a/src/core/lib/security/transport/security_connector.c
+++ b/src/core/lib/security/transport/security_connector.c
@@ -601,7 +601,7 @@ static grpc_security_connector_vtable ssl_server_vtable = {
     ssl_server_destroy, ssl_server_check_peer};
 
 static grpc_slice compute_default_pem_root_certs_once(void) {
-  grpc_slice result = gpr_empty_slice();
+  grpc_slice result = grpc_empty_slice();
 
   /* First try to load the roots from the environment. */
   char *default_root_certs_path =
diff --git a/src/core/lib/security/transport/security_handshaker.c b/src/core/lib/security/transport/security_handshaker.c
index 5e75856c7a..37d57d759b 100644
--- a/src/core/lib/security/transport/security_handshaker.c
+++ b/src/core/lib/security/transport/security_handshaker.c
@@ -124,7 +124,7 @@ static void security_handshake_failed_locked(grpc_exec_ctx *exec_ctx,
   }
   const char *msg = grpc_error_string(error);
   gpr_log(GPR_DEBUG, "Security handshake failed: %s", msg);
-  grpc_error_free_string(msg);
+
   if (!h->shutdown) {
     // TODO(ctiller): It is currently necessary to shutdown endpoints
     // before destroying them, even if we know that there are no
diff --git a/src/core/lib/security/transport/server_auth_filter.c b/src/core/lib/security/transport/server_auth_filter.c
index 5e98ba895d..36e81d6501 100644
--- a/src/core/lib/security/transport/server_auth_filter.c
+++ b/src/core/lib/security/transport/server_auth_filter.c
@@ -33,12 +33,13 @@
 
 #include <string.h>
 
+#include <grpc/support/alloc.h>
+#include <grpc/support/log.h>
+
 #include "src/core/lib/security/context/security_context.h"
 #include "src/core/lib/security/credentials/credentials.h"
 #include "src/core/lib/security/transport/auth_filters.h"
-
-#include <grpc/support/alloc.h>
-#include <grpc/support/log.h>
+#include "src/core/lib/slice/slice_internal.h"
 
 typedef struct call_data {
   grpc_metadata_batch *recv_initial_metadata;
@@ -67,48 +68,34 @@ static grpc_metadata_array metadata_batch_to_md_array(
   grpc_metadata_array_init(&result);
   for (l = batch->list.head; l != NULL; l = l->next) {
     grpc_metadata *usr_md = NULL;
-    grpc_mdelem *md = l->md;
-    grpc_mdstr *key = md->key;
-    grpc_mdstr *value = md->value;
+    grpc_mdelem md = l->md;
+    grpc_slice key = GRPC_MDKEY(md);
+    grpc_slice value = GRPC_MDVALUE(md);
     if (result.count == result.capacity) {
       result.capacity = GPR_MAX(result.capacity + 8, result.capacity * 2);
       result.metadata =
           gpr_realloc(result.metadata, result.capacity * sizeof(grpc_metadata));
     }
     usr_md = &result.metadata[result.count++];
-    usr_md->key = grpc_mdstr_as_c_string(key);
-    usr_md->value = grpc_mdstr_as_c_string(value);
-    usr_md->value_length = GRPC_SLICE_LENGTH(value->slice);
+    usr_md->key = grpc_slice_ref_internal(key);
+    usr_md->value = grpc_slice_ref_internal(value);
   }
   return result;
 }
 
-static grpc_mdelem *remove_consumed_md(grpc_exec_ctx *exec_ctx, void *user_data,
-                                       grpc_mdelem *md) {
+static grpc_filtered_mdelem remove_consumed_md(grpc_exec_ctx *exec_ctx,
+                                               void *user_data,
+                                               grpc_mdelem md) {
   grpc_call_element *elem = user_data;
   call_data *calld = elem->call_data;
   size_t i;
   for (i = 0; i < calld->num_consumed_md; i++) {
     const grpc_metadata *consumed_md = &calld->consumed_md[i];
-    /* Maybe we could do a pointer comparison but we do not have any guarantee
-       that the metadata processor used the same pointers for consumed_md in the
-       callback. */
-    if (GRPC_SLICE_LENGTH(md->key->slice) != strlen(consumed_md->key) ||
-        GRPC_SLICE_LENGTH(md->value->slice) != consumed_md->value_length) {
-      continue;
-    }
-    if (memcmp(GRPC_SLICE_START_PTR(md->key->slice), consumed_md->key,
-               GRPC_SLICE_LENGTH(md->key->slice)) == 0 &&
-        memcmp(GRPC_SLICE_START_PTR(md->value->slice), consumed_md->value,
-               GRPC_SLICE_LENGTH(md->value->slice)) == 0) {
-      return NULL; /* Delete. */
-    }
+    if (grpc_slice_eq(GRPC_MDKEY(md), consumed_md->key) &&
+        grpc_slice_eq(GRPC_MDVALUE(md), consumed_md->value))
+      return GRPC_FILTERED_REMOVE();
   }
-  return md;
-}
-
-static void destroy_op(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) {
-  gpr_free(arg);
+  return GRPC_FILTERED_MDELEM(md);
 }
 
 /* called from application code */
@@ -130,29 +117,33 @@ static void on_md_processing_done(
   if (status == GRPC_STATUS_OK) {
     calld->consumed_md = consumed_md;
     calld->num_consumed_md = num_consumed_md;
-    grpc_metadata_batch_filter(&exec_ctx, calld->recv_initial_metadata,
-                               remove_consumed_md, elem);
+    /* TODO(ctiller): propagate error */
+    GRPC_LOG_IF_ERROR(
+        "grpc_metadata_batch_filter",
+        grpc_metadata_batch_filter(&exec_ctx, calld->recv_initial_metadata,
+                                   remove_consumed_md, elem,
+                                   "Response metadata filtering error"));
+    for (size_t i = 0; i < calld->md.count; i++) {
+      grpc_slice_unref_internal(&exec_ctx, calld->md.metadata[i].key);
+      grpc_slice_unref_internal(&exec_ctx, calld->md.metadata[i].value);
+    }
     grpc_metadata_array_destroy(&calld->md);
     grpc_closure_sched(&exec_ctx, calld->on_done_recv, GRPC_ERROR_NONE);
   } else {
-    grpc_slice message;
-    grpc_transport_stream_op *close_op = gpr_malloc(sizeof(*close_op));
-    memset(close_op, 0, sizeof(*close_op));
+    for (size_t i = 0; i < calld->md.count; i++) {
+      grpc_slice_unref_internal(&exec_ctx, calld->md.metadata[i].key);
+      grpc_slice_unref_internal(&exec_ctx, calld->md.metadata[i].value);
+    }
     grpc_metadata_array_destroy(&calld->md);
     error_details = error_details != NULL
                         ? error_details
                         : "Authentication metadata processing failed.";
-    message = grpc_slice_from_copied_string(error_details);
     calld->transport_op->send_initial_metadata = NULL;
     if (calld->transport_op->send_message != NULL) {
       grpc_byte_stream_destroy(&exec_ctx, calld->transport_op->send_message);
       calld->transport_op->send_message = NULL;
     }
     calld->transport_op->send_trailing_metadata = NULL;
-    close_op->on_complete =
-        grpc_closure_create(destroy_op, close_op, grpc_schedule_on_exec_ctx);
-    grpc_transport_stream_op_add_close(&exec_ctx, close_op, status, &message);
-    grpc_call_next_op(&exec_ctx, elem, close_op);
     grpc_closure_sched(&exec_ctx, calld->on_done_recv,
                        grpc_error_set_int(GRPC_ERROR_CREATE(error_details),
                                           GRPC_ERROR_INT_GRPC_STATUS, status));