From 04f28d97b54f7a04f10c3b40d034c13c4d56c396 Mon Sep 17 00:00:00 2001 From: Makarand Dharmapurikar Date: Fri, 24 Mar 2017 11:32:51 -0700 Subject: [PATCH] review feedback and one bugfix added a needed unref_internal changed k_query_separator to a char from string review feedback addressed. added todo for changing flags to bool from int --- src/core/lib/channel/http_client_filter.c | 13 +++++++------ src/core/lib/channel/http_server_filter.c | 5 +++-- src/core/lib/security/util/b64.c | 2 +- src/core/lib/security/util/b64.h | 3 ++- 4 files changed, 13 insertions(+), 10 deletions(-) diff --git a/src/core/lib/channel/http_client_filter.c b/src/core/lib/channel/http_client_filter.c index 51f2d84834b..967904df1ee 100644 --- a/src/core/lib/channel/http_client_filter.c +++ b/src/core/lib/channel/http_client_filter.c @@ -296,28 +296,28 @@ static grpc_error *hc_mutate_op(grpc_exec_ctx *exec_ctx, * MDELEM by appending base64 encoded query to the path */ const int k_url_safe = 1; const int k_multi_line = 0; - const char *k_query_separator = "?"; - const size_t k_query_separator_len = 1; /* strlen(k_query_separator) */ + const unsigned char k_query_separator = '?'; grpc_slice path_slice = GRPC_MDVALUE(op->send_initial_metadata->idx.named.path->md); /* sum up individual component's lengths and allocate enough memory to * hold combined path+query */ size_t estimated_len = GRPC_SLICE_LENGTH(path_slice); - estimated_len += k_query_separator_len; + estimated_len++; /* for the '?' */ estimated_len += grpc_base64_estimate_encoded_size( op->send_message->length, k_url_safe, k_multi_line); estimated_len += 1; /* for the trailing 0 */ grpc_slice path_with_query_slice = grpc_slice_malloc(estimated_len); /* memcopy individual pieces into this slice */ - uint8_t *write_ptr = (uint8_t *)GRPC_SLICE_START_PTR(path_with_query_slice); + uint8_t *write_ptr = + (uint8_t *)GRPC_SLICE_START_PTR(path_with_query_slice); uint8_t *original_path = (uint8_t *)GRPC_SLICE_START_PTR(path_slice); memcpy(write_ptr, original_path, GRPC_SLICE_LENGTH(path_slice)); write_ptr += GRPC_SLICE_LENGTH(path_slice); - memcpy(write_ptr, k_query_separator, k_query_separator_len); - write_ptr += k_query_separator_len; + *write_ptr = k_query_separator; + write_ptr++; /* for the '?' */ grpc_base64_encode_core((char *)write_ptr, calld->payload_bytes, op->send_message->length, k_url_safe, @@ -326,6 +326,7 @@ static grpc_error *hc_mutate_op(grpc_exec_ctx *exec_ctx, /* remove trailing unused memory and add trailing 0 to terminate string */ char *t = (char *)GRPC_SLICE_START_PTR(path_with_query_slice); + /* safe to use strlen since base64_encode will always add '\0' */ size_t path_length = strlen(t) + 1; *(t + path_length) = '\0'; path_with_query_slice = diff --git a/src/core/lib/channel/http_server_filter.c b/src/core/lib/channel/http_server_filter.c index dbe41d3fd94..8d3c488ea0a 100644 --- a/src/core/lib/channel/http_server_filter.c +++ b/src/core/lib/channel/http_server_filter.c @@ -209,13 +209,13 @@ static grpc_error *server_filter_incoming_metadata(grpc_exec_ctx *exec_ctx, } else if (*calld->recv_cacheable_request == true) { /* We have a cacheable request made with GET verb. The path contains the * query parameter which is base64 encoded request payload. */ - const char *k_query_separator = "?"; + const char k_query_separator = '?'; grpc_slice path_slice = GRPC_MDVALUE(b->idx.named.path->md); uint8_t *path_ptr = (uint8_t *)GRPC_SLICE_START_PTR(path_slice); size_t path_length = GRPC_SLICE_LENGTH(path_slice); /* offset of the character '?' */ size_t offset = 0; - for (offset = 0; *path_ptr != k_query_separator[0] && offset < path_length; + for (offset = 0; *path_ptr != k_query_separator && offset < path_length; path_ptr++, offset++) ; if (offset < path_length) { @@ -239,6 +239,7 @@ static grpc_error *server_filter_incoming_metadata(grpc_exec_ctx *exec_ctx, grpc_slice_buffer_stream_init(&calld->read_stream, &calld->read_slice_buffer, 0); calld->seen_path_with_query = true; + grpc_slice_unref_internal(exec_ctx, query_slice); } else { gpr_log(GPR_ERROR, "GET request without QUERY"); } diff --git a/src/core/lib/security/util/b64.c b/src/core/lib/security/util/b64.c index a84e42b4f6e..0d5a9176602 100644 --- a/src/core/lib/security/util/b64.c +++ b/src/core/lib/security/util/b64.c @@ -93,7 +93,7 @@ void grpc_base64_encode_core(char *result, const void *vdata, size_t data_size, const unsigned char *data = vdata; const char *base64_chars = url_safe ? base64_url_safe_chars : base64_url_unsafe_chars; - size_t result_projected_size = + const size_t result_projected_size = grpc_base64_estimate_encoded_size(data_size, url_safe, multiline); char *current = result; diff --git a/src/core/lib/security/util/b64.h b/src/core/lib/security/util/b64.h index e84c50271e3..ef52291c6a4 100644 --- a/src/core/lib/security/util/b64.h +++ b/src/core/lib/security/util/b64.h @@ -37,7 +37,8 @@ #include /* Encodes data using base64. It is the caller's responsability to free - the returned char * using gpr_free. Returns NULL on NULL input. */ + the returned char * using gpr_free. Returns NULL on NULL input. + TODO(makdharma) : change the flags to bool from int */ char *grpc_base64_encode(const void *data, size_t data_size, int url_safe, int multiline);