From 0603402c80cce7e5e39d6d9d56a943c1cfaf7b79 Mon Sep 17 00:00:00 2001
From: Linus Groh <mail@linusgroh.de>
Date: Wed, 4 Nov 2020 09:44:57 +0000
Subject: [PATCH] LibJS: Handle circular references in Array.prototype.join()

This fixes Array.prototype.{join,toString}() crashing with arrays
containing themselves, i.e. circular references.

The spec is suspiciously silent about this, and indeed engine262, a
"100% spec compliant" ECMA-262 implementation, can't handle these cases.
I had a look at some major engines instead and they all seem to keep
track or check for circular references and return an empty string for
already seen objects.

- SpiderMonkey: "AutoCycleDetector detector(cx, obj)"
- V8: "CycleProtectedArrayJoin<JSArray>(...)"
- JavaScriptCore: "StringRecursionChecker checker(globalObject, thisObject)"
- ChakraCore: "scriptContext->CheckObject(thisArg)"

To keep things simple & consistent this uses the same pattern as
JSONObject, MarkupGenerator and js: simply putting each seen object in a
HashTable<Object*>.

Fixes #3929.
---
 Libraries/LibJS/Runtime/ArrayPrototype.cpp          | 13 +++++++++++++
 .../Tests/builtins/Array/Array.prototype.join.js    |  8 ++++++++
 2 files changed, 21 insertions(+)

diff --git a/Libraries/LibJS/Runtime/ArrayPrototype.cpp b/Libraries/LibJS/Runtime/ArrayPrototype.cpp
index 0437a0a5fe..39117d4c50 100644
--- a/Libraries/LibJS/Runtime/ArrayPrototype.cpp
+++ b/Libraries/LibJS/Runtime/ArrayPrototype.cpp
@@ -27,6 +27,7 @@
  */
 
 #include <AK/Function.h>
+#include <AK/HashTable.h>
 #include <AK/StringBuilder.h>
 #include <LibJS/Runtime/Array.h>
 #include <LibJS/Runtime/ArrayIterator.h>
@@ -39,6 +40,8 @@
 
 namespace JS {
 
+static HashTable<Object*> s_array_join_seen_objects;
+
 ArrayPrototype::ArrayPrototype(GlobalObject& global_object)
     : Object(*global_object.object_prototype())
 {
@@ -316,6 +319,13 @@ JS_DEFINE_NATIVE_FUNCTION(ArrayPrototype::join)
     auto* this_object = vm.this_value(global_object).to_object(global_object);
     if (!this_object)
         return {};
+
+    // This is not part of the spec, but all major engines do some kind of circular reference checks.
+    // FWIW: engine262, a "100% spec compliant" ECMA-262 impl, aborts with "too much recursion".
+    if (s_array_join_seen_objects.contains(this_object))
+        return js_string(vm, "");
+    s_array_join_seen_objects.set(this_object);
+
     auto length = get_length(vm, *this_object);
     if (vm.exception())
         return {};
@@ -339,6 +349,9 @@ JS_DEFINE_NATIVE_FUNCTION(ArrayPrototype::join)
             return {};
         builder.append(string);
     }
+
+    s_array_join_seen_objects.remove(this_object);
+
     return js_string(vm, builder.to_string());
 }
 
diff --git a/Libraries/LibJS/Tests/builtins/Array/Array.prototype.join.js b/Libraries/LibJS/Tests/builtins/Array/Array.prototype.join.js
index dcaf733d63..30efb7e2b5 100644
--- a/Libraries/LibJS/Tests/builtins/Array/Array.prototype.join.js
+++ b/Libraries/LibJS/Tests/builtins/Array/Array.prototype.join.js
@@ -14,3 +14,11 @@ test("basic functionality", () => {
     expect([1, null, 2, undefined, 3].join()).toBe("1,,2,,3");
     expect(Array(3).join()).toBe(",,");
 });
+
+test("circular references", () => {
+    const a = ["foo", [], [1, 2, []], ["bar"]];
+    a[1] = a;
+    a[2][2] = a;
+    // [ "foo", <circular>, [ 1, 2, <circular> ], [ "bar" ] ]
+    expect(a.join()).toBe("foo,,1,2,,bar");
+});