From 90df5c8a10df710c82750bf6a99f04ab48ae9877 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Linus=20F=C3=A4rnstrand?= Date: Thu, 4 Jul 2024 15:17:00 +0200 Subject: [PATCH 1/3] Remove error-chain and replace with handcrafted errors This changes the API of the error a lot, and the returned error chain in many calls into this library --- examples/enable.rs | 2 +- src/lib.rs | 141 +++++++++++++++++++++++++++++----------- src/macros.rs | 11 ++-- src/pooladdr.rs | 10 +-- src/rule/endpoint.rs | 6 +- src/rule/interface.rs | 8 ++- src/rule/mod.rs | 48 +++++++------- src/rule/port.rs | 24 +++---- src/transaction.rs | 22 +++---- src/utils.rs | 14 ++-- tests/anchors.rs | 4 +- tests/enable_disable.rs | 4 +- 12 files changed, 185 insertions(+), 109 deletions(-) diff --git a/examples/enable.rs b/examples/enable.rs index dfeac9a..b1bfe9a 100644 --- a/examples/enable.rs +++ b/examples/enable.rs @@ -15,7 +15,7 @@ fn main() { // Try to enable the firewall. Equivalent to the CLI command "pfctl -e". match pf.enable() { Ok(_) => println!("Enabled PF"), - Err(pfctl::Error(pfctl::ErrorKind::StateAlreadyActive, _)) => (), + Err(e) if e.kind() == pfctl::ErrorKind::StateAlreadyActive => (), err => err.expect("Unable to enable PF"), } } diff --git a/src/lib.rs b/src/lib.rs index 08df079..a9368d3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -59,15 +59,13 @@ #![deny(rust_2018_idioms)] -#[macro_use] -pub extern crate error_chain; - -use core::slice; use std::{ ffi::CStr, + fmt, fs::File, mem, os::unix::io::{AsRawFd, RawFd}, + slice, }; pub use ipnetwork; @@ -93,40 +91,107 @@ pub use crate::ruleset::*; mod transaction; pub use crate::transaction::*; -mod errors { - error_chain! { - errors { - DeviceOpenError(s: &'static str) { - description("Unable to open PF device file") - display("Unable to open PF device file at '{}'", s) - } - InvalidArgument(s: &'static str) { - display("Invalid argument: {}", s) - } - StateAlreadyActive { - description("Target state is already active") - } - InvalidRuleCombination(s: String) { - description("Rule contains incompatible values") - display("Incompatible values in rule: {}", s) - } - AnchorDoesNotExist { - display("Anchor does not exist") +pub type Result = std::result::Result; + +#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] +#[non_exhaustive] +pub enum ErrorKind { + /// Failed to open PF control file /dev/pf + DeviceOpen, + /// The firewall rule is invalidly configured + InvalidRuleCombination, + /// The supplied network interface name is not compatible with PF + InvalidInterfaceName, + /// The supplied anchor name in not compatible with PF + InvalidAnchorName, + /// The supplied port is an invalid range + InvalidPortRange, + /// The supplied rule label is not compatible with PF. + InvalidLabel, + /// The target state was already active + StateAlreadyActive, + /// This PF anchor does not exist + AnchorDoesNotExist, + /// System returned an error during ioctl system call + Ioctl, +} + +#[derive(Debug)] +pub struct Error(ErrorInternal); + +#[derive(Debug)] +enum ErrorInternal { + DeviceOpen(&'static str, std::io::Error), + InvalidRuleCombination(String), + InvalidInterfaceName(&'static str), + InvalidAnchorName(&'static str), + InvalidPortRange, + InvalidLabel(&'static str), + StateAlreadyActive, + AnchorDoesNotExist, + Ioctl(std::io::Error), +} + +impl Error { + /// Returns the kind of error that happened as an enum + pub fn kind(&self) -> ErrorKind { + use ErrorInternal::*; + match self.0 { + DeviceOpen(..) => ErrorKind::DeviceOpen, + InvalidRuleCombination(_) => ErrorKind::InvalidRuleCombination, + InvalidInterfaceName(..) => ErrorKind::InvalidInterfaceName, + InvalidAnchorName(..) => ErrorKind::InvalidAnchorName, + InvalidPortRange => ErrorKind::InvalidPortRange, + InvalidLabel(..) => ErrorKind::InvalidLabel, + StateAlreadyActive => ErrorKind::StateAlreadyActive, + AnchorDoesNotExist => ErrorKind::AnchorDoesNotExist, + Ioctl(_) => ErrorKind::Ioctl, + } + } +} + +impl From for Error { + fn from(e: ErrorInternal) -> Self { + Error(e) + } +} + +impl fmt::Display for Error { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + use ErrorInternal::*; + match &self.0 { + DeviceOpen(device_path, _) => { + write!(f, "Unable to open PF device file ({device_path})") } + InvalidRuleCombination(msg) => write!(f, "Invalid rule combination: {msg}"), + InvalidInterfaceName(reason) => write!(f, "Invalid interface name ({reason})"), + InvalidAnchorName(reason) => write!(f, "Invalid anchor name ({reason})"), + InvalidPortRange => write!(f, "Lower port is greater than upper port"), + InvalidLabel(reason) => write!(f, "Invalid rule label ({reason}"), + StateAlreadyActive => write!(f, "Target state is already active"), + AnchorDoesNotExist => write!(f, "Anchor does not exist"), + Ioctl(_) => write!(f, "Error during ioctl syscall"), } - foreign_links { - IoctlError(::std::io::Error); + } +} + +impl std::error::Error for Error { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + use ErrorInternal::*; + match &self.0 { + DeviceOpen(_, ref e) => Some(e), + Ioctl(e) => Some(e), + _ => None, } } } -pub use crate::errors::*; /// Returns the given input result, except if it is an `Err` matching the given `ErrorKind`, /// then it returns `Ok(())` instead, so the error is ignored. macro_rules! ignore_error_kind { - ($result:expr, $kind:pat) => { + ($result:expr, $kind:expr) => { match $result { - Err($crate::Error($kind, _)) => Ok(()), + Err(e) if e.kind() == $kind => Ok(()), result => result, } }; @@ -142,7 +207,9 @@ mod conversion { /// Internal trait for all types that can try to write their value into another type. pub trait TryCopyTo { - fn try_copy_to(&self, dst: &mut T) -> crate::Result<()>; + type Error; + + fn try_copy_to(&self, dst: &mut T) -> Result<(), Self::Error>; } } use crate::conversion::*; @@ -214,8 +281,7 @@ impl PfCtl { let mut pfioc_rule = unsafe { mem::zeroed::() }; pfioc_rule.rule.action = kind.into(); - name.try_copy_to(&mut pfioc_rule.anchor_call[..]) - .chain_err(|| ErrorKind::InvalidArgument("Invalid anchor name"))?; + utils::copy_anchor_name(name, &mut pfioc_rule.anchor_call[..])?; ioctl_guard!(ffi::pf_insert_rule(self.fd(), &mut pfioc_rule))?; Ok(()) @@ -248,9 +314,7 @@ impl PfCtl { pfioc_rule.pool_ticket = utils::get_pool_ticket(self.fd())?; pfioc_rule.ticket = utils::get_ticket(self.fd(), anchor, AnchorKind::Filter)?; - anchor - .try_copy_to(&mut pfioc_rule.anchor[..]) - .chain_err(|| ErrorKind::InvalidArgument("Invalid anchor name"))?; + utils::copy_anchor_name(anchor, &mut pfioc_rule.anchor[..])?; rule.try_copy_to(&mut pfioc_rule.rule)?; pfioc_rule.action = ffi::pfvar::PF_CHANGE_ADD_TAIL as u32; @@ -266,7 +330,7 @@ impl PfCtl { pub fn add_redirect_rule(&mut self, anchor: &str, rule: &RedirectRule) -> Result<()> { // prepare pfioc_rule let mut pfioc_rule = unsafe { mem::zeroed::() }; - anchor.try_copy_to(&mut pfioc_rule.anchor[..])?; + utils::copy_anchor_name(anchor, &mut pfioc_rule.anchor[..])?; rule.try_copy_to(&mut pfioc_rule.rule)?; // register redirect address in newly created address pool @@ -329,9 +393,8 @@ impl PfCtl { /// Returns total number of removed states upon success pub fn clear_interface_states(&mut self, interface: Interface) -> Result { let mut pfioc_state_kill = unsafe { mem::zeroed::() }; - interface - .try_copy_to(&mut pfioc_state_kill.psk_ifname) - .chain_err(|| ErrorKind::InvalidArgument("Incompatible interface name"))?; + interface.try_copy_to(&mut pfioc_state_kill.psk_ifname)?; + ioctl_guard!(ffi::pf_clear_states(self.fd(), &mut pfioc_state_kill))?; // psk_af holds the number of killed states Ok(pfioc_state_kill.psk_af as u32) @@ -373,7 +436,7 @@ impl PfCtl { return f(pfioc_rule); } } - bail!(ErrorKind::AnchorDoesNotExist); + Err(Error::from(ErrorInternal::AnchorDoesNotExist)) } /// Returns global number of states created by all stateful rules (see keep_state) diff --git a/src/macros.rs b/src/macros.rs index aa38f5f..0d4ac31 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -19,11 +19,12 @@ macro_rules! ioctl_guard { let error_code = io_error .raw_os_error() .expect("Errors created with last_os_error should have errno"); - let mut err = Err($crate::ErrorKind::IoctlError(io_error).into()); - if error_code == $already_active { - err = err.chain_err(|| $crate::ErrorKind::StateAlreadyActive); - } - err + + Err($crate::Error::from(if error_code == $already_active { + $crate::ErrorInternal::StateAlreadyActive + } else { + $crate::ErrorInternal::Ioctl(io_error) + })) } else { Ok(()) as $crate::Result<()> } diff --git a/src/pooladdr.rs b/src/pooladdr.rs index 9d464f8..0c54bd4 100644 --- a/src/pooladdr.rs +++ b/src/pooladdr.rs @@ -8,7 +8,7 @@ use crate::{ conversion::{CopyTo, TryCopyTo}, - ffi, Interface, Ip, Result, + ffi, Interface, Ip, }; use std::{mem, ptr, vec::Vec}; @@ -46,7 +46,9 @@ impl From for PoolAddr { } impl TryCopyTo for PoolAddr { - fn try_copy_to(&self, pf_pooladdr: &mut ffi::pfvar::pf_pooladdr) -> Result<()> { + type Error = crate::Error; + + fn try_copy_to(&self, pf_pooladdr: &mut ffi::pfvar::pf_pooladdr) -> Result<(), Self::Error> { self.interface.try_copy_to(&mut pf_pooladdr.ifname)?; self.ip.copy_to(&mut pf_pooladdr.addr); Ok(()) @@ -67,7 +69,7 @@ pub struct PoolAddrList { } impl PoolAddrList { - pub fn new(pool_addrs: &[PoolAddr]) -> Result { + pub fn new(pool_addrs: &[PoolAddr]) -> Result { let mut pool = Self::init_pool(pool_addrs)?; Self::link_elements(&mut pool); let list = Self::create_palist(&mut pool); @@ -88,7 +90,7 @@ impl PoolAddrList { self.list } - fn init_pool(pool_addrs: &[PoolAddr]) -> Result> { + fn init_pool(pool_addrs: &[PoolAddr]) -> Result, crate::Error> { let mut pool = Vec::with_capacity(pool_addrs.len()); for pool_addr in pool_addrs { let mut pf_pooladdr = unsafe { mem::zeroed::() }; diff --git a/src/rule/endpoint.rs b/src/rule/endpoint.rs index 3c83bc0..082da2e 100644 --- a/src/rule/endpoint.rs +++ b/src/rule/endpoint.rs @@ -9,7 +9,7 @@ use super::{AddrFamily, Ip, Port}; use crate::{ conversion::{CopyTo, TryCopyTo}, - ffi, Result, + ffi, }; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr, SocketAddrV4, SocketAddrV6}; @@ -92,7 +92,9 @@ impl From for Endpoint { } impl TryCopyTo for Endpoint { - fn try_copy_to(&self, pf_rule_addr: &mut ffi::pfvar::pf_rule_addr) -> Result<()> { + type Error = crate::Error; + + fn try_copy_to(&self, pf_rule_addr: &mut ffi::pfvar::pf_rule_addr) -> crate::Result<()> { self.ip.copy_to(&mut pf_rule_addr.addr); self.port .try_copy_to(unsafe { &mut pf_rule_addr.xport.range })?; diff --git a/src/rule/interface.rs b/src/rule/interface.rs index 2bb4b76..704c6ca 100644 --- a/src/rule/interface.rs +++ b/src/rule/interface.rs @@ -6,7 +6,8 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -use crate::{conversion::TryCopyTo, Result}; +use crate::conversion::TryCopyTo; +use crate::{Error, ErrorInternal}; #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct InterfaceName(String); @@ -31,11 +32,14 @@ impl> From for Interface { } impl TryCopyTo<[i8]> for Interface { - fn try_copy_to(&self, dst: &mut [i8]) -> Result<()> { + type Error = crate::Error; + + fn try_copy_to(&self, dst: &mut [i8]) -> Result<(), Self::Error> { match *self { Interface::Any => "", Interface::Name(InterfaceName(ref name)) => &name[..], } .try_copy_to(dst) + .map_err(|reason| Error::from(ErrorInternal::InvalidInterfaceName(reason))) } } diff --git a/src/rule/mod.rs b/src/rule/mod.rs index a18006f..229ae25 100644 --- a/src/rule/mod.rs +++ b/src/rule/mod.rs @@ -8,7 +8,7 @@ use crate::{ conversion::{CopyTo, TryCopyTo}, - ffi, ErrorKind, Result, ResultExt, + ffi, Error, ErrorInternal, Result, }; use derive_builder::Builder; use ipnetwork::IpNetwork; @@ -99,7 +99,7 @@ pub struct FilterRule { impl FilterRuleBuilder { pub fn build(&self) -> Result { self.build_internal() - .map_err(|e| ErrorKind::InvalidRuleCombination(e).into()) + .map_err(|e| ErrorInternal::InvalidRuleCombination(e).into()) } } @@ -128,13 +128,15 @@ impl FilterRule { "StatePolicy {:?} and protocol {:?} are incompatible", state_policy, proto ); - bail!(ErrorKind::InvalidRuleCombination(msg)); + Err(Error::from(ErrorInternal::InvalidRuleCombination(msg))) } } } } impl TryCopyTo for FilterRule { + type Error = crate::Error; + fn try_copy_to(&self, pf_rule: &mut ffi::pfvar::pf_rule) -> Result<()> { pf_rule.action = self.action.into(); pf_rule.direction = self.direction.into(); @@ -146,15 +148,15 @@ impl TryCopyTo for FilterRule { pf_rule.flagset = (&self.tcp_flags.mask).into(); pf_rule.rule_flag = self.action.rule_flags(); - self.interface - .try_copy_to(&mut pf_rule.ifname) - .chain_err(|| ErrorKind::InvalidArgument("Incompatible interface name"))?; + self.interface.try_copy_to(&mut pf_rule.ifname)?; pf_rule.proto = self.proto.into(); pf_rule.af = self.get_af()?.into(); self.from.try_copy_to(&mut pf_rule.src)?; self.to.try_copy_to(&mut pf_rule.dst)?; - self.label.try_copy_to(&mut pf_rule.label)?; + self.label + .try_copy_to(&mut pf_rule.label) + .map_err(ErrorInternal::InvalidLabel)?; self.user.copy_to(&mut pf_rule.uid); self.group.copy_to(&mut pf_rule.gid); if let Some(icmp_type) = self.icmp_type { @@ -198,7 +200,7 @@ pub struct RedirectRule { impl RedirectRuleBuilder { pub fn build(&self) -> Result { self.build_internal() - .map_err(|e| ErrorKind::InvalidRuleCombination(e).into()) + .map_err(|e| ErrorInternal::InvalidRuleCombination(e).into()) } } @@ -218,20 +220,22 @@ impl RedirectRule { } impl TryCopyTo for RedirectRule { + type Error = crate::Error; + fn try_copy_to(&self, pf_rule: &mut ffi::pfvar::pf_rule) -> Result<()> { pf_rule.action = self.action.into(); pf_rule.direction = self.direction.into(); pf_rule.quick = self.quick as u8; pf_rule.log = (&self.log).into(); - self.interface - .try_copy_to(&mut pf_rule.ifname) - .chain_err(|| ErrorKind::InvalidArgument("Incompatible interface name"))?; + self.interface.try_copy_to(&mut pf_rule.ifname)?; pf_rule.proto = self.proto.into(); pf_rule.af = self.get_af()?.into(); self.from.try_copy_to(&mut pf_rule.src)?; self.to.try_copy_to(&mut pf_rule.dst)?; - self.label.try_copy_to(&mut pf_rule.label)?; + self.label + .try_copy_to(&mut pf_rule.label) + .map_err(ErrorInternal::InvalidLabel)?; self.user.copy_to(&mut pf_rule.uid); self.group.copy_to(&mut pf_rule.gid); @@ -246,7 +250,7 @@ fn compatible_af(af1: AddrFamily, af2: AddrFamily) -> Result { (AddrFamily::Any, af) => Ok(af), (af1, af2) => { let msg = format!("AddrFamily {} and {} are incompatible", af1, af2); - bail!(ErrorKind::InvalidRuleCombination(msg)); + Err(Error::from(ErrorInternal::InvalidRuleCombination(msg))) } } } @@ -287,19 +291,19 @@ impl CopyTo for Ipv6Addr { } impl> TryCopyTo<[i8]> for T { + type Error = &'static str; + /// Safely copy a Rust string into a raw buffer. Returning an error if the string could not /// be copied to the buffer. - fn try_copy_to(&self, dst: &mut [i8]) -> Result<()> { + fn try_copy_to(&self, dst: &mut [i8]) -> std::result::Result<(), Self::Error> { let src_i8: &[i8] = unsafe { &*(self.as_ref().as_bytes() as *const _ as *const _) }; - ensure!( - src_i8.len() < dst.len(), - ErrorKind::InvalidArgument("String does not fit destination") - ); - ensure!( - !src_i8.contains(&0), - ErrorKind::InvalidArgument("String has null byte") - ); + if src_i8.len() >= dst.len() { + return Err("Too long"); + } + if src_i8.contains(&0) { + return Err("Contains null byte"); + } dst[..src_i8.len()].copy_from_slice(src_i8); // Terminate ffi string with null byte diff --git a/src/rule/port.rs b/src/rule/port.rs index 0db3da6..a481369 100644 --- a/src/rule/port.rs +++ b/src/rule/port.rs @@ -6,7 +6,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -use crate::{conversion::TryCopyTo, ffi, ErrorKind, Result}; +use crate::{conversion::TryCopyTo, ffi, Error, ErrorInternal}; // Port range representation #[derive(Debug, Default, Clone, Copy, PartialEq, Eq, Hash)] @@ -24,7 +24,9 @@ impl From for Port { } impl TryCopyTo for Port { - fn try_copy_to(&self, pf_port_range: &mut ffi::pfvar::pf_port_range) -> Result<()> { + type Error = crate::Error; + + fn try_copy_to(&self, pf_port_range: &mut ffi::pfvar::pf_port_range) -> crate::Result<()> { match *self { Port::Any => { pf_port_range.op = ffi::pfvar::PF_OP_NONE as u8; @@ -38,10 +40,9 @@ impl TryCopyTo for Port { pf_port_range.port[1] = 0; } Port::Range(start_port, end_port, modifier) => { - ensure!( - start_port <= end_port, - ErrorKind::InvalidArgument("Lower port is greater than upper port.") - ); + if start_port > end_port { + return Err(Error::from(ErrorInternal::InvalidPortRange)); + } pf_port_range.op = modifier.into(); // convert port range to network byte order pf_port_range.port[0] = start_port.to_be(); @@ -53,7 +54,9 @@ impl TryCopyTo for Port { } impl TryCopyTo for Port { - fn try_copy_to(&self, pf_pool: &mut ffi::pfvar::pf_pool) -> Result<()> { + type Error = crate::Error; + + fn try_copy_to(&self, pf_pool: &mut ffi::pfvar::pf_pool) -> crate::Result<()> { match *self { Port::Any => { pf_pool.port_op = ffi::pfvar::PF_OP_NONE as u8; @@ -66,10 +69,9 @@ impl TryCopyTo for Port { pf_pool.proxy_port[1] = 0; } Port::Range(start_port, end_port, modifier) => { - ensure!( - start_port <= end_port, - ErrorKind::InvalidArgument("Lower port is greater than upper port.") - ); + if start_port > end_port { + return Err(Error::from(ErrorInternal::InvalidPortRange)); + } pf_pool.port_op = modifier.into(); pf_pool.proxy_port[0] = start_port; pf_pool.proxy_port[1] = end_port; diff --git a/src/transaction.rs b/src/transaction.rs index 6d042f9..940791d 100644 --- a/src/transaction.rs +++ b/src/transaction.rs @@ -7,8 +7,7 @@ // except according to those terms. use crate::{ - conversion::TryCopyTo, ffi, utils, ErrorKind, FilterRule, PoolAddrList, RedirectRule, Result, - ResultExt, RulesetKind, + conversion::TryCopyTo, ffi, utils, FilterRule, PoolAddrList, RedirectRule, Result, RulesetKind, }; use std::{ collections::HashMap, @@ -117,9 +116,7 @@ impl Transaction { // prepare pfioc_rule let mut pfioc_rule = unsafe { mem::zeroed::() }; pfioc_rule.action = ffi::pfvar::PF_CHANGE_NONE as u32; - anchor - .try_copy_to(&mut pfioc_rule.anchor[..]) - .chain_err(|| ErrorKind::InvalidArgument("Invalid anchor name"))?; + utils::copy_anchor_name(anchor, &mut pfioc_rule.anchor[..])?; rule.try_copy_to(&mut pfioc_rule.rule)?; // request new address pool @@ -130,8 +127,7 @@ impl Transaction { let _pool_addr_list = if let Some(pool_addr) = rule.get_route().get_pool_addr() { // register pool address with firewall utils::add_pool_address(fd, pool_addr.clone(), pool_ticket)?; - let pool_addr_list = PoolAddrList::new(&[pool_addr.clone()]) - .chain_err(|| ErrorKind::InvalidArgument("Invalid route target"))?; + let pool_addr_list = PoolAddrList::new(&[pool_addr.clone()])?; pfioc_rule.rule.rpool.list = unsafe { pool_addr_list.to_palist() }; Some(pool_addr_list) @@ -144,16 +140,16 @@ impl Transaction { pfioc_rule.pool_ticket = pool_ticket; // add rule into transaction - ioctl_guard!(ffi::pf_add_rule(fd, &mut pfioc_rule)) + ioctl_guard!(ffi::pf_add_rule(fd, &mut pfioc_rule))?; + drop(_pool_addr_list); + Ok(()) } /// Internal helper to add redirect rule into transaction fn add_redirect_rule(fd: RawFd, anchor: &str, rule: &RedirectRule, ticket: u32) -> Result<()> { // prepare pfioc_rule let mut pfioc_rule = unsafe { mem::zeroed::() }; - anchor - .try_copy_to(&mut pfioc_rule.anchor[..]) - .chain_err(|| ErrorKind::InvalidArgument("Invalid anchor name"))?; + utils::copy_anchor_name(anchor, &mut pfioc_rule.anchor[..])?; rule.try_copy_to(&mut pfioc_rule.rule)?; // register redirect address in newly created address pool @@ -191,9 +187,7 @@ impl Transaction { ) -> Result { let mut pfioc_trans_e = unsafe { mem::zeroed::() }; pfioc_trans_e.rs_num = ruleset_kind.into(); - anchor - .try_copy_to(&mut pfioc_trans_e.anchor[..]) - .chain_err(|| ErrorKind::InvalidArgument("Invalid anchor name"))?; + utils::copy_anchor_name(anchor, &mut pfioc_trans_e.anchor[..])?; Ok(pfioc_trans_e) } } diff --git a/src/utils.rs b/src/utils.rs index b45b18a..1d3b4bd 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -6,7 +6,7 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -use crate::{conversion::TryCopyTo, ffi, AnchorKind, ErrorKind, PoolAddr, Result, ResultExt}; +use crate::{conversion::TryCopyTo, ffi, AnchorKind, Error, ErrorInternal, PoolAddr, Result}; use std::{ fs::{File, OpenOptions}, mem, @@ -22,7 +22,7 @@ pub fn open_pf() -> Result { .read(true) .write(true) .open(PF_DEV_PATH) - .chain_err(|| ErrorKind::DeviceOpenError(PF_DEV_PATH)) + .map_err(|e| Error::from(ErrorInternal::DeviceOpen(PF_DEV_PATH, e))) } /// Add pool address using the pool ticket previously obtained via `get_pool_ticket()` @@ -48,9 +48,13 @@ pub fn get_ticket(fd: RawFd, anchor: &str, kind: AnchorKind) -> Result { let mut pfioc_rule = unsafe { mem::zeroed::() }; pfioc_rule.action = ffi::pfvar::PF_CHANGE_GET_TICKET as u32; pfioc_rule.rule.action = kind.into(); - anchor - .try_copy_to(&mut pfioc_rule.anchor[..]) - .chain_err(|| ErrorKind::InvalidArgument("Invalid anchor name"))?; + copy_anchor_name(anchor, &mut pfioc_rule.anchor[..])?; ioctl_guard!(ffi::pf_change_rule(fd, &mut pfioc_rule))?; Ok(pfioc_rule.ticket) } + +pub fn copy_anchor_name(anchor: &str, destination: &mut [i8]) -> Result<()> { + anchor + .try_copy_to(destination) + .map_err(|reason| Error::from(ErrorInternal::InvalidAnchorName(reason))) +} diff --git a/tests/anchors.rs b/tests/anchors.rs index 161c898..d73523a 100644 --- a/tests/anchors.rs +++ b/tests/anchors.rs @@ -24,7 +24,7 @@ test!(add_filter_anchor { assert_matches!( pf.add_anchor(&anchor_name, pfctl::AnchorKind::Filter), - Err(pfctl::Error(pfctl::ErrorKind::StateAlreadyActive, _)) + Err(e) if e.kind() == pfctl::ErrorKind::StateAlreadyActive ); assert_matches!(pf.try_add_anchor(&anchor_name, pfctl::AnchorKind::Filter), Ok(())); }); @@ -41,7 +41,7 @@ test!(remove_filter_anchor { assert_matches!( pf.remove_anchor(&anchor_name, pfctl::AnchorKind::Filter), - Err(pfctl::Error(pfctl::ErrorKind::AnchorDoesNotExist, _)) + Err(e) if e.kind() == pfctl::ErrorKind::AnchorDoesNotExist ); assert_matches!(pf.try_remove_anchor(&anchor_name, pfctl::AnchorKind::Filter), Ok(())); }); diff --git a/tests/enable_disable.rs b/tests/enable_disable.rs index b940db5..ee0a71c 100644 --- a/tests/enable_disable.rs +++ b/tests/enable_disable.rs @@ -14,7 +14,7 @@ test!(enable_pf { pfcli::disable_firewall(); assert_matches!(pf.enable(), Ok(())); assert!(pfcli::is_enabled()); - assert_matches!(pf.enable(), Err(pfctl::Error(pfctl::ErrorKind::StateAlreadyActive, _))); + assert_matches!(pf.enable(), Err(e) if e.kind() == pfctl::ErrorKind::StateAlreadyActive); assert_matches!(pf.try_enable(), Ok(())); assert!(pfcli::is_enabled()); }); @@ -25,7 +25,7 @@ test!(disable_pf { pfcli::enable_firewall(); assert_matches!(pf.disable(), Ok(())); assert!(!pfcli::is_enabled()); - assert_matches!(pf.disable(), Err(pfctl::Error(pfctl::ErrorKind::StateAlreadyActive, _))); + assert_matches!(pf.disable(), Err(e) if e.kind() == pfctl::ErrorKind::StateAlreadyActive); assert_matches!(pf.try_disable(), Ok(())); assert!(!pfcli::is_enabled()); }); From 8b0fd8aab721eb900a7ee2156986b65eb0d675b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Linus=20F=C3=A4rnstrand?= Date: Wed, 17 Jul 2024 14:26:28 +0200 Subject: [PATCH 2/3] Remove actual error-chain dependency --- Cargo.lock | 75 +----------------------------------------------------- Cargo.toml | 1 - 2 files changed, 1 insertion(+), 75 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ca09fe1..c38d994 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8,34 +8,6 @@ version = "1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9e772942dccdf11b368c31e044e4fca9189f80a773d2f0808379de65894cbf57" -[[package]] -name = "backtrace" -version = "0.3.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ebbbf59b1c43eefa8c3ede390fcc36820b4999f7914104015be25025e0d62af2" -dependencies = [ - "backtrace-sys", - "cfg-if 0.1.0", - "libc", - "rustc-demangle", - "winapi", -] - -[[package]] -name = "backtrace-sys" -version = "0.1.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3216d6e2b2c36c648a78afab0fdcb124d5365f7eb9b0895eab395549d76280d2" -dependencies = [ - "libc", -] - -[[package]] -name = "cfg-if" -version = "0.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "de1e760d7b6535af4241fca8bd8adf68e2e7edacc6b29f5d399050c5e48cf88c" - [[package]] name = "cfg-if" version = "1.0.0" @@ -102,16 +74,6 @@ dependencies = [ "syn", ] -[[package]] -name = "error-chain" -version = "0.12.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2d2f06b9cac1506ece98fe3231e3cc9c4410ec3d5b1f24ae1c8946f0742cdefc" -dependencies = [ - "backtrace", - "version_check", -] - [[package]] name = "fnv" version = "1.0.6" @@ -124,7 +86,7 @@ version = "0.2.15" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c4567c8db10ae91089c99af84c68c38da3ec2f087c3f82960bcdbf3656b6f4d7" dependencies = [ - "cfg-if 1.0.0", + "cfg-if", "libc", "wasi", ] @@ -162,7 +124,6 @@ version = "0.4.6" dependencies = [ "assert_matches", "derive_builder", - "error-chain", "ioctl-sys", "ipnetwork", "libc", @@ -188,12 +149,6 @@ dependencies = [ "proc-macro2", ] -[[package]] -name = "rustc-demangle" -version = "0.1.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3058a43ada2c2d0b92b3ae38007a2d0fa5e9db971be260e0171408a4ff471c95" - [[package]] name = "scopeguard" version = "1.0.0" @@ -238,36 +193,8 @@ dependencies = [ "getrandom", ] -[[package]] -name = "version_check" -version = "0.9.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "49874b5167b65d7193b8aba1567f5c7d93d001cafc34600cee003eda787e483f" - [[package]] name = "wasi" version = "0.11.0+wasi-snapshot-preview1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9c8d87e72b64a3b4db28d11ce29237c246188f4f51057d65a7eab63b7987e423" - -[[package]] -name = "winapi" -version = "0.3.6" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "92c1eb33641e276cfa214a0522acad57be5c56b10cb348b3c5117db75f3ac4b0" -dependencies = [ - "winapi-i686-pc-windows-gnu", - "winapi-x86_64-pc-windows-gnu", -] - -[[package]] -name = "winapi-i686-pc-windows-gnu" -version = "0.4.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ac3b87c63620426dd9b991e5ce0329eff545bccbbb34f3be09ff6fb6ab51b7b6" - -[[package]] -name = "winapi-x86_64-pc-windows-gnu" -version = "0.4.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "712e227841d057c1ee1cd2fb22fa7e5a5461ae8e48fa2ca79ec42cfc1931183f" diff --git a/Cargo.toml b/Cargo.toml index 1b186d5..795dc70 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -16,7 +16,6 @@ travis-ci = { repository = "mullvad/pfctl-rs" } [dependencies] -error-chain = "0.12.4" ioctl-sys = "0.8.0" libc = "0.2.29" derive_builder = "0.9" From 3cca6515fcc2a9c74c643778ec7d1a138342c540 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Linus=20F=C3=A4rnstrand?= Date: Wed, 17 Jul 2024 14:27:12 +0200 Subject: [PATCH 3/3] Add to changelog that error-chain was removed --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e109a3..5d8977f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). `ipnetwork` is part of the public API. * Upgrade crate to Rust 2021 edition. * MSRV bumped to 1.69 due to use of `CStr::from_bytes_until_nul`. +* Replace `error-chain` generated errors with manually implemented error types. ### Removed * Remove `PoolAddrList::to_palist` from the public API. It should never have been exposed.