http_aws_sigv4: avoid risk of overflowed constant

- Simplify canon_query() a bit. Avoid unconditionally using length -1
  where length risks being zero at times. Pointed out by Coverity.
- Fix indent errors
- narrow some variable scopes
- fix keywords in tests

Closes #17402

diff --git a/lib/http_aws_sigv4.c b/lib/http_aws_sigv4.c
index 4d09013851b86d7a571b4cc3dda4334eb78be05e..e8ef2c459bee7acb847bb102b5f78ea8a9b27ef2 100644 (file)
--- a/lib/http_aws_sigv4.c
+++ b/lib/http_aws_sigv4.c
@@ -72,15 +72,16 @@ struct pair {
 
 static void dyn_array_free(struct dynbuf *db, size_t num_elements);
 static void pair_array_free(struct pair *pair_array, size_t num_elements);
-static CURLcode split_to_dyn_array(const char *source, char split_by,
-    struct dynbuf db[MAX_QUERY_COMPONENTS], size_t *num_splits);
+static CURLcode split_to_dyn_array(const char *source,
+                                   struct dynbuf db[MAX_QUERY_COMPONENTS],
+                                   size_t *num_splits);
 static bool is_reserved_char(const char c);
 static CURLcode uri_encode_path(struct Curl_str *original_path,
-    struct dynbuf *new_path);
+                                struct dynbuf *new_path);
 static CURLcode encode_query_component(char *component, size_t len,
-  struct dynbuf *db);
+                                       struct dynbuf *db);
 static CURLcode http_aws_decode_encode(const char *in, size_t in_len,
-  struct dynbuf *out);
+                                       struct dynbuf *out);
 static bool should_urlencode(struct Curl_str *service_name);
 
 static void sha256_to_hex(char *dst, unsigned char *sha)
@@ -577,21 +578,12 @@ UNITTEST CURLcode canon_query(const char *query, struct dynbuf *dq)
   size_t num_query_components;
   size_t counted_query_components = 0;
   size_t index;
-  size_t in_key_len;
-  size_t in_value_len;
-  size_t query_part_len;
-  const char *in_key;
-  char *in_value;
-  char *offset;
-  char *key_ptr;
-  char *value_ptr;
-  const char *query_part;
 
   if(!query)
     return result;
 
-  result = split_to_dyn_array(query, '&', &query_array[0],
-    &num_query_components);
+  result = split_to_dyn_array(query, &query_array[0],
+                              &num_query_components);
   if(result) {
     goto fail;
   }
@@ -599,11 +591,12 @@ UNITTEST CURLcode canon_query(const char *query, struct dynbuf *dq)
   /* Create list of pairs, each pair containing an encoded query
     * component */
 
-  for(index = 0; index < num_query_components;
-    index++) {
-
-    query_part_len = curlx_dyn_len(&query_array[index]);
-    query_part = curlx_dyn_ptr(&query_array[index]);
+  for(index = 0; index < num_query_components; index++) {
+    const char *in_key;
+    size_t in_key_len;
+    char *offset;
+    size_t query_part_len = curlx_dyn_len(&query_array[index]);
+    char *query_part = curlx_dyn_ptr(&query_array[index]);
 
     in_key = query_part;
 
@@ -622,17 +615,18 @@ UNITTEST CURLcode canon_query(const char *query, struct dynbuf *dq)
 
     /* Decode/encode the key */
     result = http_aws_decode_encode(in_key, in_key_len,
-      &encoded_query_array[index].key);
+                                    &encoded_query_array[index].key);
     if(result) {
       goto fail;
     }
 
     /* Decode/encode the value if it exists */
     if(offset && offset != (query_part + query_part_len - 1)) {
-      in_value = offset + 1;
+      size_t in_value_len;
+      const char *in_value = offset + 1;
       in_value_len = query_part + query_part_len - (offset + 1);
       result = http_aws_decode_encode(in_value, in_value_len,
-        &encoded_query_array[index].value);
+                                      &encoded_query_array[index].value);
       if(result) {
         goto fail;
       }
@@ -650,30 +644,33 @@ UNITTEST CURLcode canon_query(const char *query, struct dynbuf *dq)
 
   /* Sort the encoded query components by key and value */
   qsort(&encoded_query_array, num_query_components,
-    sizeof(struct pair), compare_func);
+        sizeof(struct pair), compare_func);
 
   /* Append the query components together to make a full query string */
   for(index = 0; index < num_query_components; index++) {
 
-    key_ptr = curlx_dyn_ptr(&encoded_query_array[index].key);
-    value_ptr = curlx_dyn_ptr(&encoded_query_array[index].value);
-
-    if(value_ptr && strlen(value_ptr)) {
-      result = curlx_dyn_addf(dq, "%s=%s&", key_ptr, value_ptr);
-    }
-    else {
-      /* Empty value is always encoded to key= */
-      result = curlx_dyn_addf(dq, "%s=&", key_ptr);
-    }
-    if(result) {
-      goto fail;
+    if(index)
+      result = curlx_dyn_addn(dq, "&", 1);
+    if(!result) {
+      char *key_ptr = curlx_dyn_ptr(&encoded_query_array[index].key);
+      char *value_ptr = curlx_dyn_ptr(&encoded_query_array[index].value);
+      size_t vlen = curlx_dyn_len(&encoded_query_array[index].value);
+      if(value_ptr && vlen) {
+        result = curlx_dyn_addf(dq, "%s=%s", key_ptr, value_ptr);
+      }
+      else {
+        /* Empty value is always encoded to key= */
+        result = curlx_dyn_addf(dq, "%s=", key_ptr);
+      }
     }
+    if(result)
+      break;
   }
-  /* Remove trailing & */
-  result = curlx_dyn_setlen(dq, curlx_dyn_len(dq)-1);
 
 fail:
-  pair_array_free(&encoded_query_array[0], counted_query_components);
+  if(counted_query_components)
+    /* the encoded_query_array might not be initialized yet */
+    pair_array_free(&encoded_query_array[0], counted_query_components);
   dyn_array_free(&query_array[0], num_query_components);
   return result;
 }
