consumer: the BIND reply#2 re-entry guard + session hygiene (review-002 C-1, D-1, N-6, S-2)
- C-1: BindSession::await_inbound is take-guarded — the first read takes the session's stream; the returned BindDataPlane owns it (the TakenInner pairing posture; the session stays the teardown owner), so a second call returns the family's spent shape Ok(None) and can never re-read the live data plane. A failed read restores the inner (the phase is only consumed on success; the producer ends the session on every post-reply#1 failure exit, so a re-call EOFs — never a live data-plane re-read either way). Pinned by the double-await_inbound integration test (the spent shape + the data-plane bytes asserted untouched) and the spent-BIND unit test. - D-1: one normative reply#2 reader (option a) — the session's await_inbound; BindReply1::await_inbound is cut and reply1() is documented as a view (bind_addr + raw_mut), matching its actual usage; the reply1()-twice re-entry path is closed (pinned by the view test). - N-6: the three session constructors' prologues collapsed into one private open_and_negotiate helper (teardown-at-birth in one place); zero behavior change. - S-2: the EOF-propagation doc line on all three reap sites (the dropped teardown_channel handle is deliberate — the graceful shape; clear_all aborts instead). - BindDataPlane is un-lifetimed (owns the stream); into_inner returns the owned Socks5Stream<BiStream>; docs synced (module + session docs, consumer.md §The BIND client). Verification: cargo test 352 green (all-features too), clippy -D warnings clean, fmt clean, cargo doc clean, both wasm checks clean.
This commit is contained in:
1 parent
6d14d35a54
commit
53739b20cb
4 files changed
+459
-139
No files matched your search
@@ -1,7 +1,7 @@
|
||||
---
|
||||
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: pending
|
||||
status: completed
|
||||
depends_on: []
|
||||
scope: narrow
|
||||
risk: medium
|
||||
@@ -104,8 +104,94 @@ accept-failure mappings unaffected).
|
||||
|
||||
## Notes
|
||||
|
||||
> Agent fills this during implementation.
|
||||
> 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 — 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 — 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::<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) — 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
|
||||
|
||||
> Agent fills this on completion.
|
||||
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.
|
||||
Reference in new issue
Block a user