Diff
Modified: trunk/JSTests/ChangeLog (207848 => 207849)
--- trunk/JSTests/ChangeLog 2016-10-25 22:07:20 UTC (rev 207848)
+++ trunk/JSTests/ChangeLog 2016-10-25 22:19:37 UTC (rev 207849)
@@ -1,3 +1,13 @@
+2016-10-25 Mark Lam <[email protected]>
+
+ 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.
+
+ * stress/joined-strings-should-not-exceed-max-string-length.js: Added.
+
2016-10-25 JF Bastien <[email protected]>
WebAssembly JS API: simple Module test
Added: trunk/JSTests/stress/joined-strings-should-not-exceed-max-string-length.js (0 => 207849)
--- trunk/JSTests/stress/joined-strings-should-not-exceed-max-string-length.js (rev 0)
+++ trunk/JSTests/stress/joined-strings-should-not-exceed-max-string-length.js 2016-10-25 22:19:37 UTC (rev 207849)
@@ -0,0 +1,30 @@
+//@ runFTLNoCJIT
+//@ largeHeap
+// This test should not crash or fail any assertions.
+
+function shouldEqual(stepId, actual, expected) {
+ if (actual != expected) {
+ throw stepId + ": ERROR: expect " + expected + ", actual " + actual;
+ }
+}
+
+var exception = undefined;
+
+arr = [null,null,null,null];
+str = "xx";
+
+try {
+ for (var i = 0; i < 100; ++i)
+ str = arr.join(str);
+} catch (e) {
+ exception = e;
+}
+shouldEqual(10000, exception, "Error: Out of memory");
+
+exception = undefined;
+try {
+ str += 'x';
+} catch(e) {
+ exception = e;
+}
+shouldEqual(10100, exception, undefined);
Modified: trunk/Source/_javascript_Core/ChangeLog (207848 => 207849)
--- trunk/Source/_javascript_Core/ChangeLog 2016-10-25 22:07:20 UTC (rev 207848)
+++ trunk/Source/_javascript_Core/ChangeLog 2016-10-25 22:19:37 UTC (rev 207849)
@@ -1,3 +1,50 @@
+2016-10-25 Mark Lam <[email protected]>
+
+ 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.
+
+ 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):
+
2016-10-25 JF Bastien <[email protected]>
WebAssembly: fix unknown section name handling, and check for section size overflow
Modified: trunk/Source/_javascript_Core/runtime/JSString.cpp (207848 => 207849)
--- trunk/Source/_javascript_Core/runtime/JSString.cpp 2016-10-25 22:07:20 UTC (rev 207848)
+++ trunk/Source/_javascript_Core/runtime/JSString.cpp 2016-10-25 22:19:37 UTC (rev 207849)
@@ -120,7 +120,7 @@
{
if (isSubstring()) {
StringImpl::copyChars(
- buffer, substringBase()->m_value.characters8() + substringOffset(), m_length);
+ buffer, substringBase()->m_value.characters8() + substringOffset(), length());
return;
}
@@ -143,7 +143,7 @@
StringImpl::copyChars(position, fiberString.characters8(), length);
position += length;
}
- ASSERT((buffer + m_length) == position);
+ ASSERT((buffer + length()) == position);
}
void JSRopeString::resolveRopeInternal16(UChar* buffer) const
@@ -150,7 +150,7 @@
{
if (isSubstring()) {
StringImpl::copyChars(
- buffer, substringBase()->m_value.characters16() + substringOffset(), m_length);
+ buffer, substringBase()->m_value.characters16() + substringOffset(), length());
return;
}
@@ -176,12 +176,12 @@
StringImpl::copyChars(position, fiberString.characters16(), length);
position += length;
}
- ASSERT((buffer + m_length) == position);
+ ASSERT((buffer + length()) == position);
}
void JSRopeString::resolveRopeToAtomicString(ExecState* exec) const
{
- if (m_length > maxLengthForOnStackResolve) {
+ if (length() > maxLengthForOnStackResolve) {
resolveRope(exec);
m_value = AtomicString(m_value);
setIs8Bit(m_value.impl()->is8Bit());
@@ -191,12 +191,12 @@
if (is8Bit()) {
LChar buffer[maxLengthForOnStackResolve];
resolveRopeInternal8(buffer);
- m_value = AtomicString(buffer, m_length);
+ m_value = AtomicString(buffer, length());
setIs8Bit(m_value.impl()->is8Bit());
} else {
UChar buffer[maxLengthForOnStackResolve];
resolveRopeInternal16(buffer);
- m_value = AtomicString(buffer, m_length);
+ m_value = AtomicString(buffer, length());
setIs8Bit(m_value.impl()->is8Bit());
}
@@ -215,7 +215,7 @@
RefPtr<AtomicStringImpl> JSRopeString::resolveRopeToExistingAtomicString(ExecState* exec) const
{
- if (m_length > maxLengthForOnStackResolve) {
+ if (length() > maxLengthForOnStackResolve) {
resolveRope(exec);
if (RefPtr<AtomicStringImpl> existingAtomicString = AtomicStringImpl::lookUp(m_value.impl())) {
m_value = *existingAtomicString;
@@ -229,7 +229,7 @@
if (is8Bit()) {
LChar buffer[maxLengthForOnStackResolve];
resolveRopeInternal8(buffer);
- if (RefPtr<AtomicStringImpl> existingAtomicString = AtomicStringImpl::lookUp(buffer, m_length)) {
+ if (RefPtr<AtomicStringImpl> existingAtomicString = AtomicStringImpl::lookUp(buffer, length())) {
m_value = *existingAtomicString;
setIs8Bit(m_value.impl()->is8Bit());
clearFibers();
@@ -238,7 +238,7 @@
} else {
UChar buffer[maxLengthForOnStackResolve];
resolveRopeInternal16(buffer);
- if (RefPtr<AtomicStringImpl> existingAtomicString = AtomicStringImpl::lookUp(buffer, m_length)) {
+ if (RefPtr<AtomicStringImpl> existingAtomicString = AtomicStringImpl::lookUp(buffer, length())) {
m_value = *existingAtomicString;
setIs8Bit(m_value.impl()->is8Bit());
clearFibers();
@@ -255,7 +255,7 @@
if (isSubstring()) {
ASSERT(!substringBase()->isRope());
- m_value = substringBase()->m_value.substringSharingImpl(substringOffset(), m_length);
+ m_value = substringBase()->m_value.substringSharingImpl(substringOffset(), length());
substringBase().clear();
return;
}
@@ -262,7 +262,7 @@
if (is8Bit()) {
LChar* buffer;
- if (auto newImpl = StringImpl::tryCreateUninitialized(m_length, buffer)) {
+ if (auto newImpl = StringImpl::tryCreateUninitialized(length(), buffer)) {
Heap::heap(this)->reportExtraMemoryAllocated(newImpl->cost());
m_value = WTFMove(newImpl);
} else {
@@ -276,7 +276,7 @@
}
UChar* buffer;
- if (auto newImpl = StringImpl::tryCreateUninitialized(m_length, buffer)) {
+ if (auto newImpl = StringImpl::tryCreateUninitialized(length(), buffer)) {
Heap::heap(this)->reportExtraMemoryAllocated(newImpl->cost());
m_value = WTFMove(newImpl);
} else {
@@ -301,7 +301,7 @@
// rope-of-ropes.)
void JSRopeString::resolveRopeSlowCase8(LChar* buffer) const
{
- LChar* position = buffer + m_length; // We will be working backwards over the rope.
+ LChar* position = buffer + length(); // We will be working backwards over the rope.
Vector<JSString*, 32, UnsafeVectorOverflow> workQueue; // Putting strings into a Vector is only OK because there are no GC points in this method.
for (size_t i = 0; i < s_maxInternalRopeLength && fiber(i); ++i)
@@ -337,7 +337,7 @@
void JSRopeString::resolveRopeSlowCase(UChar* buffer) const
{
- UChar* position = buffer + m_length; // We will be working backwards over the rope.
+ UChar* position = buffer + length(); // We will be working backwards over the rope.
Vector<JSString*, 32, UnsafeVectorOverflow> workQueue; // These strings are kept alive by the parent rope, so using a Vector is OK.
for (size_t i = 0; i < s_maxInternalRopeLength && fiber(i); ++i)
@@ -430,12 +430,12 @@
bool JSString::getStringPropertyDescriptor(ExecState* exec, PropertyName propertyName, PropertyDescriptor& descriptor)
{
if (propertyName == exec->propertyNames().length) {
- descriptor.setDescriptor(jsNumber(m_length), DontEnum | DontDelete | ReadOnly);
+ descriptor.setDescriptor(jsNumber(length()), DontEnum | DontDelete | ReadOnly);
return true;
}
Optional<uint32_t> index = parseIndex(propertyName);
- if (index && index.value() < m_length) {
+ if (index && index.value() < length()) {
descriptor.setDescriptor(getIndex(exec, index.value()), DontDelete | ReadOnly);
return true;
}
Modified: trunk/Source/_javascript_Core/runtime/JSString.h (207848 => 207849)
--- trunk/Source/_javascript_Core/runtime/JSString.h 2016-10-25 22:07:20 UTC (rev 207848)
+++ trunk/Source/_javascript_Core/runtime/JSString.h 2016-10-25 22:19:37 UTC (rev 207849)
@@ -106,7 +106,7 @@
{
ASSERT(!m_value.isNull());
Base::finishCreation(vm);
- m_length = length;
+ setLength(length);
setIs8Bit(m_value.impl()->is8Bit());
}
@@ -114,7 +114,7 @@
{
ASSERT(!m_value.isNull());
Base::finishCreation(vm);
- m_length = length;
+ setLength(length);
setIs8Bit(m_value.impl()->is8Bit());
Heap::heap(this)->reportExtraMemoryAllocated(cost);
}
@@ -123,7 +123,7 @@
void finishCreation(VM& vm)
{
Base::finishCreation(vm);
- m_length = 0;
+ setLength(0);
setIs8Bit(true);
}
@@ -158,10 +158,18 @@
const String& value(ExecState*) const;
const String& tryGetValue() const;
const StringImpl* tryGetValueImpl() const;
- unsigned length() const { return m_length; }
+ ALWAYS_INLINE unsigned length() const { return m_length; }
+ ALWAYS_INLINE static bool isValidLength(size_t length)
+ {
+ // While length is of type unsigned, the runtime and compilers are all
+ // expecting that m_length is a positive value <= INT_MAX.
+ // FIXME: Look into making the max length UINT_MAX to match StringImpl's max length.
+ // https://bugs.webkit.org/show_bug.cgi?id=163955
+ return length <= std::numeric_limits<int32_t>::max();
+ }
JSValue toPrimitive(ExecState*, PreferredPrimitiveType) const;
- bool toBoolean() const { return !!m_length; }
+ bool toBoolean() const { return !!length(); }
bool getPrimitiveNumber(ExecState*, double& number, JSValue&) const;
JSObject* toObject(ExecState*, JSGlobalObject*) const;
double toNumber(ExecState*) const;
@@ -170,7 +178,7 @@
bool getStringPropertySlot(ExecState*, unsigned propertyName, PropertySlot&);
bool getStringPropertyDescriptor(ExecState*, PropertyName, PropertyDescriptor&);
- bool canGetIndex(unsigned i) { return i < m_length; }
+ bool canGetIndex(unsigned i) { return i < length(); }
JSString* getIndex(ExecState*, unsigned);
static Structure* createStructure(VM&, JSGlobalObject*, JSValue);
@@ -204,6 +212,13 @@
m_flags &= ~Is8Bit;
}
+ ALWAYS_INLINE void setLength(unsigned length)
+ {
+ RELEASE_ASSERT(isValidLength(length));
+ m_length = length;
+ }
+
+private:
mutable unsigned m_flags;
// A string is represented either by a String or a rope of fibers.
@@ -210,7 +225,6 @@
unsigned m_length;
mutable String m_value;
-private:
friend class LLIntOffsetsExtractor;
static JSValue toThis(JSCell*, ExecState*, ECMAMode);
@@ -257,7 +271,7 @@
return tmp;
}
- unsigned length() const { return m_jsString->m_length; }
+ unsigned length() const { return m_jsString->length(); }
private:
void expand();
@@ -277,7 +291,7 @@
{
Base::finishCreation(vm);
ASSERT(!sumOverflows<int32_t>(s1->length(), s2->length()));
- m_length = s1->length() + s2->length();
+ setLength(s1->length() + s2->length());
setIs8Bit(s1->is8Bit() && s2->is8Bit());
setIsSubstring(false);
fiber(0).set(vm, this, s1);
@@ -289,7 +303,7 @@
{
Base::finishCreation(vm);
ASSERT(!sumOverflows<int32_t>(s1->length(), s2->length(), s3->length()));
- m_length = s1->length() + s2->length() + s3->length();
+ setLength(s1->length() + s2->length() + s3->length());
setIs8Bit(s1->is8Bit() && s2->is8Bit() && s3->is8Bit());
setIsSubstring(false);
fiber(0).set(vm, this, s1);
@@ -302,7 +316,7 @@
Base::finishCreation(vm);
RELEASE_ASSERT(!sumOverflows<int32_t>(offset, length));
RELEASE_ASSERT(offset + length <= base->length());
- m_length = length;
+ setLength(length);
setIs8Bit(base->is8Bit());
setIsSubstring(true);
if (base->isSubstring()) {
@@ -326,7 +340,7 @@
Base::finishCreation(vm);
RELEASE_ASSERT(!sumOverflows<int32_t>(offset, length));
RELEASE_ASSERT(offset + length <= base->length());
- m_length = length;
+ setLength(length);
setIs8Bit(base->is8Bit());
setIsSubstring(true);
substringBase().set(vm, this, base);
@@ -345,8 +359,7 @@
void append(VM& vm, size_t index, JSString* jsString)
{
fiber(index).set(vm, this, jsString);
- m_length += jsString->m_length;
- RELEASE_ASSERT(static_cast<int32_t>(m_length) >= 0);
+ setLength(length() + jsString->length());
setIs8Bit(is8Bit() && jsString->is8Bit());
}
@@ -674,12 +687,12 @@
ALWAYS_INLINE bool JSString::getStringPropertySlot(ExecState* exec, PropertyName propertyName, PropertySlot& slot)
{
if (propertyName == exec->propertyNames().length) {
- slot.setValue(this, DontEnum | DontDelete | ReadOnly, jsNumber(m_length));
+ slot.setValue(this, DontEnum | DontDelete | ReadOnly, jsNumber(length()));
return true;
}
Optional<uint32_t> index = parseIndex(propertyName);
- if (index && index.value() < m_length) {
+ if (index && index.value() < length()) {
slot.setValue(this, DontDelete | ReadOnly, getIndex(exec, index.value()));
return true;
}
@@ -689,7 +702,7 @@
ALWAYS_INLINE bool JSString::getStringPropertySlot(ExecState* exec, unsigned propertyName, PropertySlot& slot)
{
- if (propertyName < m_length) {
+ if (propertyName < length()) {
slot.setValue(this, DontDelete | ReadOnly, getIndex(exec, propertyName));
return true;
}
@@ -711,8 +724,8 @@
{
if (isSubstring()) {
if (is8Bit())
- return StringView(substringBase()->m_value.characters8() + substringOffset(), m_length);
- return StringView(substringBase()->m_value.characters16() + substringOffset(), m_length);
+ return StringView(substringBase()->m_value.characters8() + substringOffset(), length());
+ return StringView(substringBase()->m_value.characters16() + substringOffset(), length());
}
resolveRope(&state);
return m_value;
@@ -723,8 +736,8 @@
if (isSubstring()) {
auto& base = substringBase()->m_value;
if (is8Bit())
- return { { base.characters8() + substringOffset(), m_length }, base };
- return { { base.characters16() + substringOffset(), m_length }, base };
+ return { { base.characters8() + substringOffset(), length() }, base };
+ return { { base.characters16() + substringOffset(), length() }, base };
}
resolveRope(&state);
return { m_value, m_value };
Modified: trunk/Source/_javascript_Core/runtime/JSStringBuilder.h (207848 => 207849)
--- trunk/Source/_javascript_Core/runtime/JSStringBuilder.h 2016-10-25 22:07:20 UTC (rev 207848)
+++ trunk/Source/_javascript_Core/runtime/JSStringBuilder.h 2016-10-25 22:19:37 UTC (rev 207849)
@@ -131,7 +131,7 @@
VM& vm = exec->vm();
auto scope = DECLARE_THROW_SCOPE(vm);
String result = WTF::tryMakeString(string, strings...);
- if (!result)
+ if (UNLIKELY(!result || !JSString::isValidLength(result.length())))
return throwOutOfMemoryError(exec, scope);
return jsNontrivialString(exec, WTFMove(result));
}
Modified: trunk/Source/_javascript_Core/runtime/JSStringJoiner.cpp (207848 => 207849)
--- trunk/Source/_javascript_Core/runtime/JSStringJoiner.cpp 2016-10-25 22:07:20 UTC (rev 207848)
+++ trunk/Source/_javascript_Core/runtime/JSStringJoiner.cpp 2016-10-25 22:19:37 UTC (rev 207849)
@@ -88,11 +88,11 @@
if (!numberOfStrings)
return 0;
- Checked<unsigned, RecordOverflow> separatorLength = m_separator.length();
- Checked<unsigned, RecordOverflow> totalSeparatorsLength = separatorLength * (numberOfStrings - 1);
- Checked<unsigned, RecordOverflow> totalLength = totalSeparatorsLength + m_accumulatedStringsLength;
+ Checked<int32_t, RecordOverflow> separatorLength = m_separator.length();
+ Checked<int32_t, RecordOverflow> totalSeparatorsLength = separatorLength * (numberOfStrings - 1);
+ Checked<int32_t, RecordOverflow> totalLength = totalSeparatorsLength + m_accumulatedStringsLength;
- unsigned result;
+ int32_t result;
if (totalLength.safeGet(result) == CheckedState::DidOverflow) {
throwOutOfMemoryError(&state, scope);
return 0;
