Harden pre-latch ANNOUNCE handling and bound resend/ttl edge cases - #40
Merged
Conversation
Five robustness fixes surfaced by a pre-release audit; each was reproduced and is covered by tests that run on Linux/macOS. * Pre-latch ANNOUNCE DoS. Before a receiver latched a session, a single forged ANNOUNCE with an invalid file_size/chunk drove announceValid() to a fatal exit, so one unauthenticated datagram killed `filecast receive` while it was still "Waiting for a sender...". The post-latch path already guarded against this (see e2e_hijack); the pre-latch window did not. Validate with the side-effect-free Protocol::announceInRange() *before* latching or refreshing the deadline, and ignore an invalid one (warn and keep waiting) instead of exiting. Drops the now-unused announceValid(). New e2e_prelatch test fires two poison ANNOUNCEs, checks the receiver survives, then requires the real transfer to complete and verify. * Recovery RESEND burst ignored Ctrl+C and the wall-clock deadline for its whole duration. Re-requesting a large missing set (with --rate/--delay-ms pacing) left the receiver unresponsive for many seconds. Check g_interrupted and the deadline inside the re-request loop too, not just in the outer loop. * MAX_FILE_LENGTH (4ULL*1024*1024*1024) wraps to 0 on a 32-bit size_t, making the receiver reject every non-empty file. Use the exact v3 wire limit 0xFFFFFFFF, which fits size_t on every target. * --ttl was only checked for > 0. ttl*10 seconds (the sender's resend-phase deadline) overflows a steady_clock time_point in nanoseconds at absurd values. Cap it at 86400 (24h). * serveResends() treated a zero-length datagram as a timeout (result <= 0), letting a peer spraying empty packets drain the resend phase early. Split result < 0 (timeout, count ttl down) from result == 0 (ignore), mirroring the receiver's length==0 handling. Build is clean under -Wall -Wextra; all 10 ctest cases pass.
Condense the five explanatory comment blocks to one or two lines each, matching the surrounding code's terse style. No code changes.
The MAX_FILE_LENGTH = 0xFFFFFFFF fix let a receiver accept a max-size ANNOUNCE, but totalParts() still computed (file_length + chunk_size - 1) / chunk_size. On a 32-bit size_t that add wraps for file_length near the 0xFFFFFFFF wire limit, collapsing the part count to ~0: the receiver then rejects every data part (part >= 0) and treats the transfer as 0/0 complete, and the sender (same inlined expression) ships 0 parts. 64-bit targets are unaffected; the top chunk_size bytes of the 4 GiB range were the only broken band on 32-bit. Use overflow-safe ceiling division (file_length / chunk_size + remainder != 0) in the shared helper and route Sender through it. Pin the boundary in the TotalPartsBoundaries unit test.
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.
Summary
Five robustness fixes from a pre-release audit. Each was reproduced locally and is covered by a test that runs on Linux/macOS. Build is clean under
-Wall -Wextra; all 10ctestcases pass.The headline fix is a one-packet DoS: before a receiver latched a session, a single forged
ANNOUNCEcould killfilecast receivewhile it was still waiting for a sender.Fixes
🔴 Pre-latch ANNOUNCE DoS (
Receiver.cpp)Before a receiver latched a session, a forged
ANNOUNCEwith an invalidfile_size/chunkdroveannounceValid()to a fatalexit(2)— so one unauthenticated datagram killedfilecast receivein the very window it spends "Waiting for a sender...". The post-latch path already guarded against this (seee2e_hijack); the pre-latch window did not.Protocol::announceInRange()before latching or refreshing the deadline.announceValid().Reproduction: a single
file_size=0ANNOUNCE made the receiver exit 2 in ~0.1s; it now logs a warning and waits out its full ttl.🟡 RESEND burst ignored Ctrl+C / deadline (
Receiver.cpp)The recovery re-request loop checked
g_interruptedand the wall-clock deadline only in its outer loop, so re-requesting a large missing set (with--rate/--delay-mspacing) left the receiver unresponsive for many seconds. Now checked inside the burst too.🟡 32-bit
size_toverflow on max-size files (Receiver.cpp,Protocol.hpp,Sender.cpp)Two overflows on the same 4 GiB boundary, both harmless on a 64-bit
size_t:MAX_FILE_LENGTH:4ULL*1024*1024*1024wraps to0on a 32-bitsize_t, making the receiver reject every non-empty file. Use the exact v3 wire limit0xFFFFFFFF, which fitssize_ton every target.totalParts()(follow-up from review): with the limit fixed a max-sizeANNOUNCEis now accepted, but(file_length + chunk_size - 1) / chunk_sizestill wraps a 32-bitsize_tforfile_lengthin the topchunk_sizebytes of the range, collapsing the part count to ~0 — the receiver then rejects every data part (part >= 0) and calls the transfer 0/0 complete, and the sender (same inlined expression) ships 0 parts. Switched to overflow-safe ceiling division (file_length / chunk_size + (remainder != 0)) in the shared helper, withSenderrouted through it. Contained by the existingpart >= totalbound, so a correctness bug, not OOB/DoS.🟢
--ttloverflow guard (Main.cpp)--ttlwas only checked for> 0;ttl*10seconds (the sender's resend-phase deadline) overflows asteady_clocktime_point in nanoseconds at absurd values. Capped at86400(24h).🟢 Zero-length datagram drained the resend phase (
Sender.cpp)serveResends()treated a zero-length datagram as a timeout (result <= 0), letting a peer spraying empty packets drain the resend phase early. Splitresult < 0(timeout) fromresult == 0(ignore), mirroring the receiver'slength == 0handling.Tests
e2e_prelatch: fires two poison ANNOUNCEs (empty file, out-of-range chunk) at a receiver before the real sender starts, checks it survives, then requires the real transfer to complete and verify.e2e_hijack.sh/hijack_proxy.pythat referenced the removedannounceValid().TotalPartsBoundariesunit test now pins the0xFFFFFFFFmax-wire-size counts (64- and65489-byte chunks) — a real regression guard on 32-bit builds, a smoke test on 64-bit.ctest: 10/10 pass (was 9, pluse2e_prelatch).Deferred (follow-up issues)
Unbounded
sent_partgrowth under a full-loss RESEND storm, and three Windows-only items (WSAGetLastErrorin send diagnostics,WSAStartupordering vsinet_pton, symlink guard on.part.idx) that need CI/Windows to verify.