Changeset 202173 in webkit

Timestamp:

Jun 17, 2016, 12:22:02 PM (9 years ago)

Author:

mark.lam@apple.com

Message:

OOM Assertion failure in JSON.stringify.
​https://bugs.webkit.org/show_bug.cgi?id=158794
<rdar://problem/26826254>

Reviewed by Saam Barati.

The bug was actually in StringBuilder::appendQuotedJSONString() where it failed
to detect an imminent unsigned int overflow. The fix is to use Checked<unsigned>
for the needed math, and RELEASE_ASSERT afterwards that we did not overflow.

I also added more assertions to detect sooner if any there are any problems with
StringBuilder's m_buffer or m_length being incorrectly sized. These assertions
have been run on the JSC and layout tests without any issue.

• wtf/text/StringBuilder.cpp:

(WTF::StringBuilder::resize):
(WTF::StringBuilder::allocateBuffer):
(WTF::StringBuilder::allocateBufferUpConvert):
(WTF::StringBuilder::reallocateBuffer<LChar>):
(WTF::StringBuilder::reallocateBuffer<UChar>):
(WTF::StringBuilder::reserveCapacity):
(WTF::StringBuilder::appendUninitializedSlow):
(WTF::StringBuilder::append):
(WTF::StringBuilder::appendQuotedJSONString):

• wtf/text/StringBuilder.h:

(WTF::StringBuilder::swap):

Location:

trunk/Source/WTF

Files:

• ChangeLog (modified) (1 diff)
• wtf/text/StringBuilder.cpp (modified) (14 diffs)
• wtf/text/StringBuilder.h (modified) (2 diffs)

trunk/Source/WTF/ChangeLog

@@ +1 @@
+2016-06-17  Mark Lam  <mark.lam@apple.com>
+
+        OOM Assertion failure in JSON.stringify.
+        https://bugs.webkit.org/show_bug.cgi?id=158794
+        <rdar://problem/26826254>
+
+        Reviewed by Saam Barati.
+
+        The bug was actually in StringBuilder::appendQuotedJSONString() where it failed
+        to detect an imminent unsigned int overflow.  The fix is to use Checked<unsigned>
+        for the needed math, and RELEASE_ASSERT afterwards that we did not overflow.
+
+        I also added more assertions to detect sooner if any there are any problems with
+        StringBuilder's m_buffer or m_length being incorrectly sized.  These assertions
+        have been run on the JSC and layout tests without any issue.
+
+        * wtf/text/StringBuilder.cpp:
+        (WTF::StringBuilder::resize):
+        (WTF::StringBuilder::allocateBuffer):
+        (WTF::StringBuilder::allocateBufferUpConvert):
+        (WTF::StringBuilder::reallocateBuffer<LChar>):
+        (WTF::StringBuilder::reallocateBuffer<UChar>):
+        (WTF::StringBuilder::reserveCapacity):
+        (WTF::StringBuilder::appendUninitializedSlow):
+        (WTF::StringBuilder::append):
+        (WTF::StringBuilder::appendQuotedJSONString):
+        * wtf/text/StringBuilder.h:
+        (WTF::StringBuilder::swap):
+
 2016-06-14  Filip Pizlo  <fpizlo@apple.com>
 

trunk/Source/WTF/wtf/text/StringBuilder.cpp

@@ -1 +1 @@
 /*
- * Copyright (C) 2010, 2013 Apple Inc. All rights reserved.
+ * Copyright (C) 2010, 2013, 2016 Apple Inc. All rights reserved.
  * Copyright (C) 2012 Google Inc. All rights reserved.
  *
@@ -81 +81 @@
         }
         m_length = newSize;
+        ASSERT(m_buffer->length() >= m_length);
         return;
     }
@@ -104 +105 @@
     m_buffer = WTFMove(buffer);
     m_string = String();
+    ASSERT(m_buffer->length() == requiredLength);
 }
 
@@ -118 +120 @@
     m_buffer = WTFMove(buffer);
     m_string = String();
+    ASSERT(m_buffer->length() == requiredLength);
 }
 
@@ -125 +128 @@
 {
     ASSERT(m_is8Bit);
+    ASSERT(requiredLength >= m_length);
     // Copy the existing data into a new buffer, set result to point to the end of the existing data.
     auto buffer = StringImpl::createUninitialized(requiredLength, m_bufferCharacters16);
@@ -135 +139 @@
     m_buffer = WTFMove(buffer);
     m_string = String();
+    ASSERT(m_buffer->length() == requiredLength);
 }
 
@@ -151 +156 @@
     else
         allocateBuffer(m_buffer->characters8(), requiredLength);
+    ASSERT(m_buffer->length() == requiredLength);
 }
 
@@ -166 +172 @@
     else
         allocateBuffer(m_buffer->characters16(), requiredLength);
+    ASSERT(m_buffer->length() == requiredLength);
 }
 
@@ -190 +197 @@
         }
     }
+    ASSERT(!newCapacity || m_buffer->length() >= newCapacity);
 }
 
@@ -235 +243 @@
     CharType* result = getBufferCharacters<CharType>() + m_length;
     m_length = requiredLength;
+    ASSERT(m_buffer->length() >= m_length);
     return result;
 }
@@ -268 +277 @@
         }
 
-        memcpy(m_bufferCharacters16 + m_length, characters, static_cast<size_t>(length) * sizeof(UChar));        
+        memcpy(m_bufferCharacters16 + m_length, characters, static_cast<size_t>(length) * sizeof(UChar));
         m_length = requiredLength;
     } else
         memcpy(appendUninitialized<UChar>(length), characters, static_cast<size_t>(length) * sizeof(UChar));
+    ASSERT(m_buffer->length() >= m_length);
 }
 
@@ -413 +423 @@
     // The 2 is for the '"' quotes on each end.
     // The 6 is for characters that need to be \uNNNN encoded.
-    size_t maximumCapacityRequired = length() + 2 + string.length() * 6;
-    RELEASE_ASSERT(maximumCapacityRequired < std::numeric_limits<unsigned>::max());
-    unsigned allocationSize = maximumCapacityRequired;
+    Checked<unsigned> stringLength = string.length();
+    Checked<unsigned> maximumCapacityRequired = length();
+    maximumCapacityRequired += 2 + stringLength * 6;
+    unsigned allocationSize = maximumCapacityRequired.unsafeGet();
     // 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.
     allocationSize = std::max(allocationSize, roundUpToPowerOfTwo(allocationSize));
@@ -423 +434 @@
     else
         reserveCapacity(allocationSize);
+    ASSERT(m_buffer->length() >= allocationSize);
 
     if (is8Bit()) {
@@ -441 +453 @@
         m_length = output - m_bufferCharacters16;
     }
+    ASSERT(m_buffer->length() >= m_length);
 }
 

trunk/Source/WTF/wtf/text/StringBuilder.h

@@ -1 +1 @@
 /*
- * Copyright (C) 2009, 2010, 2012, 2013 Apple Inc. All rights reserved.
+ * Copyright (C) 2009-2010, 2012-2013, 2016 Apple Inc. All rights reserved.
  * Copyright (C) 2012 Google Inc. All rights reserved.
  *
@@ -277 +277 @@
         std::swap(m_is8Bit, stringBuilder.m_is8Bit);
         std::swap(m_bufferCharacters8, stringBuilder.m_bufferCharacters8);
+        ASSERT(!m_buffer || m_buffer->length() >= m_length);
     }