Changeset 55035 in webkit

Timestamp:

Feb 19, 2010 3:36:09 PM (14 years ago)

Author:

barraclough@apple.com

Message:

JSString::getIndex() calls value() to resolve the string value (is a rope)
to a UString, then passes the result to jsSingleCharacterSubstring without
checking for an exception. In case of out-of-memory the returned UString
is null(), which may result in an out-of-buounds substring being created.
This is bad.

Reviewed by Oliver Hunt.

Simple fix is to be able to get an index from a rope without resolving to
UString. This may be a useful optimization in some test cases.

The same bug exists in some other methods is JSString, these can be fixed
by changing them to call getIndex().

• runtime/JSString.cpp:

(JSC::JSString::resolveRope):
(JSC::JSString::getStringPropertyDescriptor):

• runtime/JSString.h:

(JSC::jsSingleCharacterSubstring):
(JSC::JSString::getIndex):
(JSC::jsSingleCharacterString):
(JSC::JSString::getStringPropertySlot):

• runtime/UStringImpl.cpp:

(JSC::singleCharacterSubstring):

• runtime/UStringImpl.h:

(JSC::UStringImpl::singleCharacterSubstring):

Location:

trunk/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• runtime/JSString.cpp (modified) (2 diffs)
• runtime/JSString.h (modified) (6 diffs)
• runtime/UStringImpl.cpp (modified) (1 diff)
• runtime/UStringImpl.h (modified) (3 diffs)

trunk/JavaScriptCore/ChangeLog

@@ +1 @@
+2010-02-19  Gavin Barraclough  <barraclough@apple.com>
+
+        Reviewed by Oliver Hunt.
+
+        JSString::getIndex() calls value() to resolve the string value (is a rope)
+        to a UString, then passes the result to jsSingleCharacterSubstring without
+        checking for an exception.  In case of out-of-memory the returned UString
+        is null(), which may result in an out-of-buounds substring being created.
+        This is bad.
+
+        Simple fix is to be able to get an index from a rope without resolving to
+        UString.  This may be a useful optimization in some test cases.
+
+        The same bug exists in some other methods is JSString, these can be fixed
+        by changing them to call getIndex().
+
+        * runtime/JSString.cpp:
+        (JSC::JSString::resolveRope):
+        (JSC::JSString::getStringPropertyDescriptor):
+        * runtime/JSString.h:
+        (JSC::jsSingleCharacterSubstring):
+        (JSC::JSString::getIndex):
+        (JSC::jsSingleCharacterString):
+        (JSC::JSString::getStringPropertySlot):
+        * runtime/UStringImpl.cpp:
+        (JSC::singleCharacterSubstring):
+        * runtime/UStringImpl.h:
+        (JSC::UStringImpl::singleCharacterSubstring):
+
 2010-02-19  Oliver Hunt  <oliver@apple.com>
 

trunk/JavaScriptCore/runtime/JSString.cpp

@@ -51 +51 @@
         m_value = newImpl;
     else {
-        for (unsigned i = 0; i < m_fiberCount; ++i) {
-            m_other.m_fibers[i]->deref();
-            m_other.m_fibers[i] = 0;
-        }
-        m_fiberCount = 0;
-        ASSERT(!isRope());
-        ASSERT(m_value == UString());
         throwOutOfMemoryError(exec);
         return;
@@ -188 +181 @@
     unsigned i = propertyName.toStrictUInt32(&isStrictUInt32);
     if (isStrictUInt32 && i < m_length) {
-        descriptor.setDescriptor(jsSingleCharacterSubstring(exec, value(exec), i), DontDelete | ReadOnly);
+        descriptor.setDescriptor(getIndex(exec, i), DontDelete | ReadOnly);
         return true;
     }

trunk/JavaScriptCore/runtime/JSString.h

@@ -42 +42 @@
     JSString* jsSingleCharacterString(JSGlobalData*, UChar);
     JSString* jsSingleCharacterString(ExecState*, UChar);
-    JSString* jsSingleCharacterSubstring(JSGlobalData*, const UString&, unsigned offset);
     JSString* jsSingleCharacterSubstring(ExecState*, const UString&, unsigned offset);
     JSString* jsSubstring(JSGlobalData*, const UString&, unsigned offset, unsigned length);
@@ -366 +365 @@
     }
 
