Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughYarr now compiles alternatives whose required input exceeds representable string lengths as non-matching paths instead of rejecting them for offset overflow. The PR adds regression coverage and updates existing test expectations and documentation. ChangesRegExp Required-Input Limits
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
|
Status: ready for review.
|
|
Preview build of c9cdf23: |
There was a problem hiding this comment.
I reviewed this PR and did not find any bugs, but a human look is still worthwhile: it changes RegExp semantics in the YARR pattern builder and byte compiler (SyntaxError becomes a never-matching path), diverges six upstream tests from WebKit, and the description itself leaves the design choice between this approach and #835 open.
What was reviewed:
- The two new
cannotMatchFromHere()paths inemitDisjunctionand thecannotMatch()state set bysetupDisjunctionOffsets:HaveCheckedInput(UINT_MAX)cannot pass (pos is at most String::MaxLength) andInput::checkInputguards wrap, so the pair fails on any input; thecontinues emit nothing before the pair and skip only the trailinguncheckInput, which was not needed. - Whether a cleared alternative can reach the JIT: it gets
m_minimumSize = UINT_MAX, which setsm_containsUnsignedLengthPatternat any nesting level, so the JIT-sidem_saveInitialStartValueslot and the new JIT asserts are not reached; a fixed-count group of cleared alternatives overflows the enclosing position via the new check, except at position 0 where the sum stays exactly UINT_MAX and the existingCheckInput/offset arithmetic still holds. - Negative lookbehind dropped on overflow: correct, since its body can never match and its captures are undefined on success anyway; the new test covers interpreter, JIT, and eager-construction modes.
Extended reasoning...
The change touches Source/JavaScriptCore/yarr (YarrPattern.cpp, YarrInterpreter.cpp, YarrJIT.cpp, YarrPattern.h), an OptionsList.h description string, six upstream regexp tests re-pinned to the new behavior, a 369-line new stress test, and the BUN-TEST-DIFFERENCES.md and CLAUDE.md count bookkeeping. It touches no auth, injection, or data-exposure surface; the risk is engine correctness in bytecode generation for overflowing input offsets. It is not approvable automatically because the paths are owned by a CODEOWNERS entry (/Source/JavaScriptCore, /JSTests, /LayoutTests/js), the PR is an intentional semantic divergence from upstream WebKit with an explicitly unsettled alternative design (#835), and the control-flow changes in emitDisjunction (early continues and a switch break) require judgement about the pre-existing offset invariants that the remaining OffsetTooLarge exits rely on.
…s, the pattern is valid The jsc-stress fixture is JSTests/stress/regexp-required-input-beyond-string-length.js of oven-sh/WebKit#864 without its run directives. The URLPattern and structuredClone cases are places where Bun passed the RegExp's SyntaxError on.
autobuild-preview-pr-864-cfa7f07f: a RegExp path that needs more input than a string can hold fails its match, and the pattern stays valid.
cfa7f07 to
5c37134
Compare
…s, the pattern is valid The jsc-stress fixture is JSTests/stress/regexp-required-input-beyond-string-length.js of oven-sh/WebKit#864 without its run directives. The URLPattern and structuredClone cases are places where Bun passed the RegExp's SyntaxError on. The bun test -t, Bun.build plugin filter and bun lint id-match cases are places where Bun compiles a pattern with Yarr itself, with no RegExp object.
autobuild-preview-pr-864-fc3e289f: a RegExp path that needs more input than a string can hold fails its match, and the pattern stays valid.
fc3e289 to
dbd0713
Compare
…ring can hold
ByteCompiler::emitDisjunction() sums, in checked 32-bit integers, the input
that has to be there for an alternative to match. When a sum passed UINT_MAX
it refused the whole pattern: RegExp::compile() stored OffsetTooLarge in the
RegExp, and every match threw "SyntaxError: Invalid regular expression:
pattern exceeds string length limits". new RegExp had accepted the pattern.
const re = /(?:a|b*)+?c(?<!(?<=(?<=x{2147483647})y{2147483647})z{2147483647})/;
re.test("ac"); // true, from JIT code
re.test("c"); // SyntaxError: the JIT code gives this match to the interpreter
re.test("ac"); // SyntaxError
new RegExp(re.source); // SyntaxError, while that RegExp is in RegExpCache
/(?:z)?x{4294967295}/.test("") // SyntaxError. V8: false
/(?<!a{4294967295})b/.test("b") // SyntaxError. V8: true
A sum that does not fit asks for more input than a string can hold
(String::MaxLength), so the alternative it belongs to can never match. The
other alternatives can. emitDisjunction() now emits two checks that no input
satisfies together in place of such an alternative, and in place of a
positive lookbehind that reaches back more than UINT_MAX. A negative
lookbehind of that kind always holds, so nothing is emitted for it.
The other OffsetTooLarge exits of emitDisjunction() guard the subtraction of
a term's position from the input checked for it, and one re-add of an amount
just subtracted. They cannot overflow and stay. ByteCompiler::compile()
asserts that none of them fires.
What this leaves: the pattern build (setupAlternativeOffsets) still refuses a
pattern whose offsets pass UINT_MAX. That is at creation, and at the first
match for a pattern over 64 characters, which is built there
(useLazyRegExpPatternConstruction): "x".repeat(80) + "y{4294967295}" is
created, throws at its first match, and the error stays in the RegExp.
* CLAUDE.md:
* JSTests/BUN-TEST-DIFFERENCES.md: A10.
* JSTests/stress/regexp-required-input-beyond-string-length.js: Added.
* JSTests/stress/regexp-bytecode-compilation-fail.js:
* JSTests/stress/regexp-test-minimum-length-filter.js:
* Source/JavaScriptCore/yarr/YarrInterpreter.cpp:
…han a string can hold
YarrPatternConstructor::setupAlternativeOffsets() gives each term its input
offset in a checked 32-bit integer. When an offset passed UINT_MAX it refused
the whole pattern with "SyntaxError: Invalid regular expression: pattern
exceeds string length limits": at new RegExp, or at the first match for a
pattern over 64 characters, which is built there.
new RegExp("a{4294967295}b") // SyntaxError. V8: created
new RegExp("c|a{4294967295}b").exec("c") // SyntaxError. V8: ["c"]
new RegExp("(?:a{4294967295}b)?c").exec("c") // SyntaxError. V8: ["c"]
The offset is the input between the start of the enclosing alternative and
the term, so the alternative can never match, and the other alternatives
can. setupDisjunctionOffsets() now takes OffsetTooLarge from
setupAlternativeOffsets() as a statement about that alternative: it clears
the terms, which have no usable offsets, and sets the minimum size to
UINT_MAX. PatternAlternative::cannotMatch() names that state, and
ByteCompiler::emitDisjunction() emits its always-failing checks for it.
An alternative that has to match a group of such alternatives adds UINT_MAX
to its own position and overflows in turn. That sum (a group with a fixed
count of one) had no overflow check. It needed none while an overflow in the
group was an error.
A minimum size over INT_MAX sets m_containsUnsignedLengthPattern, so such a
pattern runs in the interpreter, as before. The Yarr JIT asserts that it
never gets one.
Not covered: YarrParser still refuses a quantifier minimum of 2^64 - 1 or
more ("number too large in {} quantifier").
* CLAUDE.md:
* JSTests/BUN-TEST-DIFFERENCES.md: A10.
* JSTests/stress/regexp-required-input-beyond-string-length.js:
* JSTests/stress/regexp-max-size.js:
* JSTests/stress/regress-159954.js:
* LayoutTests/js/regexp-unicode-expected.txt:
* LayoutTests/js/script-tests/regexp-unicode.js:
* Source/JavaScriptCore/runtime/OptionsList.h:
* Source/JavaScriptCore/yarr/YarrInterpreter.cpp:
* Source/JavaScriptCore/yarr/YarrJIT.cpp:
* Source/JavaScriptCore/yarr/YarrPattern.cpp:
* Source/JavaScriptCore/yarr/YarrPattern.h:
…first call is test() The table ran exec() and then test() on each RegExp, and then created the same text again. RegExpCache returns the same RegExp for the same text and flags, so no pattern of the table was compiled for test() first. The table now also creates each pattern with the d flag, which is another RegExp, and calls test() on it before exec(). The comments now say what the second creation of the same text is for: an error that a match left in the shared RegExp made that creation throw. * JSTests/stress/regexp-required-input-beyond-string-length.js:
…T too The test ran with the JIT only: the byte compiler refused six of its thirteen patterns with "pattern exceeds string length limits". It does not refuse them any more, so the test also runs with --useRegExpJIT=0 and with --useLazyRegExpPatternConstruction=0. * JSTests/stress/regexp-nested-quantifier-checked-offset.js:
dbd0713 to
c9cdf23
Compare
…s, the pattern is valid The jsc-stress fixture is JSTests/stress/regexp-required-input-beyond-string-length.js of oven-sh/WebKit#864 without its run directives. The URLPattern and structuredClone cases are places where Bun passed the RegExp's SyntaxError on. The bun test -t, Bun.build plugin filter and bun lint id-match cases are places where Bun compiles a pattern with Yarr itself, with no RegExp object.
autobuild-preview-pr-864-c9cdf235: a RegExp path that needs more input than a string can hold fails its match, and the pattern stays valid.
Replaces the length commit 692d32d of #835. To decide: remove this error (this PR), or report it at creation (#835).
Problem
/(?:a|b*)+?c(?<!(?<=(?<=x{2147483647})y{2147483647})z{2147483647})/answerstest("ac")withtrue. Aftertest("c")the same call throwsSyntaxError: Invalid regular expression: pattern exceeds string length limits, and so doesnew RegExpof that text.setupAlternativeOffsets(yarr/YarrPattern.cpp:2184) andByteCompiler::emitDisjunction(yarr/YarrInterpreter.cpp:2781) refuse the pattern when a checked 32-bit input offset overflows. In V8/c|a{4294967295}b/.exec("c")is["c"].Fix
JSTests/stress/regexp-required-input-beyond-string-length.jsfails on main. Also the regexp stress tests, test262 RegExp and a differential fuzz against node.Background
setupAlternativeOffsetsgives each term an input offset.emitDisjunctionsums the input each alternative needs. A string holds at most 2^31 - 1 code units.RegExpCache(the match still throws).Downsides
JSTests/BUN-TEST-DIFFERENCES.md, A10).Notes
Relation to #835, #862, oven-sh/bun#46098 and oven-sh/bun#46347
vMayContainStrings follows ECMA-262 #835 has two halves. Its parser commit 5bfa31b (vclass sets) is not affected by this PR.new RegExp. This PR removes the error, so that commit has nothing left to report: itsSyntaxSummary::mayExceedOffsetLimitsmachinery, itstooLongrows inJSTests/stress/regexp-lazy-pattern-construction.jsand the length family of its generated test pin the opposite result for the same patterns. RegExp: report two pattern-build errors at creation again, fixvMayContainStrings (WebKit bump for oven-sh/WebKit#835) bun#46098 has a URLPattern row and fixture rows of the same kind.vMayContainStrings follows ECMA-262 #835 merges first with that commit, this PR removes the machinery and changes those rows. If the answer is to keep JSC's limit, the second commit of this PR goes and the first one still ends the late error for patterns built at creation.regexp-nested-quantifier-checked-offset.jsran with the JIT only: "Only with the JIT: the interpreter rejects some of these patterns." With--useRegExpJIT=0, main throws this error for 6 of its 13 rows. The fourth commit of this PR gives that test the other two modes.bun test -t, aBun.buildplugin filter and abun lintrule option.JSTests/BUN-TEST-DIFFERENCES.md. If another PR that adds an A10 lands first, this one takes the next number.What changes for a user
test("ac"),test("c"),test("ac")true, SyntaxError, SyntaxErrortrue,true,truetrue,true,truenew RegExp("a{4294967295}b")/c|a{4294967295}b/.exec("c")["c"]["c"]/(?:z)?x{4294967295}/.test("")falsefalse/(?<!a{4294967295})b/.test("b")truetruenew RegExp(sameText)after such a match/(?:){4294967295}c|a{4294967295}b/.exec("c")null, the search is abandoned["c"]/a{18446744073709551615}b|k/.exec("k")number too large in {} quantifier)["k"]In Bun the same error shows in three more places:
structuredClone(re)throwsTypeError: Unable to deserialize data.after such a match,new URLPatternwith such a group throws a TypeError, andbun test -t 'first|a{4294967295}b'exits with "expects a valid regular expression". All three come from the RegExp's error.Of V8's
test/mjsunit/regress/regress-126412.js(six patterns with such counts), four throw on main. With this PR all six give node's result.Why a path that overflows cannot match
setupAlternativeOffsetsthe position that overflows is the number of code units between the start of the enclosing alternative and a term. InemitDisjunctionthe sum that overflows is the input that was checked to be there plus the minimum of the alternative, or the distance a lookbehind reaches back.UINT_MAXcannot be there.built-ins/RegExp/quantifier-integer-limit.jshasb{9007199254740991}. It passed before, because a lone term does not overflow.b{9007199254740991}cdid not.The commits
YarrInterpreter.cpp.emitDisjunctionemitsHaveCheckedInput(UINT_MAX)thenCheckInput(UINT_MAX)and no terms for an alternative whose sum overflows, and for a positive lookbehind that reaches back too far. The first check holds only withUINT_MAXcode units behind the position and the second only withUINT_MAXahead, so the pair fails on every input. A negative lookbehind of that kind always holds, so nothing is emitted. The other 13OffsetTooLargeexits are subtractions of a term position from the checked count and one re-add. They cannot overflow, stay as they are, andByteCompiler::compilenow asserts that none fires.Alone, this commit ends the two answers of one RegExp and every late error of a pattern that is built at creation. It leaves the pattern build's refusal: at creation, and at the first match for a pattern over 64 characters (
"x".repeat(80) + "y{4294967295}").YarrPattern.cpp.setupDisjunctionOffsetstakesOffsetTooLargefromsetupAlternativeOffsetsas "this alternative". It clears the terms, which have no usable offsets, and sets the minimum size toUINT_MAX.PatternAlternative::cannotMatch()names that state andemitDisjunctionemits the failing pair for it. An alternative that has to match a group of such alternatives addsUINT_MAXto its position and overflows in turn. That sum (a group with a fixed count of one) had no overflow check. It needed none while an overflow inside the group was an error.A minimum size over
INT_MAXsetsm_containsUnsignedLengthPattern, so the pattern runs in the interpreter and never reaches the JIT. That gate is older than this PR. Two debug assertions at the JIT entry points state it.test(). Thedflag gives that RegExp: the same text with the same flags comes fromRegExpCache, which returns the RegExp that already ran.regexp-nested-quantifier-checked-offset.js, the test of [JSC] Fix Yarr JIT abort when nested alternatives claim more than INT_MAX characters #862, also runs with--useRegExpJIT=0and with--useLazyRegExpPatternConstruction=0.Runs (jsc of 5ce9596 and of its base 274ebb3, same flags: release, assertions on. node v26.3.0 is the reference. The generator and the three-way driver are not in the tree. The branch moved since onto 7961839: the upstream sync #860, #862, #807 and #881. The RegExp changes in them are the JIT fix of #862 and upstream's fast path for
\r?\n. The diff inSource/is the same. With the CI build of this head, c9cdf23, the new test, the test of #862 and 7 related stress tests pass in the three modes.)--useLazyRegExpPatternConstruction=0--useRegExpJIT=0,--useLazyRegExpPatternConstruction=0, 200,000 each): this errorJSTests/stressfiles with regexp, regex or yarr in the name, every mode,--memory-limitedas in CI (5,603 runs)LayoutTests/jsandfast/regex, regexp tests (364 runs)built-ins/RegExp,language/literals/regexp, the String methods that take a RegExp, sloppy and strict (5,066 runs)/uclass that matches a code point outside the BMP before$:/[\u{10235}]b$/udoes not match"a\u{10235}b"in node. One comes from another:/(?:[^]+?x^|a?)+^/does not match"a"in node. One is a search that Yarr abandons at its match limit after 9 seconds, where node finds the match in 1 second./umatch that would start between the halves of a surrogate pair (A6 ofBUN-TEST-DIFFERENCES.md).number too large in {} quantifier, and node matches a subject for 8,276 of them. The same on the base.Cost (instructions counted by single-stepping the function, for 982 regex literals from TypeScript, Prettier, React and Bun's built-in modules)
Yarr::byteCompile, per compile (983 compiles, JIT off)YarrPatternbuild, per build (474 builds of 160 literals)RegExp::match,RegExpCache,RegExp::finishCreationByteCompiler::emitDisjunction, bytes (jsc, -O3, no LTO)setupDisjunctionOffsets, bytes/(?:z)?x{4294967295}/, 100test("")calls: pattern builds / byte compiles / throwsNot in this PR
YarrParser.hrefuses a quantifier minimum of 2^64 - 1 or more (QuantifierTooLarge). V8 accepts it. The parser cannot order two saturated counts, so that is its own change, andstress/regexp-max-quantifier.jsandLayoutTests/fast/regex/overflowstay as upstream has them./(?:){4294967295}b/.exec("b")isnull). A pattern that was refused can now reach it. [JSC] A RegExp search that Yarr abandons throws a RangeError #343 turns that result into a RangeError.regular expression too large(!isSafeToRecurse()atYarrPattern.cpp:1965,:2035and:2666on main), and avclass set over 64 characters can still be refused at its first match ([JSC] Lazy RegExp creation reports two YarrPattern build errors at creation again, andvMayContainStrings follows ECMA-262 #835's parser commit).new RegExp("c|a{4294967295}b")at creation and/(?:z)?x{4294967295}/.test("")at the match as well. It does not have the two answers of one RegExp.