From e16a49472e9920cc09eff56cc3a73b9e85ea38dc Mon Sep 17 00:00:00 2001 From: Pierre Chifflier Date: Thu, 14 Mar 2024 09:33:57 +0100 Subject: [PATCH] Improve debug+trace, and require Display+Debug on errors --- src/asn1_types/integer.rs | 48 +++++--- src/asn1_types/sequence/sequence_of.rs | 30 +++-- src/asn1_types/sequence/vec.rs | 54 ++++++--- src/asn1_types/set/btreeset.rs | 53 ++++++--- src/asn1_types/set/hashset.rs | 36 +++--- src/asn1_types/set/set_of.rs | 30 +++-- src/debug.rs | 154 ++++++++++++++++++------- src/traits.rs | 42 +++---- tests/der.rs | 12 ++ 9 files changed, 304 insertions(+), 155 deletions(-) diff --git a/src/asn1_types/integer.rs b/src/asn1_types/integer.rs index 59f846a..cb463e4 100644 --- a/src/asn1_types/integer.rs +++ b/src/asn1_types/integer.rs @@ -96,19 +96,26 @@ macro_rules! impl_int { type Error = Error; fn try_from(any: &'b Any<'a>) -> Result { - any.tag().assert_eq(Self::TAG)?; - any.header.assert_primitive()?; - let uint = if is_highest_bit_set(any.as_bytes()) { - <$uint>::from_be_bytes(decode_array_int(&any)?) - } else { - // read as uint, but check if the value will fit in a signed integer - let u = <$uint>::from_be_bytes(decode_array_uint(&any)?); - if u > <$int>::MAX as $uint { - return Err(Error::IntegerTooLarge); - } - u - }; - Ok(uint as $int) + $crate::debug::trace_generic( + core::any::type_name::<$int>(), + "Conversion to int", + |any| { + any.tag().assert_eq(Self::TAG)?; + any.header.assert_primitive()?; + let uint = if is_highest_bit_set(any.as_bytes()) { + <$uint>::from_be_bytes(decode_array_int(&any)?) + } else { + // read as uint, but check if the value will fit in a signed integer + let u = <$uint>::from_be_bytes(decode_array_uint(&any)?); + if u > <$int>::MAX as $uint { + return Err(Error::IntegerTooLarge); + } + u + }; + Ok(uint as $int) + }, + any, + ) } } @@ -162,10 +169,17 @@ macro_rules! impl_uint { type Error = Error; fn try_from(any: &'b Any<'a>) -> Result { - any.tag().assert_eq(Self::TAG)?; - any.header.assert_primitive()?; - let result = Self::from_be_bytes(decode_array_uint(any)?); - Ok(result) + $crate::debug::trace_generic( + core::any::type_name::<$ty>(), + "Conversion to uint", + |any| { + any.tag().assert_eq(Self::TAG)?; + any.header.assert_primitive()?; + let result = Self::from_be_bytes(decode_array_uint(any)?); + Ok(result) + }, + any, + ) } } impl CheckDerConstraints for $ty { diff --git a/src/asn1_types/sequence/sequence_of.rs b/src/asn1_types/sequence/sequence_of.rs index c4c81d2..d8238da 100644 --- a/src/asn1_types/sequence/sequence_of.rs +++ b/src/asn1_types/sequence/sequence_of.rs @@ -1,10 +1,11 @@ use crate::*; use alloc::vec::Vec; use core::convert::TryFrom; +use core::fmt::{Debug, Display}; use core::iter::FromIterator; use core::ops::{Deref, DerefMut}; -use self::debug::trace; +use self::debug::{trace, trace_generic}; /// The `SEQUENCE OF` object is an ordered list of homogeneous types. /// @@ -130,18 +131,25 @@ where impl<'a, T, E> FromDer<'a, E> for SequenceOf where T: FromDer<'a, E>, - E: From, + E: From + Display + Debug, { fn from_der(bytes: &'a [u8]) -> ParseResult { - let (rem, any) = - trace(core::any::type_name::(), parse_der_any, bytes).map_err(Err::convert)?; - any.header - .assert_tag(Self::TAG) - .map_err(|e| nom::Err::Error(e.into()))?; - let items = SequenceIterator::::new(any.data) - .collect::, E>>() - .map_err(nom::Err::Error)?; - Ok((rem, SequenceOf::new(items))) + trace_generic( + core::any::type_name::(), + "SequenceOf::from_der", + |bytes| { + let (rem, any) = trace(core::any::type_name::(), parse_der_any, bytes) + .map_err(Err::convert)?; + any.header + .assert_tag(Self::TAG) + .map_err(|e| nom::Err::Error(e.into()))?; + let items = SequenceIterator::::new(any.data) + .collect::, E>>() + .map_err(nom::Err::Error)?; + Ok((rem, SequenceOf::new(items))) + }, + bytes, + ) } } diff --git a/src/asn1_types/sequence/vec.rs b/src/asn1_types/sequence/vec.rs index 6d2f8d4..ca4869c 100644 --- a/src/asn1_types/sequence/vec.rs +++ b/src/asn1_types/sequence/vec.rs @@ -1,8 +1,9 @@ use crate::*; use alloc::vec::Vec; use core::convert::TryFrom; +use core::fmt::Debug; -use self::debug::trace; +use self::debug::{trace, trace_generic}; // // XXX this compiles but requires bound TryFrom :/ // impl<'a, 'b, T> TryFrom<&'b Any<'a>> for Vec @@ -51,10 +52,26 @@ where type Error = Error; fn try_from(any: Any<'a>) -> Result { - any.tag().assert_eq(Self::TAG)?; - any.header.assert_constructed()?; - let items = SetIterator::::new(any.data).collect::>>()?; - Ok(items) + trace_generic( + core::any::type_name::(), + "T::from(Any)", + |any| { + any.tag().assert_eq(Self::TAG)?; + any.header.assert_constructed()?; + let items = SetIterator::::new(any.data) + .collect::>>() + .map_err(|e| { + debug_eprintln!( + core::any::type_name::(), + "≠ {}", + "Conversion from Any failed".red() + ); + e + })?; + Ok(items) + }, + any, + ) } } @@ -93,18 +110,25 @@ impl Tagged for Vec { impl<'a, T, E> FromDer<'a, E> for Vec where T: FromDer<'a, E>, - E: From, + E: From + Debug, { fn from_der(bytes: &'a [u8]) -> ParseResult { - let (rem, any) = - trace(core::any::type_name::(), parse_der_any, bytes).map_err(Err::convert)?; - any.header - .assert_tag(Self::TAG) - .map_err(|e| nom::Err::Error(e.into()))?; - let v = SequenceIterator::::new(any.data) - .collect::, E>>() - .map_err(nom::Err::Error)?; - Ok((rem, v)) + trace_generic( + core::any::type_name::(), + "Sequence::from_der", + |bytes| { + let (rem, any) = trace(core::any::type_name::(), parse_der_any, bytes) + .map_err(Err::convert)?; + any.header + .assert_tag(Self::TAG) + .map_err(|e| nom::Err::Error(e.into()))?; + let v = SequenceIterator::::new(any.data) + .collect::, E>>() + .map_err(nom::Err::Error)?; + Ok((rem, v)) + }, + bytes, + ) } } diff --git a/src/asn1_types/set/btreeset.rs b/src/asn1_types/set/btreeset.rs index 83a310e..f00df88 100644 --- a/src/asn1_types/set/btreeset.rs +++ b/src/asn1_types/set/btreeset.rs @@ -1,8 +1,8 @@ use crate::*; use alloc::collections::BTreeSet; -use core::convert::TryFrom; +use core::{convert::TryFrom, fmt::Debug}; -use self::debug::trace; +use self::debug::{trace, trace_generic}; impl Tagged for BTreeSet { const TAG: Tag = Tag::Set; @@ -16,10 +16,18 @@ where type Error = Error; fn try_from(any: Any<'a>) -> Result { - any.tag().assert_eq(Self::TAG)?; - any.header.assert_constructed()?; - let items = SetIterator::::new(any.data).collect::>>()?; - Ok(items) + trace_generic( + core::any::type_name::(), + "BTreeSet::from_der", + |any| { + any.tag().assert_eq(Self::TAG)?; + any.header.assert_constructed()?; + let items = + SetIterator::::new(any.data).collect::>>()?; + Ok(items) + }, + any, + ) } } @@ -43,21 +51,28 @@ impl<'a, T, E> FromDer<'a, E> for BTreeSet where T: FromDer<'a, E>, T: Ord, - E: From, + E: From + Debug, { fn from_der(bytes: &'a [u8]) -> ParseResult<'a, Self, E> { - let (rem, any) = - trace(core::any::type_name::(), Any::from_der, bytes).map_err(Err::convert)?; - any.tag() - .assert_eq(Self::TAG) - .map_err(|e| nom::Err::Error(e.into()))?; - any.header - .assert_constructed() - .map_err(|e| nom::Err::Error(e.into()))?; - let items = SetIterator::::new(any.data) - .collect::, E>>() - .map_err(nom::Err::Error)?; - Ok((rem, items)) + trace_generic( + core::any::type_name::(), + "BTreeSet::from_der", + |bytes| { + let (rem, any) = trace(core::any::type_name::(), Any::from_der, bytes) + .map_err(Err::convert)?; + any.tag() + .assert_eq(Self::TAG) + .map_err(|e| nom::Err::Error(e.into()))?; + any.header + .assert_constructed() + .map_err(|e| nom::Err::Error(e.into()))?; + let items = SetIterator::::new(any.data) + .collect::, E>>() + .map_err(nom::Err::Error)?; + Ok((rem, items)) + }, + bytes, + ) } } diff --git a/src/asn1_types/set/hashset.rs b/src/asn1_types/set/hashset.rs index f72c9d0..1389ac0 100644 --- a/src/asn1_types/set/hashset.rs +++ b/src/asn1_types/set/hashset.rs @@ -1,10 +1,11 @@ #![cfg(feature = "std")] use crate::*; +use core::fmt::Debug; use std::collections::HashSet; use std::convert::TryFrom; use std::hash::Hash; -use self::debug::trace; +use self::debug::{trace, trace_generic}; impl Tagged for HashSet { const TAG: Tag = Tag::Set; @@ -45,21 +46,28 @@ impl<'a, T, E> FromDer<'a, E> for HashSet where T: FromDer<'a, E>, T: Hash + Eq, - E: From, + E: From + Debug, { fn from_der(bytes: &'a [u8]) -> ParseResult<'a, Self, E> { - let (rem, any) = - trace(core::any::type_name::(), Any::from_der, bytes).map_err(Err::convert)?; - any.tag() - .assert_eq(Self::TAG) - .map_err(|e| nom::Err::Error(e.into()))?; - any.header - .assert_constructed() - .map_err(|e| nom::Err::Error(e.into()))?; - let items = SetIterator::::new(any.data) - .collect::, E>>() - .map_err(nom::Err::Error)?; - Ok((rem, items)) + trace_generic( + core::any::type_name::(), + "BTreeSet::from_der", + |bytes| { + let (rem, any) = trace(core::any::type_name::(), Any::from_der, bytes) + .map_err(Err::convert)?; + any.tag() + .assert_eq(Self::TAG) + .map_err(|e| nom::Err::Error(e.into()))?; + any.header + .assert_constructed() + .map_err(|e| nom::Err::Error(e.into()))?; + let items = SetIterator::::new(any.data) + .collect::, E>>() + .map_err(nom::Err::Error)?; + Ok((rem, items)) + }, + bytes, + ) } } diff --git a/src/asn1_types/set/set_of.rs b/src/asn1_types/set/set_of.rs index 9863a06..a71eee9 100644 --- a/src/asn1_types/set/set_of.rs +++ b/src/asn1_types/set/set_of.rs @@ -1,10 +1,11 @@ use crate::*; use alloc::vec::Vec; use core::convert::TryFrom; +use core::fmt::{Debug, Display}; use core::iter::FromIterator; use core::ops::{Deref, DerefMut}; -use self::debug::trace; +use self::debug::{trace, trace_generic}; /// The `SET OF` object is an unordered list of homogeneous types. /// @@ -130,18 +131,25 @@ where impl<'a, T, E> FromDer<'a, E> for SetOf where T: FromDer<'a, E>, - E: From, + E: From + Display + Debug, { fn from_der(bytes: &'a [u8]) -> ParseResult { - let (rem, any) = - trace(core::any::type_name::(), Any::from_der, bytes).map_err(Err::convert)?; - any.header - .assert_tag(Self::TAG) - .map_err(|e| nom::Err::Error(e.into()))?; - let items = SetIterator::::new(any.data) - .collect::, E>>() - .map_err(nom::Err::Error)?; - Ok((rem, SetOf::new(items))) + trace_generic( + core::any::type_name::(), + "SetOf::from_der", + |bytes| { + let (rem, any) = trace(core::any::type_name::(), Any::from_der, bytes) + .map_err(Err::convert)?; + any.header + .assert_tag(Self::TAG) + .map_err(|e| nom::Err::Error(e.into()))?; + let items = SetIterator::::new(any.data) + .collect::, E>>() + .map_err(nom::Err::Error)?; + Ok((rem, SetOf::new(items))) + }, + bytes, + ) } } diff --git a/src/debug.rs b/src/debug.rs index 750f568..aa0c707 100644 --- a/src/debug.rs +++ b/src/debug.rs @@ -5,6 +5,7 @@ macro_rules! debug_eprintln { ($msg: expr, $( $args:expr ),* ) => { #[cfg(feature = "debug")] { + use colored::Colorize; let s = $msg.to_string().green(); eprintln!("{} {}", s, format!($($args),*)); } @@ -16,6 +17,7 @@ macro_rules! trace_eprintln { ($msg: expr, $( $args:expr ),* ) => { #[cfg(feature = "trace")] { + use colored::Colorize; let s = $msg.to_string().green(); eprintln!("{} {}", s, format!($($args),*)); } @@ -34,6 +36,34 @@ fn eprintln_hex_dump(bytes: &[u8], max_len: usize) { } } +#[cfg(not(feature = "debug"))] +#[inline] +pub fn trace_generic(_msg: &str, _fname: &str, f: F, input: I) -> Result +where + F: Fn(I) -> Result, +{ + f(input) +} + +#[cfg(feature = "debug")] +pub fn trace_generic(msg: &str, fname: &str, f: F, input: I) -> Result +where + F: Fn(I) -> Result, + E: core::fmt::Display, +{ + trace_eprintln!(msg, "⤷ {}", fname); + let output = f(input); + match &output { + Err(e) => { + debug_eprintln!(msg, "↯ {} failed: {}", fname, e.to_string().red()); + } + _ => { + debug_eprintln!(msg, "⤶ {}", fname); + } + } + output +} + #[cfg(not(feature = "debug"))] #[inline] pub fn trace<'a, T, E, F>(_msg: &str, f: F, input: &'a [u8]) -> ParseResult<'a, T, E> @@ -47,10 +77,7 @@ where pub fn trace<'a, T, E, F>(msg: &str, f: F, input: &'a [u8]) -> ParseResult<'a, T, E> where F: Fn(&'a [u8]) -> ParseResult<'a, T, E>, - E: core::fmt::Debug, { - use colored::Colorize; - trace_eprintln!( msg, "⤷ input (len={}, type={})", @@ -67,26 +94,69 @@ where _rem.len() ); } - Err(e) => { - debug_eprintln!(msg, "↯ Parsing failed: {}", e.to_string().red()); + Err(_) => { + // NOTE: we do not need to print error, caller should print it + debug_eprintln!(msg, "↯ Parsing failed at location:"); eprintln_hex_dump(input, 16); } } res } +#[cfg(feature = "debug")] #[cfg(test)] mod tests { - use crate::{Any, FromDer}; + + use std::collections::HashSet; + + use crate::*; + use alloc::collections::BTreeSet; use hex_literal::hex; - #[cfg(feature = "debug")] + #[test] + fn debug_from_ber_any() { + assert!(Any::from_ber(&hex!("01 01 ff")).is_ok()); + } + + #[test] + fn debug_from_ber_failures() { + // wrong type + eprintln!("--"); + assert!(>::from_ber(&hex!("02 01 00")).is_err()); + } + + #[test] + fn debug_from_ber_sequence_indefinite() { + let input = &hex!("30 80 02 03 01 00 01 00 00"); + let (rem, result) = Sequence::from_ber(input).expect("parsing failed"); + assert_eq!(result.as_ref(), &input[2..7]); + assert_eq!(rem, &[]); + eprintln!("--"); + let (rem, result) = >::from_ber(input).expect("parsing failed"); + assert_eq!(&result, &[65537]); + assert_eq!(rem, &[]); + } + + #[test] + fn debug_from_ber_sequence_of() { + // parsing failure (wrong type) + let input = &hex!("30 03 01 01 00"); + eprintln!("--"); + let _ = >::from_ber(input).expect_err("parsing should fail"); + eprintln!("--"); + let _ = >::from_ber(input).expect_err("parsing should fail"); + } + + #[test] + fn debug_from_ber_u32() { + assert!(u32::from_ber(&hex!("02 01 01")).is_ok()); + } + #[test] fn debug_from_der_any() { assert!(Any::from_der(&hex!("01 01 ff")).is_ok()); } - #[cfg(feature = "debug")] #[test] fn debug_from_der_failures() { use crate::Sequence; @@ -105,17 +175,25 @@ mod tests { let _ = Sequence::from_der(&hex!("30 81 04 00 00")); } - #[cfg(feature = "debug")] #[test] fn debug_from_der_sequence() { // parsing OK, recursive - let input = &hex!("30 05 02 03 01 00 01"); + let input = &hex!("30 08 02 03 01 00 01 02 01 01"); let (rem, result) = >::from_der(input).expect("parsing failed"); - assert_eq!(&result, &[65537]); + assert_eq!(&result, &[65537, 1]); assert_eq!(rem, &[]); } - #[cfg(feature = "debug")] + #[test] + fn debug_from_der_sequence_fail() { + // tag is wrong + let input = &hex!("31 03 01 01 44"); + let _ = >::from_der(input).expect_err("parsing should fail"); + // sequence is ok but contraint fails on element + let input = &hex!("30 03 01 01 44"); + let _ = >::from_der(input).expect_err("parsing should fail"); + } + #[test] fn debug_from_der_sequence_of() { use crate::SequenceOf; @@ -127,7 +205,13 @@ mod tests { let _ = >::from_der(input).expect_err("parsing should fail"); } - #[cfg(feature = "debug")] + #[test] + fn debug_from_der_set_fail() { + // set is ok but contraint fails on element + let input = &hex!("31 03 01 01 44"); + let _ = >::from_der(input).expect_err("parsing should fail"); + } + #[test] fn debug_from_der_set_of() { use crate::SetOf; @@ -139,40 +223,22 @@ mod tests { let _ = >::from_der(input).expect_err("parsing should fail"); eprintln!("--"); let _ = >::from_der(input).expect_err("parsing should fail"); + eprintln!("--"); + let _ = >::from_der(input).expect_err("parsing should fail"); } - /// Check that it is possible to implement an error without fmt::Debug - #[cfg(not(feature = "debug"))] #[test] - fn from_der_error_not_impl_debug() { - use crate::{CheckDerConstraints, DerAutoDerive}; - use core::convert::TryFrom; - - struct MyError; - - struct A; - - impl CheckDerConstraints for A { - fn check_constraints(_any: &Any) -> crate::Result<()> { - Ok(()) - } - } - impl<'a> TryFrom> for A { - type Error = MyError; - - fn try_from(_value: Any<'a>) -> Result { - Ok(A) - } - } - impl DerAutoDerive for A {} - - impl From for MyError { - fn from(_value: crate::Error) -> Self { - Self - } - } + fn debug_from_der_string_ok() { + let input = &hex!("0c 0a 53 6f 6d 65 2d 53 74 61 74 65"); + let (rem, result) = Utf8String::from_der(input).expect("parsing failed"); + assert_eq!(result.as_ref(), "Some-State"); + assert_eq!(rem, &[]); + } - let res = A::from_der(&hex!("02 01 00")); - assert!(res.is_ok()); + #[test] + fn debug_from_der_string_fail() { + // wrong charset + let input = &hex!("12 03 41 42 43"); + let _ = NumericString::from_der(input).expect_err("parsing should fail"); } } diff --git a/src/traits.rs b/src/traits.rs index a1829af..c0666bc 100644 --- a/src/traits.rs +++ b/src/traits.rs @@ -1,9 +1,8 @@ -use crate::debug::trace; -use crate::{debug_eprintln, error::*, parse_der_any}; -use crate::{Any, Class, Explicit, Implicit, Tag, TaggedParser}; -#[cfg(feature = "debug")] -use colored::Colorize; +use crate::debug::{trace, trace_generic}; +use crate::error::*; +use crate::{parse_der_any, Any, Class, Explicit, Implicit, Tag, TaggedParser}; use core::convert::{TryFrom, TryInto}; +use core::fmt::{Debug, Display}; #[cfg(feature = "std")] use std::io::Write; @@ -181,27 +180,22 @@ where T: TryFrom, Error = E>, T: CheckDerConstraints, T: DerAutoDerive, - E: From, + E: From + Display + Debug, { fn from_der(bytes: &'a [u8]) -> ParseResult { - let (i, any) = - trace(core::any::type_name::(), parse_der_any, bytes).map_err(nom::Err::convert)?; - ::check_constraints(&any).map_err(|e| { - debug_eprintln!( - std::any::type_name::(), - "≠ Checking DER constraints failed:\n {}", - e.to_string().red() - ); - nom::Err::Error(e.into()) - })?; - let result = any - .try_into() - .map_err(|e| { - debug_eprintln!(core::any::type_name::(), "≠ Conversion from Any failed"); - e - }) - .map_err(nom::Err::Error)?; - Ok((i, result)) + trace_generic( + core::any::type_name::(), + "Any::from_der", + |bytes| { + let (i, any) = trace(core::any::type_name::(), parse_der_any, bytes) + .map_err(nom::Err::convert)?; + ::check_constraints(&any) + .map_err(|e| nom::Err::Error(e.into()))?; + let result = any.try_into().map_err(nom::Err::Error)?; + Ok((i, result)) + }, + bytes, + ) } } diff --git a/tests/der.rs b/tests/der.rs index 5fd87d7..5f6b682 100644 --- a/tests/der.rs +++ b/tests/der.rs @@ -205,6 +205,18 @@ fn from_der_null() { assert_eq!(rem, &[0xff, 0xff]); } +#[test] +fn from_der_numericstring() { + // + let input = &hex!("12 03 31 32 33"); + let (rem, result) = NumericString::from_der(input).expect("parsing failed"); + assert_eq!(result.as_ref(), "123"); + assert_eq!(rem, &[]); + // wrong charset + let input = &hex!("12 03 41 42 43"); + let _ = NumericString::from_der(input).expect_err("parsing should fail"); +} + #[test] fn from_der_octetstring() { // coverage