Skip to content

Fix Utf8JsonReader invalid-literal errors: unbounded message, dropped bytes, IndexOutOfRangeException - #134448

Open
caraioniurie47 wants to merge 2 commits into
dotnet:mainfrom
caraioniurie47:issue-30706
Open

caraioniurie47 wants to merge 2 commits into
dotnet:mainfrom
caraioniurie47:issue-30706

Conversation

@caraioniurie47

@caraioniurie47 caraioniurie47 commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

When Utf8JsonReader finds an invalid true, false or null, the exception depends on how the input is segmented, and in some layouts it is not a JsonException at all. Measured on .NET 10.0.12:

  • Single segment (byte[], or a one-segment ReadOnlySequence): the message quotes everything to the end of the buffer. CheckLiteral passes ThrowInvalidLiteral the span ConsumeLiteral gave it, _buffer.Slice(_consumed), so tb: gives 'tb:' is an invalid JSON literal, and a 1,000,007-byte payload starting [falsX gave a 1,000,105-character message.
  • Multi-segment: a truncated literal can throw IndexOutOfRangeException, e.g. segments "t", "" or "[", "tr", "". CheckLiteralMultiSegment copied a span into its message buffer only after GetNextSpan() returned the next one, so nothing was copied, and GetInvalidLiteralMultiSegment read span[0] of an empty span.
  • Multi-segment: the last segment's bytes are dropped from the message when the data ends inside the literal, for the same reason: fals read one byte per segment gives 'fal'.
  • Multi-segment: BytesConsumed after the exception can be too low when the literal straddles a segment boundary: [tru as two segments split after [t or [tr reports 0, while the byte[] and the other splits that throw a JsonException report 1. The throw path restored _totalConsumed but not _consumed, which GetNextSpan() had reset.

Change. Both paths now quote the literal's bytes up to and including the first mismatching byte, or every byte present when the data ends, so the quote is at most the literal's length and the same for every segmentation. CheckLiteralMultiSegment copies each span's matching bytes as soon as they match, which removes the AmountToWrite helper, and restores _consumed before throwing. How BytePositionInLine and LineNumber are computed is unchanged.

This addresses the literal half of #30706. The comment-handling differences listed there ({/, /+ and others, with JsonCommentHandling.Skip and Allow) are not changed here and remain. Nothing is thrown that was not thrown before, apart from JsonException replacing IndexOutOfRangeException.

Tests

  • InvalidLiteralVariousSegmentSizes added to Utf8JsonReaderTests.MultiSegment.cs, in the shape of InvalidJsonNumberVariousSegmentSizes: 11 inputs, each read as a byte[], a one-segment ReadOnlySequence, one byte per segment (the helper adds an empty last segment), and every one- and two-split ReadOnlySequence, including splits at the end so an empty last segment is covered. Each read asserts LineNumber, BytePositionInLine, BytesConsumed, Position and the quoted literal.
  • Without the change all 11 fail, in both test target frameworks: IndexOutOfRangeException for t, 'nu', 'fal' and 'tr' for the dropped bytes, 'tb:' and 'nulL]' for the whole-buffer quote. With it all 11 pass.
  • With the change, System.Text.Json.Tests, System.Text.Json.SourceGeneration.Roslyn4.4.Tests and System.Text.Json.SourceGeneration.Roslyn3.11.Tests pass in both target frameworks.

Performance

BenchmarkDotNet 0.15.2, Windows 11 x64: a Release build of main and the same build with this change to System.Text.Json, each in its own Release testhost, compared with --coreRun. The benchmark reads an array of 3000 elements to the end: Literals is true,false,null repeated, Objects is {"id":n,"active":true|false,"parent":null,"name":"item n"}. Segment size 0 is a byte[]; with 3 every literal straddles segments, the path whose copying changed. Three runs, one launch per build and then three, mean ± SD in µs:

Payload Segment main (runs 1 / 2 / 3) This branch (runs 1 / 2 / 3)
Literals 0 34.08 ± 0.13 / 33.21 ± 0.72 / 32.35 ± 0.60 32.74 ± 0.08 / 32.29 ± 0.32 / 33.13 ± 0.90
Literals 3 176.34 ± 0.74 / 170.64 ± 6.28 / 170.69 ± 4.76 172.55 ± 0.86 / 175.85 ± 2.82 / 172.25 ± 1.28
Literals 64 46.50 ± 0.11 / 46.90 ± 4.64 / 46.99 ± 0.91 40.93 ± 0.15 / 48.66 ± 0.85 / 48.61 ± 1.34
Literals 4096 38.40 ± 0.07 / 37.60 ± 0.66 / 39.79 ± 3.77 38.75 ± 0.06 / 38.68 ± 0.27 / 38.67 ± 0.98
Objects 0 348.38 ± 2.88 / 341.95 ± 6.41 / 335.60 ± 2.24 339.04 ± 0.82 / 343.34 ± 5.74 / 334.26 ± 4.94
Objects 3 1469.70 ± 10.66 / 1458.81 ± 28.13 / 1431.73 ± 17.36 1426.20 ± 5.55 / 1445.56 ± 16.71 / 1422.44 ± 6.70
Objects 64 480.26 ± 1.30 / 475.91 ± 15.57 / 473.75 ± 2.57 504.10 ± 1.28 / 503.63 ± 6.53 / 472.93 ± 18.67
Objects 4096 397.41 ± 4.68 / 390.79 ± 9.30 / 392.40 ± 9.09 388.64 ± 1.76 / 390.11 ± 5.88 / 396.80 ± 8.71

By mean, the lower build changes between runs in every row except Objects/3, where this branch is lower in all three runs by 0.6% to 3%. Objects/64, higher on this branch in runs 1 and 2, was rerun alone with twenty launches per build, once in each host order: this branch 485.7 ± 18.3 against main 481.0 ± 19.4 µs, and 490.9 ± 16.5 against 491.0 ± 15.3 µs. Its spread is about 3.5% of the mean and neither build is consistently ahead. The byte[] rows, whose only change is in the throw path, move by up to 4% between builds within a run.

Contributes to #30706

Note

AI-generated, written at my direction and reviewed by me before posting. Everything ran on Windows 11 x64 (build 26200). The measurements in the first list ran on the installed .NET 10.0.12 release with a scratch console app; the two crash inputs and the 1,000,105-character message were re-run under the Release corerun of main built locally, with the same result, and under this branch's, where they give JsonException and a 104-character message. Position after the exception was measured the same way, on all three builds, for every two-segment split of [tru, [nulL], {"":tr and [ trux]. The tests ran on Debug libraries with a Checked runtime; the benchmark as described under Performance.

When a true, false or null literal is invalid, the single-segment
reader quoted the rest of the buffer in the exception message. The
multi-segment reader dropped the last segment's bytes from the message
when the data ended inside the literal, threw IndexOutOfRangeException
instead of JsonException when nothing had been copied yet, and left
BytesConsumed too low when the literal straddled a segment boundary.

Quote the literal's bytes up to and including the first mismatch, or all
of them when the data ends, in both readers; copy each matching span as
soon as it matches; and restore _consumed before throwing.

Contributes to dotnet#30706

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Sep 22, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

@caraioniurie47

Copy link
Copy Markdown
Contributor Author

@eiriktsarpalis, this is ready for review when there is time. Thanks!

@eiriktsarpalis

Copy link
Copy Markdown
Member

Compare the unchanged multi-segment implementation at this PR's base (ec0059c08c077e839c6eb8827c846530a891d7e2) against its current head (e566572bee407d689670195e29687c29eb4b2ce6), rather than a moving main.

This benchmark reads valid JSON to completion, isolating potential successful-read regressions from exception construction costs. It covers literal-dense arrays (true, false, null) and mixed objects, each with 1-, 2-, 3-, 64-, and 4096-byte segments, plus span and single-segment-sequence controls: 14 cases per runtime. Construction is outside the measurement. Setup verifies token counts, full consumption, and that every multi-segment configuration actually contains cross-segment literals. The 17-byte stride in the literal payload avoids accidentally aligning all literals away from 64-byte/4 KiB boundaries.

@EgorBot -amd -arm -commits ec0059c,e566572bee407d689670195e29687c29eb4b2ce6 --filter "LiteralMultiSegmentBenchmarks" --launchCount 3

using BenchmarkDotNet.Running;
using System;
using System.Buffers;
using System.Text;
using System.Text.Json;
using BenchmarkDotNet.Attributes;

BenchmarkSwitcher.FromAssembly(typeof(LiteralMultiSegmentBenchmarks).Assembly).Run(args);

[MemoryDiagnoser]
public class LiteralMultiSegmentBenchmarks
{
    private const int ItemCount = 1000;
    private byte[] _utf8 = null!;
    private ReadOnlySequence<byte> _sequence;

    public enum PayloadKind { Literals, Objects }
    public enum InputLayout { Span, SingleSegmentSequence, Bytes1, Bytes2, Bytes3, Bytes64, Bytes4096 }

    [ParamsAllValues]
    public PayloadKind Payload { get; set; }

    [ParamsAllValues]
    public InputLayout Layout { get; set; }

    [GlobalSetup]
    public void Setup()
    {
        var builder = new StringBuilder("[");
        for (int i = 0; i < ItemCount; i++)
        {
            if (i != 0)
            {
                builder.Append(", ");
            }

            if (Payload == PayloadKind.Objects)
            {
                builder.Append(' ', i % 4);
            }

            // The 17-byte literal-group stride varies alignment at 64-byte and 4 KiB boundaries.
            builder.Append(Payload == PayloadKind.Literals
                ? "true,false,null"
                : FormattableString.Invariant($"{{\"id\":{i},\"active\":{(i % 2 == 0 ? "true" : "false")},\"parent\":null,\"name\":\"item {i}\"}}"));
        }

        builder.Append(']');
        _utf8 = Encoding.UTF8.GetBytes(builder.ToString());
        int segmentSize = Layout switch
        {
            InputLayout.Span or InputLayout.SingleSegmentSequence => _utf8.Length,
            InputLayout.Bytes1 => 1,
            InputLayout.Bytes2 => 2,
            InputLayout.Bytes3 => 3,
            InputLayout.Bytes64 => 64,
            InputLayout.Bytes4096 => 4096,
            _ => throw new InvalidOperationException($"Unexpected layout: {Layout}.")
        };

        if (segmentSize == _utf8.Length)
        {
            _sequence = new ReadOnlySequence<byte>(_utf8);
        }
        else
        {
            var first = new Segment(_utf8.AsMemory(0, segmentSize));
            Segment last = first;
            for (int offset = segmentSize; offset < _utf8.Length; offset += segmentSize)
            {
                last = last.Append(_utf8.AsMemory(offset, Math.Min(segmentSize, _utf8.Length - offset)));
            }

            _sequence = new ReadOnlySequence<byte>(first, 0, last, last.Memory.Length);
        }

        var reader = Layout == InputLayout.Span ? new Utf8JsonReader(_utf8) : new Utf8JsonReader(_sequence);
        int tokens = 0;
        int literals = 0;
        int splitLiterals = 0;
        while (reader.Read())
        {
            tokens++;
            if (reader.TokenType is JsonTokenType.True or JsonTokenType.False or JsonTokenType.Null)
            {
                literals++;
                if (reader.HasValueSequence)
                {
                    splitLiterals++;
                }
            }
        }

        int expectedTokens = Payload == PayloadKind.Literals ? 3 * ItemCount + 2 : 10 * ItemCount + 2;
        int expectedLiterals = Payload == PayloadKind.Literals ? 3 * ItemCount : 2 * ItemCount;
        bool multiSegment = segmentSize < _utf8.Length;
        if (reader.BytesConsumed != _utf8.Length || tokens != expectedTokens || literals != expectedLiterals
            || _sequence.IsSingleSegment == multiSegment || (multiSegment && splitLiterals == 0))
        {
            throw new InvalidOperationException(
                $"Invalid coverage for {Payload}/{Layout}: {reader.BytesConsumed} bytes, {tokens} tokens, {literals} literals, {splitLiterals} split literals.");
        }
    }

    [Benchmark]
    public long Read()
    {
        var reader = Layout == InputLayout.Span ? new Utf8JsonReader(_utf8) : new Utf8JsonReader(_sequence);
        while (reader.Read())
        {
        }

        return reader.BytesConsumed;
    }

    private sealed class Segment : ReadOnlySequenceSegment<byte>
    {
        public Segment(ReadOnlyMemory<byte> memory) => Memory = memory;

        public Segment Append(ReadOnlyMemory<byte> memory)
        {
            var next = new Segment(memory) { RunningIndex = RunningIndex + Memory.Length };
            Next = next;
            return next;
        }
    }
}

Note

This benchmark and request were generated by GitHub Copilot at the reviewer's direction.

@eiriktsarpalis

Copy link
Copy Markdown
Member

Run the existing dotnet/performance multi-segment reader benchmarks against this PR's exact base and head, on the same architectures as the focused literal benchmark above:

  • System.Text.Json.Tests.Perf_Segment.ReadMultiSegmentSequence: 4/40/400 KB documents, each with 4096- and 8192-byte segments (6 cases).
  • System.Text.Json.Tests.Perf_Reader.ReadMultiSpanSequenceEmptyLoop: all 8 payloads, compact and formatted, split into two segments (16 cases).

These are genuine ReadOnlySequence<byte> reader paths. ReadMultiSegmentSequenceUsingSpan deliberately isn't selected: it copies data into a contiguous buffer and invokes the span reader, not the multi-segment reader changed here. The comment-parsing benchmarks contain no literals, so they aren't relevant to this change.

Coverage caveat: in the current Perf_Segment resource data, none of the six 4/8 KiB layouts actually splits a true, false, or null token across a boundary. Those benchmarks remain useful throughput controls, but cannot replace the focused cross-boundary workload above. Sources inspected at dotnet/performance commit 63698409bbbf1e6cab6358c9577246cdbef11da1: Perf.Segment.cs, Perf.Reader.cs.

@EgorBot -amd -arm -commits ec0059c,e566572bee407d689670195e29687c29eb4b2ce6 --filter "*System.Text.Json.Tests.Perf_Segment.ReadMultiSegmentSequence" "*System.Text.Json.Tests.Perf_Reader.ReadMultiSpanSequenceEmptyLoop" --launchCount 3

Note

This investigation and benchmark request were generated by GitHub Copilot at the reviewer's direction.

@caraioniurie47

Copy link
Copy Markdown
Contributor Author

@eiriktsarpalis, both EgorBot requests failed before any benchmark ran (EgorBot/Benchmarks#657, EgorBot/Benchmarks#658): the agent checks out each -commits SHA from its dotnet/runtime clone, and the head e566572bee4 is a merge commit that exists only on the fork, so git checkout ended with fatal: reference is not a tree (exit 128).

Without -commits, the bot builds main and then merges pull/134448/head into that same checkout, so both sides share one main commit. Could the two requests be re-run without -commits?

Thanks.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-System.Text.Json community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants