- Revision
- 238143
- Author
- mark....@apple.com
- Date
- 2018-11-13 13:33:48 -0800 (Tue, 13 Nov 2018)
Log Message
Add OOM detection to StringPrototype's substituteBackreferences().
https://bugs.webkit.org/show_bug.cgi?id=191563
<rdar://problem/45720428>
Reviewed by Saam Barati.
JSTests:
* stress/regress-191563.js: Added.
Source/_javascript_Core:
* dfg/DFGStrengthReductionPhase.cpp:
(JSC::DFG::StrengthReductionPhase::handleNode):
* runtime/StringPrototype.cpp:
(JSC::substituteBackreferencesSlow):
(JSC::substituteBackreferencesInline):
(JSC::substituteBackreferences):
(JSC::replaceUsingRegExpSearch):
(JSC::replaceUsingStringSearch):
* runtime/StringPrototype.h:
Source/WTF:
Enhanced StringBuilder::toString() to skip the shrinkToFit(), reifyString(), and
the hasOverflowed() check if m_string is not null. When m_string is not null,
the StringBuilder either only has a single String in m_string (with m_buffer being
null), or reifyString() has already been called (resulting in a non-null m_string
with a possibly non-null m_buffer).
We can skip the overflow check because:
1. if the StringBuilder only has a single String, then there cannot be an overflow.
2. if reifyString() has already been called, then the hasOverflowed() checked has
already been done because every code path that calls reifyString() first does
the hasOverflowed() check.
We can skip shrinkToFit() because it only applies to m_buffer.
1. if the StringBuilder only has a single String, then there's no m_buffer to shrink.
2. if reifyString() has already been called, then we either came down
a. the toString() path with a null m_string previously, where we would have
already called shrinkToFit() before reifyString(), or
b. the toStringPreserveCapacity() path where we don't want to shrinkToFit().
We can skip reifyString() because:
1. if the StringBuilder only has a single String, then the string is already reified.
2. if reifyString() has been already called, then the string is already reified.
Note that if m_string is the null string and m_buffer is null, reifyString() will
replace it with the empty string. For this reason, we cannot solely check for
!m_buffer because we need to reify the null string into the empty string.
Note also that if m_string is null and m_buffer is non-null, reifyString() will
create a String and set m_string to it. However, m_buffer remains non-null.
For this reason, we cannot assert !m_buffer alone when m_string is non-null.
We add a m_isReified flag (only when assertions are enabled) to track the reified
case where both m_buffer and m_string are non-null.
* wtf/text/StringBuilder.cpp:
(WTF::StringBuilder::reifyString const):
* wtf/text/StringBuilder.h:
(WTF::StringBuilder::toString):
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (238142 => 238143)
--- trunk/JSTests/ChangeLog 2018-11-13 21:04:45 UTC (rev 238142)
+++ trunk/JSTests/ChangeLog 2018-11-13 21:33:48 UTC (rev 238143)
@@ -1,5 +1,15 @@
2018-11-13 Mark Lam <mark....@apple.com>
+ Add OOM detection to StringPrototype's substituteBackreferences().
+ https://bugs.webkit.org/show_bug.cgi?id=191563
+ <rdar://problem/45720428>
+
+ Reviewed by Saam Barati.
+
+ * stress/regress-191563.js: Added.
+
+2018-11-13 Mark Lam <mark....@apple.com>
+
LLIntSlowPath's llint_loop_osr and llint_replace should set the topCallFrame.
https://bugs.webkit.org/show_bug.cgi?id=191579
<rdar://problem/45942472>
Added: trunk/JSTests/stress/regress-191563.js (0 => 238143)
--- trunk/JSTests/stress/regress-191563.js (rev 0)
+++ trunk/JSTests/stress/regress-191563.js 2018-11-13 21:33:48 UTC (rev 238143)
@@ -0,0 +1,22 @@
+//@ skip if $memoryLimited
+
+function foo(str, count) {
+ while (str.length < count) {
+ try {
+ str += str;
+ } catch (e) {}
+ }
+ return str.substring();
+}
+var x = foo("1", 1 << 20);
+var y = foo("$1", 1 << 16);
+
+var exception;
+try {
+ var __v_6623 = x.replace(/(.+)/g, y);
+} catch (e) {
+ exception = e;
+}
+
+if (exception != "Error: Out of memory")
+ throw "FAILED";
Modified: trunk/Source/_javascript_Core/ChangeLog (238142 => 238143)
--- trunk/Source/_javascript_Core/ChangeLog 2018-11-13 21:04:45 UTC (rev 238142)
+++ trunk/Source/_javascript_Core/ChangeLog 2018-11-13 21:33:48 UTC (rev 238143)
@@ -1,3 +1,21 @@
+2018-11-12 Mark Lam <mark....@apple.com>
+
+ Add OOM detection to StringPrototype's substituteBackreferences().
+ https://bugs.webkit.org/show_bug.cgi?id=191563
+ <rdar://problem/45720428>
+
+ Reviewed by Saam Barati.
+
+ * dfg/DFGStrengthReductionPhase.cpp:
+ (JSC::DFG::StrengthReductionPhase::handleNode):
+ * runtime/StringPrototype.cpp:
+ (JSC::substituteBackreferencesSlow):
+ (JSC::substituteBackreferencesInline):
+ (JSC::substituteBackreferences):
+ (JSC::replaceUsingRegExpSearch):
+ (JSC::replaceUsingStringSearch):
+ * runtime/StringPrototype.h:
+
2018-11-13 Mark Lam <mark....@apple.com>
LLIntSlowPath's llint_loop_osr and llint_replace should set the topCallFrame.
Modified: trunk/Source/_javascript_Core/dfg/DFGStrengthReductionPhase.cpp (238142 => 238143)
--- trunk/Source/_javascript_Core/dfg/DFGStrengthReductionPhase.cpp 2018-11-13 21:04:45 UTC (rev 238142)
+++ trunk/Source/_javascript_Core/dfg/DFGStrengthReductionPhase.cpp 2018-11-13 21:33:48 UTC (rev 238143)
@@ -842,8 +842,11 @@
unsigned replLen = replace.length();
if (lastIndex < result.start || replLen) {
builder.append(string, lastIndex, result.start - lastIndex);
- if (replLen)
- builder.append(substituteBackreferences(replace, string, ovector.data(), regExp));
+ if (replLen) {
+ StringBuilder replacement;
+ substituteBackreferences(replacement, replace, string, ovector.data(), regExp);
+ builder.append(replacement);
+ }
}
lastIndex = result.end;
Modified: trunk/Source/_javascript_Core/runtime/StringPrototype.cpp (238142 => 238143)
--- trunk/Source/_javascript_Core/runtime/StringPrototype.cpp 2018-11-13 21:04:45 UTC (rev 238142)
+++ trunk/Source/_javascript_Core/runtime/StringPrototype.cpp 2018-11-13 21:33:48 UTC (rev 238143)
@@ -185,9 +185,8 @@
// ------------------------------ Functions --------------------------
-static NEVER_INLINE String substituteBackreferencesSlow(StringView replacement, StringView source, const int* ovector, RegExp* reg, size_t i)
+static NEVER_INLINE void substituteBackreferencesSlow(StringBuilder& result, StringView replacement, StringView source, const int* ovector, RegExp* reg, size_t i)
{
- StringBuilder substitutedReplacement;
bool hasNamedCaptures = reg && reg->hasNamedCaptures();
int offset = 0;
do {
@@ -198,7 +197,7 @@
if (ref == '$') {
// "$$" -> "$"
++i;
- substitutedReplacement.append(replacement.substring(offset, i - offset));
+ result.append(replacement.substring(offset, i - offset));
offset = i + 1;
continue;
}
@@ -218,7 +217,7 @@
} else if (reg && ref == '<') {
// Named back reference
if (!hasNamedCaptures) {
- substitutedReplacement.append(replacement.substring(i, 2));
+ result.append(replacement.substring(i, 2));
offset = i + 2;
advance = 1;
continue;
@@ -272,31 +271,29 @@
continue;
if (i - offset)
- substitutedReplacement.append(replacement.substring(offset, i - offset));
+ result.append(replacement.substring(offset, i - offset));
i += 1 + advance;
offset = i + 1;
if (backrefStart >= 0)
- substitutedReplacement.append(source.substring(backrefStart, backrefLength));
+ result.append(source.substring(backrefStart, backrefLength));
} while ((i = replacement.find('$', i + 1)) != notFound);
if (replacement.length() - offset)
- substitutedReplacement.append(replacement.substring(offset));
-
- return substitutedReplacement.toString();
+ result.append(replacement.substring(offset));
}
-inline String substituteBackreferencesInline(const String& replacement, StringView source, const int* ovector, RegExp* reg)
+inline void substituteBackreferencesInline(StringBuilder& result, const String& replacement, StringView source, const int* ovector, RegExp* reg)
{
size_t i = replacement.find('$');
if (UNLIKELY(i != notFound))
- return substituteBackreferencesSlow(replacement, source, ovector, reg, i);
+ return substituteBackreferencesSlow(result, replacement, source, ovector, reg, i);
- return replacement;
+ result.append(replacement);
}
-String substituteBackreferences(const String& replacement, StringView source, const int* ovector, RegExp* reg)
+void substituteBackreferences(StringBuilder& result, const String& replacement, StringView source, const int* ovector, RegExp* reg)
{
- return substituteBackreferencesInline(replacement, source, ovector, reg);
+ substituteBackreferencesInline(result, replacement, source, ovector, reg);
}
struct StringRange {
@@ -689,9 +686,13 @@
if (UNLIKELY(!sourceRanges.tryConstructAndAppend(lastIndex, result.start - lastIndex)))
OUT_OF_MEMORY(exec, scope);
- if (replLen)
- replacements.append(substituteBackreferences(replacementString, source, ovector, regExp));
- else
+ if (replLen) {
+ StringBuilder replacement(StringBuilder::OverflowHandler::RecordOverflow);
+ substituteBackreferences(replacement, replacementString, source, ovector, regExp);
+ if (UNLIKELY(replacement.hasOverflowed()))
+ OUT_OF_MEMORY(exec, scope);
+ replacements.append(replacement.toString());
+ } else
replacements.append(String());
}
}
@@ -801,7 +802,16 @@
size_t matchEnd = matchStart + searchString.impl()->length();
int ovector[2] = { static_cast<int>(matchStart), static_cast<int>(matchEnd)};
- String middlePart = callType != CallType::None ? replaceString : substituteBackreferences(replaceString, string, ovector, 0);
+ String middlePart;
+ if (callType != CallType::None)
+ middlePart = replaceString;
+ else {
+ StringBuilder replacement(StringBuilder::OverflowHandler::RecordOverflow);
+ substituteBackreferences(replacement, replaceString, string, ovector, 0);
+ if (UNLIKELY(replacement.hasOverflowed()))
+ OUT_OF_MEMORY(exec, scope);
+ middlePart = replacement.toString();
+ }
size_t leftLength = stringImpl->length() - matchEnd;
String rightPart(StringImpl::createSubstringSharingImpl(*stringImpl, matchEnd, leftLength));
Modified: trunk/Source/_javascript_Core/runtime/StringPrototype.h (238142 => 238143)
--- trunk/Source/_javascript_Core/runtime/StringPrototype.h 2018-11-13 21:04:45 UTC (rev 238142)
+++ trunk/Source/_javascript_Core/runtime/StringPrototype.h 2018-11-13 21:33:48 UTC (rev 238143)
@@ -59,7 +59,7 @@
JSCell* JIT_OPERATION operationStringProtoFuncReplaceRegExpString(
ExecState*, JSString* thisValue, RegExpObject* searchValue, JSString* replaceValue);
-String substituteBackreferences(const String& replacement, StringView source, const int* ovector, RegExp* reg);
+void substituteBackreferences(StringBuilder& result, const String& replacement, StringView source, const int* ovector, RegExp*);
EncodedJSValue JSC_HOST_CALL stringProtoFuncRepeatCharacter(ExecState*);
EncodedJSValue JSC_HOST_CALL stringProtoFuncSplitFast(ExecState*);
Modified: trunk/Source/WTF/ChangeLog (238142 => 238143)
--- trunk/Source/WTF/ChangeLog 2018-11-13 21:04:45 UTC (rev 238142)
+++ trunk/Source/WTF/ChangeLog 2018-11-13 21:33:48 UTC (rev 238143)
@@ -1,3 +1,49 @@
+2018-11-12 Mark Lam <mark....@apple.com>
+
+ Add OOM detection to StringPrototype's substituteBackreferences().
+ https://bugs.webkit.org/show_bug.cgi?id=191563
+ <rdar://problem/45720428>
+
+ Reviewed by Saam Barati.
+
+ Enhanced StringBuilder::toString() to skip the shrinkToFit(), reifyString(), and
+ the hasOverflowed() check if m_string is not null. When m_string is not null,
+ the StringBuilder either only has a single String in m_string (with m_buffer being
+ null), or reifyString() has already been called (resulting in a non-null m_string
+ with a possibly non-null m_buffer).
+
+ We can skip the overflow check because:
+ 1. if the StringBuilder only has a single String, then there cannot be an overflow.
+ 2. if reifyString() has already been called, then the hasOverflowed() checked has
+ already been done because every code path that calls reifyString() first does
+ the hasOverflowed() check.
+
+ We can skip shrinkToFit() because it only applies to m_buffer.
+ 1. if the StringBuilder only has a single String, then there's no m_buffer to shrink.
+ 2. if reifyString() has already been called, then we either came down
+ a. the toString() path with a null m_string previously, where we would have
+ already called shrinkToFit() before reifyString(), or
+ b. the toStringPreserveCapacity() path where we don't want to shrinkToFit().
+
+ We can skip reifyString() because:
+ 1. if the StringBuilder only has a single String, then the string is already reified.
+ 2. if reifyString() has been already called, then the string is already reified.
+
+ Note that if m_string is the null string and m_buffer is null, reifyString() will
+ replace it with the empty string. For this reason, we cannot solely check for
+ !m_buffer because we need to reify the null string into the empty string.
+
+ Note also that if m_string is null and m_buffer is non-null, reifyString() will
+ create a String and set m_string to it. However, m_buffer remains non-null.
+ For this reason, we cannot assert !m_buffer alone when m_string is non-null.
+ We add a m_isReified flag (only when assertions are enabled) to track the reified
+ case where both m_buffer and m_string are non-null.
+
+ * wtf/text/StringBuilder.cpp:
+ (WTF::StringBuilder::reifyString const):
+ * wtf/text/StringBuilder.h:
+ (WTF::StringBuilder::toString):
+
2018-11-10 Benjamin Poulain <benja...@webkit.org>
Fix a fixme: rename wtfObjcMsgSend to wtfObjCMsgSend
Modified: trunk/Source/WTF/wtf/text/StringBuilder.cpp (238142 => 238143)
--- trunk/Source/WTF/wtf/text/StringBuilder.cpp 2018-11-13 21:04:45 UTC (rev 238142)
+++ trunk/Source/WTF/wtf/text/StringBuilder.cpp 2018-11-13 21:33:48 UTC (rev 238143)
@@ -51,6 +51,10 @@
return;
}
+#if !ASSERT_DISABLED
+ m_isReified = true;
+#endif
+
// Check for empty.
if (!m_length) {
m_string = StringImpl::empty();
Modified: trunk/Source/WTF/wtf/text/StringBuilder.h (238142 => 238143)
--- trunk/Source/WTF/wtf/text/StringBuilder.h 2018-11-13 21:04:45 UTC (rev 238142)
+++ trunk/Source/WTF/wtf/text/StringBuilder.h 2018-11-13 21:33:48 UTC (rev 238143)
@@ -227,10 +227,15 @@
String toString()
{
+ if (!m_string.isNull()) {
+ ASSERT(!m_buffer || m_isReified);
+ ASSERT(!hasOverflowed());
+ return m_string;
+ }
+
RELEASE_ASSERT(!hasOverflowed());
shrinkToFit();
- if (m_string.isNull())
- reifyString();
+ reifyString();
return m_string;
}
@@ -358,6 +363,9 @@
static_assert(String::MaxLength == std::numeric_limits<int32_t>::max(), "");
Checked<int32_t, ConditionalCrashOnOverflow> m_length;
bool m_is8Bit { true };
+#if !ASSERT_DISABLED
+ mutable bool m_isReified { false };
+#endif
};
template <>
