Replace enumflags2 with a macro we define ourself

[bjorn3]

This avoid a proc macro, makes the api more ergonomic by removing the need for the BitFlags wrapper type, makes it impossible to construct an Access* value with invalid bits, avoids exposing all() to the user and makes it possible for us to define the exact api we want to expose to the user.

Signed-off-by: Björn Roy Baron bjorn3_gh@protonmail.com

[bjorn3]

By removing thiserror I can reduce the compile time to ~2s with most of the time taken by compiling libc (~1.5s). It does require manually writing a lot of boilerplate previously written by thiserror though, so I'm not sure if it is worth removing thiserror. In any case the diff required is this:

diff --git a/Cargo.toml b/Cargo.toml
index 22d98db..966cddd 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -17,11 +17,9 @@ readme = "README.md"
 [dependencies]
 bitflags = "1.3.2"
 libc = "0.2.133"
-thiserror = "1.0"
 
 [dev-dependencies]
 anyhow = "1.0"
-landlock = { path = "." }
 lazy_static = "1"
 strum = "0.24"
 strum_macros = "0.24"
diff --git a/src/errors.rs b/src/errors.rs
index 3047c3b..56f8da5 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -1,20 +1,62 @@
 use crate::{Access, AccessFs};
-use std::io;
+use std::error::Error;
 use std::path::PathBuf;
-use thiserror::Error;
+use std::{fmt, io};
 
 /// Maps to all errors that can be returned by a ruleset action.
-#[derive(Debug, Error)]
+#[derive(Debug)]
 #[non_exhaustive]
 pub enum RulesetError {
-    #[error(transparent)]
-    HandleAccesses(#[from] HandleAccessesError),
-    #[error(transparent)]
-    CreateRuleset(#[from] CreateRulesetError),
-    #[error(transparent)]
-    AddRules(#[from] AddRulesError),
-    #[error(transparent)]
-    RestrictSelf(#[from] RestrictSelfError),
+    HandleAccesses(HandleAccessesError),
+    CreateRuleset(CreateRulesetError),
+    AddRules(AddRulesError),
+    RestrictSelf(RestrictSelfError),
+}
+
+impl Error for RulesetError {
+    fn source(&self) -> Option<&(dyn Error + 'static)> {
+        match self {
+            RulesetError::HandleAccesses(err) => Error::source(err),
+            RulesetError::CreateRuleset(err) => Error::source(err),
+            RulesetError::AddRules(err) => Error::source(err),
+            RulesetError::RestrictSelf(err) => Error::source(err),
+        }
+    }
+}
+
+impl fmt::Display for RulesetError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            RulesetError::HandleAccesses(err) => fmt::Display::fmt(err, f),
+            RulesetError::CreateRuleset(err) => fmt::Display::fmt(err, f),
+            RulesetError::AddRules(err) => fmt::Display::fmt(err, f),
+            RulesetError::RestrictSelf(err) => fmt::Display::fmt(err, f),
+        }
+    }
+}
+
+impl std::convert::From<HandleAccessesError> for RulesetError {
+    fn from(source: HandleAccessesError) -> Self {
+        RulesetError::HandleAccesses(source)
+    }
+}
+
+impl std::convert::From<CreateRulesetError> for RulesetError {
+    fn from(source: CreateRulesetError) -> Self {
+        RulesetError::CreateRuleset(source)
+    }
+}
+
+impl std::convert::From<AddRulesError> for RulesetError {
+    fn from(source: AddRulesError) -> Self {
+        RulesetError::AddRules(source)
+    }
+}
+
+impl std::convert::From<RestrictSelfError> for RulesetError {
+    fn from(source: RestrictSelfError) -> Self {
+        RulesetError::RestrictSelf(source)
+    }
 }
 
 #[test]
@@ -28,23 +70,71 @@ fn ruleset_error_breaking_change() {
 }
 
 /// Identifies errors when updating the ruleset's handled access-rights.
-#[derive(Debug, Error)]
+#[derive(Debug)]
 #[non_exhaustive]
 pub enum HandleAccessError<T>
 where
     T: Access,
 {
-    #[error(transparent)]
-    Compat(#[from] CompatError<T>),
+    Compat(CompatError<T>),
 }
 
-#[derive(Debug, Error)]
+impl<T> Error for HandleAccessError<T>
+where
+    T: Access,
+    CompatError<T>: Error,
+    Self: fmt::Debug + fmt::Display,
+{
+    fn source(&self) -> Option<&(dyn Error + 'static)> {
+        match self {
+            HandleAccessError::Compat(err) => Error::source(err),
+        }
+    }
+}
+
+impl<T> fmt::Display for HandleAccessError<T>
+where
+    T: Access,
+    CompatError<T>: fmt::Display,
+{
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            HandleAccessError::Compat(err) => fmt::Display::fmt(err, f),
+        }
+    }
+}
+
+impl<T> std::convert::From<CompatError<T>> for HandleAccessError<T>
+where
+    T: Access,
+{
+    fn from(source: CompatError<T>) -> Self {
+        HandleAccessError::Compat(source)
+    }
+}
+
+#[derive(Debug)]
 #[non_exhaustive]
 pub enum HandleAccessesError {
-    #[error(transparent)]
     Fs(HandleAccessError<AccessFs>),
 }
 
