0.31.0: hardening and correctness pass across mux, codec, AACS/CSS, UDF/MPLS/CLPI, recovery, drive/SCSI, labels, and I/O

Library-wide review-and-fix pass: tightened AACS keydb/handshake/variant
handling and trailing-partial-unit policy, corrected MPLS mark offset and
added UDF allocation bounds, hardened the mux/codec framing and M2TS paths,
guarded SCSI READ CAPACITY short transfers and unified error mapping, added
overflow guards on untrusted disc input, and made prefetch shutdown
deterministic. Release profile now builds with thin LTO + single codegen unit.
This commit is contained in:
Matthew Jackson
2026-06-07 17:37:38 -07:00
parent 5b6ea8f5c4
commit 061f68594a
128 changed files with 11838 additions and 3831 deletions
+69 -28
View File
@@ -10,9 +10,7 @@
//!
//! This matches what every reference project does: MakeMKV (8 s sync
//! ioctl), sg_dd (60 s sync ioctl), the kernel default for SCSI block
//! devices (30 s `/sys/.../timeout`). See
//! the SCSI architecture audit (2026-04-26) for the full primary-source
//! references.
//! devices (30 s `/sys/.../timeout`).
//!
//! Pre-0.13.20 we ran an async `write() + poll(1.5s) + close-on-timeout +
//! bg reopen` pattern. That abandoned slow-but-alive commands faster than
@@ -90,23 +88,10 @@ impl SgIoTransport {
})
}
/// Clean up kernel SG_IO state and unlock the tray. NOT a hardware
/// reset — purely software cleanup before this process opens the
/// device for real work.
///
/// When a previous process is killed (SIGKILL) mid-SG_IO, the kernel
/// may hold queued commands against the dead fd, and `Drop` never
/// ran so the tray may still be locked via PREVENT MEDIUM REMOVAL.
/// This routine handles both: open + close flushes the kernel SG
/// queue (sg_release cancels commands tied to the fd), the 2 s sleep
/// gives the kernel time to finish that cleanup, then a fresh fd
/// sends ALLOW MEDIUM REMOVAL to clear any stale tray lock.
///
/// We do NOT verify the drive with TUR or escalate to SG_SCSI_RESET /
/// STOP+START UNIT. Both escalations were tried in 0.13.00.13.5
/// against the LG BU40N (Initio USB-SATA bridge); both failed to
/// recover wedged drives and made the wedge worse — see
/// the BU40N wedge recovery postmortem (2026-04-25).
/// Map the current `errno` (from a failed `libc::open`) to a typed
/// [`Error`]: `EACCES`/`EPERM` → [`Error::DevicePermission`], anything
/// else → [`Error::DeviceNotFound`]. The device path is carried in the
/// error; no English commentary (the app layer localizes).
fn open_error<T>(device: &Path) -> Result<T> {
let err = std::io::Error::last_os_error();
Err(if err.kind() == std::io::ErrorKind::PermissionDenied {
@@ -186,6 +171,17 @@ impl Drop for SgIoTransport {
let _ = Self::raw_command(self.fd, &[0x1E, 0, 0, 0, 0, 0], 3_000);
unsafe { libc::close(self.fd) };
}
// A failed execute() spawns a detached thread that opens a fresh
// fd into fd_recovery; that slot is normally drained at the top of
// the next execute(). If the transport is dropped before another
// execute() runs (the common abort-on-wedge path), the recovered
// fd would otherwise leak. Claim and close it here.
let recovered = self
.fd_recovery
.swap(-1, std::sync::atomic::Ordering::Acquire);
if recovered >= 0 {
unsafe { libc::close(recovered) };
}
}
}
@@ -224,6 +220,17 @@ impl ScsiTransport for SgIoTransport {
data: &mut [u8],
timeout_ms: u32,
) -> Result<ScsiResult> {
// Guard the entry point: `ScsiTransport` is a pub trait, so an
// external caller could pass an empty CDB. Indexing cdb[0] below
// (and in the error paths) would panic. In-crate callers always
// pass non-empty literal CDBs.
if cdb.is_empty() {
return Err(Error::ScsiError {
opcode: 0,
status: super::SCSI_STATUS_TRANSPORT_FAILURE,
sense: None,
});
}
let exec_t0 = std::time::Instant::now();
let opcode = cdb[0];
tracing::trace!(
@@ -342,14 +349,36 @@ impl ScsiTransport for SgIoTransport {
});
std::thread::spawn(move || {
let c_path = std::ffi::CString::new(path.as_os_str().as_encoded_bytes()).unwrap();
// 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,
};
let new_fd = unsafe {
libc::open(
c_path.as_ptr() as *const libc::c_char,
libc::O_RDWR | libc::O_NONBLOCK | libc::O_CLOEXEC,
)
};
recovery.store(new_fd, std::sync::atomic::Ordering::Release);
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()
{
unsafe { libc::close(new_fd) };
}
});
return Err(Error::ScsiError {
@@ -383,7 +412,12 @@ impl ScsiTransport for SgIoTransport {
});
}
let bytes_transferred = (data.len() as i32).saturating_sub(hdr.resid).max(0) as usize;
// Compute in usize so transfers in the 24 GiB range (permitted by
// the `> u32::MAX` guard above) don't wrap through an i32 cast and
// report a large successful read as ~0 bytes. A negative resid is
// clamped to 0 before subtracting.
let resid = hdr.resid.max(0) as usize;
let bytes_transferred = data.len().saturating_sub(resid);
tracing::trace!(
target: "freemkv::scsi",
phase = "ok",
@@ -504,10 +538,15 @@ fn enumerate_sg_names() -> Vec<String> {
continue;
}
let type_path = format!("/sys/class/scsi_generic/{name}/device/type");
// By design: only type-5 (optical) sg nodes are collected.
// A non-optical `type` value, or an unreadable `type` file
// (race against device teardown, restricted sysfs in a minimal
// container), is silently skipped — neither is a fatal
// enumeration error, the node simply is not an optical target.
match std::fs::read_to_string(&type_path) {
Ok(s) if s.trim() == SCSI_TYPE_OPTICAL => names.push(name),
Ok(_) => {} // not optical
Err(_) => {}
Ok(_) => {} // not optical
Err(_) => {} // type file unreadable
}
}
} else {
@@ -553,12 +592,14 @@ pub(super) fn drive_has_disc(path: &Path) -> Result<bool> {
hdr.flags = SG_FLAG_Q_AT_HEAD;
let ret = unsafe { libc::ioctl(fd, SG_IO as _, &mut hdr as *mut sg_io_hdr) };
// Capture the ioctl errno BEFORE close(): POSIX permits close() to
// set errno (e.g. EIO on a flaky USB path), which would otherwise
// clobber the ioctl failure reason reported below.
let ioctl_err = std::io::Error::last_os_error();
unsafe { libc::close(fd) };
if ret < 0 {
return Err(Error::IoError {
source: std::io::Error::last_os_error(),
});
return Err(Error::IoError { source: ioctl_err });
}
let driver_status_real = hdr.driver_status & !super::DRIVER_SENSE;
+66 -6
View File
@@ -17,9 +17,21 @@
use super::{DataDirection, ScsiResult, ScsiTransport};
use crate::error::{Error, Result};
use std::path::Path;
use std::sync::atomic::{AtomicBool, Ordering};
const K_SENSE_DATA_SIZE: usize = 32;
/// Max CDB length the SCSI commands this library issues ever use; also
/// the clamp Linux applies. Used to bound the `cdb_len` passed to the
/// shim so a pathological >255-byte slice can't wrap a `u8`.
const K_MAX_CDB_SIZE: usize = 16;
/// The C shim uses a single global IOKit handle (`g_handle`), so only one
/// [`MacScsiTransport`] may exist at a time — a second `open()` would
/// share that handle and the first `drop()` would tear it down out from
/// under the other. This flag enforces single-instance ownership.
static OPEN: AtomicBool = AtomicBool::new(false);
#[repr(C)]
#[derive(Copy, Clone)]
struct ShimDriveInfo {
@@ -66,13 +78,38 @@ impl MacScsiTransport {
dev_str
};
// Enforce single-instance: the shim's global handle can't back two
// live transports safely. Bail rather than corrupt shared state.
if OPEN.swap(true, Ordering::Acquire) {
return Err(Error::DeviceLocked {
path: bsd_name.to_string(),
kr: 0,
});
}
let mut bsd_c = bsd_name.as_bytes().to_vec();
bsd_c.push(0);
let rc = unsafe { shim_open_exclusive(bsd_c.as_ptr()) };
if rc != 0 {
return Err(Error::DeviceNotFound {
path: bsd_name.to_string(),
// Release the single-instance lock taken by the OPEN.swap above;
// a failed open must not leave it held or every later open wedges.
OPEN.store(false, Ordering::Release);
let path = bsd_name.to_string();
// The shim returns distinct negative sentinels per failure
// stage; map them to the typed variants that already exist
// rather than collapsing every failure to DeviceNotFound.
// These sentinels are not IOReturn codes, so kr is left 0.
return Err(match rc {
// -2/-3/-4: IOCreatePlugInInterfaceForService /
// QueryInterface MMCDeviceInterface /
// GetSCSITaskDeviceInterface failed.
-4..=-2 => Error::IoKitPluginFailed { path, kr: 0 },
// -5: ObtainExclusiveAccess failed (held by another
// process).
-5 => Error::DeviceLocked { path, kr: 0 },
// -1 and anything else: device not present.
_ => Error::DeviceNotFound { path },
});
}
@@ -85,6 +122,7 @@ impl MacScsiTransport {
impl Drop for MacScsiTransport {
fn drop(&mut self) {
unsafe { shim_close() };
OPEN.store(false, Ordering::Release);
}
}
@@ -94,8 +132,25 @@ impl ScsiTransport for MacScsiTransport {
cdb: &[u8],
direction: DataDirection,
data: &mut [u8],
// NOTE: timeout_ms is currently ignored on macOS. The C shim
// (`macos_shim.c`) hardcodes `SetTimeoutDuration(task, 30000)`, so
// every command uses a fixed 30 s budget regardless of the
// caller's READ_TIMEOUT_MS / READ_RECOVERY_TIMEOUT_MS / TUR value.
// Plumbing it through the shim signature is tracked separately;
// macOS is dev/test-only per the project rules.
_timeout_ms: u32,
) -> Result<ScsiResult> {
// Match the Linux guard: a >=4 GiB buffer would wrap when cast to
// u32 for the shim, producing a short transfer reported as success
// with the wrong byte count.
if data.len() > u32::MAX as usize {
return Err(Error::ScsiError {
opcode: cdb.first().copied().unwrap_or(0),
status: super::SCSI_STATUS_TRANSPORT_FAILURE,
sense: None,
});
}
let data_in = match direction {
DataDirection::FromDevice => 1,
DataDirection::ToDevice => 0,
@@ -106,10 +161,11 @@ impl ScsiTransport for MacScsiTransport {
let mut task_status: u8 = 0xFF;
let mut transfer_count: u64 = 0;
let cdb_len = cdb.len().min(K_MAX_CDB_SIZE) as u8;
let kr = unsafe {
shim_execute(
cdb.as_ptr(),
cdb.len() as u8,
cdb_len,
data.as_mut_ptr(),
data.len() as u32,
data_in,
@@ -122,7 +178,7 @@ impl ScsiTransport for MacScsiTransport {
if kr != 0 {
return Err(Error::ScsiError {
opcode: cdb[0],
opcode: cdb.first().copied().unwrap_or(0),
status: super::SCSI_STATUS_TRANSPORT_FAILURE,
sense: None,
});
@@ -131,7 +187,7 @@ impl ScsiTransport for MacScsiTransport {
if task_status != 0 {
let parsed = super::parse_sense(&sense, K_SENSE_DATA_SIZE as u8);
return Err(Error::ScsiError {
opcode: cdb[0],
opcode: cdb.first().copied().unwrap_or(0),
status: task_status,
sense: Some(parsed),
});
@@ -139,7 +195,11 @@ impl ScsiTransport for MacScsiTransport {
Ok(ScsiResult {
status: 0,
bytes_transferred: transfer_count as usize,
// Clamp to the buffer length, matching the Linux transport's
// structural bound (data.len().saturating_sub(resid)). A lying
// drive/shim can't then produce a bytes_transferred that
// exceeds the buffer a future caller might slice with.
bytes_transferred: (transfer_count as usize).min(data.len()),
sense,
})
}
+22 -16
View File
@@ -56,8 +56,7 @@ pub(crate) const TUR_TIMEOUT_MS: u32 = 5_000;
///
/// 10 s catches every legitimate slow read with comfortable margin and
/// short-circuits truly bad sectors at ~10 s rather than letting the
/// kernel mid-layer escalate for 30 s+. See the SCSI architecture audit
/// (2026-04-26) for primary-source references.
/// kernel mid-layer escalate for 30 s+.
///
/// Pre-0.13.21 this was 1.5 s, which forced the kernel mid-layer to
/// time out *normal* reads (cold-start often takes ~1.5 s) and run its
@@ -245,9 +244,9 @@ impl ScsiSense {
/// `sb_len_wr` is the number of bytes the transport actually wrote into
/// `sense`. When the buffer is too short for the relevant fields we
/// return [`ScsiSense::NONE`] for the missing pieces rather than reading
/// uninitialised memory. The minimum useful sense reply per SPC-4 is 8
/// bytes (descriptor) or 14 bytes (fixed, to reach ASC/ASCQ at offsets
/// 12/13).
/// uninitialised memory. The minimum useful sense reply is 4 bytes
/// (descriptor, to reach ASCQ at offset 3) or 14 bytes (fixed, to reach
/// ASC/ASCQ at offsets 12/13).
///
/// Pure function — same parse on every platform backend (Linux SG_IO,
/// macOS IOKit, Windows SPTI) so a regression here would silently
@@ -261,7 +260,9 @@ pub(crate) fn parse_sense(sense: &[u8], sb_len_wr: u8) -> ScsiSense {
let descriptor = response_code == 0x72 || response_code == 0x73;
if descriptor {
// Descriptor format: key/asc/ascq are at fixed offsets 1/2/3.
let asc = if n >= 3 { sense[2] } else { 0 };
// n >= 3 is guaranteed by the early return above, so byte 2 is
// always in bounds; only ascq (byte 3) needs a length check.
let asc = sense[2];
let ascq = if n >= 4 { sense[3] } else { 0 };
ScsiSense {
sense_key: sense[1] & 0x0F,
@@ -449,13 +450,15 @@ pub fn list_drives() -> Vec<DriveInfo> {
/// other ready/not-ready response → `Ok(true)` or interpreted ready
/// state. Suitable for poll-loop tick (~50 ms / drive on a healthy bus).
///
/// **Internal wedge recovery.** When the kernel's response indicates a
/// wedged target the `0xff` status pattern that means "no answer from
/// the device" — this function transparently escalates: SCSI bus reset
/// → if still wedged → USB device reset (`USBDEVFS_RESET` on Linux) →
/// retry TUR. Callers never see wedge errors and never need to know
/// about the escalation; if even the recovery path can't get a response,
/// `Err(DeviceResetFailed)` surfaces. **No SCSI primitive is exposed to
/// **No internal recovery.** A single TUR is issued; nothing else. When
/// the transport reports a wedged target (the `0xff` "no answer from the
/// device" pattern synthesised by the backend from a non-zero
/// `host_status` / `driver_status`), that failure surfaces directly to
/// the caller as `Err(Error::ScsiError)` with
/// `status == SCSI_STATUS_TRANSPORT_FAILURE (0xFF)` and `sense: None`. No
/// SCSI bus reset, no USB device reset, no retry is attempted in-library
/// (the USB-reset escalation was removed in 0.13.4 after it was shown to
/// deepen rather than clear the wedge). **No SCSI primitive is exposed to
/// outside crates** — autorip / freemkv CLI / bdemu use this single
/// function for the entire "is there a disc?" decision.
pub fn drive_has_disc(path: &Path) -> Result<bool> {
@@ -561,8 +564,11 @@ pub fn build_set_cd_speed(read_speed: u16) -> [u8; 12] {
]
}
/// Build a READ(10) CDB with the raw read flag.
pub fn build_read10_raw(lba: u32, count: u16) -> [u8; 10] {
/// Build a READ(10) CDB with Force Unit Access (FUA) set — byte 1 bit 3
/// (0x08). FUA bypasses the drive cache and reads directly from the
/// medium. (Note: this is *not* a "raw" read; raw optical reads require
/// READ CD, opcode 0xBE.)
pub fn build_read10_fua(lba: u32, count: u16) -> [u8; 10] {
[
SCSI_READ_10,
0x08,
@@ -579,7 +585,7 @@ pub fn build_read10_raw(lba: u32, count: u16) -> [u8; 10] {
#[cfg(test)]
mod parse_sense_tests {
//! Unit tests for `parse_sense_key`. Covers both SPC-4 sense data
//! Unit tests for [`parse_sense`]. Covers both SPC-4 sense data
//! formats (descriptor / fixed) and the short-buffer fallback. The
//! same helper runs on every platform backend so a regression here
//! would silently miscategorize SCSI errors on Linux, macOS, and
+5 -5
View File
@@ -87,9 +87,9 @@ pub struct SptiTransport {
handle: isize,
}
// SptiTransport's only field is the isize HANDLE — Send is auto-derived
// and intentional. Sync is NOT: handle mutation in execute() requires
// &mut, enforced by the trait object dispatch.
// SptiTransport's only field is an isize HANDLE, so the compiler
// auto-derives BOTH Send and Sync. Exclusive use of the raw handle is
// enforced by `&mut self` on `execute()`, not by any absence of Sync.
/// Normalize a device path to Windows \\.\X: format.
///
@@ -307,7 +307,7 @@ impl ScsiTransport for SptiTransport {
// is at best redundant and at worst deepens the wedge. Caller
// surfaces the failure to UX.
return Err(Error::ScsiError {
opcode: cdb[0],
opcode: cdb.first().copied().unwrap_or(0),
status: super::SCSI_STATUS_TRANSPORT_FAILURE,
sense: None,
});
@@ -324,7 +324,7 @@ impl ScsiTransport for SptiTransport {
// `ScsiSense::is_medium_error()` etc.
let parsed = super::parse_sense(&sptwb.sense, K_SENSE_SIZE as u8);
return Err(Error::ScsiError {
opcode: cdb[0],
opcode: cdb.first().copied().unwrap_or(0),
status: sptwb.spt.ScsiStatus,
sense: Some(parsed),
});