alksocks/tasks/implementation/fix-bind-reply2-reentry.md

---
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, and 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 and 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, so 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, holding 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), with 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<BiStream>` while the session stays the teardown owner; that is 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<Option<...>>` 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, so 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, since 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::<BindDataPlane>()`; `into_inner`
  returns the owned `Socks5Stream<BiStream>` (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), saying 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`,
  where 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.