From ac2e88f9567c7e90f615738fb759537385dcc858 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 27 Apr 2016 14:13:27 -0700 Subject: [PATCH] Initial attempt at adding support for max metadata size. Currently, the code does not seem to be properly causing the RPC to fail when the max size is exceeded. --- include/grpc/impl/codegen/grpc_types.h | 2 ++ .../chttp2/transport/chttp2_transport.c | 15 ++++++++++ .../chttp2/transport/incoming_metadata.c | 1 + .../chttp2/transport/incoming_metadata.h | 1 + .../ext/transport/chttp2/transport/internal.h | 3 ++ .../ext/transport/chttp2/transport/parsing.c | 22 +++++++++++--- src/core/lib/transport/metadata.h | 2 ++ test/core/end2end/cq_verifier.c | 8 +++++ test/core/end2end/cq_verifier.h | 1 + test/core/end2end/tests/large_metadata.c | 29 ++++++++++++++----- 10 files changed, 73 insertions(+), 11 deletions(-) diff --git a/include/grpc/impl/codegen/grpc_types.h b/include/grpc/impl/codegen/grpc_types.h index 4c7373006b2..b5203b6e4d2 100644 --- a/include/grpc/impl/codegen/grpc_types.h +++ b/include/grpc/impl/codegen/grpc_types.h @@ -152,6 +152,8 @@ typedef struct { channel). If this parameter is specified and the underlying is not an SSL channel, it will just be ignored. */ #define GRPC_SSL_TARGET_NAME_OVERRIDE_ARG "grpc.ssl_target_name_override" +/* Maximum metadata size */ +#define GRPC_ARG_MAX_METADATA_SIZE "grpc.max_metadata_size" /** Result of a grpc call. If the caller satisfies the prerequisites of a particular operation, the grpc_call_error returned will be GRPC_CALL_OK. diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.c b/src/core/ext/transport/chttp2/transport/chttp2_transport.c index 01507f5ca6f..c24950a1890 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.c +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.c @@ -56,6 +56,8 @@ #define DEFAULT_CONNECTION_WINDOW_TARGET (1024 * 1024) #define MAX_WINDOW 0x7fffffffu +#define DEFAULT_MAX_METADATA_SIZE 16 * 1024 + #define MAX_CLIENT_STREAM_ID 0x7fffffffu int grpc_http_trace = 0; @@ -250,6 +252,7 @@ static void init_transport(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, t->global.ping_counter = 1; t->global.pings.next = t->global.pings.prev = &t->global.pings; t->parsing.is_client = is_client; + t->parsing.max_metadata_size = DEFAULT_MAX_METADATA_SIZE; t->parsing.deframe_state = is_client ? GRPC_DTS_FH_0 : GRPC_DTS_CLIENT_PREFIX_0; t->writing.is_client = is_client; @@ -372,6 +375,18 @@ static void init_transport(grpc_exec_ctx *exec_ctx, grpc_chttp2_transport *t, &t->writing.hpack_compressor, (uint32_t)channel_args->args[i].value.integer); } + } else if (0 == strcmp(channel_args->args[i].key, + GRPC_ARG_MAX_METADATA_SIZE)) { + if (channel_args->args[i].type != GRPC_ARG_INTEGER) { + gpr_log(GPR_ERROR, "%s: must be an integer", + GRPC_ARG_MAX_METADATA_SIZE); + } else if (channel_args->args[i].value.integer < 0) { + gpr_log(GPR_ERROR, "%s: must be non-negative", + GRPC_ARG_MAX_METADATA_SIZE); + } else { + t->parsing.max_metadata_size = + (uint32_t)channel_args->args[i].value.integer; + } } } } diff --git a/src/core/ext/transport/chttp2/transport/incoming_metadata.c b/src/core/ext/transport/chttp2/transport/incoming_metadata.c index db21744f0c0..3e463a79957 100644 --- a/src/core/ext/transport/chttp2/transport/incoming_metadata.c +++ b/src/core/ext/transport/chttp2/transport/incoming_metadata.c @@ -65,6 +65,7 @@ void grpc_chttp2_incoming_metadata_buffer_add( gpr_realloc(buffer->elems, sizeof(*buffer->elems) * buffer->capacity); } buffer->elems[buffer->count++].md = elem; + buffer->size += GRPC_MDELEM_LENGTH(elem); } void grpc_chttp2_incoming_metadata_buffer_set_deadline( diff --git a/src/core/ext/transport/chttp2/transport/incoming_metadata.h b/src/core/ext/transport/chttp2/transport/incoming_metadata.h index 17ecf8e1811..7db5db8de05 100644 --- a/src/core/ext/transport/chttp2/transport/incoming_metadata.h +++ b/src/core/ext/transport/chttp2/transport/incoming_metadata.h @@ -42,6 +42,7 @@ typedef struct { size_t capacity; gpr_timespec deadline; int published; + size_t size; /* total size of metadata */ } grpc_chttp2_incoming_metadata_buffer; /** assumes everything initially zeroed */ diff --git a/src/core/ext/transport/chttp2/transport/internal.h b/src/core/ext/transport/chttp2/transport/internal.h index 98cd38abd43..d547a6e9c11 100644 --- a/src/core/ext/transport/chttp2/transport/internal.h +++ b/src/core/ext/transport/chttp2/transport/internal.h @@ -229,6 +229,9 @@ struct grpc_chttp2_transport_parsing { /** is this transport a client? (boolean) */ uint8_t is_client; + /** max metadata size */ + uint32_t max_metadata_size; + /** were settings updated? */ uint8_t settings_updated; /** was a settings ack received? */ diff --git a/src/core/ext/transport/chttp2/transport/parsing.c b/src/core/ext/transport/chttp2/transport/parsing.c index e827a43f7a4..0cf4d87f3c0 100644 --- a/src/core/ext/transport/chttp2/transport/parsing.c +++ b/src/core/ext/transport/chttp2/transport/parsing.c @@ -624,8 +624,15 @@ static void on_initial_header(void *tp, grpc_mdelem *md) { gpr_time_add(gpr_now(GPR_CLOCK_MONOTONIC), *cached_timeout)); GRPC_MDELEM_UNREF(md); } else { - grpc_chttp2_incoming_metadata_buffer_add( - &stream_parsing->metadata_buffer[0], md); + const size_t new_size = stream_parsing->metadata_buffer[0].size + + GRPC_MDELEM_LENGTH(md); + if (new_size > transport_parsing->max_metadata_size) { + stream_parsing->seen_error = 1; + GRPC_MDELEM_UNREF(md); + } else { + grpc_chttp2_incoming_metadata_buffer_add( + &stream_parsing->metadata_buffer[0], md); + } } grpc_chttp2_list_add_parsing_seen_stream(transport_parsing, stream_parsing); @@ -652,8 +659,15 @@ static void on_trailing_header(void *tp, grpc_mdelem *md) { stream_parsing->seen_error = 1; } - grpc_chttp2_incoming_metadata_buffer_add(&stream_parsing->metadata_buffer[1], - md); + const size_t new_size = stream_parsing->metadata_buffer[1].size + + GRPC_MDELEM_LENGTH(md); + if (new_size > transport_parsing->max_metadata_size) { + stream_parsing->seen_error = 1; + GRPC_MDELEM_UNREF(md); + } else { + grpc_chttp2_incoming_metadata_buffer_add( + &stream_parsing->metadata_buffer[1], md); + } grpc_chttp2_list_add_parsing_seen_stream(transport_parsing, stream_parsing); diff --git a/src/core/lib/transport/metadata.h b/src/core/lib/transport/metadata.h index 713d9e6782c..277c2579338 100644 --- a/src/core/lib/transport/metadata.h +++ b/src/core/lib/transport/metadata.h @@ -146,6 +146,8 @@ void grpc_mdelem_unref(grpc_mdelem *md); const char *grpc_mdstr_as_c_string(grpc_mdstr *s); #define GRPC_MDSTR_LENGTH(s) (GPR_SLICE_LENGTH(s->slice)) +#define GRPC_MDELEM_LENGTH(e) (GRPC_MDSTR_LENGTH(e->key) + \ + GRPC_MDSTR_LENGTH(e->value)) int grpc_mdstr_is_legal_header(grpc_mdstr *s); int grpc_mdstr_is_legal_nonbin_header(grpc_mdstr *s); diff --git a/test/core/end2end/cq_verifier.c b/test/core/end2end/cq_verifier.c index 77afe588d79..5f1a33242ee 100644 --- a/test/core/end2end/cq_verifier.c +++ b/test/core/end2end/cq_verifier.c @@ -107,6 +107,14 @@ int contains_metadata(grpc_metadata_array *array, const char *key, return has_metadata(array->metadata, array->count, key, value); } +int contains_metadata_key(grpc_metadata_array *array, const char *key) { + for (size_t i = 0; i < array->count; ++i) { + if (strcmp(array->metadata[i].key, key) == 0) + return 1; + } + return 0; +} + static gpr_slice merge_slices(gpr_slice *slices, size_t nslices) { size_t i; size_t len = 0; diff --git a/test/core/end2end/cq_verifier.h b/test/core/end2end/cq_verifier.h index b3e07c45a58..a54065950d2 100644 --- a/test/core/end2end/cq_verifier.h +++ b/test/core/end2end/cq_verifier.h @@ -62,5 +62,6 @@ void cq_expect_completion(cq_verifier *v, void *tag, int success); int byte_buffer_eq_string(grpc_byte_buffer *byte_buffer, const char *string); int contains_metadata(grpc_metadata_array *array, const char *key, const char *value); +int contains_metadata_key(grpc_metadata_array *array, const char *key); #endif /* GRPC_TEST_CORE_END2END_CQ_VERIFIER_H */ diff --git a/test/core/end2end/tests/large_metadata.c b/test/core/end2end/tests/large_metadata.c index 0e5d6b4fe01..f09b55a2c0c 100644 --- a/test/core/end2end/tests/large_metadata.c +++ b/test/core/end2end/tests/large_metadata.c @@ -98,7 +98,8 @@ static void end_test(grpc_end2end_test_fixture *f) { } /* Request with a large amount of metadata.*/ -static void test_request_with_large_metadata(grpc_end2end_test_config config) { +static void test_request_with_large_metadata(grpc_end2end_test_config config, + int allow_large_metadata) { grpc_call *c; grpc_call *s; gpr_slice request_payload_slice = gpr_slice_from_copied_string("hello world"); @@ -106,8 +107,16 @@ static void test_request_with_large_metadata(grpc_end2end_test_config config) { grpc_raw_byte_buffer_create(&request_payload_slice, 1); gpr_timespec deadline = five_seconds_time(); grpc_metadata meta; - grpc_end2end_test_fixture f = - begin_test(config, "test_request_with_large_metadata", NULL, NULL); + const char *test_name = allow_large_metadata + ? "test_request_with_large_metadata_allowed" + : "test_request_with_large_metadata_not_allowed"; + const size_t large_size = 64 * 1024; + grpc_arg arg = { GRPC_ARG_INTEGER, GRPC_ARG_MAX_METADATA_SIZE, + { .integer=(int)large_size + 1024 } }; + grpc_channel_args args = { 1, &arg }; + grpc_channel_args* use_args = allow_large_metadata ? &args : NULL; + grpc_end2end_test_fixture f = begin_test( + config, test_name, use_args, use_args); cq_verifier *cqv = cq_verifier_create(f.cq); grpc_op ops[6]; grpc_op *op; @@ -121,7 +130,6 @@ static void test_request_with_large_metadata(grpc_end2end_test_config config) { char *details = NULL; size_t details_capacity = 0; int was_cancelled = 2; - const size_t large_size = 64 * 1024; c = grpc_channel_create_call(f.client, NULL, GRPC_PROPAGATE_DEFAULTS, f.cq, "/foo", "foo.test.google.fr", deadline, NULL); @@ -214,13 +222,19 @@ static void test_request_with_large_metadata(grpc_end2end_test_config config) { cq_expect_completion(cqv, tag(1), 1); cq_verify(cqv); +// FIXME: why is this assert passing with allow_large_metadata=false? GPR_ASSERT(status == GRPC_STATUS_OK); GPR_ASSERT(0 == strcmp(details, "xyz")); GPR_ASSERT(0 == strcmp(call_details.method, "/foo")); GPR_ASSERT(0 == strcmp(call_details.host, "foo.test.google.fr")); GPR_ASSERT(was_cancelled == 0); - GPR_ASSERT(byte_buffer_eq_string(request_payload_recv, "hello world")); - GPR_ASSERT(contains_metadata(&request_metadata_recv, "key", meta.value)); + if (allow_large_metadata) { + GPR_ASSERT(byte_buffer_eq_string(request_payload_recv, "hello world")); + GPR_ASSERT(contains_metadata(&request_metadata_recv, "key", meta.value)); + } else { + GPR_ASSERT(request_payload_recv == NULL); + GPR_ASSERT(!contains_metadata_key(&request_metadata_recv, "key")); + } gpr_free(details); grpc_metadata_array_destroy(&initial_metadata_recv); @@ -243,7 +257,8 @@ static void test_request_with_large_metadata(grpc_end2end_test_config config) { } void large_metadata(grpc_end2end_test_config config) { - test_request_with_large_metadata(config); + test_request_with_large_metadata(config, 1); + test_request_with_large_metadata(config, 0); } void large_metadata_pre_init(void) {}