fix(proto): ignore coalesced datagram tail for unknown path - #782
fix(proto): ignore coalesced datagram tail for unknown path#782axelbaumlisto wants to merge 5 commits into
Conversation
`handle_coalesced` accounted for the datagram via `path_data_mut(path_id)`, which
panics with `expect("known path")` when the path is not in `paths`. Handling the
first packet of a datagram can remove the path, so the coalesced remainder must
not assume it still exists.
Discard the remainder instead, matching the existing unknown-path handling in
`process_decrypted_packet`.
97a4925 to
659ef7a
Compare
`stale_coalesced_datagram_after_path_discard_is_ignored` inlined the whole connect-and-capture sequence. Move it behind `connect_capturing_coalesced_datagram` and `CoalescedDatagram::replay` so the setup can be shared.
Exercises a coalesced datagram whose path id is in neither `paths` nor `abandoned_paths`, which `early_discard_packet` does not cover. The existing `stale_coalesced_datagram_after_path_discard_is_ignored` only covers the abandoned case.
659ef7a to
a82ab6e
Compare
divagant-martian
left a comment
divagant-martian
left a comment
There was a problem hiding this comment.
The direction is good
| self.path_data_mut(path_id) | ||
| .inc_total_recvd(data.len() as u64); | ||
| let Some(path) = self.paths.get_mut(&path_id) else { | ||
| // Handling the first packet of this datagram may have removed the path, so the |
There was a problem hiding this comment.
I've perused the code and I'm certain this is not true. The bug does exist but this is not the justification. The path might only be discarded when the timer expires, which would need a call to handle_timeout in between the first packet and the coalesced one. proto is a state machine so this is not something achievable with a data race either
There was a problem hiding this comment.
You're right. paths.remove lives only in discard_path, whose sole caller is handle_timeout — no packet removes a path mid-datagram. The real gap: an id in neither paths nor abandoned_paths passes both early_discard_packet guards. Comment rewritten.
| #[test] | ||
| fn stale_coalesced_datagram_after_path_discard_is_ignored() { | ||
| let _guard = subscribe(); | ||
| /// A coalesced handshake datagram captured before the client finished connecting, together |
There was a problem hiding this comment.
this bug exists in part because the coalesced datagram must arrive later than the handshake. A handshake datagram will always belong to PathId::Zero which is guaranteed to not escape the existing checks
There was a problem hiding this comment.
Agreed, this doesn't arise from the network — I checked, a handshake dst_cid is registered as PathId::ZERO explicitly. Keeping the test as a defensive contract: the production SIGABRT was real, but it proves no more than that.
|
|
||
| impl CoalescedDatagram { | ||
| /// Replays the datagram on `path_id`, as if it had just arrived from the network. | ||
| fn replay(self, pair: &mut ConnPair, path_id: PathId) { |
There was a problem hiding this comment.
This function makes readability slightly worse because the bug is achievable with a public api (handle_event) but the call is hidden behind a very specific helper. Please make this just to_connection_event (or something similar) receiving now and path_id, so that the public api call is explicit in the tests
There was a problem hiding this comment.
Fair. replay is now to_connection_event(now, path_id), with handle_event visible in each test. Worth noting: with the guard reverted the existing abandoned-path test still passes — only the never-opened one panics.
…l explicit Three review comments, all correct. Verified each against the code before changing anything. 1. The comment's mechanism does not exist. It claimed handling the first packet may have removed the path. `paths.remove()` appears once, in `discard_path`, whose only caller is `handle_timeout` — so a packet cannot remove a path mid-datagram, and as the reviewer notes, a synchronous state machine cannot get there by a data race either. The real reason an unknown id arrives here: `early_discard_packet`'s handshake guard needs `is_handshaking()`, and its discarded-path guard needs the id to be in `abandoned_paths`. An id that was never opened is in neither map, so the datagram proceeds; `process_decrypted_packet` drops the first packet through its own unknown-path check while the coalesced remainder assumed that lookup had succeeded. The comment now says that instead. 2. On the handshake/PathId::ZERO point — correct, and worth recording why replaying under another id is still a faithful model rather than a fabricated input: the receiver never reads the path id off the wire. The endpoint resolves it as `connection_ids[dst_cid]`, so attribution is local receiver state, not something a sender chose. Noted in the test. 3. `replay` is gone. `to_connection_event(now, path_id)` returns the event and each test calls `handle_event` itself, so the public API call that panics is visible where it matters. Evidence the new test earns its place: with the guard reverted, `coalesced_datagram_for_never_opened_path_is_ignored` panics at the `expect` while the pre-existing `stale_coalesced_datagram_after_path_discard_is_ignored` still PASSES — the abandoned-path case does not exercise this panic at all. 388 noq-proto tests pass, clippy clean, both touched files rustfmt-clean.
Removes every comment this PR added: the rationale block in `handle_coalesced`, the doc comments on `CoalescedDatagram` / `to_connection_event` / `connect_capturing_coalesced_datagram`, and the explanation block in the new test. One of them had to go regardless of style preference. The test carried an argument that replaying under a different path id is "a faithful model of the receive path" because the endpoint resolves the id from `connection_ids[dst_cid]` rather than the wire. I checked that after the review and it does not hold: a handshake `dst_cid` is registered as `PathId::ZERO` explicitly, so local state says ZERO too. Leaving a claim in the code that I had already conceded in the review thread would have been worse than leaving no comment at all. Code and tests are unchanged. 388 noq-proto tests pass; with the guard reverted, `coalesced_datagram_for_never_opened_path_is_ignored` still panics at the `expect` while `stale_coalesced_datagram_after_path_discard_is_ignored` still passes, so the new test is carrying the coverage on its own.
2 participants
Description
Fixes #781.
handle_coalescedaccounted for the datagram withpath_data_mut(path_id), which panics viaexpect("known path")when the path id is not inpaths. Handling the first packet of a datagram can remove the path, so the coalesced remainder must not assume it still exists.The remainder is now discarded, matching the existing unknown-path handling in
process_decrypted_packet.Also adds a test for a path id present in neither
pathsnorabandoned_paths, whichearly_discard_packetdoes not cover — the existingstale_coalesced_datagram_after_path_discard_is_ignoredonly covers the abandoned case. It panics atmod.rs:4163without the fix and passes with it, while the existing test stays green, confirming the two cover different cases.The connect-and-capture setup the new test needs was inlined in the existing one, so it is extracted first (
connect_capturing_coalesced_datagram,CoalescedDatagram::replay) and both tests share it.Breaking Changes
None.
Notes & open questions
Three further
expect("known path")call sites remain (migrate,populate_packet); those look like genuine internal invariants and are untouched here.Change checklist
proposed change and wrote an as clear and concise description as
they could.
intented effect.