Attachment 362775 Details for Bug 194962 – proposed patch.

[patch] proposed patch.

bug-194962.patch (text/plain), 13.34 KB, created by Mark Lam on 2019-02-22 16:01:36 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Mark Lam

Created: 2019-02-22 16:01:36 PST

Size: 13.34 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 241967)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,16 @@
>+2019-02-22  Mark Lam  <mark.lam@apple.com>
>+
>+        Add an exception check and some assertions in StringPrototype.cpp.
>+        https://bugs.webkit.org/show_bug.cgi?id=194962
>+        <rdar://problem/48013416>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * runtime/StringPrototype.cpp:
>+        (JSC::jsSpliceSubstrings):
>+        (JSC::jsSpliceSubstringsWithSeparators):
>+        (JSC::operationStringProtoFuncReplaceRegExpEmptyStr):
>+
> 2019-02-22  Robin Morisset  <rmorisset@apple.com>
> 
>         B3ReduceStrength: missing peephole optimizations for binary operations
>Index: Source/JavaScriptCore/runtime/StringPrototype.cpp
>===================================================================
>--- Source/JavaScriptCore/runtime/StringPrototype.cpp	(revision 241963)
>+++ Source/JavaScriptCore/runtime/StringPrototype.cpp	(working copy)
>@@ -1,6 +1,6 @@
> /*
>  *  Copyright (C) 1999-2001 Harri Porten (porten@kde.org)
>- *  Copyright (C) 2004-2017 Apple Inc. All rights reserved.
>+ *  Copyright (C) 2004-2019 Apple Inc. All rights reserved.
>  *  Copyright (C) 2009 Torch Mobile, Inc.
>  *  Copyright (C) 2015 Jordan Harband (ljharb@gmail.com)
>  *
>@@ -324,9 +324,14 @@ static ALWAYS_INLINE JSString* jsSpliceS
>         RELEASE_AND_RETURN(scope, jsString(exec, StringImpl::createSubstringSharingImpl(*source.impl(), std::max(0, position), std::min(sourceSize, length))));
>     }
> 
>-    int totalLength = 0;
>+    // We know that the sum of substringRanges lengths cannot exceed length of
>+    // source because the substringRanges were computed from the source string
>+    // in removeUsingRegExpSearch(). Hence, totalLength cannot exceed
>+    // String::MaxLength, and therefore, cannot overflow.
>+    Checked<int, AssertNoOverflow> totalLength = 0;
>     for (int i = 0; i < rangeCount; i++)
>         totalLength += substringRanges[i].length;
>+    ASSERT(totalLength <= String::MaxLength);
> 
>     if (!totalLength)
>         return jsEmptyString(exec);
>@@ -334,16 +339,16 @@ static ALWAYS_INLINE JSString* jsSpliceS
>     if (source.is8Bit()) {
>         LChar* buffer;
>         const LChar* sourceData = source.characters8();
>-        auto impl = StringImpl::tryCreateUninitialized(totalLength, buffer);
>+        auto impl = StringImpl::tryCreateUninitialized(totalLength.unsafeGet(), buffer);
>         if (!impl) {
>             throwOutOfMemoryError(exec, scope);
>             return nullptr;
>         }
> 
>-        int bufferPos = 0;
>+        Checked<int, AssertNoOverflow> bufferPos = 0;
>         for (int i = 0; i < rangeCount; i++) {
>             if (int srcLen = substringRanges[i].length) {
>-                StringImpl::copyCharacters(buffer + bufferPos, sourceData + substringRanges[i].position, srcLen);
>+                StringImpl::copyCharacters(buffer + bufferPos.unsafeGet(), sourceData + substringRanges[i].position, srcLen);
>                 bufferPos += srcLen;
>             }
>         }
>@@ -354,16 +359,16 @@ static ALWAYS_INLINE JSString* jsSpliceS
>     UChar* buffer;
>     const UChar* sourceData = source.characters16();
> 
>-    auto impl = StringImpl::tryCreateUninitialized(totalLength, buffer);
>+    auto impl = StringImpl::tryCreateUninitialized(totalLength.unsafeGet(), buffer);
>     if (!impl) {
>         throwOutOfMemoryError(exec, scope);
>         return nullptr;
>     }
> 
>-    int bufferPos = 0;
>+    Checked<int, AssertNoOverflow> bufferPos = 0;
>     for (int i = 0; i < rangeCount; i++) {
>         if (int srcLen = substringRanges[i].length) {
>-            StringImpl::copyCharacters(buffer + bufferPos, sourceData + substringRanges[i].position, srcLen);
>+            StringImpl::copyCharacters(buffer + bufferPos.unsafeGet(), sourceData + substringRanges[i].position, srcLen);
>             bufferPos += srcLen;
>         }
>     }
>@@ -414,17 +419,17 @@ static ALWAYS_INLINE JSString* jsSpliceS
>         }
> 
>         int maxCount = std::max(rangeCount, separatorCount);
>-        int bufferPos = 0;
>+        Checked<int, AssertNoOverflow> bufferPos = 0;
>         for (int i = 0; i < maxCount; i++) {
>             if (i < rangeCount) {
>                 if (int srcLen = substringRanges[i].length) {
>-                    StringImpl::copyCharacters(buffer + bufferPos, sourceData + substringRanges[i].position, srcLen);
>+                    StringImpl::copyCharacters(buffer + bufferPos.unsafeGet(), sourceData + substringRanges[i].position, srcLen);
>                     bufferPos += srcLen;
>                 }
>             }
>             if (i < separatorCount) {
>                 if (int sepLen = separators[i].length()) {
>-                    StringImpl::copyCharacters(buffer + bufferPos, separators[i].characters8(), sepLen);
>+                    StringImpl::copyCharacters(buffer + bufferPos.unsafeGet(), separators[i].characters8(), sepLen);
>                     bufferPos += sepLen;
>                 }
>             }
>@@ -441,23 +446,23 @@ static ALWAYS_INLINE JSString* jsSpliceS
>     }
> 
>     int maxCount = std::max(rangeCount, separatorCount);
>-    int bufferPos = 0;
>+    Checked<int, AssertNoOverflow> bufferPos = 0;
>     for (int i = 0; i < maxCount; i++) {
>         if (i < rangeCount) {
>             if (int srcLen = substringRanges[i].length) {
>                 if (source.is8Bit())
>-                    StringImpl::copyCharacters(buffer + bufferPos, source.characters8() + substringRanges[i].position, srcLen);
>+                    StringImpl::copyCharacters(buffer + bufferPos.unsafeGet(), source.characters8() + substringRanges[i].position, srcLen);
>                 else
>-                    StringImpl::copyCharacters(buffer + bufferPos, source.characters16() + substringRanges[i].position, srcLen);
>+                    StringImpl::copyCharacters(buffer + bufferPos.unsafeGet(), source.characters16() + substringRanges[i].position, srcLen);
>                 bufferPos += srcLen;
>             }
>         }
>         if (i < separatorCount) {
>             if (int sepLen = separators[i].length()) {
>                 if (separators[i].is8Bit())
>-                    StringImpl::copyCharacters(buffer + bufferPos, separators[i].characters8(), sepLen);
>+                    StringImpl::copyCharacters(buffer + bufferPos.unsafeGet(), separators[i].characters8(), sepLen);
>                 else
>-                    StringImpl::copyCharacters(buffer + bufferPos, separators[i].characters16(), sepLen);
>+                    StringImpl::copyCharacters(buffer + bufferPos.unsafeGet(), separators[i].characters16(), sepLen);
>                 bufferPos += sepLen;
>             }
>         }
>@@ -729,7 +734,9 @@ JSCell* JIT_OPERATION operationStringPro
>         // ES5.1 15.5.4.10 step 8.a.
>         searchValue->setLastIndex(exec, 0);
>         RETURN_IF_EXCEPTION(scope, nullptr);
>-        RELEASE_AND_RETURN(scope, removeUsingRegExpSearch(vm, exec, thisValue, thisValue->value(exec), regExp));
>+        const String& source = thisValue->value(exec);
>+        RETURN_IF_EXCEPTION(scope, nullptr);
>+        RELEASE_AND_RETURN(scope, removeUsingRegExpSearch(vm, exec, thisValue, source, regExp));
>     }
> 
>     CallData callData;
>Index: Source/WTF/ChangeLog
>===================================================================
>--- Source/WTF/ChangeLog	(revision 241963)
>+++ Source/WTF/ChangeLog	(working copy)
>@@ -1,3 +1,32 @@
>+2019-02-22  Mark Lam  <mark.lam@apple.com>
>+
>+        Add an exception check and some assertions in StringPrototype.cpp.
>+        https://bugs.webkit.org/show_bug.cgi?id=194962
>+        <rdar://problem/48013416>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add an AssertNoOverflow overflow handler which allows us to do CheckedArithmetic
>+        for assertion purpose only on debug builds but sacrifices no performance on
>+        release builds.
>+
>+        * wtf/CheckedArithmetic.h:
>+        (WTF::AssertNoOverflow::overflowed):
>+        (WTF::AssertNoOverflow::clearOverflow):
>+        (WTF::AssertNoOverflow::crash):
>+        (WTF::AssertNoOverflow::hasOverflowed const):
>+        (WTF::observesOverflows):
>+        (WTF::observesOverflows<AssertNoOverflow>):
>+        (WTF::safeAdd):
>+        (WTF::safeSub):
>+        (WTF::safeMultiply):
>+        (WTF::Checked::operator+=):
>+        (WTF::Checked::operator-=):
>+        (WTF::Checked::operator*=):
>+        (WTF::operator+):
>+        (WTF::operator-):
>+        (WTF::operator*):
>+
> 2019-02-21  Antoine Quint  <graouts@apple.com>
> 
>         Move UIWebTouchEventsGestureRecognizer.activeTouchesByIdentifier to SPI
>Index: Source/WTF/wtf/CheckedArithmetic.h
>===================================================================
>--- Source/WTF/wtf/CheckedArithmetic.h	(revision 241963)
>+++ Source/WTF/wtf/CheckedArithmetic.h	(working copy)
>@@ -1,5 +1,5 @@
> /*
>- * Copyright (C) 2011-2018 Apple Inc. All rights reserved.
>+ * Copyright (C) 2011-2019 Apple Inc. All rights reserved.
>  *
>  * Redistribution and use in source and binary forms, with or without
>  * modification, are permitted provided that the following conditions
>@@ -70,6 +70,24 @@ enum class CheckedState {
>     DidNotOverflow
> };
> 
>+class AssertNoOverflow {
>+public:
>+    static NO_RETURN_DUE_TO_ASSERT void overflowed()
>+    {
>+        ASSERT_NOT_REACHED();
>+    }
>+
>+    void clearOverflow() { }
>+
>+    static NO_RETURN_DUE_TO_CRASH void crash()
>+    {
>+        CRASH();
>+    }
>+
>+public:
>+    constexpr bool hasOverflowed() const { return false; }
>+};
>+
> class ConditionalCrashOnOverflow {
> public:
>     void overflowed()
>@@ -523,9 +541,25 @@ template <typename ResultType> struct Ar
>     }
> };
> 
>+template <class OverflowHandler>
>+inline bool observesOverflows() { return true; }
>+
>+template <>
>+inline bool observesOverflows<AssertNoOverflow>() { return !ASSERT_DISABLED; }
>+
> template <typename U, typename V, typename R> static inline bool safeAdd(U lhs, V rhs, R& result)
> {
>     return ArithmeticOperations<U, V, R>::add(lhs, rhs, result);
>+    return true;
>+}
>+
>+template <class OverflowHandler, typename U, typename V, typename R>
>+static inline bool safeAdd(U lhs, V rhs, R& result)
>+{
>+    if (observesOverflows<OverflowHandler>())
>+        return safeAdd(lhs, rhs, result);
>+    result = lhs + rhs;
>+    return true;
> }
> 
> template <typename U, typename V, typename R> static inline bool safeSub(U lhs, V rhs, R& result)
>@@ -533,11 +567,29 @@ template <typename U, typename V, typena
>     return ArithmeticOperations<U, V, R>::sub(lhs, rhs, result);
> }
> 
>+template <class OverflowHandler, typename U, typename V, typename R>
>+static inline bool safeSub(U lhs, V rhs, R& result)
>+{
>+    if (observesOverflows<OverflowHandler>())
>+        return safeSub(lhs, rhs, result);
>+    result = lhs - rhs;
>+    return true;
>+}
>+
> template <typename U, typename V, typename R> static inline bool safeMultiply(U lhs, V rhs, R& result)
> {
>     return ArithmeticOperations<U, V, R>::multiply(lhs, rhs, result);
> }
> 
>+template <class OverflowHandler, typename U, typename V, typename R>
>+static inline bool safeMultiply(U lhs, V rhs, R& result)
>+{
>+    if (observesOverflows<OverflowHandler>())
>+        return safeMultiply(lhs, rhs, result);
>+    result = lhs * rhs;
>+    return true;
>+}
>+
> template <typename U, typename V> static inline bool safeEquals(U lhs, V rhs)
> {
>     return ArithmeticOperations<U, V>::equals(lhs, rhs);
>@@ -676,21 +728,21 @@ public:
>     // Mutating assignment
>     template <typename U> const Checked operator+=(U rhs)
>     {
>-        if (!safeAdd(m_value, rhs, m_value))
>+        if (!safeAdd<OverflowHandler>(m_value, rhs, m_value))
>             this->overflowed();
>         return *this;
>     }
> 
>     template <typename U> const Checked operator-=(U rhs)
>     {
>-        if (!safeSub(m_value, rhs, m_value))
>+        if (!safeSub<OverflowHandler>(m_value, rhs, m_value))
>             this->overflowed();
>         return *this;
>     }
> 
>     template <typename U> const Checked operator*=(U rhs)
>     {
>-        if (!safeMultiply(m_value, rhs, m_value))
>+        if (!safeMultiply<OverflowHandler>(m_value, rhs, m_value))
>             this->overflowed();
>         return *this;
>     }
>@@ -814,7 +866,7 @@ template <typename U, typename V, typena
>     V y = 0;
>     bool overflowed = lhs.safeGet(x) == CheckedState::DidOverflow || rhs.safeGet(y) == CheckedState::DidOverflow;
>     typename Result<U, V>::ResultType result = 0;
>-    overflowed |= !safeAdd(x, y, result);
>+    overflowed |= !safeAdd<OverflowHandler>(x, y, result);
>     if (overflowed)
>         return ResultOverflowed;
>     return result;
>@@ -826,7 +878,7 @@ template <typename U, typename V, typena
>     V y = 0;
>     bool overflowed = lhs.safeGet(x) == CheckedState::DidOverflow || rhs.safeGet(y) == CheckedState::DidOverflow;
>     typename Result<U, V>::ResultType result = 0;
>-    overflowed |= !safeSub(x, y, result);
>+    overflowed |= !safeSub<OverflowHandler>(x, y, result);
>     if (overflowed)
>         return ResultOverflowed;
>     return result;
>@@ -838,7 +890,7 @@ template <typename U, typename V, typena
>     V y = 0;
>     bool overflowed = lhs.safeGet(x) == CheckedState::DidOverflow || rhs.safeGet(y) == CheckedState::DidOverflow;
>     typename Result<U, V>::ResultType result = 0;
>-    overflowed |= !safeMultiply(x, y, result);
>+    overflowed |= !safeMultiply<OverflowHandler>(x, y, result);
>     if (overflowed)
>         return ResultOverflowed;
>     return result;
>@@ -930,6 +982,7 @@ template<typename T, typename... Args> b
> 
> }
> 
>+using WTF::AssertNoOverflow;
> using WTF::Checked;
> using WTF::CheckedState;
> using WTF::CheckedInt8;

Flags:

ysuzuki: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 194962: 362775 | 362784 | 362790 | 362800 | 362805 | 362807