Merge pull request #13161 from markdroth/service_config_fixes

Fix service config parsing bugs

src/core/ext/filters/client_channel/client_channel.cc

@@ -88,7 +88,12 @@ static void method_parameters_unref(method_parameters *method_params) {
   }
 }
 
-static void method_parameters_free(grpc_exec_ctx *exec_ctx, void *value) {
+// Wrappers to pass to grpc_service_config_create_method_config_table().
+static void *method_parameters_ref_wrapper(void *value) {
+  return method_parameters_ref((method_parameters *)value);
+}
+static void method_parameters_unref_wrapper(grpc_exec_ctx *exec_ctx,
+                                            void *value) {
   method_parameters_unref((method_parameters *)value);
 }
 
@@ -117,24 +122,16 @@ static bool parse_timeout(grpc_json *field, grpc_millis *timeout) {
       gpr_free(buf);
       return false;
     }
-    // There should always be exactly 3, 6, or 9 fractional digits.
-    int multiplier = 1;
-    switch (strlen(decimal_point + 1)) {
-      case 9:
-        break;
-      case 6:
-        multiplier *= 1000;
-        break;
-      case 3:
-        multiplier *= 1000000;
-        break;
-      default:  // Unsupported number of digits.
-        gpr_free(buf);
-        return false;
+    int num_digits = (int)strlen(decimal_point + 1);
+    if (num_digits > 9) {  // We don't accept greater precision than nanos.
+      gpr_free(buf);
+      return false;
+    }
+    for (int i = 0; i < (9 - num_digits); ++i) {
+      nanos *= 10;
     }
-    nanos *= multiplier;
   }
-  int seconds = gpr_parse_nonnegative_int(buf);
+  int seconds = decimal_point == buf ? 0 : gpr_parse_nonnegative_int(buf);
   gpr_free(buf);
   if (seconds == -1) return false;
   *timeout = seconds * GPR_MS_PER_SEC + nanos / GPR_NS_PER_MS;
@@ -473,7 +470,7 @@ static void on_resolver_result_changed_locked(grpc_exec_ctx *exec_ctx,
         retry_throttle_data = parsing_state.retry_throttle_data;
         method_params_table = grpc_service_config_create_method_config_table(
             exec_ctx, service_config, method_parameters_create_from_json,
-            method_parameters_free);
+            method_parameters_ref_wrapper, method_parameters_unref_wrapper);
         grpc_service_config_destroy(service_config);
       }
     }

src/core/ext/filters/message_size/message_size_filter.cc

@@ -30,16 +30,34 @@
 #include "src/core/lib/surface/channel_init.h"
 #include "src/core/lib/transport/service_config.h"
 
