Changeset 167336 in webkit

Timestamp:

Apr 15, 2014, 4:33:11 PM (11 years ago)

Author:

fpizlo@apple.com

Message:

compileMakeRope does not emit necessary bounds checks
​https://bugs.webkit.org/show_bug.cgi?id=130684
<rdar://problem/16398388>

Reviewed by Oliver Hunt.

Add string length bounds checks in a bunch of places. We should never allow a string
to have a length greater than 2<sup>31-1 because it's not clear that the language has
semantics for it and because there is code that assumes that this cannot happen.

Also add a bunch of tests to that effect to cover the various ways in which this was
previously allowed to happen.</sup>

• dfg/DFGOperations.cpp:
• dfg/DFGSpeculativeJIT.cpp:

(JSC::DFG::SpeculativeJIT::compileMakeRope):

• ftl/FTLLowerDFGToLLVM.cpp:

(JSC::FTL::LowerDFGToLLVM::compileMakeRope):

• runtime/JSString.cpp:

(JSC::JSRopeString::RopeBuilder::expand):

• runtime/JSString.h:

(JSC::JSString::create):
(JSC::JSRopeString::RopeBuilder::append):
(JSC::JSRopeString::RopeBuilder::release):
(JSC::JSRopeString::append):

• runtime/Operations.h:

(JSC::jsString):
(JSC::jsStringFromRegisterArray):
(JSC::jsStringFromArguments):

• runtime/StringPrototype.cpp:

(JSC::stringProtoFuncIndexOf):
(JSC::stringProtoFuncSlice):
(JSC::stringProtoFuncSubstring):
(JSC::stringProtoFuncToLowerCase):

• tests/stress/make-large-string-jit-strcat.js: Added.

(foo):

• tests/stress/make-large-string-jit.js: Added.

(foo):

• tests/stress/make-large-string-strcat.js: Added.
• tests/stress/make-large-string.js: Added.

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• dfg/DFGOperations.cpp (modified) (2 diffs)
• dfg/DFGSpeculativeJIT.cpp (modified) (1 diff)
• ftl/FTLLowerDFGToLLVM.cpp (modified) (1 diff)
• runtime/JSString.cpp (modified) (1 diff)
• runtime/JSString.h (modified) (4 diffs)
• runtime/Operations.h (modified) (5 diffs)
• runtime/StringPrototype.cpp (modified) (4 diffs)
• tests/stress/make-large-string-jit-strcat.js (added)
• tests/stress/make-large-string-jit.js (added)
• tests/stress/make-large-string-strcat.js (added)
• tests/stress/make-large-string.js (added)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2014-04-15  Filip Pizlo  <fpizlo@apple.com>
+
+        compileMakeRope does not emit necessary bounds checks
+        https://bugs.webkit.org/show_bug.cgi?id=130684
+        <rdar://problem/16398388>
+
+        Reviewed by Oliver Hunt.
+        
+        Add string length bounds checks in a bunch of places. We should never allow a string
+        to have a length greater than 2^31-1 because it's not clear that the language has
+        semantics for it and because there is code that assumes that this cannot happen.
+        
+        Also add a bunch of tests to that effect to cover the various ways in which this was
+        previously allowed to happen.
+
+        * dfg/DFGOperations.cpp:
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::compileMakeRope):
+        * ftl/FTLLowerDFGToLLVM.cpp:
+        (JSC::FTL::LowerDFGToLLVM::compileMakeRope):
+        * runtime/JSString.cpp:
+        (JSC::JSRopeString::RopeBuilder::expand):
+        * runtime/JSString.h:
+        (JSC::JSString::create):
+        (JSC::JSRopeString::RopeBuilder::append):
+        (JSC::JSRopeString::RopeBuilder::release):
+        (JSC::JSRopeString::append):
+        * runtime/Operations.h:
+        (JSC::jsString):
+        (JSC::jsStringFromRegisterArray):
+        (JSC::jsStringFromArguments):
+        * runtime/StringPrototype.cpp:
+        (JSC::stringProtoFuncIndexOf):
+        (JSC::stringProtoFuncSlice):
+        (JSC::stringProtoFuncSubstring):
+        (JSC::stringProtoFuncToLowerCase):
+        * tests/stress/make-large-string-jit-strcat.js: Added.
+        (foo):
+        * tests/stress/make-large-string-jit.js: Added.
+        (foo):
+        * tests/stress/make-large-string-strcat.js: Added.
+        * tests/stress/make-large-string.js: Added.
+
 2014-04-15  Julien Brianceau  <jbriance@cisco.com>
 

