--- id: implementation/fix-bind-reply2-reentry name: "Fix — BIND reply#2 re-entry guard + consumer session hygiene (review-002 C-1, D-1, N-6, S-2)" status: completed depends_on: [] scope: narrow risk: medium impact: component level: implementation --- ## Description Close review 002's one reachable silent-corruption path plus the consumer-side residues that ride the same file (`docs/reviews/002-protocol-core-implementation-review.md`): 1. **C-1 (major) — the reply#2 phase is re-entrant with no spent-marker.** `BindSession::await_inbound` (`src/consumer.rs:757-763`) takes `&mut self`, leaves `inner` populated, and returns a `BindDataPlane<'_>` borrowing the same stream — nothing marks the phase consumed. A second call runs `read_reply2` against the *live data plane* (post-reply#2 bytes are the accepted peer's raw application data): a baffling `UnsupportedSocksVersion` at best, silent 10+-byte data corruption at worst (any peer protocol whose first byte is 0x05). The same shape replays through `reply1()` twice → `BindReply1::await_inbound`. The sibling sessions are all spent-state-guarded; this is the gap. Fix per the review's sketch: make the reply#2 phase consumed in the session's established posture — `inner.take()` (a second call hits the `None` arm and returns the existing `Ok(None)` spent shape, which the method's doc *already* claims: "`None` after a taken plane") or an equivalent spent-marker riding the session. Note the take-shape's ownership consequence: `BindDataPlane` comes to own the stream (the `TakenInner` pairing posture `Socks5ChannelSession::into_inner` already pins) while the session stays the teardown owner for the channel entry. Either mechanism is acceptable; the spent shape must match the session family's posture (`Ok(None)` or the typed `SpentSessionError` — pick one, document it). 2. **D-1 (minor) — `BindReply1::await_inbound` is dead surface** (0% covered; every test drives the session's method). The choice interacts with C-1 — pick one normative reply#2 reader: - (a) the session's `await_inbound` (the review's lean: document `reply1()` as a view, demote `BindReply1::await_inbound` to a private helper or cut it), or - (b) `BindReply1::await_inbound` (the task spec's intent — `tasks/implementation/core-consumer-bind.md:74-75`; the session's method delegates to it and the handle is tested through). The alternate path must be impossible to double-read (private, cut, or spent-guarded). 3. **N-6 (observation) — the three session constructors share a near-identical prologue** (open → `BiStream::from_joined` → negotiated → teardown-on-err; `src/consumer.rs:121-149`, `290-324`, `702-733`). Collapse into one private `open_and_negotiate` helper — the teardown-at-birth invariant in one place instead of three (a batch-2 constructor then cannot hand-copy the reap path). Zero behavior change. 4. **S-2 (observation) — one doc line on the three sessions' `reap`** (`src/consumer.rs:197-202`, `490-495`, `787-792`): the dropped `teardown_channel` `JoinHandle` is deliberate — the producer-side pump task finishes via EOF propagation (the graceful shape); `clear_all` aborts instead (the substrate's abrupt path). No code change; prevents a future reader "fixing" the handle into an abort. Tests: double `await_inbound` returns the spent shape AND the data-plane bytes survive untouched (the pinning test); the `reply1()`-twice path closed the same way; the existing BIND suite stays green unchanged (the round-trip, gate-refusal, factory/ accept-failure mappings unaffected). ## Acceptance Criteria - [ ] A second `BindSession::await_inbound` cannot re-read the data plane: it returns the spent shape, pinned by test, with the data-plane bytes asserted untouched - [ ] The `reply1()`-twice re-entry path is closed (the D-1 decision: one normative reader; the alternate private/cut/ spent-guarded) — the choice recorded in the task Notes - [ ] The session types' docs match the landed spent posture (the "`None` after a taken plane" claim is true of the code) - [ ] The three constructors share one `open_and_negotiate` helper; zero behavior change (the full consumer suite green unchanged) - [ ] The three `reap` sites carry the EOF-propagation doc line - [ ] `cargo test` green; clippy/fmt clean; both wasm checks clean ## References - docs/reviews/002-protocol-core-implementation-review.md §C-1, §D-1, §N-6, §S-2 - docs/architecture/consumer.md (the session postures; the spent-session shape) - tasks/implementation/core-consumer-bind.md (the landed BIND session; the `BindReply1` handle's spec'd intent) - tasks/implementation/review-protocol-core.md (R-2's `SpentSessionError` shape — the consistency target) ## Notes > Filled by the implementation agent (2026-09-24): - **C-1 landed as the take-shape** (the review's first sketch): `BindSession ::await_inbound` takes the session's inner stream before reading reply#2 (`inner.take()`); the returned `BindDataPlane` **owns** the `Socks5Stream` while the session stays the teardown owner — the `TakenInner` pairing posture `Socks5ChannelSession::into_inner` pinned, now carried by `BindDataPlane` itself (no new pairing type — the plane is the pair's inner half; the session is the pair's teardown owner). A second call hits the `None` arm → the family's spent shape `Ok(None)` (the method's doc claim "`None` after a taken plane" is now true). The spent shape chosen is **`Ok(None)`** (not a typed `SpentSessionError`): the method's existing return type `Result>` already carried the documented spent arm, and the sibling read-shaped surfaces (`recv_datagram`'s spent stream) return the EOF-shape `Ok(None)` — the session family's read-side posture; `SpentSessionError` stays the taking-methods' (`into_inner`) shape. - **A failed reply#2 read restores the inner** (the phase is *not* consumed on failure): the taken inner is returned to the session on error — the channel stays owned (reply#1 succeeded, the listener existed, reap via `close`/`Drop`, the error-surface contract the existing tests pin), and the session's `split()`/`close()` keep the pipe. The corruption path is the *post-success* re-entry, and that is exactly what the take-guard closes: after success the plane is taken, the spent shape holds. A post-failure re-call re-reads reply#2 against the (already-ended) session — the producer ends the session on every post-reply#1 failure exit, so the read EOFs the same way (the quiet-wait garbage test pins this shape, green unchanged). - **D-1 decision: option (a)** — the session's `await_inbound` is the one normative reply#2 reader. `BindReply1::await_inbound` is **cut** (not private: the take-shape makes the handle's borrowed-plane reader impossible — the plane now owns the stream); `reply1()` is documented as a **view** (`bind_addr` + `raw_mut`, the front-door bridge seam), which is exactly how the four existing call sites use it. `BindReply1` stays as the typed view (its `Send` probe pinned). - **`BindDataPlane` is un-lifetimed** (`BindDataPlane`, not `BindDataPlane<'s>`): it owns the stream, so the `'static` lifetime probe became a plain `assert_send::()`; `into_inner` returns the owned `Socks5Stream` (the test call site gained a `mut` + drop-before-reap, the CONNECT `TakenInner` tests' shape); the `get_socket_mut` accessor is unchanged (borrowed socket access through the owned inner). The `split()` doc notes the take-guard's consequence (post-reply#2 halves ride the plane, not the session). - **N-6 landed** as the private `open_and_negotiate(client, negotiate)` helper: open → `BiStream::from_joined` → the forked typestate's `use_stream` → the negotiation closure → teardown-on-err, one place for the teardown-at-birth invariant. The three constructors are one-liners over it (CONNECT returns the inner; ASSOCIATE/BIND return `(addr, inner)` tuples). Zero behavior change — the full consumer + integration suites stayed green unchanged (the only touched call sites were the two test-side ownership consequences above). - **S-2 landed**: the same doc line on all three `reap` sites (CONNECT, ASSOCIATE, BIND) — the dropped `teardown_channel` `JoinHandle` is deliberate (the producer-side pump finishes via EOF propagation, the graceful shape; `clear_all` aborts instead, the substrate's abrupt path). - **Test round**: the pinning integration test (`double_await_inbound_returns_the_spent_shape_and_bytes_survive` — the second call returns `Ok(None)` and the first plane's exchange completed uncorrupted, a fresh bind round-trips), the `reply1()`-twice view test (`reply1_twice_is_a_view_and_the_reader_yields_one_plane` — repeated views carry the same address, the reader yields exactly one plane, the second reader call is the spent shape), the spent-BIND unit test (`spent_bind_await_inbound_returns_the_spent_shape` — the `None`-inner arm + `reply1`/`split` spent probes), and the `Send` probe updated (`BindSession` + owned `BindDataPlane`). The existing BIND suite stayed green unchanged apart from the one `into_inner` ownership call site (the round-trip, gate-refusal, factory/accept-failure mappings all untouched). ## Summary Closed review 002's C-1 (the one reachable silent-corruption path): the BIND reply#2 phase is take-guarded — `BindSession::await_inbound` takes the session's stream, the returned `BindDataPlane` owns it (the `TakenInner` pairing posture; the session stays the teardown owner), and a second call returns the family's spent shape `Ok(None)` with no re-read of the live data plane possible. D-1 resolved as option (a): the session's `await_inbound` is the one normative reply#2 reader; `BindReply1::await_inbound` is cut and `reply1()` is documented as a view (`bind_addr` + `raw_mut`), matching its actual usage. N-6 collapsed the three session constructors' prologues into one private `open_and_negotiate` helper (teardown-at-birth in one place). S-2 added the EOF-propagation doc line to all three `reap` sites. Docs synced (module docs + the session docs + consumer.md §The BIND client; the frontmatter `last_updated` records the reconciliation). Tests: the double-read pinning test (spent shape + bytes untouched), the `reply1()`-twice view test, the spent-BIND unit test, and the existing suite green (352 total). Verification: `cargo test` + `--all-features` green, clippy `-D warnings` clean, fmt clean, `cargo doc` clean, both wasm checks clean.