Better error messages for bad metadata

2 files changed, 40 insertions, 15 deletions

diff --git a/src/core/lib/surface/call.c b/src/core/lib/surface/call.c
index 9c9ac46bed..1425913f5a 100644
--- a/src/core/lib/surface/call.c
+++ b/src/core/lib/surface/call.c
@@ -56,6 +56,7 @@
 #include "src/core/lib/surface/call.h"
 #include "src/core/lib/surface/channel.h"
 #include "src/core/lib/surface/completion_queue.h"
+#include "src/core/lib/surface/validate_metadata.h"
 #include "src/core/lib/transport/error_utils.h"
 #include "src/core/lib/transport/metadata.h"
 #include "src/core/lib/transport/static_metadata.h"
@@ -768,16 +769,13 @@ static int prepare_application_metadata(
         get_md_elem(metadata, additional_metadata, i, count);
     grpc_linked_mdelem *l = (grpc_linked_mdelem *)&md->internal_data;
     GPR_ASSERT(sizeof(grpc_linked_mdelem) == sizeof(md->internal_data));
-    if (!grpc_header_key_is_legal(md->key)) {
-      char *str = grpc_slice_to_c_string(md->key);
-      gpr_log(GPR_ERROR, "attempt to send invalid metadata key: %s", str);
-      gpr_free(str);
+    if (!GRPC_LOG_IF_ERROR("validate_metadata",
+                           grpc_validate_header_key_is_legal(md->key))) {
       break;
     } else if (!grpc_is_binary_header(md->key) &&
-               !grpc_header_nonbin_value_is_legal(md->value)) {
-      char *str = grpc_dump_slice(md->value, GPR_DUMP_HEX | GPR_DUMP_ASCII);
-      gpr_log(GPR_ERROR, "attempt to send invalid metadata value: %s", str);
-      gpr_free(str);
+               !GRPC_LOG_IF_ERROR(
+                   "validate_metadata",
+                   grpc_validate_header_nonbin_value_is_legal(md->value))) {
       break;
     }
     l->md = grpc_mdelem_from_grpc_metadata(exec_ctx, (grpc_metadata *)md);
diff --git a/src/core/lib/surface/validate_metadata.c b/src/core/lib/surface/validate_metadata.c
index 4201192e7a..8c11345c65 100644
--- a/src/core/lib/surface/validate_metadata.c
+++ b/src/core/lib/surface/validate_metadata.c
@@ -35,21 +35,40 @@
 #include <string.h>
 
 #include <grpc/grpc.h>
+#include <grpc/support/alloc.h>
 #include <grpc/support/port_platform.h>
 
-static int conforms_to(grpc_slice slice, const uint8_t *legal_bits) {
+#include "src/core/lib/iomgr/error.h"
+#include "src/core/lib/slice/slice_string_helpers.h"
+
+static grpc_error *conforms_to(grpc_slice slice, const uint8_t *legal_bits,
+                               const char *err_desc) {
   const uint8_t *p = GRPC_SLICE_START_PTR(slice);
   const uint8_t *e = GRPC_SLICE_END_PTR(slice);
   for (; p != e; p++) {
     int idx = *p;
     int byte = idx / 8;
     int bit = idx % 8;
-    if ((legal_bits[byte] & (1 << bit)) == 0) return 0;
+    if ((legal_bits[byte] & (1 << bit)) == 0) {
+      char *dump = grpc_dump_slice(slice, GPR_DUMP_HEX | GPR_DUMP_ASCII);
+      grpc_error *error = grpc_error_set_str(
+          grpc_error_set_int(GRPC_ERROR_CREATE(err_desc), GRPC_ERROR_INT_OFFSET,
+                             p - GRPC_SLICE_START_PTR(slice)),
+          GRPC_ERROR_STR_RAW_BYTES, dump);
+      gpr_free(dump);
+      return error;
+    }
   }
-  return 1;
+  return GRPC_ERROR_NONE;
 }
 
-int grpc_header_key_is_legal(grpc_slice slice) {
+static int error2int(grpc_error *error) {
+  int r = (error == GRPC_ERROR_NONE);
+  GRPC_ERROR_UNREF(error);
+  return r;
+}
+
+grpc_error *grpc_validate_header_key_is_legal(grpc_slice slice) {
   static const uint8_t legal_header_bits[256 / 8] = {
       0x00, 0x00, 0x00, 0x00, 0x00, 0x60, 0xff, 0x03, 0x00, 0x00, 0x00,
       0x80, 0xfe, 0xff, 0xff, 0x07, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
@@ -57,15 +76,23 @@ int grpc_header_key_is_legal(grpc_slice slice) {
   if (GRPC_SLICE_LENGTH(slice) == 0 || GRPC_SLICE_START_PTR(slice)[0] == ':') {
     return 0;
   }
-  return conforms_to(slice, legal_header_bits);
+  return conforms_to(slice, legal_header_bits, "Illegal header key");
 }
 
-int grpc_header_nonbin_value_is_legal(grpc_slice slice) {
+int grpc_header_key_is_legal(grpc_slice slice) {
+  return error2int(grpc_validate_header_key_is_legal(slice));
+}
+
+grpc_error *grpc_validate_header_nonbin_value_is_legal(grpc_slice slice) {
   static const uint8_t legal_header_bits[256 / 8] = {
       0x00, 0x00, 0x00, 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
       0xff, 0xff, 0xff, 0xff, 0x7f, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
       0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
-  return conforms_to(slice, legal_header_bits);
+  return conforms_to(slice, legal_header_bits, "Illegal header value");
+}
+
+int grpc_header_nonbin_value_is_legal(grpc_slice slice) {
+  return error2int(grpc_validate_header_nonbin_value_is_legal(slice));
 }
 
 int grpc_is_binary_header(grpc_slice slice) {