From e00eac3d6080d39b414f366d88c7dc167f082055 Mon Sep 17 00:00:00 2001
From: Dmitry Volyntsev <xeioex@nginx.com>
Date: Wed, 12 Jan 2022 17:59:42 +0000
Subject: [PATCH] Fixed Array.prototype.join() when array is changed while
 iterating.

Previously, the function used optimization for ordinary arrays with no
gaps (so called fast arrays).  For a fast array code took elements
directly from internal flat C array. The direct pointer may become
invalid as side-effect of custom toString() method for an element.

Specifically, the pointer was passed directly to
njs_value_to_primitive() which attempts to call toString() followed by
valueOf().  When the array size is changed as a side-effect of
toString() and not a string value is returned by toString() the pointer
becomes invalid and is passed to valueOf() which causes use-after-free.

The fix is to eliminate the micro-optimization which uses direct pointers.

Found by PolyGlot fuzzing framework.

This closes #444 issue on Github.
---
 src/njs_array.c          | 34 +++++++++-------------------------
 src/test/njs_unit_test.c |  8 ++++++++
 2 files changed, 17 insertions(+), 25 deletions(-)

diff --git a/src/njs_array.c b/src/njs_array.c
index 9b4b05ee..54b7fe04 100644
--- a/src/njs_array.c
+++ b/src/njs_array.c
@@ -1573,7 +1573,6 @@ njs_array_prototype_join(njs_vm_t *vm, njs_value_t *args, njs_uint_t nargs,
     njs_int_t          ret;
     njs_chb_t          chain;
     njs_utf8_t         utf8;
-    njs_array_t        *array;
     njs_value_t        *value, *this, entry;
     njs_string_prop_t  separator, string;
 
@@ -1606,18 +1605,11 @@ njs_array_prototype_join(njs_vm_t *vm, njs_value_t *args, njs_uint_t nargs,
     }
 
     length = 0;
-    array = NULL;
     utf8 = njs_is_byte_string(&separator) ? NJS_STRING_BYTE : NJS_STRING_UTF8;
 
-    if (njs_is_fast_array(this)) {
-        array = njs_array(this);
-        len = array->length;
-
-    } else {
-        ret = njs_object_length(vm, this, &len);
-        if (njs_slow_path(ret == NJS_ERROR)) {
-            return ret;
-        }
+    ret = njs_object_length(vm, this, &len);
+    if (njs_slow_path(ret == NJS_ERROR)) {
+        return ret;
     }
 
     if (njs_slow_path(len == 0)) {
@@ -1625,25 +1617,17 @@ njs_array_prototype_join(njs_vm_t *vm, njs_value_t *args, njs_uint_t nargs,
         return NJS_OK;
     }
 
+    value = &entry;
+
     njs_chb_init(&chain, vm->mem_pool);
 
     for (i = 0; i < len; i++) {
-        if (njs_fast_path(array != NULL
-                          && array->object.fast_array
-                          && njs_is_valid(&array->start[i])))
-        {
-            value = &array->start[i];
-
-        } else {
-            ret = njs_value_property_i64(vm, this, i, &entry);
-            if (njs_slow_path(ret == NJS_ERROR)) {
-                return ret;
-            }
-
-            value = &entry;
+        ret = njs_value_property_i64(vm, this, i, value);
+        if (njs_slow_path(ret == NJS_ERROR)) {
+            return ret;
         }
 
-        if (njs_is_valid(value) && !njs_is_null_or_undefined(value)) {
+        if (!njs_is_null_or_undefined(value)) {
             if (!njs_is_string(value)) {
                 last = njs_chb_current(&chain);
 
diff --git a/src/test/njs_unit_test.c b/src/test/njs_unit_test.c
index c9ba7b8c..ae932f0c 100644
--- a/src/test/njs_unit_test.c
+++ b/src/test/njs_unit_test.c
@@ -4801,6 +4801,14 @@ static njs_unit_test_t  njs_test[] =
               ".map(v=>v.join(''))"),
       njs_str(",1345,,1,13,13,13") },
 
+    { njs_str("var o = { toString: () => {"
+              "             for (var i = 0; i < 0x10; i++) {a.push(1)};"
+              "             return {};"
+              "}};"
+              "var a = [o];"
+              "a.join()"),
+      njs_str("TypeError: Cannot convert object to primitive value") },
+
     { njs_str("Array.prototype.splice.call({0:0,1:1,2:2,3:3,length:4},0,3,4,5)"),
       njs_str("0,1,2") },
 
-- 
2.47.3