Make errors/error reporting more lightweight

crates/proto/src/error.rs

@@ -26,6 +26,7 @@ use ring::error::Unspecified;
 use thiserror::Error;
 
 use crate::rr::{Name, RecordType};
+use crate::serialize::binary::DecodeError;
 
 #[cfg(feature = "backtrace")]
 lazy_static! {
@@ -225,7 +226,7 @@ pub enum ProtoErrorKind {
 /// The error type for errors that get returned in the crate
 #[derive(Error, Clone, Debug)]
 pub struct ProtoError {
-    kind: ProtoErrorKind,
+    kind: Box<ProtoErrorKind>,
     #[cfg(feature = "backtrace")]
     backtrack: Option<ExtBacktrace>,
 }
@@ -238,7 +239,7 @@ impl ProtoError {
 
     /// If this is a ProtoErrorKind::Busy
     pub fn is_busy(&self) -> bool {
-        matches!(self.kind, ProtoErrorKind::Busy)
+        matches!(*self.kind, ProtoErrorKind::Busy)
     }
 }
 
@@ -262,13 +263,31 @@ impl fmt::Display for ProtoError {
 impl From<ProtoErrorKind> for ProtoError {
     fn from(kind: ProtoErrorKind) -> ProtoError {
         ProtoError {
-            kind,
+            kind: Box::new(kind),
             #[cfg(feature = "backtrace")]
             backtrack: trace!(),
         }
     }
 }
 
+impl From<DecodeError> for ProtoError {
+    fn from(err: DecodeError) -> ProtoError {
+        match err {
+            DecodeError::PointerNotPriorToLabel { idx, ptr } => {
+                ProtoErrorKind::PointerNotPriorToLabel { idx, ptr }
+            }
+            DecodeError::LabelBytesTooLong(len) => ProtoErrorKind::LabelBytesTooLong(len),
+            DecodeError::UnrecognizedLabelCode(code) => ProtoErrorKind::UnrecognizedLabelCode(code),
+            DecodeError::DomainNameTooLong(len) => ProtoErrorKind::DomainNameTooLong(len),
+            DecodeError::LabelOverlapsWithOther { label, other } => {
+                ProtoErrorKind::LabelOverlapsWithOther { label, other }
+            }
+            _ => ProtoErrorKind::Msg(err.to_string()),
+        }
+        .into()
+    }
+}
+
 impl From<&'static str> for ProtoError {
     fn from(msg: &'static str) -> ProtoError {
         ProtoErrorKind::Message(msg).into()

crates/proto/src/rr/domain/name.rs

@@ -1124,7 +1124,7 @@ fn read_inner(
     label_data: &mut TinyVec<[u8; 32]>,
     label_ends: &mut TinyVec<[u8; 24]>,
     max_idx: Option<usize>,
-) -> ProtoResult<()> {
+) -> Result<(), DecodeError> {
     let mut state: LabelParseState = LabelParseState::LabelLengthOrPointer;
     let name_start = decoder.index();
 
@@ -1137,18 +1137,17 @@ fn read_inner(
         // this protects against overlapping labels
         if let Some(max_idx) = max_idx {
             if decoder.index() >= max_idx {
-                return Err(ProtoErrorKind::LabelOverlapsWithOther {
+                return Err(DecodeError::LabelOverlapsWithOther {
                     label: name_start,
                     other: max_idx,
-                }
-                .into());
+                });
             }
         }
 
         // enforce max length of name
         let cur_len = label_data.len() + label_ends.len();
         if cur_len > 255 {
-            return Err(ProtoErrorKind::DomainNameTooLong(cur_len).into());
+            return Err(DecodeError::DomainNameTooLong(cur_len));
         }
 
         state = match state {
@@ -1161,15 +1160,15 @@ fn read_inner(
                     Some(0) | None => LabelParseState::Root,
                     Some(byte) if byte & 0b1100_0000 == 0b1100_0000 => LabelParseState::Pointer,
                     Some(byte) if byte & 0b1100_0000 == 0b0000_0000 => LabelParseState::Label,
-                    Some(byte) => return Err(ProtoErrorKind::UnrecognizedLabelCode(byte).into()),
+                    Some(byte) => return Err(DecodeError::UnrecognizedLabelCode(byte)),
                 }
             }
             // labels must have a maximum length of 63
             LabelParseState::Label => {
                 let label = decoder
-                    .read_character_data_max(Some(63))?
+                    .read_character_data()?
                     .verify_unwrap(|l| l.len() <= 63)
-                    .map_err(|_| ProtoError::from("label exceeds maximum length of 63"))?;
+                    .map_err(|l| DecodeError::LabelBytesTooLong(l.len()))?;
 
                 label_data.extend_from_slice(label);
                 label_ends.push(label_data.len() as u8);
@@ -1210,11 +1209,9 @@ fn read_inner(
                         // all labels must appear "prior" to this Name
                         (*ptr as usize) < name_start
                     })
-                    .map_err(|e| {
-                        ProtoError::from(ProtoErrorKind::PointerNotPriorToLabel {
-                            idx: pointer_location,
-                            ptr: e,
-                        })
+                    .map_err(|e| DecodeError::PointerNotPriorToLabel {
+                        idx: pointer_location,
+                        ptr: e,
                     })?;
 
                 let mut pointer = decoder.clone(location);

crates/proto/src/rr/record_type.rs

@@ -249,12 +249,12 @@ impl BinEncodable for RecordType {
 
 impl<'r> BinDecodable<'r> for RecordType {
     fn read(decoder: &mut BinDecoder<'_>) -> ProtoResult<Self> {
-        decoder
+        Ok(decoder
             .read_u16()
             .map(
                 Restrict::unverified, /*RecordType is safe with any u16*/
             )
-            .map(Self::from)
+            .map(Self::from)?)
     }
 }
 

crates/proto/src/serialize/binary/bin_tests.rs

@@ -58,7 +58,7 @@ fn get_u16_data() -> Vec<(u16, Vec<u8>)> {
 #[test]
 fn read_u16() {
     test_read_data_set(get_u16_data(), |mut d| {
-        d.read_u16().map(Restrict::unverified)
+        d.read_u16().map(Restrict::unverified).map_err(Into::into)
     });
 }
 
@@ -83,7 +83,7 @@ fn get_i32_data() -> Vec<(i32, Vec<u8>)> {
 #[test]
 fn read_i32() {
     test_read_data_set(get_i32_data(), |mut d| {
-        d.read_i32().map(Restrict::unverified)
+        d.read_i32().map(Restrict::unverified).map_err(Into::into)
     });
 }
 
@@ -109,7 +109,7 @@ fn get_u32_data() -> Vec<(u32, Vec<u8>)> {
 #[test]
 fn read_u32() {
     test_read_data_set(get_u32_data(), |mut d| {
-        d.read_u32().map(Restrict::unverified)
+        d.read_u32().map(Restrict::unverified).map_err(Into::into)
     });
 }
 

crates/proto/src/serialize/binary/decoder.rs

@@ -14,8 +14,8 @@
  * limitations under the License.
  */
 
-use crate::error::{ProtoError, ProtoErrorKind, ProtoResult};
 use crate::serialize::binary::Restrict;
+use thiserror::Error;
 
 /// This is non-destructive to the inner buffer, b/c for pointer types we need to perform a reverse
 ///  seek to lookup names
@@ -25,8 +25,55 @@ use crate::serialize::binary::Restrict;
 ///  this is a simpler implementation without the cruft, at least for serializing to/from the
 ///  binary DNS protocols.
 pub struct BinDecoder<'a> {
-    buffer: &'a [u8],
-    remaining: &'a [u8],
+    buffer: &'a [u8],    // The entire original buffer
+    remaining: &'a [u8], // The unread section of the original buffer, so that reads do not cause a bounds check at the current seek offset
+}
+
+pub(crate) type DecodeResult<T> = Result<T, DecodeError>;
+
+/// An error that can occur deep in a decoder
+/// This type is kept very small so that function that use it inline often
+#[derive(Clone, Copy, Debug, Error)]
+pub enum DecodeError {
+    /// Insufficient data in the buffer for a read operation
+    #[error("unexpected end of input reached")]
+    InsufficientBytes,
+
+    /// slice_from was called with an invalid index
+    #[error(
+        "the index passed to BinDecoder::slice_from must be greater than the decoder position"
+    )]
+    InvalidPreviousIndex,
+
+    /// Pointer points to an index within or after the current label
+    #[error("label points to data not prior to idx: {idx} ptr: {ptr}")]
+    PointerNotPriorToLabel {
+        /// index of the label containing this pointer
+        idx: usize,
+        /// location to which the pointer is directing
+        ptr: u16,
+    },
+
+    /// Label bytes exceeded the limit of 63
+    #[error("label bytes exceed 63: {0}")]
+    LabelBytesTooLong(usize),
+
+    /// An unrecognized label code was found
+    #[error("unrecognized label code: {0:b}")]
+    UnrecognizedLabelCode(u8),
+
+    /// A domain name was too long
+    #[error("name label data exceed 255: {0}")]
+    DomainNameTooLong(usize),
+
+    /// Overlapping labels
+    #[error("overlapping labels name {label} other {other}")]
+    LabelOverlapsWithOther {
+        /// Start of the label that is overlaps
+        label: usize,
+        /// Start of the other label
+        other: usize,
+    },
 }
 
 impl<'a> BinDecoder<'a> {
@@ -43,12 +90,12 @@ impl<'a> BinDecoder<'a> {
     }
 
     /// Pop one byte from the buffer
-    pub fn pop(&mut self) -> ProtoResult<Restrict<u8>> {
+    pub fn pop(&mut self) -> DecodeResult<Restrict<u8>> {
         if let Some((first, remaining)) = self.remaining.split_first() {
             self.remaining = remaining;
             return Ok(Restrict::new(*first));
         }
-        Err("unexpected end of input reached".into())
+        Err(DecodeError::InsufficientBytes)
     }
 
     /// Returns the number of bytes in the buffer
@@ -102,31 +149,8 @@ impl<'a> BinDecoder<'a> {
     /// # Returns
     ///
     /// A String version of the character data
-    pub fn read_character_data(&mut self) -> ProtoResult<Restrict<&[u8]>> {
-        self.read_character_data_max(None)
-    }
-
-    /// Reads to a maximum length of data, returns an error if this is exceeded
-    pub fn read_character_data_max(
-        &mut self,
-        max_len: Option<usize>,
-    ) -> ProtoResult<Restrict<&[u8]>> {
-        let length = self
-            .pop()?
-            .map(|u| u as usize)
-            .verify_unwrap(|length| {
-                if let Some(max_len) = max_len {
-                    *length <= max_len
-                } else {
-                    true
-                }
-            })
-            .map_err(|length| {
-                ProtoError::from(ProtoErrorKind::CharacterDataTooLong {
-                    max: max_len.unwrap_or_default(),
-                    len: length,
-                })
-            })?;
+    pub fn read_character_data(&mut self) -> DecodeResult<Restrict<&[u8]>> {
+        let length = self.pop()?.unverified() as usize;
         self.read_slice(length)
     }
 
@@ -139,7 +163,7 @@ impl<'a> BinDecoder<'a> {
     /// # Returns
     ///
     /// The Vec of the specified length, otherwise an error
-    pub fn read_vec(&mut self, len: usize) -> ProtoResult<Restrict<Vec<u8>>> {
+    pub fn read_vec(&mut self, len: usize) -> DecodeResult<Restrict<Vec<u8>>> {
         self.read_slice(len).map(|s| s.map(ToOwned::to_owned))
     }
 
@@ -152,26 +176,26 @@ impl<'a> BinDecoder<'a> {
     /// # Returns
     ///
     /// The slice of the specified length, otherwise an error
-    pub fn read_slice(&mut self, len: usize) -> ProtoResult<Restrict<&'a [u8]>> {
+    pub fn read_slice(&mut self, len: usize) -> DecodeResult<Restrict<&'a [u8]>> {
         if len > self.remaining.len() {
-            return Err("buffer exhausted".into());
+            return Err(DecodeError::InsufficientBytes);
         }
         let (read, remaining) = self.remaining.split_at(len);
         self.remaining = remaining;
         Ok(Restrict::new(read))
     }
 
     /// Reads a slice from a previous index to the current
-    pub fn slice_from(&self, index: usize) -> ProtoResult<&'a [u8]> {
+    pub fn slice_from(&self, index: usize) -> DecodeResult<&'a [u8]> {
         if index > self.index() {
-            return Err("index antecedes upper bound".into());
+            return Err(DecodeError::InvalidPreviousIndex);
         }
 
         Ok(&self.buffer[index..self.index()])
     }
 
     /// Reads a byte from the buffer, equivalent to `Self::pop()`
-    pub fn read_u8(&mut self) -> ProtoResult<Restrict<u8>> {
+    pub fn read_u8(&mut self) -> DecodeResult<Restrict<u8>> {
         self.pop()
     }
 
@@ -183,7 +207,7 @@ impl<'a> BinDecoder<'a> {
     /// # Return
     ///
     /// Return the u16 from the buffer
-    pub fn read_u16(&mut self) -> ProtoResult<Restrict<u16>> {
+    pub fn read_u16(&mut self) -> DecodeResult<Restrict<u16>> {
         Ok(self
             .read_slice(2)?
             .map(|s| u16::from_be_bytes([s[0], s[1]])))
@@ -197,7 +221,7 @@ impl<'a> BinDecoder<'a> {
     /// # Return
     ///
     /// Return the i32 from the buffer
-    pub fn read_i32(&mut self) -> ProtoResult<Restrict<i32>> {
+    pub fn read_i32(&mut self) -> DecodeResult<Restrict<i32>> {
         Ok(self.read_slice(4)?.map(|s| {
             assert!(s.len() == 4);
             i32::from_be_bytes([s[0], s[1], s[2], s[3]])
@@ -212,7 +236,7 @@ impl<'a> BinDecoder<'a> {
     /// # Return
     ///
     /// Return the u32 from the buffer
-    pub fn read_u32(&mut self) -> ProtoResult<Restrict<u32>> {
+    pub fn read_u32(&mut self) -> DecodeResult<Restrict<u32>> {
         Ok(self.read_slice(4)?.map(|s| {
             assert!(s.len() == 4);
             u32::from_be_bytes([s[0], s[1], s[2], s[3]])

crates/proto/src/serialize/binary/mod.rs

@@ -20,7 +20,7 @@ mod decoder;
 mod encoder;
 mod restrict;
 
-pub use self::decoder::BinDecoder;
+pub use self::decoder::{BinDecoder, DecodeError};
 pub use self::encoder::BinEncoder;
 pub use self::encoder::EncodeMode;
 pub use self::restrict::{Restrict, RestrictedMath, Verified};
@@ -67,7 +67,10 @@ impl BinEncodable for u16 {
 
 impl<'r> BinDecodable<'r> for u16 {
     fn read(decoder: &mut BinDecoder<'_>) -> ProtoResult<Self> {
-        decoder.read_u16().map(Restrict::unverified)
+        decoder
+            .read_u16()
+            .map(Restrict::unverified)
+            .map_err(Into::into)
     }
 }
 
@@ -79,7 +82,10 @@ impl BinEncodable for i32 {
 
 impl<'r> BinDecodable<'r> for i32 {
     fn read(decoder: &mut BinDecoder<'_>) -> ProtoResult<i32> {
-        decoder.read_i32().map(Restrict::unverified)
+        decoder
+            .read_i32()
+            .map(Restrict::unverified)
+            .map_err(Into::into)
     }
 }
 
@@ -91,7 +97,10 @@ impl BinEncodable for u32 {
 
 impl<'r> BinDecodable<'r> for u32 {
     fn read(decoder: &mut BinDecoder<'_>) -> ProtoResult<Self> {
-        decoder.read_u32().map(Restrict::unverified)
+        decoder
+            .read_u32()
+            .map(Restrict::unverified)
+            .map_err(Into::into)
     }
 }