Fix Content-Length byte/char mismatch in the server-mode TCP transport by azat-msft · Pull Request #10297 · microsoft/testfx

azat-msft · 2026-07-28T21:08:56Z

The bug

TcpMessageHandler declares Content-Length in UTF-8 bytes on the write path, but the read path consumed that number as decoded characters.

Write path (correct, unchanged):

int byteCount = Encoding.UTF8.GetByteCount(messageStr);
await _writer.WriteLineAsync($"Content-Length: {byteCount}");
...
await _writer.BaseStream.WriteAsync(rentedBytes.AsMemory(0, byteCount), cancellationToken);

Read path before:

char[] commandCharsBuffer = ArrayPool<char>.Shared.Rent(commandSize);
Memory<char> memoryBuffer = new(commandCharsBuffer, 0, commandSize);
await _reader.ReadBlockAsync(memoryBuffer, cancellationToken);   // characters, not bytes
return _formatter.Deserialize<RpcMessage>(memoryBuffer);

_reader is a StreamReader, so ReadBlockAsync yields decoded characters. The #else (Jsonite, net462/netstandard2.0) branch had the identical defect with char[] commandChars = new char[commandSize].

Read path after — consume exactly Content-Length bytes, then UTF-8-decode, symmetric with the write path:

byte[] bodyBuffer = ArrayPool<byte>.Shared.Rent(commandSize);
if (!await ReadExactlyAsync(bodyBuffer, commandSize, cancellationToken)) { return null; }
int charCount = Encoding.UTF8.GetCharCount(bodyBuffer, 0, commandSize);
char[] charsBuffer = ArrayPool<char>.Shared.Rent(charCount);
Encoding.UTF8.GetChars(bodyBuffer, 0, commandSize, charsBuffer, 0);
return _formatter.Deserialize<RpcMessage>(new ReadOnlyMemory<char>(charsBuffer, 0, charCount));

For pure ASCII, bytes == characters, so nothing broke and the defect stayed latent. For any multi-byte UTF-8 content — a localized test name, an exception message, or captured standard output in German, Japanese, Czech, Cyrillic, or an emoji — the two counts disagree. The reader under-reads the frame, leaves the body's tail in the stream, and the framing desynchronizes permanently: the leftover bytes are parsed as the next frame's headers.

The symptom only appears from the second frame onward. The first frame still deserializes fine (its JSON is merely truncated at a point the parser may or may not reject); it is the following read that picks up mid-message garbage. Any regression test therefore has to send at least two messages.

Both compilation branches were affected — #if NETCOREAPP (System.Text.Json, net8+) and #else (Jsonite, net462/netstandard2.0) — and both are fixed.

This affects the live server, not just the new client package

src/Platform/Microsoft.Testing.Platform/ServerMode/JsonRpc/TcpMessageHandler.cs is the shipping MTP server-mode transport. The source-only client package added in #10085 compiles the same file as source, which is how the bug surfaced, but the defect is in the server transport itself and has always been there. Anything speaking server mode against a test host that emits non-ASCII test names or output could hit it.

The `StreamReader` buffering hazard

Reading the body as raw bytes from _reader.BaseStream while the headers still went through StreamReader would have been a worse bug: StreamReader fills its own internal buffer, so by the time the header line is returned it has typically already swallowed part of the body, and BaseStream would resume past it.

The fix therefore removes StreamReader from the read path entirely. Headers and body are both read through a single byte-level buffer owned by the handler (_readBuffer / ReadHeaderLineAsync / ReadExactlyAsync / FillReadBufferAsync), so header and body reads sit on the same side of the buffering boundary and nothing can be stranded. This mirrors what vstest's hand-written client (the now-deleted MtpServerConnection, replaced in microsoft/vstest#16300) already did correctly: read the header line at the byte level, then read exactly count bytes and Encoding.UTF8.GetString them.

