Make string MaxLength for all WTF and JS strings consistently equal to INT_MAX.
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 3 Oct 2018 18:28:55 +0000 (18:28 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 3 Oct 2018 18:28:55 +0000 (18:28 +0000)
https://bugs.webkit.org/show_bug.cgi?id=190187
<rdar://problem/42512909>

Reviewed by Michael Saboff.

JSTests:

* stress/regress-190187.js: Added.

Source/JavaScriptCore:

Allowing different max string lengths at each level opens up opportunities for
bugs to creep in.  With 2 different max length values, it is more difficult to
keep the story straight on how we do overflow / bounds checks at each place in
the code.  It's also difficult to tell if a seemingly valid check at the WTF level
will have bad ramifications at the JSC level.  Also, it's also not meaningful to
support a max length > INT_MAX.  To eliminate this class of bugs, we'll
standardize on a MaxLength of INT_MAX at all levels.

We'll also standardize the way we do length overflow checks on using
CheckedArithmetic, and add some asserts to document the assumptions of the code.

* runtime/FunctionConstructor.cpp:
(JSC::constructFunctionSkippingEvalEnabledCheck):
- Fix OOM error handling which crashed a test after the new MaxLength was applied.
* runtime/JSString.h:
(JSC::JSString::finishCreation):
(JSC::JSString::createHasOtherOwner):
(JSC::JSString::setLength):
* runtime/JSStringInlines.h:
(JSC::jsMakeNontrivialString):
* runtime/Operations.h:
(JSC::jsString):

Source/WTF:

* wtf/text/StringConcatenate.h:
(WTF::tryMakeStringFromAdapters):
(WTF::sumWithOverflow): Deleted.
* wtf/text/StringImpl.h:
* wtf/text/WTFString.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@236804 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/stress/regress-190187.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/FunctionConstructor.cpp
Source/JavaScriptCore/runtime/JSString.h
Source/JavaScriptCore/runtime/JSStringInlines.h
Source/JavaScriptCore/runtime/Operations.h
Source/WTF/ChangeLog
Source/WTF/wtf/text/StringConcatenate.h
Source/WTF/wtf/text/StringImpl.h
Source/WTF/wtf/text/WTFString.h

index b2354ed..08edee9 100644 (file)
@@ -1,3 +1,13 @@
+2018-10-03  Mark Lam  <mark.lam@apple.com>
+
+        Make string MaxLength for all WTF and JS strings consistently equal to INT_MAX.
+        https://bugs.webkit.org/show_bug.cgi?id=190187
+        <rdar://problem/42512909>
+
+        Reviewed by Michael Saboff.
+
+        * stress/regress-190187.js: Added.
+
 2018-10-02  Caio Lima  <ticaiolima@gmail.com>
 
         [BigInt] BigInt.proptotype.toString is broken when radix is power of 2
diff --git a/JSTests/stress/regress-190187.js b/JSTests/stress/regress-190187.js
new file mode 100644 (file)
index 0000000..72de2d8
--- /dev/null
@@ -0,0 +1,18 @@
+//@ runDefault
+//@ skip if $memoryLimited or $buildType == "debug"
+//@ slow!
+
+try {
+    var v1 = "AAAAAAAAAAA";
+    for(var i = 0; i < 27; i++)
+      v1 = v1 + v1;
+    var v2;
+    var v3 = RegExp.prototype.toString.call({source:v1,flags:v1});
+    v3 += v1;
+    v2 += v3.localeCompare(v1);
+} catch (e) {
+    exception = e;
+}
+
+if (exception != "Error: Out of memory")
+    throw "FAILED";
index 49bdec3..21f16cc 100644 (file)
@@ -1,3 +1,34 @@
+2018-10-03  Mark Lam  <mark.lam@apple.com>
+
+        Make string MaxLength for all WTF and JS strings consistently equal to INT_MAX.
+        https://bugs.webkit.org/show_bug.cgi?id=190187
+        <rdar://problem/42512909>
+
+        Reviewed by Michael Saboff.
+
+        Allowing different max string lengths at each level opens up opportunities for
+        bugs to creep in.  With 2 different max length values, it is more difficult to
+        keep the story straight on how we do overflow / bounds checks at each place in
+        the code.  It's also difficult to tell if a seemingly valid check at the WTF level
+        will have bad ramifications at the JSC level.  Also, it's also not meaningful to
+        support a max length > INT_MAX.  To eliminate this class of bugs, we'll
+        standardize on a MaxLength of INT_MAX at all levels.
+
+        We'll also standardize the way we do length overflow checks on using
+        CheckedArithmetic, and add some asserts to document the assumptions of the code.
+
+        * runtime/FunctionConstructor.cpp:
+        (JSC::constructFunctionSkippingEvalEnabledCheck):
+        - Fix OOM error handling which crashed a test after the new MaxLength was applied.
+        * runtime/JSString.h:
+        (JSC::JSString::finishCreation):
+        (JSC::JSString::createHasOtherOwner):
+        (JSC::JSString::setLength):
+        * runtime/JSStringInlines.h:
+        (JSC::jsMakeNontrivialString):
+        * runtime/Operations.h:
+        (JSC::jsString):
+
 2018-10-03  Koby Boyango  <koby.b@mce-sys.com>
 
         [JSC] Add a C++ callable overload of objectConstructorSeal
index 34fb118..265049f 100644 (file)
@@ -144,7 +144,11 @@ JSObject* constructFunctionSkippingEvalEnabledCheck(
         {
             // The spec mandates that the parameters parse as a valid parameter list
             // independent of the function body.
-            String program = makeString("(", prefix, "(", parameterBuilder.toString(), "){\n\n})");
+            String program = tryMakeString("(", prefix, "(", parameterBuilder.toString(), "){\n\n})");
+            if (UNLIKELY(!program)) {
+                throwOutOfMemoryError(exec, scope);
+                return nullptr;
+            }
             SourceCode source = makeSource(program, sourceOrigin, sourceURL, position);
             JSValue exception;
             checkSyntax(exec, source, &exception);
index efb3bb9..605f40a 100644 (file)
@@ -95,8 +95,12 @@ public:
         return &vm.stringSpace;
     }
     
-    static const unsigned MaxLength = std::numeric_limits<int32_t>::max();
-    
+    // We employ overflow checks in many places with the assumption that MaxLength
+    // is INT_MAX. Hence, it cannot be changed into another length value without
+    // breaking all the bounds and overflow checks that assume this.
+    static constexpr unsigned MaxLength = std::numeric_limits<int32_t>::max();
+    static_assert(MaxLength == String::MaxLength, "");
+
 private:
     JSString(VM& vm, Ref<StringImpl>&& value)
         : JSCell(vm, vm.stringStructure.get())
@@ -109,7 +113,7 @@ private:
     {
     }
 
-    void finishCreation(VM& vm, size_t length)
+    void finishCreation(VM& vm, unsigned length)
     {
         ASSERT(!m_value.isNull());
         Base::finishCreation(vm);
@@ -117,7 +121,7 @@ private:
         setIs8Bit(m_value.impl()->is8Bit());
     }
 
-    void finishCreation(VM& vm, size_t length, size_t cost)
+    void finishCreation(VM& vm, unsigned length, size_t cost)
     {
         ASSERT(!m_value.isNull());
         Base::finishCreation(vm);
@@ -145,7 +149,7 @@ public:
     }
     static JSString* createHasOtherOwner(VM& vm, Ref<StringImpl>&& value)
     {
-        size_t length = value->length();
+        unsigned length = value->length();
         JSString* newString = new (NotNull, allocateCell<JSString>(vm.heap)) JSString(vm, WTFMove(value));
         newString->finishCreation(vm, length);
         return newString;
@@ -209,6 +213,7 @@ protected:
 
     ALWAYS_INLINE void setLength(unsigned length)
     {
+        ASSERT(length <= MaxLength);
         m_length = length;
     }
 
@@ -255,10 +260,14 @@ public:
                 return false;
             if (m_index == JSRopeString::s_maxInternalRopeLength)
                 expand();
-            if (static_cast<int32_t>(m_jsString->length() + jsString->length()) < 0) {
+
+            static_assert(JSString::MaxLength == std::numeric_limits<int32_t>::max(), "");
+            auto sum = checkedSum<int32_t>(m_jsString->length(), jsString->length());
+            if (sum.hasOverflowed()) {
                 this->overflowed();
                 return false;
             }
+            ASSERT(static_cast<unsigned>(sum.unsafeGet()) <= MaxLength);
             m_jsString->append(m_vm, m_index++, jsString);
             return true;
         }
index ad02bd6..19d4756 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2016-2018 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -50,6 +50,7 @@ inline JSValue jsMakeNontrivialString(ExecState* exec, StringType&& string, Stri
     String result = tryMakeString(std::forward<StringType>(string), std::forward<StringTypes>(strings)...);
     if (UNLIKELY(!result))
         return throwOutOfMemoryError(exec, scope);
+    ASSERT(result.length() <= JSString::MaxLength);
     return jsNontrivialString(exec, WTFMove(result));
 }
 
index 6e87659..61d533c 100644 (file)
@@ -1,6 +1,6 @@
 /*
  *  Copyright (C) 1999-2000 Harri Porten (porten@kde.org)
- *  Copyright (C) 2002-2017 Apple Inc. All rights reserved.
+ *  Copyright (C) 2002-2018 Apple Inc. All rights reserved.
  *
  *  This library is free software; you can redistribute it and/or
  *  modify it under the terms of the GNU Library General Public
@@ -42,12 +42,13 @@ ALWAYS_INLINE JSString* jsString(ExecState* exec, JSString* s1, JSString* s2)
     VM& vm = exec->vm();
     auto scope = DECLARE_THROW_SCOPE(vm);
 
-    int32_t length1 = s1->length();
+    unsigned length1 = s1->length();
     if (!length1)
         return s2;
-    int32_t length2 = s2->length();
+    unsigned length2 = s2->length();
     if (!length2)
         return s1;
+    static_assert(JSString::MaxLength == std::numeric_limits<int32_t>::max(), "");
     if (sumOverflows<int32_t>(length1, length2)) {
         throwOutOfMemoryError(exec, scope);
         return nullptr;
@@ -61,19 +62,19 @@ ALWAYS_INLINE JSString* jsString(ExecState* exec, JSString* s1, JSString* s2, JS
     VM& vm = exec->vm();
     auto scope = DECLARE_THROW_SCOPE(vm);
 
-    int32_t length1 = s1->length();
+    unsigned length1 = s1->length();
     if (!length1)
         RELEASE_AND_RETURN(scope, jsString(exec, s2, s3));
 
-    int32_t length2 = s2->length();
+    unsigned length2 = s2->length();
     if (!length2)
         RELEASE_AND_RETURN(scope, jsString(exec, s1, s3));
 
-    int32_t length3 = s3->length();
+    unsigned length3 = s3->length();
     if (!length3)
         RELEASE_AND_RETURN(scope, jsString(exec, s1, s2));
 
-
+    static_assert(JSString::MaxLength == std::numeric_limits<int32_t>::max(), "");
     if (sumOverflows<int32_t>(length1, length2, length3)) {
         throwOutOfMemoryError(exec, scope);
         return nullptr;
@@ -86,15 +87,13 @@ ALWAYS_INLINE JSString* jsString(ExecState* exec, const String& u1, const String
     VM* vm = &exec->vm();
     auto scope = DECLARE_THROW_SCOPE(*vm);
 
-    int32_t length1 = u1.length();
-    int32_t length2 = u2.length();
-    int32_t length3 = u3.length();
-    
-    if (length1 < 0 || length2 < 0 || length3 < 0) {
-        throwOutOfMemoryError(exec, scope);
-        return nullptr;
-    }
-    
+    unsigned length1 = u1.length();
+    unsigned length2 = u2.length();
+    unsigned length3 = u3.length();
+    ASSERT(length1 <= JSString::MaxLength);
+    ASSERT(length2 <= JSString::MaxLength);
+    ASSERT(length3 <= JSString::MaxLength);
+
     if (!length1)
         RELEASE_AND_RETURN(scope, jsString(exec, jsString(vm, u2), jsString(vm, u3)));
 
@@ -104,6 +103,7 @@ ALWAYS_INLINE JSString* jsString(ExecState* exec, const String& u1, const String
     if (!length3)
         RELEASE_AND_RETURN(scope, jsString(exec, jsString(vm, u1), jsString(vm, u2)));
 
+    static_assert(JSString::MaxLength == std::numeric_limits<int32_t>::max(), "");
     if (sumOverflows<int32_t>(length1, length2, length3)) {
         throwOutOfMemoryError(exec, scope);
         return nullptr;
index 515df4d..c53affb 100644 (file)
@@ -1,3 +1,17 @@
+2018-10-03  Mark Lam  <mark.lam@apple.com>
+
+        Make string MaxLength for all WTF and JS strings consistently equal to INT_MAX.
+        https://bugs.webkit.org/show_bug.cgi?id=190187
+        <rdar://problem/42512909>
+
+        Reviewed by Michael Saboff.
+
+        * wtf/text/StringConcatenate.h:
+        (WTF::tryMakeStringFromAdapters):
+        (WTF::sumWithOverflow): Deleted.
+        * wtf/text/StringImpl.h:
+        * wtf/text/WTFString.h:
+
 2018-10-03  Michael Catanzaro  <mcatanzaro@igalia.com>
 
         -Wunused-variable in RenderLayer::updateScrollableAreaSet
index 18ff3f8..148262e 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2010-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2010-2018 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -27,6 +27,7 @@
 #define StringConcatenate_h
 
 #include <string.h>
+#include <wtf/CheckedArithmetic.h>
 
 #ifndef AtomicString_h
 #include <wtf/text/AtomicString.h>
@@ -141,9 +142,7 @@ public:
         while (m_characters[length])
             ++length;
 
-        if (length > std::numeric_limits<unsigned>::max()) // FIXME this is silly https://bugs.webkit.org/show_bug.cgi?id=165790
-            CRASH();
-
+        RELEASE_ASSERT(length <= String::MaxLength);
         m_length = length;
     }
 
@@ -259,24 +258,6 @@ public:
     }
 };
 
-inline void sumWithOverflow(bool& overflow, unsigned& total, unsigned addend)
-{
-    unsigned oldTotal = total;
-    total = oldTotal + addend;
-    if (total < oldTotal)
-        overflow = true;
-}
-
-template<typename... Unsigned>
-inline void sumWithOverflow(bool& overflow, unsigned& total, unsigned addend, Unsigned ...addends)
-{
-    unsigned oldTotal = total;
-    total = oldTotal + addend;
-    if (total < oldTotal)
-        overflow = true;
-    sumWithOverflow(overflow, total, addends...);
-}
-
 template<typename Adapter>
 inline bool are8Bit(Adapter adapter)
 {
@@ -305,12 +286,13 @@ inline void makeStringAccumulator(ResultType* result, Adapter adapter, Adapters
 template<typename StringTypeAdapter, typename... StringTypeAdapters>
 String tryMakeStringFromAdapters(StringTypeAdapter adapter, StringTypeAdapters ...adapters)
 {
-    bool overflow = false;
-    unsigned length = adapter.length();
-    sumWithOverflow(overflow, length, adapters.length()...);
-    if (overflow)
+    static_assert(String::MaxLength == std::numeric_limits<int32_t>::max(), "");
+    auto sum = checkedSum<int32_t>(adapter.length(), adapters.length()...);
+    if (sum.hasOverflowed())
         return String();
 
+    unsigned length = sum.unsafeGet();
+    ASSERT(length <= String::MaxLength);
     if (are8Bit(adapter, adapters...)) {
         LChar* buffer;
         RefPtr<StringImpl> resultImpl = StringImpl::tryCreateUninitialized(length, buffer);
index 1d2fef8..1f671c4 100644 (file)
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
- * Copyright (C) 2005-2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2005-2018 Apple Inc. All rights reserved.
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
@@ -130,6 +130,9 @@ struct StringStats {
 
 class StringImplShape {
     WTF_MAKE_NONCOPYABLE(StringImplShape);
+public:
+    static constexpr unsigned MaxLength = std::numeric_limits<int32_t>::max();
+
 protected:
     StringImplShape(unsigned refCount, unsigned length, const LChar*, unsigned hashAndFlags);
     StringImplShape(unsigned refCount, unsigned length, const UChar*, unsigned hashAndFlags);
@@ -180,6 +183,8 @@ class StringImpl : private StringImplShape {
 public:
     enum BufferOwnership { BufferInternal, BufferOwned, BufferSubstring, BufferExternal };
 
+    static constexpr unsigned MaxLength = StringImplShape::MaxLength;
+
     // The bottom 6 bits in the hash are flags.
     static constexpr const unsigned s_flagCount = 6;
 private:
index ddedefd..14c4f10 100644 (file)
@@ -365,6 +365,8 @@ public:
     // This is useful for clearing String-based caches.
     void clearImplIfNotShared();
 
+    static constexpr unsigned MaxLength = StringImpl::MaxLength;
+
 private:
     template<typename CharacterType> void removeInternal(const CharacterType*, unsigned, unsigned);