Repository navigation
Check a shared string's copy against LimitMemory before ConvertResult makes it - #4256
Merged
Merged
Conversation
… makes it Engine.ConvertResult runs as an engine entry, so LimitMemory bounds it even with ResultLimits.Unlimited, the default. But the converter checks constraints once per Engine.ConstraintCheckInterval values (10,000), not per character, and a slice view or a deferred a + b costs the conversion a copy of its whole length. Under LimitMemory(16 MB), 64 slices of one 1,048,576-character string cost the script 2.1 MB and cost ConvertResult 134 MB of copies before the entry's closing check refused it; 10,000 such values would all be copied before the first check. ResultConverter.ConvertString now checks a slice view or deferred concatenation that nothing has read yet against what is left of the budget, by its length, before copying it. It goes through the new internal MemoryLimitConstraint.CheckBeforeCopying, which charges and refuses a copy that would not fit, the way ChargeDeferredConcatenation refuses an over-long node. The same conversion now refuses with 14.7 MB copied. Flat text is handed back without a copy and is never checked, so a host string larger than the whole budget still converts. A host LazyJsString is the host's own allocation and stays on the existing cadence. JsValue.ToObject() and ToString() stay unbounded, as documented since sebastienros#4182. They also amplify shared structure that contains no strings: twenty nested [x, x] arrays become 100 MB on ToObject(). The constraints and untrusted-code guides, the ConvertResult and MemoryLimitConstraint remarks, THREAT_MODEL TM-06/TM-17, migration guide 4.27 and Jint/Constraints/AGENTS.md now say so. Fixes sebastienros#4175 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YVwU18SgMKKzYrrCh4TMpT
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
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.
Fixes #4175
What the issue says, and what I found
Long
sliceviews and longa + bvalues can share one string's characters.LimitMemorycharges each of them only for what it adds, so a host that reads all of them copies the shared text once per value. I reproduced both shapes from the issue. The documentation half of the issue already shipped in #4182, and #4185 madeMaxOutputCharactersrefuse a string by its length before copying it.One gap was left, in the read path the docs recommend.
Engine.ConvertResultruns as an engine entry, soLimitMemorybounds it even withResultLimits.Unlimited, which is the default. But the converter checks constraints once perEngine.ConstraintCheckIntervalvalues (10,000), not per character. Each value here costs a 2 MB copy, so the conversion copied all 64 values before the entry's closing check refused it. With 10,000 such values it would copy 20 GB before the first check.The fix
ResultConverter.ConvertStringnow checks a slice view or deferred concatenation that nothing has read yet against the remaining budget, by its length, before copying it. It calls a new internalMemoryLimitConstraint.CheckBeforeCopying. A copy that would not fit is charged and refused, the same wayChargeDeferredConcatenationrefuses an over-long node. A copy that fits is not charged up front, because the allocation counter sees it once it is made.Flat text is returned without a copy, so it is never checked: a host string larger than the whole budget still converts. A host
LazyJsStringis the host's own allocation and stays on the existing cadence.Measured with a host probe on net10.0 under
LimitMemory(16 MB):ToObject()ConvertResult(Unlimited)beforerope + 'q'over one 1M-char ropesplit()segments of one string[x, x]arrays, no stringsWhat
LimitMemoryalready bounded, all refused under 16 MB before this change:toUpperCase,JSON.stringify) or reading a character of each rope;ToObject()on the values during the run.What stays unbounded, and why
ToObject()andToString()remain unbounded, as documented since #4182. Neither has an operation to charge, and a bareJsStringhas no engine. They also amplify shared structure that contains no strings: in the last control row, twenty nested[x, x]arrays become 100 MB onToObject(). So a fix specific to strings would not make either of them a bounded read.Charging views and ropes for their full logical length when they are built would bring back the quadratic charge they exist to remove. A tokenizer doing
s = s.slice(100)over 1M characters is charged 5.5 MB today and would be charged 11 GB. An accumulator doing 10,000 ×s = s + chunkis charged 3.2 MB and would be charged 10 GB.Cost
Every string
ConvertResultconverts now pays one extra null test on its backing field. A slice view or deferred concatenation that has not been read yet, underLimitMemory, also pays two type tests and one read of the allocation counter. The change is inConvertResultonly: no interpreter path is touched, andJsStringandMemoryLimitConstraint.Check()are unchanged.Tests
TheMemoryLimitAloneStopsConvertResultBeforeItCopiesSharedValuesPastTheBudget(slice views, concatenations): both cases fail on the unfixed code, which copied 16.8 MB against a 4 MB budget where the test allows 8 MB.ConvertResultUnderTheMemoryLimitCopiesWhatFitsAndNeverWeighsFlatTextis a guard. It passes on the unfixed code. I confirmed it fails against a variant of the fix that checks every string, including flat ones.Jint.Tests.PublicInterfaceon net8.0, net10.0 and net472: 10,984 passed, 63 skipped, 0 failed.Jint.Testson net8.0, net10.0 and net472: 35,928 passed, 16 skipped, 0 failed.Docs updated to match: the constraints and untrusted-code guides, the
ConvertResultandMemoryLimitConstraintremarks, THREAT_MODEL TM-06 and TM-17, migration guide 4.27, andJint/Constraints/AGENTS.md.🤖 Generated with Claude Code
https://claude.ai/code/session_01YVwU18SgMKKzYrrCh4TMpT