Changeset 236804 in webkit


Ignore:
Timestamp:
Oct 3, 2018 11:28:55 AM (6 years ago)
Author:
mark.lam@apple.com
Message:

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.

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:
Location:
trunk
Files:
1 added
10 edited

Legend:

Unmodified
Added
Removed

  • trunk/JSTests/ChangeLog

    r236737 r236804  
     12018-10-03  Mark Lam  <mark.lam@apple.com>
     2
     3        Make string MaxLength for all WTF and JS strings consistently equal to INT_MAX.
     4        https://bugs.webkit.org/show_bug.cgi?id=190187
     5        <rdar://problem/42512909>
     6
     7        Reviewed by Michael Saboff.
     8
     9        * stress/regress-190187.js: Added.
     10
    1112018-10-02  Caio Lima  <ticaiolima@gmail.com>
    212

  • trunk/Source/JavaScriptCore/ChangeLog

    r236791 r236804  
     12018-10-03  Mark Lam  <mark.lam@apple.com>
     2
     3        Make string MaxLength for all WTF and JS strings consistently equal to INT_MAX.
     4        https://bugs.webkit.org/show_bug.cgi?id=190187
     5        <rdar://problem/42512909>
     6
     7        Reviewed by Michael Saboff.
     8
     9        Allowing different max string lengths at each level opens up opportunities for
     10        bugs to creep in.  With 2 different max length values, it is more difficult to
     11        keep the story straight on how we do overflow / bounds checks at each place in
     12        the code.  It's also difficult to tell if a seemingly valid check at the WTF level
     13        will have bad ramifications at the JSC level.  Also, it's also not meaningful to
     14        support a max length > INT_MAX.  To eliminate this class of bugs, we'll
     15        standardize on a MaxLength of INT_MAX at all levels.
     16
     17        We'll also standardize the way we do length overflow checks on using
     18        CheckedArithmetic, and add some asserts to document the assumptions of the code.
     19
     20        * runtime/FunctionConstructor.cpp:
     21        (JSC::constructFunctionSkippingEvalEnabledCheck):
     22        - Fix OOM error handling which crashed a test after the new MaxLength was applied.
     23        * runtime/JSString.h:
     24        (JSC::JSString::finishCreation):
     25        (JSC::JSString::createHasOtherOwner):
     26        (JSC::JSString::setLength):
     27        * runtime/JSStringInlines.h:
     28        (JSC::jsMakeNontrivialString):
     29        * runtime/Operations.h:
     30        (JSC::jsString):
     31
    1322018-10-03  Koby Boyango  <koby.b@mce-sys.com>
    233

  • trunk/Source/JavaScriptCore/runtime/FunctionConstructor.cpp

    r236697 r236804  
    145145            // The spec mandates that the parameters parse as a valid parameter list
    146146            // independent of the function body.
    147             String program = makeString("(", prefix, "(", parameterBuilder.toString(), "){\n\n})");
     147            String program = tryMakeString("(", prefix, "(", parameterBuilder.toString(), "){\n\n})");
     148            if (UNLIKELY(!program)) {
     149                throwOutOfMemoryError(exec, scope);
     150                return nullptr;
     151            }
    148152            SourceCode source = makeSource(program, sourceOrigin, sourceURL, position);
    149153            JSValue exception;

  • trunk/Source/JavaScriptCore/runtime/JSString.h

    r236369 r236804  
    9696    }
    9797   
    98     static const unsigned MaxLength = std::numeric_limits<int32_t>::max();
    99    
     98    // We employ overflow checks in many places with the assumption that MaxLength
     99    // is INT_MAX. Hence, it cannot be changed into another length value without
     100    // breaking all the bounds and overflow checks that assume this.
     101    static constexpr unsigned MaxLength = std::numeric_limits<int32_t>::max();
     102    static_assert(MaxLength == String::MaxLength, "");
     103
    100104private:
    101105    JSString(VM& vm, Ref<StringImpl>&& value)
    … …  
    110114    }
    111115
    112     void finishCreation(VM& vm, size_t length)
     116    void finishCreation(VM& vm, unsigned length)
    113117    {
    114118        ASSERT(!m_value.isNull());
    … …  
    118122    }
    119123
    120     void finishCreation(VM& vm, size_t length, size_t cost)
     124    void finishCreation(VM& vm, unsigned length, size_t cost)
    121125    {
    122126        ASSERT(!m_value.isNull());
    … …  
    146150    static JSString* createHasOtherOwner(VM& vm, Ref<StringImpl>&& value)
    147151    {
    148         size_t length = value->length();
     152        unsigned length = value->length();
    149153        JSString* newString = new (NotNull, allocateCell<JSString>(vm.heap)) JSString(vm, WTFMove(value));
    150154        newString->finishCreation(vm, length);
    … …  
    210214    ALWAYS_INLINE void setLength(unsigned length)
    211215    {
     216        ASSERT(length <= MaxLength);
    212217        m_length = length;
    213218    }
    … …  
    256261            if (m_index == JSRopeString::s_maxInternalRopeLength)
    257262                expand();
    258             if (static_cast<int32_t>(m_jsString->length() + jsString->length()) < 0) {
     263
     264            static_assert(JSString::MaxLength == std::numeric_limits<int32_t>::max(), "");
     265            auto sum = checkedSum<int32_t>(m_jsString->length(), jsString->length());
     266            if (sum.hasOverflowed()) {
    259267                this->overflowed();
    260268                return false;
    261269            }
     270            ASSERT(static_cast<unsigned>(sum.unsafeGet()) <= MaxLength);
    262271            m_jsString->append(m_vm, m_index++, jsString);
    263272            return true;

  • trunk/Source/JavaScriptCore/runtime/JSStringInlines.h

    r225150 r236804  
    11/*
    2  * Copyright (C) 2016 Apple Inc. All rights reserved.
     2 * Copyright (C) 2016-2018 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    5151    if (UNLIKELY(!result))
    5252        return throwOutOfMemoryError(exec, scope);
     53    ASSERT(result.length() <= JSString::MaxLength);
    5354    return jsNontrivialString(exec, WTFMove(result));
    5455}

  • trunk/Source/JavaScriptCore/runtime/Operations.h

    r236697 r236804  
    11/*
    22 *  Copyright (C) 1999-2000 Harri Porten (porten@kde.org)
    3  *  Copyright (C) 2002-2017 Apple Inc. All rights reserved.
     3 *  Copyright (C) 2002-2018 Apple Inc. All rights reserved.
    44 *
    55 *  This library is free software; you can redistribute it and/or
    … …  
    4343    auto scope = DECLARE_THROW_SCOPE(vm);
    4444
    45     int32_t length1 = s1->length();
     45    unsigned length1 = s1->length();
    4646    if (!length1)
    4747        return s2;
    48     int32_t length2 = s2->length();
     48    unsigned length2 = s2->length();
    4949    if (!length2)
    5050        return s1;
     51    static_assert(JSString::MaxLength == std::numeric_limits<int32_t>::max(), "");
    5152    if (sumOverflows<int32_t>(length1, length2)) {
    5253        throwOutOfMemoryError(exec, scope);
    … …  
    6263    auto scope = DECLARE_THROW_SCOPE(vm);
    6364
    64     int32_t length1 = s1->length();
     65    unsigned length1 = s1->length();
    6566    if (!length1)
    6667        RELEASE_AND_RETURN(scope, jsString(exec, s2, s3));
    6768
    68     int32_t length2 = s2->length();
     69    unsigned length2 = s2->length();
    6970    if (!length2)
    7071        RELEASE_AND_RETURN(scope, jsString(exec, s1, s3));
    7172
    72     int32_t length3 = s3->length();
     73    unsigned length3 = s3->length();
    7374    if (!length3)
    7475        RELEASE_AND_RETURN(scope, jsString(exec, s1, s2));
    7576
    76 
     77    static_assert(JSString::MaxLength == std::numeric_limits<int32_t>::max(), "");
    7778    if (sumOverflows<int32_t>(length1, length2, length3)) {
    7879        throwOutOfMemoryError(exec, scope);
    … …  
    8788    auto scope = DECLARE_THROW_SCOPE(*vm);
    8889
    89     int32_t length1 = u1.length();
    90     int32_t length2 = u2.length();
    91     int32_t length3 = u3.length();
    92    
    93     if (length1 < 0 || length2 < 0 || length3 < 0) {
    94         throwOutOfMemoryError(exec, scope);
    95         return nullptr;
    96     }
    97    
     90    unsigned length1 = u1.length();
     91    unsigned length2 = u2.length();
     92    unsigned length3 = u3.length();
     93    ASSERT(length1 <= JSString::MaxLength);
     94    ASSERT(length2 <= JSString::MaxLength);
     95    ASSERT(length3 <= JSString::MaxLength);
     96
    9897    if (!length1)
    9998        RELEASE_AND_RETURN(scope, jsString(exec, jsString(vm, u2), jsString(vm, u3)));
    … …  
    105104        RELEASE_AND_RETURN(scope, jsString(exec, jsString(vm, u1), jsString(vm, u2)));
    106105
     106    static_assert(JSString::MaxLength == std::numeric_limits<int32_t>::max(), "");
    107107    if (sumOverflows<int32_t>(length1, length2, length3)) {
    108108        throwOutOfMemoryError(exec, scope);

  • trunk/Source/WTF/ChangeLog

    r236790 r236804  
     12018-10-03  Mark Lam  <mark.lam@apple.com>
     2
     3        Make string MaxLength for all WTF and JS strings consistently equal to INT_MAX.
     4        https://bugs.webkit.org/show_bug.cgi?id=190187
     5        <rdar://problem/42512909>
     6
     7        Reviewed by Michael Saboff.
     8
     9        * wtf/text/StringConcatenate.h:
     10        (WTF::tryMakeStringFromAdapters):
     11        (WTF::sumWithOverflow): Deleted.
     12        * wtf/text/StringImpl.h:
     13        * wtf/text/WTFString.h:
     14
    1152018-10-03  Michael Catanzaro  <mcatanzaro@igalia.com>
    216

  • trunk/Source/WTF/wtf/text/StringConcatenate.h

    r228576 r236804  
    11/*
    2  * Copyright (C) 2010-2016 Apple Inc. All rights reserved.
     2 * Copyright (C) 2010-2018 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    2828
    2929#include <string.h>
     30#include <wtf/CheckedArithmetic.h>
    3031
    3132#ifndef AtomicString_h
    … …  
    142143            ++length;
    143144
    144         if (length > std::numeric_limits<unsigned>::max()) // FIXME this is silly https://bugs.webkit.org/show_bug.cgi?id=165790
    145             CRASH();
    146 
     145        RELEASE_ASSERT(length <= String::MaxLength);
    147146        m_length = length;
    148147    }
    … …  
    260259};
    261260
    262 inline void sumWithOverflow(bool& overflow, unsigned& total, unsigned addend)
    263 {
    264     unsigned oldTotal = total;
    265     total = oldTotal + addend;
    266     if (total < oldTotal)
    267         overflow = true;
    268 }
    269 
    270 template<typename... Unsigned>
    271 inline void sumWithOverflow(bool& overflow, unsigned& total, unsigned addend, Unsigned ...addends)
    272 {
    273     unsigned oldTotal = total;
    274     total = oldTotal + addend;
    275     if (total < oldTotal)
    276         overflow = true;
    277     sumWithOverflow(overflow, total, addends...);
    278 }
    279 
    280261template<typename Adapter>
    281262inline bool are8Bit(Adapter adapter)
    … …  
    306287String tryMakeStringFromAdapters(StringTypeAdapter adapter, StringTypeAdapters ...adapters)
    307288{
    308     bool overflow = false;
    309     unsigned length = adapter.length();
    310     sumWithOverflow(overflow, length, adapters.length()...);
    311     if (overflow)
     289    static_assert(String::MaxLength == std::numeric_limits<int32_t>::max(), "");
     290    auto sum = checkedSum<int32_t>(adapter.length(), adapters.length()...);
     291    if (sum.hasOverflowed())
    312292        return String();
    313293
     294    unsigned length = sum.unsafeGet();
     295    ASSERT(length <= String::MaxLength);
    314296    if (are8Bit(adapter, adapters...)) {
    315297        LChar* buffer;

  • trunk/Source/WTF/wtf/text/StringImpl.h

    r236599 r236804  
    11/*
    22 * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
    3  * Copyright (C) 2005-2017 Apple Inc. All rights reserved.
     3 * Copyright (C) 2005-2018 Apple Inc. All rights reserved.
    44 * Copyright (C) 2009 Google Inc. All rights reserved.
    55 *
    … …  
    131131class StringImplShape {
    132132    WTF_MAKE_NONCOPYABLE(StringImplShape);
     133public:
     134    static constexpr unsigned MaxLength = std::numeric_limits<int32_t>::max();
     135
    133136protected:
    134137    StringImplShape(unsigned refCount, unsigned length, const LChar*, unsigned hashAndFlags);
    … …  
    180183public:
    181184    enum BufferOwnership { BufferInternal, BufferOwned, BufferSubstring, BufferExternal };
     185
     186    static constexpr unsigned MaxLength = StringImplShape::MaxLength;
    182187
    183188    // The bottom 6 bits in the hash are flags.

  • trunk/Source/WTF/wtf/text/WTFString.h

    r235721 r236804  
    365365    // This is useful for clearing String-based caches.
    366366    void clearImplIfNotShared();
     367
     368    static constexpr unsigned MaxLength = StringImpl::MaxLength;
    367369
    368370private:
Note: See TracChangeset for help on using the changeset viewer.