+impl Error for HandleAccessesError {
+    fn source(&self) -> Option<&(dyn Error + 'static)> {
+        match self {
+            HandleAccessesError::Fs(err) => Error::source(err),
+        }
+    }
+}
+
+impl fmt::Display for HandleAccessesError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            HandleAccessesError::Fs(err) => fmt::Display::fmt(err, f),
+        }
+    }
+}
+
 // Generically implement for all the access implementations rather than for the cases listed in
 // HandleAccessesError (with #[from]).
 impl<A> From<HandleAccessError<A>> for HandleAccessesError
@@ -57,34 +147,103 @@ where
 }
 
 /// Identifies errors when creating a ruleset.
-#[derive(Debug, Error)]
+#[derive(Debug)]
 #[non_exhaustive]
 pub enum CreateRulesetError {
     /// The `landlock_create_ruleset()` system call failed.
-    #[error("failed to create a ruleset: {source}")]
     #[non_exhaustive]
     CreateRulesetCall { source: io::Error },
     /// Missing call to [`RulesetAttr::handle_access()`](crate::RulesetAttr::handle_access).
-    #[error("missing handled access")]
     MissingHandledAccess,
 }
 
+impl Error for CreateRulesetError {
+    fn source(&self) -> Option<&(dyn Error + 'static)> {
+        match self {
+            CreateRulesetError::CreateRulesetCall { source, .. } => Some(source),
+            CreateRulesetError::MissingHandledAccess { .. } => None,
+        }
+    }
+}
+
+impl fmt::Display for CreateRulesetError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            CreateRulesetError::CreateRulesetCall { source } => {
+                write!(f, "failed to create a ruleset: {source}",)
+            }
+            CreateRulesetError::MissingHandledAccess {} => {
+                write!(f, "missing handled access")
+            }
+        }
+    }
+}
+
 /// Identifies errors when adding a rule to a ruleset.
-#[derive(Debug, Error)]
+#[derive(Debug)]
 #[non_exhaustive]
 pub enum AddRuleError<T>
 where
     T: Access,
 {
     /// The `landlock_add_rule()` system call failed.
-    #[error("failed to add a rule: {source}")]
     #[non_exhaustive]
-    AddRuleCall { source: io::Error },
+    AddRuleCall {
+        source: io::Error,
+    },
     /// The rule's access-rights are not all handled by the (requested) ruleset access-rights.
-    #[error("access-rights not handled by the ruleset: {incompatible:?}")]
-    UnhandledAccess { access: T, incompatible: T },
-    #[error(transparent)]
-    Compat(#[from] CompatError<T>),
+    UnhandledAccess {
+        access: T,
+        incompatible: T,
+    },
+    Compat(CompatError<T>),
+}
+
+impl<T> Error for AddRuleError<T>
+where
+    T: Access,
+    CompatError<T>: Error,
+    Self: fmt::Debug + fmt::Display,
+{
+    fn source(&self) -> Option<&(dyn Error + 'static)> {
+        match self {
+            AddRuleError::AddRuleCall { source, .. } => Some(source),
+            AddRuleError::UnhandledAccess { .. } => None,
+            AddRuleError::Compat(err) => Error::source(err),
+        }
+    }
+}
+
+impl<T> fmt::Display for AddRuleError<T>
+where
+    T: Access,
+    T: fmt::Debug,
+    CompatError<T>: fmt::Display,
+{
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            AddRuleError::AddRuleCall { source } => {
+                write!(f, "failed to add a rule: {source}",)
+            }
+            AddRuleError::UnhandledAccess {
+                access: _,
+                incompatible,
+            } => write!(
+                f,
+                "access-rights not handled by the ruleset: {incompatible:?}",
+            ),
+            AddRuleError::Compat(err) => fmt::Display::fmt(err, f),
+        }
+    }
+}
+
+impl<T> std::convert::From<CompatError<T>> for AddRuleError<T>
+where
+    T: Access,
+{
+    fn from(source: CompatError<T>) -> Self {
+        AddRuleError::Compat { 0: source }
+    }
 }
 
 // Generically implement for all the access implementations rather than for the cases listed in
