From eedd84f3017e7280ed895a1e1a6d4eb26c368495 Mon Sep 17 00:00:00 2001 From: Nicolas Boichat Date: Tue, 22 Jul 2025 21:35:39 +0800 Subject: [PATCH] uucore: fsext: Change MountInfo's mount_root/dir to OsString This is necessary to fix handling of non-Unicode filepath in `df`. Note that other field might also need to be modified in the future. --- Cargo.lock | 1 + fuzz/Cargo.lock | 1 + src/uu/comm/src/comm.rs | 2 +- src/uu/df/src/df.rs | 12 ++-- src/uu/df/src/filesystem.rs | 18 +++--- src/uu/df/src/table.rs | 27 ++++----- src/uu/stat/src/stat.rs | 16 ++--- src/uucore/Cargo.toml | 1 + src/uucore/src/lib/features/fsext.rs | 88 ++++++++++++++++------------ 9 files changed, 90 insertions(+), 76 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a441ee4df..6a46111d8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3977,6 +3977,7 @@ dependencies = [ "bigdecimal", "blake2b_simd", "blake3", + "bstr", "chrono", "clap", "crc32fast", diff --git a/fuzz/Cargo.lock b/fuzz/Cargo.lock index cd90e4e67..21ed1dbee 100644 --- a/fuzz/Cargo.lock +++ b/fuzz/Cargo.lock @@ -1584,6 +1584,7 @@ dependencies = [ "bigdecimal", "blake2b_simd", "blake3", + "bstr", "clap", "crc32fast", "data-encoding", diff --git a/src/uu/comm/src/comm.rs b/src/uu/comm/src/comm.rs index a0e10b747..2d35d4d8b 100644 --- a/src/uu/comm/src/comm.rs +++ b/src/uu/comm/src/comm.rs @@ -101,7 +101,7 @@ impl OrderChecker { return true; } - let is_ordered = current_line >= &self.last_line; + let is_ordered = *current_line >= *self.last_line; if !is_ordered && !self.has_error { eprintln!( "{}", diff --git a/src/uu/df/src/df.rs b/src/uu/df/src/df.rs index bf1964db3..f0a8c40e8 100644 --- a/src/uu/df/src/df.rs +++ b/src/uu/df/src/df.rs @@ -630,9 +630,9 @@ mod tests { dev_id: String::new(), dev_name: String::from(dev_name), fs_type: String::new(), - mount_dir: String::from(mount_dir), + mount_dir: mount_dir.into(), mount_option: String::new(), - mount_root: String::from(mount_root), + mount_root: mount_root.into(), remote: false, dummy: false, } @@ -680,9 +680,9 @@ mod tests { dev_id: String::from(dev_id), dev_name: String::new(), fs_type: String::new(), - mount_dir: String::from(mount_dir), + mount_dir: mount_dir.into(), mount_option: String::new(), - mount_root: String::new(), + mount_root: "/".into(), remote: false, dummy: false, } @@ -725,9 +725,9 @@ mod tests { dev_id: String::new(), dev_name: String::new(), fs_type: String::from(fs_type), - mount_dir: String::from(mount_dir), + mount_dir: mount_dir.into(), mount_option: String::new(), - mount_root: String::new(), + mount_root: "/".into(), remote, dummy, } diff --git a/src/uu/df/src/filesystem.rs b/src/uu/df/src/filesystem.rs index 00905ab61..69e6b6dff 100644 --- a/src/uu/df/src/filesystem.rs +++ b/src/uu/df/src/filesystem.rs @@ -127,18 +127,18 @@ impl Filesystem { let _stat_path = if mount_info.mount_dir.is_empty() { #[cfg(unix)] { - mount_info.dev_name.clone() + mount_info.dev_name.clone().into() } #[cfg(windows)] { // On windows, we expect the volume id - mount_info.dev_id.clone() + mount_info.dev_id.clone().into() } } else { mount_info.mount_dir.clone() }; #[cfg(unix)] - let usage = FsUsage::new(statfs(_stat_path).ok()?); + let usage = FsUsage::new(statfs(&_stat_path).ok()?); #[cfg(windows)] let usage = FsUsage::new(Path::new(&_stat_path)).ok()?; Some(Self { @@ -205,6 +205,8 @@ mod tests { mod mount_info_from_path { + use std::ffi::OsString; + use uucore::fsext::MountInfo; use crate::filesystem::{FsError, mount_info_from_path}; @@ -215,9 +217,9 @@ mod tests { dev_id: String::default(), dev_name: String::default(), fs_type: String::default(), - mount_dir: String::from(mount_dir), + mount_dir: OsString::from(mount_dir), mount_option: String::default(), - mount_root: String::default(), + mount_root: OsString::default(), remote: Default::default(), dummy: Default::default(), } @@ -312,6 +314,8 @@ mod tests { #[cfg(not(windows))] mod over_mount { + use std::ffi::OsString; + use crate::filesystem::{Filesystem, FsError, is_over_mounted}; use uucore::fsext::MountInfo; @@ -320,9 +324,9 @@ mod tests { dev_id: String::default(), dev_name: dev_name.map(String::from).unwrap_or_default(), fs_type: String::default(), - mount_dir: String::from(mount_dir), + mount_dir: OsString::from(mount_dir), mount_option: String::default(), - mount_root: String::default(), + mount_root: OsString::default(), remote: Default::default(), dummy: Default::default(), } diff --git a/src/uu/df/src/table.rs b/src/uu/df/src/table.rs index e3725c07b..baa22ea71 100644 --- a/src/uu/df/src/table.rs +++ b/src/uu/df/src/table.rs @@ -35,7 +35,7 @@ pub(crate) struct Row { fs_type: String, /// Path at which the filesystem is mounted. - fs_mount: String, + fs_mount: OsString, /// Total number of bytes in the filesystem regardless of whether they are used. bytes: u64, @@ -277,7 +277,7 @@ impl<'a> RowFormatter<'a> { if self.is_total_row && !self.options.columns.contains(&Column::Source) { get_message("df-total") } else { - self.row.fs_mount.to_string() + self.row.fs_mount.to_string_lossy().into_owned() } } Column::Itotal => self.scaled_inodes(self.row.inodes), @@ -288,8 +288,7 @@ impl<'a> RowFormatter<'a> { .row .file .as_ref() - .map(|s| s.to_string_lossy().into_owned()) - .unwrap_or("-".into()), + .map_or("-".into(), |s| s.to_string_lossy().into_owned()), Column::Fstype => self.row.fs_type.to_string(), #[cfg(target_os = "macos")] @@ -525,7 +524,7 @@ mod tests { file: Some("/path/to/file".into()), fs_device: "my_device".to_string(), fs_type: "my_type".to_string(), - fs_mount: "my_mount".to_string(), + fs_mount: "my_mount".into(), bytes: 100, bytes_used: 25, @@ -684,7 +683,7 @@ mod tests { }; let row = Row { fs_device: "my_device".to_string(), - fs_mount: "my_mount".to_string(), + fs_mount: "my_mount".into(), bytes: 100, bytes_used: 25, @@ -711,7 +710,7 @@ mod tests { let row = Row { fs_device: "my_device".to_string(), fs_type: "my_type".to_string(), - fs_mount: "my_mount".to_string(), + fs_mount: "my_mount".into(), bytes: 100, bytes_used: 25, @@ -737,7 +736,7 @@ mod tests { }; let row = Row { fs_device: "my_device".to_string(), - fs_mount: "my_mount".to_string(), + fs_mount: "my_mount".into(), inodes: 10, inodes_used: 2, @@ -781,7 +780,7 @@ mod tests { let row = Row { fs_device: "my_device".to_string(), fs_type: "my_type".to_string(), - fs_mount: "my_mount".to_string(), + fs_mount: "my_mount".into(), bytes: 4000, bytes_used: 1000, @@ -808,7 +807,7 @@ mod tests { let row = Row { fs_device: "my_device".to_string(), fs_type: "my_type".to_string(), - fs_mount: "my_mount".to_string(), + fs_mount: "my_mount".into(), bytes: 4096, bytes_used: 1024, @@ -874,9 +873,9 @@ mod tests { dev_id: "28".to_string(), dev_name: "none".to_string(), fs_type: "9p".to_string(), - mount_dir: "/usr/lib/wsl/drivers".to_string(), + mount_dir: "/usr/lib/wsl/drivers".into(), mount_option: "ro,nosuid,nodev,noatime".to_string(), - mount_root: "/".to_string(), + mount_root: "/".into(), remote: false, dummy: false, }, @@ -905,9 +904,9 @@ mod tests { dev_id: "28".to_string(), dev_name: "none".to_string(), fs_type: "9p".to_string(), - mount_dir: "/usr/lib/wsl/drivers".to_string(), + mount_dir: "/usr/lib/wsl/drivers".into(), mount_option: "ro,nosuid,nodev,noatime".to_string(), - mount_root: "/".to_string(), + mount_root: "/".into(), remote: false, dummy: false, }, diff --git a/src/uu/stat/src/stat.rs b/src/uu/stat/src/stat.rs index c33eb354f..b9aebbbe7 100644 --- a/src/uu/stat/src/stat.rs +++ b/src/uu/stat/src/stat.rs @@ -22,7 +22,6 @@ use std::ffi::{OsStr, OsString}; use std::fs::{FileType, Metadata}; use std::io::Write; use std::os::unix::fs::{FileTypeExt, MetadataExt}; -use std::os::unix::prelude::OsStrExt; use std::path::Path; use std::{env, fs}; @@ -258,7 +257,7 @@ struct Stater { show_fs: bool, from_user: bool, files: Vec, - mount_list: Option>, + mount_list: Option>, default_tokens: Vec, default_dev_tokens: Vec, } @@ -876,7 +875,7 @@ impl Stater { })? .iter() .map(|mi| mi.mount_dir.clone()) - .collect::>(); + .collect::>(); // Reverse sort. The longer comes first. mount_list.sort(); mount_list.reverse(); @@ -899,7 +898,8 @@ impl Stater { for root in self.mount_list.as_ref()? { if path.starts_with(root) { - return Some(root.clone()); + // TODO: This is probably wrong, we should pass the OsString + return Some(root.to_string_lossy().into_owned()); } } None @@ -992,7 +992,7 @@ impl Stater { 'h' => OutputType::Unsigned(meta.nlink()), // inode number 'i' => OutputType::Unsigned(meta.ino()), - // mount point + // mount point: TODO: This should be an OsStr 'm' => OutputType::Str(self.find_mount_point(file).unwrap()), // file name 'n' => OutputType::Str(display_name.to_string()), @@ -1092,11 +1092,7 @@ impl Stater { OsString::from(file) }; if self.show_fs { - #[cfg(unix)] - let p = file.as_bytes(); - #[cfg(not(unix))] - let p = file.into_string().unwrap(); - match statfs(p) { + match statfs(&file) { Ok(meta) => { let tokens = &self.default_tokens; diff --git a/src/uucore/Cargo.toml b/src/uucore/Cargo.toml index 9a37f22fa..04175299f 100644 --- a/src/uucore/Cargo.toml +++ b/src/uucore/Cargo.toml @@ -19,6 +19,7 @@ all-features = true path = "src/lib/lib.rs" [dependencies] +bstr = { workspace = true } chrono = { workspace = true, optional = true } clap = { workspace = true } uucore_procs = { workspace = true } diff --git a/src/uucore/src/lib/features/fsext.rs b/src/uucore/src/lib/features/fsext.rs index 7ae6281d4..e1bea1689 100644 --- a/src/uucore/src/lib/features/fsext.rs +++ b/src/uucore/src/lib/features/fsext.rs @@ -28,8 +28,9 @@ static EXIT_ERR: i32 = 1; #[cfg(windows)] use crate::show_warning; -#[cfg(windows)] use std::ffi::OsStr; +#[cfg(unix)] +use std::os::unix::ffi::OsStrExt; #[cfg(windows)] use std::os::windows::ffi::OsStrExt; #[cfg(windows)] @@ -61,17 +62,15 @@ fn to_nul_terminated_wide_string(s: impl AsRef) -> Vec { use libc::{ S_IFBLK, S_IFCHR, S_IFDIR, S_IFIFO, S_IFLNK, S_IFMT, S_IFREG, S_IFSOCK, mode_t, strerror, }; -use std::borrow::Cow; #[cfg(unix)] -use std::ffi::CStr; -#[cfg(unix)] -use std::ffi::CString; +use std::ffi::{CStr, CString}; use std::io::Error as IOError; #[cfg(unix)] use std::mem; #[cfg(windows)] use std::path::Path; use std::time::UNIX_EPOCH; +use std::{borrow::Cow, ffi::OsString}; #[cfg(any( target_os = "linux", @@ -123,14 +122,16 @@ impl BirthTime for Metadata { } } +// TODO: Types for this struct are probably mostly wrong. Possibly, most of them +// should be OsString. #[derive(Debug, Clone)] pub struct MountInfo { /// Stores `volume_name` in windows platform and `dev_id` in unix platform pub dev_id: String, pub dev_name: String, pub fs_type: String, - pub mount_root: String, - pub mount_dir: String, + pub mount_root: OsString, + pub mount_dir: OsString, /// We only care whether this field contains "bind" pub mount_option: String, pub remote: bool, @@ -138,7 +139,9 @@ pub struct MountInfo { } #[cfg(any(target_os = "linux", target_os = "android"))] -fn replace_special_chars(s: String) -> String { +fn replace_special_chars(s: &[u8]) -> Vec { + use bstr::ByteSlice; + // Replace // // * ASCII space with a regular space character, @@ -152,7 +155,11 @@ fn replace_special_chars(s: String) -> String { impl MountInfo { #[cfg(any(target_os = "linux", target_os = "android"))] - fn new(file_name: &str, raw: &[&str]) -> Option { + fn new(file_name: &str, raw: &[&[u8]]) -> Option { + use std::ffi::OsStr; + use std::os::unix::ffi::OsStrExt; + use std::os::unix::ffi::OsStringExt; + let dev_name; let fs_type; let mount_root; @@ -165,21 +172,24 @@ impl MountInfo { // "man proc" for more details LINUX_MOUNTINFO => { const FIELDS_OFFSET: usize = 6; - let after_fields = raw[FIELDS_OFFSET..].iter().position(|c| *c == "-").unwrap() + let after_fields = raw[FIELDS_OFFSET..] + .iter() + .position(|c| *c == b"-") + .unwrap() + FIELDS_OFFSET + 1; - dev_name = raw[after_fields + 1].to_string(); - fs_type = raw[after_fields].to_string(); - mount_root = raw[3].to_string(); - mount_dir = replace_special_chars(raw[4].to_string()); - mount_option = raw[5].to_string(); + dev_name = String::from_utf8_lossy(raw[after_fields + 1]).to_string(); + fs_type = String::from_utf8_lossy(raw[after_fields]).to_string(); + mount_root = OsStr::from_bytes(raw[3]).to_owned(); + mount_dir = OsString::from_vec(replace_special_chars(raw[4])); + mount_option = String::from_utf8_lossy(raw[5]).to_string(); } LINUX_MTAB => { - dev_name = raw[0].to_string(); - fs_type = raw[2].to_string(); - mount_root = String::new(); - mount_dir = replace_special_chars(raw[1].to_string()); - mount_option = raw[3].to_string(); + dev_name = String::from_utf8_lossy(raw[0]).to_string(); + fs_type = String::from_utf8_lossy(raw[2]).to_string(); + mount_root = OsString::new(); + mount_dir = OsString::from_vec(replace_special_chars(raw[1])); + mount_option = String::from_utf8_lossy(raw[3]).to_string(); } _ => return None, }; @@ -343,7 +353,7 @@ fn is_remote_filesystem(dev_name: &str, fs_type: &str) -> bool { } #[cfg(all(unix, not(any(target_os = "aix", target_os = "redox"))))] -fn mount_dev_id(mount_dir: &str) -> String { +fn mount_dev_id(mount_dir: &OsStr) -> String { use std::os::unix::fs::MetadataExt; if let Ok(stat) = std::fs::metadata(mount_dir) { @@ -426,10 +436,10 @@ pub fn read_fs_list() -> UResult> { .or_else(|_| File::open(LINUX_MTAB).map(|f| (LINUX_MTAB, f)))?; let reader = BufReader::new(f); Ok(reader - .lines() + .split(b'\n') .map_while(Result::ok) .filter_map(|line| { - let raw_data = line.split_whitespace().collect::>(); + let raw_data = line.split(|c| *c == b' ').collect::>(); MountInfo::new(file_name, &raw_data) }) .collect::>()) @@ -855,11 +865,13 @@ impl FsMeta for StatFs { } #[cfg(unix)] -pub fn statfs

