Skip to content
Permalink
Browse files
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):



Canonical link: https://commits.webkit.org/46340@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@55035 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
Gavin Barraclough committed Feb 19, 2010
1 parent 9e04510 commit a4b3e6a93fd2c6681c81fb2fa72a8c444573a2ed
Showing 5 changed files with 75 additions and 14 deletions.
@@ -1,3 +1,32 @@
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>

RS = Gavin Barraclough.
@@ -50,13 +50,6 @@ void JSString::resolveRope(ExecState* exec) const
if (PassRefPtr<UStringImpl> newImpl = UStringImpl::tryCreateUninitialized(m_length, buffer))
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;
}
@@ -187,7 +180,7 @@ bool JSString::getStringPropertyDescriptor(ExecState* exec, const Identifier& pr
bool isStrictUInt32;
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;
}

@@ -41,7 +41,6 @@ namespace JSC {

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);
JSString* jsSubstring(ExecState*, const UString&, unsigned offset, unsigned length);
@@ -365,8 +364,10 @@ namespace JSC {
return fixupVPtr(globalData, new (globalData) JSString(globalData, UString(&c, 1)));
}

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];
if (c <= 0xFF)
@@ -391,7 +392,21 @@ namespace JSC {
inline JSString* JSString::getIndex(ExecState* exec, unsigned i)
{
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));
}

inline JSString* jsString(JSGlobalData* globalData, const UString& s)
@@ -445,7 +460,6 @@ namespace JSC {
inline JSString* jsEmptyString(ExecState* exec) { return jsEmptyString(&exec->globalData()); }
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); }
inline JSString* jsNontrivialString(ExecState* exec, const char* s) { return jsNontrivialString(&exec->globalData(), s); }
@@ -461,7 +475,7 @@ namespace JSC {
bool isStrictUInt32;
unsigned i = propertyName.toStrictUInt32(&isStrictUInt32);
if (isStrictUInt32 && i < m_length) {
slot.setValue(jsSingleCharacterSubstring(exec, value(exec), i));
slot.setValue(getIndex(exec, i));
return true;
}

@@ -471,7 +485,7 @@ namespace JSC {
ALWAYS_INLINE bool JSString::getStringPropertySlot(ExecState* exec, unsigned propertyName, PropertySlot& slot)
{
if (propertyName < m_length) {
slot.setValue(jsSingleCharacterSubstring(exec, value(exec), propertyName));
slot.setValue(getIndex(exec, propertyName));
return true;
}

@@ -149,4 +149,24 @@ void URopeImpl::destructNonRecursive()
}
}

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
@@ -212,6 +212,8 @@ class UStringImpl : public UStringOrRopeImpl {
ASSERT(!isStatic() || !isIdentifier());
}

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.
UStringImpl() { }
@@ -342,6 +344,7 @@ class URopeImpl : public UStringOrRopeImpl {
Fiber m_fibers[1];

friend class UStringOrRopeImpl;
friend PassRefPtr<UStringImpl> singleCharacterSubstring(UStringOrRopeImpl* ropeoid, unsigned index);
};

inline void UStringOrRopeImpl::deref()
@@ -354,6 +357,8 @@ inline void UStringOrRopeImpl::deref()

bool equal(const UStringImpl*, const UStringImpl*);

PassRefPtr<UStringImpl> singleCharacterSubstring(UStringOrRopeImpl* ropeoid, unsigned index);

}

#endif

0 comments on commit a4b3e6a

Please sign in to comment.