merge #72 into openSUSE/libpathrs:main

Aleksa Sarai (8):
  capi: procfs: use open-enum to detect invalid enum values
  bindings: add support for PATHRS_PROC_ROOT
  tests: procfs: run some tests as root
  tests: procfs: add ProcRoot tests
  procfs: add support for ProcfsBase::ProcRoot
  procfs: create temporary handle for non-subset=pid subpaths
  tests: mntns: always set MS_SLAVE for tests
  utils: sysctl: prettify asserts

LGTMs: cyphar

Author: cyphar

CHANGELOG.md

@@ -7,6 +7,20 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ## [Unreleased] ##
 
 ### Added ###
+- procfs: add support for operating on files in the `/proc` root (or other
+  processes) with `ProcfsBase::ProcRoot`.
+
+  While the cached file descriptor shouldn't leak into containers (container
+  runtimes know to set `PR_SET_DUMPABLE`, and our cached file descriptor is
+  `O_CLOEXEC`), I felt a little uncomfortable about having a global unmasked
+  procfs handle sitting around in `libpathrs`. So, in order to avoid making a
+  file descriptor leak by a `libpathrs` user catastrophic, `libpathrs` will
+  always try to use a "limited" procfs handle as the global cached handle
+  (which is much safer to leak into a container) and for operations on
+  `ProcfsBase::ProcRoot`, a temporary new "unrestricted" procfs handle is
+  created just for that operartion. This is more expensive, but it avoids a
+  potential leak turning into a breakout or other nightmare scenario.
+
 - python bindings: The `cffi` build script is now a little easier to use for
   distributions that want to build the python bindings at the same time as the
   main library. After compiling the library, set the `PATHRS_SRC_ROOT`
