Two review findings on the fragment reassembler.
1. A whole datagram could vanish. entry.total was assigned before addRange
was asked, so a last fragment (MF=0) whose range was already covered by
MF=1 fragments answered fragInsertDuplicate and returned nil — while the
entry was already complete(). Nothing re-examined it, because every later
fragment is a duplicate too, so it died at its deadline with all its bytes
present. A duplicate now falls through to the completion check: it
contributes no bytes (held bytes still win) but it does contribute the
total length. This is what the documented first-wins policy always
implied; the code just did not do it.
The sender needed is non-conforming, so the old behaviour was safe rather
than exploitable — but it contradicted the comment three screens up, and
that comment is the next reader's only defence.
Also closed positively: a last fragment declaring an end BELOW the bytes
already held now poisons the datagram instead of quietly never completing.
2. The 5 s timeout was not a memory ceiling and the comment said it was.
sweep ran only when a NEW key was created, so once fragmented traffic
stopped, up to fragMaxEntries entries stayed resident indefinitely.
Both halves are fixed, and the honest one is the comment. sweep now runs
on EVERY fragment — an O(64) scan on a path that is already the rare one —
which releases residue as soon as any fragment arrives instead of waiting
for an unrelated new datagram. That still does not cover total silence, so
fragTimeout now documents the guarantee the code actually keeps: bounded
by fragMaxEntries/fragMaxTotalBytes at all times, released on the next
fragment, NOT "freed within 5 s".
No timer, deliberately: it would need a goroutine with a lifecycle tied to
something returnDeviceWrapper has no teardown hook for, and a goroutine
that must be stopped and might not be is a failure this project has
already paid for — to reclaim at most ~1.1 MiB that only exists after
fragmented traffic has already happened. What bounds growth is the byte
and entry ceiling; this timeout's job is correctness, and for that a
check driven by the arriving fragment is exact.
The now-unreachable per-key deadline check is removed rather than left as
dead defence in depth.
16 mutations, all red. M15 (duplicate returns early again) reds only the
buggy case while the control and the poison case stay green, so the test is
shown able to see both an assembled datagram and a lost one. M17 (sweep back
inside the new-key branch) reds the new test while both old timeout subtests
stay green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BHw89tdWddzhjUc4bAH4tS
omarandClaude Opus 5
b91fba1295