RegExp.prototype.exec must always access lastIndex
https://bugs.webkit.org/show_bug.cgi?id=209375
Reviewed by Saam Barati.
JSTests:
* mozilla/ecma_3/RegExp/15.10.6.2-2.js:
Sync test with mozilla-central's copy.
(https://searchfox.org/mozilla-central/source/js/src/tests/non262/RegExp/15.10.6.2-2.js)
* stress/regexp-exec-always-accesses-lastindex.js: Added.
* test262/expectations.yaml:
Mark eight test cases as passing.
Source/JavaScriptCore:
From https://tc39.es/ecma262/#sec-regexpbuiltinexec:
21.2.5.2.2 Runtime Semantics: RegExpBuiltinExec ( R, S )
...
4. Let lastIndex be ? ToLength(? Get(R, "lastIndex")).
...
8. If global is false and sticky is false, set lastIndex to 0.
That is, we're always obliged to verify that lastIndex is Number-coercible, even if we don't use the value.
DFG, in particular, must make sure strength reductions don't apply when lastIndex isn't an unsigned integer
(i.e., when user code has written something strange to it).
foldToConstant already has an early out for this, but it needs to apply to convertToStatic too.
Furthermore, ToLength clamps negative values to 0, so correct getRegExpObjectLastIndexAsUnsigned accordingly.
* dfg/DFGStrengthReductionPhase.cpp:
(JSC::DFG::StrengthReductionPhase::handleNode):
* runtime/RegExpObjectInlines.h:
(JSC::getRegExpObjectLastIndexAsUnsigned):
(JSC::RegExpObject::execInline):
(JSC::RegExpObject::matchInline):
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@259246 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
index 0959c69..6203e5c 100644
--- a/JSTests/ChangeLog
+++ b/JSTests/ChangeLog
@@ -1,3 +1,19 @@
+2020-03-30 Ross Kirsling <ross.kirsling@sony.com>
+
+ RegExp.prototype.exec must always access lastIndex
+ https://bugs.webkit.org/show_bug.cgi?id=209375
+
+ Reviewed by Saam Barati.
+
+ * mozilla/ecma_3/RegExp/15.10.6.2-2.js:
+ Sync test with mozilla-central's copy.
+ (https://searchfox.org/mozilla-central/source/js/src/tests/non262/RegExp/15.10.6.2-2.js)
+
+ * stress/regexp-exec-always-accesses-lastindex.js: Added.
+
+ * test262/expectations.yaml:
+ Mark eight test cases as passing.
+
2020-03-30 Caio Lima <ticaiolima@gmail.com>
[JSC] Public class field should accept "static" as field name
diff --git a/JSTests/mozilla/ecma_3/RegExp/15.10.6.2-2.js b/JSTests/mozilla/ecma_3/RegExp/15.10.6.2-2.js
index cce4c2c..5ec34d2 100644
--- a/JSTests/mozilla/ecma_3/RegExp/15.10.6.2-2.js
+++ b/JSTests/mozilla/ecma_3/RegExp/15.10.6.2-2.js
@@ -139,15 +139,6 @@
addThis();
/*
- * Now let's set |lastIndex| to -1, so the match should again be null -
- */
- status = inSection(5);
- pattern.lastIndex = -1;
- actualmatch = pattern.exec(string);
- expectedmatch = null;
- addThis();
-
- /*
* Now try some edge-case values. Thanks to the work done in
* http://bugzilla.mozilla.org/show_bug.cgi?id=124339, |lastIndex|
* is now stored as a double instead of a uint32 (unsigned integer).
@@ -161,12 +152,6 @@
actualmatch = pattern.exec(string);
expectedmatch = null;
addThis();
-
- status = inSection(7);
- pattern.lastIndex = -Math.pow(2,32);
- actualmatch = pattern.exec(string);
- expectedmatch = null;
- addThis();
status = inSection(8);
pattern.lastIndex = Math.pow(2,32) + 1;
@@ -174,47 +159,23 @@
expectedmatch = null;
addThis();
- status = inSection(9);
- pattern.lastIndex = -(Math.pow(2,32) + 1);
- actualmatch = pattern.exec(string);
- expectedmatch = null;
- addThis();
-
status = inSection(10);
pattern.lastIndex = Math.pow(2,32) * 2;
actualmatch = pattern.exec(string);
expectedmatch = null;
addThis();
- status = inSection(11);
- pattern.lastIndex = -Math.pow(2,32) * 2;
- actualmatch = pattern.exec(string);
- expectedmatch = null;
- addThis();
-
status = inSection(12);
pattern.lastIndex = Math.pow(2,40);
actualmatch = pattern.exec(string);
expectedmatch = null;
addThis();
- status = inSection(13);
- pattern.lastIndex = -Math.pow(2,40);
- actualmatch = pattern.exec(string);
- expectedmatch = null;
- addThis();
-
status = inSection(14);
pattern.lastIndex = Number.MAX_VALUE;
actualmatch = pattern.exec(string);
expectedmatch = null;
addThis();
-
- status = inSection(15);
- pattern.lastIndex = -Number.MAX_VALUE;
- actualmatch = pattern.exec(string);
- expectedmatch = null;
- addThis();
diff --git a/JSTests/stress/regexp-exec-always-accesses-lastindex.js b/JSTests/stress/regexp-exec-always-accesses-lastindex.js
new file mode 100644
index 0000000..a8846f17
--- /dev/null
+++ b/JSTests/stress/regexp-exec-always-accesses-lastindex.js
@@ -0,0 +1,50 @@
+function shouldBe(actual, expected) {
+ if (actual !== expected)
+ throw new Error(`expected ${expected} but got ${actual}`);
+}
+
+function shouldThrowTypeError(func) {
+ let error;
+ try {
+ func();
+ } catch (e) {
+ error = e;
+ }
+
+ if (!(error instanceof TypeError))
+ throw new Error('Expected TypeError!');
+}
+
+function test1() {
+ const r = /./;
+
+ let count = 0;
+ r.lastIndex = {
+ valueOf() {
+ count++;
+ return 0;
+ }
+ };
+
+ r.exec('abc');
+ shouldBe(count, 1);
+}
+noInline(test1);
+
+function test2() {
+ const r = /./;
+
+ r.lastIndex = {
+ valueOf() {
+ return Symbol();
+ }
+ };
+
+ r.exec('abc');
+}
+noInline(test2);
+
+for (let i = 0; i < 1e5; i++) {
+ test1();
+ shouldThrowTypeError(test2);
+}
diff --git a/JSTests/test262/expectations.yaml b/JSTests/test262/expectations.yaml
index e6034f1..81c0831 100644
--- a/JSTests/test262/expectations.yaml
+++ b/JSTests/test262/expectations.yaml
@@ -1663,15 +1663,6 @@
test/built-ins/RegExp/prototype/dotAll/cross-realm.js:
default: 'Test262Error: cross-realm RegExp.prototype Expected a TypeError to be thrown but no exception was thrown at all'
strict mode: 'Test262Error: cross-realm RegExp.prototype Expected a TypeError to be thrown but no exception was thrown at all'
-test/built-ins/RegExp/prototype/exec/S15.10.6.2_A5_T3.js:
- default: "TypeError: null is not an object (evaluating '__executed[0]')"
- strict mode: "TypeError: null is not an object (evaluating '__executed[0]')"
-test/built-ins/RegExp/prototype/exec/failure-lastindex-access.js:
- default: 'Test262Error: Expected SameValue(«0», «1») to be true'
- strict mode: 'Test262Error: Expected SameValue(«0», «1») to be true'
-test/built-ins/RegExp/prototype/exec/success-lastindex-access.js:
- default: 'Test262Error: Expected SameValue(«0», «1») to be true'
- strict mode: 'Test262Error: Expected SameValue(«0», «1») to be true'
test/built-ins/RegExp/prototype/exec/u-lastindex-adv.js:
default: 'Test262Error: Expected SameValue(«�», «null») to be true'
strict mode: 'Test262Error: Expected SameValue(«�», «null») to be true'
@@ -1690,9 +1681,6 @@
test/built-ins/RegExp/prototype/sticky/cross-realm.js:
default: 'Test262Error: cross-realm RegExp.prototype Expected a TypeError to be thrown but no exception was thrown at all'
strict mode: 'Test262Error: cross-realm RegExp.prototype Expected a TypeError to be thrown but no exception was thrown at all'
-test/built-ins/RegExp/prototype/test/S15.10.6.3_A1_T22.js:
- default: 'Test262Error: #1: __re = /(?:ab|cd)\d?/g; __re.lastIndex=-1; __executed = __re.test("aacd22 "); __executed === true'
- strict mode: 'Test262Error: #1: __re = /(?:ab|cd)\d?/g; __re.lastIndex=-1; __executed = __re.test("aacd22 "); __executed === true'
test/built-ins/RegExp/prototype/unicode/cross-realm.js:
default: 'Test262Error: cross-realm RegExp.prototype Expected a TypeError to be thrown but no exception was thrown at all'
strict mode: 'Test262Error: cross-realm RegExp.prototype Expected a TypeError to be thrown but no exception was thrown at all'
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index a7e27e2..826f8b4 100644
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,32 @@
+2020-03-30 Ross Kirsling <ross.kirsling@sony.com>
+
+ RegExp.prototype.exec must always access lastIndex
+ https://bugs.webkit.org/show_bug.cgi?id=209375
+
+ Reviewed by Saam Barati.
+
+ From https://tc39.es/ecma262/#sec-regexpbuiltinexec:
+ 21.2.5.2.2 Runtime Semantics: RegExpBuiltinExec ( R, S )
+ ...
+ 4. Let lastIndex be ? ToLength(? Get(R, "lastIndex")).
+ ...
+ 8. If global is false and sticky is false, set lastIndex to 0.
+
+ That is, we're always obliged to verify that lastIndex is Number-coercible, even if we don't use the value.
+
+ DFG, in particular, must make sure strength reductions don't apply when lastIndex isn't an unsigned integer
+ (i.e., when user code has written something strange to it).
+ foldToConstant already has an early out for this, but it needs to apply to convertToStatic too.
+
+ Furthermore, ToLength clamps negative values to 0, so correct getRegExpObjectLastIndexAsUnsigned accordingly.
+
+ * dfg/DFGStrengthReductionPhase.cpp:
+ (JSC::DFG::StrengthReductionPhase::handleNode):
+ * runtime/RegExpObjectInlines.h:
+ (JSC::getRegExpObjectLastIndexAsUnsigned):
+ (JSC::RegExpObject::execInline):
+ (JSC::RegExpObject::matchInline):
+
2020-03-30 Don Olmstead <don.olmstead@sony.com>
Non-unified build fixes late March 2020 edition
diff --git a/Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp b/Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp
index b3e41d8..aa915e6 100644
--- a/Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp
+++ b/Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp
@@ -535,6 +535,37 @@
ASSERT(m_node->op() != RegExpMatchFast);
+ // This will only work if we can prove what the value of lastIndex is. To do this
+ // safely, we need to execute the insertion set so that we see any previous strength
+ // reductions. This is needed for soundness since otherwise the effectfulness of any
+ // previous strength reductions would be invisible to us.
+ ASSERT(regExpObjectNode);
+ executeInsertionSet();
+ unsigned lastIndex = UINT_MAX;
+ for (unsigned otherNodeIndex = m_nodeIndex; otherNodeIndex--;) {
+ Node* otherNode = m_block->at(otherNodeIndex);
+ if (otherNode == regExpObjectNode) {
+ lastIndex = 0;
+ break;
+ }
+ if (otherNode->op() == SetRegExpObjectLastIndex
+ && otherNode->child1() == regExpObjectNode
+ && otherNode->child2()->isInt32Constant()
+ && otherNode->child2()->asInt32() >= 0) {
+ lastIndex = static_cast<unsigned>(otherNode->child2()->asInt32());
+ break;
+ }
+ if (writesOverlap(m_graph, otherNode, RegExpObject_lastIndex))
+ break;
+ }
+ if (lastIndex == UINT_MAX) {
+ if (verbose)
+ dataLog("Giving up because the last index is not known.\n");
+ break;
+ }
+ if (!regExp->globalOrSticky())
+ lastIndex = 0;
+
auto foldToConstant = [&] {
Node* stringNode = nullptr;
if (m_node->op() == RegExpExecNonGlobalOrSticky)
@@ -571,39 +602,6 @@
return false;
}
- unsigned lastIndex;
- if (regExp->globalOrSticky()) {
- // This will only work if we can prove what the value of lastIndex is. To do this
- // safely, we need to execute the insertion set so that we see any previous strength
- // reductions. This is needed for soundness since otherwise the effectfulness of any
- // previous strength reductions would be invisible to us.
- ASSERT(regExpObjectNode);
- executeInsertionSet();
- lastIndex = UINT_MAX;
- for (unsigned otherNodeIndex = m_nodeIndex; otherNodeIndex--;) {
- Node* otherNode = m_block->at(otherNodeIndex);
- if (otherNode == regExpObjectNode) {
- lastIndex = 0;
- break;
- }
- if (otherNode->op() == SetRegExpObjectLastIndex
- && otherNode->child1() == regExpObjectNode
- && otherNode->child2()->isInt32Constant()
- && otherNode->child2()->asInt32() >= 0) {
- lastIndex = static_cast<unsigned>(otherNode->child2()->asInt32());
- break;
- }
- if (writesOverlap(m_graph, otherNode, RegExpObject_lastIndex))
- break;
- }
- if (lastIndex == UINT_MAX) {
- if (verbose)
- dataLog("Giving up because the last index is not known.\n");
- return false;
- }
- } else
- lastIndex = 0;
-
m_graph.watchpoints().addLazily(globalObject->havingABadTimeWatchpoint());
Structure* structure = globalObject->regExpMatchesArrayStructure();
diff --git a/Source/JavaScriptCore/runtime/RegExpObjectInlines.h b/Source/JavaScriptCore/runtime/RegExpObjectInlines.h
index 0d9843f..854afcb 100644
--- a/Source/JavaScriptCore/runtime/RegExpObjectInlines.h
+++ b/Source/JavaScriptCore/runtime/RegExpObjectInlines.h
@@ -42,20 +42,14 @@
unsigned lastIndex;
if (LIKELY(jsLastIndex.isUInt32())) {
lastIndex = jsLastIndex.asUInt32();
- if (lastIndex > input.length()) {
- scope.release();
- regExpObject->setLastIndex(globalObject, 0);
+ if (lastIndex > input.length())
return UINT_MAX;
- }
} else {
double doubleLastIndex = jsLastIndex.toInteger(globalObject);
RETURN_IF_EXCEPTION(scope, UINT_MAX);
- if (doubleLastIndex < 0 || doubleLastIndex > input.length()) {
- scope.release();
- regExpObject->setLastIndex(globalObject, 0);
+ if (doubleLastIndex > input.length())
return UINT_MAX;
- }
- lastIndex = static_cast<unsigned>(doubleLastIndex);
+ lastIndex = (doubleLastIndex < 0) ? 0 : static_cast<unsigned>(doubleLastIndex);
}
return lastIndex;
}
@@ -70,14 +64,15 @@
RETURN_IF_EXCEPTION(scope, { });
bool globalOrSticky = regExp->globalOrSticky();
+ unsigned lastIndex = getRegExpObjectLastIndexAsUnsigned(globalObject, this, input);
+ RETURN_IF_EXCEPTION(scope, { });
+ if (lastIndex == UINT_MAX && globalOrSticky) {
+ scope.release();
+ setLastIndex(globalObject, 0);
+ return jsNull();
+ }
- unsigned lastIndex;
- if (globalOrSticky) {
- lastIndex = getRegExpObjectLastIndexAsUnsigned(globalObject, this, input);
- EXCEPTION_ASSERT(!scope.exception() || lastIndex == UINT_MAX);
- if (lastIndex == UINT_MAX)
- return jsNull();
- } else
+ if (!globalOrSticky)
lastIndex = 0;
MatchResult result;
@@ -115,9 +110,12 @@
}
unsigned lastIndex = getRegExpObjectLastIndexAsUnsigned(globalObject, this, input);
- EXCEPTION_ASSERT(!scope.exception() || (lastIndex == UINT_MAX));
- if (lastIndex == UINT_MAX)
+ RETURN_IF_EXCEPTION(scope, { });
+ if (lastIndex == UINT_MAX) {
+ scope.release();
+ setLastIndex(globalObject, 0);
return MatchResult::failed();
+ }
MatchResult result = globalObject->regExpGlobalData().performMatch(vm, globalObject, regExp, string, input, lastIndex);
RETURN_IF_EXCEPTION(scope, { });
