audit fixes: propagate real read errors, log SCSI failures, cap recovery threads

Confirmed code-audit findings (v1.6.10..HEAD):
- aacs/mod.rs read_first: propagate a real DiscRead I/O error instead of
  collapsing every read failure to AacsNoKeys; skip only a missing candidate.
- drive::lock_tray / set_speed: log a warning on SCSI command failure so a
  failed tray-lock / speed-set is visible (was silently discarded).
- scsi/linux.rs: cap outstanding detached fd-recovery threads (8); past the cap
  the close/reopen runs inline so a sustained bridge wedge can't spawn unbounded.
- docs/unlock-bridge.md: reconcile with the current unlock_bridge.rs (no
  all_unlockers()/NotApplicable; unlocker_names() is a literal; MT1959->LD).
- scsi/macos.rs test: assert the OPEN delta, not the process-global bit, so the
  presence-probe leak test can't flake under concurrent tests.

docs/unlock-bridge.md

@@ -2,23 +2,31 @@
 
 Bridges libfreemkv's drive layer to the `freemkv-unlock` crate: one generic
 SCSI-transport adapter, identity/host-cert mapping, and the dispatch that
-news up `all_unlockers()` and runs the first matching one. libfreemkv names
-no individual unlocker — it only calls this bridge.
+assembles the unlocker list and runs each one's `unlock()` until one claims
+the drive. libfreemkv names no individual unlocker — it only calls this bridge.
 
 ## `Dispatch`
 
-Result of a capability dispatch: `(matched_name, result)`. `matched_name` is
-the unlocker that handled it (or `""` if none did) — lets the caller record
-WHICH unlocker ran (e.g. `MT1959` vs `Renesas`), distinct from the id-only
-identity lookup `unlocker_name`. Iterating stops at the first unlocker whose
-capability method returns anything other than `NotApplicable` — i.e. an actual
-unlock (`Ok`) OR a real failure such as a dead bus (`Err(Transport)`), which
-the caller must surface rather than skip.
+Result of a dispatch: `(matched_name, result)`. `matched_name` is the
+unlocker that handled it (or `""` if none did) — lets the caller record WHICH
+unlocker ran (e.g. `LD` vs `Renesas`), distinct from the id-only identity
+lookup `unlocker_name`. Iterating stops at the first unlocker whose
+`unlock()` returns anything other than `Ok(None)` — i.e. an actual unlock
+(`Ok(Some)`) OR a real failure such as a dead bus (`Err`), which the caller
+must surface rather than skip.
+
+## `run_features` / `run_bus`
+
+Both build their own fixed `Vec<Box<dyn fu::Unlocker>>` — `run_features` the
+firmware unlockers (freemkv, LD, Renesas), `run_bus` the disc-keyed ones
+(AACS, DVD) — and hand it to the shared `run()` loop. There is no
+`all_unlockers()` registry; adding or removing an unlocker means editing
+both this file's two lists and `unlocker_names()` below.
 
 ## `unlocker_names`
 
-The names of every REGISTERED unlocker, in dispatch order. Registry-driven —
-sourced from `all_unlockers()`, so adding/removing an unlocker updates every
-report with no other change (no hardcoded names). The per-unlocker "did it
-run this rip" outcome is computed by the caller, which has the disc + drive
-runtime state this crate cannot see.
+The unlocker names, in dispatch order, for the user-facing unlocker matrix.
+A hardcoded `vec!["freemkv", "LD", "Renesas", "AACS", "DVD"]` — kept in sync
+with `run_features`/`run_bus` by hand, not registry-driven. The per-unlocker
+"did it run this rip" outcome is computed by the caller, which has the disc +
+drive runtime state this crate cannot see.

src/aacs/mod.rs

@@ -105,17 +105,19 @@ pub(crate) fn role_paths(udf: &crate::udf::UdfFs, role: AacsRole) -> Vec<String>
     v
 }
 
