merge #130 into openSUSE/libpathrs:main

Aleksa Sarai (4):
  errors: document and export ErrorKind
  errors: hide internal error types from ErrorKind
  capi: error: map pure-Rust errors into equivalent errno values
  tests: unify error checking for operations

LGTMs: cyphar

CHANGELOG.md

@@ -30,6 +30,15 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
   efficient in some scenarios (especially with the openat2-based resolver or
   for C FFI users where function calls are expensive) as it saves one file
   descriptor allocation and extra function calls.
+- Error: `ErrorKind` is now exported, allowing you to programmatically handle
+  errors returned by libpathrs. This interface may change in the future (in
+  particular, `ErrorKind::OsError` might change its representation of `errno`
+  values).
+- capi: errors that are returned by libpathrs itself (such as invalid arguments
+  being passed by users) will now contain a `saved_errno` value that makes
+  sense for the error type (so `ErrorKind::InvalidArgument` will result in an
+  `EINVAL` value for `saved_errno`). This will allow C users to have a nicer
+  time handling errors programmatically.
 
 ### Fixes ###
 - multiarch: we now build correctly on 32-bit architectures as well as

src/capi/error.rs

@@ -19,7 +19,7 @@
 
 use crate::{
     capi::{ret::CReturn, utils::Leakable},
-    error::{Error, ErrorKind},
+    error::Error,
 };
 
 use std::{
@@ -100,16 +100,13 @@ impl From<&Error> for CError {
             CString::new(desc).expect("CString::new(description) failed in CError generation")
         };
 
-        // TODO: We should probably convert some of our internal errors into
-        //       equivalent POSIX-style errors (InvalidArgument => -EINVAL, for
-        //       instance).
-        let errno = match err.kind() {
-            ErrorKind::OsError(Some(err)) => err.abs(),
-            _ => 0,
-        };
+        // Map the error to a C errno if possible.
+        // TODO: We might want to use ESERVERFAULT (An untranslatable error
+        // occurred) for untranslatable errors?
+        let saved_errno = err.kind().errno().unwrap_or(0).unsigned_abs();
 
         CError {
-            saved_errno: errno.try_into().unwrap_or(0),
+            saved_errno: saved_errno.into(),
             description: desc.into_raw(),
         }
     }
@@ -186,3 +183,94 @@ pub unsafe extern "C" fn pathrs_errorinfo_free(ptr: *mut CError) {
     // and that this isn't a double-free.
     unsafe { (*ptr).free() }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::error::{Error, ErrorImpl};
+
+    use std::io::Error as IOError;
+
+    use pretty_assertions::assert_eq;
+
+    #[test]
+    fn cerror_ioerror_errno() {
+        let err = Error::from(ErrorImpl::OsError {
+            operation: "fake operation".into(),
+            source: IOError::from_raw_os_error(libc::ENOANO),
+        });
+
+        assert_eq!(
+            err.kind().errno(),
+            Some(libc::ENOANO),
+            "basic kind().errno() should return the right error"
+        );
+
+        let cerr = CError::from(&err);
+        assert_eq!(
+            cerr.saved_errno,
+            libc::ENOANO as u64,
+            "cerror should contain errno for OsError"
+        );
+    }
+
+    #[test]
+    fn cerror_einval_errno() {
+        let err = Error::from(ErrorImpl::InvalidArgument {
+            name: "fake argument".into(),
+            description: "fake description".into(),
+        });
+
+        assert_eq!(
+            err.kind().errno(),
+            Some(libc::EINVAL),
+            "InvalidArgument kind().errno() should return the right error"
+        );
+
+        let cerr = CError::from(&err);
+        assert_eq!(
+            cerr.saved_errno,
+            libc::EINVAL as u64,
+            "cerror should contain EINVAL errno for InvalidArgument"
+        );
+    }
+
+    #[test]
+    fn cerror_enosys_errno() {
+        let err = Error::from(ErrorImpl::NotImplemented {
+            feature: "fake feature".into(),
+        });
+
+        assert_eq!(
+            err.kind().errno(),
+            Some(libc::ENOSYS),
+            "NotImplemented kind().errno() should return the right error"
+        );
+
+        let cerr = CError::from(&err);
+        assert_eq!(
+            cerr.saved_errno,
+            libc::ENOSYS as u64,
+            "cerror should contain ENOSYS errno for NotImplemented"
+        );
+    }
+
+    #[test]
+    fn cerror_no_errno() {
+        let err = Error::from(ErrorImpl::SafetyViolation {
+            description: "fake safety violation".into(),
+        });
+
+        assert_eq!(
+            err.kind().errno(),
+            None,
+            "SafetyViolation kind().errno() should return the no errno"
+        );
+
+        let cerr = CError::from(&err);
+        assert_eq!(
+            cerr.saved_errno, 0,
+            "cerror should contain zero errno for SafetyViolation"
+        );
+    }
+}

