From b121fc7c3f4e766da11b80b939d5d161a831ba84 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Thu, 3 Nov 2016 15:22:59 -0700 Subject: [PATCH 1/4] Support percent-encoding status messages --- include/grpc/support/slice.h | 4 ++ src/core/lib/channel/http_client_filter.c | 57 +++++++++++++++++++---- src/core/lib/channel/http_server_filter.c | 28 ++++++++++- src/core/lib/support/slice.c | 8 ++++ src/core/lib/surface/call.c | 5 +- src/core/lib/transport/metadata_batch.c | 1 + test/core/end2end/tests/binary_metadata.c | 37 ++++++++++++++- 7 files changed, 124 insertions(+), 16 deletions(-) diff --git a/include/grpc/support/slice.h b/include/grpc/support/slice.h index b31fe6c0c56..ce7737e529f 100644 --- a/include/grpc/support/slice.h +++ b/include/grpc/support/slice.h @@ -121,6 +121,10 @@ GPRAPI gpr_slice gpr_empty_slice(void); GPRAPI int gpr_slice_cmp(gpr_slice a, gpr_slice b); GPRAPI int gpr_slice_str_cmp(gpr_slice a, const char *b); +/* Do two slices point at the same memory, with the same length + If a or b is inlined, actually compares data */ +GPRAPI int gpr_slice_is_equivalent(gpr_slice a, gpr_slice b); + #ifdef __cplusplus } #endif diff --git a/src/core/lib/channel/http_client_filter.c b/src/core/lib/channel/http_client_filter.c index 1dc05fb20d1..0875cc18c41 100644 --- a/src/core/lib/channel/http_client_filter.c +++ b/src/core/lib/channel/http_client_filter.c @@ -36,6 +36,7 @@ #include #include #include "src/core/lib/profiling/timers.h" +#include "src/core/lib/support/percent_encoding.h" #include "src/core/lib/support/string.h" #include "src/core/lib/transport/static_metadata.h" #include "src/core/lib/transport/transport_impl.h" @@ -56,6 +57,7 @@ typedef struct call_data { grpc_linked_mdelem payload_bin; grpc_metadata_batch *recv_initial_metadata; + grpc_metadata_batch *recv_trailing_metadata; uint8_t *payload_bytes; /* Vars to read data off of send_message */ @@ -69,14 +71,16 @@ typedef struct call_data { bool send_message_blocked; /** Closure to call when finished with the hc_on_recv hook */ - grpc_closure *on_done_recv; + grpc_closure *on_done_recv_initial_metadata; + grpc_closure *on_done_recv_trailing_metadata; grpc_closure *on_complete; grpc_closure *post_send; /** Receive closures are chained: we inject this closure as the on_done_recv up-call on transport_op, and remember to call our on_done_recv member after handling it. */ - grpc_closure hc_on_recv; + grpc_closure hc_on_recv_initial_metadata; + grpc_closure hc_on_recv_trailing_metadata; grpc_closure hc_on_complete; grpc_closure got_slice; grpc_closure send_done; @@ -106,6 +110,16 @@ static grpc_mdelem *client_recv_filter(void *user_data, grpc_mdelem *md) { grpc_call_element_send_close_with_message(a->exec_ctx, a->elem, GRPC_STATUS_CANCELLED, &message); return NULL; + } else if (md->key == GRPC_MDSTR_GRPC_MESSAGE) { + gpr_slice pct_decoded_msg = + gpr_permissive_percent_decode_slice(md->value->slice); + if (gpr_slice_is_equivalent(pct_decoded_msg, md->value->slice)) { + gpr_slice_unref(pct_decoded_msg); + return md; + } else { + return grpc_mdelem_from_metadata_strings( + GRPC_MDSTR_GRPC_MESSAGE, grpc_mdstr_from_slice(pct_decoded_msg)); + } } else if (md == GRPC_MDELEM_CONTENT_TYPE_APPLICATION_SLASH_GRPC) { return NULL; } else if (md->key == GRPC_MDSTR_CONTENT_TYPE) { @@ -129,8 +143,8 @@ static grpc_mdelem *client_recv_filter(void *user_data, grpc_mdelem *md) { return md; } -static void hc_on_recv(grpc_exec_ctx *exec_ctx, void *user_data, - grpc_error *error) { +static void hc_on_recv_initial_metadata(grpc_exec_ctx *exec_ctx, + void *user_data, grpc_error *error) { grpc_call_element *elem = user_data; call_data *calld = elem->call_data; client_recv_filter_args a; @@ -138,7 +152,21 @@ static void hc_on_recv(grpc_exec_ctx *exec_ctx, void *user_data, a.exec_ctx = exec_ctx; grpc_metadata_batch_filter(calld->recv_initial_metadata, client_recv_filter, &a); - calld->on_done_recv->cb(exec_ctx, calld->on_done_recv->cb_arg, error); + grpc_closure_run(exec_ctx, calld->on_done_recv_initial_metadata, + GRPC_ERROR_REF(error)); +} + +static void hc_on_recv_trailing_metadata(grpc_exec_ctx *exec_ctx, + void *user_data, grpc_error *error) { + grpc_call_element *elem = user_data; + call_data *calld = elem->call_data; + client_recv_filter_args a; + a.elem = elem; + a.exec_ctx = exec_ctx; + grpc_metadata_batch_filter(calld->recv_trailing_metadata, client_recv_filter, + &a); + grpc_closure_run(exec_ctx, calld->on_done_recv_trailing_metadata, + GRPC_ERROR_REF(error)); } static void hc_on_complete(grpc_exec_ctx *exec_ctx, void *user_data, @@ -281,8 +309,15 @@ static void hc_mutate_op(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, if (op->recv_initial_metadata != NULL) { /* substitute our callback for the higher callback */ calld->recv_initial_metadata = op->recv_initial_metadata; - calld->on_done_recv = op->recv_initial_metadata_ready; - op->recv_initial_metadata_ready = &calld->hc_on_recv; + calld->on_done_recv_initial_metadata = op->recv_initial_metadata_ready; + op->recv_initial_metadata_ready = &calld->hc_on_recv_initial_metadata; + } + + if (op->recv_trailing_metadata != NULL) { + /* substitute our callback for the higher callback */ + calld->recv_trailing_metadata = op->recv_trailing_metadata; + calld->on_done_recv_trailing_metadata = op->on_complete; + op->on_complete = &calld->hc_on_recv_trailing_metadata; } } @@ -308,11 +343,15 @@ static grpc_error *init_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, grpc_call_element_args *args) { call_data *calld = elem->call_data; - calld->on_done_recv = NULL; + calld->on_done_recv_initial_metadata = NULL; + calld->on_done_recv_trailing_metadata = NULL; calld->on_complete = NULL; calld->payload_bytes = NULL; gpr_slice_buffer_init(&calld->slices); - grpc_closure_init(&calld->hc_on_recv, hc_on_recv, elem); + grpc_closure_init(&calld->hc_on_recv_initial_metadata, + hc_on_recv_initial_metadata, elem); + grpc_closure_init(&calld->hc_on_recv_trailing_metadata, + hc_on_recv_trailing_metadata, elem); grpc_closure_init(&calld->hc_on_complete, hc_on_complete, elem); grpc_closure_init(&calld->got_slice, got_slice, elem); grpc_closure_init(&calld->send_done, send_done, elem); diff --git a/src/core/lib/channel/http_server_filter.c b/src/core/lib/channel/http_server_filter.c index f2221fb0fbe..eb335b6b5ce 100644 --- a/src/core/lib/channel/http_server_filter.c +++ b/src/core/lib/channel/http_server_filter.c @@ -37,6 +37,7 @@ #include #include #include "src/core/lib/profiling/timers.h" +#include "src/core/lib/support/percent_encoding.h" #include "src/core/lib/transport/static_metadata.h" #define EXPECTED_CONTENT_TYPE "application/grpc" @@ -86,6 +87,23 @@ typedef struct { grpc_exec_ctx *exec_ctx; } server_filter_args; +static grpc_mdelem *server_filter_outgoing_metadata(void *user_data, + grpc_mdelem *md) { + if (md->key == GRPC_MDSTR_GRPC_MESSAGE) { + gpr_slice pct_encoded_msg = gpr_percent_encode_slice( + md->value->slice, gpr_compatible_percent_encoding_unreserved_bytes); + if (gpr_slice_is_equivalent(pct_encoded_msg, md->value->slice)) { + gpr_slice_unref(pct_encoded_msg); + return md; + } else { + return grpc_mdelem_from_metadata_strings( + GRPC_MDSTR_GRPC_MESSAGE, grpc_mdstr_from_slice(pct_encoded_msg)); + } + } else { + return md; + } +} + static grpc_mdelem *server_filter(void *user_data, grpc_mdelem *md) { server_filter_args *a = user_data; grpc_call_element *elem = a->elem; @@ -255,7 +273,7 @@ static void hs_recv_message_ready(grpc_exec_ctx *exec_ctx, void *user_data, } } -static void hs_mutate_op(grpc_call_element *elem, +static void hs_mutate_op(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, grpc_transport_stream_op *op) { /* grab pointers to our data from the call element */ call_data *calld = elem->call_data; @@ -291,6 +309,12 @@ static void hs_mutate_op(grpc_call_element *elem, op->on_complete = &calld->hs_on_complete; } } + + if (op->send_trailing_metadata) { + server_filter_args a = {elem, exec_ctx}; + grpc_metadata_batch_filter(op->send_trailing_metadata, + server_filter_outgoing_metadata, &a); + } } static void hs_start_transport_op(grpc_exec_ctx *exec_ctx, @@ -298,7 +322,7 @@ static void hs_start_transport_op(grpc_exec_ctx *exec_ctx, grpc_transport_stream_op *op) { GRPC_CALL_LOG_OP(GPR_INFO, elem, op); GPR_TIMER_BEGIN("hs_start_transport_op", 0); - hs_mutate_op(elem, op); + hs_mutate_op(exec_ctx, elem, op); grpc_call_next_op(exec_ctx, elem, op); GPR_TIMER_END("hs_start_transport_op", 0); } diff --git a/src/core/lib/support/slice.c b/src/core/lib/support/slice.c index 8a2c0a90865..f15a9a1e21a 100644 --- a/src/core/lib/support/slice.c +++ b/src/core/lib/support/slice.c @@ -348,3 +348,11 @@ int gpr_slice_str_cmp(gpr_slice a, const char *b) { if (d != 0) return d; return memcmp(GPR_SLICE_START_PTR(a), b, b_length); } + +int gpr_slice_is_equivalent(gpr_slice a, gpr_slice b) { + if (a.refcount == NULL || b.refcount == NULL) { + return gpr_slice_cmp(a, b); + } + return a.data.refcounted.length == b.data.refcounted.length && + a.data.refcounted.bytes == b.data.refcounted.bytes; +} diff --git a/src/core/lib/surface/call.c b/src/core/lib/surface/call.c index 6c25952c0a9..3f4afd9a294 100644 --- a/src/core/lib/surface/call.c +++ b/src/core/lib/surface/call.c @@ -632,9 +632,6 @@ static int prepare_application_metadata(grpc_call *call, int count, if (call->send_extra_metadata_count == 0) { prepend_extra_metadata = 0; } else { - for (i = 0; i < call->send_extra_metadata_count; i++) { - GRPC_MDELEM_REF(call->send_extra_metadata[i].md); - } for (i = 1; i < call->send_extra_metadata_count; i++) { call->send_extra_metadata[i].prev = &call->send_extra_metadata[i - 1]; } @@ -680,6 +677,7 @@ static int prepare_application_metadata(grpc_call *call, int count, &call->send_extra_metadata[call->send_extra_metadata_count - 1]; batch->list.head->prev = NULL; batch->list.tail->next = NULL; + call->send_extra_metadata_count = 0; break; case 3: { /* prepend AND md */ @@ -695,6 +693,7 @@ static int prepare_application_metadata(grpc_call *call, int count, batch->list.tail = linked_from_md(last_md); batch->list.head->prev = NULL; batch->list.tail->next = NULL; + call->send_extra_metadata_count = 0; break; } default: diff --git a/src/core/lib/transport/metadata_batch.c b/src/core/lib/transport/metadata_batch.c index 84b5a74d513..f2d4595d3a6 100644 --- a/src/core/lib/transport/metadata_batch.c +++ b/src/core/lib/transport/metadata_batch.c @@ -154,6 +154,7 @@ void grpc_metadata_batch_filter(grpc_metadata_batch *batch, grpc_mdelem *orig = l->md; grpc_mdelem *filt = filter(user_data, orig); next = l->next; + gpr_log(GPR_DEBUG, "FILT: %p %p->%p", l, orig, filt); if (filt == NULL) { if (l->prev) { l->prev->next = l->next; diff --git a/test/core/end2end/tests/binary_metadata.c b/test/core/end2end/tests/binary_metadata.c index 73b0f17c241..b94bb84e545 100644 --- a/test/core/end2end/tests/binary_metadata.c +++ b/test/core/end2end/tests/binary_metadata.c @@ -234,7 +234,22 @@ static void test_request_response_with_metadata_and_payload( op->op = GRPC_OP_SEND_STATUS_FROM_SERVER; op->data.send_status_from_server.trailing_metadata_count = 0; op->data.send_status_from_server.status = GRPC_STATUS_OK; - op->data.send_status_from_server.status_details = "xyz"; + op->data.send_status_from_server.status_details = + "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f\x10\x11\x12" + "\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f\x20\x21\x22\x23\x24" + "\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f\x30\x31\x32\x33\x34\x35\x36" + "\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f\x40\x41\x42\x43\x44\x45\x46\x47\x48" + "\x49\x4a\x4b\x4c\x4d\x4e\x4f\x50\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a" + "\x5b\x5c\x5d\x5e\x5f\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c" + "\x6d\x6e\x6f\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e" + "\x7f\x80\x81\x82\x83\x84\x85\x86\x87\x88\x89\x8a\x8b\x8c\x8d\x8e\x8f\x90" + "\x91\x92\x93\x94\x95\x96\x97\x98\x99\x9a\x9b\x9c\x9d\x9e\x9f\xa0\xa1\xa2" + "\xa3\xa4\xa5\xa6\xa7\xa8\xa9\xaa\xab\xac\xad\xae\xaf\xb0\xb1\xb2\xb3\xb4" + "\xb5\xb6\xb7\xb8\xb9\xba\xbb\xbc\xbd\xbe\xbf\xc0\xc1\xc2\xc3\xc4\xc5\xc6" + "\xc7\xc8\xc9\xca\xcb\xcc\xcd\xce\xcf\xd0\xd1\xd2\xd3\xd4\xd5\xd6\xd7\xd8" + "\xd9\xda\xdb\xdc\xdd\xde\xdf\xe0\xe1\xe2\xe3\xe4\xe5\xe6\xe7\xe8\xe9\xea" + "\xeb\xec\xed\xee\xef\xf0\xf1\xf2\xf3\xf4\xf5\xf6\xf7\xf8\xf9\xfa\xfb\xfc" + "\xfd\xfe\xff"; op->flags = 0; op->reserved = NULL; op++; @@ -246,7 +261,25 @@ static void test_request_response_with_metadata_and_payload( cq_verify(cqv); GPR_ASSERT(status == GRPC_STATUS_OK); - GPR_ASSERT(0 == strcmp(details, "xyz")); + GPR_ASSERT( + 0 == + strcmp(details, + "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f\x10" + "\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f\x20" + "\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f\x30" + "\x31\x32\x33\x34\x35\x36\x37\x38\x39\x3a\x3b\x3c\x3d\x3e\x3f\x40" + "\x41\x42\x43\x44\x45\x46\x47\x48\x49\x4a\x4b\x4c\x4d\x4e\x4f\x50" + "\x51\x52\x53\x54\x55\x56\x57\x58\x59\x5a\x5b\x5c\x5d\x5e\x5f\x60" + "\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f\x70" + "\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f\x80" + "\x81\x82\x83\x84\x85\x86\x87\x88\x89\x8a\x8b\x8c\x8d\x8e\x8f\x90" + "\x91\x92\x93\x94\x95\x96\x97\x98\x99\x9a\x9b\x9c\x9d\x9e\x9f\xa0" + "\xa1\xa2\xa3\xa4\xa5\xa6\xa7\xa8\xa9\xaa\xab\xac\xad\xae\xaf\xb0" + "\xb1\xb2\xb3\xb4\xb5\xb6\xb7\xb8\xb9\xba\xbb\xbc\xbd\xbe\xbf\xc0" + "\xc1\xc2\xc3\xc4\xc5\xc6\xc7\xc8\xc9\xca\xcb\xcc\xcd\xce\xcf\xd0" + "\xd1\xd2\xd3\xd4\xd5\xd6\xd7\xd8\xd9\xda\xdb\xdc\xdd\xde\xdf\xe0" + "\xe1\xe2\xe3\xe4\xe5\xe6\xe7\xe8\xe9\xea\xeb\xec\xed\xee\xef\xf0" + "\xf1\xf2\xf3\xf4\xf5\xf6\xf7\xf8\xf9\xfa\xfb\xfc\xfd\xfe\xff")); GPR_ASSERT(0 == strcmp(call_details.method, "/foo")); GPR_ASSERT(0 == strcmp(call_details.host, "foo.test.google.fr")); GPR_ASSERT(was_cancelled == 0); From b044445174f0afd04c44b86813b49a222d6e25d0 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 4 Nov 2016 08:24:34 -0700 Subject: [PATCH 2/4] Remove spam --- src/core/lib/transport/metadata_batch.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/core/lib/transport/metadata_batch.c b/src/core/lib/transport/metadata_batch.c index f2d4595d3a6..84b5a74d513 100644 --- a/src/core/lib/transport/metadata_batch.c +++ b/src/core/lib/transport/metadata_batch.c @@ -154,7 +154,6 @@ void grpc_metadata_batch_filter(grpc_metadata_batch *batch, grpc_mdelem *orig = l->md; grpc_mdelem *filt = filter(user_data, orig); next = l->next; - gpr_log(GPR_DEBUG, "FILT: %p %p->%p", l, orig, filt); if (filt == NULL) { if (l->prev) { l->prev->next = l->next; From 268557790603150f3519b451b089b98d281d9b80 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 4 Nov 2016 10:54:20 -0700 Subject: [PATCH 3/4] Fix memory leak --- src/core/lib/support/slice.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/lib/support/slice.c b/src/core/lib/support/slice.c index f15a9a1e21a..2959a6716f0 100644 --- a/src/core/lib/support/slice.c +++ b/src/core/lib/support/slice.c @@ -351,7 +351,7 @@ int gpr_slice_str_cmp(gpr_slice a, const char *b) { int gpr_slice_is_equivalent(gpr_slice a, gpr_slice b) { if (a.refcount == NULL || b.refcount == NULL) { - return gpr_slice_cmp(a, b); + return gpr_slice_cmp(a, b) == 0; } return a.data.refcounted.length == b.data.refcounted.length && a.data.refcounted.bytes == b.data.refcounted.bytes; From 077da7fa318df97db196621740629e5b910bb325 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Wed, 16 Nov 2016 14:55:44 -0800 Subject: [PATCH 4/4] Fix merge error --- src/core/lib/channel/http_server_filter.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/core/lib/channel/http_server_filter.c b/src/core/lib/channel/http_server_filter.c index 80f3fc3cc35..b42ff06039c 100644 --- a/src/core/lib/channel/http_server_filter.c +++ b/src/core/lib/channel/http_server_filter.c @@ -37,7 +37,7 @@ #include #include #include "src/core/lib/profiling/timers.h" -#include "src/core/lib/support/percent_encoding.h" +#include "src/core/lib/slice/percent_encoding.h" #include "src/core/lib/transport/static_metadata.h" #define EXPECTED_CONTENT_TYPE "application/grpc" @@ -90,10 +90,10 @@ typedef struct { static grpc_mdelem *server_filter_outgoing_metadata(void *user_data, grpc_mdelem *md) { if (md->key == GRPC_MDSTR_GRPC_MESSAGE) { - gpr_slice pct_encoded_msg = gpr_percent_encode_slice( - md->value->slice, gpr_compatible_percent_encoding_unreserved_bytes); - if (gpr_slice_is_equivalent(pct_encoded_msg, md->value->slice)) { - gpr_slice_unref(pct_encoded_msg); + grpc_slice pct_encoded_msg = grpc_percent_encode_slice( + md->value->slice, grpc_compatible_percent_encoding_unreserved_bytes); + if (grpc_slice_is_equivalent(pct_encoded_msg, md->value->slice)) { + grpc_slice_unref(pct_encoded_msg); return md; } else { return grpc_mdelem_from_metadata_strings(