From 6957c7251c5e631253d6acd3ed8e77ea781a4a73 Mon Sep 17 00:00:00 2001 From: Rob Ekl Date: Wed, 2 Sep 2026 11:07:06 -0500 Subject: [PATCH] scsi: route raw_command through checked_cdb_len; add the fd hand-off release edge (#16) raw_command clamped CDBs with cdb.len().min(16) instead of routing through the shared checked_cdb_len guard, so an over-length or empty CDB became a different command rather than a rejected one; it now returns the crate Result so a rejected CDB is distinguishable from an I/O failure. The recovery/Drop fd hand-off lacked a release edge: the teardown swap was not a release op and the recovery CAS was not an acquire load, so a recovery thread publishing its fd after Drop drained the slot could miss dead==true and leak the descriptor. Both sides are now AcqRel, with the execute() drain left at Acquire (sound via the release-sequence rule). A loom model (cfg(loom) dev-dep, its own CI job) provides the regression coverage cargo test cannot. Module docs for fd_handoff and timeline now resolve in their own scope, and item docs stay within comment-guard's caps. Fixes #16. --- .github/workflows/ci.yml | 43 ++++ CHANGELOG.md | 9 + Cargo.lock | 182 ++++++++++++++ Cargo.toml | 25 ++ Makefile | 10 +- src/scsi/fd_handoff.rs | 510 +++++++++++++++++++++++++++++++++++++++ src/scsi/linux.rs | 219 ++++++++++++----- src/scsi/mod.rs | 6 + 8 files changed, 946 insertions(+), 58 deletions(-) create mode 100644 src/scsi/fd_handoff.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a32310eb..c6e35f8f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -136,6 +136,49 @@ jobs: - run: cargo test --tests working-directory: libfreemkv + # The memory orderings in `scsi::fd_handoff` — the fd hand-off between a + # recovery thread and `SgIoTransport::drop` — cannot be checked by `cargo + # test`: they compile to the same instructions on x86_64, so the WRONG ones + # pass every run. loom enumerates the executions the C++20 model permits and + # reports the leaked descriptor directly, which makes it the only regression + # coverage that part of the module has. If this job stops running, that + # coverage disappears silently behind a green build. + # + # Its own job rather than a step in `test` because `RUSTFLAGS` is part of the + # rust-cache key: folding it into `test` costs that job a full cache miss on + # every run. It takes the same sibling checkout as the jobs above even though + # `--no-default-features` drops freemkv-unlock from the BUILD — `--features` + # does not exempt an optional path dependency from manifest RESOLUTION, so + # without the sibling this job would fail the moment the branch tip carries + # the path-dep form of that dependency rather than a git tag. + loom: + needs: resolve-msrv + runs-on: ubuntu-latest + env: + RUSTFLAGS: --cfg loom + steps: + - uses: actions/checkout@v7 + with: + path: libfreemkv + - uses: actions/checkout@v7 + with: + repository: freemkv/freemkv-unlock + ref: "${{ github.ref_name == 'qa' && 'qa' || 'dev' }}" + path: freemkv-unlock + - uses: dtolnay/rust-toolchain@master + with: + toolchain: ${{ needs.resolve-msrv.outputs.v }} + components: clippy + - uses: Swatinem/rust-cache@v2 + with: + workspaces: libfreemkv + - run: cargo test --lib --no-default-features --features scsi fd_handoff + working-directory: libfreemkv + # `lint` runs clippy without `--cfg loom`, so this is the only place the + # model code is linted at all. + - run: cargo clippy --lib --no-default-features --features scsi --profile test -- -D warnings + working-directory: libfreemkv + deny: runs-on: ubuntu-latest steps: diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c507043..ce5fc79c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,15 @@ ## [1.6.15] — unreleased +### Fixed + +- Linux: `SgIoTransport::raw_command` clamped an over-length CDB with `cdb.len().min(16)` instead of routing it through the shared `checked_cdb_len` guard that `execute()` uses. Under SPC-4 a CDB's length is fixed by its opcode, so truncating one does not shorten the command — it sends a descriptor the drive will read as something else. An empty CDB was likewise unguarded, and went to the sg driver as a zero-length command descriptor. Unreachable — `raw_command` is private and its one caller passes a fixed 6-byte ALLOW MEDIUM REMOVAL — but it was the one transport path that did the thing the guard exists to prevent. +- Linux: the fd hand-off between a background recovery thread and `SgIoTransport::drop` had no release edge, so a recovery thread that published its fd after teardown had drained the slot was not guaranteed to observe `dead == true` and could leave the descriptor unclosed. `Drop`'s claiming `swap` was `Acquire` (a relaxed store, heading no release sequence) and the recovery thread's `compare_exchange` was `Release` (a relaxed load, acquiring nothing) — neither side alone was enough, and upgrading only one still leaks. Both are now `AcqRel`. Worst case was a leaked file descriptor on a transport already being torn down. + +### Changed + +- The Linux recovery-thread fd hand-off moved to `scsi::fd_handoff`, where it is compiled and unit-tested on every platform (`linux.rs` is built on one host only) and model-checked under `loom`. No public API change. + ### Security - Fixed a soundness bug (GHSA-j8ww-f5fg-9pmh, low severity): `PrefetchedSectorSource::into_channels` handed out a `Sender>` recycle channel whose buffers the producer re-exposed with `unsafe { Vec::set_len }` guarded only by capacity, so a downstream caller recycling a `Vec::with_capacity(n)` (len 0) could drive `set_len` over uninitialized memory — undefined behaviour reachable from safe code. The producer now uses `Vec::resize`, removing the `unsafe`; buffer pooling (the real cross-thread alloc/free win) is unchanged. No in-tree caller triggered it. diff --git a/Cargo.lock b/Cargo.lock index 70305130..22aceba1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -13,6 +13,15 @@ dependencies = [ "cpufeatures 0.3.1", ] +[[package]] +name = "aho-corasick" +version = "1.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c982642fa9e8606056828ee9a8505737230110bb1099153c79efe865c59d12ba" +dependencies = [ + "memchr", +] + [[package]] name = "autocfg" version = "1.5.1" @@ -55,6 +64,16 @@ version = "3.20.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "72f5acc6cb2ba439de613abc23857ec3d78374d8ed5ac84e9d11336e87da8649" +[[package]] +name = "cc" +version = "1.4.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0ad534f4357a5264cce5019c989cf66a4f0dc4e0d1b1d15f8aacec0ff7360273" +dependencies = [ + "find-msvc-tools", + "shlex", +] + [[package]] name = "cfg-if" version = "1.0.4" @@ -223,6 +242,12 @@ version = "2.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "da7c62ceae207dd37ea5b845da6a0696c799f85e97da1ab5b7910be3c1c80223" +[[package]] +name = "find-msvc-tools" +version = "0.1.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d45db016d36b838f563236e9193d0ee6ce38f3f68b6c94e914b4929c96bbb890" + [[package]] name = "flate2" version = "1.1.10" @@ -248,6 +273,21 @@ dependencies = [ "tracing", ] +[[package]] +name = "generator" +version = "0.8.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b3b854b0e584ead1a33f18b2fcad7cf7be18b3875c78816b753639aa501513ae" +dependencies = [ + "cc", + "cfg-if", + "libc", + "log", + "rustversion", + "windows-link", + "windows-result", +] + [[package]] name = "generic-array" version = "0.14.7" @@ -310,6 +350,12 @@ version = "1.0.18" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8f42a60cbdf9a97f5d2305f08a87dc4e09308d1276d28c869c684d7777685682" +[[package]] +name = "lazy_static" +version = "1.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bbd2bcb4c963f2ddae06a2efc7e9f3591312473c50c6685e1f298068316e66fe" + [[package]] name = "libc" version = "0.2.189" @@ -325,6 +371,7 @@ dependencies = [ "crossbeam-channel", "freemkv-unlock", "libc", + "loom", "memchr", "rand", "rayon", @@ -349,12 +396,43 @@ version = "0.4.34" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f9f8bd3e56ce4dfc153cf470fffbfa98c7620958b312ca5c3a4b8d5181fd13c6" +[[package]] +name = "loom" +version = "0.7.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "419e0dc8046cb947daa77eb95ae174acfbddb7673b4151f56d1eed8e93fbfaca" +dependencies = [ + "cfg-if", + "generator", + "scoped-tls", + "tracing", + "tracing-subscriber", +] + +[[package]] +name = "matchers" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d1525a2a28c7f4fa0fc98bb91ae755d1e2d1505079e05539e35bc876b5d65ae9" +dependencies = [ + "regex-automata", +] + [[package]] name = "memchr" version = "2.8.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cf8baf1c55e62ffcace7a9f06f4bd9cd3f0c4beb022d3b367256b91b87513d98" +[[package]] +name = "nu-ansi-term" +version = "0.50.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5" +dependencies = [ + "windows-sys", +] + [[package]] name = "num-bigint" version = "0.5.1" @@ -456,6 +534,23 @@ dependencies = [ "crossbeam-utils", ] +[[package]] +name = "regex-automata" +version = "0.4.18" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ad8553b9b26413251cbf30e620595c7a41b3887f03da04579c0e6b0d6a06b4b2" +dependencies = [ + "aho-corasick", + "memchr", + "regex-syntax", +] + +[[package]] +name = "regex-syntax" +version = "0.8.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d6f6ff9a378485b298a5286656da665ba74413d36db0979633275d2e708145d4" + [[package]] name = "roxmltree" version = "0.20.0" @@ -475,6 +570,18 @@ dependencies = [ "windows-sys", ] +[[package]] +name = "rustversion" +version = "1.0.23" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cf54715a573b99ac80df0bc206da022bcd442c974952c7b9720069370852e21f" + +[[package]] +name = "scoped-tls" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e1cf6437eb19a8f4a6cc0f7dca544973b0b78843adbfeb3683d1a94a0024a294" + [[package]] name = "serde" version = "1.0.229" @@ -540,12 +647,33 @@ dependencies = [ "digest 0.11.3", ] +[[package]] +name = "sharded-slab" +version = "0.1.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f40ca3c46823713e0d4209592e8d6e826aa57e928f09752619fc696c499637f6" +dependencies = [ + "lazy_static", +] + +[[package]] +name = "shlex" +version = "2.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f8fadd59c855ef2080decdef8ff161eb6661b86933c9d82e5ba29dc602a55aba" + [[package]] name = "simd-adler32" version = "0.3.10" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3a219298ac11a56ea9a6d2120044824d6f01aeb034955e7af7bc16858527deea" +[[package]] +name = "smallvec" +version = "1.16.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b9be42f50aa861c555654aa3a37f52f4b1074bacf4e48fe0ef7fa584e80f1f0f" + [[package]] name = "syn" version = "2.0.119" @@ -581,6 +709,15 @@ dependencies = [ "windows-sys", ] +[[package]] +name = "thread_local" +version = "1.1.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1ad99c4c6d32803332c548b1af0540b357b3f5fc0be8f6c6bfe8b2e6ae784070" +dependencies = [ + "cfg-if", +] + [[package]] name = "tracing" version = "0.1.44" @@ -610,6 +747,36 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "db97caf9d906fbde555dd62fa95ddba9eecfd14cb388e4f491a66d74cd5fb79a" dependencies = [ "once_cell", + "valuable", +] + +[[package]] +name = "tracing-log" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ee855f1f400bd0e5c02d150ae5de3840039a3f54b025156404e34c23c03f47c3" +dependencies = [ + "log", + "once_cell", + "tracing-core", +] + +[[package]] +name = "tracing-subscriber" +version = "0.3.23" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cb7f578e5945fb242538965c2d0b04418d38ec25c79d160cd279bf0731c8d319" +dependencies = [ + "matchers", + "nu-ansi-term", + "once_cell", + "regex-automata", + "sharded-slab", + "smallvec", + "thread_local", + "tracing", + "tracing-core", + "tracing-log", ] [[package]] @@ -630,6 +797,12 @@ version = "1.0.24" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e6e4313cd5fcd3dad5cafa179702e2b244f760991f45397d14d4ebf38247da75" +[[package]] +name = "valuable" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ba73ea9cf16a25df0c8caa16c51acb937d5712a8429db78a3ee29d5dcacd3a65" + [[package]] name = "version_check" version = "0.9.5" @@ -642,6 +815,15 @@ version = "0.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f0805222e57f7521d6a62e36fa9163bc891acd422f971defe97d64e70d0a4fe5" +[[package]] +name = "windows-result" +version = "0.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7781fa89eaf60850ac3d2da7af8e5242a5ea78d1a11c49bf2910bb5a73853eb5" +dependencies = [ + "windows-link", +] + [[package]] name = "windows-sys" version = "0.61.2" diff --git a/Cargo.toml b/Cargo.toml index 28780e6a..78feb441 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -101,10 +101,35 @@ libc = "0.2" [dev-dependencies] tempfile = "3" +# Permutation model checker for the `scsi::fd_handoff` atomics — the fd +# hand-off between a Linux recovery thread and `SgIoTransport::drop`. A normal +# test cannot see a missing release edge there: the orderings compile to the +# same instructions on x86_64, so the wrong ones pass every time. loom +# enumerates the executions the C++20 model permits and reports the leaked +# descriptor directly. +# +# Behind `cfg(loom)`, which nothing sets unless you ask for it, so this is out +# of the graph for every ordinary build and for every CI job but the dedicated +# `loom` one. Run it with: +# +# RUSTFLAGS="--cfg loom" cargo test --lib --no-default-features \ +# --features scsi fd_handoff +# +# `cargo test --lib` specifically: as a DEV-dependency loom is only in scope for +# test targets, which is why `fd_handoff` gates its import on `all(loom, test)` +# and falls back to std elsewhere. Without that, `--cfg loom` would break plain +# `cargo build`/`clippy` with an unresolved-crate error. +[target.'cfg(loom)'.dev-dependencies] +loom = "0.7" + [[bench]] name = "sgio_read" harness = false +[lints.rust] +# `loom` is set by RUSTFLAGS for the model checker above, never by a feature. +unexpected_cfgs = { level = "warn", check-cfg = ['cfg(loom)'] } + [lints.clippy] uninlined_format_args = "allow" diff --git a/Makefile b/Makefile index 52c5a1ee..17edbdd5 100644 --- a/Makefile +++ b/Makefile @@ -1,7 +1,7 @@ # libfreemkv — local dev helper. # Mirrors the workspace-wide CI checks but scoped to this single crate. -.PHONY: test build check ci clean +.PHONY: test build check loom ci clean test: cargo test --tests @@ -13,7 +13,13 @@ check: cargo fmt --check cargo clippy --all-targets -- -D warnings -ci: check build test +# Model-check the scsi::fd_handoff atomics. `cargo test` cannot fail on those +# orderings — they compile to the same instructions on x86_64 — so this is the +# only check on them. Mirrors the `loom` CI job. +loom: + RUSTFLAGS="--cfg loom" cargo test --lib --no-default-features --features scsi fd_handoff + +ci: check build test loom clean: cargo clean diff --git a/src/scsi/fd_handoff.rs b/src/scsi/fd_handoff.rs new file mode 100644 index 00000000..90b4035b --- /dev/null +++ b/src/scsi/fd_handoff.rs @@ -0,0 +1,510 @@ +//! The fd hand-off between a Linux recovery thread and the transport that +//! spawned it. +//! +//! When `SgIoTransport::execute` sees a transport-level failure it abandons +//! its fd and spawns a detached thread to open a fresh one. That thread has no +//! reference back to the transport — the transport may have been dropped +//! before the `open()` returns — so the two sides rendezvous through a +//! single-slot mailbox: an `AtomicI32` holding an owned fd (`-1` = empty) and +//! an `AtomicBool` saying whether the transport is already gone. +//! +//! The invariant, and the only thing that matters here: **every fd that +//! reaches the slot is closed exactly once.** Closing zero times leaks a +//! descriptor; closing twice is worse than a leak, because a `close()` of an +//! fd number the process has since reused closes an unrelated file. +//! +//! # Why this lives in `scsi/`, not in `linux.rs` +//! +//! `linux.rs` is compiled on exactly one host, so nothing in it can be tested +//! on macOS or Windows CI — the same reason [`super::checked_cdb_len`] lives +//! in the parent module. The protocol below is plain atomics with no syscalls +//! in it, so it is compiled and tested on every platform even though Linux is +//! its only caller. +//! +//! # Memory ordering +//! +//! The subtle case is a `Drop` that races the recovery thread. The two sides +//! are ordered only through the slot, so the release edge has to be carried by +//! the slot's own operations: +//! +//! 1. [`claim_for_teardown`] publishes `dead = true` and then claims the slot. +//! 2. [`publish_recovered_fd`] fills the slot and then reads `dead`. +//! +//! Publish-first needs nothing special: teardown's swap is an RMW, so it reads +//! the last value in the slot's modification order — the fd — and closes it by +//! coherence alone. The dangerous order is teardown-first, where +//! `claim_for_teardown` finds the slot empty and returns, leaving the recovery +//! thread as the only one that can close the fd — and it will only do so if its +//! `dead` load observes the `true`. +//! +//! That observation is not free. Rust's atomics follow the C++20 rules +//! (`std::sync::atomic` module docs: "Rust atomics currently follow the same +//! rules as C++20 atomics, specifically the rules from the `intro.races` +//! section, without the 'consume' memory ordering"), and under those rules: +//! +//! - Every operation on the slot is a read-modify-write — `swap` or +//! `compare_exchange`, never a plain store after construction. That matters, +//! because "a release sequence headed by a release operation A on an atomic +//! object M is a maximal contiguous sub-sequence of side effects in the +//! modification order of M, where the first operation is A, and every +//! subsequent operation is an atomic read-modify-write operation" +//! ([intro.races], N4868). So once `claim_for_teardown`'s `swap` executes, +//! *every* later slot operation is in its release sequence — including one +//! that lands behind an intervening drain or a second recovery thread, +//! which is why the argument does not depend on the CAS reading the `swap`'s +//! value directly. +//! - Landing in that sequence is still not enough to synchronize with its +//! head. That needs both halves: "an atomic operation A that performs a +//! release operation on an atomic object M synchronizes with an atomic +//! operation B that performs an acquire operation on M and takes its value +//! from any side effect in the release sequence headed by A" +//! ([atomics.order]/2, N4868). +//! +//! Both halves of that were missing: +//! +//! - `swap(-1, Acquire)` is not a release store. Per the [`Ordering::Acquire`] +//! docs, "using this ordering for an operation that combines loads and +//! stores leads to a `Relaxed` store operation" — so it heads no release +//! sequence, and there was nothing for the CAS to synchronize with. +//! - `compare_exchange(.., Release, Relaxed)` is not an acquire load. Per the +//! [`Ordering::Release`] docs, it "leads to a `Relaxed` load operation" — so +//! even against a proper release store it would establish no edge. +//! +//! Upgrading only one side therefore fixes nothing; the loom models in this +//! file still report the leak with either half applied alone. Both are +//! `AcqRel` below. +//! +//! Concretely, the CAS in [`publish_recovered_fd`] carries both roles at once: +//! its release half publishes the `open()` to [`take_recovered_fd`], and its +//! acquire half is what lets the `dead` load that follows observe a teardown +//! that has already claimed the slot. +//! +//! [`take_recovered_fd`] is deliberately left at `Acquire`, which makes its +//! store half relaxed. That is harmless precisely because of the release- +//! sequence rule above: an RMW extends the sequence whatever its own ordering, +//! so a drain landing between `claim_for_teardown` and a later CAS does not +//! break the edge. It needs no release half of its own — it is called from +//! `execute()`, which cannot run concurrently with `Drop` (both need the +//! transport), so it never has anything of its own to publish to a teardown. +//! `loom_drain_does_not_break_the_release_sequence` is the check on that. +//! +//! `dead`'s `Release` store is belt-and-braces: it is sequenced before the +//! `AcqRel` swap, and the swap is what carries the edge. Downgrading it to +//! `Relaxed` leaves all three models passing. +//! +//! # Building the model +//! +//! ```text +//! RUSTFLAGS="--cfg loom" cargo test --lib --no-default-features \ +//! --features scsi fd_handoff +//! ``` +//! +//! Under those flags the protocol runs on loom's instrumented atomics, so the +//! models check the code the transport actually calls rather than a +//! transcription that could drift from it. The alias below is gated on `test` +//! as well as `loom`, because `loom` is a DEV-dependency and so is not in the +//! graph of a non-test target: `cfg(loom)` alone would break `cargo build`, +//! `cargo check` and `cargo clippy --all-targets` under those flags with an +//! unresolved-crate error. Non-test targets keep std's atomics. +//! +//! One consequence to know about: under `all(loom, test)`, `SgIoTransport`'s +//! atomics are loom's, and those panic outside a `loom::model`. No unit test +//! constructs a transport today — `raw_command` is a free-standing `fn` that +//! touches none — but the first one that does must live behind +//! `cfg(not(loom))`. + +// See "Building the model" above for why this is gated on `test` too. +#[cfg(all(loom, test))] +pub(crate) use loom::sync::atomic::{AtomicBool, AtomicI32, Ordering}; +#[cfg(not(all(loom, test)))] +pub(crate) use std::sync::atomic::{AtomicBool, AtomicI32, Ordering}; + +/// The empty slot. Not a valid fd; `open()` never returns a negative value. +pub(crate) const EMPTY: i32 = -1; + +/// Interpret a value taken out of the slot. Only a non-negative value is an fd +/// the caller now owns; `EMPTY` — or any other negative, which nothing can put +/// there — is nothing to close. Deliberately `>= 0` rather than `!= EMPTY`, to +/// keep the guard the call sites used before this protocol was factored out. +fn taken(value: i32) -> Option { + (value >= 0).then_some(value) +} + +/// Take whatever fd a recovery thread has published, transferring ownership to +/// the caller. `None` if no recovery has completed. +/// +/// Called from `execute()` at the top of a command, which is why it does not +/// consult `dead`: a live `execute()` means the transport is not being torn +/// down. `Acquire` pairs with the `AcqRel` CAS in [`publish_recovered_fd`] so +/// the `open()` that produced the fd happens-before the caller uses it. +pub(crate) fn take_recovered_fd(slot: &AtomicI32) -> Option { + taken(slot.swap(EMPTY, Ordering::Acquire)) +} + +/// Publish a freshly opened fd from a recovery thread. Returns the fd the +/// **caller** must now close, or `None` if it was handed off: +/// +/// - The slot was already full — another recovery thread won, so we close +/// ours rather than overwrite (and leak) the winner's. +/// - The transport was torn down mid-`open()`; nothing will drain the slot, +/// so we re-claim through it rather than close `new_fd` directly, which +/// would double-close against a teardown swapping at the same moment. +/// +/// Both orderings below are `AcqRel`; the module docs say why. +pub(crate) fn publish_recovered_fd( + slot: &AtomicI32, + dead: &AtomicBool, + new_fd: i32, +) -> Option { + // A negative `new_fd` would go in as the `EMPTY` sentinel, or as a value no + // drain will hand back — silently lost either way. The caller checks + // `open()` before getting here; this pins that as the contract. + debug_assert!(new_fd >= 0, "only a real fd may enter the slot"); + if slot + .compare_exchange(EMPTY, new_fd, Ordering::AcqRel, Ordering::Relaxed) + .is_err() + { + return Some(new_fd); + } + if dead.load(Ordering::Acquire) { + return taken(slot.swap(EMPTY, Ordering::AcqRel)); + } + None +} + +/// Mark the transport dead and claim any fd still sitting in the slot, +/// transferring ownership to the caller. Called from `Drop`. +/// +/// The `dead` store must be ordered before the claim: a recovery thread that +/// fills the slot after we have drained it has to see `dead == true`, or its +/// fd is never closed by anyone. `AcqRel` on the swap is what carries that +/// edge — see the module docs for why `Acquire` alone did not. +pub(crate) fn claim_for_teardown(slot: &AtomicI32, dead: &AtomicBool) -> Option { + dead.store(true, Ordering::Release); + taken(slot.swap(EMPTY, Ordering::AcqRel)) +} + +/// Ownership tests. These run on every platform's CI (the module has no Linux +/// in it) and drive each case directly rather than hoping a thread schedule +/// produces it. The assertion is always the same one: an fd that enters the +/// slot leaves it through exactly one caller. +/// +/// They cover the ownership protocol, NOT the memory orderings — those compile +/// to the same instructions on x86_64, so these pass against the wrong ones. +/// [`loom_tests`] is what covers the orderings. +#[cfg(all(test, not(loom)))] +mod tests { + use super::*; + + /// Fresh slot + liveness flag, as `SgIoTransport::open` builds them. + fn slot() -> (AtomicI32, AtomicBool) { + (AtomicI32::new(EMPTY), AtomicBool::new(false)) + } + + // ── Positive: the ordinary hand-off ──────────────────────────────────── + + /// The path taken on every successful recovery: the thread publishes, the + /// next `execute()` drains. Neither side is asked to close anything. + #[test] + fn published_fd_reaches_the_next_execute() { + let (s, d) = slot(); + assert_eq!( + publish_recovered_fd(&s, &d, 7), + None, + "publisher handed off" + ); + assert_eq!(take_recovered_fd(&s), Some(7), "execute() picks it up"); + assert_eq!(take_recovered_fd(&s), None, "slot is empty afterwards"); + } + + /// Recovery completes, but the transport is dropped before another + /// `execute()` runs (the abort-on-wedge path). Teardown owns the close. + #[test] + fn teardown_claims_an_undrained_fd() { + let (s, d) = slot(); + assert_eq!(publish_recovered_fd(&s, &d, 7), None); + assert_eq!(claim_for_teardown(&s, &d), Some(7), "Drop closes it"); + } + + /// A small thing that the `EMPTY` sentinel exists to get right: `0` is a + /// perfectly legal descriptor and must not read as an empty slot. + #[test] + fn fd_zero_is_a_real_fd_not_an_empty_slot() { + let (s, d) = slot(); + assert_eq!(publish_recovered_fd(&s, &d, 0), None); + assert_eq!(take_recovered_fd(&s), Some(0)); + } + + // ── Negative: every way the fd could go unclosed or be closed twice ──── + + /// Teardown wins the race. It drains an empty slot and will never look + /// again, so the late publisher must get its own fd back — this is the + /// leak the `Acquire`/`Release` orderings could produce in practice. + #[test] + fn publish_after_teardown_returns_the_fd_to_the_publisher() { + let (s, d) = slot(); + assert_eq!(claim_for_teardown(&s, &d), None, "nothing published yet"); + assert_eq!( + publish_recovered_fd(&s, &d, 7), + Some(7), + "transport is gone — publisher must close its own fd" + ); + assert_eq!(take_recovered_fd(&s), None, "and must not leave it behind"); + } + + /// Two recovery threads for one transport. The loser closes the fd it + /// opened; it must not overwrite the winner's, which nothing would then + /// close. + #[test] + fn losing_publisher_closes_its_own_fd_and_leaves_the_winners() { + let (s, d) = slot(); + assert_eq!(publish_recovered_fd(&s, &d, 7), None, "winner"); + assert_eq!(publish_recovered_fd(&s, &d, 9), Some(9), "loser closes 9"); + assert_eq!(take_recovered_fd(&s), Some(7), "winner's fd survived"); + } + + /// Teardown after the fd has already been drained by `execute()`. The slot + /// is empty and teardown must claim nothing — returning the stale value + /// here would close an fd the transport is still using. + #[test] + fn teardown_after_drain_claims_nothing() { + let (s, d) = slot(); + assert_eq!(publish_recovered_fd(&s, &d, 7), None); + assert_eq!(take_recovered_fd(&s), Some(7)); + assert_eq!(claim_for_teardown(&s, &d), None, "no double close"); + } + + /// Teardown twice (belt and braces — `Drop` runs once, but the second + /// claim must be inert rather than re-yielding the fd). + #[test] + fn teardown_is_idempotent() { + let (s, d) = slot(); + assert_eq!(publish_recovered_fd(&s, &d, 7), None); + assert_eq!(claim_for_teardown(&s, &d), Some(7)); + assert_eq!(claim_for_teardown(&s, &d), None); + } + + /// Empty slot, nothing published: both drains are no-ops. + #[test] + fn draining_an_empty_slot_yields_nothing() { + let (s, d) = slot(); + assert_eq!(take_recovered_fd(&s), None); + assert_eq!(claim_for_teardown(&s, &d), None); + } + + /// Accounting over the orders the three operations can run in, one whole + /// operation at a time: for each, the fd is handed to exactly one caller. + /// + /// Sequences, not interleavings — each function is the atomic unit here, so + /// this cannot express the case the ordering bug lives in (a teardown swap + /// landing between `publish_recovered_fd`'s CAS and its `dead` load). That + /// one is only reachable in the loom models below. + #[test] + fn every_sequence_closes_the_fd_exactly_once() { + // Each sequence returns how many callers were handed the fd across the + // whole run; the answer is always exactly one. + fn exactly_one(order: &str, run: impl Fn() -> usize) { + assert_eq!(run(), 1, "`{order}` must close fd 7 exactly once"); + } + + exactly_one("publish, drain", || { + let (s, d) = slot(); + publish_recovered_fd(&s, &d, 7).is_some() as usize + + take_recovered_fd(&s).is_some() as usize + }); + exactly_one("publish, teardown", || { + let (s, d) = slot(); + publish_recovered_fd(&s, &d, 7).is_some() as usize + + claim_for_teardown(&s, &d).is_some() as usize + }); + exactly_one("teardown, publish", || { + let (s, d) = slot(); + claim_for_teardown(&s, &d).is_some() as usize + + publish_recovered_fd(&s, &d, 7).is_some() as usize + }); + exactly_one("drain, publish, teardown", || { + let (s, d) = slot(); + take_recovered_fd(&s).is_some() as usize + + publish_recovered_fd(&s, &d, 7).is_some() as usize + + claim_for_teardown(&s, &d).is_some() as usize + }); + } + + /// The real race, run for real: a teardown thread against a recovery + /// thread, many times over. This exercises the protocol under a genuine + /// scheduler — it does NOT prove the memory orderings (x86 and, in + /// practice, AArch64 will happily pass the weaker ones). The loom model + /// below is what covers those; this covers the ownership logic. + #[test] + fn concurrent_teardown_and_publish_close_each_fd_exactly_once() { + use std::sync::Arc; + + for round in 0..2_000 { + let s = Arc::new(AtomicI32::new(EMPTY)); + let d = Arc::new(AtomicBool::new(false)); + let fd = 7; + + let (s1, d1) = (Arc::clone(&s), Arc::clone(&d)); + let teardown = std::thread::spawn(move || claim_for_teardown(&s1, &d1)); + let (s2, d2) = (Arc::clone(&s), Arc::clone(&d)); + let recovery = std::thread::spawn(move || publish_recovered_fd(&s2, &d2, fd)); + + let claimed = teardown.join().unwrap(); + let returned = recovery.join().unwrap(); + + let closers = claimed.is_some() as usize + returned.is_some() as usize; + assert_eq!( + closers, 1, + "round {round}: fd must be closed exactly once, not {closers} times \ + (0 = leaked descriptor, 2 = close() of a reused fd number)" + ); + assert_eq!( + s.load(Ordering::Acquire), + EMPTY, + "round {round}: slot must not be left holding an fd nobody owns" + ); + } + } +} + +/// Memory-ordering models. `cargo test` cannot observe a missing release edge: +/// the orderings compile to the same instructions on x86_64, and on AArch64 the +/// window is far too narrow to hit by chance. loom enumerates the executions +/// the C++20 model permits instead, and reports the stale `dead` load directly. +#[cfg(all(test, loom))] +mod loom_tests { + use super::*; + use loom::sync::Arc; + use loom::sync::atomic::AtomicUsize; + + /// `Drop` against a recovery thread: the execution this file exists for. + /// + /// With `swap(.., Acquire)` in [`claim_for_teardown`], or with + /// `compare_exchange(.., Release, ..)` in [`publish_recovered_fd`] — either + /// one alone, not just both — loom finds an execution where teardown + /// drains the empty slot, the recovery thread's CAS then fills it, and the + /// `dead` load still reads `false`. Nobody closes the fd. + #[test] + fn loom_teardown_racing_publish_closes_the_fd_exactly_once() { + loom::model(|| { + let slot = Arc::new(AtomicI32::new(EMPTY)); + let dead = Arc::new(AtomicBool::new(false)); + let closes = Arc::new(AtomicUsize::new(0)); + + let (s, d, c) = (slot.clone(), dead.clone(), closes.clone()); + let teardown = loom::thread::spawn(move || { + if claim_for_teardown(&s, &d).is_some() { + c.fetch_add(1, Ordering::Relaxed); + } + }); + + let (s, d, c) = (slot.clone(), dead.clone(), closes.clone()); + let recovery = loom::thread::spawn(move || { + if publish_recovered_fd(&s, &d, 7).is_some() { + c.fetch_add(1, Ordering::Relaxed); + } + }); + + teardown.join().unwrap(); + recovery.join().unwrap(); + + assert_eq!( + closes.load(Ordering::Relaxed), + 1, + "fd must be closed exactly once (0 = leaked, 2 = double close)" + ); + assert_eq!(slot.load(Ordering::Relaxed), EMPTY, "slot left non-empty"); + }); + } + + /// Two recovery threads against a teardown. `execute()` spawns a thread on + /// every transport-level failure and an earlier one may still be blocked in + /// `open()`, so this is reachable, and it is the case where a CAS lands + /// behind an intervening slot operation rather than reading the teardown + /// swap's value directly — the release-sequence half of the module docs. + /// + /// Bounded rather than exhaustive: unbounded, this model runs for many + /// minutes, which is too slow for the CI step. Three preemptions is enough + /// to reach the leaking schedule — the old orderings fail this test. + #[test] + fn loom_two_publishers_racing_teardown_close_each_fd_exactly_once() { + let mut model = loom::model::Builder::new(); + model.preemption_bound = Some(3); + model.check(|| { + let slot = Arc::new(AtomicI32::new(EMPTY)); + let dead = Arc::new(AtomicBool::new(false)); + let closes = Arc::new(AtomicUsize::new(0)); + + let publishers: Vec<_> = [7, 9] + .into_iter() + .map(|fd| { + let (s, d, c) = (slot.clone(), dead.clone(), closes.clone()); + loom::thread::spawn(move || { + if publish_recovered_fd(&s, &d, fd).is_some() { + c.fetch_add(1, Ordering::Relaxed); + } + }) + }) + .collect(); + + let (s, d, c) = (slot.clone(), dead.clone(), closes.clone()); + let teardown = loom::thread::spawn(move || { + if claim_for_teardown(&s, &d).is_some() { + c.fetch_add(1, Ordering::Relaxed); + } + }); + + for p in publishers { + p.join().unwrap(); + } + teardown.join().unwrap(); + + assert_eq!( + closes.load(Ordering::Relaxed), + 2, + "both fds must be closed, exactly once each" + ); + assert_eq!(slot.load(Ordering::Relaxed), EMPTY, "slot left non-empty"); + }); + } + + /// The check on leaving [`take_recovered_fd`] at `Acquire` — i.e. with a + /// relaxed store half. A drain landing between the teardown swap and a + /// recovery CAS must not break the release sequence the edge rides on. + #[test] + fn loom_drain_does_not_break_the_release_sequence() { + loom::model(|| { + let slot = Arc::new(AtomicI32::new(EMPTY)); + let dead = Arc::new(AtomicBool::new(false)); + let closes = Arc::new(AtomicUsize::new(0)); + + // execute() drains, then (later, same transport) Drop tears down. + let (s, d, c) = (slot.clone(), dead.clone(), closes.clone()); + let transport = loom::thread::spawn(move || { + if take_recovered_fd(&s).is_some() { + c.fetch_add(1, Ordering::Relaxed); + } + if claim_for_teardown(&s, &d).is_some() { + c.fetch_add(1, Ordering::Relaxed); + } + }); + + let (s, d, c) = (slot.clone(), dead.clone(), closes.clone()); + let recovery = loom::thread::spawn(move || { + if publish_recovered_fd(&s, &d, 7).is_some() { + c.fetch_add(1, Ordering::Relaxed); + } + }); + + transport.join().unwrap(); + recovery.join().unwrap(); + + assert_eq!( + closes.load(Ordering::Relaxed), + 1, + "fd must be closed exactly once (0 = leaked, 2 = double close)" + ); + assert_eq!(slot.load(Ordering::Relaxed), EMPTY, "slot left non-empty"); + }); + } +} diff --git a/src/scsi/linux.rs b/src/scsi/linux.rs index 3f844895..014dd277 100644 --- a/src/scsi/linux.rs +++ b/src/scsi/linux.rs @@ -9,9 +9,13 @@ //! See docs/scsi-linux.md for the escalation ladder, the design //! rationale, and the pre-0.13.20 async-poll design it replaced. +use super::fd_handoff::{ + AtomicBool, AtomicI32, claim_for_teardown, publish_recovered_fd, take_recovered_fd, +}; use super::{DataDirection, ScsiResult, ScsiTransport}; use crate::error::{Error, Result}; use std::path::Path; +use std::sync::Arc; const SG_IO: u32 = 0x2285; const SG_DXFER_NONE: i32 = -1; @@ -24,6 +28,10 @@ const SG_FLAG_Q_AT_HEAD: u32 = 0x10; /// 16 bytes. Matches `K_MAX_CDB_SIZE` in the macOS and Windows backends. const K_MAX_CDB_SIZE: usize = 16; +/// SPC-4 PREVENT ALLOW MEDIUM REMOVAL (0x1E) with PREVENT=0. Sent by `Drop` so +/// the tray is not left locked; a six-byte group-0 CDB. +const ALLOW_MEDIUM_REMOVAL: [u8; 6] = [0x1E, 0, 0, 0, 0, 0]; + #[repr(C)] #[allow(non_camel_case_types)] struct sg_io_hdr { @@ -61,12 +69,15 @@ const _: () = assert!(std::mem::size_of::() == 64); pub struct SgIoTransport { pub fd: i32, device_path: std::path::PathBuf, - pub fd_recovery: std::sync::Arc, - /// Set to `true` by `Drop` before the transport is torn down. The - /// recovery thread checks this after a successful `compare_exchange` - /// and closes `new_fd` itself when the transport is already gone, - /// preventing an fd leak when Drop races the recovery thread. - dead: std::sync::Arc, + /// Single-slot mailbox a background recovery thread publishes a freshly + /// opened fd into. Drained by `execute()`, or claimed by `Drop` if the + /// transport dies first. The protocol — and the memory ordering that makes + /// it leak-free — lives in `super::fd_handoff`. + pub fd_recovery: Arc, + /// Set to `true` by `Drop` before it claims the slot. A recovery thread + /// that publishes after that point sees it and closes its own fd, since + /// nothing will ever drain the slot again. + dead: Arc, } impl SgIoTransport { @@ -86,8 +97,8 @@ impl SgIoTransport { Ok(SgIoTransport { fd, device_path: device, - fd_recovery: std::sync::Arc::new(std::sync::atomic::AtomicI32::new(-1)), - dead: std::sync::Arc::new(std::sync::atomic::AtomicBool::new(false)), + fd_recovery: Arc::new(AtomicI32::new(super::fd_handoff::EMPTY)), + dead: Arc::new(AtomicBool::new(false)), }) } @@ -107,14 +118,22 @@ impl SgIoTransport { }) } - /// Send a raw SCSI command on an fd. Used by reset() before the - /// transport is constructed. - fn raw_command(fd: i32, cdb: &[u8], timeout_ms: u32) -> std::result::Result<(), ()> { + /// Send a raw SCSI command on a bare fd, for the one caller that has no + /// live `&mut self` to route through `execute()`: `Drop`, unlocking the + /// tray on its way out. + /// + /// The CDB goes through the shared [`super::checked_cdb_len`] guard rather + /// than the old `cdb.len().min(16)` clamp — see docs/scsi-mod.md, + /// "checked_cdb_len rationale". Returning a typed [`Error`] rather than + /// `()` is what lets `raw_command_cdb_guard_tests` tell a rejected CDB + /// from an I/O failure without a real `/dev/sg*` device. + fn raw_command(fd: i32, cdb: &[u8], timeout_ms: u32) -> Result<()> { + let cmd_len = super::checked_cdb_len(cdb, K_MAX_CDB_SIZE)?; let mut sense = [0u8; 32]; let mut hdr: sg_io_hdr = unsafe { std::mem::zeroed() }; hdr.interface_id = b'S' as i32; hdr.dxfer_direction = SG_DXFER_NONE; - hdr.cmd_len = cdb.len().min(16) as u8; + hdr.cmd_len = cmd_len; hdr.mx_sb_len = sense.len() as u8; hdr.dxfer_len = 0; hdr.dxferp = std::ptr::null_mut(); @@ -124,15 +143,32 @@ impl SgIoTransport { hdr.flags = SG_FLAG_Q_AT_HEAD; let ret = unsafe { libc::ioctl(fd, SG_IO as _, &mut hdr as *mut sg_io_hdr) }; + if ret < 0 { + return Err(Error::IoError { + source: std::io::Error::last_os_error(), + }); + } // Mask DRIVER_SENSE (0x08): it only flags "sense data present", not // a failure — matches execute()'s driver_status_real handling so a // benign CHECK CONDITION isn't misread as a transport error. let driver_status_real = hdr.driver_status & !super::DRIVER_SENSE; - if ret < 0 || hdr.status != 0 || hdr.host_status != 0 || driver_status_real != 0 { - Err(()) - } else { - Ok(()) + if hdr.host_status != 0 || driver_status_real != 0 { + // No SCSI status was delivered: same synthesised sentinel and same + // `sense: None` that `execute()` reports for a transport wedge. + return Err(Error::ScsiError { + opcode: cdb[0], + status: super::SCSI_STATUS_TRANSPORT_FAILURE, + sense: None, + }); } + if hdr.status != 0 { + return Err(Error::ScsiError { + opcode: cdb[0], + status: hdr.status, + sense: Some(super::parse_sense(&sense, hdr.sb_len_wr)), + }); + } + Ok(()) } fn to_c_path(device: &Path) -> Vec { @@ -174,20 +210,13 @@ impl Drop for SgIoTransport { fn drop(&mut self) { if self.fd >= 0 { // Unlock tray before closing — don't leave it locked. - let _ = Self::raw_command(self.fd, &[0x1E, 0, 0, 0, 0, 0], 3_000); + let _ = Self::raw_command(self.fd, &ALLOW_MEDIUM_REMOVAL, 3_000); unsafe { libc::close(self.fd) }; } - // Signal the recovery thread this transport is gone. Must be set - // before the fd_recovery swap so it can't observe dead=false and - // store into a slot Drop is no longer going to drain. - self.dead.store(true, std::sync::atomic::Ordering::Release); // A failed execute() spawns a thread that opens a fresh fd into - // fd_recovery, normally drained at the top of the next execute(). - // If dropped first (abort-on-wedge), claim and close it here. - let recovered = self - .fd_recovery - .swap(-1, std::sync::atomic::Ordering::Acquire); - if recovered >= 0 { + // fd_recovery, normally drained at the top of the next execute(). If + // dropped first (abort-on-wedge), claim and close it here. + if let Some(recovered) = claim_for_teardown(&self.fd_recovery, &self.dead) { unsafe { libc::close(recovered) }; } } @@ -221,10 +250,7 @@ impl ScsiTransport for SgIoTransport { ); // Check if a background recovery has produced a new fd. - let recovered = self - .fd_recovery - .swap(-1, std::sync::atomic::Ordering::Acquire); - if recovered >= 0 { + if let Some(recovered) = take_recovered_fd(&self.fd_recovery) { // Close the old fd if it's still valid. if self.fd >= 0 { unsafe { libc::close(self.fd) }; @@ -329,31 +355,11 @@ impl ScsiTransport for SgIoTransport { if new_fd < 0 { return; } - // Publish only into an empty (-1) slot. If two recovery - // threads race, the loser closes its own fd rather than - // overwriting (and leaking) the winner's. - if recovery - .compare_exchange( - -1, - new_fd, - std::sync::atomic::Ordering::Release, - std::sync::atomic::Ordering::Relaxed, - ) - .is_err() - { - // Another recovery thread already stored its fd; ours - // was not stored so it's our responsibility to close it. - unsafe { libc::close(new_fd) }; - return; - } - // Check whether Drop raced us: if the transport is already - // dead it won't drain fd_recovery, so swap to atomically - // claim new_fd and close it; a -1 result means Drop won. - if dead.load(std::sync::atomic::Ordering::Acquire) { - let claimed = recovery.swap(-1, std::sync::atomic::Ordering::AcqRel); - if claimed >= 0 { - unsafe { libc::close(claimed) }; - } + // Hand the fd to the transport. Comes back to us only if + // nobody there will ever close it: another recovery thread + // won the slot, or Drop already tore the transport down. + if let Some(orphan) = publish_recovered_fd(&recovery, &dead, new_fd) { + unsafe { libc::close(orphan) }; } }); @@ -586,3 +592,104 @@ pub(super) fn drive_has_disc(path: &Path) -> Result { }) } } + +// ── CDB guard wiring ─────────────────────────────────────────────────────── +#[cfg(test)] +mod raw_command_cdb_guard_tests { + use super::*; + + /// `ioctl()` on this fails with `EBADF` before touching any device, so the + /// tests below need no `/dev/sg*` and have no side effects — they run in + /// ordinary CI. Any error that is NOT `InvalidCdbLength` therefore means + /// the CDB cleared the guard and the syscall was reached. + const NO_FD: i32 = -1; + + fn reached_the_ioctl(err: &Error) -> bool { + !matches!(err, Error::InvalidCdbLength { .. }) + } + + // ── Negative: CDBs the guard must reject ────────────────────────────── + + /// The regression this module exists for. `raw_command` used to set + /// `cmd_len = cdb.len().min(16)`, silently shortening an over-length CDB + /// into a *different* SPC-4 command and issuing it. It must fail the + /// caller instead, with the same error `execute()` gives. + /// + /// Before the fix this test fails by reaching the `ioctl` and returning + /// `IoError(EBADF)` — the guard never ran. + #[test] + fn over_length_cdb_is_rejected_not_truncated() { + let cdb = [0u8; K_MAX_CDB_SIZE + 1]; + match SgIoTransport::raw_command(NO_FD, &cdb, 3_000) { + Err(Error::InvalidCdbLength { len, max }) => { + assert_eq!(len, K_MAX_CDB_SIZE + 1); + assert_eq!(max, K_MAX_CDB_SIZE); + } + other => panic!( + "over-length CDB must be rejected with InvalidCdbLength, got {other:?} — \ + a non-guard error means it was truncated to {K_MAX_CDB_SIZE} bytes and sent" + ), + } + } + + /// Well past the field width, to show the guard is a bound and not a + /// one-off check at `max + 1`. + #[test] + fn far_over_length_cdb_is_rejected() { + let cdb = [0u8; 260]; + assert!(matches!( + SgIoTransport::raw_command(NO_FD, &cdb, 3_000), + Err(Error::InvalidCdbLength { + len: 260, + max: K_MAX_CDB_SIZE + }) + )); + } + + /// An empty CDB must be rejected rather than handed to the sg driver as a + /// zero-length command descriptor, which under SPC-4 is not a command at + /// all. (The pre-fix code did not panic on this — it never read the opcode + /// — it just issued `cmd_len = 0`. The two error paths added here DO read + /// `cdb[0]`, so the guard is now load-bearing for that too.) + #[test] + fn empty_cdb_is_rejected_before_the_opcode_is_read() { + assert!(matches!( + SgIoTransport::raw_command(NO_FD, &[], 3_000), + Err(Error::InvalidCdbLength { + len: 0, + max: K_MAX_CDB_SIZE + }) + )); + } + + // ── Positive: CDBs the guard must let through ───────────────────────── + + /// The only CDB `raw_command` is called with in the crate. It must still + /// reach the syscall — a guard that rejected this would silently stop + /// unlocking the tray on `Drop`. + #[test] + fn drops_allow_medium_removal_still_reaches_the_ioctl() { + let err = SgIoTransport::raw_command(NO_FD, &ALLOW_MEDIUM_REMOVAL, 3_000) + .expect_err("EBADF on fd -1"); + assert!( + reached_the_ioctl(&err), + "the 6-byte CDB Drop sends must pass the guard, got {err:?}" + ); + } + + /// Every real SPC-4 CDB length (groups 0-5: 6, 10, 12, 16), plus the + /// shortest a caller could legally construct, plus the boundary case — a + /// CDB of exactly `K_MAX_CDB_SIZE` must not be caught by an off-by-one. + #[test] + fn in_range_cdb_lengths_reach_the_ioctl() { + for len in [1usize, 6, 10, 12, K_MAX_CDB_SIZE] { + let cdb = vec![0x1Eu8; len]; + let err = SgIoTransport::raw_command(NO_FD, &cdb, 3_000).expect_err("EBADF on fd -1"); + assert!( + reached_the_ioctl(&err), + "a {len}-byte CDB is within the {K_MAX_CDB_SIZE}-byte field and must \ + pass the guard, got {err:?}" + ); + } + } +} diff --git a/src/scsi/mod.rs b/src/scsi/mod.rs index 146c0371..77e48319 100644 --- a/src/scsi/mod.rs +++ b/src/scsi/mod.rs @@ -12,6 +12,12 @@ mod macos; #[cfg(target_os = "windows")] mod windows; +// The Linux recovery-thread fd hand-off. Compiled on every platform though +// only `linux.rs` uses it, so its tests run everywhere — the same reasoning as +// `checked_cdb_len`. Rationale and memory ordering: the module's own docs. +#[cfg_attr(not(target_os = "linux"), allow(dead_code))] +pub(crate) mod fd_handoff; + #[allow(unused_imports)] use crate::error::{Error, Result}; use std::path::Path;