From 559e6a4d720c493dc015866b9739c94f5e2561bc Mon Sep 17 00:00:00 2001 From: Marko Lahma Date: Fri, 25 Sep 2026 22:02:29 +0300 Subject: [PATCH] Check MaxOutputCharacters against a string's length before ConvertResult 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 #4175 Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/THREAT_MODEL.md | 7 ++- .../HostDeferredStringMemoryLimitTests.cs | 58 ++++++++++++++++++- Jint/Engine.cs | 7 ++- Jint/Runtime/Interop/ResultConverter.cs | 40 ++++++++----- docs/guide/constraints.md | 10 ++-- 5 files changed, 98 insertions(+), 24 deletions(-) diff --git a/.github/THREAT_MODEL.md b/.github/THREAT_MODEL.md index f0216bb185..89db03e127 100644 --- a/.github/THREAT_MODEL.md +++ b/.github/THREAT_MODEL.md @@ -767,6 +767,10 @@ run additional code and consume CPU or memory outside the intended execution bud - `ConvertResult` copies Jint-owned arrays, typed arrays, maps, sets, and enumerable object properties to a detached CLR graph, rejects cycles, and enforces the selected limits before known-size output allocations and before property getters are read. +- `ConvertResult` checks `MaxStringLength` and `MaxOutputCharacters` against a string's length + before copying its characters, so a long `slice` view or deferred `a + b` over shared + characters is refused without being flattened, and `MaxOutputCharacters` alone bounds the + characters one conversion copies. - Jint's JSON serializer enforces the same limits while walking, counts escaped characters before appending, and checks exact UTF-8 bytes before touching a writer. - Conversion, JSON serialization, and bounded JavaScript error rendering run under execution @@ -776,9 +780,6 @@ run additional code and consume CPU or memory outside the intended execution bud **Missing or residual mitigation.** - Result limits are unlimited by default. -- `MaxStringLength` is checked against a string's length before it is copied, but - `MaxOutputCharacters` is counted after each copy, so a conversion can copy one string past - it. Configure both; together they bound what a conversion copies. - Shared references that are not cycles are converted once per occurrence and can amplify the detached CLR graph. `MaxPropertyCount` is the structural-work and container-allocation bound; string, character, and binary-byte limits do not substitute for it. diff --git a/Jint.Tests.PublicInterface/HostDeferredStringMemoryLimitTests.cs b/Jint.Tests.PublicInterface/HostDeferredStringMemoryLimitTests.cs index 9eea03602f..57735f742e 100644 --- a/Jint.Tests.PublicInterface/HostDeferredStringMemoryLimitTests.cs +++ b/Jint.Tests.PublicInterface/HostDeferredStringMemoryLimitTests.cs @@ -18,7 +18,8 @@ namespace Jint.Tests.PublicInterface; /// No test here flattens the value it is about: the assertions read , which a /// deferred value answers from the node, so a broken build fails them without allocating the 16 MB a script /// here can reach. The largest real allocations are the 6 MB host string one test hands in and the 2 MB -/// the guard's own script reads back, so the suite needs no heap cap to run a broken build safely. +/// strings a script reads back or a conversion copies, so the suite needs no heap cap to run a broken build +/// safely. /// /// /// The shapes are the ones a deferred + makes cheap to build and expensive to read: a doubling, @@ -245,4 +246,59 @@ public void ConvertResultRefusesValuesSharingOneStringBeforeCopyingThem(string e .Which.Limit.Should().Be(ResultLimit.StringLength); constraint.AllocatedBytes.Should().BeLessThan(1L << 20, "copying one value would allocate 2 MB"); } + + /// + /// set on its own is a bound on what the conversion copies, + /// not only on what it hands back: a slice view and a deferred concatenation answer their length without + /// flattening, so a string that would take the running total past the limit is refused by that length + /// before its characters are copied. Each value here is about 1,048,576 characters, a 2 MB copy, against a + /// limit of 500,000, and the conversion refuses it having allocated next to nothing. The count reported is + /// the one a copy would have made. + /// + [TestCase("big.slice(1)", (1 << 20) - 1, TestName = "{m}(slice view)")] + [TestCase("big + 'y'", (1 << 20) + 1, TestName = "{m}(deferred concatenation)")] + [TestCase("new String(big.slice(1))", (1 << 20) - 1, TestName = "{m}(String object over a slice view)")] + [TestCase("new String(big + 'y')", (1 << 20) + 1, TestName = "{m}(String object over a deferred concatenation)")] + public void OutputCharactersAloneRefusesAStringBeforeCopyingIt(string expression, int length) + { + var engine = new Engine(options => options.LimitMemory(16_000_000)); + var constraint = engine.Constraints.Find()!; + var value = engine.Evaluate($"var big = 'x'.repeat(1 << 20); {expression}"); + + var failure = Caught.Exception(() => engine.ConvertResult(value, new ResultLimits { MaxOutputCharacters = 500_000 })); + + var refusal = failure.Should().BeOfType().Which; + refusal.Limit.Should().Be(ResultLimit.OutputCharacters); + refusal.Maximum.Should().Be(500_000); + refusal.Observed.Should().Be(length); + constraint.AllocatedBytes.Should().BeLessThan(64 * 1024, "copying the value would allocate 2 MB"); + } + + /// + /// The running total is checked the same way: 64 slice views of one 1,048,576-character string under a + /// limit of 1,500,000 characters copy the first, about 2 MB, and refuse the second, which would take the + /// total to 2,097,149, before copying it — so no conversion copies more characters than the limit, where + /// counting after each copy let one more string through. + /// + [Test] + public void OutputCharactersRefusesTheStringThatWouldCrossTheLimitBeforeCopyingIt() + { + var engine = new Engine(options => options.LimitMemory(16_000_000)); + var constraint = engine.Constraints.Find()!; + var value = engine.Evaluate(""" + var big = 'x'.repeat(1 << 20); + var values = []; + for (var i = 0; i < 64; i++) { values.push(big.slice(i + 1)); } + values + """); + + var failure = Caught.Exception(() => engine.ConvertResult(value, new ResultLimits { MaxOutputCharacters = 1_500_000 })); + + var refusal = failure.Should().BeOfType().Which; + refusal.Limit.Should().Be(ResultLimit.OutputCharacters); + refusal.Observed.Should().Be(1_048_575 + 1_048_574); + constraint.AllocatedBytes.Should().BeLessThan( + 3_000_000, + "the value within the limit is copied, 2,097,150 bytes, and a copy of the second would add 2,097,148"); + } } diff --git a/Jint/Engine.cs b/Jint/Engine.cs index 832a69cb57..fffdee7cdd 100644 --- a/Jint/Engine.cs +++ b/Jint/Engine.cs @@ -4275,9 +4275,10 @@ public JsValue GetValue(JsValue scope, JsValue property) /// to override for one call. /// /// - /// Set and for an - /// untrusted result. The first refuses a string by its length before copying it, and the second stops the - /// conversion once the characters copied exceed it; a memory limit charges a copy only after it is made. + /// Set for an untrusted result: each string is checked by its + /// length before it is copied, and one that would take the characters copied past the limit is refused, so + /// the limit bounds the characters the conversion copies. A memory limit charges a copy only after it is + /// made. refuses any one string longer than it the same way. /// /// public object? ConvertResult(JsValue value, ResultLimits? limits = null) diff --git a/Jint/Runtime/Interop/ResultConverter.cs b/Jint/Runtime/Interop/ResultConverter.cs index 675c95446d..828db7c016 100644 --- a/Jint/Runtime/Interop/ResultConverter.cs +++ b/Jint/Runtime/Interop/ResultConverter.cs @@ -62,10 +62,7 @@ public ConversionContext(Engine engine, ResultLimits limits) { if (value.IsString()) { - CountStringLength(((JsString) value).Length); - var text = value.ToString(); - CountOutputCharacters(text.Length); - return text; + return ConvertString((JsString) value); } if (value.IsSymbol()) @@ -81,10 +78,7 @@ private object ConvertObject(ObjectInstance instance) switch (instance) { case StringInstance stringInstance: - CountStringLength(stringInstance.StringData.Length); - var text = stringInstance.StringData.ToString(); - CountOutputCharacters(text.Length); - return text; + return ConvertString(stringInstance.StringData); case JsDate date: return date.ToDateTime(); case BooleanInstance booleanInstance: @@ -259,9 +253,7 @@ private object ConvertTypedArray(JsTypedArray array) for (var i = 0; i < enumerableKeys.Count; i++) { CheckConstraints(); - var key = enumerableKeys[i].ToString(); - CountStringLength(((JsString) enumerableKeys[i]).Length); - CountOutputCharacters(key.Length); + var key = ConvertString((JsString) enumerableKeys[i]); result.Add(key, Convert(instance.Get(enumerableKeys[i]))); } @@ -306,6 +298,28 @@ private void CountProperties(long count) _propertyCount = observed; } + /// + /// Checks a string against both character limits by its length, and only then copies it. + /// + /// + /// A slice view, a deferred concatenation and a host's answer + /// from a field, while on one of them + /// copies every character, up to of them. Checking after the copy + /// would bound what the conversion returns, not what it allocates. What + /// counts is still the text that was copied, so a host + /// string that materializes more than it declared cannot slip characters past the total. + /// + private string ConvertString(JsString value) + { + var length = value.Length; + CountStringLength(length); + CheckOutputCharacters(length); + + var text = value.ToString(); + _outputCharacters = CheckOutputCharacters(text.Length); + return text; + } + private void CountStringLength(int length) { if (length > _limits.MaxStringLength) @@ -314,7 +328,7 @@ private void CountStringLength(int length) } } - private void CountOutputCharacters(int length) + private long CheckOutputCharacters(int length) { var observed = checked(_outputCharacters + length); if (observed > _limits.MaxOutputCharacters) @@ -322,7 +336,7 @@ private void CountOutputCharacters(int length) ThrowLimit(ResultLimit.OutputCharacters, _limits.MaxOutputCharacters, observed); } - _outputCharacters = observed; + return observed; } private void CountBytes(long count) diff --git a/docs/guide/constraints.md b/docs/guide/constraints.md index d2fb26e925..a72e036366 100644 --- a/docs/guide/constraints.md +++ b/docs/guide/constraints.md @@ -90,10 +90,12 @@ var detached = engine.ConvertResult(value, ResultLimits.Conservative); ``` -`MaxStringLength` is checked against a string's length before its characters are copied, and -`MaxOutputCharacters` stops the conversion once the characters copied so far exceed it, so one conversion -copies no more than the two added together: 3,000,000 characters under `ResultLimits.Conservative`. Set both. -A refusal throws `ResultLimitExceededException`, whose `Limit` names the bound that was reached. +Both character limits are checked against a string's length before its characters are copied, and a slice +view or a deferred `a + b` knows its length without being flattened. `MaxStringLength` refuses any one string +or property name longer than it, and `MaxOutputCharacters` refuses the one that would take the running total +past it, so `MaxOutputCharacters` on its own bounds the characters one conversion copies: 2,000,000 under +`ResultLimits.Conservative`. A refusal throws `ResultLimitExceededException`, whose `Limit` names the bound +that was reached. Called without limits, `ConvertResult` uses `Options.ResultLimits`: unlimited by default, and `UntrustedCodeLimits.ResultLimits` (`ResultLimits.Conservative` unless changed) on an engine built with