From 8fc888ebed853314a98ff720d01f3de93737a182 Mon Sep 17 00:00:00 2001 From: Sylvestre Ledru Date: Fri, 12 Sep 2025 16:39:22 +0200 Subject: [PATCH] safe traversal: use the nix crate instead of unsafe calls to libc --- src/uucore/Cargo.toml | 8 +- src/uucore/src/lib/features/safe_traversal.rs | 259 +++++++----------- 2 files changed, 99 insertions(+), 168 deletions(-) diff --git a/src/uucore/Cargo.toml b/src/uucore/Cargo.toml index b34543372..5d3677294 100644 --- a/src/uucore/Cargo.toml +++ b/src/uucore/Cargo.toml @@ -80,7 +80,13 @@ fluent-bundle = { workspace = true } thiserror = { workspace = true } [target.'cfg(unix)'.dependencies] walkdir = { workspace = true, optional = true } -nix = { workspace = true, features = ["fs", "uio", "zerocopy", "signal"] } +nix = { workspace = true, features = [ + "fs", + "uio", + "zerocopy", + "signal", + "dir", +] } xattr = { workspace = true, optional = true } [dev-dependencies] diff --git a/src/uucore/src/lib/features/safe_traversal.rs b/src/uucore/src/lib/features/safe_traversal.rs index d470f6412..2fe3df400 100644 --- a/src/uucore/src/lib/features/safe_traversal.rs +++ b/src/uucore/src/lib/features/safe_traversal.rs @@ -16,12 +16,17 @@ #[cfg(test)] use std::os::unix::ffi::OsStringExt; -use std::ffi::{CStr, CString, OsStr, OsString}; +use std::ffi::{CString, OsStr, OsString}; use std::io; use std::os::unix::ffi::OsStrExt; -use std::os::unix::io::{AsRawFd, RawFd}; +use std::os::unix::io::{AsFd, AsRawFd, BorrowedFd, FromRawFd, OwnedFd, RawFd}; use std::path::Path; +use nix::dir::Dir; +use nix::fcntl::{OFlag, openat}; +use nix::sys::stat::{FileStat, Mode, fstatat}; +use nix::unistd::{UnlinkatFlags, unlinkat}; + // Custom error types for better error reporting #[derive(thiserror::Error, Debug)] pub enum SafeTraversalError { @@ -71,87 +76,46 @@ impl From for io::Error { } } -// RAII wrapper for DIR pointer -struct Dir { - dirp: *mut libc::DIR, -} +// Helper function to read directory entries using nix +fn read_dir_entries(fd: &OwnedFd) -> io::Result> { + let mut entries = Vec::new(); -impl Dir { - fn from_fd(fd: RawFd) -> io::Result { - let dirp = unsafe { libc::fdopendir(fd) }; - if dirp.is_null() { - Err(io::Error::last_os_error()) - } else { - Ok(Dir { dirp }) + // Duplicate the fd for Dir (it takes ownership) + let dup_fd = nix::unistd::dup(fd).map_err(|e| io::Error::from_raw_os_error(e as i32))?; + + let mut dir = Dir::from_fd(dup_fd).map_err(|e| io::Error::from_raw_os_error(e as i32))?; + + for entry_result in dir.iter() { + let entry = entry_result.map_err(|e| io::Error::from_raw_os_error(e as i32))?; + + let name = entry.file_name(); + let name_os = OsStr::from_bytes(name.to_bytes()); + + if name_os != "." && name_os != ".." { + entries.push(name_os.to_os_string()); } } - fn read_entries(&self) -> io::Result> { - let mut entries = Vec::new(); - - loop { - // Clear errno before readdir as per POSIX requirements - unsafe { *libc::__errno_location() = 0 }; - - let entry = unsafe { libc::readdir(self.dirp) }; - if entry.is_null() { - let errno = unsafe { *libc::__errno_location() }; - if errno != 0 { - return Err(io::Error::from_raw_os_error(errno)); - } - break; - } - - let name = unsafe { CStr::from_ptr((*entry).d_name.as_ptr()) }; - let name_os = OsStr::from_bytes(name.to_bytes()); - - if name_os != "." && name_os != ".." { - entries.push(name_os.to_os_string()); - } - } - - Ok(entries) - } -} - -impl Drop for Dir { - fn drop(&mut self) { - if !self.dirp.is_null() { - unsafe { - libc::closedir(self.dirp); - } - } - } + Ok(entries) } /// A directory file descriptor that enables safe traversal pub struct DirFd { - fd: RawFd, - owned: bool, + fd: OwnedFd, } impl DirFd { /// Open a directory and return a file descriptor pub fn open(path: &Path) -> io::Result { - let path_cstr = CString::new(path.as_os_str().as_bytes()) - .map_err(|_| SafeTraversalError::PathContainsNull)?; - - let fd = unsafe { - libc::open( - path_cstr.as_ptr(), - libc::O_RDONLY | libc::O_DIRECTORY | libc::O_CLOEXEC, - ) - }; - - if fd < 0 { - Err(SafeTraversalError::OpenFailed { + let flags = OFlag::O_RDONLY | OFlag::O_DIRECTORY | OFlag::O_CLOEXEC; + let fd = nix::fcntl::open(path, flags, Mode::empty()).map_err(|e| { + SafeTraversalError::OpenFailed { path: path.to_string_lossy().into_owned(), - source: io::Error::last_os_error(), + source: io::Error::from_raw_os_error(e as i32), } - .into()) - } else { - Ok(DirFd { fd, owned: true }) - } + })?; + + Ok(DirFd { fd }) } /// Open a subdirectory relative to this directory @@ -159,48 +123,36 @@ impl DirFd { let name_cstr = CString::new(name.as_bytes()).map_err(|_| SafeTraversalError::PathContainsNull)?; - let fd = unsafe { - libc::openat( - self.fd, - name_cstr.as_ptr(), - libc::O_RDONLY | libc::O_DIRECTORY | libc::O_CLOEXEC, - ) - }; - - if fd < 0 { - Err(SafeTraversalError::OpenFailed { + let flags = OFlag::O_RDONLY | OFlag::O_DIRECTORY | OFlag::O_CLOEXEC; + let fd = openat(&self.fd, name_cstr.as_c_str(), flags, Mode::empty()).map_err(|e| { + SafeTraversalError::OpenFailed { path: name.to_string_lossy().into_owned(), - source: io::Error::last_os_error(), + source: io::Error::from_raw_os_error(e as i32), } - .into()) - } else { - Ok(DirFd { fd, owned: true }) - } + })?; + + Ok(DirFd { fd }) } /// Get raw stat data for a file relative to this directory - pub fn stat_at(&self, name: &OsStr, follow_symlinks: bool) -> io::Result { + pub fn stat_at(&self, name: &OsStr, follow_symlinks: bool) -> io::Result { let name_cstr = CString::new(name.as_bytes()).map_err(|_| SafeTraversalError::PathContainsNull)?; - let mut stat: libc::stat = unsafe { std::mem::zeroed() }; let flags = if follow_symlinks { - 0 + nix::fcntl::AtFlags::empty() } else { - libc::AT_SYMLINK_NOFOLLOW + nix::fcntl::AtFlags::AT_SYMLINK_NOFOLLOW }; - let ret = unsafe { libc::fstatat(self.fd, name_cstr.as_ptr(), &mut stat, flags) }; - - if ret < 0 { - Err(SafeTraversalError::StatFailed { + let stat = fstatat(&self.fd, name_cstr.as_c_str(), flags).map_err(|e| { + SafeTraversalError::StatFailed { path: name.to_string_lossy().into_owned(), - source: io::Error::last_os_error(), + source: io::Error::from_raw_os_error(e as i32), } - .into()) - } else { - Ok(stat) - } + })?; + + Ok(stat) } /// Get metadata for a file relative to this directory @@ -214,43 +166,18 @@ impl DirFd { } /// Get raw stat data for this directory - pub fn fstat(&self) -> io::Result { - let mut stat: libc::stat = unsafe { std::mem::zeroed() }; + pub fn fstat(&self) -> io::Result { + let stat = nix::sys::stat::fstat(&self.fd).map_err(|e| SafeTraversalError::StatFailed { + path: "".to_string(), + source: io::Error::from_raw_os_error(e as i32), + })?; - let ret = unsafe { libc::fstat(self.fd, &mut stat) }; - - if ret < 0 { - Err(SafeTraversalError::StatFailed { - path: "".to_string(), - source: io::Error::last_os_error(), - } - .into()) - } else { - Ok(stat) - } + Ok(stat) } /// Read directory entries pub fn read_dir(&self) -> io::Result> { - // Duplicate the fd for fdopendir (it takes ownership) - let dup_fd = unsafe { libc::dup(self.fd) }; - if dup_fd < 0 { - return Err(SafeTraversalError::ReadDirFailed { - path: "".to_string(), - source: io::Error::last_os_error(), - } - .into()); - } - - let dir = Dir::from_fd(dup_fd).map_err(|e| { - unsafe { libc::close(dup_fd) }; - SafeTraversalError::ReadDirFailed { - path: "".to_string(), - source: e, - } - })?; - - dir.read_entries().map_err(|e| { + read_dir_entries(&self.fd).map_err(|e| { SafeTraversalError::ReadDirFailed { path: "".to_string(), source: e, @@ -263,22 +190,23 @@ impl DirFd { pub fn unlink_at(&self, name: &OsStr, is_dir: bool) -> io::Result<()> { let name_cstr = CString::new(name.as_bytes()).map_err(|_| SafeTraversalError::PathContainsNull)?; - let flags = if is_dir { libc::AT_REMOVEDIR } else { 0 }; - - let ret = unsafe { libc::unlinkat(self.fd, name_cstr.as_ptr(), flags) }; - - if ret < 0 { - Err(SafeTraversalError::UnlinkFailed { - path: name.to_string_lossy().into_owned(), - source: io::Error::last_os_error(), - } - .into()) + let flags = if is_dir { + UnlinkatFlags::RemoveDir } else { - Ok(()) - } + UnlinkatFlags::NoRemoveDir + }; + + unlinkat(&self.fd, name_cstr.as_c_str(), flags).map_err(|e| { + SafeTraversalError::UnlinkFailed { + path: name.to_string_lossy().into_owned(), + source: io::Error::from_raw_os_error(e as i32), + } + })?; + + Ok(()) } - /// Create a DirFd from an existing file descriptor (does not take ownership) + /// Create a DirFd from an existing file descriptor (takes ownership) pub fn from_raw_fd(fd: RawFd) -> io::Result { if fd < 0 { return Err(io::Error::new( @@ -286,23 +214,21 @@ impl DirFd { "invalid file descriptor", )); } - Ok(DirFd { fd, owned: false }) - } -} - -impl Drop for DirFd { - fn drop(&mut self) { - if self.owned && self.fd >= 0 { - unsafe { - libc::close(self.fd); - } - } + // SAFETY: We've verified fd >= 0, and the caller is transferring ownership + let owned_fd = unsafe { OwnedFd::from_raw_fd(fd) }; + Ok(DirFd { fd: owned_fd }) } } impl AsRawFd for DirFd { fn as_raw_fd(&self) -> RawFd { - self.fd + self.fd.as_raw_fd() + } +} + +impl AsFd for DirFd { + fn as_fd(&self) -> BorrowedFd<'_> { + self.fd.as_fd() } } @@ -374,11 +300,11 @@ impl FileType { /// Metadata wrapper for safer access to file information #[derive(Debug, Clone)] pub struct Metadata { - stat: libc::stat, + stat: FileStat, } impl Metadata { - pub fn from_stat(stat: libc::stat) -> Self { + pub fn from_stat(stat: FileStat) -> Self { Self { stat } } @@ -410,11 +336,6 @@ impl Metadata { } } - /// Get the raw libc::stat for compatibility with existing code - pub fn as_raw_stat(&self) -> &libc::stat { - &self.stat - } - /// Compatibility methods to match std::fs::Metadata interface pub fn is_dir(&self) -> bool { self.file_type().is_directory() @@ -558,6 +479,7 @@ mod tests { use super::*; use std::fs; use std::os::unix::fs::symlink; + use std::os::unix::io::IntoRawFd; use tempfile::TempDir; #[test] @@ -696,11 +618,16 @@ mod tests { fn test_from_raw_fd() { let temp_dir = TempDir::new().unwrap(); let dir_fd = DirFd::open(temp_dir.path()).unwrap(); - let raw_fd = dir_fd.as_raw_fd(); - let borrowed_fd = DirFd::from_raw_fd(raw_fd).unwrap(); - assert_eq!(borrowed_fd.as_raw_fd(), raw_fd); - assert!(!borrowed_fd.owned); // Should not own the FD + // Duplicate the fd first so we don't have ownership conflicts + let dup_fd = nix::unistd::dup(&dir_fd).unwrap(); + let from_raw_fd = DirFd::from_raw_fd(dup_fd.into_raw_fd()).unwrap(); + + // Both should refer to the same directory + let stat1 = dir_fd.fstat().unwrap(); + let stat2 = from_raw_fd.fstat().unwrap(); + assert_eq!(stat1.st_ino, stat2.st_ino); + assert_eq!(stat1.st_dev, stat2.st_dev); } #[test] @@ -772,9 +699,7 @@ mod tests { assert_eq!(metadata.mode() & libc::S_IFMT as u32, libc::S_IFREG as u32); assert_eq!(metadata.nlink(), 1); - // Test raw stat access - let raw_stat = metadata.as_raw_stat(); - assert_eq!(raw_stat.st_size, metadata.size() as i64); + assert!(metadata.size() > 0); } #[test]