From e8bad70b8d164b81a740f29fc5e3ab634338648d Mon Sep 17 00:00:00 2001 From: Shiva Kiran Koninty Date: Mon, 24 Aug 2026 20:56:38 +0530 Subject: [PATCH 1/2] Gate media removal tests behind `mmc` feature to fix `cargo test` --- README.md | 4 ++-- tests/media_removal.rs | 10 ++++++---- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 4f64dd9..b8e4b21 100644 --- a/README.md +++ b/README.md @@ -42,7 +42,7 @@ git commit --no-verify ### Hardware tests The MMC tests are ignored by default as they require a CD/DVD drive. ```sh -cargo test -- --include-ignored +cargo test --all-features -- --include-ignored ``` A few of the tests require manual intervention (such as media @@ -51,7 +51,7 @@ These are located in [`tests/`](./tests). Example, to run the media removal test: ```sh -MANUAL_TESTS=1 cargo test -- media_removal --include-ignored +MANUAL_TESTS=1 cargo test --all-features -- media_removal --include-ignored ``` ## See also diff --git a/tests/media_removal.rs b/tests/media_removal.rs index 9c9db5a..96dbc42 100644 --- a/tests/media_removal.rs +++ b/tests/media_removal.rs @@ -1,12 +1,14 @@ //! Tests that would remove the drive media -use libcdio_rs::{ - Mmc, - mmc::{MmcCloseTrayError, MmcError, MmcSenseData, MmcStartStopError, SenseKey}, -}; +#[cfg(feature = "mmc")] #[test] #[ignore = "requires a drive with mmc"] fn media_removal() { + use libcdio_rs::{ + Mmc, + mmc::{MmcCloseTrayError, MmcError, MmcSenseData, MmcStartStopError, SenseKey}, + }; + if std::env::var("MANUAL_TESTS").is_err() { return; } From 9b7d9906011b7943004cfa0f4934b02428a90f20 Mon Sep 17 00:00:00 2001 From: Shiva Kiran Koninty Date: Mon, 24 Aug 2026 21:07:40 +0530 Subject: [PATCH 2/2] drive: Fix thread safety of `Drive::devices()` Memory corruption issues were discovered when running drive related tests together parallelly (with `cargo test -- --include-ignored`). It was found that the GNU/Linux initialization routine `open_am_linux()` was NOT thread safe. `Drive::devices()` invokes `cdio_get_devices()`, which internally initialized an instance of `CdIo_t`, making this whole chain unsafe. Earlier only `CdIo_t`'s C constructor `cdio_open()` and cleanup function `cdio_destroy()` were deemed thread unsafe, due to their use of a static, and a `Mutex` was used to uphold safety. Given that even the underlying intialization routine is also unsafe, reuse the lock for all routines that in any way initialize `CdIo_t`. --- src/cdio.rs | 16 ++++++++++------ src/drive.rs | 11 ++++++++++- 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/cdio.rs b/src/cdio.rs index cd3645b..3124509 100644 --- a/src/cdio.rs +++ b/src/cdio.rs @@ -48,7 +48,7 @@ impl Cdio { // SAFETY: This invokes cdio_init(), which mutates a static variable. // CDIO_LAST_DRIVER_LOCK is held to prevent data races. - let _lock = CDIO_LAST_DRIVER_LOCK.lock().unwrap(); + let _lock = CDIO_INIT_LOCK.lock().unwrap(); return unsafe { libcdio_sys::cdio_open_am(source, driver, access_mode) }; /// Although prefixed "MMC", this does imply read-write for all @@ -67,7 +67,7 @@ impl Deref for Cdio { impl Drop for Cdio { fn drop(&mut self) { - let _lock = CDIO_LAST_DRIVER_LOCK.lock().unwrap(); + let _lock = CDIO_INIT_LOCK.lock().unwrap(); // SAFETY: This method invokes modifies a static variable. // CDIO_LAST_DRIVER_LOCK is held to prevent data races. @@ -75,7 +75,11 @@ impl Drop for Cdio { } } -/// A lock guarding a private static named `CdIo_last_driver`. It must be held -/// before invoking any libcdio methods that modify this value. -/// As of libcdio v2.3.0, such methods are `cdio_init()` and `cdio_destroy()`. -static CDIO_LAST_DRIVER_LOCK: Mutex<()> = Mutex::new(()); +/// A lock that must be held before any routine that initializes or +/// destroys `CdIo_t`. +/// It was found that the GNU/Linux driver initialization routine, +/// is NOT thread safe as of libcdio v2.4.0. +/// Apart from that, this also guards the use of a private static +/// named `CdIo_last_driver`, used by `CdIo_t` during init +/// and cleanup. +pub(crate) static CDIO_INIT_LOCK: Mutex<()> = Mutex::new(()); diff --git a/src/drive.rs b/src/drive.rs index 90e4a29..77bc39e 100644 --- a/src/drive.rs +++ b/src/drive.rs @@ -29,7 +29,10 @@ use bitflags::bitflags; use libcdio_sys::cdio_hwinfo_t; use thiserror::Error; -use crate::cdio::Cdio; +use crate::{ + cdio::{CDIO_INIT_LOCK, Cdio}, + logging, +}; /// An interface to a disc drive. pub struct Drive { @@ -39,6 +42,12 @@ pub struct Drive { impl Drive { /// Returns a list of connected drives. pub fn drives() -> Vec { + logging::init_logger(); + + // SAFETY: This method internally initializes an instance of CdIo_t, + // which is not thread safe. Hold CDIO_INIT_LOCK to uphold thread + // safety. + let _lock = CDIO_INIT_LOCK.lock().unwrap(); let drive_list = unsafe { libcdio_sys::cdio_get_devices(libcdio_sys::driver_id_t_DRIVER_DEVICE) }; if drive_list.is_null() {