Merge r230026 - appendQuotedJSONString stops on arithmetic overflow i…

…nstead of propagating it upwards https://bugs.webkit.org/show_bug.cgi?id=183894 Reviewed by Saam Barati. JSTests: * stress/json-stringified-overflow.js: Added. (catch): Source/JavaScriptCore: Use the return value of appendQuotedJSONString to fail more gracefully when given a string that is too large to handle. * runtime/JSONObject.cpp: (JSC::Stringifier::appendStringifiedValue): Source/WTF: appendQuotedJSONString now returns a bool indicating whether it succeeded, instead of silently failing when given a string too large to fit in 4GB. * wtf/text/StringBuilder.h: * wtf/text/StringBuilderJSON.cpp: (WTF::StringBuilder::appendQuotedJSONString):
WebKit · Apr 9, 2018 · d043f0e · d043f0e
1 parent dc2aa21
commit d043f0e
Show file tree

Hide file tree

Showing 7 changed files with 51 additions and 7 deletions.
diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
@@ -1,3 +1,13 @@
+2018-03-28  Robin Morisset  <rmorisset@apple.com>
+
+        appendQuotedJSONString stops on arithmetic overflow instead of propagating it upwards
+        https://bugs.webkit.org/show_bug.cgi?id=183894
+
+        Reviewed by Saam Barati.
+
+        * stress/json-stringified-overflow.js: Added.
+        (catch):
+
 2018-03-22  Michael Saboff  <msaboff@apple.com>
 
         Race Condition in arrayProtoFuncReverse() causes wrong results or crash

diff --git a/JSTests/stress/json-stringified-overflow.js b/JSTests/stress/json-stringified-overflow.js
@@ -0,0 +1,3 @@
+try {
+      JSON.stringify("123".padStart(1073741823))
+} catch (e) {}
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,15 @@
+2018-03-28  Robin Morisset  <rmorisset@apple.com>
+
+        appendQuotedJSONString stops on arithmetic overflow instead of propagating it upwards
+        https://bugs.webkit.org/show_bug.cgi?id=183894
+
+        Reviewed by Saam Barati.
+
+        Use the return value of appendQuotedJSONString to fail more gracefully when given a string that is too large to handle.
+
+        * runtime/JSONObject.cpp:
+        (JSC::Stringifier::appendStringifiedValue):
+
 2018-03-25  Saam Barati  <sbarati@apple.com>
 
         r228149 accidentally removed code that resets m_emptyCursor at the end of a GC

diff --git a/Source/JavaScriptCore/runtime/JSONObject.cpp b/Source/JavaScriptCore/runtime/JSONObject.cpp
@@ -357,8 +357,10 @@ Stringifier::StringifyResult Stringifier::appendStringifiedValue(StringBuilder&
     if (value.isString()) {
         const String& string = asString(value)->value(m_exec);
         RETURN_IF_EXCEPTION(scope, StringifyFailed);
-        builder.appendQuotedJSONString(string);
-        return StringifySucceeded;
+        if (builder.appendQuotedJSONString(string))
+            return StringifySucceeded;
+        throwOutOfMemoryError(m_exec, scope);
+        return StringifyFailed;
     }
 
     if (value.isNumber()) {

diff --git a/Source/WTF/ChangeLog b/Source/WTF/ChangeLog
@@ -1,3 +1,17 @@
+2018-03-28  Robin Morisset  <rmorisset@apple.com>
+
+        appendQuotedJSONString stops on arithmetic overflow instead of propagating it upwards
+        https://bugs.webkit.org/show_bug.cgi?id=183894
+
+        Reviewed by Saam Barati.
+
+        appendQuotedJSONString now returns a bool indicating whether it succeeded, instead of silently failing when given a string too large
+        to fit in 4GB.
+
+        * wtf/text/StringBuilder.h:
+        * wtf/text/StringBuilderJSON.cpp:
+        (WTF::StringBuilder::appendQuotedJSONString):
+
 2018-03-25  Carlos Alberto Lopez Perez  <clopez@igalia.com>
 
         WebProcess memory monitor: use %zu format specifier for size_t

diff --git a/Source/WTF/wtf/text/StringBuilder.h b/Source/WTF/wtf/text/StringBuilder.h
@@ -174,7 +174,7 @@ class StringBuilder {
         append(U16_TRAIL(c));
     }
 
-    WTF_EXPORT_PRIVATE void appendQuotedJSONString(const String&);
+    WTF_EXPORT_PRIVATE bool appendQuotedJSONString(const String&);
 
     template<unsigned characterCount>
     ALWAYS_INLINE void appendLiteral(const char (&characters)[characterCount]) { append(characters, characterCount - 1); }

diff --git a/Source/WTF/wtf/text/StringBuilderJSON.cpp b/Source/WTF/wtf/text/StringBuilderJSON.cpp
@@ -74,16 +74,18 @@ ALWAYS_INLINE static void appendQuotedJSONStringInternal(OutputCharacterType*& o
     }
 }
 
-void StringBuilder::appendQuotedJSONString(const String& string)
+bool StringBuilder::appendQuotedJSONString(const String& string)
 {
     // Make sure we have enough buffer space to append this string without having
     // to worry about reallocating in the middle.
     // The 2 is for the '"' quotes on each end.
     // The 6 is for characters that need to be \uNNNN encoded.
-    Checked<unsigned> stringLength = string.length();
-    Checked<unsigned> maximumCapacityRequired = length();
+    Checked<unsigned, RecordOverflow> stringLength = string.length();
+    Checked<unsigned, RecordOverflow> maximumCapacityRequired = length();
     maximumCapacityRequired += 2 + stringLength * 6;
-    unsigned allocationSize = maximumCapacityRequired.unsafeGet();
+    unsigned allocationSize;
+    if (CheckedState::DidOverflow == maximumCapacityRequired.safeGet(allocationSize))
+        return false;
     // This max() is here to allow us to allocate sizes between the range [2^31, 2^32 - 2] because roundUpToPowerOfTwo(1<<31 + some int smaller than 1<<31) == 0.
     // FIXME: roundUpToPowerOfTwo should take Checked<unsigned> and abort if it fails to round up.
     // https://bugs.webkit.org/show_bug.cgi?id=176086
@@ -113,6 +115,7 @@ void StringBuilder::appendQuotedJSONString(const String& string)
         m_length = output - m_bufferCharacters16;
     }
     ASSERT(m_buffer->length() >= m_length);
+    return true;
 }
 
 } // namespace WTF