Skip to content

jsc-stress: add Yarr \B + surrogate match-length underflow regression test - #30396

Closed
robobun wants to merge 1 commit into
mainfrom
farm/571dd7af/yarr-non-bmp-trampoline-index-sync
Closed

robobun wants to merge 1 commit into
mainfrom
farm/571dd7af/yarr-non-bmp-trampoline-index-sync

Conversation

@robobun

@robobun robobun commented May 8, 2026 •

Copy link
Copy Markdown
Collaborator

What

bun -e 'console.log(/\B|x{1,2}?/u.exec("a\u{10ffff}b")[0].length)'

On ARM64 with Bun <= 1.3.14 this prints 4294967295 ((unsigned)-1). On x86_64 it prints 0.

Why

The Yarr JIT's non-BMP first-character optimization (ENABLE_YARR_JIT_UNICODE_CAN_INCREMENT_INDEX_FOR_NON_BMP, ARM64-only) records firstCharacterAdditionalReadSize = 1 whenever tryReadUnicodeChar decodes a surrogate pair, so the body-alternative retry loop can step past the whole code point.

In the BodyAlternativeEnd backtrack trampoline, the branch taken when lastAlternative.minimumSize > firstAlternative.minimumSize set matchStart = index + firstCharacterAdditionalReadSize but jumped to beginOp->m_reentry without applying the same adjustment to m_regs.index. A zero-width first alternative such as \B could then succeed at the stale index, producing output[0] > output[1]; createRegExpMatchesArray built the whole-match substring with length (unsigned)(end - start) = 4294967295.

Fix

Fixed upstream in WebKit while this PR was open: WebKit/WebKit@116394de195b ("[JSC] Yarr JIT \B+unicode+surrogate produces corrupted JSString, crashes WebContent", webkit.org/b/312604) applies the same trampoline change plus a SetForScope guard on word-boundary assertion reads.

That commit reached this repo through the WEBKIT_VERSION bump to d81bcc3d833c in #33133, so main is already fixed. My equivalent source change, oven-sh/WebKit#221, was closed as superseded.

This PR

Adds test/js/bun/jsc-stress/fixtures/yarr-non-bmp-backtrack-trampoline-index-sync.js so a future WebKit bump cannot silently regress this. The fixture asserts the whole-match substring is always well-formed (length <= input.length, index + length <= input.length) across the pattern shapes that reach this trampoline branch (delta == 1, delta == 2, a matching second alternative, /mu which disables the optimization), plus upstream's reduction from webkit.org/b/312604.