@@ -1010,41 +1007,38 @@ static void dyn_array_free(struct dynbuf *db, size_t num_elements)
 {
   size_t index;
 
-  for(index = 0; index < num_elements; index++) {
+  for(index = 0; index < num_elements; index++)
     curlx_dyn_free((&db[index]));
-  }
-
 }
 
 /*
-* Splits source string by split_by, and creates an array of dynbuf in db
-* db is initialized by this function
+* Splits source string by SPLIT_BY, and creates an array of dynbuf in db.
+* db is initialized by this function.
 * Caller is responsible for freeing the array elements with dyn_array_free
 */
 
-static CURLcode split_to_dyn_array(const char *source, char split_by,
-  struct dynbuf db[MAX_QUERY_COMPONENTS], size_t *num_splits_out)
-{
+#define SPLIT_BY '&'
 
+static CURLcode split_to_dyn_array(const char *source,
+                                   struct dynbuf db[MAX_QUERY_COMPONENTS],
+                                   size_t *num_splits_out)
+{
   CURLcode result = CURLE_OK;
-
   size_t len = strlen(source);
-
   size_t pos = 0;     /* Position in result buffer */
   size_t start = 0;   /* Start of current segment */
   size_t segment_length = 0;
   size_t index = 0;
-  size_t num_splits;
+  size_t num_splits = 0;
 
-  /* Split source_ptr on split_by and store the segment offsets and
-   * length in array */
-  num_splits = 0;
+  /* Split source_ptr on SPLIT_BY and store the segment offsets and length in
+   * array */
   for(pos = 0; pos < len; pos++) {
-    if(source[pos] == split_by) {
+    if(source[pos] == SPLIT_BY) {
       if(segment_length) {
         curlx_dyn_init(&db[index], segment_length + 1);
         result = curlx_dyn_addn(&db[index], &source[start],
-          segment_length);
+                                segment_length);
         if(result) {
           goto fail;
         }
@@ -1064,14 +1058,14 @@ static CURLcode split_to_dyn_array(const char *source, char split_by,
   if(segment_length) {
     curlx_dyn_init(&db[index], segment_length + 1);
     result = curlx_dyn_addn(&db[index], &source[start],
-                           segment_length);
+                            segment_length);
     if(result) {
       goto fail;
     }
     if(++num_splits == MAX_QUERY_COMPONENTS) {
       goto fail;
     }
-}
+  }
 fail:
   *num_splits_out = num_splits;
   return result;
@@ -1084,9 +1078,8 @@ static bool is_reserved_char(const char c)
 }
 
 static CURLcode uri_encode_path(struct Curl_str *original_path,
-struct dynbuf *new_path)
+                                struct dynbuf *new_path)
 {
-
   const char *p = curlx_str(original_path);
   CURLcode result = CURLE_OK;
   size_t index;
@@ -1113,9 +1106,8 @@ fail:
 
 
 static CURLcode encode_query_component(char *component, size_t len,
-  struct dynbuf *db)
+                                       struct dynbuf *db)
 {
-
   size_t index;
   CURLcode result = CURLE_OK;
   unsigned char this_char;
@@ -1149,17 +1141,16 @@ fail:
 */
 
 static CURLcode http_aws_decode_encode(const char *in, size_t in_len,
-struct dynbuf *out)
+                                       struct dynbuf *out)
 {
-  CURLcode result = CURLE_OK;
   char *out_s;
   size_t out_s_len;
+  CURLcode result =
+    Curl_urldecode(in, in_len, &out_s, &out_s_len, REJECT_NADA);
 
-  result = Curl_urldecode(in, in_len, &out_s, &out_s_len, REJECT_NADA);
-
-  if(result) {
+  if(result)
     goto fail;
-  }
+
   result = encode_query_component(out_s, out_s_len, out);
   Curl_safefree(out_s);
 fail:

diff --git a/tests/data/test1979 b/tests/data/test1979
index d89b097a07eeb30b554085aff3a9696f63a90dfb..3cff1314dbddf979f19714a00b26bd4a2b496b51 100644 (file)
--- a/tests/data/test1979
+++ b/tests/data/test1979
@@ -3,6 +3,7 @@
 <keywords>
 unittest
 canon_string
+aws-sigv4
 </keywords>
 </info>
 

diff --git a/tests/data/test1980 b/tests/data/test1980
index 6e801fe061874e126869eb2723d7d9b4c72bfcfd..4ac36377f8a65489463674f945b2dd98b98dea06 100644 (file)
--- a/tests/data/test1980
+++ b/tests/data/test1980
@@ -3,6 +3,7 @@
 <keywords>
 unittest
 canon_query
+aws-sigv4
 </keywords>
 </info>