Yarr: reject non-ASCII identity escapes in unicode patterns - #517
Conversation
|
Warning Review limit reached
On-demand reviews are free for the next 23 days. After that, they cost $0.25 per reviewed file. Or wait 17 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughThe parser now rejects non-ASCII identity escapes in ChangesRegular Expression Validation
Windows ARM64 Build Setup
Merge Risk: ⚪ Minimal · up to The PR makes a localized parser correction with focused regression coverage, and no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the bug, specification requirement, implementation change, preserved behavior, and test coverage. It does not include the repository template's Bugzilla URL, review line, or changed-file/function list, but the core information is complete. Full details: Linked Issues checkExplanation The Yarr parser change rejects non-ASCII identity escapes in u and v patterns, and the stress test covers the reported cases from issue 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line boolean logic fix with thorough test coverage.
What was reviewed:
- Traced the old vs new condition in
isIdentityEscapeAnError: for ASCII non-NUL and NUL inputs the result is unchanged; only non-ASCII now correctly errors underu/v. - Confirmed
strchris only reached whenchis ASCII and non-zero, so no truncation/NUL-terminator issues. - Checked all other
isIdentityEscapeAnErrorcall sites pass ASCII literals, and\-in classes is special-cased before the check — no behavior change there. - Test file covers both reject and accept cases across
u,v, classes,\q{}, and non-unicode fallback.
Extended reasoning...
Overview
Single-line change to isIdentityEscapeAnError in Source/JavaScriptCore/yarr/YarrParser.h, plus a new stress test JSTests/stress/regexp-unicode-identity-escape-non-ascii.js. The old condition (isASCII(ch) && !strchr(...)) || !ch only flagged ASCII characters not in the allowlist; non-ASCII characters slipped through and were accepted as identity escapes under the u/v flags, contrary to ECMA-262 (which restricts IdentityEscape to SyntaxCharacter or /, all ASCII). The new condition !isASCII(ch) || !ch || !strchr(...) rejects non-ASCII outright while preserving identical behavior for ASCII inputs.
Security risks
None. This tightens input validation (more patterns now throw SyntaxError). No new memory access, no widening of accepted inputs. strchr was already only called with byte-range values and still is — the !isASCII(ch) || !ch guards short-circuit before it.
Level of scrutiny
Moderate: Yarr is a hot path shared by all regex compilation, but the change is a pure boolean rewrite whose truth table is small enough to enumerate. I verified: ASCII-in-allowlist → no error (unchanged), ASCII-not-in-allowlist → error (unchanged), NUL → error (unchanged), non-ASCII → error (new, the fix). Non-unicode compilation is gated out by isEitherUnicodeCompilation() and untouched. All other callers of isIdentityEscapeAnError pass hard-coded ASCII literals ('B', 'x', 'k', 'p'/'P', 'q'), so only the parseEscape default case that reads peek() changes behavior — exactly as the PR description states.
Other factors
The new test exercises BMP and astral escapes, inside/outside character classes, inside \q{}, and asserts the positive cases (\$, \/, [\-], [\&] with v, non-unicode \Ç, unescaped Ç under u) still compile and match. V8 and SpiderMonkey already throw here, so this aligns with other engines. No outstanding reviewer comments; coderabbit was rate-limited and posted no findings.
Preview Builds
|
73543b0 to
5be0654
Compare
There was a problem hiding this comment.
LGTM — targeted spec-conformance fix with clear logic and good test coverage.
What was reviewed:
- Truth-tabled the new predicate: non-ASCII → error, NUL → error (unchanged), ASCII-in-allowlist → ok, ASCII-not-in-allowlist → error;
strchrnow only reached for non-NUL ASCII, so no widening of its input. - Confirmed all other
isIdentityEscapeAnErrorcall sites pass ASCII literals ('B', 'x', 'k', 'p'/'P', 'q'), so only theparseEscapedefault case changes behavior;\-in classes is special-cased before the check. - Change is gated on
isEitherUnicodeCompilation(), so legacy patterns are untouched; the new stress test covers BMP/astral, u/v, class/\q{}, and the must-still-compile cases.
Extended reasoning...
Overview
One-line boolean rewrite in isIdentityEscapeAnError (Source/JavaScriptCore/yarr/YarrParser.h) plus a new stress test. The old condition (isASCII(ch) && !strchr(...)) || !ch let non-ASCII characters through as valid identity escapes in u/v patterns because the isASCII guard (there to keep strchr byte-safe) short-circuited the whole check. The new condition !isASCII(ch) || !ch || !strchr(...) rejects non-ASCII outright, keeps the NUL rejection, and only reaches strchr for non-NUL ASCII — a strictly narrower input than before.
Security risks
None. This is a parser predicate that only decides whether to set ErrorCode::InvalidIdentityEscape. No memory management, no new allocations, no widening of strchr input. The change is monotonically stricter (only rejects patterns that were previously — incorrectly — accepted), so it cannot introduce new accepted inputs.
Level of scrutiny
Yarr is a critical path, but this specific change is a self-contained boolean predicate that is easy to verify by truth table. It is gated on isEitherUnicodeCompilation(), so non-unicode regexes are entirely unaffected. I walked every call site of isIdentityEscapeAnError in the file: the ones for 'B', 'x', 'k', 'p'/'P', 'q' all pass ASCII literals and are behaviorally unchanged; only the default: branch of parseEscape (which passes peek()) sees new behavior for non-ASCII input, and that branch already special-cases \- in classes before the check. The astral-plane test case works because peek() returns a lead surrogate (non-ASCII) in the 16-bit path.
Other factors
The fix matches ECMA-262 (IdentityEscape under [+UnicodeMode] is SyntaxCharacter | /, plus ClassSetReservedPunctuator in v-mode class sets — all ASCII) and aligns with V8/SpiderMonkey. The accompanying stress test is thorough: BMP and astral escapes under both u and v, inside/outside classes, inside \q{}, the NUL case, plus positive cases (\$, \/, [\-], [\&] with v, non-unicode \Ç) to guard against over-rejection. Compat risk is minimal since spec-compliant code and cross-engine code could never have relied on the old behavior.
…erns Under the u and v flags, an identity escape of a non-ASCII character must be a SyntaxError, but Yarr only validated ASCII escapes. /\Ç/u compiled and matched with the Annex B meaning. The fix is in oven-sh/WebKit#517. This bumps WEBKIT_VERSION to its preview build and adds coverage. Fixes #40441.
5be0654 to
1c7c53d
Compare
…erns Under the u and v flags, an identity escape of a non-ASCII character must be a SyntaxError, but Yarr only validated ASCII escapes. /\Ç/u compiled and matched with the Annex B meaning. The fix is in oven-sh/WebKit#517. This bumps WEBKIT_VERSION to its preview build and adds coverage. Fixes #40441.
1c7c53d to
77467a7
Compare
…erns Under the u and v flags, an identity escape of a non-ASCII character must be a SyntaxError, but Yarr only validated ASCII escapes. /\Ç/u compiled and matched with the Annex B meaning. The fix is in oven-sh/WebKit#517. This bumps WEBKIT_VERSION to its preview build and adds coverage. Fixes #40441.
|
Rebased onto main at 2da33d5 (the pin bun currently uses). This is a one-line spec-conformance fix with a stress test, ready for review. Each WebKit pin bump on the bun side needs a rebase and a fresh preview build here, so a review when convenient would help land oven-sh/bun#40459. |
77467a7 to
ed064c0
Compare
…erns Under the u and v flags, an identity escape of a non-ASCII character must be a SyntaxError, but Yarr only validated ASCII escapes. /\Ç/u compiled and matched with the Annex B meaning. The fix is in oven-sh/WebKit#517. This bumps WEBKIT_VERSION to its preview build and adds coverage. Fixes #40441.
|
Rebased onto 7259739 (the pin bun uses). This PR now carries a second commit that fixes the windows-11-arm preview lane: the scoop installer aborts without an error on the 20260823 runner image, so every preview build failed at "Install LLVM and Ninja" with scoop not recognized. Ninja 1.13.2 and 7-Zip are preinstalled on both current windows-11-arm images, so the job now uses them directly and keeps the LLVM 21.1.8 download. The full preview run for 3f6828a is green. Other PRs on this repo hit the same lane failure until this lands on main. |
3f6828a to
65909ba
Compare
…erns Under the u and v flags, an identity escape of a non-ASCII character must be a SyntaxError, but Yarr only validated ASCII escapes. /\Ç/u compiled and matched with the Annex B meaning. The fix is in oven-sh/WebKit#517. This bumps WEBKIT_VERSION to its preview build and adds coverage. Fixes #40441.
…erns Under the u and v flags, an identity escape of a non-ASCII character must be a SyntaxError, but Yarr only validated ASCII escapes. /\Ç/u compiled and matched with the Annex B meaning. The fix is in oven-sh/WebKit#517. This bumps WEBKIT_VERSION to its preview build and adds coverage. Fixes #40441.
65909ba to
9bf3013
Compare
…erns Under the u and v flags, an identity escape of a non-ASCII character must be a SyntaxError, but Yarr only validated ASCII escapes. /\Ç/u compiled and matched with the Annex B meaning. The fix is in oven-sh/WebKit#517. This bumps WEBKIT_VERSION to its preview build and adds coverage. Fixes #40441.
Under the u and v flags, IdentityEscape only allows SyntaxCharacter or '/', plus ClassSetReservedPunctuator inside v-mode class sets. All of those are ASCII. isIdentityEscapeAnError gated the error on isASCII(ch), so a non-ASCII escaped character skipped the check. /\Ç/u compiled and matched with the Annex B meaning instead of throwing a SyntaxError. Treat any non-ASCII character as an invalid identity escape when the pattern is a unicode or unicode-sets pattern. Fixes oven-sh/bun#40441.
9bf3013 to
f390a25
Compare
…erns Under the u and v flags, an identity escape of a non-ASCII character must be a SyntaxError, but Yarr only validated ASCII escapes. /\Ç/u compiled and matched with the Annex B meaning. The fix is in oven-sh/WebKit#517. This bumps WEBKIT_VERSION to its preview build and adds coverage. Fixes #40441.
1 participant
Problem
Under the
uandvflags,new RegExp("\\Ç", "u")compiles and\ÇmatchesÇwith the Annex B meaning. ECMA-262 requires a SyntaxError: IdentityEscape in unicode patterns allows only SyntaxCharacter or/, plus ClassSetReservedPunctuator insidev-mode class sets. All of those are ASCII.The cause is
isIdentityEscapeAnError(Source/JavaScriptCore/yarr/YarrParser.h:860). The error condition is gated onisASCII(ch)becausestrchronly handles bytes. A non-ASCIIchfails the gate, so the escape is accepted as an identity escape. V8 and SpiderMonkey both throw. Upstream WebKit has the same bug.Fix
Treat any non-ASCII character as an invalid identity escape in unicode and unicode-sets patterns. The
strchrcall is now only reached for non-NUL ASCII characters, as before.Every other call site of
isIdentityEscapeAnErrorpasses an ASCII literal, so only the IdentityEscape default case inparseEscapechanges behavior.\-inside a class is special-cased before the check and stays valid. Non-unicode patterns are unchanged.Tests
JSTests/stress/regexp-unicode-identity-escape-non-ascii.js covers BMP and astral escapes with
uandv, inside and outside classes, inside\q{}, and the cases that must keep compiling (\$,\/,[\-],[\&]withv, non-unicode\Ç). The test fails on the current engine and passes with the fix (verified against a local JSCOnly debug build).Fixes oven-sh/bun#40441.