src/error.rs

@@ -36,6 +36,10 @@ use std::{borrow::Cow, io::Error as IOError};
 //       export the crate types here without std::backtrace::Backtrace.
 // MSRV(1.65): Use std::backtrace::Backtrace.
 
+/// Opaque error type for libpathrs.
+///
+/// If you wish to do non-trivial error handling with libpathrs errors, use
+/// [`Error::kind`] to get an [`ErrorKind`] you can handle programmatically.
 #[derive(thiserror::Error, Debug)]
 #[error(transparent)]
 pub struct Error(#[from] Box<ErrorImpl>);
@@ -49,7 +53,7 @@ impl<E: Into<ErrorImpl>> From<E> for Error {
 }
 
 impl Error {
-    pub(crate) fn kind(&self) -> ErrorKind {
+    pub fn kind(&self) -> ErrorKind {
         self.0.kind()
     }
 }
@@ -100,16 +104,28 @@ pub(crate) enum ErrorImpl {
     },
 }
 
-// TODO: Export this?
+/// Underlying error class for libpathrs errors.
+///
+/// This is similar in concept to [`std::io::ErrorKind`]. Note that the
 #[derive(Debug, PartialEq, Eq, Clone, Copy)]
 #[non_exhaustive]
-pub(crate) enum ErrorKind {
+pub enum ErrorKind {
+    /// The requested feature is not implemented in libpathrs.
     NotImplemented,
+    /// The requested feature is not supported by the system.
     NotSupported,
+    /// The provided arguments to libpathrs were invalid.
     InvalidArgument,
+    /// libpaths encountered a state where the safety of the operation could not
+    /// be guaranteeed. This is usually the result of an attack by a malicious
+    /// program.
     SafetyViolation,
-    BadSymlinkStack,
-    ParseError,
+    /// Some internal error occurred. For more information, see the string
+    /// description of the original [`Error`].
+    InternalError,
+    /// The underlying error came from a system call. The provided
+    /// [`std::io::RawOsError`] is the numerical value of the `errno` number, if
+    /// available.
     // TODO: We might want to use Option<std::io::ErrorKind>?
     OsError(Option<i32>),
 }
@@ -121,17 +137,39 @@ impl ErrorImpl {
             Self::NotSupported { .. } => ErrorKind::NotSupported,
             Self::InvalidArgument { .. } => ErrorKind::InvalidArgument,
             Self::SafetyViolation { .. } => ErrorKind::SafetyViolation,
-            Self::BadSymlinkStackError { .. } => ErrorKind::BadSymlinkStack,
-            Self::ParseIntError(_) => ErrorKind::ParseError,
+            // Any syscall-related errors get mapped to an OsError, since the
+            // distinction doesn't matter to users checking error values.
             Self::OsError { source, .. } => ErrorKind::OsError(source.raw_os_error()),
             Self::RawOsError { source, .. } => {
                 ErrorKind::OsError(source.root_cause().raw_os_error())
             }
+            // These errors are internal error types that we don't want to
+            // expose outside of the crate. All that matters to users is that
+            // there was some internal error.
+            Self::BadSymlinkStackError { .. } | Self::ParseIntError(_) => ErrorKind::InternalError,
             Self::Wrapped { source, .. } => source.kind(),
         }
     }
 }
 