trunk/Source/JavaScriptCore/dfg/DFGOperations.cpp

@@ -969 +969 @@
     VM& vm = exec->vm();
     NativeCallFrameTracer tracer(&vm, exec);
+    
+    if (static_cast<int32_t>(left->length() + right->length()) < 0) {
+        throwOutOfMemoryError(exec);
+        return 0;
+    }
 
     return JSRopeString::create(vm, left, right);
@@ -977 +982 @@
     VM& vm = exec->vm();
     NativeCallFrameTracer tracer(&vm, exec);
+
+    Checked<int32_t, RecordOverflow> length = a->length();
+    length += b->length();
+    length += c->length();
+    if (length.hasOverflowed()) {
+        throwOutOfMemoryError(exec);
+        return 0;
+    }
 
     return JSRopeString::create(vm, a, b, c);

trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp

@@ -2794 +2794 @@
     m_jit.load32(JITCompiler::Address(opGPRs[0], JSString::offsetOfFlags()), scratchGPR);
     m_jit.load32(JITCompiler::Address(opGPRs[0], JSString::offsetOfLength()), allocatorGPR);
+    if (!ASSERT_DISABLED) {
+        JITCompiler::Jump ok = m_jit.branch32(
+            JITCompiler::GreaterThanOrEqual, allocatorGPR, TrustedImm32(0));
+        m_jit.breakpoint();
+        ok.link(&m_jit);
+    }
     for (unsigned i = 1; i < numOpGPRs; ++i) {
         m_jit.and32(JITCompiler::Address(opGPRs[i], JSString::offsetOfFlags()), scratchGPR);
-        m_jit.add32(JITCompiler::Address(opGPRs[i], JSString::offsetOfLength()), allocatorGPR);
+        speculationCheck(
+            Uncountable, JSValueSource(), nullptr,
+            m_jit.branchAdd32(
+                JITCompiler::Overflow,
+                JITCompiler::Address(opGPRs[i], JSString::offsetOfLength()), allocatorGPR));
     }
     m_jit.and32(JITCompiler::TrustedImm32(JSString::Is8Bit), scratchGPR);
     m_jit.store32(scratchGPR, JITCompiler::Address(resultGPR, JSString::offsetOfFlags()));
+    if (!ASSERT_DISABLED) {
+        JITCompiler::Jump ok = m_jit.branch32(
+            JITCompiler::GreaterThanOrEqual, allocatorGPR, TrustedImm32(0));
+        m_jit.breakpoint();
+        ok.link(&m_jit);
+    }
     m_jit.store32(allocatorGPR, JITCompiler::Address(resultGPR, JSString::offsetOfLength()));
     

trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp

