diff --git a/sv2/codec-sv2/src/decoder.rs b/sv2/codec-sv2/src/decoder.rs index 6793c13b25..e85b60b67a 100644 --- a/sv2/codec-sv2/src/decoder.rs +++ b/sv2/codec-sv2/src/decoder.rs @@ -33,7 +33,7 @@ use core::marker::PhantomData; #[cfg(feature = "noise_sv2")] use framing_sv2::framing::HandShakeFrame; use framing_sv2::{ - framing::{Frame, Sv2Frame}, + framing::{Frame, SizeHint, Sv2Frame}, header::Header, }; #[cfg(feature = "noise_sv2")] @@ -43,9 +43,7 @@ use noise_sv2::NoiseEngine; #[cfg(feature = "noise_sv2")] use noise_sv2::NOISE_FRAME_HEADER_SIZE; -#[cfg(feature = "noise_sv2")] -use crate::error::Error; -use crate::error::Result; +use crate::error::{Error, Result}; use crate::Error::MissingBytes; #[cfg(feature = "noise_sv2")] @@ -357,13 +355,15 @@ pub struct WithoutNoise { } impl WithoutNoise { - /// Attempts to decode the next frame, returning either a frame or an error indicating how many - /// bytes are missing. - /// - /// Attempts to decode the next Sv2 frame. + /// Attempts to decode the next frame, returning either a frame or an error describing how the + /// buffered bytes differ from the frame size declared by the header. /// - /// On success, the decoded frame is returned. Otherwise, an error indicating the number of - /// missing bytes required to complete the frame is returned. + /// On success, the decoded frame is returned. Otherwise, the error is either + /// `Error::MissingBytes`, carrying the number of bytes still required to complete the frame, + /// or `Error::UnexpectedTrailingBytes`, carrying the number of bytes buffered beyond the + /// expected frame size. The latter discards the buffered data; since the surplus bytes may + /// have contained the start of the next frame, the caller should treat the stream as + /// desynchronized. /// /// In the case of `Error::MissingBytes`, the user should resize the decoder buffer using /// `writable`, read another chunk from the incoming message stream, and then call `next_frame` @@ -373,19 +373,25 @@ impl WithoutNoise { pub fn next_frame(&mut self) -> Result> { let len = self.buffer.len(); let src = self.buffer.get_data_by_ref(len); - let hint = Sv2Frame::::size_hint(src) as usize; - match hint { - 0 => { + match Sv2Frame::::size_hint(src) { + SizeHint::Exact => { self.missing_b = Header::SIZE; let src = self.buffer.get_data_owned(); let frame = Sv2Frame::::from_bytes_unchecked(src); Ok(frame) } - _ => { - self.missing_b = hint; + SizeHint::Missing(missing) => { + self.missing_b = missing; Err(MissingBytes(self.missing_b)) } + SizeHint::Surplus(surplus) => { + // Reachable if the buffer was filled past the slice returned by `writable`; the + // frame boundary is lost, so drain everything and report the surplus. + self.missing_b = Header::SIZE; + let _ = self.buffer.get_data_owned(); + Err(Error::UnexpectedTrailingBytes(surplus)) + } } } @@ -446,7 +452,7 @@ mod prop_tests { use buffer_sv2::Buffer as IsBuffer; #[cfg(feature = "noise_sv2")] use framing_sv2::framing::Frame; - use framing_sv2::framing::Sv2Frame; + use framing_sv2::{framing::Sv2Frame, header::Header}; #[cfg(feature = "noise_sv2")] use key_utils::{Secp256k1PublicKey, Secp256k1SecretKey}; #[cfg(feature = "noise_sv2")] @@ -598,6 +604,36 @@ mod prop_tests { TestResult::from_bool(missing_bytes_count > 0) } + /// Verifies that over-filling the buffer (calling `writable` twice before `next_frame`) + /// surfaces `UnexpectedTrailingBytes`, drains the buffer, and leaves the decoder usable. + #[test] + fn test_decoder_excess_bytes_drains_and_recovers() { + let msg = TestMessage { value: 42 }; + let frame = Sv2Frame::::from_message(msg.clone(), 0, 0, false).unwrap(); + let mut encoder = Encoder::::new(); + let encoded = encoder.encode(frame).unwrap(); + let encoded: &[u8] = encoded.as_ref(); + + let mut decoder = StandardDecoder::::new(); + decoder.writable().copy_from_slice(&encoded[..Header::SIZE]); + assert!(matches!( + decoder.next_frame(), + Err(crate::Error::MissingBytes(_)) + )); + decoder.writable().copy_from_slice(&encoded[Header::SIZE..]); + let surplus = decoder.writable().len(); + match decoder.next_frame() { + Err(crate::Error::UnexpectedTrailingBytes(n)) => assert_eq!(n, surplus), + Ok(_) => panic!("expected UnexpectedTrailingBytes, got a frame"), + Err(e) => panic!("expected UnexpectedTrailingBytes, got {e:?}"), + } + + let mut decoded = + decode_frame(&mut decoder, encoded, None).expect("decoder should recover"); + let decoded_msg: TestMessage = binary_sv2::from_bytes(decoded.payload()).unwrap(); + assert_eq!(decoded_msg, msg); + } + /// Verifies that a single decoder instance correctly decodes two consecutive independent /// frames in sequence, confirming that internal state resets between frames. #[quickcheck] diff --git a/sv2/codec-sv2/src/error.rs b/sv2/codec-sv2/src/error.rs index 097706a884..38f5581e9a 100644 --- a/sv2/codec-sv2/src/error.rs +++ b/sv2/codec-sv2/src/error.rs @@ -55,6 +55,9 @@ pub enum Error { /// Unexpected state in the Noise protocol. UnexpectedNoiseState, + + /// The decoder buffer holds a complete frame plus the given number of surplus bytes. + UnexpectedTrailingBytes(usize), } impl fmt::Display for Error { @@ -94,6 +97,9 @@ impl fmt::Display for Error { UnexpectedNoiseState => { write!(f, "Noise state is incorrect") } + UnexpectedTrailingBytes(u) => { + write!(f, "Buffer holds `{u}` bytes beyond the end of the frame") + } } } } diff --git a/sv2/codec-sv2/src/lib.rs b/sv2/codec-sv2/src/lib.rs index d001aca843..f0aec94794 100644 --- a/sv2/codec-sv2/src/lib.rs +++ b/sv2/codec-sv2/src/lib.rs @@ -52,6 +52,8 @@ pub use error::{Error, Result}; pub use decoder::{StandardEitherFrame, StandardSv2Frame}; +pub use framing_sv2::framing::SizeHint; + pub use decoder::StandardDecoder; #[cfg(feature = "noise_sv2")] pub use decoder::StandardNoiseDecoder; diff --git a/sv2/framing-sv2/src/framing.rs b/sv2/framing-sv2/src/framing.rs index 72507fb1d0..455afb3f5c 100644 --- a/sv2/framing-sv2/src/framing.rs +++ b/sv2/framing-sv2/src/framing.rs @@ -18,7 +18,7 @@ use crate::{header::Header, Error}; use alloc::vec::Vec; use binary_sv2::{to_writer, GetSize, Serialize}; -use core::convert::TryFrom; +use core::{cmp::Ordering, convert::TryFrom, fmt}; #[cfg(not(feature = "with_buffer_pool"))] type Slice = Vec; @@ -26,6 +26,46 @@ type Slice = Vec; #[cfg(feature = "with_buffer_pool")] type Slice = buffer_sv2::Slice; +/// Describes how the length of a byte slice relates to the frame size declared by its [`Header`]. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum SizeHint { + /// The slice does not hold a complete frame yet, and the given number of bytes are still + /// missing, either from the [`Header`] or from the payload it declares. + Missing(usize), + + /// The slice holds a complete frame followed by the given number of surplus bytes. + Surplus(usize), + + /// The slice holds exactly one complete frame. + Exact, +} + +impl SizeHint { + /// Returns `true` if the slice holds exactly one complete frame. + pub fn is_exact(&self) -> bool { + matches!(self, SizeHint::Exact) + } + + /// Returns the number of bytes still needed to complete the frame, or `0` if the frame is + /// already complete (with or without surplus bytes). + pub fn missing_bytes(&self) -> usize { + match self { + SizeHint::Missing(missing) => *missing, + SizeHint::Surplus(_) | SizeHint::Exact => 0, + } + } +} + +impl fmt::Display for SizeHint { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + SizeHint::Missing(missing) => write!(f, "missing {missing} bytes"), + SizeHint::Surplus(surplus) => write!(f, "complete frame with {surplus} surplus bytes"), + SizeHint::Exact => write!(f, "exactly one complete frame"), + } + } +} + /// Represents either an Sv2 frame or a handshake frame. /// /// A wrapper used when generic reference to a frame is needed, but the kind of frame ([`Sv2Frame`] @@ -116,17 +156,14 @@ impl + AsRef<[u8]>> Sv2Frame { /// Tries to build a [`Sv2Frame`] from raw bytes. /// /// It assumes the raw bytes represent a serialized [`Sv2Frame`] frame (`Self.serialized`). - /// Returns a [`Sv2Frame`] on success, or the number of the bytes needed to complete the frame - /// as an error. `Self.serialized` is [`Some`], but nothing is assumed or checked about the + /// Returns a [`Sv2Frame`] on success, or the [`SizeHint`] describing the size mismatch as an + /// error. `Self.serialized` is [`Some`], but nothing is assumed or checked about the /// correctness of the payload. #[inline] - pub fn from_bytes(mut bytes: B) -> Result { - let hint = Self::size_hint(bytes.as_mut()); - - if hint == 0 { - Ok(Self::from_bytes_unchecked(bytes)) - } else { - Err(hint) + pub fn from_bytes(mut bytes: B) -> Result { + match Self::size_hint(bytes.as_mut()) { + SizeHint::Exact => Ok(Self::from_bytes_unchecked(bytes)), + hint => Err(hint), } } @@ -142,29 +179,22 @@ impl + AsRef<[u8]>> Sv2Frame { } } - /// After parsing `bytes` into a [`Header`], this function helps to determine if the - /// `msg_length` field is correctly representing the size of the frame. - /// - Returns `0` if the byte slice is of the expected size according to the header. - /// - Returns a negative value if the byte slice is shorter than expected; this value represents - /// how many bytes are missing. - /// - Returns a positive value if the byte slice is longer than expected; this value indicates - /// the surplus of bytes beyond the expected size. + /// Compares the size of `bytes` against the expected frame size, i.e. [`Header::SIZE`] plus + /// the `msg_length` declared in the parsed [`Header`]. + /// + /// If `bytes` is too short to contain a full [`Header`], the returned [`SizeHint::Missing`] + /// only accounts for the bytes needed to complete the header. #[inline] - pub fn size_hint(bytes: &[u8]) -> isize { - match Header::from_bytes(bytes) { - Err(_) => { - // Returns how many bytes are missing from the expected frame size - (Header::SIZE - bytes.len()) as isize - } - Ok(header) => { - if bytes.len() - Header::SIZE == header.len() { - // expected frame size confirmed - 0 - } else { - // Returns how many excess bytes are beyond the expected frame size - (bytes.len() - Header::SIZE) as isize + header.len() as isize - } - } + pub fn size_hint(bytes: &[u8]) -> SizeHint { + if bytes.len() < Header::SIZE { + return SizeHint::Missing(Header::SIZE - bytes.len()); + } + let header = Header::from_bytes(bytes).expect("header parsing only fails on short input"); + let expected = Header::SIZE + header.len(); + match bytes.len().cmp(&expected) { + Ordering::Less => SizeHint::Missing(expected - bytes.len()), + Ordering::Equal => SizeHint::Exact, + Ordering::Greater => SizeHint::Surplus(bytes.len() - expected), } } @@ -245,8 +275,8 @@ impl HandShakeFrame { /// Builds a [`HandShakeFrame`] from raw bytes. Nothing is assumed or checked about the /// correctness of the payload. - pub fn from_bytes(bytes: Slice) -> Result { - Ok(Self::from_bytes_unchecked(bytes)) + pub fn from_bytes(bytes: Slice) -> Self { + Self::from_bytes_unchecked(bytes) } #[inline] @@ -311,7 +341,7 @@ mod tests { #[test] fn test_size_hint() { let h = Sv2Frame::>::size_hint(&[0, 128, 30, 46, 0, 0][..]); - assert!(h == 46); + assert_eq!(h, SizeHint::Missing(46)); } #[derive(Debug, Clone)] @@ -446,8 +476,9 @@ mod tests { let hint = Sv2Frame::>::size_hint(&bytes); assert_eq!( - hint, 0, - "size_hint should return 0 when bytes match expected frame size exactly" + hint, + SizeHint::Exact, + "size_hint should return Exact when bytes match expected frame size exactly" ); } @@ -456,15 +487,10 @@ mod tests { let bytes: Vec = bytes.iter().take(Header::SIZE - 1).copied().collect(); let hint = Sv2Frame::>::size_hint(&bytes); - let expected = (Header::SIZE - bytes.len()) as isize; - assert!( - hint > 0, - "size_hint should be positive when header is incomplete" - ); assert_eq!( - hint, expected, - "size_hint should return missing bytes count: expected {}, got {}", - expected, hint + hint, + SizeHint::Missing(Header::SIZE - bytes.len()), + "size_hint should return the bytes missing to complete the header" ); } @@ -545,8 +571,7 @@ mod tests { fn prop_handshake_frame_from_bytes(payload: Vec) { let payload: Vec = payload.iter().take(1000).copied().collect(); - let frame = HandShakeFrame::from_bytes(payload.clone().into()) - .expect("HandShakeFrame::from_bytes should succeed for any valid payload"); + let frame = HandShakeFrame::from_bytes(payload.clone().into()); let recovered = frame.get_payload_when_handshaking(); assert_eq!( @@ -606,7 +631,6 @@ mod tests { ); } - #[ignore = "size_hint semantics are broken (see https://github.com/stratum-mining/stratum/issues/2086)"] #[quickcheck] fn prop_size_hint_truncated_payload(msg_length: ValidU24, cut: u16) { let msg_type = 0x01u8; @@ -627,19 +651,13 @@ mod tests { let hint = Sv2Frame::>::size_hint(&bytes); - assert!( - hint < 0, - "size_hint should be negative when payload is truncated" - ); - assert_eq!( hint, - -(missing as isize), + SizeHint::Missing(missing), "size_hint should equal missing bytes" ); } - #[ignore = "size_hint semantics are broken (see https://github.com/stratum-mining/stratum/issues/2086)"] #[quickcheck] fn prop_size_hint_extra_bytes(msg_length: ValidU24, extra: u16) { let msg_type = 0x01u8; @@ -654,55 +672,35 @@ mod tests { let hint = Sv2Frame::>::size_hint(&bytes); - assert!( - hint > 0, - "size_hint should be positive when extra bytes exist" - ); - - assert_eq!( - hint, extra as isize, - "size_hint should equal number of extra bytes" - ); - } - - #[ignore = "size_hint semantics are broken (see https://github.com/stratum-mining/stratum/issues/2086)"] - #[quickcheck] - fn prop_size_hint_matches_delta(msg_length: ValidU24, delta: i16) { - let msg_type = 0x01u8; - let ext = 0u16; - - let header = Header::from_len(msg_length.0, msg_type, ext).unwrap(); - - let expected = msg_length.0 as isize; - let actual = (expected + delta as isize).max(0) as usize; - - let mut bytes = vec![0u8; Header::SIZE + actual]; - binary_sv2::to_writer(header, &mut bytes[..Header::SIZE]).unwrap(); - - let hint = Sv2Frame::>::size_hint(&bytes); - assert_eq!( hint, - actual as isize - expected, - "size_hint must equal actual - expected payload size" + SizeHint::Surplus(extra), + "size_hint should equal number of extra bytes" ); } - #[ignore = "size_hint semantics are broken (see https://github.com/stratum-mining/stratum/issues/2086)"] #[quickcheck] - fn prop_size_hint_monotonic_growth(msg_length: ValidU24) { - let header = Header::from_len(msg_length.0, 1, 0).unwrap(); - let total = Header::SIZE + msg_length.0 as usize; + fn prop_size_hint_incremental_arrival(msg_length: ValidU24) { + let payload_len = (msg_length.0 % 4096) as usize; + let header = Header::from_len(payload_len as u32, 1, 0).unwrap(); + let total = Header::SIZE + payload_len; let mut full = vec![0u8; total]; binary_sv2::to_writer(header, &mut full[..Header::SIZE]).unwrap(); - let mut prev = isize::MIN; - - for i in 0..=total { + for i in 0..total { let hint = Sv2Frame::>::size_hint(&full[..i]); - assert!(hint >= prev, "hint should increase as more bytes arrive"); - prev = hint; + let expected = if i < Header::SIZE { + SizeHint::Missing(Header::SIZE - i) + } else { + SizeHint::Missing(total - i) + }; + assert_eq!(hint, expected, "hint mismatch with {i} of {total} bytes"); } + + assert_eq!( + Sv2Frame::>::size_hint(&full), + SizeHint::Exact + ); } } diff --git a/sv2/parsers-sv2/src/tlv/error.rs b/sv2/parsers-sv2/src/tlv/error.rs index 0e738c08f5..8792b45fa6 100644 --- a/sv2/parsers-sv2/src/tlv/error.rs +++ b/sv2/parsers-sv2/src/tlv/error.rs @@ -1,4 +1,5 @@ use core::fmt; +use framing_sv2::framing::SizeHint; /// Generic errors that can occur during TLV field encoding and decoding. #[derive(Debug, PartialEq, Eq)] @@ -20,8 +21,8 @@ pub enum TlvError { /// Failed to construct a StandardSv2Frame from bytes. /// - /// Contains the error code returned by `Sv2Frame::from_bytes`. - FrameConstructionFailed(isize), + /// Contains the [`SizeHint`] returned by `Sv2Frame::from_bytes`. + FrameConstructionFailed(SizeHint), } impl fmt::Display for TlvError { @@ -34,12 +35,8 @@ impl fmt::Display for TlvError { ), TlvError::EncodingError(err) => write!(f, "Failed to encode TLV data: {:?}", err), TlvError::DecodingError(err) => write!(f, "Failed to decode TLV data: {:?}", err), - TlvError::FrameConstructionFailed(code) => { - write!( - f, - "Failed to construct Sv2Frame from bytes (error code: {})", - code - ) + TlvError::FrameConstructionFailed(hint) => { + write!(f, "Failed to construct Sv2Frame from bytes: {:?}", hint) } } } @@ -56,7 +53,7 @@ mod tests { assert!(err.to_string().contains("5")); assert!(err.to_string().contains("10")); - let err = TlvError::FrameConstructionFailed(-1); - assert!(err.to_string().contains("-1")); + let err = TlvError::FrameConstructionFailed(SizeHint::Missing(1)); + assert!(err.to_string().contains("Missing(1)")); } }