@@ -100,32 +259,89 @@ where
 
 /// Identifies errors when adding rules to a ruleset thanks to an iterator returning
 /// Result<Rule, E> items.
-#[derive(Debug, Error)]
+#[derive(Debug)]
 #[non_exhaustive]
 pub enum AddRulesError {
-    #[error(transparent)]
     Fs(AddRuleError<AccessFs>),
 }
 
-#[derive(Debug, Error)]
+impl Error for AddRulesError {
+    fn source(&self) -> Option<&(dyn Error + 'static)> {
+        match self {
+            AddRulesError::Fs(err) => Error::source(err),
+        }
+    }
+}
+
+impl fmt::Display for AddRulesError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            AddRulesError::Fs(err) => fmt::Display::fmt(err, f),
+        }
+    }
+}
+
+#[derive(Debug)]
 #[non_exhaustive]
 pub enum CompatError<T>
 where
     T: Access,
 {
-    #[error(transparent)]
-    PathBeneath(#[from] PathBeneathError),
-    #[error(transparent)]
-    Access(#[from] AccessError<T>),
+    PathBeneath(PathBeneathError),
+    Access(AccessError<T>),
 }
 
-#[derive(Debug, Error)]
+impl<T> Error for CompatError<T>
+where
+    T: Access,
+    AccessError<T>: Error,
+    Self: fmt::Debug + fmt::Display,
+{
+    fn source(&self) -> Option<&(dyn Error + 'static)> {
+        match self {
+            CompatError::PathBeneath(err) => Error::source(err),
+            CompatError::Access(err) => Error::source(err),
+        }
+    }
+}
+
+impl<T> fmt::Display for CompatError<T>
+where
+    T: Access,
+    AccessError<T>: fmt::Display,
+{
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            CompatError::PathBeneath(err) => fmt::Display::fmt(err, f),
+            CompatError::Access(err) => fmt::Display::fmt(err, f),
+        }
+    }
+}
+
+impl<T> std::convert::From<PathBeneathError> for CompatError<T>
+where
+    T: Access,
+{
+    fn from(source: PathBeneathError) -> Self {
+        CompatError::PathBeneath(source)
+    }
+}
+
+impl<T> std::convert::From<AccessError<T>> for CompatError<T>
+where
+    T: Access,
+{
+    fn from(source: AccessError<T>) -> Self {
+        CompatError::Access(source)
+    }
+}
+
+#[derive(Debug)]
 #[non_exhaustive]
 pub enum PathBeneathError {
     /// To check that access-rights are consistent with a file descriptor, a call to
     /// [`RulesetCreatedAttr::add_rule()`](crate::RulesetCreatedAttr::add_rule)
     /// looks at the file type with an `fstat()` system call.
-    #[error("failed to check file descriptor type: {source}")]
     #[non_exhaustive]
     StatCall { source: io::Error },
     /// This error is returned by
@@ -133,14 +349,39 @@ pub enum PathBeneathError {
     /// if the related PathBeneath object is not set to best-effort,
     /// and if its allowed access-rights contain directory-only ones
     /// whereas the file descriptor doesn't point to a directory.
-    #[error("incompatible directory-only access-rights: {incompatible:?}")]
     DirectoryAccess {
         access: AccessFs,
         incompatible: AccessFs,
     },
 }
 
-#[derive(Debug, Error)]
+impl Error for PathBeneathError {
+    fn source(&self) -> Option<&(dyn Error + 'static)> {
+        match self {
+            PathBeneathError::StatCall { source, .. } => Some(source),
+            PathBeneathError::DirectoryAccess { .. } => None,
+        }
+    }
+}
+
+impl fmt::Display for PathBeneathError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            PathBeneathError::StatCall { source } => {
+                write!(f, "failed to check file descriptor type: {source}",)
+            }
+            PathBeneathError::DirectoryAccess {
+                access: _,
+                incompatible,
+            } => write!(
+                f,
+                "incompatible directory-only access-rights: {incompatible:?}",
+            ),
+        }
+    }
+}
+
+#[derive(Debug)]
 // Exhaustive enum
 pub enum AccessError<T>
 where
