Skip to content

Commit

Permalink
Code inside strength reduction can incorrectly prove that we know wha…
Browse files Browse the repository at this point in the history
…t lastIndex is

https://bugs.webkit.org/show_bug.cgi?id=230802
<rdar://problem/83543699>

Reviewed by Mark Lam.

JSTests:

* stress/dont-fold-regexp-exec-when-we-dont-know-last-index-and-regexp-is-constant.js: Added.
(assert):
(let.reg.RegExp.foo.g.doExec):
(noInline.doExec):

Source/JavaScriptCore:

The phase was searching backwards in the graph to see if it found the RegExp
node. However, the RegExp node might be a JSConstant. Hence, the program
didn't allocate it. So we can't assume that we know what the lastIndex is.
We were incorrectly assuming it was "0" in a program like this:
a: JSConstant(RegExp)
b: RegExpExec(@A)

And we assumed we're invoking RegExpExec with lastIndex is 0, because we found
our RegExp in a backwards search. This is likely because we're also matching
NewRegExp nodes, in which case, it is valid to say lastIndex is 0.

This caused us to return a constant value that would've been the exec
result had we invoked it with a NewRegExpNode.

* dfg/DFGStrengthReductionPhase.cpp:
(JSC::DFG::StrengthReductionPhase::run):
(JSC::DFG::StrengthReductionPhase::handleNode):


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@283232 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
[email protected] committed Sep 29, 2021
1 parent fe3574a commit c9fee8e
Show file tree
Hide file tree
Showing 4 changed files with 64 additions and 2 deletions.
13 changes: 13 additions & 0 deletions JSTests/ChangeLog
Original file line number Diff line number Diff line change
@@ -1,3 +1,16 @@
2021-09-29 Saam Barati <[email protected]>

Code inside strength reduction can incorrectly prove that we know what lastIndex is
https://bugs.webkit.org/show_bug.cgi?id=230802
<rdar://problem/83543699>

Reviewed by Mark Lam.

* stress/dont-fold-regexp-exec-when-we-dont-know-last-index-and-regexp-is-constant.js: Added.
(assert):
(let.reg.RegExp.foo.g.doExec):
(noInline.doExec):

2021-09-29 Saam Barati <[email protected]>

DoesGCCheck does not use enough bits for nodeIndex
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
function assert(b) {
if (!b)
throw new Error;
}

let reg = RegExp(/foo/g)
function doExec() {
return reg.exec("-foo");
}
noInline(doExec)

for (let i = 0; i < 1000; ++i) {
let r = doExec();
if ((i % 2) === 0)
assert(r[0] === "foo");
else
assert(r === null);
}

26 changes: 26 additions & 0 deletions Source/JavaScriptCore/ChangeLog
Original file line number Diff line number Diff line change
@@ -1,3 +1,29 @@
2021-09-29 Saam Barati <[email protected]>

Code inside strength reduction can incorrectly prove that we know what lastIndex is
https://bugs.webkit.org/show_bug.cgi?id=230802
<rdar://problem/83543699>

Reviewed by Mark Lam.

The phase was searching backwards in the graph to see if it found the RegExp
node. However, the RegExp node might be a JSConstant. Hence, the program
didn't allocate it. So we can't assume that we know what the lastIndex is.
We were incorrectly assuming it was "0" in a program like this:
a: JSConstant(RegExp)
b: RegExpExec(@a)

And we assumed we're invoking RegExpExec with lastIndex is 0, because we found
our RegExp in a backwards search. This is likely because we're also matching
NewRegExp nodes, in which case, it is valid to say lastIndex is 0.

This caused us to return a constant value that would've been the exec
result had we invoked it with a NewRegExpNode.

* dfg/DFGStrengthReductionPhase.cpp:
(JSC::DFG::StrengthReductionPhase::run):
(JSC::DFG::StrengthReductionPhase::handleNode):

2021-09-29 Yusuke Suzuki <[email protected]>

[JSC] Use FixedVector in JITConstantPool
Expand Down
8 changes: 6 additions & 2 deletions Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -491,11 +491,13 @@ class StrengthReductionPhase : public Phase {

Node* regExpObjectNode = nullptr;
RegExp* regExp = nullptr;
bool regExpObjectNodeIsConstant = false;
if (m_node->op() == RegExpExec || m_node->op() == RegExpTest || m_node->op() == RegExpMatchFast) {
regExpObjectNode = m_node->child2().node();
if (RegExpObject* regExpObject = regExpObjectNode->dynamicCastConstant<RegExpObject*>(vm()))
if (RegExpObject* regExpObject = regExpObjectNode->dynamicCastConstant<RegExpObject*>(vm())) {
regExp = regExpObject->regExp();
else if (regExpObjectNode->op() == NewRegexp)
regExpObjectNodeIsConstant = true;
} else if (regExpObjectNode->op() == NewRegexp)
regExp = regExpObjectNode->castOperand<RegExp*>();
else {
if (verbose)
Expand Down Expand Up @@ -545,6 +547,8 @@ class StrengthReductionPhase : public Phase {
for (unsigned otherNodeIndex = m_nodeIndex; otherNodeIndex--;) {
Node* otherNode = m_block->at(otherNodeIndex);
if (otherNode == regExpObjectNode) {
if (regExpObjectNodeIsConstant)
break;
lastIndex = 0;
break;
}
Expand Down

0 comments on commit c9fee8e

Please sign in to comment.