From 33566ac58c347f51bcb922e71a9a71dca7d916a0 Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Fri, 18 Oct 2024 12:08:27 +1100 Subject: [PATCH] Split encode::Error into two parts The `consensus::encode::Error` contains an IO error but reading from a buffer only ever errors for EOF. We converted all instances of EOF to `MissingData` already so now we can split the IO error apart from the actual encoding errors variants. --- bitcoin/src/bip152.rs | 5 +- bitcoin/src/blockdata/transaction.rs | 4 +- bitcoin/src/blockdata/witness.rs | 30 ++++++----- bitcoin/src/consensus/encode.rs | 42 +++++++++------- bitcoin/src/consensus/error.rs | 75 +++++++++++++++++++++------- bitcoin/src/consensus/mod.rs | 2 +- bitcoin/src/consensus/serde.rs | 26 +++++----- bitcoin/src/merkle_tree/block.rs | 5 +- bitcoin/src/psbt/raw.rs | 4 +- 9 files changed, 124 insertions(+), 69 deletions(-) diff --git a/bitcoin/src/bip152.rs b/bitcoin/src/bip152.rs index c3ccee11c..f7f79cf43 100644 --- a/bitcoin/src/bip152.rs +++ b/bitcoin/src/bip152.rs @@ -315,10 +315,11 @@ impl Decodable for BlockTransactionsRequest { .checked_mul(mem::size_of::()) .ok_or(consensus::parse_failed_error("invalid length"))?; if byte_size > encode::MAX_VEC_SIZE { - return Err(encode::Error::OversizedVectorAllocation { + return Err(encode::ParseError::OversizedVectorAllocation { requested: byte_size, max: encode::MAX_VEC_SIZE, - }); + } + .into()); } let mut indexes = Vec::with_capacity(nb_indexes); diff --git a/bitcoin/src/blockdata/transaction.rs b/bitcoin/src/blockdata/transaction.rs index 009605baf..8eabc3054 100644 --- a/bitcoin/src/blockdata/transaction.rs +++ b/bitcoin/src/blockdata/transaction.rs @@ -913,7 +913,7 @@ impl Decodable for Transaction { } } // We don't support anything else - x => Err(encode::Error::UnsupportedSegwitFlag(x)), + x => Err(encode::ParseError::UnsupportedSegwitFlag(x).into()), } // non-segwit } else { @@ -1486,7 +1486,7 @@ mod tests { let tx_bytes = hex!("0000fd000001021921212121212121212121f8b372b0239cc1dff600000000004f4f4f4f4f4f4f4f000000000000000000000000000000333732343133380d000000000000000000000000000000ff000000000009000dff000000000000000800000000000000000d"); let tx: Result = deserialize(&tx_bytes); assert!(tx.is_err()); - assert!(tx.unwrap_err().to_string().contains("witness flag set but no witnesses present")); + assert!(matches!(tx.unwrap_err(), crate::consensus::Error::Parse(_))); } #[test] diff --git a/bitcoin/src/blockdata/witness.rs b/bitcoin/src/blockdata/witness.rs index 08e39d483..d5ca14db0 100644 --- a/bitcoin/src/blockdata/witness.rs +++ b/bitcoin/src/blockdata/witness.rs @@ -26,10 +26,11 @@ impl Decodable for Witness { // Minimum size of witness element is 1 byte, so if the count is // greater than MAX_VEC_SIZE we must return an error. if witness_elements > MAX_VEC_SIZE { - return Err(self::Error::OversizedVectorAllocation { + return Err(encode::ParseError::OversizedVectorAllocation { requested: witness_elements, max: MAX_VEC_SIZE, - }); + } + .into()); } if witness_elements == 0 { Ok(Witness::default()) @@ -48,21 +49,26 @@ impl Decodable for Witness { let element_size_len = compact_size::encoded_size(element_size); let required_len = cursor .checked_add(element_size) - .ok_or(self::Error::OversizedVectorAllocation { - requested: usize::MAX, - max: MAX_VEC_SIZE, - })? + .ok_or(encode::Error::Parse( + encode::ParseError::OversizedVectorAllocation { + requested: usize::MAX, + max: MAX_VEC_SIZE, + }, + ))? .checked_add(element_size_len) - .ok_or(self::Error::OversizedVectorAllocation { - requested: usize::MAX, - max: MAX_VEC_SIZE, - })?; + .ok_or(encode::Error::Parse( + encode::ParseError::OversizedVectorAllocation { + requested: usize::MAX, + max: MAX_VEC_SIZE, + }, + ))?; if required_len > MAX_VEC_SIZE + witness_index_space { - return Err(self::Error::OversizedVectorAllocation { + return Err(encode::ParseError::OversizedVectorAllocation { requested: required_len, max: MAX_VEC_SIZE, - }); + } + .into()); } // We will do content.rotate_left(witness_index_space) later. diff --git a/bitcoin/src/consensus/encode.rs b/bitcoin/src/consensus/encode.rs index e5aa9fdf9..5b9902f01 100644 --- a/bitcoin/src/consensus/encode.rs +++ b/bitcoin/src/consensus/encode.rs @@ -36,7 +36,7 @@ use crate::taproot::TapLeafHash; use crate::transaction::{Transaction, TxIn, TxOut}; #[rustfmt::skip] // Keep public re-exports separate. -pub use super::{Error, FromHexError}; +pub use super::{Error, FromHexError, ParseError}; /// Encodes an object into a vector. pub fn serialize(data: &T) -> Vec { @@ -76,6 +76,7 @@ pub fn deserialize_hex(hex: &str) -> Result { /// doesn't consume the entire vector. pub fn deserialize_partial(data: &[u8]) -> Result<(T, usize), Error> { let mut decoder = Cursor::new(data); + let rv = Decodable::consensus_decode_from_finite_reader(&mut decoder)?; let consumed = decoder.position() as usize; @@ -224,7 +225,7 @@ impl ReadExt for R { 0xFF => { let x = self.read_u64()?; if x < 0x1_0000_0000 { // I.e., would have fit in a `u32`. - Err(Error::NonMinimalVarInt) + Err(ParseError::NonMinimalVarInt.into()) } else { Ok(x) } @@ -232,7 +233,7 @@ impl ReadExt for R { 0xFE => { let x = self.read_u32()?; if x < 0x1_0000 { // I.e., would have fit in a `u16`. - Err(Error::NonMinimalVarInt) + Err(ParseError::NonMinimalVarInt.into()) } else { Ok(x as u64) } @@ -240,7 +241,7 @@ impl ReadExt for R { 0xFD => { let x = self.read_u16()?; if x < 0xFD { // Could have been encoded as a `u8`. - Err(Error::NonMinimalVarInt) + Err(ParseError::NonMinimalVarInt.into()) } else { Ok(x as u64) } @@ -649,7 +650,8 @@ impl Decodable for CheckedData { let data = read_bytes_from_finite_reader(r, opts)?; let expected_checksum = sha2_checksum(&data); if expected_checksum != checksum { - Err(self::Error::InvalidChecksum { expected: expected_checksum, actual: checksum }) + Err(ParseError::InvalidChecksum { expected: expected_checksum, actual: checksum } + .into()) } else { Ok(CheckedData { data, checksum }) } @@ -858,50 +860,50 @@ mod tests { discriminant( &test_varint_encode(0xFF, &(0x100000000_u64 - 1).to_le_bytes()).unwrap_err() ), - discriminant(&Error::NonMinimalVarInt) + discriminant(&ParseError::NonMinimalVarInt.into()) ); assert_eq!( discriminant(&test_varint_encode(0xFE, &(0x10000_u64 - 1).to_le_bytes()).unwrap_err()), - discriminant(&Error::NonMinimalVarInt) + discriminant(&ParseError::NonMinimalVarInt.into()) ); assert_eq!( discriminant(&test_varint_encode(0xFD, &(0xFD_u64 - 1).to_le_bytes()).unwrap_err()), - discriminant(&Error::NonMinimalVarInt) + discriminant(&ParseError::NonMinimalVarInt.into()) ); assert_eq!( discriminant(&deserialize::>(&[0xfd, 0x00, 0x00]).unwrap_err()), - discriminant(&Error::NonMinimalVarInt) + discriminant(&ParseError::NonMinimalVarInt.into()) ); assert_eq!( discriminant(&deserialize::>(&[0xfd, 0xfc, 0x00]).unwrap_err()), - discriminant(&Error::NonMinimalVarInt) + discriminant(&ParseError::NonMinimalVarInt.into()) ); assert_eq!( discriminant(&deserialize::>(&[0xfd, 0xfc, 0x00]).unwrap_err()), - discriminant(&Error::NonMinimalVarInt) + discriminant(&ParseError::NonMinimalVarInt.into()) ); assert_eq!( discriminant(&deserialize::>(&[0xfe, 0xff, 0x00, 0x00, 0x00]).unwrap_err()), - discriminant(&Error::NonMinimalVarInt) + discriminant(&ParseError::NonMinimalVarInt.into()) ); assert_eq!( discriminant(&deserialize::>(&[0xfe, 0xff, 0xff, 0x00, 0x00]).unwrap_err()), - discriminant(&Error::NonMinimalVarInt) + discriminant(&ParseError::NonMinimalVarInt.into()) ); assert_eq!( discriminant( &deserialize::>(&[0xff, 0xff, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00]) .unwrap_err() ), - discriminant(&Error::NonMinimalVarInt) + discriminant(&ParseError::NonMinimalVarInt.into()) ); assert_eq!( discriminant( &deserialize::>(&[0xff, 0xff, 0xff, 0xff, 0x00, 0x00, 0x00, 0x00, 0x00]) .unwrap_err() ), - discriminant(&Error::NonMinimalVarInt) + discriminant(&ParseError::NonMinimalVarInt.into()) ); let mut vec_256 = vec![0; 259]; @@ -1031,7 +1033,10 @@ mod tests { // by making sure it fails with `MissingData` and not an `OversizedVectorAllocation` Error. let err = deserialize::(&serialize(&(super::MAX_VEC_SIZE as u32))).unwrap_err(); - assert!(matches!(err, Error::MissingData)); + match err { + Error::Io(e) => panic!("unexpected I/O error {}", e), + Error::Parse(e) => assert_eq!(e, ParseError::MissingData), + } test_len_is_max_vec::(); test_len_is_max_vec::(); @@ -1056,7 +1061,10 @@ mod tests { let mut buf = Vec::new(); buf.emit_compact_size(super::MAX_VEC_SIZE / mem::size_of::()).unwrap(); let err = deserialize::>(&buf).unwrap_err(); - assert!(matches!(err, Error::MissingData)); + match err { + Error::Io(e) => panic!("unexpected I/O error {}", e), + Error::Parse(e) => assert_eq!(e, ParseError::MissingData), + } } #[test] diff --git a/bitcoin/src/consensus/error.rs b/bitcoin/src/consensus/error.rs index d4f16fe1e..75dad6f69 100644 --- a/bitcoin/src/consensus/error.rs +++ b/bitcoin/src/consensus/error.rs @@ -56,6 +56,54 @@ impl std::error::Error for DecodeEr pub enum Error { /// And I/O error. Io(io::Error), + /// Error parsing encoded object. + Parse(ParseError), +} + +internals::impl_from_infallible!(Error); + +impl fmt::Display for Error { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + use Error::*; + + match *self { + Io(ref e) => write_err!(f, "IO error"; e), + Parse(ref e) => write_err!(f, "error parsing encoded object"; e), + } + } +} + +#[cfg(feature = "std")] +impl std::error::Error for Error { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + use Error::*; + + match *self { + Io(ref e) => Some(e), + Parse(ref e) => Some(e), + } + } +} + +impl From for Error { + fn from(e: io::Error) -> Self { + use io::ErrorKind; + + match e.kind() { + ErrorKind::UnexpectedEof => Error::Parse(ParseError::MissingData), + _ => Error::Io(e), + } + } +} + +impl From for Error { + fn from(e: ParseError) -> Self { Error::Parse(e) } +} + +/// Encoding is invalid. +#[derive(Debug, PartialEq, Eq)] +#[non_exhaustive] +pub enum ParseError { /// Missing data (early end of file or slice too short). MissingData, // TODO: Can we add more context? /// Tried to allocate an oversized vector. @@ -80,14 +128,13 @@ pub enum Error { UnsupportedSegwitFlag(u8), } -internals::impl_from_infallible!(Error); +internals::impl_from_infallible!(ParseError); -impl fmt::Display for Error { +impl fmt::Display for ParseError { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - use Error::*; + use ParseError::*; match *self { - Io(ref e) => write_err!(f, "IO error"; e), MissingData => write!(f, "missing data (early end of file or slice too short)"), OversizedVectorAllocation { requested: ref r, max: ref m } => write!(f, "allocation of oversized vector: requested {}, maximum {}", r, m), @@ -102,12 +149,11 @@ impl fmt::Display for Error { } #[cfg(feature = "std")] -impl std::error::Error for Error { +impl std::error::Error for ParseError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - use Error::*; + use ParseError::*; match self { - Io(e) => Some(e), MissingData | OversizedVectorAllocation { .. } | InvalidChecksum { .. } @@ -118,17 +164,6 @@ impl std::error::Error for Error { } } -impl From for Error { - fn from(e: io::Error) -> Self { - use io::ErrorKind; - - match e.kind() { - ErrorKind::UnexpectedEof => Error::MissingData, - _ => Error::Io(e), - } - } -} - /// Hex deserialization error. #[derive(Debug)] pub enum FromHexError { @@ -169,4 +204,6 @@ impl From for FromHexError { /// Constructs a `Error::ParseFailed` error. // This whole variant should go away because of the inner string. -pub(crate) fn parse_failed_error(msg: &'static str) -> Error { Error::ParseFailed(msg) } +pub(crate) fn parse_failed_error(msg: &'static str) -> Error { + Error::Parse(ParseError::ParseFailed(msg)) +} diff --git a/bitcoin/src/consensus/mod.rs b/bitcoin/src/consensus/mod.rs index 8a43ae329..7ef30ada6 100644 --- a/bitcoin/src/consensus/mod.rs +++ b/bitcoin/src/consensus/mod.rs @@ -20,7 +20,7 @@ use crate::consensus; #[doc(inline)] pub use self::{ encode::{deserialize, deserialize_partial, serialize, Decodable, Encodable, ReadExt, WriteExt}, - error::{Error, FromHexError, DecodeError}, + error::{Error, FromHexError, DecodeError, ParseError}, }; pub(crate) use self::error::parse_failed_error; diff --git a/bitcoin/src/consensus/serde.rs b/bitcoin/src/consensus/serde.rs index f96363f18..2751098fb 100644 --- a/bitcoin/src/consensus/serde.rs +++ b/bitcoin/src/consensus/serde.rs @@ -17,8 +17,7 @@ use serde::de::{SeqAccess, Unexpected, Visitor}; use serde::ser::SerializeSeq; use serde::{Deserializer, Serializer}; -use super::encode::Error as ConsensusError; -use super::{Decodable, Encodable}; +use super::{Decodable, Encodable, Error, ParseError}; use crate::consensus::{DecodeError, IterReader}; /// Hex-encoding strategy @@ -358,26 +357,25 @@ impl serde::de::Expected for DisplayExpected { } // not a trait impl because we panic on some variants -fn consensus_error_into_serde(error: ConsensusError) -> E { +fn consensus_error_into_serde(error: ParseError) -> E { match error { - ConsensusError::Io(error) => panic!("unexpected IO error {:?}", error), - ConsensusError::MissingData => + ParseError::MissingData => E::custom("missing data (early end of file or slice too short)"), - ConsensusError::OversizedVectorAllocation { requested, max } => E::custom(format_args!( + ParseError::OversizedVectorAllocation { requested, max } => E::custom(format_args!( "the requested allocation of {} items exceeds maximum of {}", requested, max )), - ConsensusError::InvalidChecksum { expected, actual } => E::invalid_value( + ParseError::InvalidChecksum { expected, actual } => E::invalid_value( Unexpected::Bytes(&actual), &DisplayExpected(format_args!( "checksum {:02x}{:02x}{:02x}{:02x}", expected[0], expected[1], expected[2], expected[3] )), ), - ConsensusError::NonMinimalVarInt => + ParseError::NonMinimalVarInt => E::custom(format_args!("compact size was not encoded minimally")), - ConsensusError::ParseFailed(msg) => E::custom(msg), - ConsensusError::UnsupportedSegwitFlag(flag) => + ParseError::ParseFailed(msg) => E::custom(msg), + ParseError::UnsupportedSegwitFlag(flag) => E::invalid_value(Unexpected::Unsigned(flag.into()), &"segwit version 1 flag"), } } @@ -390,7 +388,9 @@ where match self { DecodeError::Other(error) => error, DecodeError::TooManyBytes => E::custom(format_args!("got more bytes than expected")), - DecodeError::Consensus(error) => consensus_error_into_serde(error), + DecodeError::Consensus(Error::Parse(e)) => consensus_error_into_serde(e), + DecodeError::Consensus(Error::Io(_)) => + unreachable!("iterator never returns I/O error"), } } } @@ -403,7 +403,9 @@ where match self { DecodeError::Other(error) => error.into_de_error(), DecodeError::TooManyBytes => DE::custom(format_args!("got more bytes than expected")), - DecodeError::Consensus(error) => consensus_error_into_serde(error), + DecodeError::Consensus(Error::Parse(e)) => consensus_error_into_serde(e), + DecodeError::Consensus(Error::Io(_)) => + unreachable!("iterator never returns I/O error"), } } } diff --git a/bitcoin/src/merkle_tree/block.rs b/bitcoin/src/merkle_tree/block.rs index 1b96efe12..ef363d643 100644 --- a/bitcoin/src/merkle_tree/block.rs +++ b/bitcoin/src/merkle_tree/block.rs @@ -426,10 +426,11 @@ impl Decodable for PartialMerkleTree { let nb_bytes_for_bits = r.read_compact_size()? as usize; if nb_bytes_for_bits > MAX_VEC_SIZE { - return Err(encode::Error::OversizedVectorAllocation { + return Err(encode::ParseError::OversizedVectorAllocation { requested: nb_bytes_for_bits, max: MAX_VEC_SIZE, - }); + } + .into()); } let mut bits = vec![false; nb_bytes_for_bits * 8]; for chunk in bits.chunks_mut(8) { diff --git a/bitcoin/src/psbt/raw.rs b/bitcoin/src/psbt/raw.rs index 906af3b63..110624b58 100644 --- a/bitcoin/src/psbt/raw.rs +++ b/bitcoin/src/psbt/raw.rs @@ -82,10 +82,10 @@ impl Key { let key_byte_size: u64 = byte_size - 1; if key_byte_size > MAX_VEC_SIZE.to_u64() { - return Err(encode::Error::OversizedVectorAllocation { + return Err(encode::Error::Parse(encode::ParseError::OversizedVectorAllocation { requested: key_byte_size as usize, max: MAX_VEC_SIZE, - } + }) .into()); }