Repository navigation
Check MaxOutputCharacters against a string's length before ConvertResult copies it - #4185
Merged
lahma merged 1 commit intoSep 25, 2026
Conversation
…ult copies it ResultConverter counted MaxOutputCharacters after value.ToString(), so with MaxStringLength unset a conversion flattened and copied the string that crossed the limit before refusing it: a slice view or a deferred a + b of any length up to JsString.MaxLength. Under MaxOutputCharacters = 500,000 alone, refusing one 1,048,576-character value allocated 2.1 MB (4.2 MB for a rope on net472); refusing the second of 64 slice views under 1,500,000 allocated 4.2 MB, one copy past the limit. Primitive strings, String objects and property names now go through one ConvertString that checks both character limits against JsString.Length before copying. Every JsString representation answers Length from a field, so the check flattens nothing. What counts toward the total is still the copied text's length, so a host LazyJsString that materializes more than it declared cannot slip characters past it. The two refusals above now allocate 1.6-8.3 KB and 2.1 MB. The constraints guide, the ConvertResult remarks and THREAT_MODEL.md TM-17 said MaxOutputCharacters was counted after each copy and told hosts to set both limits; they now say it is checked before the copy and bounds the characters a conversion copies on its own. Part of sebastienros#4175 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This was referenced Oct 5, 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
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.
Engine.ConvertResultcountedResultLimits.MaxOutputCharactersafterToString(). WithMaxStringLengthunset, it flattened and copied the string that crossed the limit before refusing it. That string could be a slice view or a deferreda + bof any length up toJsString.MaxLength. #4182 found this and documented it as a residual; this PR fixes it.The defect
ResultConverter.ConvertPrimitive, theStringInstancecase and the property-name loop each did:The fix
All three sites now go through a single
ConvertString. It checks both limits againstJsString.LengthbeforeToString(). EveryJsStringrepresentation answersLengthfrom a field, so the check flattens nothing:SlicedString._lengthRopeString._lengthConcatenatedString's builder lengthLazyJsString._length_value.LengthThe running total is still committed from the copied text's length. For every honest string the count, the refusal,
Limit,MaximumandObservedare unchanged. A hostLazyJsStringthat materializes more characters than it declared still cannot slip them past the total.Measured (
MemoryLimitConstraint.AllocatedBytesover theConvertResultentry, the same probe as #4182's test)MaxOutputCharacters = 500_000alone, one 1,048,575-char slice viewa + bnew String(slice view)new String(a + b)MaxOutputCharacters = 1_500_000alone, 64 slice views of one 1 MB string (refused on the 2nd)In the last row, the first view fits the limit and is copied on purpose (about 2 MB). The fix removes the copy of the view that crosses the limit.
Tests
Two tests in
HostDeferredStringMemoryLimitTests, five cases:OutputCharactersAloneRefusesAStringBeforeCopyingIt: a slice view, a deferred concatenation, and aStringobject over each, all underMaxOutputCharactersalone. Each assertsOutputCharacters,Maximum,Observedand fewer than 64 KiB allocated.OutputCharactersRefusesTheStringThatWouldCrossTheLimitBeforeCopyingIt: the running-total case. It assertsObservedand less than 3,000,000 B allocated.All five cases fail on the unfixed converter on both net10.0 and net472, and only on the allocation assertion. For example:
Expected constraint.AllocatedBytes to be less than 65536L because copying the value would allocate 2 MB, but found 2098816L. The existing #4182 test (ConvertResultRefusesValuesSharingOneStringBeforeCopyingThem, refused onStringLengthunderConservative) andHostResultLimitsTests.StringAndAggregateCharacterLimitsAreIndependent(Observed10) pass unchanged.Docs
Three places told hosts that
MaxOutputCharactersis counted after each copy and that they must set both limits. Each now says it is checked by length before the copy and on its own bounds the characters one conversion copies:docs/guide/constraints.md, "Bound what the host copies out of a result": the paragraph that said one conversion copies up toMaxStringLength + MaxOutputCharacters(3,000,000 underConservative) now givesMaxOutputCharacters(2,000,000).Engine.ConvertResultXML remarks..github/THREAT_MODEL.mdTM-17: the residual-mitigation bullet is removed and an existing-mitigation bullet is added. Its "enforces the selected limits before known-size output allocations" was not true forMaxOutputCharacterson strings until now.The sample (
guide-bounded-result) is unchanged.Verification (Release, rebased on
85571424b)Jint.Testsnet10.0: 12,802 total, 12,797 passed, 5 skipped, 0 failedJint.Testsnet472: 8,850 total, 8,844 passed, 6 skipped, 0 failedJint.Tests.PublicInterfacenet10.0: 3,824 total, 3,803 passed, 21 skipped, 0 failedJint.Tests.PublicInterfacenet472: 3,031 total, 3,010 passed, 21 skipped, 0 failedMigrationGuideTests,AgentInstructionFileTests,SpecCitationTests: 11/11 passedI ran no benchmark:
ConvertResultis not a hot path. Per string, the change adds one comparison before the copy.Base:
85571424b23877e045b499374a723fb3e33edc84Part of #4175
🤖 Generated with Claude Code