From 7ab12fcdbdbb3e1e7722eb6385a7d88813afd79e Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 3 Mar 2023 12:11:02 +1100 Subject: [PATCH 1/2] Use Error variants locally Add a `use Error::*` line to use error variants locally, while we are at it make usage uniform. --- src/lib.rs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 04e1e5589..f30a35971 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -679,14 +679,16 @@ pub enum Error { impl fmt::Display for Error { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + use Error::*; + match *self { - Error::MissingSeparator => write!(f, "missing human-readable separator, \"{}\"", SEP), - Error::InvalidChecksum => write!(f, "invalid checksum"), - Error::InvalidLength => write!(f, "invalid length"), - Error::InvalidChar(n) => write!(f, "invalid character (code={})", n), - Error::InvalidData(n) => write!(f, "invalid data point ({})", n), - Error::InvalidPadding => write!(f, "invalid padding"), - Error::MixedCase => write!(f, "mixed-case strings not allowed"), + MissingSeparator => write!(f, "missing human-readable separator, \"{}\"", SEP), + InvalidChecksum => write!(f, "invalid checksum"), + InvalidLength => write!(f, "invalid length"), + InvalidChar(n) => write!(f, "invalid character (code={})", n), + InvalidData(n) => write!(f, "invalid data point ({})", n), + InvalidPadding => write!(f, "invalid padding"), + MixedCase => write!(f, "mixed-case strings not allowed"), } } } @@ -694,7 +696,7 @@ impl fmt::Display for Error { #[cfg(feature = "std")] impl std::error::Error for Error { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - use self::Error::*; + use Error::*; match *self { MissingSeparator | InvalidChecksum | InvalidLength | InvalidChar(_) From 25462c9bbdc6684764b4d5dc3f259ecc4d2baf1f Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 3 Mar 2023 12:29:07 +1100 Subject: [PATCH 2/2] Implement TryFrom various integer types Currently we use `Error::InvalidData(u8)` to attempt to cache an invalid value encountered while converting to a `u8`. This is buggy because we cast to the `u8` to stash the value so we loose valuable bits. Implement `TryFrom` by doing: - Add an `Error::Overflow` variant. - Use `Error::Overflow` for conversion _outside_ of `TryFrom` impls - Add a new `TryFromError` and nest it in `Error::TryFrom` - Implement `TryFrom for u5` impls for all standard integer types. Inspired by issue #60 and the stdlib macros of the same name. --- src/error.rs | 31 ++++++++++++ src/lib.rs | 136 ++++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 149 insertions(+), 18 deletions(-) create mode 100644 src/error.rs diff --git a/src/error.rs b/src/error.rs new file mode 100644 index 000000000..1b936c60c --- /dev/null +++ b/src/error.rs @@ -0,0 +1,31 @@ +// Written by the Rust Bitcoin developers. +// SPDX-License-Identifier: CC0-1.0 + +// TODO: We can get this from `bitcoin-internals` crate once that is released. + +//! # Error +//! +//! Error handling macros and helpers. +//! + +/// Formats error. +/// +/// If `std` feature is OFF appends error source (delimited by `: `). We do this because +/// `e.source()` is only available in std builds, without this macro the error source is lost for +/// no-std builds. +#[macro_export] +macro_rules! write_err { + ($writer:expr, $string:literal $(, $args:expr)*; $source:expr) => { + { + #[cfg(feature = "std")] + { + let _ = &$source; // Prevents clippy warnings. + write!($writer, $string $(, $args)*) + } + #[cfg(not(feature = "std"))] + { + write!($writer, concat!($string, ": {}") $(, $args)*, $source) + } + } + } +} diff --git a/src/lib.rs b/src/lib.rs index f30a35971..a59e499a4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -74,6 +74,8 @@ use core::{fmt, mem}; #[cfg(feature = "std")] use std::borrow::Cow; +mod error; + /// Integer in the range `0..32` #[derive(PartialEq, Eq, Debug, Copy, Clone, Default, PartialOrd, Ord, Hash)] #[allow(non_camel_case_types)] @@ -91,18 +93,56 @@ impl From for u8 { fn from(v: u5) -> u8 { v.0 } } -impl TryFrom for u5 { - type Error = Error; - - /// Errors if `value` is out of range. - fn try_from(value: u8) -> Result { - if value > 31 { - Err(Error::InvalidData(value)) - } else { - Ok(u5(value)) - } +macro_rules! impl_try_from_upper_bounded { + ($($ty:ident)+) => { + $( + impl TryFrom<$ty> for u5 { + type Error = TryFromIntError; + + /// Tries to create the target number type from a source number type. + /// + /// # Errors + /// + /// Returns an error if `value` overflows a `u5`. + fn try_from(value: $ty) -> Result { + if value > 31 { + Err(TryFromIntError::PosOverflow) + } else { + let x = u8::try_from(value).expect("within range"); + Ok(u5(x)) + } + } + } + )+ } } +macro_rules! impl_try_from_both_bounded { + ($($ty:ident)+) => { + $( + impl TryFrom<$ty> for u5 { + type Error = TryFromIntError; + + /// Tries to create the target number type from a source number type. + /// + /// # Errors + /// + /// Returns an error if `value` is outside of the range of a `u5`. + fn try_from(value: $ty) -> Result { + if value < 0 { + Err(TryFromIntError::NegOverflow) + } else if value > 31 { + Err(TryFromIntError::PosOverflow) + } else { + let x = u8::try_from(value).expect("within range"); + Ok(u5(x)) + } + } + } + )+ + } +} +impl_try_from_upper_bounded!(u8 u16 u32 u64 u128); +impl_try_from_both_bounded!(i8 i16 i32 i64 i128); impl AsRef for u5 { fn as_ref(&self) -> &u8 { &self.0 } @@ -340,7 +380,10 @@ impl> CheckBase32> for T { type Err = Error; fn check_base32(self) -> Result, Self::Err> { - self.as_ref().iter().map(|x| u5::try_from(*x)).collect::, Error>>() + self.as_ref() + .iter() + .map(|x| u5::try_from(*x).map_err(Error::TryFrom)) + .collect::, Error>>() } } @@ -669,12 +712,14 @@ pub enum Error { InvalidLength, /// Some part of the string contains an invalid character. InvalidChar(char), - /// Some part of the data has an invalid value. - InvalidData(u8), /// The bit conversion failed due to a padding issue. InvalidPadding, /// The whole string must be of one case. MixedCase, + /// Attempted to convert a value which overflows a `u5`. + Overflow, + /// Conversion to u5 failed. + TryFrom(TryFromIntError), } impl fmt::Display for Error { @@ -686,9 +731,10 @@ impl fmt::Display for Error { InvalidChecksum => write!(f, "invalid checksum"), InvalidLength => write!(f, "invalid length"), InvalidChar(n) => write!(f, "invalid character (code={})", n), - InvalidData(n) => write!(f, "invalid data point ({})", n), InvalidPadding => write!(f, "invalid padding"), MixedCase => write!(f, "mixed-case strings not allowed"), + TryFrom(ref e) => write_err!(f, "conversion to u5 failed"; e), + Overflow => write!(f, "attempted to convert a value which overflows a u5"), } } } @@ -699,12 +745,54 @@ impl std::error::Error for Error { use Error::*; match *self { + TryFrom(ref e) => Some(e), MissingSeparator | InvalidChecksum | InvalidLength | InvalidChar(_) - | InvalidData(_) | InvalidPadding | MixedCase => None, + | InvalidPadding | MixedCase | Overflow => None, + } + } +} + +impl From for Error { + fn from(e: TryFromIntError) -> Self { Error::TryFrom(e) } +} + +/// Error return when TryFrom fails for T -> u5 conversion. +#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)] +pub enum TryFromIntError { + /// Attempted to convert a negative value to a `u5`. + NegOverflow, + /// Attempted to convert a value which overflows a `u5`. + PosOverflow, +} + +impl fmt::Display for TryFromIntError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + use TryFromIntError::*; + + match *self { + NegOverflow => write!(f, "attempted to convert a negative value to a u5"), + PosOverflow => write!(f, "attempted to convert a value which overflows a u5"), } } } +#[cfg(feature = "std")] +impl std::error::Error for TryFromIntError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + use TryFromIntError::*; + + match *self { + NegOverflow | PosOverflow => None, + } + } +} + +// impl From for Error { +// fn from(e: convert::Error) -> Self { +// Error::InvalidData(e) +// } +// } + /// Convert between bit sizes /// /// # Errors @@ -738,7 +826,7 @@ where let v: u32 = u32::from(Into::::into(*value)); if (v >> from) != 0 { // Input value exceeds `from` bit size - return Err(Error::InvalidData(v as u8)); + return Err(Error::Overflow); } acc = (acc << from) | v; bits += from; @@ -885,7 +973,7 @@ mod tests { // Set of [data, from_bits, to_bits, pad, expected error] let tests: Vec<(Vec, u32, u32, bool, Error)> = vec![ (vec![0xff], 8, 5, false, Error::InvalidPadding), - (vec![0x02], 1, 1, true, Error::InvalidData(0x02)), + (vec![0x02], 1, 1, true, Error::Overflow), ]; for t in tests { let (data, from_bits, to_bits, pad, expected_error) = t; @@ -918,7 +1006,10 @@ mod tests { assert!([0u8, 1, 2, 30, 31, 255].check_base32().is_err()); assert!([1u8, 2, 3, 4].check_base32().is_ok()); - assert_eq!([30u8, 31, 35, 20].check_base32(), Err(Error::InvalidData(35))); + assert_eq!( + [30u8, 31, 35, 20].check_base32(), + Err(Error::TryFrom(TryFromIntError::PosOverflow)) + ) } #[test] @@ -1028,4 +1119,13 @@ mod tests { assert_eq!(encoded_str, "hrp1qqqq40atq3"); } + + #[test] + fn try_from_err() { + assert!(u5::try_from(32_u8).is_err()); + assert!(u5::try_from(32_u16).is_err()); + assert!(u5::try_from(32_u32).is_err()); + assert!(u5::try_from(32_u64).is_err()); + assert!(u5::try_from(32_u128).is_err()); + } }