+impl ErrorKind {
+    /// Return a C-like errno for the [`ErrorKind`].
+    ///
+    /// Aside from fetching the errno represented by standard
+    /// [`ErrorKind::OsError`] errors, pure-Rust errors are also mapped to C
+    /// errno values where appropriate.
+    #[cfg(any(feature = "capi", test))]
+    pub(crate) fn errno(&self) -> Option<i32> {
+        match self {
+            ErrorKind::NotImplemented => Some(libc::ENOSYS),
+            ErrorKind::InvalidArgument => Some(libc::EINVAL),
+            ErrorKind::OsError(errno) => *errno,
+            // TODO: Should we remap SafetyViolation?
+            _ => None,
+        }
+    }
+}
+
 // Private trait necessary to work around the "orphan trait" restriction.
 pub(crate) trait ErrorExt: Sized {
     /// Wrap a `Result<..., Error>` with an additional context string.
@@ -175,3 +213,29 @@ impl<T, E: ErrorExt> ErrorExt for Result<T, E> {
         self.map_err(|err| err.with_wrap(context_fn))
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    use pretty_assertions::assert_eq;
+
+    #[test]
+    fn error_kind_errno() {
+        assert_eq!(
+            ErrorKind::InvalidArgument.errno(),
+            Some(libc::EINVAL),
+            "ErrorKind::InvalidArgument is equivalent to EINVAL"
+        );
+        assert_eq!(
+            ErrorKind::NotImplemented.errno(),
+            Some(libc::ENOSYS),
+            "ErrorKind::NotImplemented is equivalent to ENOSYS"
+        );
+        assert_eq!(
+            ErrorKind::OsError(Some(libc::ENOANO)).errno(),
+            Some(libc::ENOANO),
+            "ErrorKind::OsError(...)::errno() returns the inner errno"
+        );
+    }
+}

src/tests.rs

@@ -26,6 +26,9 @@ pub(crate) mod common {
 
     mod handle;
     pub(in crate::tests) use handle::*;
+
+    mod error;
+    pub(in crate::tests) use error::*;
 }
 
 #[cfg(feature = "capi")]

src/tests/capi/utils.rs

@@ -69,9 +69,10 @@ impl ErrorImpl for CapiError {
         if let Some(errno) = self.errno {
             ErrorKind::OsError(Some(errno.0))
         } else {
-            // TODO TODO: We should probably expose the libpathrs internal
-            // ErrorKind types in the C API somehow?
-            ErrorKind::InvalidArgument
+            // TODO: We should probably have an actual "no-op" error here that
+            //       is unused except for these tests so we can properly detect
+            //       a bad ErrorKind.
+            ErrorKind::InternalError
         }
     }
 }

src/tests/common/error.rs

@@ -0,0 +1,53 @@
+/*
+ * libpathrs: safe path resolution on Linux
+ * Copyright (C) 2019-2024 Aleksa Sarai <[email protected]>
+ * 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 <https://www.gnu.org/licenses/>.
+ */
+
+use crate::{error::ErrorKind, tests::traits::ErrorImpl};
+
+use std::fmt::Debug;
+
+use anyhow::Error;
+
+pub(in crate::tests) fn check_err<T1, Err1, T2>(
+    result: &Result<T1, Err1>,
+    expected: &Result<T2, ErrorKind>,
+) -> Result<(), Error>
+where
+    T1: Debug,
+    Err1: ErrorImpl,
+    T2: Debug,
+{
+    let result = result.as_ref();
+    let expected = expected.as_ref();
+
+    match (result, expected) {
+        (Err(error), Err(expected_kind)) => {
+            let kind = error.kind();
+            let (result_errno, expected_errno) = (kind.errno(), expected_kind.errno());
+
+            if kind != *expected_kind && result_errno != expected_errno {
+                anyhow::bail!(
+                    "expected error {expected_kind:?} (errno: {expected_errno:?}) but got {error:?} (kind: {kind:?}, errno: {result_errno:?})"
+                );
+            }
+        }
+        (Ok(_), Ok(_)) => (),
+        (result, expected) => anyhow::bail!("expected {expected:?} but got {result:?}"),
+    }
+    Ok(())
+}