@@ -148,51 +389,166 @@ where
 {
     /// The access-rights set is empty, which doesn't make sense and would be rejected by the
     /// kernel.
-    #[error("empty access-right")]
     Empty,
     /// The access-rights set was forged with the unsafe `BitFlags::from_bits_unchecked()` and it
     /// contains unknown bits.
-    #[error("unknown access-rights (at build time): {unknown:?}")]
     Unknown { access: T, unknown: T },
     /// The best-effort approach was (deliberately) disabled and the requested access-rights are
     /// fully incompatible with the running kernel.
-    #[error("fully incompatible access-rights: {access:?}")]
     Incompatible { access: T },
     /// The best-effort approach was (deliberately) disabled and the requested access-rights are
     /// partially incompatible with the running kernel.
-    #[error("partially incompatible access-rights: {incompatible:?}")]
     PartiallyCompatible { access: T, incompatible: T },
 }
 
-#[derive(Debug, Error)]
+impl<T> Error for AccessError<T>
+where
+    T: Access,
+    Self: fmt::Debug + fmt::Display,
+{
+}
+
+impl<T> fmt::Display for AccessError<T>
+where
+    T: Access,
+    T: fmt::Debug,
+{
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            AccessError::Empty {} => write!(f, "empty access-right"),
+            AccessError::Unknown { access: _, unknown } => write!(
+                f,
+                "unknown access-rights (at build time): {unknown:?}",
+                unknown = unknown
+            ),
+            AccessError::Incompatible { access } => write!(
+                f,
+                "fully incompatible access-rights: {access:?}",
+                access = access
+            ),
+            AccessError::PartiallyCompatible {
+                access: _,
+                incompatible,
+            } => write!(
+                f,
+                "partially incompatible access-rights: {incompatible:?}",
+                incompatible = incompatible
+            ),
+        }
+    }
+}
+
+#[derive(Debug)]
 #[non_exhaustive]
 pub enum RestrictSelfError {
     /// The `prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)` system call failed.
-    #[error("failed to set no_new_privs: {source}")]
     #[non_exhaustive]
     SetNoNewPrivsCall { source: io::Error },
     /// The `landlock_restrict_self() `system call failed.
-    #[error("failed to restrict the calling thread: {source}")]
     #[non_exhaustive]
     RestrictSelfCall { source: io::Error },
 }
 
-#[derive(Debug, Error)]
+impl Error for RestrictSelfError {
+    fn source(&self) -> Option<&(dyn Error + 'static)> {
+        match self {
+            RestrictSelfError::SetNoNewPrivsCall { source, .. } => Some(source),
+            RestrictSelfError::RestrictSelfCall { source, .. } => Some(source),
+        }
+    }
+}
+
+impl fmt::Display for RestrictSelfError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            RestrictSelfError::SetNoNewPrivsCall { source } => {
+                write!(f, "failed to set no_new_privs: {source}",)
+            }
+            RestrictSelfError::RestrictSelfCall { source } => {
+                write!(f, "failed to restrict the calling thread: {source}",)
+            }
+        }
+    }
+}
+
+#[derive(Debug)]
 #[non_exhaustive]
 pub enum PathFdError {
     /// The `open()` system call failed.
-    #[error("failed to open \"{path}\": {source}")]
     #[non_exhaustive]
     OpenCall { source: io::Error, path: PathBuf },
 }
 
+impl Error for PathFdError {
+    fn source(&self) -> Option<&(dyn Error + 'static)> {
+        match self {
+            PathFdError::OpenCall { source, .. } => Some(source),
+        }
+    }
+}
+
+impl fmt::Display for PathFdError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            PathFdError::OpenCall { source, path } => {
+                write!(
+                    f,
+                    "failed to open \"{path}\": {source}",
+                    path = path.display()
+                )
+            }
+        }
+    }
+}
+
 #[cfg(test)]
