[Security review][L4] Undeliverable datagrams silently strand transfers at large --mtu - #33
Merged
Merged
Conversation
A TRANSFER datagram is TRANSFER_HEADER (18) + chunk bytes, but --mtu and MAX_CHUNK both allowed a chunk up to 65507. With --mtu in [65490, 65507] every non-final part became a 65508..65525-byte datagram, which sendto() rejects with EMSGSIZE; sendPart swallowed that as a warning and returned true, so the sender 'finished' while each >64 KiB part silently never left (and was never re-sent on RESEND). Derive MAX_CHUNK from MAX_UDP_PAYLOAD - TRANSFER_HEADER (65489) and bound --mtu by the same constants, so the framing overhead can never push a datagram past the IPv4 UDP limit. Update the README range and tests.
7 tasks
Capping --mtu at 65489 fixed only the IPv4 arithmetic ceiling. The real L4 root cause is that sendPart swallowed every sendto() error as a warning and returned true, so a permanent EMSGSIZE silently stranded the transfer: the sender 'completed' while the receiver timed out. On macOS this bites at any --mtu above ~9198, because net.inet.udp.maxdgram defaults to 9216. - sendPart: treat EMSGSIZE/WSAEMSGSIZE as fatal (return false, clear error) instead of a swallowed warning; other (transient) errors are unchanged and still recoverable via RESEND. - Reject an oversized --mtu at sender startup, before hashing the whole file, by comparing the datagram size against the platform's UDP send limit (net.inet.udp.maxdgram on macOS/BSD, else the 65507 IPv4 ceiling). - Add an e2e regression test asserting an oversized --mtu is rejected (never reported as sent) and the largest deliverable --mtu still transfers. Verified on macOS: --mtu 65489 now exits 1 with 'use --mtu <= 9198' instead of a silent broken transfer; --mtu 9198/8000/1500 transfer and verify.
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
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.
Problem (L4)
A TRANSFER datagram is
TRANSFER_HEADER (18) + chunkbytes. Two compounding issues made a large--mtusilently corrupt transfers:--mtuwas accepted up to 65507, so--mtu ∈ [65490, 65507]produced a 65508…65525-byte datagram — over the 65507-byte IPv4 UDP limit.sendPart()treated anysendto()error as a warning and returnedtrue:EMSGSIZEis permanent (RESEND retries hit the same wall), so the sender "finished" successfully while the receiver got a broken/timed-out transfer.This bites hard on macOS/BSD, where
net.inet.udp.maxdgramdefaults to 9216 — so any--mtuabove ~9198 reproduced the silent-stranding symptom, even after the cap.Runtime proof (macOS, before the root-cause fix):
send --mtu 65489printedSent ... at 625 MiB/s(success) while the receiver reportedError: Transfer timed out with 3 part(s) missing; only the small tail part was delivered.Fix
Cap (arithmetic ceiling):
Protocol:MAX_UDP_PAYLOAD = 65507;MAX_CHUNK = MAX_UDP_PAYLOAD - TRANSFER_HEADER(65489).--mtubounded to[MIN_CHUNK, MAX_CHUNK]via the shared constants.Root cause (no more silent success):
sendPart()now treatsEMSGSIZE/WSAEMSGSIZEas fatal (return false, clear error); other, transient errors are unchanged and still recoverable via RESEND.--mtuat startup, before hashing the whole file, by comparing the datagram size against the platform's UDP send limit (net.inet.udp.maxdgramon macOS/BSD, else the 65507 IPv4 ceiling), with an actionable message (use --mtu <= N).Docs & tests:
64..65489.AnnounceInRange).--mtumust be rejected (never reported as sent), and the largest deliverable--mtumust still transfer.Verification (macOS, after)
--mtu 65489→ exits 1:Error: --mtu 65489 needs a 65507-byte datagram, over this system's UDP send limit of 9216; use --mtu <= 9198.--mtu 9198(datagram == 9216 limit) → transfers, sha256 verified;--mtu 9199→ rejected.--mtu 8000/1500→ transfer and verify.Reviewed via an adversarial multi-dimension pass (correctness / portability / edge-cases + tests); no blockers or majors, only cosmetic nits (one of which — a
size_tunderflow in the hint string under a pathological sub-18-byte sysctl — is addressed).