Skip to content

[JSC] Keep cycle detection in the array to string conversions (join, toString, toLocaleString) #446

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
robobun wants to merge 1 commit into from
Closed

[JSC] Keep cycle detection in the array to string conversions (join, toString, toLocaleString) #446

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
305 changes: 193 additions & 112 deletions JSTests/stress/string-conversion-recursion.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
// Cyclic string conversions must follow the spec: they recurse until the stack is exhausted and
// then throw a RangeError. They must never silently substitute the empty string, and a conversion
// that happens to reuse a receiver already on the stack without actually being cyclic must produce
// its normal result.
// Bun's JavaScriptCore keeps cycle detection for the array to string conversions (StringRecursionChecker):
// an array whose conversion is already on the stack converts to the empty string instead of recursing
// until the stack overflows, which is what V8 and SpiderMonkey do, so `a = [1]; a[1] = a; a.join()` is
// "1," here exactly as in Node.js. Upstream removed the detection (https://bugs.webkit.org/show_bug.cgi?id=320820)
// and throws a RangeError instead. Everything else follows upstream: only the array conversions take part,
// so Error.prototype.toString and RegExp.prototype.toString cycles overflow the stack, and a conversion that
// is deep but not cyclic overflows it too.

function shouldBe(actual, expected) {
if (actual !== expected)
throw new Error(`bad value: expected ${JSON.stringify(expected)} but got ${JSON.stringify(actual)}`);
}