@@ -2944 +2944 @@
         for (unsigned i = 1; i < numKids; ++i) {
             flags = m_out.bitAnd(flags, m_out.load32(kids[i], m_heaps.JSString_flags));
-            length = m_out.add(length, m_out.load32(kids[i], m_heaps.JSString_length));
+            LValue lengthAndOverflow = m_out.addWithOverflow32(
+                length, m_out.load32(kids[i], m_heaps.JSString_length));
+            speculate(Uncountable, noValue(), 0, m_out.extractValue(lengthAndOverflow, 1));
+            length = m_out.extractValue(lengthAndOverflow, 0);
         }
         m_out.store32(

trunk/Source/JavaScriptCore/runtime/JSString.cpp

@@ -39 +39 @@
     ASSERT(m_index == JSRopeString::s_maxInternalRopeLength);
     JSString* jsString = m_jsString;
+    RELEASE_ASSERT(jsString);
     m_jsString = jsStringBuilder(&m_vm);
     m_index = 0;

trunk/Source/JavaScriptCore/runtime/JSString.h

@@ -2 +2 @@
  *  Copyright (C) 1999-2001 Harri Porten (porten@kde.org)
  *  Copyright (C) 2001 Peter Kelly (pmk@post.com)
- *  Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008 Apple Inc. All rights reserved.
+ *  Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2014 Apple Inc. All rights reserved.
  *
  *  This library is free software; you can redistribute it and/or
@@ -124 +124 @@
         {
             ASSERT(value);
-            size_t length = value->length();
+            int32_t length = value->length();
+            RELEASE_ASSERT(length >= 0);
             size_t cost = value->cost();
             JSString* newString = new (NotNull, allocateCell<JSString>(vm.heap)) JSString(vm, value);
@@ -229 +230 @@
             }
 
-            void append(JSString* jsString)
+            bool append(JSString* jsString)
             {
                 if (m_index == JSRopeString::s_maxInternalRopeLength)
                     expand();
+                if (static_cast<int32_t>(m_jsString->length() + jsString->length()) < 0) {
+                    m_jsString = 0;
+                    return false;
+                }
                 m_jsString->append(m_vm, m_index++, jsString);
+                return true;
             }
 
             JSRopeString* release()
             {
+                RELEASE_ASSERT(m_jsString);
                 JSRopeString* tmp = m_jsString;
                 m_jsString = 0;
@@ -287 +294 @@
             m_fibers[index].set(vm, this, jsString);
             m_length += jsString->m_length;
+            RELEASE_ASSERT(static_cast<int32_t>(m_length) >= 0);
             setIs8Bit(is8Bit() && jsString->is8Bit());
         }

trunk/Source/JavaScriptCore/runtime/Operations.h

@@ -39 +39 @@
     VM& vm = exec->vm();
 
-    unsigned length1 = s1->length();
+    int32_t length1 = s1->length();
     if (!length1)
         return s2;
-    unsigned length2 = s2->length();
+    int32_t length2 = s2->length();
     if (!length2)
         return s1;
-    if ((length1 + length2) < length1)
+    if ((length1 + length2) < 0)
         return throwOutOfMemoryError(exec);
 
@@ -55 +55 @@
     VM* vm = &exec->vm();
 
-    unsigned length1 = u1.length();
-    unsigned length2 = u2.length();
-    unsigned length3 = u3.length();
+    int32_t length1 = u1.length();
+    int32_t length2 = u2.length();
+    int32_t length3 = u3.length();
+    
+    if (length1 < 0 || length2 < 0 || length3 < 0)
+        return throwOutOfMemoryError(exec);
+    
     if (!length1)
         return jsString(exec, jsString(vm, u2), jsString(vm, u3));
@@ -65 +69 @@
         return jsString(exec, jsString(vm, u1), jsString(vm, u2));
 
-    if ((length1 + length2) < length1)
-        return throwOutOfMemoryError(exec);
-    if ((length1 + length2 + length3) < length3)
+    if ((length1 + length2) < 0)
+        return throwOutOfMemoryError(exec);
+    if ((length1 + length2 + length3) < 0)
         return throwOutOfMemoryError(exec);
 
@@ -78 +82 @@
     JSRopeString::RopeBuilder ropeBuilder(*vm);
 
-    unsigned oldLength = 0;
-
     for (unsigned i = 0; i < count; ++i) {
         JSValue v = strings[-static_cast<int>(i)].jsValue();
-        ropeBuilder.append(v.toString(exec));
-
-        if (ropeBuilder.length() < oldLength) // True for overflow
+        if (!ropeBuilder.append(v.toString(exec)))
             return throwOutOfMemoryError(exec);
-        oldLength = ropeBuilder.length();
     }
 
@@ -98 +97 @@
     ropeBuilder.append(thisValue.toString(exec));
 
-    unsigned oldLength = 0;
-
     for (unsigned i = 0; i < exec->argumentCount(); ++i) {
         JSValue v = exec->argument(i);
-        ropeBuilder.append(v.toString(exec));
-
-        if (ropeBuilder.length() < oldLength) // True for overflow
+        if (!ropeBuilder.append(v.toString(exec)))
             return throwOutOfMemoryError(exec);
-        oldLength = ropeBuilder.length();
     }
 

trunk/Source/JavaScriptCore/runtime/StringPrototype.cpp

@@ -763 +763 @@
     if (!a1.isUndefined()) {
         int len = thisJSString->length();
+        RELEASE_ASSERT(len >= 0);
         if (a1.isUInt32())
             pos = std::min<uint32_t>(a1.asUInt32(), len);
@@ -917 +918 @@
     String s = thisValue.toString(exec)->value(exec);
     int len = s.length();
+    RELEASE_ASSERT(len >= 0);
 
     JSValue a0 = exec->argument(0);
@@ -1228 +1230 @@
     JSValue a1 = exec->argument(1);
     int len = jsString->length();
+    RELEASE_ASSERT(len >= 0);
 
     double start = a0.toNumber(exec);
@@ -1265 +1268 @@
     if (!sSize)
         return JSValue::encode(sVal);
+    RELEASE_ASSERT(sSize >= 0);
 
     StringImpl* ourImpl = s.impl();