alkdev/alksocks
6
0 Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

review: rfc1928 post-fork full review — 001

Browse Source 
Security + code-smell + coverage review of the targeted fork as owned
code (the extraction review was narrow; this is the full pass).

Findings: 1 major (F-1: reply_success/reply_error write replies with
.write() — a partial-write hazard invisible to clippy's
unused_io_amount under the err_when-chaining idiom, executable-probe
verified; rfc1928.md's write_all rationale is false for these sites),
7 minors (2 security-posture notes, 2 extraction leftovers, 3
coverage gaps), 4 observations. No critical findings.

Verification: 38+32 tests, clippy (all-targets + wasm32) -D warnings,
fmt, doc, wasm check, publish dry-run, MSRV 1.88 native+wasm — all
green. Coverage 92.63% regions / 95.40% lines (cargo-llvm-cov); gaps
attributed per finding. Differential suite's 15 upstream citations
spot-checked against the v1.0.0 reference checkout — all accurate.
This commit is contained in:
glm-5.3-flash committed 2026-09-23 05:46:04 +00:00
1 parent f8c34444fb
commit 4bd25c093e
1 file changed
+653
Download Patch File Download Diff File Expand all files Collapse all files
docs/reviews/001-rfc1928-full-review.md
+653
View File
@@ -0,0 +1,653 @@
---
status: open
last_updated: 2026-09-23
reviewed_artifacts:
- src/rfc1928/mod.rs
- src/rfc1928/server.rs
- src/rfc1928/client.rs
- src/rfc1928/util/mod.rs
- src/rfc1928/util/stream.rs
- src/rfc1928/util/target_addr.rs
- src/rfc1928/LICENSE
- src/lib.rs
- tests/rfc1928-differential.rs
- Cargo.toml
tool: manual source read + cargo test/clippy/fmt/doc/wasm + cargo-llvm-cov + reference-checkout probes
reviewer: post-fork full review (security + code smells + coverage)
---
# Review 001 — `src/rfc1928/` Post-Fork Full Review
## Purpose
First full review of the targeted fork (`src/rfc1928/`) as owned code.
The extraction review (`tasks/implementation/
review-vendor-extraction.md`) was deliberately narrow — extraction
fidelity vs the reference checkout — and its six criteria are now
superseded in part by ADR-013 (the fork posture dissolves the
verbatim-preservation frame). This review asks the three questions
the extraction review did not:
1. **Security**: where RFC 1928 wire data meets allocation, panic,
error-mapping, or trust boundaries — reviewed independently of
upstream (upstream's choices are not this crate's security
posture anymore).
2. **Code smells**: convention-compliance (AGENTS.md) plus
dead/duplicated/misleading surface left over from the extraction
and the fork pass.
3. **Coverage** (`cargo-llvm-cov`): important gaps, not raw numbers —
attributed to specific behaviors and their risk.
The scope is `src/rfc1928/` only (the forked core). The
producer/consumer/adapter layers do not exist yet (the tree is the
core plus the differential suite); the doors and sessions named in
the architecture docs are future work. That boundary is what makes
several findings below "dead code today, load-bearing when the
wrapper lands" — flagged as such rather than as current bugs.
## Methodology
- Full read of all five forked source files + the differential suite
+ the module docs, cross-referenced against ADR-011/013 and
`docs/architecture/rfc1928.md`.
- Line-by-line behavior comparison against the reference checkout
(`/workspace/fast-socks5` v1.0.0) for every security-relevant path:
length-driven allocation, reply-code mapping, error→reply-code
mapping, auth state machine, the `read_exact!` macro's
allocation expression, `copy_bidirectional` semantics.
- Independent verification of RFC 1928 edge behaviors (unassigned
reply codes, domain caps, RSV/FRAG handling, method-preference
order) against RFC text — not against upstream.
- `cargo test` (38 lib + 32 differential + 2 doc), `cargo clippy
--all-targets -- -D warnings` (clean), `cargo fmt --check` (clean),
`cargo doc --no-deps` (0 warnings), `cargo check/clippy --target
wasm32-unknown-unknown -- -D warnings` (clean), `cargo publish
--dry-run --allow-dirty` (packages cleanly, 35 files 279.5 KiB).
- MSRV: `cargo +1.88 check` native + wasm32 — passes (the manifest's
`rust-version = "1.88"` is real; 1.85 was never claimed).
- `cargo llvm-cov` (summary + `--show-missing-lines`) — per-file
attribution below.
- **Executable probes for the clippy-suppression finding** (F-1):
a scratch crate (`/tmp/opencode/io-probe`) reproducing the exact
generic/`err_when` shape; five variants driven to isolate the
mechanism. All probe files deleted after the run.
## Verification Baseline
All commands on the reviewed tree (commit `f8c3444`, clean):
- `cargo test`: **38 lib + 32 integration + 2 doc (ignored) — all
pass**.
- `cargo clippy --all-targets -- -D warnings`: **clean**.
- `cargo clippy --target wasm32-unknown-unknown -- -D warnings`:
**clean**.
- `cargo fmt --check`: **clean**.
- `cargo doc --no-deps`: **0 warnings**.
- `cargo check --target wasm32-unknown-unknown`: **clean** (the
wasm-clean invariant holds — no `net`-shaped feature, tokio on the
wasm-clean subset).
- `cargo publish --dry-run --allow-dirty`: **completes** (35 files,
279.5 KiB).
- `cargo +1.88 check` (native + wasm32): **clean**.
- `cargo llvm-cov`: **92.63% regions, 95.08% functions, 95.40%
lines** (1499 covered / 69 missed).
### Coverage breakdown
| Module | Lines | Functions | Regions |
|---|---:|---:|---:|
| client.rs | 95.44% | 98.31% | 93.81% |
| mod.rs | 97.30% | 95.65% | 93.36% |
| server.rs | **92.75%** | **89.86%** | 91.09% |
| util/stream.rs | 100.00% | 100.00% | 78.57% |
| util/target_addr.rs | 98.47% | 100.00% | 93.37% |
| **TOTAL** | **95.40%** | **95.08%** | **92.63%** |
Overall healthy for a protocol codec. The uncovered lines
(`--show-missing-lines`) decompose into: the client's v6
request-encoding arm (testable, untested), the `AsyncWrite`
impl's `poll_flush`/`poll_shutdown` passthroughs, the
`StandardAuthentication` macro arms (dead public surface today),
`SocksServerError::to_reply_error`'s non-`Bug` fall-through arms,
`accept_password_auth`'s `CheckResult` impls for `Option`/`bool`,
and assorted error arms. Attribution lives with the findings below.
## Summary Statistics
| Severity | Count | IDs |
|----------|------:|-----|
| Critical | 0 | — |
| Major | 1 | F-1 |
| Minor | 7 | S-1, S-2, C-1, C-2, L-1, L-2, L-3 |
| Observation | 4 | N-1, N-2, N-3, N-4 |
No critical findings. The one major is a latent correctness hazard
masked by a clippy-lint gap (F-1 — the same class as the vendor
review's F-1: a hazard invisible to the automated gates). The minors
split: two security-posture notes that are dead-code today (S-1, S-2),
four extraction leftovers that become real decisions when the wrapper
lands (C-1, C-2, L-1, L-2), two small coverage gaps (L-2, L-3), and
the smell findings (F-1's doc face, N-1..N-4).
Findings use the alkty/alktunnels prefixes adapted to a single-file
scope: `F` (found-correctness/security), `S` (security posture),
`C` (code smells/convention), `L` (coverage gaps), `N`
(observations/notes).
---
# Part A — Findings
## F-1 [major] — `reply_success`/`reply_error` write replies with `.write()`, not `.write_all()`: a real partial-write hazard, invisible to clippy because the `err_when` chain suppresses `unused_io_amount` — and rfc1928.md's rationale claim is false for these sites
**Files**: `src/rfc1928/server.rs:516` (`reply_success`),
`src/rfc1928/server.rs:529` (`reply_error`);
`docs/architecture/rfc1928.md:155-158` (the rationale claim)
**Problem**: both reply writers send the 10/22-byte reply with
`tokio::io::AsyncWriteExt::write`, which may write fewer bytes than
offered (a partial write). Upstream has the same calls verbatim
(`src/server.rs:932`, `src/server.rs:948`) — this site rode the
extraction unchanged. The client-side sibling (`request_header`,
upstream `src/client.rs:331`) was correctly converted to `write_all`
at extraction, and rfc1928.md records exactly that: *"`request_header`
writes with `write_all` where upstream wrote with `write` (a partial
write of a request header would corrupt the conversation)"*. But the
record frames it as a `clippy::unused_io_amount` consequence
("byte-identical on a healthy stream — `clippy::unused_io_amount` is
deny-by-default"), and **the server-side `reply_*` sites survived
despite the same lint applying** — so the claim as written is false
for them, and the hazard they carry is live:
1. **The hazard is real on the target substrates.** A reply is
written to a `BiStream`/duplex half in the channels world, where
`poll_write` is a bounded-channel send and *can* accept a prefix
under memory pressure (the channel's bounded per-channel buffer is
exactly the partial-write producer on this substrate — the same
class of behavior tokio documents for `AsyncWrite` generally:
"a call to write represents an attempt to write some bytes"). A
short write of a 10-byte reply desynchronizes the conversation:
the client's `read_request_reply` parses the tail of the reply as
the start of proxied data (or vice versa on a second reply —
reply#2 of BIND), and the two sides' protocol streams separate.
The failure is silent misalignment, not a typed error.
2. **Why the gate misses it (executable probe, scratch crate, five
variants)**: `clippy::unused_io_amount` is deny-by-default and
fires on `.write()` in three control shapes (bare generic
receiver, struct-field-of-generic receiver, concrete receiver —
all confirmed erroring in the probe). It is suppressed by the
`err_when(...)?` chain: `ErrorContext`'s blanket impl is generic
over `T` for `Result<T, io::Error>`, so `.write(..).await
.err_when(...)?` type-infers `T = usize` *without the lint's
type-walk ever binding a concrete `usize`* — the lint's
expression-shape check misses it. The probe's minimal
disambiguation: the same `.write().await.err_when(...)?` under a
concrete receiver type fires when the chain resolves through
`io::Result<usize>`, and stays silent through the generic
blanket-impl path (exactly the typestate's shape); `map(|_| ())`
chains show the same suppression; `write_all` never trips the
lint regardless. So the codebase's standard error-chaining
idiom is a lint-suppressor for exactly this class of bug — worth
knowing beyond these two sites.
3. **The docs currently mis-teach.** rfc1928.md's adjustment list
presents the `write_all` conversion as a lint-driven, client-side
fix and leaves the reader to conclude the `reply_*` sites were
somehow out of the lint's reach. The truth is worse than the doc
says and better than "unknown": the sites are identical hazards
that the lint *would* flag in isolation but doesn't flag here.
A review that leaves the doc claim standing certifies a false
invariant.
**Fix (small)**: `.write(&reply).await` → `.write_all(&reply).await`
at both sites (identical semantics on a healthy stream, the RFC
reply is a single bounded frame, and `write_all` is the project's
established posture — the client site and every other write in the
fork already use it). Update the rfc1928.md bullet to record the
server-side sites as part of the same adjustment (one clause: "and
`reply_success`/`reply_error` likewise — the `unused_io_amount`
linter's blind spot under `err_when`-chaining is not a substitute
for the conversion"). No wire change, no API change.
**Lift**: closes the one live correctness hazard in the forked code
and makes the doc invariant true. Trivial effort (two call sites +
one doc bullet); the differential suite already covers both reply
forms byte-for-byte, so the fix is regression-guarded.
---
## S-1 [minor] — `read_address`'s domain arm allocates from an attacker-controlled byte with no cap before allocation — `vec![0u8; len as usize]` trusts a 255-max that only the *encoder* enforces
**Files**: `src/rfc1928/util/target_addr.rs:222-229` (`read_address`)
**Problem**: the domain arm reads a 1-byte length, then immediately
allocates `vec![0u8; len as usize]`. The cap is real — the wire
length field is one byte, so `len ≤ 255` by construction, and 255
bytes is bounded — **but the code's safety rests on that implicit
fact with no assertion, doc note, or test pinning it**. The parallel
sites in the same file and the client bound *explicitly*: the
client's `request_header` checks `domain.len() > u8::MAX` before
encoding; `to_be_bytes` checks `DomainLenTooLong` before encoding.
The decode side has no corresponding visible bound — a reader (or a
future refactor that widens the length field, e.g. a hypothetical
FRAG-style extension or a misreading of the "1-byte length" rule)
would turn this into an unbounded allocation from wire input.
Today the 1-byte field makes it safe: this is a posture finding, not
a bug. But the crate's own security posture elsewhere treats
wire-driven allocation as a named, guarded boundary (`TargetAddr`
doc: "The domain name will be passed along to the proxy server"; the
differential suite pins the 255 cap on the encode side). The decode
side — the side where the attacker controls the bytes — deserves the
same visible guard.
**Recommendation**: one doc line on `read_address` ("the 1-byte
length field bounds the allocation at 255 — widening the field
requires a cap here") plus a pinning test asserting the 255-byte
domain decodes and a 256-byte claim is impossible by construction
(the suite's `command_domain_255_bytes_is_the_wire_maximum` already
pins 255 on the *server* path — the gap is only that nothing states
the invariant). Alternatively add an explicit
`DomainLenTooLong`-shaped check; zero behavior change. Low priority
— filed so the invariant is written down before the producer half
routes attacker traffic through this codec in production.
## S-2 [minor] — The server's auth-reject path sends `SOCKS5_AUTH_METHOD_NOT_ACCEPTABLE` (0xff) as the *password sub-protocol's* status byte — faithful to upstream, but it is not the sub-protocol's defined rejection byte, and the ambiguity is undocumented
**Files**: `src/rfc1928/server.rs:352-359` (`reject`),
`src/rfc1928/server.rs:344` (`accept`'s success byte)
**Problem**: RFC 1929 (the username/password sub-protocol) defines
the status byte as: `0x00` = success, *any other value* = failure.
The fork (following upstream) writes `[1, 0xff]` on reject — 0xff
being `SOCKS5_AUTH_METHOD_NOT_ACCEPTABLE`, the *method-negotiation*
phase's constant, repurposed here. This is RFC-1929-compatible (any
non-zero works) but conflates two vocabularies, and the module docs
present the auth flow as RFC-facing. The observable consequence is
benign today (interoperable servers/clients accept any non-zero),
but: (a) a strict peer that treats the status byte as a boolean
"0x00 vs anything" is fine, while one that echoes the value back for
logging may mislabel it; (b) the in-tree `client.rs` check
(`is_success != SOCKS5_REPLY_SUCCEEDED`) is written against 0x00 —
the pairing works but only because both sides happen to use the
same arbitrary constant.
**Recommendation**: a doc comment on `reject` naming the choice
("0xff is the convention upstream picked; RFC 1929 requires only
non-0x00") — or switch the constant to a literal `0xff` with a
comment detaching it from the method-negotiation vocabulary. No
wire change (the bytes are upstream-faithful and interoperate);
the finding is the undocumented vocabulary conflation. Trivial.
---
## C-1 [minor] — Dead error vocabulary: `SocksServerError::Bug` and `ConnectError` are constructed nowhere — extraction leftovers with no consumer in the forked region
**Files**: `src/rfc1928/server.rs:72-73` (`Bug`),
`src/rfc1928/server.rs:66-67` (`ConnectError(#[from] ...)`),
`src/rfc1928/util/stream.rs:46-60` (`ConnectError` enum),
`src/rfc1928/util/stream.rs:62-74` (`to_reply_error`)
**Problem**: upstream constructs `SocksServerError::Bug(...)` three
times — all in the cut kernel-net region (`src/server.rs:1066`,
`1174`, `1250`: `DnsResolveHelper`/`run_udp_proxy_custom`'s addr
post-processing). Upstream constructs `ConnectError` exactly once —
in the cut `tcp_connect` dialers (`src/util/stream.rs:89-104`). In
the fork, both are **unconstructable dead code**: grep across `src/`
and `tests/` finds zero construction sites. `SocksError`'s
`ServerError(#[from] SocksServerError)` transport keeps them
reachable in the public API (they compile, they document, they
map — but nothing can ever produce one). The
`to_reply_error` mappings for `ConnectError` are tested
(`util/stream.rs` tests) — testing a mapping for an error no code
can raise.
The extraction review's F-4 accepted a *string* naming a cut
function as a fidelity keep; the fork dissolved that rationale, and
these two are the remaining dead vocabulary from the same category.
`rfc1928.md`'s not-forked inventory says the `tcp_connect*` dialers
stayed in the reference checkout — but `ConnectError` rode along
because the cut table's `util/stream.rs` row ("the `read_exact!`/
`ready!` macros + `ConnectError` stay") predates the fork's
dead-code sweep and was never revisited.
**Fix (small, behavioral-no-op)**: either (a) delete
`SocksServerError::Bug` and the `ConnectError` enum +
`SocksServerError::ConnectError` variant + its `to_reply_error` +
its tests, or (b) keep them with a one-line module-doc note that
they are the hand-port map's companions (the reply-code mapping is
what a future kernel-net port would reuse). Option (a) is cleaner —
`SocksServerError::Bug`'s `"BUG: ..."` display is misleading in a
crate that pins no-panics as a convention — but (b) is defensible if
the kernel-net hand-port is still anticipated. Decide when the
producer layer lands (it will either consume `ConnectError`'s
mappings — the dial-callback shape maps dial errors to reply codes —
or not; that is the deciding consumer). Until then this is dead
weight on the public error surface.
## C-2 [minor] — `StandardAuthentication` (the macro-generated enum + `allow_no_auth`) has zero in-tree callers — the only forked surface nothing consumes
**Files**: `src/rfc1928/server.rs:396-459` (`auth_method_enums!`
expansion + `StandardAuthentication` + `allow_no_auth`), coverage:
`server.rs:417-427` (the macro's match arms) and `445-458`
(`allow_no_auth`) are uncovered
**Problem**: upstream's `StandardAuthentication` is consumed by the
cut `Socks5Socket`/`Config` legacy layer (`src/server.rs:134-144`,
`762-768`). In the fork, `grep StandardAuthentication` finds it only
in its own definition and one differential-suite *comment*. Nothing
constructs `StandardAuthentication::*` — the typestate's
`negotiate_auth` is driven with `&[NoAuthentication]` /
`&[PasswordAuthentication]` everywhere. Its uncovered lines are the
largest contiguous uncovered block in `server.rs` (the macro match
arms + `allow_no_auth`'s two branches — 0% covered).
This is the ADR-013 posture's stated shape ("the `#[allow(dead_code)]`
on `AuthenticationMethod` swept with actual usage") applied
selectively: `AuthenticationMethod` got its sweep, the
macro-generated enum did not — but the macro itself
(`auth_method_enums!`) is documented as a user-facing extension
point (its doc: "If you want to add your own custom methods, you can
generate a similar enum"), and the producer layer's per-door auth
selection (ADR-005's auth-split) plausibly wants it. So this is a
judgment call, not a violation: keep the machinery (it is small,
generic, and the producer's auth seams may drive it), or cut it
until a consumer exists (the crate's "no speculative surface" lean,
matching the client `Config`'s minimal keep).
**Recommendation**: keep, but make the posture explicit — one module
doc line on `StandardAuthentication` ("kept as the negotiation
machinery's combinator — the producer layer's auth-split (ADR-005)
is its anticipated consumer; currently uncalled in-tree") and note
the coverage block as accepted-uncovered. If the producer layer
lands without consuming it, cut it then. Zero code change now.
## L-1 [minor] — The client's v6 request-encoding arm is untested (0% of `request_header`'s V6 branch)
**Files**: `src/rfc1928/client.rs:293-300` (uncovered),
`tests/rfc1928-differential.rs` (no v6 request test)
**Problem**: coverage's single most important gap on the client
side: `request_header`'s `TargetAddr::Ip(SocketAddr::V6)` arm — the
ATYP=4, 22-byte request form — is never exercised. The v6 *reply*
form is tested twice (`new_reply_v6_form`,
`reply_v6_form_is_22_bytes`, `udp_codec_round_trip_ipv6`), and
`read_address`'s v6 decode is tested (`read_address_ipv6_round_trip`)
— but the *request* encoding of a v6 target has no test. A regression
in the v6 arm (e.g. the `packet[4..20]`/`packet[20..22]` slice
bounds, the ATYP byte, the port offset) would pass the entire suite.
The v6 slice arithmetic is exactly the kind of boundary code that
drifts silently.
**Fix (small)**: one differential-suite test: mock server reads a
22-byte request for a v6 target and pins
`[05, 01, 00, 04, <16 octets>, <port>]`. Small effort, closes the
highest-value gap.
## L-2 [minor] — `Socks5Stream`'s `AsyncRead`/`AsyncWrite` passthrough impls are barely covered: only the happy `poll_read`/`poll_write` ride the tests
**Files**: `src/rfc1928/client.rs:357-396` (the impls; `390-395`
— `poll_flush`/`poll_shutdown` — uncovered; `383-388` covered only
via one `flush()` call in `io_impls_pipe_data_through_the_session`)
**Problem**: the impls are 5-line passthroughs, so the risk is
near-zero — but they are the consumer half's data plane (every
`pump_bidi`-shaped consumer rides them), and `poll_shutdown` in
particular is load-bearing for the EOF-sentinel convention (the
channels data plane's half-close rides `poll_shutdown` reaching the
underlying `BiStream`). A test that shuts down a `Socks5Stream` and
asserts the underlying stream sees the shutdown would pin the
half-close propagation the two-pump contract relies on. The
`DuplexStream`-based harness can observe it (shutdown of one half →
EOF for the peer).
**Recommendation**: one test: complete the handshake, call
`session.shutdown().await`, assert the mock reads EOF. Small
effort, pins the consumer half-close shape before the consumer
layer's pumps depend on it.
## L-3 [minor] — `server.rs`'s uncovered block is the macro + auth-selection machinery (C-2's face) plus two error arms worth one test each
**Files**: `src/rfc1928/server.rs:96-97`
(`to_reply_error`'s `AddrError` arm), `155-163`
(`CheckResult` for `Option`/`bool`), `177-179`
(`skip_auth_this_is_not_rfc_compliant`), `568`
(`read_command`'s version-mismatch return — covered via the
differential suite's variant only at a different call shape), `632`
(`wait_on_tcp`'s io-error arm)
**Problem**: mostly dead-surface coverage debt (see C-2). Two arms
deserve a note: `wait_on_tcp`'s `Err(err)` arm (the io-error path of
the UDP-associate control stream — reachable by dropping the duplex
mid-read; one line of test) and `to_reply_error`'s fall-through
(`GeneralFailure` for the `Io`/`FromUtf8`/`EOF` arms — the mapping
the producer's error surface will rely on when mapping session
errors to reply codes; one test asserting the mapping table).
Both are one-test-cheap; the rest is dead weight until C-2's
decision.
---
# Part B — Non-findings (verified correct, recorded to bound the re-review)
- **The F-1-class panic surface is clean.** Post-extraction-review
sweep re-verified: no `unwrap`/`expect`/`panic!`/`unreachable!`/
`todo!` outside `#[cfg(test)]` modules (grep-verified; every hit
is inside a `mod tests` region). The three translated panics
(`from_u8`'s unassigned-code arm, `use_password_auth`'s
`unreachable!`, `reply_error`'s runtime parse) are all present in
their translated forms with pinning tests
(`reply_error_unassigned_codes_map_to_general_failure`, the
typed-error translation note, the compile-time
`V4_UNSPECIFIED_ZERO`).
- **Length-driven allocation is bounded everywhere else.** The
greeting/methods read (`vec![0u8; methods_len as usize]`, 1-byte
cap = 255), the password sub-protocol's username/password reads
(1-byte caps), the domain length byte (S-1) — all bounded by a
1-byte wire field ≤ 255. No u16/u32-driven allocation exists in
the forked region (the client's `request_header` uses a fixed
`MAX_ADDR_LEN + 3` stack buffer — 263 bytes, bounds-checked
slices). The differential suite's `command_domain_255_bytes_is
_the_wire_maximum` pins the boundary.
- **The method-negotiation preference order is genuinely
server-authoritative.** The outer-loop-in-`server_methods`-order
shape matches upstream (`src/server.rs:901-902`) and RFC 1928 §3's
"the server selects one" reading; the differential suite pins both
directions (client-offers-password-first gets no-auth when the
server offers only none; `accept_password_auth` prefers password
over none when both are offered). The 0xff refusal path is pinned.
- **`reply_success` carries the caller's `sock_addr` verbatim** —
the typestate does not substitute its own address (upstream's
`127.0.0.1` substitution lives in the cut `run_tcp_proxy` caller);
the differential suite pins this distinction with the correct
upstream citation. This matters for BIND's reply#1 (the bound
address must be the *listener's*, supplied by the caller).
- **`copy_bidirectional`'s result is discarded intentionally** —
`transfer` maps neither direction's error to a reply because at
the transfer interception point the reply has already been sent;
the errors are observable only as stream termination. Upstream
does the same (`let _ =`). The two-pump contract (AGENTS.md
convention 8) is the *caller's* shape (`pump_bidi`), not
`transfer`'s; `transfer` remains the upstream-faithful
interception-point helper. The differential suite pins the
both-directions-to-completion behavior.
- **The differential suite's 15 upstream file+line citations were
spot-checked against the reference checkout** — all accurate
(`server.rs:903-918` negotiation loop, `server.rs:944-956` error
reply, `server.rs:1380` `new_reply` privacy, `server.rs:1000-1003`
request header read, `server.rs:1087` the 127.0.0.1 substitution,
`server.rs:572-582`/`587-596` password reads, `server.rs:620-627`
reject write, `client.rs:277-285` UDP-associate fallback +
missing-target, `client.rs:310-317`/`331` domain encode + write,
`client.rs:86-93` skip_auth branch, `lib.rs:313-327`/`332-336`/
`342` UDP codec, `util/target_addr.rs:40-48` the reply-code
mapping). The suite pins bytes, not upstream text — it survives
upstream churn, and the citations are provenance, not assertions.
- **The MSRV (`rust-version = "1.88"`) is real**: `cargo +1.88
check` passes native and wasm32. (alktty's N6-class finding —
declared-but-unverified MSRV — does not reproduce here.)
- **Dependency hygiene**: the dependency set is exactly
tokio (wasm-clean subset, `default-features = false`) /
tracing / thiserror — `tracing` is currently referenced by no
source file (see N-2) but is a declared, load-bearing posture for
the fork ("Upstream `log` calls ... are dropped or re-pointed at
the crate's `tracing`", rfc1928.md). `default-features = false`
on tokio keeps the wasm graph minimal; verified by the wasm
checks.
- **The LICENSE notice is the extended form ADR-013 specifies**:
upstream MIT text + copyright preserved, fork-point statement
(v1.0.0, extraction range `57b107b..5b18d4d`, fork at `5b18d4d`),
and the modification note (post-fork changes are the project's
under MIT OR Apache-2.0). The module docs carry the same
provenance. Convention 17's "extend, never replace" holds.
- **Wasm-clean invariant re-verified**: no `tokio::net` anywhere in
`src/`; the only `tokio::io` uses are the portable `AsyncRead`/
`AsyncWrite`/`copy_bidirectional`; `cargo check` +
`clippy -D warnings` both clean on wasm32.
- **`cargo publish --dry-run` completes** — the manifest is
publishable (no alkty-N9-class missing-README issue; there is no
`readme` field and none needed).
---
# Part C — Notes and observations
## N-1 [observation] — rfc1928.md's "write_all adjustment" bullet is half-true (F-1's doc face)
The bullet (`docs/architecture/rfc1928.md:153-158`) records the
client-side `write_all` conversion and attributes it to
`clippy::unused_io_amount` being deny-by-default — which would make
the server-side `.write()` sites impossible. The probe (F-1)
resolves the contradiction: the lint fires on bare `.write()?` but
is suppressed by the `err_when`/`map(|_| ())` chaining idiom these
sites use. The bullet should be updated to record *both* conversions
(client `request_header` — landed; server `reply_success`/
`reply_error` — F-1's fix) and drop the linter attribution in favor
of the real rule (replies are small, single-shot, and must be
all-or-nothing). Recorded as the doc face of F-1; fixing F-1 without
fixing this bullet leaves the invariant false in docs.
## N-2 [observation] — `tracing` is a declared dependency referenced by no source file
`grep -rn tracing src/` is empty; the dependency exists because the
extraction adjustment list says log calls were "dropped or re-pointed
at the crate's `tracing`" — in practice all were *dropped* (the
typestate has no logging surface). Two postures are defensible:
(a) keep the dep as the fork's declared logging seam for the
producer layer's diagnostics (the `try_notify!` secondary-error
site — upstream logged "extra error while reporting an error to the
client" — is the natural first consumer); or (b) drop it from
`[dependencies]` until the first real use. (a) is the likely
outcome and costs nothing; filed so the dependency's purpose is
stated rather than assumed. `Cargo.toml`'s comment policy already
carries the feature-note precedent.
## N-3 [observation] — `CheckResult`'s `bool`/`Option` impls and `ErrorContext`'s `FromUtf8Error` impl are unused in-tree (the accept_password_auth machinery's loose ends)
`accept_password_auth`'s closure contract (`R: CheckResult`) is
designed for `Result`/`Option`/`bool` returns; in-tree every caller
(tests only) returns `bool`. The `Option`/`bool` impls are uncovered
(`server.rs:155-163`). `ErrorContext`'s `FromUtf8Error` impl *is*
used (`read_username_password`'s two `err_when("converting ...")`
sites) — covered. No action now; the same "keep or cut when the
producer lands" decision as C-2 applies (the producer's
username/password mapping may want `Result`-returning checkers with
typed rejections).
## N-4 [observation] — The `Addr` enum's `Domain(String)` variant comment (`// Vec<[u8]> or Box<[u8]> or String ?`) rode the extraction
`src/rfc1928/util/target_addr.rs:207` carries upstream's
deliberating comment. It is exactly the "non-obvious
design-alternative" class AGENTS.md convention 1 neither requires
nor forbids — but it reads as an unresolved question rather than a
pinned decision (the `String` choice is upstream's and is fine:
UTF-8 validation happens at parse). One-line resolution: either pin
it ("`String` — validated UTF-8 at the codec boundary") or drop the
comment. Trivial.
---
# What's Good
- **The wire vocabulary is sound and pinned.** The reply form
(10/22-byte), the zero-address error form, the method-negotiation
preference order, the 255-byte domain cap, the UDP RSV/FRAG
discipline, and the client's request encoding are all
byte-pinned in a suite that names its RFC basis per test. The
92.63% region coverage with no uncovered *wire-format* path
(the gaps are machinery, not codecs) is the right shape for a
protocol core.
- **The panic-surface discipline survived the fork.** All three
extraction-time panic translations are in place with pinning
tests, and the whole-region sweep finds no new panic path —
wire-reachable values map to typed errors everywhere (the F-1 of
the extraction review stays fixed).
- **The fork's two carried changes are real and guarded.**
`new_reply` is published and is the single reply-form encoder
(no re-encoding anywhere); the BIND surface (`TCPBind` flowing
through `read_command`, `reply_success`'s addr parameter) is
present exactly as ADR-007 describes, with the driving correctly
deferred to producer-side code.
- **The typestate genericity is intact** — `T:
AsyncRead + AsyncWrite + Unpin` throughout, no substrate type
anywhere in the core (the wasm checks verify the structural
half; the grep sweep verifies the naming half).
- **The differential suite is the right artifact** — bytes over
text, upstream citations as provenance, six groups that map to
the fork's actual risk surface, duplex-only (wasm-clean crate's
native build). Its citations verified accurate against the v1.0.0
checkout.
---
# Remediation
Volume is small — one trivial code fix plus doc/test riders — so
recommended remediations are included per finding:
| ID | Finding | Recommended fix | Effort | Risk |
|----|---------|----------------|--------|------|
| F-1 | `.write()` reply sites + false doc rationale | `.write_all` at 2 sites + rfc1928.md bullet update | trivial | none |
| S-1 | domain-length allocation invariant undocumented | doc line + pinning test | small | none |
| S-2 | 0xff vocabulary conflation undocumented | doc line on `reject` | trivial | none |
| C-1 | `Bug`/`ConnectError` dead vocabulary | decide keep-with-note vs cut (decide at producer layer) | trivial | none |
| C-2 | `StandardAuthentication` uncalled | note posture now; cut if still uncalled when producer lands | trivial | none |
| L-1 | client v6 request form untested | one differential test | small | none |
| L-2 | `poll_shutdown` passthrough unpinned | one EOF-propagation test | small | none |
| L-3 | `wait_on_tcp` io-error arm uncovered | one test | trivial | none |
| N-1 | rfc1928.md bullet (F-1's face) | rides F-1 | trivial | none |
| N-2 | `tracing` dependency purpose | note or first consumer | trivial | none |
| N-3 | `CheckResult` impls unused | decide at producer layer | none | none |
| N-4 | `Addr::Domain` comment | pin or drop | trivial | none |
Suggested decomposition: (1) the F-1 batch (F-1 + N-1 + S-2 + N-4 —
one commit, code + doc + no behavior change beyond the
partial-write hardening); (2) the coverage batch (L-1 + L-2 + L-3 +
S-1's pinning test — one commit, tests only); (3) the
decide-at-producer-layer items (C-1, C-2, N-2, N-3) — record the
posture, no code now.
Per the established workflow, every finding should be re-verified
in the follow-up session before task decomposition (the F-1
mechanism claim in particular rests on an executable probe that
should be reproduced fresh).
---
# References
- ADR-011 (the extraction mechanics, the cut table), ADR-013 (the
targeted fork — the ownership posture this review assumes),
ADR-007 (BIND), ADR-005 (auth-split), ADR-004 (the dial-callback
shape — C-1's deciding consumer), ADR-009 (feature gates)
- `docs/architecture/rfc1928.md` (the fork spec — N-1's bullet)
- `tasks/implementation/review-vendor-extraction.md` (the prior,
narrower review; its F-1 is re-verified clean here)
- `/workspace/fast-socks5` (v1.0.0 — the behavioral reference; all
citations in this review refer to it)
- The house pattern: alkty `docs/reviews/001-code-review.md` (the
coverage/correctness/smell triad), alktunnels
`docs/reviews/001-implementation-review.md` (the severity legend
and re-validation discipline)
- RFC 1928 (§6 the reply form, §3 method negotiation, §7 the UDP
associate frame), RFC 1929 (the password sub-protocol's status
byte — S-2)