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

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 51f2d84834..967904df1e 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 dbe41d3fd9..8d3c488ea0 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 a84e42b4f6..0d5a917660 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 e84c50271e..ef52291c6a 100644
--- a/src/core/lib/security/util/b64.h
+++ b/src/core/lib/security/util/b64.h
@@ -37,7 +37,8 @@
 #include <grpc/slice.h>
 
 /* 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);