From 640fc70d0163e65cffc7b9d1830cb10d3ae65848 Mon Sep 17 00:00:00 2001 From: Kun Lai Date: Thu, 5 Feb 2026 21:02:34 +0800 Subject: [PATCH] feat(fs): replace `is_empty_disk` with more precise `has_valuable_data` Introduce `has_valuable_data()` to better determine whether a block device contains meaningful filesystem data: - When an expected filesystem type (`fs_hint`) is provided, it strictly checks for that type and errors if something else is detected. - Without a hint, it treats known partition tables like `PTTYPE="atari"` as non-valuable (workaround for util-linux bug), while considering any other detected signature as valuable data. - Update callers in `cryptpilot-crypt` and tests to use the new logic, flipping boolean expectations accordingly. This improves safety during volume initialization by avoiding accidental overwrites of unexpected but valid data. Signed-off-by: Kun Lai --- cryptpilot-core/src/fs/mkfs.rs | 70 ++++++++++++++++++-------- cryptpilot-crypt/src/cmd/open.rs | 6 ++- cryptpilot-crypt/tests/volume_tests.rs | 28 +++++++++-- 3 files changed, 78 insertions(+), 26 deletions(-) diff --git a/cryptpilot-core/src/fs/mkfs.rs b/cryptpilot-core/src/fs/mkfs.rs index e098bfa..58d8f6e 100644 --- a/cryptpilot-core/src/fs/mkfs.rs +++ b/cryptpilot-core/src/fs/mkfs.rs @@ -191,7 +191,12 @@ pub async fn force_mkfs( Ok(()) } -pub async fn is_empty_disk(device_path: &Path) -> Result { +/// Checks whether the device contains valuable data (i.e., a known filesystem). +/// +/// - Partition tables (e.g., PTTYPE="atari") are ignored and treated as "no valuable data". +/// - If `fs_hint` is `Some`, and the expected filesystem is not detected, returns an error. +/// - If `fs_hint` is `None`, returns `true` if something detected on the device. +pub async fn has_valuable_data(device_path: &Path, fs_hint: Option) -> Result { Command::new("blkid") .arg("-p") .arg(device_path) @@ -199,31 +204,54 @@ pub async fn is_empty_disk(device_path: &Path) -> Result { .run_with_status_checker(|code, stdout, stderr| { match code { 0 => { - // Found signatures, check if they are ext4, xfs, vfat or swap let output = String::from_utf8_lossy(&stdout); - let has_fs = output.contains("TYPE=\"ext4\"") - || output.contains("TYPE=\"xfs\"") - || output.contains("TYPE=\"vfat\"") - || output.contains("TYPE=\"swap\""); - Ok(!has_fs) // Return true if NOT one of these filesystems (empty or other fs) + + match fs_hint { + Some(expected_fs) => { + let expected_str = match expected_fs { + MakeFsType::Swap => "swap", + MakeFsType::Ext4 => "ext4", + MakeFsType::Xfs => "xfs", + MakeFsType::Vfat => "vfat", + }; + + if output.contains(&format!("TYPE=\"{}\"", expected_str)) { + Ok(true) + } else { + // Something else detected (may have partition table, unknown FS, etc.) + bail!( + "Something else on {device_path:?} is detected, but expected '{expected_str}', found blkid output '{}'", + output.trim() + ); + } + } + None => { + // Check if it's mistakenly identified as PTTYPE="atari" partition table. + // See: https://bugs.launchpad.net/ubuntu/+source/util-linux/+bug/2015355 + if output.contains("PTTYPE=\"atari\"") { + tracing::debug!("Found PTTYPE=\"atari\" partition table on {device_path:?}, treating as no valuable data"); + Ok(false) + }else{ + // Log which one was found (for debug) + tracing::debug!("Found filesystem signature on {device_path:?}, blkid output: {}", output.trim()); + Ok(true) + } + } + } + } + 2 => { + // No signatures found + Ok(false) } - 2 => Ok(true), // No signatures found _ => { let stdout = String::from_utf8_lossy(&stdout); let stderr = String::from_utf8_lossy(&stderr); - if stdout.contains("Input/output error") - || stderr.contains("Input/output error") - { - // If we can't even read the disk, treat it as "uninitialized" - Ok(true) - } else { - bail!( - "blkid failed with exit code {}: stdout={}, stderr={}", - code, - stdout, - stderr - ) - } + bail!( + "blkid failed with exit code {}: stdout='{}', stderr='{}'", + code, + stdout.trim(), + stderr.trim() + ); } } }) diff --git a/cryptpilot-crypt/src/cmd/open.rs b/cryptpilot-crypt/src/cmd/open.rs index f80ead8..bb3884c 100644 --- a/cryptpilot-crypt/src/cmd/open.rs +++ b/cryptpilot-crypt/src/cmd/open.rs @@ -58,7 +58,11 @@ pub async fn open_for_specific_volume(volume_config: &VolumeConfig, check_fs: bo // Check if filesystem is ready if check_fs && volume_config.extra_config.makefs.is_some() - && cryptpilot::fs::mkfs::is_empty_disk(&volume_config.volume_path()).await? + && !cryptpilot::fs::mkfs::has_valuable_data( + &volume_config.volume_path(), + volume_config.extra_config.makefs, + ) + .await? { // TODO: replace with RAII here let _ = cryptpilot::fs::luks2::close(&volume_config.volume).await; diff --git a/cryptpilot-crypt/tests/volume_tests.rs b/cryptpilot-crypt/tests/volume_tests.rs index d794a48..ff30e3a 100644 --- a/cryptpilot-crypt/tests/volume_tests.rs +++ b/cryptpilot-crypt/tests/volume_tests.rs @@ -164,7 +164,11 @@ pub async fn run_test_on_volume(config_str: &str, use_external_suite: bool) -> R open_then(&volume_config, |volume_config| async move { if !matches!(volume_config.extra_config.integrity, Some(true)) { assert!( - !cryptpilot::fs::mkfs::is_empty_disk(&volume_config.volume_path()).await? + cryptpilot::fs::mkfs::has_valuable_data( + &volume_config.volume_path(), + volume_config.extra_config.makefs + ) + .await? ); } Ok(()) @@ -221,7 +225,11 @@ pub async fn run_test_on_volume(config_str: &str, use_external_suite: bool) -> R open_then(&volume_config, |volume_config| async move { if !matches!(volume_config.extra_config.integrity, Some(true)) { assert!( - !cryptpilot::fs::mkfs::is_empty_disk(&volume_config.volume_path()).await? + cryptpilot::fs::mkfs::has_valuable_data( + &volume_config.volume_path(), + volume_config.extra_config.makefs + ) + .await? ); } Ok(()) @@ -288,14 +296,26 @@ pub async fn run_test_on_volume(config_str: &str, use_external_suite: bool) -> R // Just Open it and do nothing but checking open_then(&volume_config, |volume_config| async move { // Add assertion to check if disk is empty - assert!(cryptpilot::fs::mkfs::is_empty_disk(&volume_config.volume_path()).await?); + assert!( + !cryptpilot::fs::mkfs::has_valuable_data( + &volume_config.volume_path(), + volume_config.extra_config.makefs + ) + .await? + ); Ok(()) }) .await?; // Test again open_then(&volume_config, |volume_config| async move { // Add assertion to check if disk is still empty after re-open - assert!(cryptpilot::fs::mkfs::is_empty_disk(&volume_config.volume_path()).await?); + assert!( + !cryptpilot::fs::mkfs::has_valuable_data( + &volume_config.volume_path(), + volume_config.extra_config.makefs + ) + .await? + ); Ok(()) }) .await?;