Changeset 262830 in webkit

Timestamp:

Jun 9, 2020, 7:04:14 PM (5 years ago)

Author:

mark.lam@apple.com

Message:

Stringifier::appendStringifiedValue() should not assume it is always safe to recurse.
​https://bugs.webkit.org/show_bug.cgi?id=213006
<rdar://problem/64154840>

Reviewed by Keith Miller.

JSTests:

• stress/json-stringify-executing-in-reserved-zone.js: Added.

Source/JavaScriptCore:

In r262727, I suggested that Alexey Shvayka add an assertion in
Stringifier::appendStringifiedValue() to assert that it is safe to recurse because
we don't expect it to recurse into itself. Turns out this is a bad idea because
a client may be doing the recursing before calling Stringifier::appendStringifiedValue().
As a result, Stringifier::appendStringifiedValue() ends up being executed with
the stack pointer already in the reserved zone. This is legal, and is what the
reserved zone is intended for as long as we don't recurse from here. However,
this also means that asserting vm.isSafeToRecurseSoft() here will surely fail
because we are already in the reserved zone area. The fix is simply to remove
this faulty assertion.

• runtime/JSONObject.cpp:

(JSC::Stringifier::appendStringifiedValue):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/json-stringify-executing-in-reserved-zone.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/runtime/JSONObject.cpp (modified) (2 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2020-06-09  Mark Lam  <mark.lam@apple.com>
+
+        Stringifier::appendStringifiedValue() should not assume it is always safe to recurse.
+        https://bugs.webkit.org/show_bug.cgi?id=213006
+        <rdar://problem/64154840>
+
+        Reviewed by Keith Miller.
+
+        * stress/json-stringify-executing-in-reserved-zone.js: Added.
+
 2020-06-09  Mark Lam  <mark.lam@apple.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2020-06-09  Mark Lam  <mark.lam@apple.com>
+
+        Stringifier::appendStringifiedValue() should not assume it is always safe to recurse.
+        https://bugs.webkit.org/show_bug.cgi?id=213006
+        <rdar://problem/64154840>
+
+        Reviewed by Keith Miller.
+
+        In r262727, I suggested that Alexey Shvayka add an assertion in
+        Stringifier::appendStringifiedValue() to assert that it is safe to recurse because
+        we don't expect it to recurse into itself.  Turns out this is a bad idea because
+        a client may be doing the recursing before calling Stringifier::appendStringifiedValue().
+        As a result, Stringifier::appendStringifiedValue() ends up being executed with
+        the stack pointer already in the reserved zone.  This is legal, and is what the
+        reserved zone is intended for as long as we don't recurse from here.  However,
+        this also means that asserting vm.isSafeToRecurseSoft() here will surely fail
+        because we are already in the reserved zone area.  The fix is simply to remove
+        this faulty assertion.
+
+        * runtime/JSONObject.cpp:
+        (JSC::Stringifier::appendStringifiedValue):
+
 2020-06-09  Mark Lam  <mark.lam@apple.com>
 

trunk/Source/JavaScriptCore/runtime/JSONObject.cpp

@@ -1 +1 @@
 /*
- * Copyright (C) 2009-2019 Apple Inc. All rights reserved.
+ * Copyright (C) 2009-2020 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -315 +315 @@
 
     // Recursion is avoided by !holderStackWasEmpty check and do/while loop at the end of this method.
-    ASSERT(vm.isSafeToRecurseSoft());
+    // We're having this recursion check here as a fail safe in case the code
+    // below get modified such that recursion is no longer avoided.
     if (UNLIKELY(!vm.isSafeToRecurseSoft())) {
         throwStackOverflowError(m_globalObject, scope);