fix(channels): duplicate adopt/open must not destroy the live channel; fuzz targets 3-5
Found by the manager_routing fuzz target (docs/research/fuzzing.md \u00a77.8): - ChannelManager::open_channel / adopt_channel used HashMap::insert(...).is_some() as a collision check \u2014 insert REPLACES the existing entry, so a duplicate adopt/open installed the new state, dropped the live channel's demux_sender (spurious EOF to its readers, subsequently routed chunks lost) and still returned Err(ChannelExists). Fixed with contains_key pre-check; map untouched on collision. Regression tests: adopt_channel_duplicate_id_leaves_ live_channel_intact, open_channel_duplicate_id_leaves_live_channel_ intact. New fuzz targets (\u00a77.4 step 3): - manager_routing: Arbitrary op sequences over ChannelManager; exact counter models (parked/dropped must equal the manager's monotonic counters), parked-bytes bound per \u00a76.2-1, clear_all ledger-vs-map semantics, drainer-byte reconciliation (lossless routing) - envelope_semantic: constructors -> serde -> write_frame/read_frame structural round-trip; event-type constants; call.error parse-back - spec_parse: OpRegisterRequest::from_json -> rebuild -> registry registration (attacker schemas compile at register, CF-003) - fuzz/shared/src/arbitrary_value.rs: bounded Arbitrary for serde_json::Value; 20 spec_parse + 4 manager_routing seeds - corpus replay for the new targets in fuzz/shared tests Verification: cargo test 684 passed (682 + 2 regression); clippy -D warnings clean (main + fuzz/shared); fmt clean (main + fuzz); cargo fuzz build clean; 20 s smoke on all three new targets clean (manager_routing 79k, envelope_semantic 141k, spec_parse 517k runs); crash input replays clean post-fix
This commit is contained in:
1 parent
c211c163fd
commit
a1f257757b
14 files changed
+1133
-5
No files matched your search
@@ -1,7 +1,8 @@
|
||||
# Fuzzing alkcall: Research and Recommendation
|
||||
|
||||
**Status:** step 1 of §7.4 adopted (`fuzz/` workspace, targets 1–2, committed
|
||||
seeds, detached runner; campaigns run — §7.7)
|
||||
seeds, detached runner; campaigns run — §7.7); step 3 targets 3–5 adopted,
|
||||
one real finding fixed — see §7.8
|
||||
**Date:** 2026-09-27
|
||||
**Research inputs:** web survey of the 2025–2026 Rust fuzzing ecosystem, survey
|
||||
of fuzzing practice in comparable Rust protocol crates (rustls, quinn, quiche,
|
||||
@@ -376,7 +377,12 @@ comparisons), and a `-dict` of JSON tokens for the envelope targets.
|
||||
2. Run a local 10–30 min campaign per target **via the detached runner
|
||||
(§7.6 — never in the foreground of an agent session)**; fix anything
|
||||
found; triage §6.2 candidates with targeted corpus entries.
|
||||
**Done for targets 1–2 — see §7.7.**
|
||||
3. Add targets 3–5, the smoke CI job, and `.gitignore` entries.
|
||||
**Targets 3–5 done — see §7.8. CI deferred (no CI platform exists in
|
||||
this repo yet; the stable-side corpus replay `cargo test
|
||||
--manifest-path fuzz/shared/Cargo.toml` is the drop-in smoke gate
|
||||
when a platform is chosen).**
|
||||
4. Scheduled campaign tier; then OSS-Fuzz application.
|
||||
|
||||
### 7.5 Relationship to existing tests
|
||||
@@ -623,6 +629,85 @@ inputs worth curating. §6.2-3 (pre-payload allocation window) remains
|
||||
bounded as designed. §6.2 items 1/2/4/5 are stateful targets
|
||||
(§7.4 step 3) and were not exercised by these parser-level campaigns.
|
||||
|
||||
---
|
||||
|
||||
## 7.8 Step 3: targets 3–5 (2026-09-27)
|
||||
|
||||
**Adopted.** Three more targets, all over already-`pub` APIs (the
|
||||
`#[cfg(fuzzing)]` exposure never materialized — quinn/rustls-style
|
||||
exposure has not been needed for any of the five targets):
|
||||
|
||||
| Target | Drives | Invariants |
|
||||
|---|---|---|
|
||||
| `manager_routing` | `ChannelManager` under `#[derive(Arbitrary)]` op sequences (`Route`/`Open`/`Adopt`/`Teardown`/`ClearAll`), 256 ops × 4 KiB payloads, live mux runner, drainer tasks | no-panic; **exact counter models**: harness parked/dropped counters must equal the manager's monotonic `early_arrival_count`/`dropped_unknown_chunks` after every op; parked bytes ≤ distinct-ids × 64 × 16 MiB (the documented per-channel bound, §6.2-1); post-sequence `clear_all` reconciles every routed byte against the drainers' totals (lossless bounded-buffer routing, sentinel semantics included) |
|
||||
| `envelope_semantic` | `#[derive(Arbitrary)]` `EnvelopeKind` → the six `EventEnvelope` constructors → serde round-trip → `write_frame`/`read_frame` | no-panic; constructors emit exactly the wire-stable event-type constants; any `Value` payload survives a serde round-trip; `call.error` payloads always parse back as `CallError` (ADR-016 closed schema); structural `write_frame`/`read_frame` round-trip |
|
||||
| `spec_parse` | raw bytes → `serde_json` → `OpRegisterRequest::from_json` (the `pub` wrapper over `rebuild_spec_for`) → `OperationRegistry::register` (attacker-shaped schemas **compile at registration**, CF-003) | no-panic; parse failures are clean `INVALID_INPUT`; `spec_to_json_pub` → `from_json` round-trips the rebuilt spec; registration is always `Result` (uncompilable schema = compile error, never silent); registry bookkeeping survives repeated attacker-shaped inserts |
|
||||
|
||||
New supporting pieces: `fuzz/shared/src/arbitrary_value.rs` (a bounded
|
||||
`Arbitrary` impl for `serde_json::Value` — depth 6, width 6, sized
|
||||
strings/keys), and 20 `spec_parse` + 4 `manager_routing` committed
|
||||
seeds (the op-sequence corpus is tiny because structured `Arbitrary`
|
||||
inputs self-generate; the four committed seeds are semantic fixtures
|
||||
including the duplicate-adopt sequence that pinned the bug below).
|
||||
|
||||
### First real finding: duplicate adopt/open destroyed the live channel
|
||||
|
||||
The `manager_routing` campaign found it in its first minutes — the
|
||||
§6.2 stateful thesis, confirmed: **the bug is invisible to
|
||||
example-based tests** because it needs an *interleaving* (adopt → drop
|
||||
write half → duplicate adopt → route) that no unit test thinks to
|
||||
replay.
|
||||
|
||||
**Mechanism** (`src/channels/manager.rs`, `open_channel` and
|
||||
`adopt_channel`, pre-fix):
|
||||
|
||||
```rust
|
||||
if channels.insert(channel_id, state).is_some() {
|
||||
return Err(ManagerError::ChannelExists(channel_id));
|
||||
}
|
||||
```
|
||||
|
||||
`HashMap::insert` **replaces** an occupied entry and returns the old
|
||||
value — so the collision-detection idiom silently *installed the new
|
||||
state and dropped the old `ChannelState`*, whose `demux_sender` was
|
||||
the live channel's read half. A duplicate `adopt_channel`/`open_channel`
|
||||
on an in-use id returned `Err(ChannelExists)` (correct) **while
|
||||
destroying the live channel** (incorrect): readers saw a spurious EOF,
|
||||
subsequently routed chunks were lost, and the mux write half kept
|
||||
framing onto the transport for a channel the demux no longer fed.
|
||||
|
||||
Reproducibility: a failed adopt on a live channel whose mux pump had
|
||||
finished (the write half dropped — as any caller does) re-registered a
|
||||
pump, succeeded through `mux.register`, and hit the replacing insert.
|
||||
The duplicate-adopt path is reachable whenever an open-op response is
|
||||
replayed or a connection-owner race re-announces an id.
|
||||
|
||||
**Fix** (this commit): check `channels.contains_key(&channel_id)` and
|
||||
return before any insert — the map is never touched on collision in
|
||||
either `open_channel` or `adopt_channel`. Regression tests:
|
||||
`adopt_channel_duplicate_id_leaves_live_channel_intact`,
|
||||
`open_channel_duplicate_id_leaves_live_channel_intact` (both verify
|
||||
routing survives a rejected duplicate; the first also verifies the
|
||||
EOF sentinel remains the only EOF source).
|
||||
|
||||
**Also fixed in the harness** (fuzzer-vs-harness findings, not crate
|
||||
bugs): `clear_all` returns the opener-ledger entries (ADR-047 §7),
|
||||
not channel-map entries — adopted channels carry no ledger entry, so
|
||||
`drained.len() == open_count()` is *not* an invariant; the monotonic
|
||||
counters survive `clear_all`; and drainer-byte reconciliation needs
|
||||
per-id supersession when a fresh stream replaces a drained one.
|
||||
|
||||
### Campaign results (smoke tier, 20 s per target)
|
||||
|
||||
All three targets ran clean (`-fork=1 -rss_limit_mb=2048
|
||||
-malloc_limit_mb=2048`): `manager_routing` 79k runs, `envelope_semantic`
|
||||
141k runs, `spec_parse` 517k runs — zero crashes beyond the found-and-
|
||||
fixed channel bug above. The stable side stayed green throughout:
|
||||
684 tests (682 + the two regression tests), clippy `-D warnings`, fmt
|
||||
(main + fuzz workspace + shared). Detached 10-min campaigns on the new
|
||||
targets follow this commit; §6.2-1 (parked-bytes bound) is now encoded
|
||||
as an exact counter model and the §7.4 step-3 stateful coverage exists.
|
||||
|
||||
## 8. Answering the "if / how" directly
|
||||
|
||||
- **If?** Yes — justified by position in the dependency graph, two stable
|
||||
|
||||
Reference in new issue
Block a user