Give JavaScript strings a maximum length instead of a wrapped array rent - #3015
Merged
Merged
Conversation
Collaborator
Author
|
Not ready to merge despite green CI — the benchmark gate has not been run. This PR adds a length check to paths that are hot: Per the repo's standing rule, this needs full SunSpider + Dromaeo tables at the default job on an idle machine, with >1% on any row blocking. Rows to watch: SunSpider Holding until the maintainer schedules the run; the tables will be posted here. |
Collaborator
Author
Benchmark gate - BLOCKEDTwo full pairs, default job, serial on an otherwise idle machine. Base pinned to A row counts as a regression only if it exceeds +1% in both pairs. This machine's run-to-run envelope reached about +/-10% on the Dromaeo rows and 23 of 50 rows changed sign between the two pairs, so neither pair proves anything on its own.
Four rows regress in both pairs, and three of them are string rows
That is a coherent pattern rather than four survivors of a coin toss, and there is a mechanism for it. The mechanism: a virtual
|
Collaborator
Author
Benchmark gate - PASS (after 132b00b)
The three regressions this PR had are goneFive pairs for the SunSpider rows, two full pairs for Dromaeo. Median is across all pairs.
Full SunSpider table, five pairs
Full Dromaeo table, two pairs
What still shows up, and why it is not this PRThree rows exceed +1% in a majority of the five pairs: None of them touches a string. After They are also what chance predicts at this machine's envelope: the run-to-run spread reached about ±10% on the Dromaeo rows, and across 26 SunSpider rows and five pairs, three rows clearing 3/5 is the expected count rather than a surprise. Worth stating plainly that a strict >1% single-pair gate is not enforceable on this hardware — a row is only trustworthy here when repeated pairs agree and a mechanism exists. Allocation columns stayed byte-exact throughout, before and after the fix. |
ValueStringBuilder.Grow computed its required size (_pos + additionalCapacityBeyondPos) as an unchecked int. Past 2^31 characters that wraps negative, the (uint) -> Math.Max -> (int) round-trip carries the wrapped value through, and ArrayPool<char>.Shared.Rent(-2147483648) throws an ArgumentOutOfRangeException that escapes Evaluate past every catch in the script. The ClrLimits.MaxArrayLength cap in the same expression bounds only the doubling term, never the required size. Adopt V8's limit, (1 << 29) - 24 = 536,870,888 characters, as JsString.MaxLength and guard the paths that build a string from JavaScript with a catchable RangeError: Invalid string length. repeat and padStart/padEnd move from the CLR array ceiling to it; GetSubstitution, the @@replace accumulator, replace, replaceAll, join, toLocaleString, String.raw, String.prototype.concat, '+'/'+=' and template literals gain one they never had. Grow itself stays realm-free - it also backs the JSON serializer, URI encoding and the Intl/Temporal formatters - and only stops lying about the size. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The length guard read jsString.Length before calling Append. JsString.Length is virtual and ConcatenatedString overrides it with a two-branch null coalesce, so every `s += t` paid a dispatch it had never paid before - the pre-fix path called Append(rprim) and let the override do the coercion internally, reading no length at all. It showed up in the gate. Against ff980b7, three string rows regressed in both of two pairs: Dromaeo ObjectString [Modern=True&Prepared=True] +7.50/+1.80, SunSpider string-base64 +2.22/+1.25 and string-fasta +1.31/+2.64. SunSpider's string-base64 and string-fasta are that loop and almost nothing else. Both Append overrides already hold the field that answers the question - the base calls ToString() anyway, and ConcatenatedString reads _stringBuilder or _value on the very next line - so moving the check inside costs an add and a compare, no dispatch. The realm becomes a parameter, which is two field loads in place of a virtual call. StringPrototype.Concat drops its own pre-check for the same reason. Re-measured over five pairs: string-fasta -0.64% median, string-base64 +0.34%, 3d-raytrace -0.72%, ObjectString back to noise. No string row regresses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sebastienros#3016 landed the staging/ directory while this branch was still open, so it had to park the file this issue was found through. The fix is here, so the exclusion goes: 102,324 passed / 0 failed with it removed, and the file itself passes in both strict and sloppy mode. It is the upstream coverage for exactly this defect - a substitution whose result exceeds the maximum string length - and it is the reason the issue could be written with a reproduction at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lahma
force-pushed
the
fix/string-length-limit
branch
from
August 15, 2026 09:44
132b00b to
47a00b4
Compare
This was referenced Aug 15, 2026
lahma
added a commit
to lahma/jint
that referenced
this pull request
Aug 15, 2026
sebastienros#3015 gave JavaScript strings a maximum length, JsString.MaxLength = (1 << 29) - 24, and guarded every path that builds one with a catchable RangeError: Invalid string length. JSON.stringify was not in that set, so it went on producing strings the engine's own rule forbids: var a = new Array(200000).fill("x".repeat(4000)); JSON.stringify(a).length; // 800600001, and 800600001 > 536870888 Anything that later concatenates that string correctly refuses it, so the inconsistency is observable, and V8 answers RangeError for the same input. Bigger inputs did not merely produce an over-long string: an array whose length alone puts the document out of reach ran until the builder hit the CLR array ceiling and then died in the finally below, an OutOfMemoryException escaping Evaluate past every catch in the script. The guard goes where this walk accumulates - once per array element, once per object key (both the generic and the shaped path) and before a string, a raw JSON chunk or a host SerializeToJson result is copied in - so an over-long document is refused while it is being built rather than after the whole cost has been paid. It does not go into ValueStringBuilder, which also backs URI encoding and the Intl/Temporal formatters and has no realm to raise a JavaScript error into; sebastienros#3015 left that split in place deliberately and this follows it. Every estimate the guard is handed is a lower bound on the finished document, so it can never refuse one that would have fit. The array bound is what makes a hopeless array fail immediately instead of after half a billion characters: an array announces its length, and every index contributes at least two characters - a separator plus at least one character of value text, an element with no JSON representation being written as null. Object members carry no such bound, because a member whose value has no representation rewinds the document, so those sites check only what is already final. Serialize's finally ended in json.ToString(), which on the throwing path materialized the whole partial document - the very allocation the guard exists to avoid, and where the OutOfMemoryException above was actually thrown. It disposes now; ToString() disposes on the way out, so the success path is unchanged. The IBufferWriter overloads keep the ceiling they always had. Their result is bytes a host consumes and never a JsString, so the language's string limit has no business bounding them. JSON.parse needs nothing: a reviver that concatenates does it through the ordinary string-building paths, which sebastienros#3015 guards, and the RangeError reaches the script's catch through the parse machinery unwrapped. Verified with both 'x'.repeat(...) and a + b past the limit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lahma
added a commit
that referenced
this pull request
Aug 15, 2026
#3028) #3015 gave JavaScript strings a maximum length, JsString.MaxLength = (1 << 29) - 24, and guarded every path that builds one with a catchable RangeError: Invalid string length. JSON.stringify was not in that set, so it went on producing strings the engine's own rule forbids: var a = new Array(200000).fill("x".repeat(4000)); JSON.stringify(a).length; // 800600001, and 800600001 > 536870888 Anything that later concatenates that string correctly refuses it, so the inconsistency is observable, and V8 answers RangeError for the same input. Bigger inputs did not merely produce an over-long string: an array whose length alone puts the document out of reach ran until the builder hit the CLR array ceiling and then died in the finally below, an OutOfMemoryException escaping Evaluate past every catch in the script. The guard goes where this walk accumulates - once per array element, once per object key (both the generic and the shaped path) and before a string, a raw JSON chunk or a host SerializeToJson result is copied in - so an over-long document is refused while it is being built rather than after the whole cost has been paid. It does not go into ValueStringBuilder, which also backs URI encoding and the Intl/Temporal formatters and has no realm to raise a JavaScript error into; #3015 left that split in place deliberately and this follows it. Every estimate the guard is handed is a lower bound on the finished document, so it can never refuse one that would have fit. The array bound is what makes a hopeless array fail immediately instead of after half a billion characters: an array announces its length, and every index contributes at least two characters - a separator plus at least one character of value text, an element with no JSON representation being written as null. Object members carry no such bound, because a member whose value has no representation rewinds the document, so those sites check only what is already final. Serialize's finally ended in json.ToString(), which on the throwing path materialized the whole partial document - the very allocation the guard exists to avoid, and where the OutOfMemoryException above was actually thrown. It disposes now; ToString() disposes on the way out, so the success path is unchanged. The IBufferWriter overloads keep the ceiling they always had. Their result is bytes a host consumes and never a JsString, so the language's string limit has no business bounding them. JSON.parse needs nothing: a reviver that concatenates does it through the ordinary string-building paths, which #3015 guards, and the RangeError reaches the script's catch through the parse machinery unwrapped. Verified with both 'x'.repeat(...) and a + b past the limit. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 23, 2026
legrab
added a commit
to legrab/pocok
that referenced
this pull request
Aug 25, 2026
Updated [Jint](https://github.com/sebastienros/jint) from 4.16.0 to 4.16.1. <details> <summary>Release notes</summary> _Sourced from [Jint's releases](https://github.com/sebastienros/jint/releases)._ ## 4.16.1 Jint 4.16.1 is the **first release from the new `4.x` maintenance branch**, and it marks the point where the two lines separate: `main` is now **5.0.0 development**, and `4.x` is where the 4.16.x line continues. **What that means for you.** If you are on 4.16.0, this is a drop-in update — it is correctness and conformance work only, **no API change and no changed default**. Every public signature is the same one 4.16.0 shipped, on all five target frameworks. If you want the 4.x line, take it from `4.x` and expect fixes rather than features. If you want to follow where the engine is going, watch `main` — v5 brings breaking API changes, an opt-in WHATWG web API surface, Web Workers, Node compatibility and a raised .NET Framework floor, and every one of them is recorded as it lands in [`docs/v5-migration.md`](https://github.com/sebastienros/jint/blob/main/docs/v5-migration.md). From this release onward the 4.x public surface is snapshotted per target framework in `Jint.Tests.PublicInterface/Verify/`, so "did the API move?" is a diff rather than a judgement call — on this branch a diff there is a bug, and comparing those files against `main`'s is the v4→v5 delta. ### Highlights **Conformance, from a suite that now runs more of test262.** The `staging/` directory is generated and executed for the first time (#3016), which is roughly 2,800 additional cases — largely SpiderMonkey's own suite contributed upstream, covering behaviour the stable directories never reach. Much of the work below is what it found. **Built-ins do what the spec says, step by step.** The array built-ins perform the internal methods they name rather than equivalents (#3066); `Array.from` honours `IsConstructor` and a typed array's `length` write throws (#3043); an array truncation walks downwards and the generics report the writes they fail (#3072); argument validation and evaluation order are corrected in five built-ins (#3069); `Map` and `Set` get the `[[SetData]]` tombstone their traversals are specified over (#3073); `Date.prototype.setTime` stores the clipped time value (#3042); and `Array.prototype.values`/`keys`/`entries` no longer gate on an array-like receiver (#3236). **Iterators and control flow.** A throw from the iterator step no longer closes the iterator (#3047); the `done` flag is consulted before stepping again (#3048); a rejected `return()` propagates out of an abandoned `for await` loop (#3113); an optional-chain short circuit is distinguished from a genuine `undefined` (#3040); a computed property key is evaluated even when spelled as a literal (#3039) and survives an `await` or `yield` intact (#3144, #3150); and destructuring the rest of an exhausted array yields an empty array rather than 2³² elements (#3263). **Numeric and string accuracy.** `Math.acosh`, `asinh`, `atanh`, `cbrt`, `expm1` and `log1p` are ported from fdlibm for correctly-rounded results across every target framework (#3050); `toFixed` formats from the double's exact value and reads `this` from `[[NumberData]]` (#3071); `String.prototype` case conversion derives from Jint's own Unicode tables rather than the host's culture data (#3068); and the regex engine is chosen per subject, with `RegExp.prototype.replace` no longer rewriting `lastIndex` (#3070). **Bounds that hold.** JavaScript strings have a maximum length instead of a wrapped array rent (#3015); a JSON document too long to become a string is refused while it is being built (#3028); a frame displaced by a proper tail call keeps counting while its trampoline runs, so `MaxRecursionDepth` cannot be evaded by leaving and re-entering the trampoline (#3022); and an `Atomics` waiter is released when nothing can ever notify it again (#3029). **Error messages no longer run user JavaScript** (#3041) — rendering a message for a value with a script-supplied `toString` used to invoke it, from inside the failure path. **Internationalization.** The five Temporal members the proposal removed are dropped (#3014), and `u`-extension options are canonicalized with every date format the spec allows (#3018). Two fixes in this release come from **@svenrog** — a sloppy function answering its own `arguments` (#3061) and the outer link on a parked `Function`-constructor environment (#3063). ## What's Changed * Drop the five Temporal members the proposal removed by @lahma in sebastienros/jint#3014 * Canonicalize u-extension options and format every date the spec allows by @lahma in sebastienros/jint#3018 * Mark a global created by an unresolvable assignment, and stop a waitAsync timeout outliving its engine by @lahma in sebastienros/jint#3019 * Run test262's staging/ directory too by @lahma in sebastienros/jint#3016 * Give JavaScript strings a maximum length instead of a wrapped array rent by @lahma in sebastienros/jint#3015 * Let a for-of frame decline the unwind it can only rethrow by @lahma in sebastienros/jint#3017 * Keep counting a frame a tail call replaced while its trampoline runs by @lahma in sebastienros/jint#3022 * Unpark staging/Temporal/removed-methods.js, which #3014 already fixed by @lahma in sebastienros/jint#3023 * Drop the Islamic date conversions no calendar path reaches by @lahma in sebastienros/jint#3027 * Let an Atomics waiter go when nothing can ever notify it again by @lahma in sebastienros/jint#3029 * Refuse a JSON document too long to be a string while it is being built by @lahma in sebastienros/jint#3028 * Bump the microsoft group with 3 updates by @dependabot[bot] in sebastienros/jint#3033 * Bump the analyzers group with 1 update by @dependabot[bot] in sebastienros/jint#3031 * Add initial threat model for untrusted scripts by @sebastienros in sebastienros/jint#3030 * Stop ClassBenchmark rebuilding its engine per iteration by @lahma in sebastienros/jint#3053 * createRealm installs a full $262 on the new realm and returns it by @lahma in sebastienros/jint#3044 * Give the benchmark suite a measurement environment by @lahma in sebastienros/jint#3055 * Evaluate a computed property key even when it is spelled as a literal by @lahma in sebastienros/jint#3039 * Stop error messages from running user JavaScript by @lahma in sebastienros/jint#3041 * Array.from honours IsConstructor, and a typed array's length write throws by @lahma in sebastienros/jint#3043 * Consult the iterator's done flag before stepping it again by @lahma in sebastienros/jint#3048 * Date.prototype.setTime must store the clipped time value by @lahma in sebastienros/jint#3042 * Answer a sloppy function's own arguments instead of throwing by @svenrog in sebastienros/jint#3061 * Keep the outer link on a parked Function-constructor environment by @svenrog in sebastienros/jint#3063 * A throw from the iterator step must not close the iterator by @lahma in sebastienros/jint#3047 ... (truncated) Commits viewable in [compare view](sebastienros/jint@v4.16.0...v4.16.1). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details>
This was referenced Aug 26, 2026
This was referenced Aug 27, 2026
This was referenced Sep 6, 2026
This was referenced Sep 13, 2026
This was referenced Sep 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ValueStringBuilder.Growcomputes the size it needs as_pos + additionalCapacityBeyondPos, in uncheckedint. Past 2³¹ characters that sum wraps negative; the(uint)→Math.Max→(int)round-trip carries the wrapped value straight through, andArrayPool<char>.Shared.Rent(-2147483648)throws. TheClrLimits.MaxArrayLengthcap sitting in that very expression bounds only the doubling term — never the required-size term — so nothing stopped it.The result is a CLR exception escaping
engine.Evaluatepast everycatchin the running script:That is the new
ReplaceMathTargetsMoreCharactersThanAStringCanHoldtest run against the unfixed engine — the script wraps the call intry/catchand the exception goes straight past it. Three sibling rows reportedSystem.OutOfMemoryExceptionthe same way;repeat/padStart/padEnddid not throw at all and quietly built ~1 GB strings.V8 answers
RangeError: Invalid string length, which a script can handle.The limit
JsString.MaxLengthis V8'sString::kMaxLength,(1 << 29) - 24= 536,870,888 characters, so a script that builds too large a string now fails identically on both engines.Three layers
1.
Growcan no longer wrap. The required size is computed as along, and a size no array could hold reports as theOverflowExceptionit is. No JavaScriptRangeErrorat this level: the builder also backs the JSON serializer, URI encode/decode and the Intl/Temporal formatters (~40 call sites), none of which has a realm to throw into. Its job here is only to stop lying about the size.2. The JavaScript-semantic paths throw
RangeError: Invalid string length— a message already in use — checked before the piece that would take the result past the limit is appended, so a rejected build never first allocates what it is about to refuse:String.prototype.repeatStringPrototype.RepeatClrLimits.MaxArrayLength→ retargetedpadStart/padEndStringPrototype.StringPadClrLimits.MaxArrayLength→ retargetedGetSubstitutionRegExpPrototype.GetSubstitution@@replaceaccumulatorRegExpPrototype.Replacestringconcat → OOM)String.prototype.replacetailStringPrototype.ReplaceString.prototype.replaceAllStringPrototype.ReplaceAllArray.prototype.joinArrayPrototype.JoinArray.prototype.toLocaleStringArrayPrototype.ToLocaleString+/+=JintBinaryExpression.ApplyAdditionToPrimitives,AdditionChainExpression.EvaluateWithoutSuspension/ConcatAll,JintAssignmentExpression.ComputeCompoundString.prototype.concatStringPrototype.ConcatJintTemplateLiteralExpressionString.rawStringConstructor.Raw3.
GetSubstitutionalso gets an up-front estimate. A replacement pattern expands without bound ("$&$&$&…"repeats the match once per token), so a per-append check alone would only fire after the builder already held half a billion characters — a gigabyte, allocated purely to be discarded, which is exactly what the reported repro does.MinimumSubstitutionLengthsums the token contributions that are knowable without running user code ($&, the two context tokens,$n/$nn) and counts everything else as zero. That is an exact lower bound, so it can never over-count and never refuse a substitution that fits; it just lets the pathological case be refused while the builder is still empty, which is also what V8 does.$<name>is the one token deliberately skipped, because resolving it runs aGetthat must not run twice; the per-append checks remain the exact enforcement and cover it. This estimate is what makes the tests below cost a few megabytes rather than a gigabyte.The realm parameter on
GetSubstitutionGetSubstitutionisinternal staticwith exactly three callers —RegExpPrototype.Replace,StringPrototype.Replace,StringPrototype.ReplaceAll— and all three are instance methods holding_realm, so it simply takes aRealmnow. NoThrow.CreateRangeError/ErrorDispatchInfosentinel: that exists for genuinely realm-less callers and would be indirection for nothing here.String.prototype.concatwasstaticand needed the same thing, so it became an instance method (the source generator handles both;Repeatnext door is already instance +FastCall).Hot paths touched, and what to watch
The check is a widened add and a compare against a constant. On the interpreter paths the realm is resolved only inside the cold branch (
context.Engine.Realmwalks_realmInConstruction ?? ExecutionContext.Realm), so the warm path never touches it.Methods touched that are genuinely hot:
JintBinaryExpression.ApplyAdditionToPrimitives— the string branch materializes both operands into locals beforestring.Concat(it built the same two strings before, via+), plus one add and one compare. Gains anEvaluationContextparameter, unused on the numeric paths.AdditionChainExpression.EvaluateWithoutSuspensionandConcatAll— one add per operand, one compare per chain; the ≥5-operand paths accumulate the total in the loop that was already filling thestring[].JintAssignmentExpression.ComputeCompound,+=string branch — the coercion moved out ofJsString.Appendto the call site (same coercion, same order), plus one virtualLengthread, one add and one compare.JsString.Append/JsString.ConcatenatedString.Append— signature changed fromAppend(JsValue)toAppend(string); the callers coerce, because they are the ones that need the length before the append.JintTemplateLiteralExpression.EvaluateInternal— one add and one compare per quasi and per interpolation.ArrayPrototype.Join— one add and one compare per element; the element read moved ahead of the separator append so a single check covers both (appending the separator is not observable, so the reordering is not either).RegExpPrototype.GetSubstitution— one extra pass over the replacement pattern (typically a handful of characters), on the$-containing path only.StringPrototype.Concat— static → instance.Benchmark rows worth watching in the SunSpider + Dromaeo gate: string-base64, string-fasta, string-tagcloud, string-unpack-code, string-validate-input, 3d-raytrace and access-nbody (heavy
+), plus Dromaeo's string-base and object-string rows. I have deliberately run no benchmarks — that gate is yours, serially, on an idle machine.Tests
New
Jint.Tests/Runtime/StringLengthLimitTests.cs, including a port ofstaging/sm/String/replace-math.js(staging/is not part of Jint's generated test262 projection, so it lives here — the same precedent asDataViewBoundsTests). Every row asserts the throw is something the script catches: thetry/catchis written in JavaScript, so a CLR exception escaping the engine fails the test as an escaping exception rather than being mistaken for a handled error. Every row is also a path whose limit is decided from small inputs, so nothing allocates 512 MB — the whole class runs in well under a second.A theory of ten substitution patterns that fit pins that the up-front estimate never over-counts (
$$,$&, both context tokens,$1/$12/$0,$<name>with and without named groups, and a$&spelled inside a group name).Updated to pin what is actually enforced:
ExecutionConstraintTests.ShouldThrowRangeErrorWhenPadStartExceedsMaxStringLength,ShouldLimitStringSizeForPadEnd(its 536870911 target is now past the cap, so it usesJsString.MaxLength— still allowed, still built incrementally, still interrupted by the memory limit) andStringTests.RepeatRejectsCountsThatExceedTheMaximumStringLength.All green in Release:
Jint.TestsJint.Tests.PublicInterfaceJint.Tests.CommonScriptsJint.Tests.Test262Closes #3011
🤖 Generated with Claude Code