-#[derive(Debug, Error)]
+#[derive(Debug)]
 pub enum TestRulesetError {
-    #[error(transparent)]
-    Ruleset(#[from] RulesetError),
-    #[error(transparent)]
-    PathFd(#[from] PathFdError),
-    #[error(transparent)]
-    File(#[from] std::io::Error),
+    Ruleset(RulesetError),
+    PathFd(PathFdError),
+    File(std::io::Error),
+}
+
+
+#[cfg(test)]
+impl Error for TestRulesetError {
+    fn source(&self) -> Option<&(dyn Error + 'static)> {
+        match self {
+            TestRulesetError::Ruleset(err) => Error::source(err),
+            TestRulesetError::PathFd(err) => Error::source(err),
+            TestRulesetError::File(err) => Error::source(err),
+        }
+    }
+}
+
+#[cfg(test)]
+impl fmt::Display for TestRulesetError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        match self {
+            TestRulesetError::Ruleset(err) => fmt::Display::fmt(err, f),
+            TestRulesetError::PathFd(err) => fmt::Display::fmt(err, f),
+            TestRulesetError::File(err) => fmt::Display::fmt(err, f),
+        }
+    }
+}
+#[cfg(test)]
+
+impl std::convert::From<RulesetError> for TestRulesetError {
+    fn from(source: RulesetError) -> Self {
+        TestRulesetError::Ruleset(source)
+    }
+}
+
+#[cfg(test)]
+impl std::convert::From<PathFdError> for TestRulesetError {
+    fn from(source: PathFdError) -> Self {
+        TestRulesetError::PathFd(source)
+    }
+}
+
+#[cfg(test)]
+impl std::convert::From<std::io::Error> for TestRulesetError {
+    fn from(source: std::io::Error) -> Self {
+        TestRulesetError::File(source)
+    }
 }

[l0kod]

Commit 282090a switched from bitflags to enumflags2 to be more ergonomic. While your BitOr and BitAnd implementations help, I think the make_bitflags!() macro is pretty handy to avoid repeating the enum type for each union. There is currently 14 bits used for AccessFs but this will grow over time. Do you have ideas to get a similar not-too-verbose writing?

FYI, as you may have seen in the comments, I'd like to wrap this type to forbid calling all() because it can lead to compatibility issues. I'm not sure how to write it without too much code duplication though, especially with the extra bitflag traits.

[bjorn3]

I think the make_bitflags!() macro is pretty handy to avoid repeating the enum type for each union.

I can write one again.

Edit: done

FYI, as you may have seen in the comments, I'd like to wrap this type to forbid calling all() because it can lead to compatibility issues. I'm not sure how to write it without too much code duplication though, especially with the extra bitflag traits.

Maybe the bitflags! macro could add a #[non_exhaustive] option or something to suppress generating all(), is_all(), complement() and other method which would change behavior if new flags are added? I can imagine there are other cases where this would be useful too.

Edit: has been suggested before, but was rejected: bitflags/bitflags#269

[l0kod]

I think the make_bitflags!() macro is pretty handy to avoid repeating the enum type for each union.

I can write one again.

Edit: done

Great!

Can you please merge both commits, refresh perf numbers and add a "Breaking change" warning explaining how to migrate? In theory this is not a breaking change with the previous version but in practice people are already using the main branch. I should probably release a 0.2 version soon, and keep the #12 (and #28) changes for the 0.3.

The macro definition should probably be moved to the access.rs file, but I'm not sure if it's allowed by rustc.

FYI, as you may have seen in the comments, I'd like to wrap this type to forbid calling all() because it can lead to compatibility issues. I'm not sure how to write it without too much code duplication though, especially with the extra bitflag traits.

Maybe the bitflags! macro could add a #[non_exhaustive] option or something to suppress generating all(), is_all(), complement() and other method which would change behavior if new flags are added? I can imagine there are other cases where this would be useful too.

Edit: has been suggested before, but was rejected: bitflags/bitflags#269

We should wrap the bitflags in our own type to avoid public all()-like methods. This can be part of another PR though.

FYI, I plan to change these Access* types to improve the ease of use of the API (e.g. automatic access right inference according to a ruleset's ABI). This PR helps by getting rid of BitFlags.

[bjorn3]

Ready for review. I've rewritten it to use a macro we define ourself for creating the bitflags types to allow controlling the api we expose to the user.

[bjorn3]

This error is no longer possible as the public api of Access* ensures no unknown bits are set.

[bjorn3]

I've removed this warning as Access*::all() is no longer part of the public api.

[l0kod]

Thanks for the update!

Could you please describe in the changelog file the potential required changes for users when updating this crate?

[l0kod]

known_and_unknown_flags() seems easier to understand.

[bjorn3]

Not sure what happened, but I can't find this method anymore.

[bjorn3]

I've fixed the review comments and added a changelog entry.