Fix -jm jitter CSV corruption: only decode the header at a real buffer boundary - #34
Open
robster7674 wants to merge 1 commit into
Open
robster7674 wants to merge 1 commit into
robster7674 wants to merge 1 commit into
Conversation
…undary AddPayloadToBuffer() embeds the packet_num/send_count/send_freq header once, at the start of each logical buffer_length-sized buffer passed to send(). On a stream socket a single recv() is not guaranteed to return one full such buffer - especially once buffer_length exceeds one TCP segment/receive-buffer worth (e.g. -l 1M) - so OutputPayloadFromBuffer() was being called on every raw recv() return and decoding whatever bytes happened to sit at that chunk's offset 0. Once the transfer desyncs from buffer-aligned reads (observed after the first few packets on a live WS2025 test), most rows in the jitter CSV decode the buffer's own fill pattern instead of a real header. Track bytes already consumed into the current logical buffer per thread (jitter_buffer_offset) and only call OutputPayloadFromBuffer() when a recv() lands on offset 0 of a new buffer with enough bytes for the 20-byte header; otherwise skip decoding that chunk but keep advancing the offset so later recv()s stay aligned to the real buffer boundaries on the wire. Reproduced live (ntttcp 5.40, Windows Server 2025, TCP, -l 1M): a control run with -jm removed completes with no errors; the same command with -jm produces 'Unexpected disconnect'/'error in send/recv' and a CSV where most rows are the literal ASCII fill byte 'A' (0x4141414141414141) reinterpreted as the header fields. Reproduced identically at both 1 and 2 connections, ruling out a concurrency-specific cause. Not yet re-verified against a compiled build (opening for review/discussion; happy to build and re-test against the same live hosts once there is agreement this is the right fix shape).
Author
|
@microsoft-github-policy-service agree |
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
1 participant
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.
The bug
-jm's receiver side (OutputPayloadFromBuffer, called fromDoSendsReceives) decodes the 20-byte jitter header (packet_num,send_count,send_freq) from the first bytes of whatever the currentrecv()call just returned - unconditionally, on every single receive.AddPayloadToBufferon the sender side embeds that header exactly once, at the very start of each logicalbuffer_length-sized buffer handed tosend(). On a TCP stream socket, onerecv()call is not guaranteed to return one full such buffer - the OS is free to deliver it in smaller chunks, and for anybuffer_lengththat exceeds a single TCP segment/receive-buffer's worth (e.g.-l 1M), it reliably does. Once delivery desyncs from buffer-aligned reads (observed after the first few packets on a live run),OutputPayloadFromBufferdecodes whatever bytes happen to be at that chunk's offset 0 - almost always the buffer's own fill pattern, not a header - and the CSV fills with garbage instead of real counters.Live repro (not a synthetic worst case)
ntttcp 5.40, Windows Server 2025, TCP,
-l 1M, single connection, one thread. A run with-jmproduces a CSV where most rows read like:1094795585=0x41414141and4702111234474983745=0x4141414141414141- the literal ASCII fill byte'A'repeated, decoded as if it werepacket_num/send_count/send_freq. A control run with identical parameters minus-jmcompletes with no errors - isolates the defect to the-jmpath specifically.The fix
Track how many bytes have already been consumed into the current logical buffer, per thread (
jitter_buffer_offset). Only decode a header when arecv()call lands on offset 0 of a new logical buffer and carries at least the 20 header bytes; otherwise skip decoding that chunk (there is no header in it) while still advancing the running offset, so laterrecv()calls stay aligned to the real buffer boundaries on the wire.This is deliberately conservative rather than a full byte-stream reassembly: on a large
buffer_lengthsplit across many smallrecv()s, this samples less densely than "one header per logical buffer" would in the best case, but it never decodes a false header. Correctness over sampling density seemed like the right tradeoff for a first pass; happy to discuss a fuller reassembly approach if that's preferred.Verification
Live-tested on the same two-VM setup as the repro above, after building via this fork's own CI (GitHub Actions,
windows-latest, unmodified toolset):-jm-jm-jm)Repeated the full verification a second time, independently, on a fresh live setup: 3088 and 5388 data rows, again 0 corrupted in both.
Not otherwise touched: buffer size validation,
-jm's CLI parsing, the CSV header line, or anything outside this one receive-side decode path.