-// Walk an AACS role's candidate paths and return the first that reads;
-// generic over `&str`/`String` so it accepts `role_paths`' `Vec<String>`.
-// Returns `Error::AacsNoKeys` if no candidate is present.
+// Walk an AACS role's candidate paths, returning the first that reads.
+// A missing candidate is skipped; any other error (e.g. a real `DiscRead`)
+// propagates; `AacsNoKeys` only when every candidate was absent.
 pub(crate) fn read_first<S, F>(candidates: &[S], mut read: F) -> crate::error::Result<Vec<u8>>
 where
     S: AsRef<str>,
     F: FnMut(&str) -> crate::error::Result<Vec<u8>>,
 {
     for path in candidates {
-        if let Ok(buf) = read(path.as_ref()) {
-            return Ok(buf);
+        match read(path.as_ref()) {
+            Ok(buf) => return Ok(buf),
+            Err(crate::error::Error::UdfNotFound { .. }) => continue,
+            Err(e) => return Err(e),
         }
     }
     Err(crate::error::Error::AacsNoKeys)

src/drive/mod.rs

@@ -984,7 +984,10 @@ impl Drive {
     pub fn set_speed(&mut self, speed_kbs: u16) {
         let cdb = crate::scsi::build_set_cd_speed(speed_kbs);
         let mut dummy = [0u8; 0];
-        let _ = self.scsi_execute(&cdb, crate::scsi::DataDirection::None, &mut dummy, 5_000);
+        if let Err(e) = self.scsi_execute(&cdb, crate::scsi::DataDirection::None, &mut dummy, 5_000)
+        {
+            tracing::warn!(target: "freemkv::drive", error = %e, "SET CD SPEED failed");
+        }
     }
 
     /// Lock the tray so the disc cannot be ejected during a rip.
@@ -998,10 +1001,13 @@ impl Drive {
             0x00,
         ];
         let mut buf = [0u8; 0];
-        let _ =
+        if let Err(e) =
             self.scsi
                 .as_mut()
-                .execute(&prevent, crate::scsi::DataDirection::None, &mut buf, 5_000);
+                .execute(&prevent, crate::scsi::DataDirection::None, &mut buf, 5_000)
+        {
+            tracing::warn!(target: "freemkv::drive", error = %e, "PREVENT MEDIUM REMOVAL failed");
+        }
     }
 
     /// Unlock the tray so the user can manually eject the disc.

src/scsi/linux.rs

@@ -32,6 +32,12 @@ const K_MAX_CDB_SIZE: usize = 16;
 /// the tray is not left locked; a six-byte group-0 CDB.
 const ALLOW_MEDIUM_REMOVAL: [u8; 6] = [0x1E, 0, 0, 0, 0, 0];
 
+/// Cap on detached fd-recovery threads outstanding at once (process-wide).
+/// A sustained bridge wedge would otherwise spawn 2 threads per failed ioctl
+/// with no bound; past the cap, recovery runs inline instead of spawning.
+const MAX_RECOVERY_THREADS: usize = 8;
+static RECOVERY_THREADS: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0);
+
 #[repr(C)]
 #[allow(non_camel_case_types)]
 struct sg_io_hdr {
@@ -332,36 +338,64 @@ impl ScsiTransport for SgIoTransport {
             let recovery = self.fd_recovery.clone();
             let dead = self.dead.clone();
 
-            std::thread::spawn(move || {
-                if old_fd >= 0 {
-                    unsafe { libc::close(old_fd) };
-                }
-            });
+            // Cap outstanding recovery threads: past MAX_RECOVERY_THREADS a
+            // sustained wedge would spawn unbounded threads, so fall back to
+            // doing the close/reopen inline on this thread instead.
+            use std::sync::atomic::Ordering;
+            if RECOVERY_THREADS.load(Ordering::Acquire) < MAX_RECOVERY_THREADS {
+                RECOVERY_THREADS.fetch_add(1, Ordering::AcqRel);
+                std::thread::spawn(move || {
+                    if old_fd >= 0 {
+                        unsafe { libc::close(old_fd) };
+                    }
+                    RECOVERY_THREADS.fetch_sub(1, Ordering::AcqRel);
+                });
+            } else if old_fd >= 0 {
+                unsafe { libc::close(old_fd) };
+            }
 
-            std::thread::spawn(move || {
-                // Don't unwrap: a device path with an interior NUL would
-                // panic this detached thread (silently swallowed). Bail
-                // and leave fd_recovery untouched instead.
-                let c_path = match std::ffi::CString::new(path.as_os_str().as_encoded_bytes()) {
-                    Ok(c) => c,
-                    Err(_) => return,
-                };
+            if RECOVERY_THREADS.load(Ordering::Acquire) < MAX_RECOVERY_THREADS {
+                RECOVERY_THREADS.fetch_add(1, Ordering::AcqRel);
+                std::thread::spawn(move || {
+                    // Don't unwrap: a device path with an interior NUL would
+                    // panic this detached thread (silently swallowed). Bail
+                    // and leave fd_recovery untouched instead.
+                    let c_path = match std::ffi::CString::new(path.as_os_str().as_encoded_bytes()) {
+                        Ok(c) => c,
+                        Err(_) => {
+                            RECOVERY_THREADS.fetch_sub(1, Ordering::AcqRel);
+                            return;
+                        }
+                    };
+                    let new_fd = unsafe {
+                        libc::open(
+                            c_path.as_ptr() as *const libc::c_char,
+                            libc::O_RDWR | libc::O_NONBLOCK | libc::O_CLOEXEC,
+                        )
+                    };
+                    if new_fd >= 0 {
+                        // 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) };
+                        }
+                    }
+                    RECOVERY_THREADS.fetch_sub(1, Ordering::AcqRel);
+                });
+            } else if let Ok(c_path) = std::ffi::CString::new(path.as_os_str().as_encoded_bytes()) {
+                // At the recovery-thread cap: reopen inline (blocking this
+                // call briefly) instead of spawning an unbounded 9th thread.
                 let new_fd = unsafe {
                     libc::open(
                         c_path.as_ptr() as *const libc::c_char,
                         libc::O_RDWR | libc::O_NONBLOCK | libc::O_CLOEXEC,
                     )
                 };
-                if new_fd < 0 {
-                    return;
+                if new_fd >= 0 {
+                    self.fd = new_fd;
                 }
-                // 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) };
-                }
-            });
+            }
 
             return Err(Error::ScsiError {
                 opcode: cdb[0],

src/scsi/macos.rs

@@ -310,6 +310,10 @@ mod tests {
     #[test]
     fn presence_probe_does_not_open_a_transport() {
         let path = Path::new("/dev/freemkv-no-such-device");
+        // Snapshot rather than assume `false`: OPEN is a process-global bit any
+        // concurrent test's live transport can hold, so compare the delta — it
+        // scopes the check to this call, can't flake, still catches a held lock.
+        let open_before = OPEN.load(Ordering::Acquire);
         let t0 = std::time::Instant::now();
         let r = drive_has_disc(path);
         let elapsed = t0.elapsed();
@@ -323,9 +327,10 @@ mod tests {
             "probe took {elapsed:?}: the transport path's unconditional 500 ms \
              post-unmount sleep means this budget can only be met without it"
         );
-        assert!(
-            !OPEN.load(Ordering::Acquire),
-            "the probe must not leave the exclusive-transport lock held"
+        assert_eq!(
+            OPEN.load(Ordering::Acquire),
+            open_before,
+            "the probe must not change the exclusive-transport lock state"
         );
     }
 

src/unlock_bridge.rs

@@ -118,7 +118,7 @@ fn run(
     ("", Ok(None))
 }
 
-/// Drive-prep: run the FIRMWARE unlockers (freemkv / MT1959 / Renesas), which
+/// Drive-prep: run the FIRMWARE unlockers (freemkv / LD / Renesas), which
 /// key off the drive rather than the disc, so `kind` is `Unknown` and they need
 /// no certs. Each removes bus encryption at the drive and reads the OEM Volume
 /// ID best-effort.