-typedef struct message_size_limits {
+typedef struct {
   int max_send_size;
   int max_recv_size;
 } message_size_limits;
 
-static void message_size_limits_free(grpc_exec_ctx* exec_ctx, void* value) {
-  gpr_free(value);
+typedef struct {
+  gpr_refcount refs;
+  message_size_limits limits;
+} refcounted_message_size_limits;
+
+static void* refcounted_message_size_limits_ref(void* value) {
+  refcounted_message_size_limits* limits =
+      (refcounted_message_size_limits*)value;
+  gpr_ref(&limits->refs);
+  return value;
+}
+
+static void refcounted_message_size_limits_unref(grpc_exec_ctx* exec_ctx,
+                                                 void* value) {
+  refcounted_message_size_limits* limits =
+      (refcounted_message_size_limits*)value;
+  if (gpr_unref(&limits->refs)) {
+    gpr_free(value);
+  }
 }
 
-static void* message_size_limits_create_from_json(const grpc_json* json) {
+static void* refcounted_message_size_limits_create_from_json(
+    const grpc_json* json) {
   int max_request_message_bytes = -1;
   int max_response_message_bytes = -1;
   for (grpc_json* field = json->child; field != NULL; field = field->next) {
@@ -60,10 +78,12 @@ static void* message_size_limits_create_from_json(const grpc_json* json) {
       if (max_response_message_bytes == -1) return NULL;
     }
   }
-  message_size_limits* value =
-      (message_size_limits*)gpr_malloc(sizeof(message_size_limits));
-  value->max_send_size = max_request_message_bytes;
-  value->max_recv_size = max_response_message_bytes;
+  refcounted_message_size_limits* value =
+      (refcounted_message_size_limits*)gpr_malloc(
+          sizeof(refcounted_message_size_limits));
+  gpr_ref_init(&value->refs, 1);
+  value->limits.max_send_size = max_request_message_bytes;
+  value->limits.max_recv_size = max_response_message_bytes;
   return value;
 }
 
@@ -82,7 +102,7 @@ typedef struct call_data {
 
 typedef struct channel_data {
   message_size_limits limits;
-  // Maps path names to message_size_limits structs.
+  // Maps path names to refcounted_message_size_limits structs.
   grpc_slice_hash_table* method_limit_table;
 } channel_data;
 
@@ -164,19 +184,19 @@ static grpc_error* init_call_elem(grpc_exec_ctx* exec_ctx,
   // size to the receive limit.
   calld->limits = chand->limits;
   if (chand->method_limit_table != NULL) {
-    message_size_limits* limits =
-        (message_size_limits*)grpc_method_config_table_get(
+    refcounted_message_size_limits* limits =
+        (refcounted_message_size_limits*)grpc_method_config_table_get(
             exec_ctx, chand->method_limit_table, args->path);
     if (limits != NULL) {
-      if (limits->max_send_size >= 0 &&
-          (limits->max_send_size < calld->limits.max_send_size ||
+      if (limits->limits.max_send_size >= 0 &&
+          (limits->limits.max_send_size < calld->limits.max_send_size ||
            calld->limits.max_send_size < 0)) {
-        calld->limits.max_send_size = limits->max_send_size;
+        calld->limits.max_send_size = limits->limits.max_send_size;
       }
-      if (limits->max_recv_size >= 0 &&
-          (limits->max_recv_size < calld->limits.max_recv_size ||
+      if (limits->limits.max_recv_size >= 0 &&
+          (limits->limits.max_recv_size < calld->limits.max_recv_size ||
            calld->limits.max_recv_size < 0)) {
-        calld->limits.max_recv_size = limits->max_recv_size;
+        calld->limits.max_recv_size = limits->limits.max_recv_size;
       }
     }
   }
@@ -237,8 +257,10 @@ static grpc_error* init_channel_elem(grpc_exec_ctx* exec_ctx,
     if (service_config != NULL) {
       chand->method_limit_table =
           grpc_service_config_create_method_config_table(
-              exec_ctx, service_config, message_size_limits_create_from_json,
-              message_size_limits_free);
+              exec_ctx, service_config,
+              refcounted_message_size_limits_create_from_json,
+              refcounted_message_size_limits_ref,
+              refcounted_message_size_limits_unref);
       grpc_service_config_destroy(service_config);
     }
   }

src/core/lib/transport/service_config.cc

@@ -111,7 +111,13 @@ const char* grpc_service_config_get_lb_policy_name(
 static size_t count_names_in_method_config_json(grpc_json* json) {
   size_t num_names = 0;
   for (grpc_json* field = json->child; field != NULL; field = field->next) {
-    if (field->key != NULL && strcmp(field->key, "name") == 0) ++num_names;
+    if (field->key != NULL && strcmp(field->key, "name") == 0) {
+      if (field->type != GRPC_JSON_ARRAY) return -1;
+      for (grpc_json* name = field->child; name != NULL; name = name->next) {
+        if (name->type != GRPC_JSON_OBJECT) return -1;
+        ++num_names;
+      }
+    }
   }
   return num_names;
 }
@@ -148,6 +154,8 @@ static char* parse_json_method_name(grpc_json* json) {
 static bool parse_json_method_config(
     grpc_exec_ctx* exec_ctx, grpc_json* json,
     void* (*create_value)(const grpc_json* method_config_json),
+    void* (*ref_value)(void* value),
+    void (*unref_value)(grpc_exec_ctx* exec_ctx, void* value),
     grpc_slice_hash_table_entry* entries, size_t* idx) {
   // Construct value.
   void* method_config = create_value(json);
@@ -162,6 +170,7 @@ static bool parse_json_method_config(
       if (child->type != GRPC_JSON_ARRAY) goto done;
       for (grpc_json* name = child->child; name != NULL; name = name->next) {
         char* path = parse_json_method_name(name);
+        if (path == NULL) goto done;
         gpr_strvec_add(&paths, path);
       }
     }
@@ -170,11 +179,12 @@ static bool parse_json_method_config(
   // Add entry for each path.
   for (size_t i = 0; i < paths.count; ++i) {
     entries[*idx].key = grpc_slice_from_copied_string(paths.strs[i]);
-    entries[*idx].value = method_config;
+    entries[*idx].value = ref_value(method_config);
     ++*idx;
   }
   success = true;
 done:
+  unref_value(exec_ctx, method_config);
   gpr_strvec_destroy(&paths);
   return success;
 }
@@ -182,7 +192,8 @@ done:
 grpc_slice_hash_table* grpc_service_config_create_method_config_table(
     grpc_exec_ctx* exec_ctx, const grpc_service_config* service_config,
     void* (*create_value)(const grpc_json* method_config_json),
-    void (*destroy_value)(grpc_exec_ctx* exec_ctx, void* value)) {
+    void* (*ref_value)(void* value),
+    void (*unref_value)(grpc_exec_ctx* exec_ctx, void* value)) {
   const grpc_json* json = service_config->json_tree;
   // Traverse parsed JSON tree.
   if (json->type != GRPC_JSON_OBJECT || json->key != NULL) return NULL;
@@ -196,7 +207,9 @@ grpc_slice_hash_table* grpc_service_config_create_method_config_table(
       // Find number of entries.
       for (grpc_json* method = field->child; method != NULL;
            method = method->next) {
-        num_entries += count_names_in_method_config_json(method);
+        size_t count = count_names_in_method_config_json(method);
+        if (count <= 0) return NULL;
+        num_entries += count;
       }
       // Populate method config table entries.
       entries = (grpc_slice_hash_table_entry*)gpr_malloc(
@@ -204,8 +217,13 @@ grpc_slice_hash_table* grpc_service_config_create_method_config_table(
       size_t idx = 0;
       for (grpc_json* method = field->child; method != NULL;
            method = method->next) {
-        if (!parse_json_method_config(exec_ctx, method, create_value, entries,
-                                      &idx)) {
+        if (!parse_json_method_config(exec_ctx, method, create_value, ref_value,
+                                      unref_value, entries, &idx)) {
+          for (size_t i = 0; i < idx; ++i) {
+            grpc_slice_unref_internal(exec_ctx, entries[i].key);
+            unref_value(exec_ctx, entries[i].value);
+          }
+          gpr_free(entries);
           return NULL;
         }
       }
@@ -216,7 +234,7 @@ grpc_slice_hash_table* grpc_service_config_create_method_config_table(
   grpc_slice_hash_table* method_config_table = NULL;
   if (entries != NULL) {
     method_config_table =
-        grpc_slice_hash_table_create(num_entries, entries, destroy_value, NULL);
+        grpc_slice_hash_table_create(num_entries, entries, unref_value, NULL);
     gpr_free(entries);
   }
   return method_config_table;

src/core/lib/transport/service_config.h

@@ -46,12 +46,13 @@ const char* grpc_service_config_get_lb_policy_name(
 /// Creates a method config table based on the data in \a json.
 /// The table's keys are request paths.  The table's value type is
 /// returned by \a create_value(), based on data parsed from the JSON tree.
-/// \a destroy_value is used to clean up values.
+/// \a ref_value() and \a unref_value() are used to ref and unref values.
 /// Returns NULL on error.
 grpc_slice_hash_table* grpc_service_config_create_method_config_table(
     grpc_exec_ctx* exec_ctx, const grpc_service_config* service_config,
     void* (*create_value)(const grpc_json* method_config_json),
-    void (*destroy_value)(grpc_exec_ctx* exec_ctx, void* value));
+    void* (*ref_value)(void* value),
+    void (*unref_value)(grpc_exec_ctx* exec_ctx, void* value));
 
 /// A helper function for looking up values in the table returned by
 /// \a grpc_service_config_create_method_config_table().

test/core/end2end/tests/cancel_after_accept.c

@@ -130,7 +130,8 @@ static void test_cancel_after_accept(grpc_end2end_test_config config,
         "{\n"
         "  \"methodConfig\": [ {\n"
         "    \"name\": [\n"
-        "      { \"service\": \"service\", \"method\": \"method\" }\n"
+        "      { \"service\": \"service\", \"method\": \"method\" },\n"
+        "      { \"service\": \"unused\" }\n"
         "    ],\n"
         "    \"timeout\": \"5s\"\n"
         "  } ]\n"

test/core/end2end/tests/max_message_length.c

@@ -138,7 +138,8 @@ static void test_max_message_length_on_request(grpc_end2end_test_config config,
             ? "{\n"
               "  \"methodConfig\": [ {\n"
               "    \"name\": [\n"
-              "      { \"service\": \"service\", \"method\": \"method\" }\n"
+              "      { \"service\": \"service\", \"method\": \"method\" },\n"
+              "      { \"service\": \"unused\" }\n"
               "    ],\n"
               "    \"maxRequestMessageBytes\": \"5\"\n"
               "  } ]\n"