-    inline JSString* jsSingleCharacterSubstring(JSGlobalData* globalData, const UString& s, unsigned offset)
-    {
+    inline JSString* jsSingleCharacterSubstring(ExecState* exec, const UString& s, unsigned offset)
+    {
+        JSGlobalData* globalData = &exec->globalData();
+
         ASSERT(offset < static_cast<unsigned>(s.size()));
         UChar c = s.data()[offset];
@@ -392 +393 @@
     {
         ASSERT(canGetIndex(i));
-        return jsSingleCharacterSubstring(&exec->globalData(), value(exec), i);
+        if (!isRope())
+            return jsSingleCharacterSubstring(exec, m_value, i);
+
+        if (i < m_other.m_fibers[0]->length())
+            return jsString(exec, singleCharacterSubstring(m_other.m_fibers[0], i));
+        i -= m_other.m_fibers[0]->length();
+
+        ASSERT(m_fiberCount >= 2);
+        if (i < m_other.m_fibers[1]->length())
+            return jsString(exec, singleCharacterSubstring(m_other.m_fibers[1], i));
+        i -= m_other.m_fibers[1]->length();
+
+        ASSERT(m_fiberCount == 3);
+        ASSERT(i < m_other.m_fibers[2]->length());
+        return jsString(exec, singleCharacterSubstring(m_other.m_fibers[2], i));
     }
 
@@ -446 +461 @@
     inline JSString* jsString(ExecState* exec, const UString& s) { return jsString(&exec->globalData(), s); }
     inline JSString* jsSingleCharacterString(ExecState* exec, UChar c) { return jsSingleCharacterString(&exec->globalData(), c); }
-    inline JSString* jsSingleCharacterSubstring(ExecState* exec, const UString& s, unsigned offset) { return jsSingleCharacterSubstring(&exec->globalData(), s, offset); }
     inline JSString* jsSubstring(ExecState* exec, const UString& s, unsigned offset, unsigned length) { return jsSubstring(&exec->globalData(), s, offset, length); }
     inline JSString* jsNontrivialString(ExecState* exec, const UString& s) { return jsNontrivialString(&exec->globalData(), s); }
@@ -462 +476 @@
         unsigned i = propertyName.toStrictUInt32(&isStrictUInt32);
         if (isStrictUInt32 && i < m_length) {
-            slot.setValue(jsSingleCharacterSubstring(exec, value(exec), i));
+            slot.setValue(getIndex(exec, i));
             return true;
         }
@@ -472 +486 @@
     {
         if (propertyName < m_length) {
-            slot.setValue(jsSingleCharacterSubstring(exec, value(exec), propertyName));
+            slot.setValue(getIndex(exec, propertyName));
             return true;
         }

trunk/JavaScriptCore/runtime/UStringImpl.cpp

@@ -150 +150 @@
 }
 
+PassRefPtr<UStringImpl> singleCharacterSubstring(UStringOrRopeImpl* impl, unsigned index)
+{
+top:
+    if (impl->isRope()) {
+        URopeImpl* rope = static_cast<URopeImpl*>(impl);
+        for (unsigned i = 0; i < rope->m_fiberCount; ++i) {
+            UStringOrRopeImpl* currentFiber = rope->fibers(i);
+            unsigned fiberLength = currentFiber->length();
+            if (index < fiberLength) {
+                impl = currentFiber;
+                goto top;
+            }
+            index -= fiberLength;
+        }
+        CRASH();
+    }
+
+    return static_cast<UStringImpl*>(impl)->singleCharacterSubstring(index);
+}
+
 } // namespace JSC

trunk/JavaScriptCore/runtime/UStringImpl.h

@@ -213 +213 @@
     }
 
+    PassRefPtr<UStringImpl> singleCharacterSubstring(unsigned index) { ASSERT(index < length()); return create(this, index, 1); }
+
 private:
     // For SmallStringStorage, which allocates an array and uses an in-place new.
@@ -343 +345 @@
 
     friend class UStringOrRopeImpl;
+    friend PassRefPtr<UStringImpl> singleCharacterSubstring(UStringOrRopeImpl* ropeoid, unsigned index);
 };
 
@@ -355 +358 @@
 bool equal(const UStringImpl*, const UStringImpl*);
 
+PassRefPtr<UStringImpl> singleCharacterSubstring(UStringOrRopeImpl* ropeoid, unsigned index);
+
 }