No src/** change; the source fix arrived via the WebKit dependency.

Verification

Fail before (CI build 52790, this branch at d4c75723 on the pre-#33133 WebKit): every aarch64 lane that ran jsc-stress.test.ts (darwin-26, darwin-14, debian-13, ubuntu-25.04, alpine-3.23, windows-11) failed with

error: \B|x{1,2}? on "a<U+10FFFF>b": match[0].length=4294967295 exceeds input.length=4

Pass after (rebased onto main, which carries the fixed WebKit c9ad5813fd):

$ bun bd test test/js/bun/jsc-stress/jsc-stress.test.ts -t yarr
(pass) JSC JIT Stress Tests > JS (Baseline/DFG/FTL) > yarr-non-bmp-backtrack-trampoline-index-sync.js

x86_64 never enables the optimization, so the meaningful pass/fail signal is the aarch64 lanes.

@github-actions github-actions Bot added the claude label May 8, 2026
@robobun

robobun commented May 8, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 1:46 PM PT - Jul 2nd, 2026

❌ @robobun, your commit 3a0ad2f has 1 failures in Build #68009 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 30396

That installs a local version of the PR into your bun-30396 executable, so you can run:

bun-30396 --bun

@robobun

robobun commented May 8, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status

Ready to merge. Test-only PR; the source fix already shipped on main via the WebKit bump in #33133.

Reproduced (fail-before), CI build 52790: this fixture, run on this branch before the WebKit bump, failed on every aarch64 lane (darwin-26, darwin-14, debian-13, ubuntu-25.04, alpine-3.23, windows-11) with

error: \B|x{1,2}? on "a<U+10FFFF>b": match[0].length=4294967295 exceeds input.length=4

the exact (unsigned)-1 underflow from the report. x86_64 is unaffected (the optimization is CPU(ARM64)-only).

Pass-after, CI build 68009 (rebased onto main): the fixture passes on every aarch64 test lane — alpine-3.23, darwin-14, darwin-26, debian-13, ubuntu-25.04, windows-11 — against the fixed WebKit c9ad5813fd.

Root cause: Yarr JIT BodyAlternativeEnd backtrack trampoline advanced matchStart by firstCharacterAdditionalReadSize without advancing m_regs.index, so a zero-width \B first alternative could return output[0] > output[1]. Fixed upstream in WebKit/WebKit@116394de195b (webkit.org/b/312604). oven-sh/WebKit#221 (my equivalent change) was closed as superseded once that landed via oven-sh/WebKit#263.

CI on build 68009: 285/286 jobs green; no annotation mentions this fixture. The single failure is test/js/bun/terminal/terminal.test.ts > creates subprocess with terminal attached timing out on darwin-14-x64 — a pre-existing PTY flake, also red on build 67990 (branch farm/73b69977/tls-sni-alpn), which does not contain this change. This PR touches only test/js/bun/jsc-stress/**, so it cannot cause a macOS PTY timeout.

Reviews: CodeRabbit "no actionable comments", claude-bot "LGTM"; no open review threads.

… test

On ARM64, /\B|x{1,2}?/u.exec("a\u{10ffff}b")[0].length returned 4294967295.

The Yarr JIT non-BMP first-character optimization (ARM64-only,
YARR_JIT_UNICODE_CAN_INCREMENT_INDEX_FOR_NON_BMP) advanced matchStart by
firstCharacterAdditionalReadSize in the BodyAlternativeEnd backtrack
trampoline without advancing m_regs.index, so a zero-width first
alternative like \B could return output[0] > output[1] and the match
length underflowed to (unsigned)-1.

Fixed upstream in WebKit 116394de195b (webkit.org/b/312604), pulled into
Bun by the WEBKIT_VERSION bump to d81bcc3d833c in #33133. This adds a
jsc-stress fixture so a future WebKit bump cannot silently regress it;
before the bump every aarch64 CI lane reproduced the underflow.
@robobun
robobun force-pushed the farm/571dd7af/yarr-non-bmp-trampoline-index-sync branch from 708a6f1 to 3a0ad2f Compare July 2, 2026 17:14
@robobun robobun changed the title yarr: keep index in sync with matchStart in non-BMP backtrack trampoline (ARM64) jsc-stress: add Yarr \B + surrogate match-length underflow regression test Jul 2, 2026
@robobun
robobun marked this pull request as ready for review July 2, 2026 17:14
@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9e1a3ba5-8878-472a-ab4c-ace6b34a0f4e

📥 Commits

Reviewing files that changed from the base of the PR and between 1498d7b and 3a0ad2f.

📒 Files selected for processing (2)
  • test/js/bun/jsc-stress/fixtures/yarr-non-bmp-backtrack-trampoline-index-sync.js
  • test/js/bun/jsc-stress/jsc-stress.test.ts

Walkthrough

Adds a new Bun/JSC stress test fixture exercising Yarr JIT handling of non-BMP first-character optimization and backtrack trampoline behavior, including a match-boundary validation helper and regression assertions, then registers the fixture in the JS JIT stress test suite's fixture list.

Changes

Regression test addition

Layer / File(s) Summary
Fixture setup, check helper, and test inputs
test/js/bun/jsc-stress/fixtures/yarr-non-bmp-backtrack-trampoline-index-sync.js
Adds documentation comments explaining the underflow regression, a check(re, input, label) helper enforcing match null/type/boundary invariants, and a fixed inputs array of non-BMP strings.
Regex variant loop and direct regression assertions
test/js/bun/jsc-stress/fixtures/yarr-non-bmp-backtrack-trampoline-index-sync.js
Adds a nested loop over multiple regex variants and inputs using check, plus two direct assertions covering previously-reported empty-match and undefined-match cases.
Register fixture in stress test suite
test/js/bun/jsc-stress/jsc-stress.test.ts
Adds the new fixture to the jsFixtures array under a "Yarr JIT" section so it runs in the baseline/DFG/FTL stress loop.

Sequence Diagram(s)

Not applicable — this change is test/fixture-only with no interaction flow to diagram.

Estimated code review effort: Medium

Suggested labels: test, javascriptcore

Suggested reviewers: none identified from provided context

Poem

A rabbit hops through code so deep,
Chasing bugs that JITs might keep,
Non-BMP chars, backtrack trampoline,
Boundaries checked, matches clean,
One more fixture, safe to leap. 🐇

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: a Yarr JIT regression test for the \\B/surrogate underflow issue.
Description check ✅ Passed The description covers what changed and how it was verified, with clear context, root cause, fix status, and test results.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — test-only addition of a well-documented Yarr JIT regression fixture following the existing jsc-stress pattern.

Extended reasoning...

Overview

This PR adds a single JSC stress test fixture (yarr-non-bmp-backtrack-trampoline-index-sync.js) and registers it in the jsFixtures array of test/js/bun/jsc-stress/jsc-stress.test.ts. The fixture guards against a Yarr JIT bug where /\B|x{1,2}?/u.exec("a\u{10ffff}b")[0].length returned 4294967295 on ARM64 due to a match-start/index desync in the BodyAlternativeEnd backtrack trampoline. The underlying fix already landed via a WebKit bump (#33133); this PR only adds the regression coverage. No src/** changes.

Security risks

None. This is a pure test addition — a fixture file executed only by the test runner in a spawned subprocess, plus a two-line list entry. It touches no runtime, auth, crypto, network, or filesystem code paths.

Level of scrutiny

Low. The change is mechanical and follows the established jsc-stress fixture pattern exactly: a .js fixture in test/js/bun/jsc-stress/fixtures/ that throws on failure, driven by testLoopCount from preload.js (verified present), and registered in the existing jsFixtures list under a new // Yarr JIT comment group. The fixture is self-contained, uses no I/O, and asserts simple invariants (m[0].length <= input.length, m.index + m[0].length <= input.length) that cannot be flaky.

Other factors

The PR description documents CI-verified fail-before (build 52790, all aarch64 lanes hit the 4294967295 underflow) and pass-after on the current WebKit, satisfying the "prove the test fails for the right reason" requirement. The fixture covers multiple pattern shapes (delta==1, delta==2, matching second alternative, /mu disabling the optimization) and includes upstream's own reduction from webkit.org/b/312604. No bugs were found by the bug-hunting system, and there are no prior reviewer comments or outstanding requests on the timeline.

@robobun

robobun commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator Author

Stale PR review: closing.

The bug is fixed, and this fixture no longer reaches the code it names. The fix (WebKit/WebKit@116394de195b) reached main with the WebKit upgrade in #32414 (91ce3b4), not with #33133 as the fixture header says. At Bun's current pin (oven-sh/WebKit@299c5323879e), YarrJIT.cpp:9398 enables the ARM64 non-BMP first-character optimization only when the pattern body has a non-zero minimum size. Every pattern in this fixture has a zero-width \B alternative, so on aarch64 the fixture runs the same code as on x64. Upstream's regression test, JSTests/stress/regexp-boundary-assertion-with-surrogate.js, is in the fork at that pin, and the fork CI runs JSTests on linux arm64 since oven-sh/WebKit#645. #40231 is open and adds its own Yarr JIT group to the same jsFixtures list in jsc-stress.test.ts.

Reopen if this evidence is wrong.

@robobun robobun closed this Sep 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant