Changeset 207849 in webkit


Ignore:
Timestamp:
Oct 25, 2016 3:19:37 PM (8 years ago)
Author:
mark.lam@apple.com
Message:

JSStringJoiner::joinedLength() should limit joined string lengths to INT_MAX.
​https://bugs.webkit.org/show_bug.cgi?id=163937
<rdar://problem/28642990>

Reviewed by Geoffrey Garen.

JSTests:

  • stress/joined-strings-should-not-exceed-max-string-length.js: Added.

Source/JavaScriptCore:

JSStringJoiner::joinedLength() was previously limiting it to UINT_MAX. This is
inconsistent with other parts of string code which expects a max length of INT_MAX.
This is now fixed.

Also fixed jsMakeNontrivialString() to ensure that the resultant string length
is valid. It was previously allowing lengths greater than INT_MAX. This was
caught by the new assertion in JSString::setLength().

There are already pre-existing assertions in various JSString::finishCreation()
which do RELEASE_ASSERTs on the string length. To be consistent, I'm making the
assertion in JSString::setLength() a RELEASE_ASSERT. If this proves to be a
performance issue, I'll change this to a debug ASSERT later.

  • runtime/JSString.cpp:

(JSC::JSRopeString::resolveRopeInternal8):
(JSC::JSRopeString::resolveRopeInternal8NoSubstring):
(JSC::JSRopeString::resolveRopeInternal16):
(JSC::JSRopeString::resolveRopeInternal16NoSubstring):
(JSC::JSRopeString::resolveRopeToAtomicString):
(JSC::JSRopeString::resolveRopeToExistingAtomicString):
(JSC::JSRopeString::resolveRope):
(JSC::JSRopeString::resolveRopeSlowCase8):
(JSC::JSRopeString::resolveRopeSlowCase):
(JSC::JSString::getStringPropertyDescriptor):

  • runtime/JSString.h:

(JSC::JSString::finishCreation):
(JSC::JSString::length):
(JSC::JSString::isValidLength):
(JSC::JSString::toBoolean):
(JSC::JSString::canGetIndex):
(JSC::JSString::setLength):
(JSC::JSString::getStringPropertySlot):
(JSC::JSRopeString::unsafeView):
(JSC::JSRopeString::viewWithUnderlyingString):

  • runtime/JSStringBuilder.h:

(JSC::jsMakeNontrivialString):

  • runtime/JSStringJoiner.cpp:

(JSC::JSStringJoiner::joinedLength):