@@ -28,6 +42,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
   filesystem configurations (POSIX ACLs, weird filesystem-specific mount
   options). (#71)
 
+- capi: Passing invalid `pathrs_proc_base_t` values to `pathrs_proc_*` will now
+  return an error rather than resulting in Undefined Behaviour™.
+
 ## [0.1.0] - 2024-09-14 ##
 
 > 負けたくないことに理由って要る?

Cargo.toml

@@ -37,7 +37,7 @@ maintenance = { status = "experimental" }
 crate-type = ["rlib"]
 
 [features]
-capi = ["dep:rand"]
+capi = ["dep:rand", "dep:open-enum"]
 # Only used for tests.
 _test_as_root = []
 
@@ -53,6 +53,8 @@ itertools = "^0.13"
 lazy_static = "^1"
 libc = "^0.2"
 memchr = "^2"
+# MSRV(1.65): Update to >=0.4.1 which uses let_else. 0.4.0 was broken.
+open-enum = {version = "=0.3.0", optional = true }
 rand = { version = "^0.8", optional = true }
 rustix = { version = "^0.38", features = ["fs"] }
 thiserror = "^1"

contrib/bindings/python/pathrs/_pathrs.py

@@ -220,6 +220,7 @@ def _convert_mode(mode):
 	return flags
 
 
+PROC_ROOT = libpathrs_so.PATHRS_PROC_ROOT
 PROC_SELF = libpathrs_so.PATHRS_PROC_SELF
 PROC_THREAD_SELF = libpathrs_so.PATHRS_PROC_THREAD_SELF
 

go-pathrs/libpathrs_linux.go

@@ -195,6 +195,7 @@ func pathrsHardlink(rootFd uintptr, path, target string) error {
 type pathrsProcBase C.pathrs_proc_base_t
 
 const (
+	pathrsProcRoot       pathrsProcBase = C.PATHRS_PROC_ROOT
 	pathrsProcSelf       pathrsProcBase = C.PATHRS_PROC_SELF
 	pathrsProcThreadSelf pathrsProcBase = C.PATHRS_PROC_THREAD_SELF
 )

go-pathrs/procfs_linux.go

@@ -27,7 +27,10 @@ import (
 type ProcBase int
 
 const (
-	unimplementedProcBaseRoot ProcBase = iota
+	// Use /proc. Note that this mode may be more expensive because we have to
+	// take steps to try to avoid leaking unmasked procfs handles, so you
+	// should use [ProcBaseSelf] if you can.
+	ProcBaseRoot ProcBase = iota
 	// Use /proc/self. For most programs, this is the standard choice.
 	ProcBaseSelf
 	// Use /proc/thread-self. In multi-threaded programs where one thread has
@@ -38,6 +41,8 @@ const (
 
 func (b ProcBase) toPathrsBase() (pathrsProcBase, error) {
 	switch b {
+	case ProcBaseRoot:
+		return pathrsProcRoot, nil
 	case ProcBaseSelf:
 		return pathrsProcSelf, nil
 	case ProcBaseThreadSelf:
@@ -47,36 +52,65 @@ func (b ProcBase) toPathrsBase() (pathrsProcBase, error) {
 	}
 }
 
+func (b ProcBase) namePrefix() string {
+	switch b {
+	case ProcBaseRoot:
+		return "/proc/"
+	case ProcBaseSelf:
+		return "/proc/self/"
+	case ProcBaseThreadSelf:
+		return "/proc/thread-self/"
+	default:
+		return "<invalid procfs base>/"
+	}
+}
+
 // ProcHandleCloser is a callback that needs to be called when you are done
-// operating on an *os.File fetched using ProcThreadSelfOpen.
+// operating on an *os.File fetched using [ProcThreadSelfOpen].
 type ProcHandleCloser func()
 
-// TODO: Consider exporting procOpen once we have ProcBaseRoot.
-
+// TODO: Should we expose procOpen?
 func procOpen(base ProcBase, path string, flags int) (*os.File, ProcHandleCloser, error) {
 	pathrsBase, err := base.toPathrsBase()
 	if err != nil {
 		return nil, nil, err
 	}
+	namePrefix := base.namePrefix()
+
 	switch base {
-	case ProcBaseSelf:
+	case ProcBaseRoot, ProcBaseSelf:
 		fd, err := pathrsProcOpen(pathrsBase, path, flags)
 		if err != nil {
 			return nil, nil, err
 		}
-		return os.NewFile(fd, "/proc/self/"+path), nil, nil
+		return os.NewFile(fd, namePrefix+path), nil, nil
 	case ProcBaseThreadSelf:
 		runtime.LockOSThread()
 		fd, err := pathrsProcOpen(pathrsBase, path, flags)
 		if err != nil {
 			runtime.UnlockOSThread()
 			return nil, nil, err
 		}
-		return os.NewFile(fd, "/proc/thread-self/"+path), runtime.UnlockOSThread, nil
+		return os.NewFile(fd, namePrefix+path), runtime.UnlockOSThread, nil
 	}
 	panic("unreachable")
 }
 
+// ProcRootOpen safely opens a given path from inside /proc/.
+//
+// This function must only be used for accessing global information from procfs
+// (such as /proc/cpuinfo) or information about other processes (such as
+// /proc/1). Accessing your own process information should be done using
+// [ProcSelfOpen] or [ProcThreadSelfOpen].
+func ProcRootOpen(path string, flags int) (*os.File, error) {
+	file, closer, err := procOpen(ProcBaseRoot, path, flags)
+	if closer != nil {
+		// should not happen
+		panic("non-zero closer returned from procOpen(ProcBaseRoot)")
+	}
+	return file, err
+}
+
 // ProcSelfOpen safely opens a given path from inside /proc/self/.
 //
 // This method is recommend for getting process information about the current
@@ -86,13 +120,14 @@ func procOpen(base ProcBase, path string, flags int) (*os.File, ProcHandleCloser
 //
 // For such non-heterogeneous processes, /proc/self may reference to a task
 // that has different state from the current goroutine and so it may be
-// preferable to use ProcThreadSelfOpen. The same is true if a user really
+// preferable to use [ProcThreadSelfOpen]. The same is true if a user really
 // wants to inspect the current OS thread's information (such as
 // /proc/thread-self/stack or /proc/thread-self/status which is always uniquely
 // per-thread).
 //
-// Unlike ProcThreadSelfOpen, this method does not involve locking the
-// goroutine to the current OS thread and so is simpler to use.
+// Unlike [ProcThreadSelfOpen], this method does not involve locking the
+// goroutine to the current OS thread and so is simpler to use and
+// theoretically has slightly less overhead.
 func ProcSelfOpen(path string, flags int) (*os.File, error) {
 	file, closer, err := procOpen(ProcBaseSelf, path, flags)
 	if closer != nil {
@@ -105,7 +140,7 @@ func ProcSelfOpen(path string, flags int) (*os.File, error) {
 // ProcThreadSelfOpen safely opens a given path from inside /proc/thread-self/.
 //
 // Most Go processes have heterogeneous threads (all threads have most of the
-// same kernel state such as CLONE_FS) and so ProcSelfOpen is preferable for
+// same kernel state such as CLONE_FS) and so [ProcSelfOpen] is preferable for
 // most users.
 //
 // For non-heterogeneous threads, or users that actually want thread-specific
@@ -129,7 +164,7 @@ func ProcThreadSelfOpen(path string, flags int) (*os.File, ProcHandleCloser, err
 //
 // This is effectively equivalent to doing a Proc*Open(O_PATH|O_NOFOLLOW) of
 // the path and then doing unix.Readlinkat(fd, ""), but with the benefit that
-// thread locking is not necessary for ProcBaseThreadSelf.
+// thread locking is not necessary for [ProcBaseThreadSelf].
 func ProcReadlink(base ProcBase, path string) (string, error) {
 	pathrsBase, err := base.toPathrsBase()
 	if err != nil {

include/pathrs.h

@@ -43,10 +43,14 @@
  * pathrs_proc_*. This is necessary because /proc/thread-self is not present on
  * pre-3.17 kernels and so it may be necessary to emulate /proc/thread-self
  * access on those older kernels.
- *
- * NOTE: Currently, operating on /proc/... directly is not supported.
  */
-typedef enum {
+enum pathrs_proc_base_t {
+    /**
+     * Use `/proc`. Note that this mode may be more expensive because we have
+     * to take steps to try to avoid leaking unmasked procfs handles, so you
+     * should use `PATHRS_PROC_SELF` if you can.
+     */
+    PATHRS_PROC_ROOT = 1342308351,
     /**
      * Use /proc/self. For most programs, this is the standard choice.
      */
@@ -61,7 +65,8 @@ typedef enum {
      * be killed (such as Go -- where you want to use runtime.LockOSThread).
      */
     PATHRS_PROC_THREAD_SELF = 1051549215,
-} pathrs_proc_base_t;
+};
+typedef uint64_t pathrs_proc_base_t;
 
 /**
  * Attempts to represent a Rust Error type in C. This structure must be freed

src/capi/procfs.rs

@@ -19,26 +19,33 @@
 
 use crate::{
     capi::{ret::IntoCReturn, utils},
-    error::Error,
+    error::{Error, ErrorImpl},
     flags::OpenFlags,
     procfs::{ProcfsBase, GLOBAL_PROCFS_HANDLE},
 };
 
-use std::os::unix::io::{OwnedFd, RawFd};
+use std::{
+    convert::TryFrom,
+    os::unix::io::{OwnedFd, RawFd},
+};
 
 use libc::{c_char, c_int, size_t};
+use open_enum::open_enum;
 
 /// Indicate what base directory should be used when doing operations with
 /// pathrs_proc_*. This is necessary because /proc/thread-self is not present on
 /// pre-3.17 kernels and so it may be necessary to emulate /proc/thread-self
 /// access on those older kernels.
-///
-/// NOTE: Currently, operating on /proc/... directly is not supported.
-#[repr(C)]
+#[open_enum]
+#[repr(u64)]
 #[derive(Debug, PartialEq, Eq, Clone, Copy)]
 #[allow(non_camel_case_types, dead_code)]
 pub enum CProcfsBase {
-    //PATHRS_PROC_ROOT, // TODO
+    /// Use `/proc`. Note that this mode may be more expensive because we have
+    /// to take steps to try to avoid leaking unmasked procfs handles, so you
+    /// should use `PATHRS_PROC_SELF` if you can.
+    PATHRS_PROC_ROOT = 0x5001_FFFF,
+
     /// Use /proc/self. For most programs, this is the standard choice.
     PATHRS_PROC_SELF = 0x091D_5E1F,
 
@@ -52,11 +59,19 @@ pub enum CProcfsBase {
     PATHRS_PROC_THREAD_SELF = 0x3EAD_5E1F,
 }
 
-impl From<CProcfsBase> for ProcfsBase {
-    fn from(c_base: CProcfsBase) -> Self {
+impl TryFrom<CProcfsBase> for ProcfsBase {
+    type Error = Error;
+
+    fn try_from(c_base: CProcfsBase) -> Result<Self, Self::Error> {
         match c_base {
-            CProcfsBase::PATHRS_PROC_SELF => ProcfsBase::ProcSelf,
-            CProcfsBase::PATHRS_PROC_THREAD_SELF => ProcfsBase::ProcThreadSelf,
+            CProcfsBase::PATHRS_PROC_ROOT => Ok(ProcfsBase::ProcRoot),
+            CProcfsBase::PATHRS_PROC_SELF => Ok(ProcfsBase::ProcSelf),
+            CProcfsBase::PATHRS_PROC_THREAD_SELF => Ok(ProcfsBase::ProcThreadSelf),
+            _ => Err(ErrorImpl::InvalidArgument {
+                name: "procfs base".into(),
+                description: "the procfs base must be one of the PATHRS_PROC_* values".into(),
+            }
+            .into()),
         }
     }
 }
@@ -65,6 +80,7 @@ impl From<CProcfsBase> for ProcfsBase {
 impl From<ProcfsBase> for CProcfsBase {
     fn from(base: ProcfsBase) -> Self {
         match base {
+            ProcfsBase::ProcRoot => CProcfsBase::PATHRS_PROC_ROOT,
             ProcfsBase::ProcSelf => CProcfsBase::PATHRS_PROC_SELF,
             ProcfsBase::ProcThreadSelf => CProcfsBase::PATHRS_PROC_THREAD_SELF,
         }
@@ -114,12 +130,13 @@ pub unsafe extern "C" fn pathrs_proc_open(
     flags: c_int,
 ) -> RawFd {
     || -> Result<_, Error> {
+        let base = base.try_into()?;
         let path = unsafe { utils::parse_path(path) }?; // SAFETY: C caller guarantees path is safe.
         let oflags = OpenFlags::from_bits_retain(flags);
 
         match oflags.contains(OpenFlags::O_NOFOLLOW) {
-            true => GLOBAL_PROCFS_HANDLE.open(base.into(), path, oflags),
-            false => GLOBAL_PROCFS_HANDLE.open_follow(base.into(), path, oflags),
+            true => GLOBAL_PROCFS_HANDLE.open(base, path, oflags),
+            false => GLOBAL_PROCFS_HANDLE.open_follow(base, path, oflags),
         }
     }()
     .map(OwnedFd::from)
@@ -170,8 +187,9 @@ pub unsafe extern "C" fn pathrs_proc_readlink(
     linkbuf_size: size_t,
 ) -> c_int {
     || -> Result<_, Error> {
+        let base = base.try_into()?;
         let path = unsafe { utils::parse_path(path) }?; // SAFETY: C caller guarantees path is safe.
-        let link_target = GLOBAL_PROCFS_HANDLE.readlink(base.into(), path)?;
+        let link_target = GLOBAL_PROCFS_HANDLE.readlink(base, path)?;
         // SAFETY: C caller guarantees buffer is at least linkbuf_size and can
         // be written to.
         unsafe { utils::copy_path_into_buffer(link_target, linkbuf, linkbuf_size) }