Repository navigation
Conversation
…h Unicode character class backtrackCharacterClassGreedy rematched forward from beginIndex on every single backtrack step for /u-mode variable-width classes, making each step O(n) and unanchored patterns like /[^"]*X/u O(n^3) on 16-bit input. The interpreter already steps backward one code unit (two for a valid surrogate pair) in O(1); do the same in the JIT, bounded by beginIndex so a lone trail at the start of the quantified range is not combined with a lead that precedes it.
WalkthroughA JavaScriptCore RegExp JIT backtracking path now rolls back one Unicode code point in O(1) time. A new stress test validates surrogate behavior, mixed-width unwinding, and execution time for large inputs. ChangesUnicode RegExp backtracking
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Preview Builds
|
…icodeCharImpl The forward read classifies surrogates with surrogateTagMask (& 0xdc00); the backward step must use the same classification so both sides agree on how many code units a given pair consumed. Adds coverage for U+F800/U+FC00.
There was a problem hiding this comment.
Thanks — 1e7b919 addresses the mask-consistency issue; the backward step now uses surrogateTagMask so it agrees with tryReadUnicodeCharImpl on which unit pairs count as 2-wide, and the added U+F800/U+FC00 tests cover it. I didn't find anything else, but this is hand-written JIT codegen on the regex hot path, so worth a human pass before merge.
What was reviewed:
beginIndexis stored under exactly!isFixedWidthCharacterClass()(==!hasOneCharacterSize() || invert()), so the frame slot is always live when this path runs.- Both
load16Unalignedsites read addresses the forward greedy loop already touched; theindex > beginIndexguard bounds the second load and prevents over-stepping past the range start. - Traced the visited-position invariant: a unit can't satisfy both
& surrogateTagMask == 0xd800and== 0xdc00, so ifindex-1was a 1-unit forward step its predecessor can't test as a lead — no over-retreat. #elsefallback (sub32(1)) matches old behavior sincereadCharacterdoesn't combine surrogates withoutYARR_JIT_UNICODE_EXPRESSIONS.
Extended reasoning...
Overview
Replaces the O(n)-per-step rematch loop in backtrackCharacterClassGreedy (variable-width /u character class on 16-bit input) with an O(1) backward step: retreat one code unit, and if it tests as a trail surrogate with a lead immediately before it (bounded by the stored beginIndex), retreat one more. ~30 lines of MacroAssembler codegen in YarrJIT.cpp plus a new stress test. My earlier review flagged that the first revision used & 0xfc00 while the forward path uses surrogateTagMask (& 0xdc00), causing index/count desync on inputs like U+F800 U+FC00; commit 1e7b919 switched both mask sites to surrogateTagMask and added test coverage for that case.
Security risks
Regex JIT operates on attacker-controlled strings, so an off-by-one here could in principle produce an out-of-bounds load16Unaligned or leave index below beginIndex. I checked both loads against the forward scan's proven-in-bounds range: the first load at offset checkedOffset - inputPosition after index -= 1 is the same address the forward loop's readCharacter used at that position; the second load (offset+1) is guarded by index > beginIndex, so it never reads before input[beginIndex - offset], which the forward loop's first iteration already read. The second sub32 is reached only when index > beginIndex, so index stays >= beginIndex. I don't see a memory-safety hazard, but the argument is invariant-based rather than structurally obvious.
Level of scrutiny
High. This is hand-authored JIT emission on a hot, security-relevant path (YARR handles untrusted regex subjects across all of Bun). The correctness depends on a non-local invariant — that the forward greedy loop visits position P as a code-point start only if input[P-1] does not test as a lead under the same mask — which I verified by case analysis but which a reviewer with YARR context should sanity-check, particularly around the errorCodePoint / lone-trail slow-path behavior in tryReadUnicodeCharSlowThunkGenerator. The first revision already had one subtle inconsistency, which raises the bar for a second pair of eyes.
Other factors
!isFixedWidthCharacterClass()is definitionally!hasOneCharacterSize() || invert(), matching thebeginIndexstore guard ingenerateCharacterClassGreedy, so the frame slot is always initialized on this path.regT2is newly used here withm_usesT2 = trueset, consistent withgenerateCharacterClassGreedy.- The
#elsebranch degrades tosub32(1), which matches pre-PR semantics because withoutYARR_JIT_UNICODE_EXPRESSIONSthe forwardreadCharacternever combines pairs. - Stress test covers BMP/non-BMP mixes, lone leads/trails, the
q-prefixedbeginIndexboundary, the F800/FC00 mask-consistency case, and a timing regression guard. - The remaining
surrogateTagMask = 0xdc00dc00vs0xfc00fc00question is pre-existing and reasonably deferred.
Deferring to human review rather than auto-approving given the code path; no outstanding issues from my side.
Problem
For
/u-mode regexes on 16-bit input,backtrackCharacterClassGreedyhandled variable-width character classes (e.g. any negated class like[^"]) by resetting tobeginIndexand walking forwardcount - 1code points on every backtrack step. That makes a single attempt O(n^2), and an unanchored match O(n^3).Minimal repro:
This shows up in the wild as a multi-second stall in
prettierwithparser: "typescript"on any file that contains a non-ASCII character, via@typescript-eslint's JSX detection regex/(?:^[^"'`]*<\/)|(?:^[^/]{2}.*\/>)/mu.Fix
Step backward one code unit, then load the unit at the new position: if it is a trail surrogate and the preceding unit (bounded by
beginIndex, whichgenerateCharacterClassGreedystores for exactly this case) is a lead surrogate, step back one more. This mirrors what the YARR interpreter'sbacktrackCharacterClassalready does viauncheckInput(1)+tryReadBackward().The surrogate classification uses
surrogateTagMaskso it agrees withtryReadUnicodeCharImplon how many units a given pair consumed; thebeginIndexbound prevents pairing a lone trail at the start of the range with a lead that precedes the range.Added
JSTests/stress/regexp-unicode-greedy-charclass-backtrack.jscovering mixed BMP/non-BMP backtracking, lone surrogates, U+F800/U+FC00, and the cubic-regression timing.Upstream
The same issue is being fixed upstream as WebKit#69364 (tracked at https://bugs.webkit.org/show_bug.cgi?id=319370) with a slightly different implementation. Either works for bun; once that lands, the next upstream sync will replace this.