Location:
trunk
Files:
1 added
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/JSTests/ChangeLog

    r207846 r207849  
     12016-10-25  Mark Lam  <mark.lam@apple.com>
     2
     3        JSStringJoiner::joinedLength() should limit joined string lengths to INT_MAX.
     4        https://bugs.webkit.org/show_bug.cgi?id=163937
     5        <rdar://problem/28642990>
     6
     7        Reviewed by Geoffrey Garen.
     8
     9        * stress/joined-strings-should-not-exceed-max-string-length.js: Added.
     10
    1112016-10-25  JF Bastien  <jfbastien@apple.com>
    212

  • trunk/Source/JavaScriptCore/ChangeLog

    r207843 r207849  
     12016-10-25  Mark Lam  <mark.lam@apple.com>
     2
     3        JSStringJoiner::joinedLength() should limit joined string lengths to INT_MAX.
     4        https://bugs.webkit.org/show_bug.cgi?id=163937
     5        <rdar://problem/28642990>
     6
     7        Reviewed by Geoffrey Garen.
     8
     9        JSStringJoiner::joinedLength() was previously limiting it to UINT_MAX.  This is
     10        inconsistent with other parts of string code which expects a max length of INT_MAX.
     11        This is now fixed.
     12
     13        Also fixed jsMakeNontrivialString() to ensure that the resultant string length
     14        is valid.  It was previously allowing lengths greater than INT_MAX.  This was
     15        caught by the new assertion in JSString::setLength().
     16
     17        There are already pre-existing assertions in various JSString::finishCreation()
     18        which do RELEASE_ASSERTs on the string length.  To be consistent, I'm making the
     19        assertion in JSString::setLength() a RELEASE_ASSERT.  If this proves to be a
     20        performance issue, I'll change this to a debug ASSERT later.
     21
     22        * runtime/JSString.cpp:
     23        (JSC::JSRopeString::resolveRopeInternal8):
     24        (JSC::JSRopeString::resolveRopeInternal8NoSubstring):
     25        (JSC::JSRopeString::resolveRopeInternal16):
     26        (JSC::JSRopeString::resolveRopeInternal16NoSubstring):
     27        (JSC::JSRopeString::resolveRopeToAtomicString):
     28        (JSC::JSRopeString::resolveRopeToExistingAtomicString):
     29        (JSC::JSRopeString::resolveRope):
     30        (JSC::JSRopeString::resolveRopeSlowCase8):
     31        (JSC::JSRopeString::resolveRopeSlowCase):
     32        (JSC::JSString::getStringPropertyDescriptor):
     33        * runtime/JSString.h:
     34        (JSC::JSString::finishCreation):
     35        (JSC::JSString::length):
     36        (JSC::JSString::isValidLength):
     37        (JSC::JSString::toBoolean):
     38        (JSC::JSString::canGetIndex):
     39        (JSC::JSString::setLength):
     40        (JSC::JSString::getStringPropertySlot):
     41        (JSC::JSRopeString::unsafeView):
     42        (JSC::JSRopeString::viewWithUnderlyingString):
     43        * runtime/JSStringBuilder.h:
     44        (JSC::jsMakeNontrivialString):
     45        * runtime/JSStringJoiner.cpp:
     46        (JSC::JSStringJoiner::joinedLength):
     47
    1482016-10-25  JF Bastien  <jfbastien@apple.com>
    249

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

    r206386 r207849  
    121121    if (isSubstring()) {
    122122        StringImpl::copyChars(
    123             buffer, substringBase()->m_value.characters8() + substringOffset(), m_length);
     123            buffer, substringBase()->m_value.characters8() + substringOffset(), length());
    124124        return;
    125125    }
    … …  
    144144        position += length;
    145145    }
    146     ASSERT((buffer + m_length) == position);
     146    ASSERT((buffer + length()) == position);
    147147}
    148148
    … …  
    151151    if (isSubstring()) {
    152152        StringImpl::copyChars(
    153             buffer, substringBase()->m_value.characters16() + substringOffset(), m_length);
     153            buffer, substringBase()->m_value.characters16() + substringOffset(), length());
    154154        return;
    155155    }
    … …  
    177177        position += length;
    178178    }
    179     ASSERT((buffer + m_length) == position);
     179    ASSERT((buffer + length()) == position);
    180180}
    181181
    182182void JSRopeString::resolveRopeToAtomicString(ExecState* exec) const
    183183{
    184     if (m_length > maxLengthForOnStackResolve) {
     184    if (length() > maxLengthForOnStackResolve) {
    185185        resolveRope(exec);
    186186        m_value = AtomicString(m_value);
    … …  
    192192        LChar buffer[maxLengthForOnStackResolve];
    193193        resolveRopeInternal8(buffer);
    194         m_value = AtomicString(buffer, m_length);
     194        m_value = AtomicString(buffer, length());
    195195        setIs8Bit(m_value.impl()->is8Bit());
    196196    } else {
    197197        UChar buffer[maxLengthForOnStackResolve];
    198198        resolveRopeInternal16(buffer);
    199         m_value = AtomicString(buffer, m_length);
     199        m_value = AtomicString(buffer, length());
    200200        setIs8Bit(m_value.impl()->is8Bit());
    201201    }
    … …  
    216216RefPtr<AtomicStringImpl> JSRopeString::resolveRopeToExistingAtomicString(ExecState* exec) const
    217217{
    218     if (m_length > maxLengthForOnStackResolve) {
     218    if (length() > maxLengthForOnStackResolve) {
    219219        resolveRope(exec);
    220220        if (RefPtr<AtomicStringImpl> existingAtomicString = AtomicStringImpl::lookUp(m_value.impl())) {
    … …  
    230230        LChar buffer[maxLengthForOnStackResolve];
    231231        resolveRopeInternal8(buffer);
    232         if (RefPtr<AtomicStringImpl> existingAtomicString = AtomicStringImpl::lookUp(buffer, m_length)) {
     232        if (RefPtr<AtomicStringImpl> existingAtomicString = AtomicStringImpl::lookUp(buffer, length())) {
    233233            m_value = *existingAtomicString;
    234234            setIs8Bit(m_value.impl()->is8Bit());
    … …  
    239239        UChar buffer[maxLengthForOnStackResolve];
    240240        resolveRopeInternal16(buffer);
    241         if (RefPtr<AtomicStringImpl> existingAtomicString = AtomicStringImpl::lookUp(buffer, m_length)) {
     241        if (RefPtr<AtomicStringImpl> existingAtomicString = AtomicStringImpl::lookUp(buffer, length())) {
    242242            m_value = *existingAtomicString;
    243243            setIs8Bit(m_value.impl()->is8Bit());
    … …  
    256256    if (isSubstring()) {
    257257        ASSERT(!substringBase()->isRope());
    258         m_value = substringBase()->m_value.substringSharingImpl(substringOffset(), m_length);
     258        m_value = substringBase()->m_value.substringSharingImpl(substringOffset(), length());
    259259        substringBase().clear();
    260260        return;
    … …  
    263263    if (is8Bit()) {
    264264        LChar* buffer;
    265         if (auto newImpl = StringImpl::tryCreateUninitialized(m_length, buffer)) {
     265        if (auto newImpl = StringImpl::tryCreateUninitialized(length(), buffer)) {
    266266            Heap::heap(this)->reportExtraMemoryAllocated(newImpl->cost());
    267267            m_value = WTFMove(newImpl);
    … …  
    277277
    278278    UChar* buffer;
    279     if (auto newImpl = StringImpl::tryCreateUninitialized(m_length, buffer)) {
     279    if (auto newImpl = StringImpl::tryCreateUninitialized(length(), buffer)) {
    280280        Heap::heap(this)->reportExtraMemoryAllocated(newImpl->cost());
    281281        m_value = WTFMove(newImpl);
    … …  
    302302void JSRopeString::resolveRopeSlowCase8(LChar* buffer) const
    303303{
    304     LChar* position = buffer + m_length; // We will be working backwards over the rope.
     304    LChar* position = buffer + length(); // We will be working backwards over the rope.
    305305    Vector<JSString*, 32, UnsafeVectorOverflow> workQueue; // Putting strings into a Vector is only OK because there are no GC points in this method.
    306306   
    … …  
    338338void JSRopeString::resolveRopeSlowCase(UChar* buffer) const
    339339{
    340     UChar* position = buffer + m_length; // We will be working backwards over the rope.
     340    UChar* position = buffer + length(); // We will be working backwards over the rope.
    341341    Vector<JSString*, 32, UnsafeVectorOverflow> workQueue; // These strings are kept alive by the parent rope, so using a Vector is OK.
    342342
    … …  
    431431{
    432432    if (propertyName == exec->propertyNames().length) {
    433         descriptor.setDescriptor(jsNumber(m_length), DontEnum | DontDelete | ReadOnly);
     433        descriptor.setDescriptor(jsNumber(length()), DontEnum | DontDelete | ReadOnly);
    434434        return true;
    435435    }
    436436   
    437437    Optional<uint32_t> index = parseIndex(propertyName);
    438     if (index && index.value() < m_length) {
     438    if (index && index.value() < length()) {
    439439        descriptor.setDescriptor(getIndex(exec, index.value()), DontDelete | ReadOnly);
    440440        return true;

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

    r206555 r207849  
    107107        ASSERT(!m_value.isNull());
    108108        Base::finishCreation(vm);
    109         m_length = length;
     109        setLength(length);
    110110        setIs8Bit(m_value.impl()->is8Bit());
    111111    }
    … …  
    115115        ASSERT(!m_value.isNull());
    116116        Base::finishCreation(vm);
    117         m_length = length;
     117        setLength(length);
    118118        setIs8Bit(m_value.impl()->is8Bit());
    119119        Heap::heap(this)->reportExtraMemoryAllocated(cost);
    … …  
    124124    {
    125125        Base::finishCreation(vm);
    126         m_length = 0;
     126        setLength(0);
    127127        setIs8Bit(true);
    128128    }
    … …  
    159159    const String& tryGetValue() const;
    160160    const StringImpl* tryGetValueImpl() const;
    161     unsigned length() const { return m_length; }
     161    ALWAYS_INLINE unsigned length() const { return m_length; }
     162    ALWAYS_INLINE static bool isValidLength(size_t length)
     163    {
     164        // While length is of type unsigned, the runtime and compilers are all
     165        // expecting that m_length is a positive value <= INT_MAX.
     166        // FIXME: Look into making the max length UINT_MAX to match StringImpl's max length.
     167        // https://bugs.webkit.org/show_bug.cgi?id=163955
     168        return length <= std::numeric_limits<int32_t>::max();
     169    }
    162170
    163171    JSValue toPrimitive(ExecState*, PreferredPrimitiveType) const;
    164     bool toBoolean() const { return !!m_length; }
     172    bool toBoolean() const { return !!length(); }
    165173    bool getPrimitiveNumber(ExecState*, double& number, JSValue&) const;
    166174    JSObject* toObject(ExecState*, JSGlobalObject*) const;
    … …  
    171179    bool getStringPropertyDescriptor(ExecState*, PropertyName, PropertyDescriptor&);
    172180
    173     bool canGetIndex(unsigned i) { return i < m_length; }
     181    bool canGetIndex(unsigned i) { return i < length(); }
    174182    JSString* getIndex(ExecState*, unsigned);
    175183
    … …  
    205213    }
    206214
     215    ALWAYS_INLINE void setLength(unsigned length)
     216    {
     217        RELEASE_ASSERT(isValidLength(length));
     218        m_length = length;
     219    }
     220
     221private:
    207222    mutable unsigned m_flags;
    208223
    … …  
    211226    mutable String m_value;
    212227
    213 private:
    214228    friend class LLIntOffsetsExtractor;
    215229
    … …  
    258272        }
    259273
    260         unsigned length() const { return m_jsString->m_length; }
     274        unsigned length() const { return m_jsString->length(); }
    261275
    262276    private:
    … …  
    278292        Base::finishCreation(vm);
    279293        ASSERT(!sumOverflows<int32_t>(s1->length(), s2->length()));
    280         m_length = s1->length() + s2->length();
     294        setLength(s1->length() + s2->length());
    281295        setIs8Bit(s1->is8Bit() && s2->is8Bit());
    282296        setIsSubstring(false);
    … …  
    290304        Base::finishCreation(vm);
    291305        ASSERT(!sumOverflows<int32_t>(s1->length(), s2->length(), s3->length()));
    292         m_length = s1->length() + s2->length() + s3->length();
     306        setLength(s1->length() + s2->length() + s3->length());
    293307        setIs8Bit(s1->is8Bit() && s2->is8Bit() &&  s3->is8Bit());
    294308        setIsSubstring(false);
    … …  
    303317        RELEASE_ASSERT(!sumOverflows<int32_t>(offset, length));
    304318        RELEASE_ASSERT(offset + length <= base->length());
    305         m_length = length;
     319        setLength(length);
    306320        setIs8Bit(base->is8Bit());
    307321        setIsSubstring(true);
    … …  
    327341        RELEASE_ASSERT(!sumOverflows<int32_t>(offset, length));
    328342        RELEASE_ASSERT(offset + length <= base->length());
    329         m_length = length;
     343        setLength(length);
    330344        setIs8Bit(base->is8Bit());
    331345        setIsSubstring(true);
    … …  
    346360    {
    347361        fiber(index).set(vm, this, jsString);
    348         m_length += jsString->m_length;
    349         RELEASE_ASSERT(static_cast<int32_t>(m_length) >= 0);
     362        setLength(length() + jsString->length());
    350363        setIs8Bit(is8Bit() && jsString->is8Bit());
    351364    }
    … …  
    675688{
    676689    if (propertyName == exec->propertyNames().length) {
    677         slot.setValue(this, DontEnum | DontDelete | ReadOnly, jsNumber(m_length));
     690        slot.setValue(this, DontEnum | DontDelete | ReadOnly, jsNumber(length()));
    678691        return true;
    679692    }
    680693
    681694    Optional<uint32_t> index = parseIndex(propertyName);
    682     if (index && index.value() < m_length) {
     695    if (index && index.value() < length()) {
    683696        slot.setValue(this, DontDelete | ReadOnly, getIndex(exec, index.value()));
    684697        return true;
    … …  
    690703ALWAYS_INLINE bool JSString::getStringPropertySlot(ExecState* exec, unsigned propertyName, PropertySlot& slot)
    691704{
    692     if (propertyName < m_length) {
     705    if (propertyName < length()) {
    693706        slot.setValue(this, DontDelete | ReadOnly, getIndex(exec, propertyName));
    694707        return true;
    … …  
    712725    if (isSubstring()) {
    713726        if (is8Bit())
    714             return StringView(substringBase()->m_value.characters8() + substringOffset(), m_length);
    715         return StringView(substringBase()->m_value.characters16() + substringOffset(), m_length);
     727            return StringView(substringBase()->m_value.characters8() + substringOffset(), length());
     728        return StringView(substringBase()->m_value.characters16() + substringOffset(), length());
    716729    }
    717730    resolveRope(&state);
    … …  
    724737        auto& base = substringBase()->m_value;
    725738        if (is8Bit())
    726             return { { base.characters8() + substringOffset(), m_length }, base };
    727         return { { base.characters16() + substringOffset(), m_length }, base };
     739            return { { base.characters8() + substringOffset(), length() }, base };
     740        return { { base.characters16() + substringOffset(), length() }, base };
    728741    }
    729742    resolveRope(&state);

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

    r206525 r207849  
    132132    auto scope = DECLARE_THROW_SCOPE(vm);
    133133    String result = WTF::tryMakeString(string, strings...);
    134     if (!result)
     134    if (UNLIKELY(!result || !JSString::isValidLength(result.length())))
    135135        return throwOutOfMemoryError(exec, scope);
    136136    return jsNontrivialString(exec, WTFMove(result));

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

    r206386 r207849  
    8989        return 0;
    9090
    91     Checked<unsigned, RecordOverflow> separatorLength = m_separator.length();
    92     Checked<unsigned, RecordOverflow> totalSeparatorsLength = separatorLength * (numberOfStrings - 1);
    93     Checked<unsigned, RecordOverflow> totalLength = totalSeparatorsLength + m_accumulatedStringsLength;
     91    Checked<int32_t, RecordOverflow> separatorLength = m_separator.length();
     92    Checked<int32_t, RecordOverflow> totalSeparatorsLength = separatorLength * (numberOfStrings - 1);
     93    Checked<int32_t, RecordOverflow> totalLength = totalSeparatorsLength + m_accumulatedStringsLength;
    9494
    95     unsigned result;
     95    int32_t result;
    9696    if (totalLength.safeGet(result) == CheckedState::DidOverflow) {
    9797        throwOutOfMemoryError(&state, scope);
Note: See TracChangeset for help on using the changeset viewer.