[webkit-reviews] review granted: [Bug 143511] JSON.stringify should throw stack overflow error : [Attachment 397192] Patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted  review:
Bug 143511: JSON.stringify should throw stack overflow error
https://bugs.webkit.org/show_bug.cgi?id=143511

Attachment 397192: Patch

https://bugs.webkit.org/attachment.cgi?id=397192&action=review




--- Comment #19 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 397192
  --> https://bugs.webkit.org/attachment.cgi?id=397192
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397192&action=review

r=me with fixes.

> Source/JavaScriptCore/runtime/JSONObject.cpp:319
> +constexpr unsigned maximumRecursion = 40000;

Let's call this "maximumHolderStackRecursion".	"maximumRecursion" can
logically work, but is just too misleading as it strongly suggests that it has
to do with native stack recursion instead of the side stack.

> Source/JavaScriptCore/runtime/JSONObject.cpp:410
> +    if (UNLIKELY(m_holderStack.size() > maximumRecursion)) {

Let's make this "m_holderStack.size() >= maximumRecursion" instead of ">".  The
reason is that we're about to add one more entry below.  If we check for ">
max", then we'll end up allow max + 1 entries which is not what the name says. 
Technically, "== max" is sufficient, but ">= max" is a stronger guarantee in
case bugs creep in later.

Additionally, since Stringifier::appendStringifiedValue() can call
Stringifier::Holder::appendNextProperty(), and
Stringifier::Holder::appendNextProperty() can call
Stringifier::appendStringifiedValue(), we have the makings of true native stack
recursion.  Though I don't think your tests will take this path of recursion
(and I didn't work on it enough to figure out to get there), we should add a
real stack recursion check.  Let's add the following to the top of
Stringifier::appendStringifiedValue() after the declaration of the ThrowScope:

    if (UNLIKELY(!vm.isSafeToRecurseSoft())) {
	throwStackOverflowError(m_globalObject, scope);
	return StringifyFailed;
    }

Would be nice if you can figure out a test for this overflow too, but I won't
insist on that.  If you do add just a test, I recommend that at the top of the
test JS file, insert the following line:
//@ requireOptions("--maxPerThreadStackUsage=204800")

That will allow the test to run with a smaller stack, and cause
vm.isSafeToRecurseSoft() to return false sooner.  In this example, I specified
a stack size of 200 KB.  Please adjust it as needed to allow your test to run
(in case it's too small or too big).