function shouldThrowRangeError(func) {
function shouldThrow(func, errorConstructor, message) {
let error;
try {
func();
Expand All @@ -17,87 +20,120 @@ function shouldThrowRangeError(func) {
}
if (!error)
throw new Error("didn't throw");
if (!(error instanceof RangeError))
throw new Error(`expected RangeError but got ${error}`);
if (!(error instanceof errorConstructor))
throw new Error(`expected ${errorConstructor.name} but got ${error}`);
if (message !== undefined)
shouldBe(error.message, message);
}

// Array.prototype.join, Array.prototype.toString and Array.prototype.toLocaleString have no cycle
// detection in the specification, so a self-referential array overflows the stack.
shouldThrowRangeError(() => {
let array = [];
array[0] = array;
return array.join();
});
function selfContaining() {
let array = [1];
array[1] = array;
return array;
}

shouldThrowRangeError(() => {
let array = [];
array[0] = array;
return array.toString();
});
// An array that contains itself: the nested conversion of the array is the empty string, through every
// entry point. Array.prototype.toString and ToString / ToPrimitive of an array go through JSArray::fastToString,
// Array.prototype.join and Array.prototype.toLocaleString are checked themselves.
shouldBe(selfContaining().join(), "1,");
shouldBe(selfContaining().join("-"), "1-");
shouldBe(selfContaining().join(""), "1");
shouldBe(selfContaining().toString(), "1,");
shouldBe(selfContaining().toLocaleString(), "1,");
shouldBe(String(selfContaining()), "1,");
shouldBe(`${selfContaining()}`, "1,");
shouldBe(selfContaining() + "", "1,");
shouldBe([selfContaining()].join(), "1,");

shouldThrowRangeError(() => {
{
let array = [];
array[0] = array;
return `${array}`;
});
shouldBe(array.join(), "");
shouldBe(String(array), "");
shouldBe(array.toLocaleString(), "");
}

shouldThrowRangeError(() => {
let array = [];
array[0] = array;
return array.toLocaleString();
});
// Every occurrence of the array being converted becomes empty; the conversion does not fan out.
{
let array = selfContaining();
array[2] = array;
shouldBe(array.join(), "1,,");
shouldBe(String(array), "1,,");
shouldBe(array.toLocaleString(), "1,,");
}

// Only the array whose conversion is in progress becomes empty; everything reachable before the cycle
// closes is converted normally, so the result depends on where the conversion starts.
{
let a = [1];
let b = [2, a];
a.push(b);
shouldBe(a.join(), "1,2,");
shouldBe(b.join(), "2,1,");
shouldBe(String(a), "1,2,");
shouldBe(String(b), "2,1,");
shouldBe(a.toLocaleString(), "1,2,");
shouldBe(b.toLocaleString(), "2,1,");
}

// Indirect cycles overflow too.
shouldThrowRangeError(() => {
{
let array = [1, "webkit"];
array[2] = [3, 4, [5, 6, [array]]];
return array.toString();
});
shouldBe(array.toString(), "1,webkit,3,4,5,6,");
shouldBe(array.join("|"), "1|webkit|3,4,5,6,");
}

shouldThrowRangeError(() => {
// The cycle may close through user code that converts the array again.
{
let array = ["a"];
array.push({ toString() { return array.join("~"); } });
return array.join("-");
});
shouldBe(array.join("-"), "a-");
shouldBe(String(array), "a,");
}

// A cycle whose fan-out is larger than one must still terminate: the depth-first descent hits the
// stack limit and the RangeError propagates out instead of the join fanning out exponentially.
shouldThrowRangeError(() => {
let array = [];
array[0] = array;
array[1] = array;
return array.toString();
});
{
let array = ["a"];
array.push({ toLocaleString() { return array.toLocaleString(); } });
shouldBe(array.toLocaleString(), "a,");
}

// Error.prototype.toString has no cycle detection either.
shouldThrowRangeError(() => {
let error = new Error;
error.name = error;
error.message = error;
return `${error}`;
});
// Array.prototype.toString calling a replaced join still detects the cycle inside that join.
{
let array = selfContaining();
array.join = function() { return "J:" + Array.prototype.join.call(this); };
shouldBe(array.toString(), "J:1,J:");
shouldBe(String(array), "J:1,J:");
}

shouldThrowRangeError(() => {
let error = new Error;
error.message = { toString() { return Error.prototype.toString.call(error); } };
return `${error}`;
});
// Array-like receivers are tracked too: these take the generic join / toLocaleString paths.
{
let object = { length: 2, 0: "x" };
object[1] = { toString() { return Array.prototype.join.call(object, "+"); } };
shouldBe(Array.prototype.join.call(object, "-"), "x-");
}

// Nor does RegExp.prototype.toString.
shouldThrowRangeError(() => {
let regExp = /a/;
Object.defineProperty(regExp, "source", { get() { return regExp; } });
return `${regExp}`;
});
{
let object = { length: 2, 0: 1 };
object[1] = { toLocaleString() { return Array.prototype.toLocaleString.call(object); } };
shouldBe(Array.prototype.toLocaleString.call(object), "1,");
}

shouldThrowRangeError(() => {
let regExp = /a/;
Object.defineProperty(regExp, "flags", { get() { return RegExp.prototype.toString.call(regExp); } });
return `${regExp}`;
});
// Converting the same array twice in a row, or the same array twice within one conversion, is not a cycle.
{
let array = selfContaining();
shouldBe(array.join(), "1,");
shouldBe(array.join(), "1,");
shouldBe(String(array), "1,");

// These conversions terminate on their own. Sharing a receiver with a conversion further up the
// stack is not a cycle, so each must return its ordinary result.
let shared = [1, 2];
shouldBe([shared, shared].toString(), "1,2,1,2");
shouldBe([shared, shared].toLocaleString(), "1,2,1,2");
shouldBe([shared, shared].join("|"), "1,2|1,2");
shouldBe([[1, [2]], [3]].join(), "1,2,3");
}

// Error.prototype.toString and RegExp.prototype.toString do not take part, so applying them to an array that
// is being converted, or converting an array-like from inside them, is not a cycle either.
{
let array = [];
array[0] = { toString() { return Error.prototype.toString.call(array); } };
Expand All @@ -122,65 +158,110 @@ shouldThrowRangeError(() => {
shouldBe(array.join("-"), "a-b");
}

// The receiver of an inner conversion being an ancestor's receiver is fine as long as the value
// graph is acyclic.
// A conversion that throws must unregister every array it was converting, whether it was the outermost one
// or nested inside another; otherwise the next conversion of the same array would be taken for a cycle.
{
let error = new Error;
error.name = "E";
error.message = { toString() { return Error.prototype.toString.call({ name: "inner", message: "m" }); } };
shouldBe(`${error}`, "E: inner: m");
}
let thrower = { toString() { throw new Error("boom"); }, toLocaleString() { throw new Error("locale boom"); } };

// A deep but acyclic nesting still converts, and a shared subtree is visited every time it occurs
// rather than being replaced by the empty string on the second visit.
{
let shared = [1, 2];
shouldBe([shared, shared].toString(), "1,2,1,2");
shouldBe([shared, shared].toLocaleString(), "1,2,1,2");
shouldBe([shared, shared].join("|"), "1,2|1,2");
}
let array = [1, thrower];
shouldThrow(() => array.join(), Error, "boom");
shouldThrow(() => String(array), Error, "boom");
shouldThrow(() => array.toLocaleString(), Error, "locale boom");
array[1] = 2;
shouldBe(array.join(), "1,2");
shouldBe(String(array), "1,2");
shouldBe(array.toLocaleString(), "1,2");

// Recovering after a stack overflow must leave no state behind that suppresses later conversions.
{
let array = [];
array[0] = array;
for (let i = 0; i < 2; ++i)
shouldThrowRangeError(() => array.toString());
shouldBe([1, 2].toString(), "1,2");
let inner = [thrower];
let outer = [inner];
shouldThrow(() => outer.join(), Error, "boom");
shouldThrow(() => String(outer), Error, "boom");
shouldThrow(() => outer.toLocaleString(), Error, "locale boom");
shouldThrow(() => Array.prototype.join.call({ length: 1, 0: inner }), Error, "boom");
inner[0] = "i";
shouldBe(outer.join(), "i");
shouldBe(String(outer), "i");
shouldBe(outer.toLocaleString(), "i");
shouldBe(Array.prototype.join.call({ length: 1, 0: inner }), "i");

shouldBe(selfContaining().join(), "1,");
}

// Error.prototype.toString and RegExp.prototype.toString cycles behave as upstream: they overflow the stack.
shouldThrow(() => {
let error = new Error;
error.name = error;
shouldThrowRangeError(() => `${error}`);
shouldBe(`${new Error("m")}`, "Error: m");
error.message = error;
return `${error}`;
}, RangeError);

shouldThrow(() => {
let error = new Error;
error.message = { toString() { return Error.prototype.toString.call(error); } };
return `${error}`;
}, RangeError);

shouldThrow(() => {
let regExp = /a/;
Object.defineProperty(regExp, "source", { get() { return regExp; } });
return `${regExp}`;
}, RangeError);

shouldThrow(() => {
let regExp = /a/;
Object.defineProperty(regExp, "flags", { get() { return RegExp.prototype.toString.call(regExp); } });
return `${regExp}`;
}, RangeError);

// Cycle detection does not replace the stack check: a nesting that is deep but acyclic still overflows (the
// shallowest overflowing depth is around 1000 arrays in a debug ASan build and around 5000 in release), and
// the overflow unwinding through thousands of conversions leaves every array convertible again.
{
let deepest = [0];
let top = deepest;
for (let i = 0; i < 100000; ++i)
top = [top];
let second = top[0];

shouldThrow(() => String(top), RangeError);
shouldThrow(() => top.join(), RangeError);
shouldThrow(() => top.toLocaleString(), RangeError);

top[0] = "top";
shouldBe(String(top), "top");
second[0] = "second";
shouldBe(String([second]), "second");
shouldBe([second, deepest].join("-"), "second-0");
shouldBe(selfContaining().join(), "1,");
}

// The optimizing tiers reach the array conversions through their own paths, so warm them up on an
// acyclic array and then check that a cyclic one still overflows there and that the overflow leaves
// the warmed-up conversions intact.
// The DFG and FTL compile Array.prototype.join on an array with the original structure into their own operation
// and reach ToString / ToPrimitive of an array through their own paths. Warm the call sites up on an acyclic
// array with the same (contiguous) shape, then the cyclic array must convert exactly as it does in the
// interpreter, and the acyclic one must still convert afterwards.
{
const convert = a => `${a}`;
const join = a => a.join("-");
const toLocale = a => a.toLocaleString();
noInline(convert);
const join = array => array.join("-");
const convert = array => `${array}`;
const toLocale = array => array.toLocaleString();
noInline(join);
noInline(convert);
noInline(toLocale);

let acyclic = [1, 2, 3];
let cyclic = [];
cyclic[0] = cyclic;

for (let i = 0; i < 20000; ++i) {
shouldBe(convert(acyclic), "1,2,3");
shouldBe(join(acyclic), "1-2-3");
shouldBe(toLocale(acyclic), "1,2,3");
let acyclic = [1, [2, 3], "x"];
for (let i = 0; i < testLoopCount; ++i) {
shouldBe(join(acyclic), "1-2,3-x");
shouldBe(convert(acyclic), "1,2,3,x");
shouldBe(toLocale(acyclic), "1,2,3,x");
}

for (const f of [convert, join, toLocale]) {
shouldThrowRangeError(() => f(cyclic));
shouldThrowRangeError(() => f(cyclic));
let cyclic = selfContaining();
for (let i = 0; i < 10; ++i) {
shouldBe(join(cyclic), "1-");
shouldBe(convert(cyclic), "1,");
shouldBe(toLocale(cyclic), "1,");
}

shouldBe(convert(acyclic), "1,2,3");
shouldBe(join(acyclic), "1-2-3");
shouldBe(toLocale(acyclic), "1,2,3");
shouldBe(join(acyclic), "1-2,3-x");
shouldBe(convert(acyclic), "1,2,3,x");
shouldBe(toLocale(acyclic), "1,2,3,x");
}
11 changes: 11 additions & 0 deletions Source/JavaScriptCore/dfg/DFGOperations.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,10 @@
#include "WeakMapPrototype.h"
#include "WeakSetPrototype.h"

#if USE(BUN_JSC_ADDITIONS)
#include "StringRecursionChecker.h"
#endif

#if ENABLE(JIT)
#if ENABLE(DFG_JIT)

Expand Down Expand Up @@ -1748,6 +1752,13 @@ static ALWAYS_INLINE JSString* arrayJoinWithStringSeparator(JSGlobalObject* glob
auto view = separator->view(globalObject);
RETURN_IF_EXCEPTION(scope, { });

#if USE(BUN_JSC_ADDITIONS)
// Same as arrayProtoFuncJoin, which this operation stands in for once a join call site is DFG or FTL compiled.
StringRecursionChecker checker(scope.vm(), array);
if (checker.isRecursive()) [[unlikely]]
return jsEmptyString(scope.vm());
#endif

bool sawHoles = false;
bool genericCase = false;
return fastArrayJoin(globalObject, array, view, length, sawHoles, genericCase);
Expand Down
Loading
Loading