From 8a1e9df17bd27e40894cfea21c570c381db6c088 Mon Sep 17 00:00:00 2001 From: SteveLauC Date: Sun, 9 Jun 2024 18:40:38 +0800 Subject: [PATCH] refactor: I/O safety for sys/stat.rs (#2439) * refactor: I/O safety for sys/stat.rs * fix test on FreeBSD & correct changelog PR number --- changelog/2439.changed.md | 1 + src/fcntl.rs | 27 +++++++--- src/sys/stat.rs | 73 +++++++++++++++++---------- test/sys/test_stat.rs | 102 +++++++++++--------------------------- test/test_fcntl.rs | 8 +-- test/test_unistd.rs | 15 ++---- 6 files changed, 103 insertions(+), 123 deletions(-) create mode 100644 changelog/2439.changed.md diff --git a/changelog/2439.changed.md b/changelog/2439.changed.md new file mode 100644 index 0000000000..ea5a992b31 --- /dev/null +++ b/changelog/2439.changed.md @@ -0,0 +1 @@ +Module `sys/stat` now adopts I/O safety. diff --git a/src/fcntl.rs b/src/fcntl.rs index ab82e4a73d..f38b4d5d8a 100644 --- a/src/fcntl.rs +++ b/src/fcntl.rs @@ -66,9 +66,8 @@ pub use self::posix_fadvise::{posix_fadvise, PosixFadviseAdvice}; /// use nix::errno::Errno; /// use nix::fcntl::AT_FDCWD; /// use nix::sys::stat::fstat; -/// use std::os::fd::AsRawFd; /// -/// let never = fstat(AT_FDCWD.as_raw_fd()).unwrap(); +/// let never = fstat(AT_FDCWD).unwrap(); /// ``` // // SAFETY: @@ -585,13 +584,20 @@ unsafe fn inner_readlink( Some(_) => unreachable!("redox does not have readlinkat(2)"), #[cfg(any(linux_android, target_os = "freebsd", target_os = "hurd"))] Some(dirfd) => { + // SAFETY: + // + // If this call of `borrow_raw()` is safe or not depends on the + // usage of `unsafe fn inner_readlink()`. + let dirfd = unsafe { + std::os::fd::BorrowedFd::borrow_raw(dirfd) + }; let flags = if path.is_empty() { AtFlags::AT_EMPTY_PATH } else { AtFlags::empty() }; super::sys::stat::fstatat( - Some(dirfd), + dirfd, path, flags | AtFlags::AT_SYMLINK_NOFOLLOW, ) @@ -602,11 +608,16 @@ unsafe fn inner_readlink( target_os = "freebsd", target_os = "hurd" )))] - Some(dirfd) => super::sys::stat::fstatat( - Some(dirfd), - path, - AtFlags::AT_SYMLINK_NOFOLLOW, - ), + Some(dirfd) => { + // SAFETY: + // + // If this call of `borrow_raw()` is safe or not depends on the + // usage of `unsafe fn inner_readlink()`. + let dirfd = unsafe { + std::os::fd::BorrowedFd::borrow_raw(dirfd) + }; + super::sys::stat::fstatat(dirfd, path, AtFlags::AT_SYMLINK_NOFOLLOW) + }, None => super::sys::stat::lstat(path), } .map(|x| x.st_size) diff --git a/src/sys/stat.rs b/src/sys/stat.rs index c5854eec01..ba3a76f704 100644 --- a/src/sys/stat.rs +++ b/src/sys/stat.rs @@ -6,11 +6,10 @@ pub use libc::stat as FileStat; pub use libc::{dev_t, mode_t}; #[cfg(not(target_os = "redox"))] -use crate::fcntl::{at_rawfd, AtFlags}; +use crate::fcntl::AtFlags; use crate::sys::time::{TimeSpec, TimeVal}; use crate::{errno::Errno, NixPath, Result}; use std::mem; -use std::os::unix::io::RawFd; libc_bitflags!( /// "File type" flags for `mknod` and related functions. @@ -168,17 +167,18 @@ pub fn mknod( /// Create a special or ordinary file, relative to a given directory. #[cfg(not(any(apple_targets, target_os = "redox", target_os = "haiku")))] -pub fn mknodat( - dirfd: Option, +pub fn mknodat( + dirfd: Fd, path: &P, kind: SFlag, perm: Mode, dev: dev_t, ) -> Result<()> { - let dirfd = at_rawfd(dirfd); + use std::os::fd::AsRawFd; + let res = path.with_nix_path(|cstr| unsafe { libc::mknodat( - dirfd, + dirfd.as_fd().as_raw_fd(), cstr.as_ptr(), kind.bits() | perm.bits() as mode_t, dev, @@ -233,9 +233,11 @@ pub fn lstat(path: &P) -> Result { Ok(unsafe { dst.assume_init() }) } -pub fn fstat(fd: RawFd) -> Result { +pub fn fstat(fd: Fd) -> Result { + use std::os::fd::AsRawFd; + let mut dst = mem::MaybeUninit::uninit(); - let res = unsafe { libc::fstat(fd, dst.as_mut_ptr()) }; + let res = unsafe { libc::fstat(fd.as_fd().as_raw_fd(), dst.as_mut_ptr()) }; Errno::result(res)?; @@ -243,16 +245,17 @@ pub fn fstat(fd: RawFd) -> Result { } #[cfg(not(target_os = "redox"))] -pub fn fstatat( - dirfd: Option, +pub fn fstatat( + dirfd: Fd, pathname: &P, f: AtFlags, ) -> Result { - let dirfd = at_rawfd(dirfd); + use std::os::fd::AsRawFd; + let mut dst = mem::MaybeUninit::uninit(); let res = pathname.with_nix_path(|cstr| unsafe { libc::fstatat( - dirfd, + dirfd.as_fd().as_raw_fd(), cstr.as_ptr(), dst.as_mut_ptr(), f.bits() as libc::c_int, @@ -269,8 +272,11 @@ pub fn fstatat( /// # References /// /// [fchmod(2)](https://pubs.opengroup.org/onlinepubs/9699919799/functions/fchmod.html). -pub fn fchmod(fd: RawFd, mode: Mode) -> Result<()> { - let res = unsafe { libc::fchmod(fd, mode.bits() as mode_t) }; +pub fn fchmod(fd: Fd, mode: Mode) -> Result<()> { + use std::os::fd::AsRawFd; + + let res = + unsafe { libc::fchmod(fd.as_fd().as_raw_fd(), mode.bits() as mode_t) }; Errno::result(res).map(drop) } @@ -299,19 +305,21 @@ pub enum FchmodatFlags { /// /// [fchmodat(2)](https://pubs.opengroup.org/onlinepubs/9699919799/functions/fchmodat.html). #[cfg(not(target_os = "redox"))] -pub fn fchmodat( - dirfd: Option, +pub fn fchmodat( + dirfd: Fd, path: &P, mode: Mode, flag: FchmodatFlags, ) -> Result<()> { + use std::os::fd::AsRawFd; + let atflag = match flag { FchmodatFlags::FollowSymlink => AtFlags::empty(), FchmodatFlags::NoFollowSymlink => AtFlags::AT_SYMLINK_NOFOLLOW, }; let res = path.with_nix_path(|cstr| unsafe { libc::fchmodat( - at_rawfd(dirfd), + dirfd.as_fd().as_raw_fd(), cstr.as_ptr(), mode.bits() as mode_t, atflag.bits() as libc::c_int, @@ -383,9 +391,15 @@ pub fn lutimes( /// /// [futimens(2)](https://pubs.opengroup.org/onlinepubs/9699919799/functions/futimens.html). #[inline] -pub fn futimens(fd: RawFd, atime: &TimeSpec, mtime: &TimeSpec) -> Result<()> { +pub fn futimens( + fd: Fd, + atime: &TimeSpec, + mtime: &TimeSpec, +) -> Result<()> { + use std::os::fd::AsRawFd; + let times: [libc::timespec; 2] = [*atime.as_ref(), *mtime.as_ref()]; - let res = unsafe { libc::futimens(fd, ×[0]) }; + let res = unsafe { libc::futimens(fd.as_fd().as_raw_fd(), ×[0]) }; Errno::result(res).map(drop) } @@ -418,13 +432,15 @@ pub enum UtimensatFlags { /// /// [utimensat(2)](https://pubs.opengroup.org/onlinepubs/9699919799/functions/utimens.html). #[cfg(not(target_os = "redox"))] -pub fn utimensat( - dirfd: Option, +pub fn utimensat( + dirfd: Fd, path: &P, atime: &TimeSpec, mtime: &TimeSpec, flag: UtimensatFlags, ) -> Result<()> { + use std::os::fd::AsRawFd; + let atflag = match flag { UtimensatFlags::FollowSymlink => AtFlags::empty(), UtimensatFlags::NoFollowSymlink => AtFlags::AT_SYMLINK_NOFOLLOW, @@ -432,7 +448,7 @@ pub fn utimensat( let times: [libc::timespec; 2] = [*atime.as_ref(), *mtime.as_ref()]; let res = path.with_nix_path(|cstr| unsafe { libc::utimensat( - at_rawfd(dirfd), + dirfd.as_fd().as_raw_fd(), cstr.as_ptr(), ×[0], atflag.bits() as libc::c_int, @@ -443,14 +459,19 @@ pub fn utimensat( } #[cfg(not(target_os = "redox"))] -pub fn mkdirat( - fd: Option, +pub fn mkdirat( + fd: Fd, path: &P, mode: Mode, ) -> Result<()> { - let fd = at_rawfd(fd); + use std::os::fd::AsRawFd; + let res = path.with_nix_path(|cstr| unsafe { - libc::mkdirat(fd, cstr.as_ptr(), mode.bits() as mode_t) + libc::mkdirat( + fd.as_fd().as_raw_fd(), + cstr.as_ptr(), + mode.bits() as mode_t, + ) })?; Errno::result(res).map(drop) diff --git a/test/sys/test_stat.rs b/test/sys/test_stat.rs index 2b85578821..be7bc2815e 100644 --- a/test/sys/test_stat.rs +++ b/test/sys/test_stat.rs @@ -5,7 +5,6 @@ use std::fs::File; use std::os::unix::fs::symlink; #[cfg(not(any(target_os = "redox", target_os = "haiku")))] use std::os::unix::fs::PermissionsExt; -use std::os::unix::prelude::AsRawFd; #[cfg(not(target_os = "redox"))] use std::path::Path; #[cfg(not(any(target_os = "redox", target_os = "haiku")))] @@ -102,15 +101,13 @@ fn test_stat_and_fstat() { let stat_result = stat(&filename); assert_stat_results(stat_result); - let fstat_result = fstat(file.as_raw_fd()); + let fstat_result = fstat(&file); assert_stat_results(fstat_result); } #[test] #[cfg(not(any(target_os = "netbsd", target_os = "redox")))] fn test_fstatat() { - use std::os::fd::AsRawFd; - let tempdir = tempfile::tempdir().unwrap(); let filename = tempdir.path().join("foo.txt"); File::create(&filename).unwrap(); @@ -118,11 +115,7 @@ fn test_fstatat() { fcntl::open(tempdir.path(), fcntl::OFlag::empty(), stat::Mode::empty()) .unwrap(); - let result = stat::fstatat( - Some(dirfd.as_raw_fd()), - &filename, - fcntl::AtFlags::empty(), - ); + let result = stat::fstatat(&dirfd, &filename, fcntl::AtFlags::empty()); assert_stat_results(result); } @@ -147,7 +140,7 @@ fn test_stat_fstat_lstat() { let lstat_result = lstat(&linkname); assert_lstat_results(lstat_result); - let fstat_result = fstat(link.as_raw_fd()); + let fstat_result = fstat(&link); assert_stat_results(fstat_result); } @@ -160,14 +153,14 @@ fn test_fchmod() { let mut mode1 = Mode::empty(); mode1.insert(Mode::S_IRUSR); mode1.insert(Mode::S_IWUSR); - fchmod(file.as_raw_fd(), mode1).unwrap(); + fchmod(&file, mode1).unwrap(); let file_stat1 = stat(&filename).unwrap(); assert_eq!(file_stat1.st_mode as mode_t & 0o7777, mode1.bits()); let mut mode2 = Mode::empty(); mode2.insert(Mode::S_IROTH); - fchmod(file.as_raw_fd(), mode2).unwrap(); + fchmod(&file, mode2).unwrap(); let file_stat2 = stat(&filename).unwrap(); assert_eq!(file_stat2.st_mode as mode_t & 0o7777, mode2.bits()); @@ -176,8 +169,6 @@ fn test_fchmod() { #[test] #[cfg(not(target_os = "redox"))] fn test_fchmodat() { - use std::os::fd::AsRawFd; - let _dr = crate::DirRestore::new(); let tempdir = tempfile::tempdir().unwrap(); let filename = "foo.txt"; @@ -191,13 +182,7 @@ fn test_fchmodat() { let mut mode1 = Mode::empty(); mode1.insert(Mode::S_IRUSR); mode1.insert(Mode::S_IWUSR); - fchmodat( - Some(dirfd.as_raw_fd()), - filename, - mode1, - FchmodatFlags::FollowSymlink, - ) - .unwrap(); + fchmodat(&dirfd, filename, mode1, FchmodatFlags::FollowSymlink).unwrap(); let file_stat1 = stat(&fullpath).unwrap(); assert_eq!(file_stat1.st_mode as mode_t & 0o7777, mode1.bits()); @@ -206,7 +191,13 @@ fn test_fchmodat() { let mut mode2 = Mode::empty(); mode2.insert(Mode::S_IROTH); - fchmodat(None, filename, mode2, FchmodatFlags::FollowSymlink).unwrap(); + fchmodat( + fcntl::AT_FDCWD, + filename, + mode2, + FchmodatFlags::FollowSymlink, + ) + .unwrap(); let file_stat2 = stat(&fullpath).unwrap(); assert_eq!(file_stat2.st_mode as mode_t & 0o7777, mode2.bits()); @@ -279,8 +270,6 @@ fn test_lutimes() { #[test] #[cfg(not(any(target_os = "redox", target_os = "haiku")))] fn test_futimens() { - use std::os::fd::AsRawFd; - let tempdir = tempfile::tempdir().unwrap(); let fullpath = tempdir.path().join("file"); drop(File::create(&fullpath).unwrap()); @@ -288,20 +277,13 @@ fn test_futimens() { let fd = fcntl::open(&fullpath, fcntl::OFlag::empty(), stat::Mode::empty()) .unwrap(); - futimens( - fd.as_raw_fd(), - &TimeSpec::seconds(10), - &TimeSpec::seconds(20), - ) - .unwrap(); + futimens(&fd, &TimeSpec::seconds(10), &TimeSpec::seconds(20)).unwrap(); assert_times_eq(10, 20, &fs::metadata(&fullpath).unwrap()); } #[test] #[cfg(not(any(target_os = "redox", target_os = "haiku")))] fn test_utimensat() { - use std::os::fd::AsRawFd; - let _dr = crate::DirRestore::new(); let tempdir = tempfile::tempdir().unwrap(); let filename = "foo.txt"; @@ -313,7 +295,7 @@ fn test_utimensat() { .unwrap(); utimensat( - Some(dirfd.as_raw_fd()), + &dirfd, filename, &TimeSpec::seconds(12345), &TimeSpec::seconds(678), @@ -325,7 +307,7 @@ fn test_utimensat() { chdir(tempdir.path()).unwrap(); utimensat( - None, + fcntl::AT_FDCWD, filename, &TimeSpec::seconds(500), &TimeSpec::seconds(800), @@ -343,16 +325,13 @@ fn test_mkdirat_success_path() { let dirfd = fcntl::open(tempdir.path(), fcntl::OFlag::empty(), stat::Mode::empty()) .unwrap(); - mkdirat(Some(dirfd.as_raw_fd()), filename, Mode::S_IRWXU) - .expect("mkdirat failed"); + mkdirat(&dirfd, filename, Mode::S_IRWXU).expect("mkdirat failed"); assert!(Path::exists(&tempdir.path().join(filename))); } #[test] #[cfg(not(any(target_os = "redox", target_os = "haiku")))] fn test_mkdirat_success_mode() { - use std::os::fd::AsRawFd; - let expected_bits = stat::SFlag::S_IFDIR.bits() | stat::Mode::S_IRWXU.bits(); let tempdir = tempfile::tempdir().unwrap(); @@ -360,8 +339,7 @@ fn test_mkdirat_success_mode() { let dirfd = fcntl::open(tempdir.path(), fcntl::OFlag::empty(), stat::Mode::empty()) .unwrap(); - mkdirat(Some(dirfd.as_raw_fd()), filename, Mode::S_IRWXU) - .expect("mkdirat failed"); + mkdirat(&dirfd, filename, Mode::S_IRWXU).expect("mkdirat failed"); let permissions = fs::metadata(tempdir.path().join(filename)) .unwrap() .permissions(); @@ -372,8 +350,6 @@ fn test_mkdirat_success_mode() { #[test] #[cfg(not(target_os = "redox"))] fn test_mkdirat_fail() { - use std::os::fd::AsRawFd; - let tempdir = tempfile::tempdir().unwrap(); let not_dir_filename = "example_not_dir"; let filename = "example_subdir_dir"; @@ -383,8 +359,7 @@ fn test_mkdirat_fail() { stat::Mode::empty(), ) .unwrap(); - let result = - mkdirat(Some(dirfd.as_raw_fd()), filename, Mode::S_IRWXU).unwrap_err(); + let result = mkdirat(dirfd, filename, Mode::S_IRWXU).unwrap_err(); assert_eq!(result, Errno::ENOTDIR); } @@ -424,21 +399,10 @@ fn test_mknodat() { let tempdir = tempfile::tempdir().unwrap(); let target_dir = Dir::open(tempdir.path(), OFlag::O_DIRECTORY, Mode::S_IRWXU).unwrap(); - mknodat( - Some(target_dir.as_raw_fd()), - file_name, - SFlag::S_IFREG, - Mode::S_IRWXU, - 0, - ) - .unwrap(); - let mode = fstatat( - Some(target_dir.as_raw_fd()), - file_name, - AtFlags::AT_SYMLINK_NOFOLLOW, - ) - .unwrap() - .st_mode as mode_t; + mknodat(&target_dir, file_name, SFlag::S_IFREG, Mode::S_IRWXU, 0).unwrap(); + let mode = fstatat(&target_dir, file_name, AtFlags::AT_SYMLINK_NOFOLLOW) + .unwrap() + .st_mode as mode_t; assert_eq!(mode & libc::S_IFREG, libc::S_IFREG); assert_eq!(mode & libc::S_IRWXU, libc::S_IRWXU); } @@ -446,8 +410,6 @@ fn test_mknodat() { #[test] #[cfg(not(any(target_os = "redox", target_os = "haiku")))] fn test_futimens_unchanged() { - use std::os::fd::AsRawFd; - let tempdir = tempfile::tempdir().unwrap(); let fullpath = tempdir.path().join("file"); drop(File::create(&fullpath).unwrap()); @@ -463,8 +425,7 @@ fn test_futimens_unchanged() { .modified() .unwrap(); - futimens(fd.as_raw_fd(), &TimeSpec::UTIME_OMIT, &TimeSpec::UTIME_OMIT) - .unwrap(); + futimens(&fd, &TimeSpec::UTIME_OMIT, &TimeSpec::UTIME_OMIT).unwrap(); let new_atime = fs::metadata(fullpath.as_path()) .unwrap() @@ -481,8 +442,6 @@ fn test_futimens_unchanged() { #[test] #[cfg(not(any(target_os = "redox", target_os = "haiku")))] fn test_utimensat_unchanged() { - use std::os::fd::AsRawFd; - let _dr = crate::DirRestore::new(); let tempdir = tempfile::tempdir().unwrap(); let filename = "foo.txt"; @@ -501,7 +460,7 @@ fn test_utimensat_unchanged() { .modified() .unwrap(); utimensat( - Some(dirfd.as_raw_fd()), + &dirfd, filename, &TimeSpec::UTIME_OMIT, &TimeSpec::UTIME_OMIT, @@ -529,23 +488,20 @@ fn test_chflags() { sys::stat::{fstat, FileFlag}, unistd::chflags, }; - use std::os::unix::io::AsRawFd; use tempfile::NamedTempFile; let f = NamedTempFile::new().unwrap(); - let initial = FileFlag::from_bits_truncate( - fstat(f.as_raw_fd()).unwrap().st_flags.into(), - ); + let initial = + FileFlag::from_bits_truncate(fstat(&f).unwrap().st_flags.into()); // UF_OFFLINE is preserved by all FreeBSD file systems, but not interpreted // in any way, so it's handy for testing. let commanded = initial ^ FileFlag::UF_OFFLINE; chflags(f.path(), commanded).unwrap(); - let changed = FileFlag::from_bits_truncate( - fstat(f.as_raw_fd()).unwrap().st_flags.into(), - ); + let changed = + FileFlag::from_bits_truncate(fstat(&f).unwrap().st_flags.into()); assert_eq!(commanded, changed); } diff --git a/test/test_fcntl.rs b/test/test_fcntl.rs index 046058496b..efc38efbd9 100644 --- a/test/test_fcntl.rs +++ b/test/test_fcntl.rs @@ -405,7 +405,6 @@ mod linux_android { fn test_ofd_write_lock() { use nix::sys::stat::fstat; use std::mem; - use std::os::fd::AsRawFd; let tmp = NamedTempFile::new().unwrap(); @@ -416,8 +415,7 @@ mod linux_android { // skip the test. skip!("/proc/locks does not work on overlayfs"); } - let inode = - fstat(tmp.as_raw_fd()).expect("fstat failed").st_ino as usize; + let inode = fstat(&tmp).expect("fstat failed").st_ino as usize; let mut flock: libc::flock = unsafe { mem::zeroed() // required for Linux/mips @@ -445,7 +443,6 @@ mod linux_android { fn test_ofd_read_lock() { use nix::sys::stat::fstat; use std::mem; - use std::os::fd::AsRawFd; let tmp = NamedTempFile::new().unwrap(); @@ -456,8 +453,7 @@ mod linux_android { // skip the test. skip!("/proc/locks does not work on overlayfs"); } - let inode = - fstat(tmp.as_raw_fd()).expect("fstat failed").st_ino as usize; + let inode = fstat(&tmp).expect("fstat failed").st_ino as usize; let mut flock: libc::flock = unsafe { mem::zeroed() // required for Linux/mips diff --git a/test/test_unistd.rs b/test/test_unistd.rs index a86a170bac..53d1bc79a0 100644 --- a/test/test_unistd.rs +++ b/test/test_unistd.rs @@ -192,12 +192,8 @@ fn test_mkfifoat() { mkfifoat(Some(dirfd.as_raw_fd()), mkfifoat_name, Mode::S_IRUSR).unwrap(); - let stats = stat::fstatat( - Some(dirfd.as_raw_fd()), - mkfifoat_name, - fcntl::AtFlags::empty(), - ) - .unwrap(); + let stats = + stat::fstatat(&dirfd, mkfifoat_name, fcntl::AtFlags::empty()).unwrap(); let typ = stat::SFlag::from_bits_truncate(stats.st_mode); assert_eq!(typ, SFlag::S_IFIFO); } @@ -231,8 +227,7 @@ fn test_mkfifoat_directory() { let tempdir = tempdir().unwrap(); let dirfd = open(tempdir.path(), OFlag::empty(), Mode::empty()).unwrap(); let mkfifoat_dir = "mkfifoat_dir"; - stat::mkdirat(Some(dirfd.as_raw_fd()), mkfifoat_dir, Mode::S_IRUSR) - .unwrap(); + stat::mkdirat(&dirfd, mkfifoat_dir, Mode::S_IRUSR).unwrap(); mkfifoat(Some(dirfd.as_raw_fd()), mkfifoat_dir, Mode::S_IRUSR) .expect_err("assertion failed"); @@ -742,12 +737,12 @@ fn test_getresgid() { fn test_pipe() { let (fd0, fd1) = pipe().unwrap(); let m0 = stat::SFlag::from_bits_truncate( - stat::fstat(fd0.as_raw_fd()).unwrap().st_mode as mode_t, + stat::fstat(&fd0).unwrap().st_mode as mode_t, ); // S_IFIFO means it's a pipe assert_eq!(m0, SFlag::S_IFIFO); let m1 = stat::SFlag::from_bits_truncate( - stat::fstat(fd1.as_raw_fd()).unwrap().st_mode as mode_t, + stat::fstat(&fd1).unwrap().st_mode as mode_t, ); assert_eq!(m1, SFlag::S_IFIFO); }