(path: P) -> Result -where - P: Into>, -{ - match CString::new(path) { +pub fn statfs(path: &OsStr) -> Result { + #[cfg(unix)] + let p = path.as_bytes(); + #[cfg(not(unix))] + let p = path.into_string().unwrap(); + + match CString::new(p) { Ok(p) => { let mut buffer: StatFs = unsafe { mem::zeroed() }; unsafe { @@ -1060,8 +1072,8 @@ mod tests { // spell-checker:ignore (word) relatime let info = MountInfo::new( LINUX_MOUNTINFO, - &"106 109 253:6 / /mnt rw,relatime - xfs /dev/fs0 rw" - .split_ascii_whitespace() + &b"106 109 253:6 / /mnt rw,relatime - xfs /dev/fs0 rw" + .split(|c| *c == b' ') .collect::>(), ) .unwrap(); @@ -1075,8 +1087,8 @@ mod tests { // Test parsing with different amounts of optional fields. let info = MountInfo::new( LINUX_MOUNTINFO, - &"106 109 253:6 / /mnt rw,relatime master:1 - xfs /dev/fs0 rw" - .split_ascii_whitespace() + &b"106 109 253:6 / /mnt rw,relatime master:1 - xfs /dev/fs0 rw" + .split(|c| *c == b' ') .collect::>(), ) .unwrap(); @@ -1086,8 +1098,8 @@ mod tests { let info = MountInfo::new( LINUX_MOUNTINFO, - &"106 109 253:6 / /mnt rw,relatime master:1 shared:2 - xfs /dev/fs0 rw" - .split_ascii_whitespace() + &b"106 109 253:6 / /mnt rw,relatime master:1 shared:2 - xfs /dev/fs0 rw" + .split(|c| *c == b' ') .collect::>(), ) .unwrap(); @@ -1101,8 +1113,8 @@ mod tests { fn test_mountinfo_dir_special_chars() { let info = MountInfo::new( LINUX_MOUNTINFO, - &r#"317 61 7:0 / /mnt/f\134\040\011oo rw,relatime shared:641 - ext4 /dev/loop0 rw"# - .split_ascii_whitespace() + &br#"317 61 7:0 / /mnt/f\134\040\011oo rw,relatime shared:641 - ext4 /dev/loop0 rw"# + .split(|c| *c == b' ') .collect::>(), ) .unwrap(); @@ -1111,8 +1123,8 @@ mod tests { let info = MountInfo::new( LINUX_MTAB, - &r#"/dev/loop0 /mnt/f\134\040\011oo ext4 rw,relatime 0 0"# - .split_ascii_whitespace() + &br#"/dev/loop0 /mnt/f\134\040\011oo ext4 rw,relatime 0 0"# + .split(|c| *c == b' ') .collect::>(), ) .unwrap();