Check MaxOutputCharacters against a string's length before ConvertResult copies it

.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.

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 <see cref="JsString.Length"/>, 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.
 /// </para>
 /// <para>
 /// The shapes are the ones a deferred <c>+</c> 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");
     }
+
+    /// <summary>
+    /// <see cref="ResultLimits.MaxOutputCharacters"/> 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.
+    /// </summary>
+    [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<MemoryLimitConstraint>()!;
+        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<ResultLimitExceededException>().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");
+    }
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    [Test]
+    public void OutputCharactersRefusesTheStringThatWouldCrossTheLimitBeforeCopyingIt()
+    {
+        var engine = new Engine(options => options.LimitMemory(16_000_000));
+        var constraint = engine.Constraints.Find<MemoryLimitConstraint>()!;
+        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<ResultLimitExceededException>().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");
+    }
 }

Jint/Engine.cs

@@ -4275,9 +4275,10 @@ public JsValue GetValue(JsValue scope, JsValue property)
     /// <paramref name="limits"/> to override <see cref="Options.ResultLimits"/> for one call.
     /// </para>
     /// <para>
-    /// Set <see cref="ResultLimits.MaxStringLength"/> and <see cref="ResultLimits.MaxOutputCharacters"/> 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 <see cref="ResultLimits.MaxOutputCharacters"/> 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. <see cref="ResultLimits.MaxStringLength"/> refuses any one string longer than it the same way.
     /// </para>
     /// </remarks>
     public object? ConvertResult(JsValue value, ResultLimits? limits = null)

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;
         }
 
+        /// <summary>
+        /// Checks a string against both character limits by its length, and only then copies it.
+        /// </summary>
+        /// <remarks>
+        /// A slice view, a deferred concatenation and a host's <see cref="LazyJsString"/> answer
+        /// <see cref="JsString.Length"/> from a field, while <see cref="JsString.ToString()"/> on one of them
+        /// copies every character, up to <see cref="JsString.MaxLength"/> of them. Checking after the copy
+        /// would bound what the conversion returns, not what it allocates. What
+        /// <see cref="ResultLimits.MaxOutputCharacters"/> counts is still the text that was copied, so a host
+        /// string that materializes more than it declared cannot slip characters past the total.
+        /// </remarks>
+        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,15 +328,15 @@ private void CountStringLength(int length)
             }
         }
 
-        private void CountOutputCharacters(int length)
+        private long CheckOutputCharacters(int length)
         {
             var observed = checked(_outputCharacters + length);
             if (observed > _limits.MaxOutputCharacters)
             {
                 ThrowLimit(ResultLimit.OutputCharacters, _limits.MaxOutputCharacters, observed);
             }
 
-            _outputCharacters = observed;
+            return observed;
         }
 
         private void CountBytes(long count)

docs/guide/constraints.md

@@ -90,10 +90,12 @@ var detached = engine.ConvertResult(value, ResultLimits.Conservative);
 ```
 <!-- endSnippet -->
 
-`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