- Revision
- 252836
- Author
- [email protected]
- Date
- 2019-11-23 15:23:31 -0800 (Sat, 23 Nov 2019)
Log Message
[JSC] GetSubstitution is performed incorrectly via RegExp.prototype[@@replace]
https://bugs.webkit.org/show_bug.cgi?id=204490
Reviewed by Mark Lam.
JSTests:
* stress/replace-indexed-backreferences.js: Added.
* test262/expectations.yaml: Mark four test cases as passing.
Source/_javascript_Core:
String.prototype.replace and RegExp.prototype[Symbol.replace] are meant to perform the same substitution
of $-backreferences (called GetSubstitution in the spec: https://tc39.es/ecma262/#sec-getsubstitution).
The implementation of this in StringPrototype.cpp is correct but the one in RegExpPrototype.js is not.
In particular, the latter *removes* backreferences with out-of-range indices, instead of leaving them as-is.
One thing that is *not* broken in either implementation and thus maintained here is the fact $10 is interpreted
as $1 followed by a 0 when we have 1 <= n < 10 captures (and analogously for other invalid $nn backreferences).
This behavior is consistent across all engines but currently described incorrectly in the spec; this patch thus
aligns with the spec PR currently open to correct this (https://github.com/tc39/ecma262/pull/1732).
* builtins/RegExpPrototype.js:
(getSubstitution): Ensure that invalid backreferences remain untouched in the output string.
(replace): Fix off-by-one error when populating captures list. We shouldn't be reserving a slot for the full match.
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (252835 => 252836)
--- trunk/JSTests/ChangeLog 2019-11-23 21:36:33 UTC (rev 252835)
+++ trunk/JSTests/ChangeLog 2019-11-23 23:23:31 UTC (rev 252836)
@@ -1,3 +1,13 @@
+2019-11-23 Ross Kirsling <[email protected]>
+
+ [JSC] GetSubstitution is performed incorrectly via RegExp.prototype[@@replace]
+ https://bugs.webkit.org/show_bug.cgi?id=204490
+
+ Reviewed by Mark Lam.
+
+ * stress/replace-indexed-backreferences.js: Added.
+ * test262/expectations.yaml: Mark four test cases as passing.
+
2019-11-22 Tadeu Zagallo <[email protected]>
[WebAssembly] Improve Wasm::LLIntGenerator
Added: trunk/JSTests/stress/replace-indexed-backreferences.js (0 => 252836)
--- trunk/JSTests/stress/replace-indexed-backreferences.js (rev 0)
+++ trunk/JSTests/stress/replace-indexed-backreferences.js 2019-11-23 23:23:31 UTC (rev 252836)
@@ -0,0 +1,51 @@
+function shouldBe(actual, expected) {
+ if (actual !== expected)
+ throw new Error(`expected ${expected} but got ${actual}`);
+}
+
+const search = /q=(\w+)\+(\w+)/;
+const string = 'q=abc+def';
+
+shouldBe(string.replace(search, '$1 $2'), 'abc def');
+shouldBe(string.replace(search, '$01 $02'), 'abc def');
+shouldBe(string.replace(search, '$0$1$3$2$4'), '$0abc$3def$4');
+shouldBe(string.replace(search, '$00$01$03$02$04'), '$00abc$03def$04');
+shouldBe(string.replace(search, '$10$21$32$43'), 'abc0def1$32$43');
+
+shouldBe(search[Symbol.replace](string, '$1 $2'), 'abc def');
+shouldBe(search[Symbol.replace](string, '$01 $02'), 'abc def');
+shouldBe(search[Symbol.replace](string, '$0$1$3$2$4'), '$0abc$3def$4');
+shouldBe(search[Symbol.replace](string, '$00$01$03$02$04'), '$00abc$03def$04');
+shouldBe(search[Symbol.replace](string, '$10$21$32$43'), 'abc0def1$32$43');
+
+const longSearch = new RegExp(
+ '(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)' +
+ '(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)' +
+ '(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)' +
+ '(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)' +
+ '(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)' +
+ '(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)' +
+ '(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)' +
+ '(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)' +
+ '(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)' +
+ '(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)(\\w)'
+);
+
+const longString =
+ 'abcdefghijklmnopqrstuvwxyABCDEFGHIJKLMNOPQRSTUVWXY' +
+ 'abcdefghijklmnopqrstuvwxyABCDEFGHIJKLMNOPQRSTUVWXY';
+
+const longReplace =
+ '$0$1$2$3$4$5$6$7$8$9' +
+ '$10$11$12$13$14$15$16$17$18$19' +
+ '$20$21$22$23$24$25$26$27$28$29' +
+ '$30$31$32$33$34$35$36$37$38$39' +
+ '$40$41$42$43$44$45$46$47$48$49' +
+ '$50$51$52$53$54$55$56$57$58$59' +
+ '$60$61$62$63$64$65$66$67$68$69' +
+ '$70$71$72$73$74$75$76$77$78$79' +
+ '$80$81$82$83$84$85$86$87$88$89' +
+ '$90$91$92$93$94$95$96$97$98$99$100';
+
+shouldBe(longString.replace(longSearch, longReplace), '$0abcdefghijklmnopqrstuvwxyABCDEFGHIJKLMNOPQRSTUVWXYabcdefghijklmnopqrstuvwxyABCDEFGHIJKLMNOPQRSTUVWXj0');
+shouldBe(longSearch[Symbol.replace](longString, longReplace), '$0abcdefghijklmnopqrstuvwxyABCDEFGHIJKLMNOPQRSTUVWXYabcdefghijklmnopqrstuvwxyABCDEFGHIJKLMNOPQRSTUVWXj0');
Modified: trunk/JSTests/test262/expectations.yaml (252835 => 252836)
--- trunk/JSTests/test262/expectations.yaml 2019-11-23 21:36:33 UTC (rev 252835)
+++ trunk/JSTests/test262/expectations.yaml 2019-11-23 23:23:31 UTC (rev 252836)
@@ -1492,12 +1492,6 @@
test/built-ins/RegExp/prototype/Symbol.replace/result-coerce-matched.js:
default: 'Test262Error: Expected SameValue(«», «foo[toString value]bar») to be true'
strict mode: 'Test262Error: Expected SameValue(«», «foo[toString value]bar») to be true'
-test/built-ins/RegExp/prototype/Symbol.replace/subst-capture-idx-1.js:
- default: 'Test262Error: Expected SameValue(«a[cd]e», «a[cd$4$0]e») to be true'
- strict mode: 'Test262Error: Expected SameValue(«a[cd]e», «a[cd$4$0]e») to be true'
-test/built-ins/RegExp/prototype/Symbol.replace/subst-capture-idx-2.js:
- default: 'Test262Error: Expected SameValue(«a[cd]e», «a[cd$04$00]e») to be true'
- strict mode: 'Test262Error: Expected SameValue(«a[cd]e», «a[cd$04$00]e») to be true'
test/built-ins/RegExp/prototype/Symbol.search/u-lastindex-advance.js:
default: 'Test262Error: Expected SameValue(«1», «-1») to be true'
strict mode: 'Test262Error: Expected SameValue(«1», «-1») to be true'
Modified: trunk/Source/_javascript_Core/ChangeLog (252835 => 252836)
--- trunk/Source/_javascript_Core/ChangeLog 2019-11-23 21:36:33 UTC (rev 252835)
+++ trunk/Source/_javascript_Core/ChangeLog 2019-11-23 23:23:31 UTC (rev 252836)
@@ -1,3 +1,25 @@
+2019-11-23 Ross Kirsling <[email protected]>
+
+ [JSC] GetSubstitution is performed incorrectly via RegExp.prototype[@@replace]
+ https://bugs.webkit.org/show_bug.cgi?id=204490
+
+ Reviewed by Mark Lam.
+
+ String.prototype.replace and RegExp.prototype[Symbol.replace] are meant to perform the same substitution
+ of $-backreferences (called GetSubstitution in the spec: https://tc39.es/ecma262/#sec-getsubstitution).
+
+ The implementation of this in StringPrototype.cpp is correct but the one in RegExpPrototype.js is not.
+ In particular, the latter *removes* backreferences with out-of-range indices, instead of leaving them as-is.
+
+ One thing that is *not* broken in either implementation and thus maintained here is the fact $10 is interpreted
+ as $1 followed by a 0 when we have 1 <= n < 10 captures (and analogously for other invalid $nn backreferences).
+ This behavior is consistent across all engines but currently described incorrectly in the spec; this patch thus
+ aligns with the spec PR currently open to correct this (https://github.com/tc39/ecma262/pull/1732).
+
+ * builtins/RegExpPrototype.js:
+ (getSubstitution): Ensure that invalid backreferences remain untouched in the output string.
+ (replace): Fix off-by-one error when populating captures list. We shouldn't be reserving a slot for the full match.
+
2019-11-22 Saam Barati <[email protected]>
Use LLInt profiling to rule out generating an IC for get_by_val
Modified: trunk/Source/_javascript_Core/builtins/RegExpPrototype.js (252835 => 252836)
--- trunk/Source/_javascript_Core/builtins/RegExpPrototype.js 2019-11-23 21:36:33 UTC (rev 252835)
+++ trunk/Source/_javascript_Core/builtins/RegExpPrototype.js 2019-11-23 23:23:31 UTC (rev 252836)
@@ -222,10 +222,15 @@
default:
let chCode = ch.charCodeAt(0);
if (chCode >= 0x30 && chCode <= 0x39) {
+ let originalStart = start - 1;
start++;
+
let n = chCode - 0x30;
- if (n > m)
+ if (n > m) {
+ result = result + replacement.substring(originalStart, start);
break;
+ }
+
if (start < replacementLength) {
let nextChCode = replacement.charCodeAt(start);
if (nextChCode >= 0x30 && nextChCode <= 0x39) {
@@ -237,11 +242,14 @@
}
}
- if (n == 0)
+ if (n == 0) {
+ result = result + replacement.substring(originalStart, start);
break;
+ }
- if (captures[n] != @undefined)
- result = result + captures[n];
+ let capture = captures[n - 1];
+ if (capture !== @undefined)
+ result = result + capture;
} else
result = result + "$";
break;
@@ -313,13 +321,13 @@
let capN = result[n];
if (capN !== @undefined)
capN = @toString(capN);
- captures[n] = capN;
+ captures.@push(capN);
}
let replacement;
if (functionalReplace) {
- let replacerArgs = [ matched ].concat(captures.slice(1));
+ let replacerArgs = [ matched ].concat(captures);
replacerArgs.@push(position);
replacerArgs.@push(str);