One compatibility detail worth calling out: the old StreamReader had byte-order-mark detection enabled by default and silently swallowed a leading UTF-8 preamble. Some peers write their headers through a preamble-emitting encoder (new StreamWriter(stream, Encoding.UTF8) — the platform's own ServerTests do exactly this), so the new reader keeps that tolerance explicitly via TrimPreamble, and there is a test for it. Cancellation semantics and the SocketError.ConnectionReset graceful-disconnect handling are unchanged.

Tests

The regression tests live in test/UnitTests/Microsoft.Testing.Platform.UnitTests/ServerMode/TcpMessageHandlerTests.cs, merged into the existing test class. They stand up a pair of TcpMessageHandlers joined over loopback TCP, so the bytes on the wire are exactly what a real server and client exchange. That project multi-targets net462 alongside net8.0/net9.0, so every test runs against both the #if NETCOREAPP (System.Text.Json) and #else (Jsonite) compilation branches.

Covering the byte/char defect itself:

ReadAsync_RawUtf8FramesFromPeer_DoesNotDesynchronizeSubsequentFrames — two frames from a conformant peer that emits raw UTF-8, the first with multi-byte content. Fails before the fix on every TFM.
ReadAsync_BodyLargerThanReadBuffer_SpansMultipleRefillsWithoutDesynchronizing — a body far larger than the read buffer, so it spans several refills; a second frame follows to prove the reader stopped on the exact byte boundary. Fails before the fix.
ReadAsync_HandlerWrittenFramesWithNonAscii_DoesNotDesynchronizeSubsequentFrames — the same round trip through TcpMessageHandler on both ends, using real client/log and telemetry/update notifications. See the caveat below: this one only exercises the defect on net462.
ReadAsync_AsciiFrames_RoundTripInOrder — guards the ASCII fast path against over-reading.

Covering the paths this change introduces:

ReadAsync_HeaderLineLongerThanInitialBuffer_GrowsAndParsesFrame — a 5 KB header line, driving the _headerLineBuffer growth path through several doublings.
ReadAsync_PeerDisconnectsMidBody_ReturnsNull — a peer that announces a body then hangs up mid-way, which must surface as a graceful disconnect rather than a hang or a throw. Fails before the fix.
ReadAsync_LeadingByteOrderMark_IsSkipped — pins the BOM tolerance the old StreamReader provided implicitly.

Coverage caveat worth knowing

ReadAsync_HandlerWrittenFramesWithNonAscii_... cannot detect this bug on .NET. Utf8JsonWriter uses JavaScriptEncoder.Default, which escapes every non-ASCII character to \uXXXX, so on net8.0/net9.0 the body reaches the wire as pure ASCII, byte count equals char count, and the pre-fix reader passes it unchanged. It only exercises the regression on net462, where Jsonite emits BMP characters unescaped. The docstring says so outright.

The cross-TFM coverage comes from the two tests that frame raw UTF-8 bytes themselves and so bypass formatter escaping entirely.

This is also why the bug stayed latent: MTP-to-MTP traffic is accidentally ASCII on the wire, so the defect only bites when the peer emits raw UTF-8 — which is exactly what an external client such as vstest's does.

Failure output before the fix

net8.0:

failed ReadAsync_RawUtf8FramesFromPeer_DoesNotDesynchronizeSubsequentFrames (100ms)
  System.Text.Json.JsonReaderException: 'C' is invalid after a single JSON value.
  Expected end of data. LineNumber: 0 | BytePositionInLine: 90.

The 'C' is the C of the next frame's Content-Length header being read as part of the previous body — the desynchronization made visible.

net462, where the non-ASCII tests hang until the read times out:

failed ReadAsync_RawUtf8FramesFromPeer_DoesNotDesynchronizeSubsequentFrames (30s 080ms)
  Timed out reading a frame; the transport is most likely desynchronized.
failed ReadAsync_HandlerWrittenFramesWithNonAscii_DoesNotDesynchronizeSubsequentFrames (30s 080ms)
  Timed out reading a frame; the transport is most likely desynchronized.

Verified by swapping TcpMessageHandler.cs back to its parent revision, rebuilding, and re-running — so these are genuine failing-before/passing-after regression tests, not tests fitted to the new implementation.

Suite results (before → after)

Run locally in Release on Windows.

Suite	Before	After
`Microsoft.Testing.Platform.UnitTests` (net8.0)	1440/1440	1447/1447
`Microsoft.Testing.Platform.UnitTests` (net9.0)	1440/1440	1447/1447
`Microsoft.Testing.Platform.UnitTests` (net462)	1416/1416	1423/1423
↳ `ServerTests` (live server over TCP)	5/5	5/5
↳ serialization tests	78/78	78/78
`Microsoft.Testing.Platform.ServerClient.UnitTests` (net8.0)	24/24	24/24
`Microsoft.Testing.Platform.ServerClient.UnitTests` (net462)	24/24	24/24
`MtpServerClientSourcePackage*` (contract + hostile consumer)	9/9	9/9
`MtpServerClientAcceptanceTests` (real generated MTP app)	3/3	3/3
MTP acceptance `Server*`	14/14	14/14

The platform suites are up 7 (the 4 transport tests plus 3 new buffer-path tests), and ServerClient.UnitTests is unchanged at 24.

The ServerTests row is the interesting one: it is the platform's own server-mode suite driving a real TestApplication over TCP, and it is what caught the BOM regression in an intermediate version of this change.

The fix is additionally validated outside this repo: a locally-built pack of the source package was consumed by the vstest retarget work, where CrossPlatEngine builds clean on net462/netstandard2.0/net8.0 and the MTP suites pass 140/140 and 16/16 — including the raw-UTF-8 peer traffic that exposes this bug.

Relationship to #10085

The src/ change is independent of #10085 and applies to main as-is. The regression tests live in Microsoft.Testing.Platform.UnitTests, which exists on main and already multi-targets net462, so nothing about this fix requires the source-package work.

Verified rather than assumed — rebasing all three commits onto current main:

rebases cleanly, zero conflicts;
TcpMessageHandlerTests.cs is byte-identical on main and on this PR's base;
every symbol the new tests use exists on main in the same files;
the only difference in TcpMessageHandler.cs between main and this base is an unrelated 5-line Substring-vs-range-operator change from Add a source-only MTP server-mode client package #10085, which none of these commits touch.

The base is left as nohwnd-mtp-client-source-package so it reads as an increment on that work, but it can be retargeted to main at any time if the shipping-server fix should land independently.

Provenance

This was raised by an automated reviewer on #10085 and acknowledged there by the author as a real latent bug, to be fixed in a dedicated platform PR rather than inside the packaging change. This is that PR, stacked on nohwnd-mtp-client-source-package so it reads as an increment on that work.

TcpMessageHandler declares Content-Length in UTF-8 bytes on the write path but consumed that number as decoded characters on the read path (ReadBlockAsync over a StreamReader). For pure ASCII the two counts coincide, so nothing broke. For any multi-byte UTF-8 content - a localized test name, an exception message, or captured standard output in German, Japanese, Czech, Cyrillic, or an emoji - the reader under-reads the frame, leaves its tail in the stream, and the framing desynchronizes permanently: the leftover bytes are parsed as the next frame's headers. The symptom therefore surfaces on the second frame, not the first. Both compilation branches were affected: the #if NETCOREAPP path via ReadBlockAsync(Memory<char>) and the #else (Jsonite, net462/netstandard2.0) path via ReadBlockAsync(char[], ...). The read path now consumes exactly Content-Length bytes and UTF-8-decodes them, symmetric with the write path. Because StreamReader would have buffered part of the body while reading the headers, the headers are read at the byte level too: header and body share one buffering boundary, so nothing can be stranded on the other side of it. The previous StreamReader had byte-order-mark detection on by default, so a leading UTF-8 preamble is still skipped for peers that write their headers through a preamble-emitting encoder. This file is shared with the live MTP server, not only with the new source-only client package, so the fix applies to the shipping server transport as well. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 365edd84-6344-4ce4-9048-2c1ed1b7d939

Swaps the interim feed pack from the plain 2.4.0-dev build output to the coordinator's canonical 2.4.0-dev.utf8fix1 drop of microsoft/testfx#10297. Byte-equivalent content: all 184 contentFiles are identical between the two packs, including TcpMessageHandler.cs with both ReadExactlyAsync and the TrimPreamble BOM tolerance. Only the version metadata differs. The rename is the point. While the package is served from a committed local folder, NuGet caches by version, so a plain 2.4.0-dev risks silently resolving a stale cache entry from an earlier drop of the same name. The unique suffix makes that impossible, matching the convention the branch already used for 2.4.0-dev.numberfix. Re-verified from a cleared package cache: CrossPlatEngine clean on all three TFMs, MTP unit tests 140/140, MtpUnderVstestTests 16/16. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd0793f4-d530-44b6-b881-fed3be6aa52f

Evangelink

Summary

Reviewed in depth and validated locally — this is a real, correctly-diagnosed bug and the fix is sound. Approving.

What I verified (not just read)

I checked out the head commit into a worktree, built both TFM legs, and ran the suites:

Check	Result
`Microsoft.Testing.Platform.ServerClient.UnitTests` (net8.0)	28/28 pass
`Microsoft.Testing.Platform.ServerClient.UnitTests` (net462)	28/28 pass
`Microsoft.Testing.Platform.UnitTests` (net8.0, incl. `ServerTests`)	1440/1440 pass
`Microsoft.Testing.Platform.UnitTests` (net462)	1416/1416 pass
Build both legs	clean, 0 warnings

The regression tests genuinely fail without the fix. I reverted only TcpMessageHandler.cs to its parent revision, rebuilt, and re-ran — ReadAsync_RawUtf8FramesFromPeer_DoesNotDesynchronizeSubsequentFrames and ReadAsync_LeadingByteOrderMark_IsSkipped both fail on net8.0 with exactly the 'C' is invalid after a single JSON value reader exception the description quotes (the C of the next frame's Content-Length). That is a properly failing-before/passing-after regression test, not a test written to match the new implementation.

The bug is confirmed present on main, not just on the stacked base branch: main's WriteRequestAsync declares Encoding.UTF8.GetByteCount(messageStr) while ReadAsync consumes that count through StreamReader.ReadBlockAsync (characters) on both compilation branches. So this really is a shipping server-transport defect, and the framing desynchronizes permanently once any multi-byte body goes over the wire.

Correctness review of the new read path

The reasoning about StreamReader is right and worth keeping in the comment: reading headers through a StreamReader and the body from BaseStream would have been strictly worse, because the reader's internal buffer would already have swallowed part of the body. Collapsing both onto one byte-level buffer is the correct shape.

Specific points I checked and found sound:

FillReadBufferAsync is only ever called when _readBufferOffset == _readBufferCount, so the unconditional _readBufferOffset = 0 cannot discard unconsumed bytes.
Offset/count are assigned only after the await, so a throwing/cancelled read leaves the buffer state intact.
GetCharCount/GetChars use the same Encoding.UTF8 replacement fallback, so the counts cannot disagree on malformed input.
Both pooled buffers are returned on every exit path, including the mid-frame-disconnect return null.
Dispose() swapping _reader.Dispose() for _readStream.Dispose() is behavior-preserving: StreamReader owned the stream (leaveOpen: false), and both real call sites (MessageHandlerFactory and MtpServerProcess) pass the same NetworkStream for both directions.
Cancellation semantics are genuinely unchanged: the removed NET7_0_OR_GREATER/NET6_0_OR_GREATER ladder existed only for StreamReader.ReadLineAsync, which forwards the token to the same Stream.ReadAsync the new code calls directly.

To back the parts the PR's own tests do not reach, I wrote six throwaway probes against the fixed handler — a 20 KB body spanning many FillReadBufferAsync calls, a 5 KB header line forcing several lineBuffer doublings, a frame dribbled one byte at a time so header and body straddle refills at arbitrary offsets, bare-LF headers, and a peer that disconnects mid-body. All six pass: the buffer-boundary logic is correct. (Deleted afterwards — see the coverage note inline.)

Findings

Nothing blocking. Three non-blocking notes inline: a per-frame allocation on the hot read path, a test-coverage gap for the multi-iteration buffer paths, and a pre-existing unbounded Content-Length that this change makes strictly better rather than worse.

One process question

The src/ half of this fix is independent of #10085 and applies to main as-is, but the PR is stacked on nohwnd-mtp-client-source-package because the regression test lives in the new ServerClient.UnitTests project. That couples a shipping-server bug fix to the packaging work. Worth deciding explicitly whether you want the transport fix to be able to merge to main on its own (e.g. with the test placed in Microsoft.Testing.Platform.UnitTests instead) — entirely your call, and not a reason to hold this up if #10085 is landing shortly.

…ouple from microsoft#10085 Follow-up to the review on microsoft#10297. Three non-blocking findings plus the process question, all addressed. Reuse the header line buffer (review: per-frame allocation) ReadHeaderLineAsync allocated a fresh 256-byte array per header line, roughly three per frame, on the hottest read path -- server mode emits a testing/testUpdates/tests notification per test. The buffer is now an instance field grown in place. This is safe for the same reason _readBufferOffset and _readBufferCount already are: reads are single-threaded by construction, driven by exactly one read loop. Document the unbounded Content-Length as a conscious decision (review: observation) commandSize is still trusted unbounded. That is deliberate: server mode is a loopback channel to a test host this process launched, and legitimate bodies are unbounded in principle, so a cap would be a guess that risks rejecting valid traffic. Recorded in a comment rather than changed, and noted that the byte-based read more than halves the previous worst case, which rented commandSize chars. Cover the multi-iteration buffer paths (review: coverage gap) The previous tests all used frames under ReadBufferSize and header lines under HeaderLineInitialCapacity, so the two loops this change introduced only ever ran a single iteration. Three tests added: - a body far larger than the read buffer, so ReadExactlyAsync refills several times and the body spans buffer boundaries; - a header line far longer than the initial header buffer, so the growth path runs through several doublings; - a peer that announces a body then disconnects mid-way, which must surface as ReadAsync returning null rather than hanging or throwing. The first and third fail against the pre-fix transport; the second guards a path this change introduced. Decouple the fix from the source-package work (review: process question) The src/ half applies to main as-is, but the tests lived in the new ServerClient.UnitTests project, coupling a shipping-server bug fix to the packaging change. They now live in Microsoft.Testing.Platform.UnitTests, merged into the existing TcpMessageHandlerTests. That project targets net462 alongside net8.0/net9.0, so both the #if NETCOREAPP (System.Text.Json) and #else (Jsonite) compilation branches are still covered, and this PR no longer depends on microsoft#10085 for anything. Assertions were reworked to compare the JSON-RPC method rather than a params payload: params binding requires a registered method, so the previous payload-based assertions were not portable to a transport-level test outside the ServerClient harness. The non-ASCII content moved into the method name, which both formatters always bind, so the assertions still compare every multi-byte character rather than merely checking that a frame arrived. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 365edd84-6344-4ce4-9048-2c1ed1b7d939

…rage On .NET the System.Text.Json formatter escapes non-ASCII to \uXXXX, so that test's body reaches the wire as pure ASCII and it degrades to an ASCII round trip there. The docstring only hinted at this; state plainly which tests do carry the cross-TFM coverage so a future reader does not over-trust it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 365edd84-6344-4ce4-9048-2c1ed1b7d939

azat-msft · 2026-07-30T08:29:34Z

Thanks for the depth here — reverting TcpMessageHandler.cs to its parent to confirm the tests genuinely fail, and writing throwaway probes for the buffer-boundary paths, is exactly the review this deserved given it is live server transport code.

All three inline findings are addressed in a12f08a08, and I answered your process question below with evidence rather than an opinion.

Changes since your review

Per-frame allocation — lineBuffer is now the instance field _headerLineBuffer, grown in place. I also recorded the one consequence worth naming: it grows to the longest header line seen on the connection and stays there, bounded by the same trust boundary as Content-Length.

Unbounded Content-Length — behaviour unchanged, reasoning now recorded in a comment so it reads as a decision. Your observation that this change halves the previous worst case is part of that rationale.

Coverage gaps — three tests added, covering the ReadExactlyAsync refill loop across buffer boundaries, the _headerLineBuffer doubling path, and ReadExactlyAsync returning false on a mid-body disconnect. I verified the first and third fail against the pre-fix transport on both net8.0 and net462, so they are real regression guards rather than tests fitted to the implementation.

The process question — the fix is now genuinely independent of #10085

You were right that this was worth deciding explicitly, and acting on it turned out to be cheap. The tests have moved from ServerClient.UnitTests into Microsoft.Testing.Platform.UnitTests, merged into the existing TcpMessageHandlerTests. That project already multi-targets net462 alongside net8.0/net9.0, so both the #if NETCOREAPP (System.Text.Json) and #else (Jsonite) compilation branches are still covered — the reason the tests were in the new project in the first place was convenience, not necessity.

I verified the decoupling rather than assuming it. Rebasing all three commits onto current main (e08d1635b):

rebases cleanly, zero conflicts;
TcpMessageHandlerTests.cs is byte-identical on main and on this PR's base, so the tests land in the same place either way;
every symbol the new tests use (JsonRpcMethods.ClientLog/TelemetryUpdate, LogEventArgs, ServerLogMessage, TelemetryEventArgs, FormatterUtilities, TcpMessageHandler) exists on main in the same files;
the only difference in TcpMessageHandler.cs between main and this PR's base is an unrelated 5-line Substring-vs-range-operator change from Add a source-only MTP server-mode client package #10085, which none of my commits touch.

So the transport fix can merge to main on its own whenever you want it to. I have deliberately left the base as nohwnd-mtp-client-source-package rather than retargeting unilaterally — you have already approved this shape, and if #10085 is landing shortly the stacking is harmless. But it is now a one-command retarget if you would rather the shipping-server fix went in independently. Entirely your call; say the word and I will rebase.

One thing worth knowing either way: the fix is validated end to end outside this repo. A locally-built pack of the source package was consumed by the vstest retarget work, where CrossPlatEngine builds clean on net462/netstandard2.0/net8.0 and the MTP suites pass 140/140 and 16/16 — including the raw-UTF-8 peer traffic that is precisely what exposes this bug.

Test results after the changes

Suite	Before this PR	After
`Microsoft.Testing.Platform.UnitTests` net8.0	1440/1440	1447/1447
`Microsoft.Testing.Platform.UnitTests` net9.0	1440/1440	1447/1447
`Microsoft.Testing.Platform.UnitTests` net462	1416/1416	1423/1423
`ServerClient.UnitTests` net8.0	24/24	24/24
`ServerClient.UnitTests` net462	24/24	24/24
`MtpServerClientSourcePackage*` (contract + hostile consumer)	9/9	9/9
`MtpServerClientAcceptanceTests`	3/3	3/3
MTP acceptance `Server*`	14/14	14/14

The platform suites are up 7 (4 tests moved in from ServerClient.UnitTests, 3 new), and ServerClient.UnitTests is back to its original 24 — the arithmetic confirms nothing was lost in the move.

One coverage caveat I made explicit

While re-reviewing the committed change I confirmed something the docstring had only hinted at, and stated it plainly in 58bba249f: ReadAsync_HandlerWrittenFramesWithNonAscii_... cannot detect this bug on .NET. Utf8JsonWriter uses JavaScriptEncoder.Default, which escapes every non-ASCII character to \uXXXX, so on net8.0/net9.0 the body reaches the wire as pure ASCII, byte count equals char count, and the pre-fix reader would have passed it unchanged. It only exercises the regression on net462, where Jsonite emits BMP characters unescaped.

The cross-TFM coverage comes from ReadAsync_RawUtf8FramesFromPeer_... and ReadAsync_BodyLargerThanReadBuffer_..., which frame raw UTF-8 bytes themselves and so bypass formatter escaping on every TFM. This is also the reason the bug stayed latent for so long: MTP-to-MTP traffic is accidentally ASCII on the wire, and it only bites when the peer emits raw UTF-8 — which is exactly what an external client such as vstest's does. The docstring now says all of this outright so nobody over-trusts that test as cross-TFM coverage.

Replaces the interim local-feed pack with a build of microsoft/testfx#10297, which stacks the Content-Length byte/char fix onto microsoft#10085. The transport now reads exactly Content-Length bytes and UTF-8-decodes them, symmetric with the write path, and reads the headers through the same byte-level buffer so no StreamReader can buffer part of the body across the boundary. That drop also carries microsoft#10085's ServerRequestHandler signature change (the result is now constrained to a serializable dictionary), so FakeMtpServerClient is updated to match. Verification on this drop: - CrossPlatEngine builds clean on net462, netstandard2.0 and net8.0. - MTP unit tests 140/140 (70 per axis, net11.0 and net481). - MtpUnderVstestTests 16/16 on both console axes. Note the 16/16: the two /logger:trx failures reported against the earlier drop do not reproduce here, so they look like a local deployment issue rather than anything in the retarget. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cd0793f4-d530-44b6-b881-fed3be6aa52f

Evangelink

New review pass

Re-reviewed after a12f08a0 / 58bba249. The diagnosis and the fix are correct, and I verified it rather than reading it. Nothing below blocks; the first two are the ones I'd actually act on.

What I ran

Built Microsoft.Testing.Platform.UnitTests (net8.0/net9.0/net462) from the head commit:

Check	Result
`TcpMessageHandlerTests` — net8.0	8/8 pass
`TcpMessageHandlerTests` — net462	8/8 pass
Full `Microsoft.Testing.Platform.UnitTests` — net8.0	1932/1932 pass
Full `Microsoft.Testing.Platform.UnitTests` — net462	1891/1891 pass

Then I reverted only TcpMessageHandler.cs to dbb946d4~1, rebuilt, and re-ran, to confirm the tests are genuine failing-before regression tests rather than fitted to the new implementation:

failed ReadAsync_RawUtf8FramesFromPeer_DoesNotDesynchronizeSubsequentFrames
  System.Text.Json.JsonReaderException: 'C' is invalid after a single JSON value.
failed ReadAsync_LeadingByteOrderMark_IsSkipped
  System.Text.Json.JsonReaderException: 'C' is invalid after a single JSON value.
failed ReadAsync_BodyLargerThanReadBuffer_SpansMultipleRefillsWithoutDesynchronizing
  Timed out reading a frame; the transport is most likely desynchronized.
failed ReadAsync_PeerDisconnectsMidBody_ReturnsNull
  Timed out reading a frame; the transport is most likely desynchronized.

total: 8, failed: 4, succeeded: 4

4 of the 8 fail pre-fix on net8.0 alone. Claim confirmed.

Reader correctness

Points I specifically convinced myself of, since this is framing code where a subtle error desynchronizes a live connection:

Headers and body share one _readBuffer with one _readBufferOffset/_readBufferCount pair, and both ReadHeaderLineAsync and ReadExactlyAsync drain it before refilling. That's the invariant the whole fix rests on and it holds — no bytes can be stranded on the wrong side of a buffering boundary, which is exactly the trap that made the naive "headers via StreamReader, body via BaseStream" fix wrong.
Header lines are accumulated as bytes and decoded once at the terminator, so a multi-byte character — or the BOM itself — split across a refill is handled correctly. Easy to get wrong with a per-chunk decode; this doesn't.
TcpMessageHandler is the only IMessageHandler implementation in the tree, so there's no sibling transport carrying the same defect.
The token reaching ReadAsync is _testApplicationCancellationTokenSource.CancellationToken (PassiveNode), i.e. connection-lifetime. So the net462 WithCancellationAsync pattern abandoning a read that still owns _readBuffer is a teardown-only concern, not a mid-connection corruption risk. Worth having checked given the buffer is now shared instance state rather than a per-call local.
Dispose swapping _reader.Dispose() for _readStream.Dispose() is equivalent — StreamReader.Dispose disposed the underlying stream anyway.

1. The .NET path now transcodes bytes → chars → bytes

Json.Deserialize<T>(ReadOnlyMemory<char> utf8Json) calls JsonDocument.Parse(ReadOnlyMemory<char>), which immediately transcodes back to UTF-8 into its own pooled array:

// System.Text.Json
public static JsonDocument Parse(ReadOnlyMemory<char> json, JsonDocumentOptions options = default)
{
    ReadOnlySpan<char> jsonChars = json.Span;
    int expectedByteCount = JsonReaderHelper.GetUtf8ByteCount(jsonChars);
    byte[] utf8Bytes = ArrayPool<byte>.Shared.Rent(expectedByteCount);
    ...

So on #if NETCOREAPP the new code does GetCharCount + Rent<char> + GetChars, and STJ then undoes all three — while the raw UTF-8 bytes we started from were already sitting in bodyBuffer. JsonDocument.Parse(ReadOnlyMemory<byte> utf8Json) is the overload that wants exactly what we have.

An IMessageFormatter.Deserialize<T>(ReadOnlyMemory<byte>) overload (NETCOREAPP only — net462/Jsonite keeps Encoding.UTF8.GetString) removes one transcode and one pooled rent per frame on the hottest read path, which is the same path this PR is otherwise careful to keep allocation-free.

Worth doing partly for a non-performance reason: Deserialize<T>(ReadOnlyMemory<char> **utf8Json**) and MessageFormatter.Deserialize<T>(ReadOnlyMemory<char> **serializedUtf8Content**) are a char-typed parameter with a utf8-shaped name. That naming is the byte/char ambiguity this PR exists to fix, still present one layer down. Fine as a follow-up, but it belongs on the record next to this fix rather than rediscovered later.

2. Retarget to `main`

Base is nohwnd-mtp-client-source-package (#10085), still open. This fixes the shipping server-mode transport, and the description already establishes it rebases onto main cleanly with the tests landing in a project that exists there. I'd retarget now rather than leave a live-transport fix gated on an unrelated packaging PR.

3. Negative `Content-Length` crashes the read loop (verified)

The new comment says commandSize is deliberately trusted unbounded, and I agree with that reasoning for large values. But negative values aren't just unbounded — they throw. I probed it:

Content-Length: -5

PROBE-RESULT: threw System.ArgumentOutOfRangeException:
  minimumLength ('-5') must be a non-negative value. (Parameter 'minimumLength')

int.TryParse happily yields -5, the is -1 guard doesn't catch it, and ArrayPool<byte>.Shared.Rent(-5) throws past the SocketException/IOException filter — so a malformed header takes down the read loop with an unrelated exception type instead of the graceful null disconnect every other malformed-frame path produces. (net462 gets OverflowException from new byte[-5].)

Pre-existing, and I wouldn't ask for it normally. But the PR now carries a comment asserting this input was consciously reasoned about, and if (commandSize < 0) { return null; } is one line that makes the comment true.

4. The trust-boundary argument is weaker for `_headerLineBuffer` than for the body

The comment justifies unbounded header-line growth by pointing at Content-Length's trust boundary. The two aren't equivalent: bodies are legitimately unbounded (a stack trace, captured stdout), which is what makes refusing to cap them correct. Header lines have no legitimate large case, and a peer that simply never sends \n doubles the buffer forever. If a cap is ever wanted, headers are both cheaper and more defensible than the body — worth saying so rather than grouping them.

5. Undocumented line-terminator delta

StreamReader.ReadLineAsync treated a bare \r as a terminator; ReadHeaderLineAsync breaks only on \n. Protocol-irrelevant, and arguably stricter-is-better — but the comment reads as if it enumerates the cases:

// Tolerate both CRLF and a bare LF.

Adding "a bare CR is not a terminator, unlike StreamReader" closes it.

Test nits

ConnectedHandlers.CreateAsync leaks listener and clientSocket if ConnectAsync/AcceptTcpClientAsync throws.
WriteRawFrame uses blocking stream.Write, and every frame is written before any read starts. ReadAsync_BodyLargerThanReadBuffer_... pushes ~27 KB that way and relies on socket buffers absorbing it. Fine today, but the margin is invisible in the test, so growing the payload later turns a clear assertion failure into a CI hang. Starting the read first — or a comment naming the constraint — makes the dependency explicit.
ReadWithTimeoutAsync leaves an uncancelled 30 s Task.Delay per call.
ReadAsync_LeadingByteOrderMark_IsSkipped carries NonAsciiMethod, so pre-fix it fails for the byte/char reason rather than isolating BOM handling. I checked it does still catch a BOM-only regression (the \uFEFF prefix makes the first header unparseable → contentSize stays -1 → ReadAsync returns null → the cast yields null → NRE on .Method), so the coverage is real. The failure mode just won't point at the BOM.

Good change — the byte-level rewrite is the right shape rather than a patch over ReadBlockAsync, and the pre-fix failure evidence holds up under independent reproduction. My only real asks are the retarget (#2) and, if you want it in scope, the one-line negative guard (#3).

azat-msft mentioned this pull request Jul 28, 2026

Harden the MTP client: UTF-8 framing, shutdown, uid filter, and tests nohwnd/vstest#12

Merged

Evangelink self-requested a review July 29, 2026 07:37

Evangelink approved these changes Jul 29, 2026

View reviewed changes

azat-msft and others added 2 commits July 30, 2026 10:14

Evangelink reviewed Jul 30, 2026

View reviewed changes

Evangelink merged commit cc44d19 into microsoft:nohwnd-mtp-client-source-package Jul 30, 2026
58 checks passed

Evangelink mentioned this pull request Jul 30, 2026

Harden the server-mode transport read path #10346

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix Content-Length byte/char mismatch in the server-mode TCP transport - #10297

Fix Content-Length byte/char mismatch in the server-mode TCP transport#10297
Evangelink merged 3 commits into
microsoft:nohwnd-mtp-client-source-packagefrom
azat-msft:azat-msft-fix-mtp-utf8-frame-length

azat-msft commented Jul 28, 2026 •

edited

Loading

Uh oh!

Evangelink left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

azat-msft commented Jul 30, 2026

Uh oh!

Evangelink left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

azat-msft commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The bug

This affects the live server, not just the new client package

The StreamReader buffering hazard

Tests

Coverage caveat worth knowing

Failure output before the fix

Suite results (before → after)

Relationship to #10085

Provenance

Uh oh!

Evangelink left a comment

Choose a reason for hiding this comment

Summary

What I verified (not just read)

Correctness review of the new read path

Findings

One process question

Uh oh!

Uh oh!

Uh oh!

Uh oh!

azat-msft commented Jul 30, 2026

Changes since your review

The process question — the fix is now genuinely independent of #10085

Test results after the changes

One coverage caveat I made explicit

Uh oh!

Evangelink left a comment

Choose a reason for hiding this comment

New review pass

What I ran

Reader correctness

1. The .NET path now transcodes bytes → chars → bytes

2. Retarget to main

3. Negative Content-Length crashes the read loop (verified)

4. The trust-boundary argument is weaker for _headerLineBuffer than for the body

5. Undocumented line-terminator delta

Test nits

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

azat-msft commented Jul 28, 2026 •

edited

Loading

The `StreamReader` buffering hazard

2. Retarget to `main`

3. Negative `Content-Length` crashes the read loop (verified)

4. The trust-boundary argument is weaker for `_headerLineBuffer` than for the body