From 5d291eaa92b2b3e77e7b3779df1de31b7d3ac14b Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Wed, 24 Jul 2024 04:18:28 +1000 Subject: [PATCH] OpenFlags: switch to bitflags It makes far more sense to expose O_* flags as bitflags. We could re-export nix::OFlags (when we switch to it) but since we want to have special handling for O_ACCMODE, it makes more sense to have our own type anyway. Signed-off-by: Aleksa Sarai --- src/capi/core.rs | 6 +-- src/flags.rs | 84 +++++++++++++++++++++++++++++++++++++++ src/handle.rs | 50 +---------------------- src/lib.rs | 9 ++++- src/root.rs | 11 ++--- src/tests/test_resolve.rs | 16 ++++---- src/utils.rs | 13 ++++-- 7 files changed, 118 insertions(+), 71 deletions(-) create mode 100644 src/flags.rs diff --git a/src/capi/core.rs b/src/capi/core.rs index c2086fc7..80573f9b 100644 --- a/src/capi/core.rs +++ b/src/capi/core.rs @@ -98,13 +98,13 @@ pub extern "C" fn pathrs_root_open(path: *const c_char) -> RawFd { /// pathrs_errorinfo(). #[no_mangle] pub extern "C" fn pathrs_reopen(fd: RawFd, flags: c_int) -> RawFd { - let flags = OpenFlags(flags); + let flags = OpenFlags::from_bits_retain(flags); fd.reopen(flags) .and_then(|file| { // Rust sets O_CLOEXEC by default, without an opt-out. We need to // disable it if we weren't asked to do O_CLOEXEC. - if flags.0 & libc::O_CLOEXEC == 0 { + if !flags.contains(OpenFlags::O_CLOEXEC) { syscalls::fcntl_unset_cloexec(file.as_raw_fd()).context(error::RawOsSnafu { operation: "clear O_CLOEXEC on fd", })?; @@ -198,7 +198,7 @@ pub extern "C" fn pathrs_creat( ret::with_fd(root_fd, |root: &mut Root| { let mode = mode & !libc::S_IFMT; let perm = Permissions::from_mode(mode); - root.create_file(parse_path(path)?, OpenFlags(flags), &perm) + root.create_file(parse_path(path)?, OpenFlags::from_bits_retain(flags), &perm) }) } diff --git a/src/flags.rs b/src/flags.rs new file mode 100644 index 00000000..fddf9722 --- /dev/null +++ b/src/flags.rs @@ -0,0 +1,84 @@ +/* + * libpathrs: safe path resolution on Linux + * Copyright (C) 2019-2024 Aleksa Sarai + * Copyright (C) 2019-2024 SUSE LLC + * + * This program is free software: you can redistribute it and/or modify it under + * the terms of the GNU Lesser General Public License as published by the Free + * Software Foundation, either version 3 of the License, or (at your option) any + * later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT ANY + * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A + * PARTICULAR PURPOSE. See the GNU General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License along + * with this program. If not, see . + */ + +bitflags! { + /// Wrapper for the underlying `libc`'s `O_*` flags. + /// + /// The flag values and their meaning is identical to the description in the + /// `open(2)` man page. + /// + /// # Caveats + /// + /// For historical reasons, the first three bits of `open(2)`'s flags are for + /// the access mode and are actually treated as a 2-bit number. So, it is + /// incorrect to attempt to do any checks on the access mode without masking it + /// correctly. So some helpers were added to make usage more ergonomic. + #[derive(Default, PartialEq, Eq, PartialOrd, Ord, Hash, Debug, Clone, Copy)] + pub struct OpenFlags: libc::c_int { + const O_EXCL = libc::O_EXCL; + const O_PATH = libc::O_PATH; + const O_RDWR = libc::O_RDWR; + const O_SYNC = libc::O_SYNC; + const O_ASYNC = libc::O_ASYNC; + const O_CREAT = libc::O_CREAT; + const O_DSYNC = libc::O_DSYNC; + const O_FSYNC = libc::O_FSYNC; + const O_RSYNC = libc::O_RSYNC; + const O_TRUNC = libc::O_TRUNC; + const O_APPEND = libc::O_APPEND; + const O_DIRECT = libc::O_DIRECT; + const O_NDELAY = libc::O_NDELAY; + const O_NOCTTY = libc::O_NOCTTY; + const O_RDONLY = libc::O_RDONLY; + const O_WRONLY = libc::O_WRONLY; + const O_ACCMODE = libc::O_ACCMODE; + const O_CLOEXEC = libc::O_CLOEXEC; + const O_NOATIME = libc::O_NOATIME; + const O_TMPFILE = libc::O_TMPFILE; + const O_NOFOLLOW = libc::O_NOFOLLOW; + const O_NONBLOCK = libc::O_NONBLOCK; + const O_DIRECTORY = libc::O_DIRECTORY; + //const O_LARGEFILE = libc::O_LARGEFILE; + const _ = !0; + } +} + +impl OpenFlags { + /// Grab the access mode bits from the flags. + #[inline] + pub fn access_mode(self) -> libc::c_int { + self.bits() & libc::O_ACCMODE + } + + /// Does the access mode imply read access? + #[inline] + pub fn wants_read(self) -> bool { + let acc = self.access_mode(); + acc == libc::O_RDONLY || acc == libc::O_RDWR + } + + /// Does the access mode imply write access? Note that there are several + /// other bits (such as `O_TRUNC`) which imply write access but are not part + /// of the access mode, and thus a `false` value from `.wants_write()` does + /// not guarantee that the kernel will not do a `MAY_WRITE` check. + #[inline] + pub fn wants_write(self) -> bool { + let acc = self.access_mode(); + acc == libc::O_WRONLY || acc == libc::O_RDWR + } +} diff --git a/src/handle.rs b/src/handle.rs index 9389bb01..381e2b0d 100644 --- a/src/handle.rs +++ b/src/handle.rs @@ -18,12 +18,10 @@ #![forbid(unsafe_code)] -use crate::{error::Error, utils::RawFdExt}; +use crate::{error::Error, flags::OpenFlags, utils::RawFdExt}; use std::fs::File; -use libc::c_int; - /// A handle to an existing inode within a [`Root`]. /// /// This handle references an already-resolved path which can be used for the @@ -47,52 +45,6 @@ pub struct Handle { inner: File, } -/// Wrapper for the underlying `libc`'s `O_*` flags. -/// -/// The flag values and their meaning is identical to the description in the -/// `open(2)` man page. -/// -/// # Caveats -/// -/// For historical reasons, the first three bits of `open(2)`'s flags are for -/// the access mode and are actually treated as a 2-bit number. So, it is -/// incorrect to attempt to do any checks on the access mode without masking it -/// correctly. So some helpers were added to make usage more ergonomic. -// TODO: Should probably be a u64, and use a constructor... -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -pub struct OpenFlags(pub c_int); - -impl From for OpenFlags { - fn from(flags: c_int) -> Self { - Self(flags) - } -} - -impl OpenFlags { - /// Grab the access mode bits from the flags. - #[inline] - pub fn access_mode(self) -> c_int { - self.0 & libc::O_ACCMODE - } - - /// Does the access mode imply read access? - #[inline] - pub fn wants_read(self) -> bool { - let acc = self.access_mode(); - acc == libc::O_RDONLY || acc == libc::O_RDWR - } - - /// Does the access mode imply write access? Note that there are several - /// other bits (such as `O_TRUNC`) which imply write access but are not part - /// of the access mode, and thus a `false` value from `.wants_write()` does - /// not guarantee that the kernel will not do a `MAY_WRITE` check. - #[inline] - pub fn wants_write(self) -> bool { - let acc = self.access_mode(); - acc == libc::O_WRONLY || acc == libc::O_RDWR - } -} - impl Handle { /// "Upgrade" the handle to a usable [`File`] handle. /// diff --git a/src/lib.rs b/src/lib.rs index 8c0a2f43..0e43807d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -57,11 +57,11 @@ //! // Resolve the path. //! let handle = root.resolve(unsafe_path)?; //! // Upgrade the handle to a full std::fs::File. -//! let file = handle.reopen(libc::O_RDONLY)?; +//! let file = handle.reopen(OpenFlags::O_RDONLY)?; //! //! // Or, in one line: //! let file = root.resolve(unsafe_path)? -//! .reopen(libc::O_RDONLY)?; +//! .reopen(OpenFlags::O_RDONLY)?; //! # Ok(()) //! # } //! ``` @@ -127,6 +127,11 @@ extern crate libc; #[macro_use] extern crate snafu; +// `OpenFlags` +pub mod flags; +#[doc(inline)] +pub use flags::OpenFlags; + // `Handle` implementation. mod handle; #[doc(inline)] diff --git a/src/root.rs b/src/root.rs index 8658ac18..45f0d02f 100644 --- a/src/root.rs +++ b/src/root.rs @@ -390,7 +390,7 @@ impl Root { pub fn create_file>( &self, path: P, - flags: OpenFlags, + mut flags: OpenFlags, perm: &Permissions, ) -> Result { // Get a handle for the lexical parent of the target path. It must @@ -407,11 +407,12 @@ impl Root { // XXX: openat2(2) supports doing O_CREAT on trailing symlinks without // O_NOFOLLOW. We might want to expose that here, though because it // can't be done with the emulated backend that might be a bad idea. - let flags = flags.0 | libc::O_CREAT; - let file = - syscalls::openat(dirfd, name, flags, perm.mode()).context(error::RawOsSnafu { + flags.insert(OpenFlags::O_CREAT); + let file = syscalls::openat(dirfd, name, flags.bits(), perm.mode()).context( + error::RawOsSnafu { operation: "pathrs create_file", - })?; + }, + )?; Ok(Handle::from_file_unchecked(file)) } diff --git a/src/tests/test_resolve.rs b/src/tests/test_resolve.rs index 4a2c6f5d..ea2c02de 100644 --- a/src/tests/test_resolve.rs +++ b/src/tests/test_resolve.rs @@ -281,9 +281,9 @@ mod utils { format!("{:?} ({})", err, Errno(err)) } - pub(super) fn check_reopen>( + pub(super) fn check_reopen( handle: &Handle, - flags: F, + flags: OpenFlags, expected_error: Option, ) -> Result<(), Error> { let file = match (handle.reopen(flags), expected_error) { @@ -390,18 +390,18 @@ mod utils { match real_file_type { libc::S_IFDIR => { - check_reopen(&handle, libc::O_RDONLY, None)?; - check_reopen(&handle, libc::O_DIRECTORY, None)?; + check_reopen(&handle, OpenFlags::O_RDONLY, None)?; + check_reopen(&handle, OpenFlags::O_DIRECTORY, None)?; } libc::S_IFREG => { - check_reopen(&handle, libc::O_RDWR, None)?; - check_reopen(&handle, libc::O_DIRECTORY, Some(libc::ENOTDIR))?; + check_reopen(&handle, OpenFlags::O_RDWR, None)?; + check_reopen(&handle, OpenFlags::O_DIRECTORY, Some(libc::ENOTDIR))?; } _ => { - check_reopen(&handle, libc::O_PATH, None)?; + check_reopen(&handle, OpenFlags::O_PATH, None)?; check_reopen( &handle, - libc::O_PATH | libc::O_DIRECTORY, + OpenFlags::O_PATH | OpenFlags::O_DIRECTORY, Some(libc::ENOTDIR), )?; } diff --git a/src/utils.rs b/src/utils.rs index 3512c8bd..56098638 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -181,10 +181,15 @@ impl RawFdExt for RawFd { // TODO: We should look into using O_EMPTYPATH if it's available to // avoid the /proc dependency -- though then again, as_unsafe_path // necessarily requires /proc. - syscalls::openat_follow(PROCFS_HANDLE.as_raw_fd(), proc_subpath(*self)?, flags.0, 0) - .context(error::RawOsSnafu { - operation: "reopen fd through procfs", - }) + syscalls::openat_follow( + PROCFS_HANDLE.as_raw_fd(), + proc_subpath(*self)?, + flags.bits(), + 0, + ) + .context(error::RawOsSnafu { + operation: "reopen fd through procfs", + }) } fn as_unsafe_path(&self) -> Result {