Fixed njs_value_index().

Previous, there were several issues with njs_values_hash_test().
1) The function assumed string types of left and right values are identical.
   (If current value is a long string it assumed the second value is also
   a long string). Long vs short strings collision.
2) The function assumed if hashes are identical value length are equal.
   Long vs long string collision.

The fix is to always check value sizes.

In addition, for short strings njs_value_index() calculated hash value including
padding bytes (which values are undefined). It could result in identical
short strings (but with different padding bytes) treated as different strings.

diff --git a/src/njs_string.c b/src/njs_string.c
index 2f267a9ef90e18afae36b7b76e59bf4ab3fe0662..23d8e51d77514ce65b9b6325cea63845c3b53df1 100644 (file)
--- a/src/njs_string.c
+++ b/src/njs_string.c
@@ -4723,23 +4723,22 @@ uri_error:
 static njs_int_t
 njs_values_hash_test(njs_lvlhsh_query_t *lhq, void *data)
 {
-    u_char       *start;
+    njs_str_t    string;
     njs_value_t  *value;
 
     value = data;
 
-    if (lhq->key.length == sizeof(njs_value_t)) {
-        start = (u_char *) value;
+    if (njs_is_string(value)) {
+        njs_string_get(value, &string);
 
     } else {
-        /*
-         * Only primitive values are added into values_hash.
-         * If size != sizeof(njs_value_t) it is a long string.
-         */
-        start = value->long_string.data->start;
+        string.start = (u_char *) value;
+        string.length = sizeof(njs_value_t);
     }
 
-    if (memcmp(lhq->key.start, start, lhq->key.length) == 0) {
+    if (lhq->key.length == string.length
+        && memcmp(lhq->key.start, string.start, string.length) == 0)
+    {
         return NJS_OK;
     }
 
@@ -4768,21 +4767,28 @@ njs_value_index(njs_vm_t *vm, const njs_value_t *src, njs_uint_t runtime)
     u_char              *start;
     uint32_t            value_size, size, length;
     njs_int_t           ret;
+    njs_str_t           str;
     njs_bool_t          long_string;
     njs_value_t         *value;
     njs_string_t        *string;
     njs_lvlhsh_t        *values_hash;
     njs_lvlhsh_query_t  lhq;
 
-    long_string = src->type == NJS_STRING
-                  && src->short_string.size == NJS_STRING_LONG;
+    long_string = 0;
+    value_size = sizeof(njs_value_t);
+
+    if (njs_is_string(src)) {
+        njs_string_get(src, &str);
 
-    if (long_string) {
-        size = src->long_string.size;
-        start = src->long_string.data->start;
+        size = (uint32_t) str.length;
+        start = str.start;
+
+        if (src->short_string.size == NJS_STRING_LONG) {
+            long_string = 1;
+        }
 
     } else {
-        size = sizeof(njs_value_t);
+        size = value_size;
         start = (u_char *) src;
     }
 
@@ -4798,22 +4804,18 @@ njs_value_index(njs_vm_t *vm, const njs_value_t *src, njs_uint_t runtime)
         value = lhq.value;
 
     } else {
-        value_size = 0;
-
         if (long_string) {
-            /* Long string value is allocated together with string. */
-            value_size = sizeof(njs_value_t) + sizeof(njs_string_t);
-
             length = src->long_string.data->length;
 
             if (size != length && length > NJS_STRING_MAP_STRIDE) {
                 size = njs_string_map_offset(size)
                        + njs_string_map_size(length);
             }
+
+            value_size += sizeof(njs_string_t) + size;
         }
 
-        value = njs_mp_align(vm->mem_pool, sizeof(njs_value_t),
-                             value_size + size);
+        value = njs_mp_align(vm->mem_pool, sizeof(njs_value_t), value_size);
         if (njs_slow_path(value == NULL)) {
             return NJS_INDEX_NONE;
         }

diff --git a/src/test/njs_unit_test.c b/src/test/njs_unit_test.c
index b2f5d760536203f0ddab585b0e935e8ae7bb0be1..a077d1407e6c085d271124dda58bbfa5a4cd65f8 100644 (file)
--- a/src/test/njs_unit_test.c
+++ b/src/test/njs_unit_test.c
@@ -9593,6 +9593,22 @@ static njs_unit_test_t  njs_test[] =
     { njs_str("String.length"),
       njs_str("1") },
 
+    /* values_hash long vs long string collision. */
+    { njs_str("'XXXXXXXXXXXXXXXQWEEAB' + 'XXXXXXXXXXXXXXXZHGP'"),
+      njs_str("XXXXXXXXXXXXXXXQWEEABXXXXXXXXXXXXXXXZHGP") },
+
+    /* values_hash short vs long string collision. */
+    { njs_str("'SHAAAB' + 'XXXXXXXXXXXXXXXUETBF'"),
+      njs_str("SHAAABXXXXXXXXXXXXXXXUETBF") },
+
+    /* values_hash long vs short string collision. */
+    { njs_str("'XXXXXXXXXXXXXXXUETBF' + 'SHAAAB'"),
+      njs_str("XXXXXXXXXXXXXXXUETBFSHAAAB") },
+
+    /* values_hash short vs short string collision. */
+    { njs_str("'XUBAAAB' + 'XGYXKY'"),
+      njs_str("XUBAAABXGYXKY") },
+
     { njs_str("String.__proto__ === Function.prototype"),
       njs_str("true") },