Conversation
… Yarr accepts \ + non-ASCII as an identity escape in Unicode mode
RegExp.escape classified each code point with StringView::contains(char16_t)
and isStrWhiteSpace(char16_t). A supplementary code point was truncated to
its low 16 bits before the check. U+2002A (low bits 0x2A, '*') came out as
"\<U+2002A>" and U+20009 (low bits 0x09, tab) came out as "\ud840\udc09".
896 of the 1,048,576 supplementary code points were affected. With the u or
v flag the escaped result then never matched the input. The punctuator
checks now run only for ASCII code points and the whitespace and surrogate
checks only for BMP code points. Supplementary code points pass through
unchanged, as the spec requires.
Yarr's isIdentityEscapeAnError only reported an error for an ASCII character
outside the SyntaxCharacter set. In UnicodeMode the spec allows only
SyntaxCharacter and '/' (plus '-' in a class and ClassSetReservedPunctuator
in a v-mode class set). "\é" and "\中" were accepted with the u flag, and a
supplementary character after the backslash was consumed as a single code
unit, so /^\u{1F600}$/u matched nothing and [\u{1F600}] matched the lone
surrogates. Any non-ASCII character after a backslash is now a SyntaxError
in UnicodeMode, which matches V8 and the spec. Non-Unicode patterns keep
the Annex B behaviour.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughChangesThe patch corrects supplementary-code-point handling in RegExp Unicode behavior
Merge Risk: ⚪ Minimal · up to The updated supplementary-plane coverage is bounded and introduces no identified merge-readiness risk. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives a detailed problem statement, fix summary, scope, and verification results. However, it does not follow the required template because it omits the Bugzilla URL, reviewer line, and changed-file/function list. Resolution Add the associated Bugzilla URL, include the required “Reviewed by NOBODY (OOPS!).” line or reviewer information, and list the changed files with relevant functions. Keep the existing problem, fix, scope, and verification details if desired.
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
Preview Builds
|
…stress test stays fast in every configuration
There was a problem hiding this comment.
Thanks for bounding the supplementary-plane sweep — ~60 code units × 16 planes is comfortably under the stress-test time limit now. I re-reviewed and didn't find any bugs; since this changes what Yarr accepts in Unicode mode (previously-tolerated \ + non-ASCII now throws SyntaxError), a JSC reviewer's look is still worthwhile.
What was reviewed
regExpConstructorEscape: theisASCII/U_IS_BMPguards cover every narrowing call site, and the removed surrogate-pair\ubranch is genuinely unreachable under the new condition.isIdentityEscapeAnError: non-Unicode mode is untouched (outerisEitherUnicodeCompilation()guard), andstrchris still only reached with an ASCII argument.- Stress tests: throw-based assertions, no printing, bounded iteration; the added self-check confirms every listed BMP code unit is actually rewritten before the plane sweep asserts its supplementary alias is not.
Extended reasoning...
Overview
Two narrowly-scoped spec-compliance fixes plus tests. In RegExpConstructor.cpp, regExpConstructorEscape was passing a char32_t code point into StringView::contains(char16_t), isStrWhiteSpace(char16_t), and U16_IS_SURROGATE, silently truncating supplementary code points and mis-escaping any whose low 16 bits collided with an escaped BMP value; the fix gates those checks behind isASCII()/U_IS_BMP() with explicit static_cast<char16_t> and drops the now-dead supplementary \uXXXX\uXXXX branch. In YarrParser.h, isIdentityEscapeAnError is reordered so that in u/v mode any non-ASCII escape is an InvalidIdentityEscape (matching the spec's IdentityEscape[+UnicodeMode] production), where before it silently fell through as valid. JSTests/stress/regexp-escape.js gains targeted supplementary-code-point cases and a bounded plane × aliasing-code-unit sweep, and a new regexp-unicode-identity-escape-non-ascii.js covers the Yarr change across atoms, classes, groups, \q{}, and v-mode set operations.
Security risks
None identified. This tightens input validation (more patterns rejected in Unicode mode) and removes an implicit integer narrowing in a host built-in. No memory management, allocation sizing, or bounds arithmetic changed; strchr is now only reached when isASCII(ch) holds, so no wide-value truncation into it either.
Level of scrutiny
Moderate. The C++ delta is small and mechanical, but the Yarr change is a user-visible behavior change: new RegExp("\\é", "u") used to compile in JSC and will now throw. That's the spec-mandated behavior and matches V8, but it's the kind of compat surface a JSC reviewer should sign off on. Both changed source directories are listed under @ WebKit/jsc-reviewers in .github/CODEOWNERS.
Other factors
The one concern from the prior review — the exhaustive 1,048,576-iteration sweep in regexp-escape.js exceeding the JSTests 200ms budget — was addressed in commit e11053b by replacing it with a targeted sweep of the ~60 BMP code units RegExp.escape actually rewrites, crossed with all 16 supplementary planes (≈960 iterations), plus a self-check that each listed BMP code unit is in fact escaped. Both test files use throw-only assertions with no printing, per JSTests/README.md conventions. No candidate issues were raised or ruled out by the bug hunt beyond that.
…s prebuilt tarballs again
main now carries the RegExp.escape half of this branch: upstream landed it as 320682@main (https://bugs.webkit.org/show_bug.cgi?id=323642) with JSTests/stress/regexp-escape-supplementary.js. Take main's RegExpConstructor.cpp and regexp-escape.js, so the branch differs from main only by the Yarr identity-escape fix and its test.
\ + non-ASCII as an identity escape in Unicode mode
Problem
With the
uorvflag, Yarr accepts\followed by any non-ASCII character as an identity escape.isIdentityEscapeAnError(YarrParser.h) reports an error only forisASCII(ch) && !strchr(syntaxChars, ch). The spec (IdentityEscape[+UnicodeMode]) allows only SyntaxCharacter and/, plus-in a class and ClassSetReservedPunctuator in a v-mode class set. V8 throws "Invalid escape" for every one of these./\é/uand/\中/ucompile and match the character (V8, spec: SyntaxError).parseEscapedoesatomPatternCharacter(consume()), which takes one code unit. So/^\😀$/ubecomes a lone lead surrogate atom followed by a lone trail surrogate atom and matches no input at all: none of the 1.1M single-code-point strings match it./[\😀]/umatches"\ud83d"and"\ude00"but not"\u{1F600}".Fix
In Unicode mode
isIdentityEscapeAnErrornow returns true for any code unit that is NUL, non-ASCII, or ASCII outside the allowed set. Non-Unicode patterns keep the Annex B behaviour. Every other caller of the function already passes an ASCII character, so only thedefault:arm ofparseEscapechanges behaviour.Scope
This PR first also fixed
RegExp.escapenarrowing a supplementary code point to 16 bits before classifying it. Upstream has since landed that half as 320682@main (https://bugs.webkit.org/show_bug.cgi?id=323642, withJSTests/stress/regexp-escape-supplementary.js), and it reached this fork'smainthrough the upstream merge in #614. The branch now takesmain'sRegExpConstructor.cppandregexp-escape.jsunchanged. Its diff againstmainis the Yarr change and its test. Upstreammainstill has the Yarr defect.Verification
JSTests/stress/regexp-unicode-identity-escape-non-ascii.js:\+ BMP, supplementary, lone surrogate, U+2028 and U+FEFF throw SyntaxError withuandvin an atom, a class, a group, a\q{}and a set operation. The allowed escapes still parse, and non-Unicode patterns still accept the same inputs.regexp-escape-supplementary.js,regexp-escape.js,regexp-v-flag-escaped-hyphen-after-set-operand.js,regexp-vflag-property-of-strings.jsandregexp-unicode-mix-escaped-and-literal-surrogates.jsunder a bun debug ASAN build (Linux x64) linked against the preview build of this head (autobuild-preview-pr-577-86df2756). All pass. The same build passes bun's URLPattern suite (v-flag patterns, 408 tests).mainwithout this change, the new stress file fails at its first assertion and bun's test for it fails 7 of 18 cases (RegExp: reject \ + non-ASCII identity escapes with the u and v flags (WebKit bump for oven-sh/WebKit#577) bun#41767).