Merge pull request #2969 from TheBlueMatt/2024-03-fix-upgradable-enum
authorvalentinewallace <valentinewallace@users.noreply.github.com>
Mon, 8 Apr 2024 14:34:14 +0000 (16:34 +0200)
committerGitHub <noreply@github.com>
Mon, 8 Apr 2024 14:34:14 +0000 (16:34 +0200)
1  2 
lightning/src/lib.rs
lightning/src/util/ser_macros.rs

diff --combined lightning/src/lib.rs
index 3b5a4ebfbf18171c9fc95b5d201693a171858b68,c9ae3f26eeb9ca460eb11b2f06da249dfeed4dec..3bcd5d72c64e19c3adf89d5c8580016582e2462c
@@@ -94,7 -94,9 +94,9 @@@ pub use std::io
  pub use core2::io;
  
  #[cfg(not(feature = "std"))]
- mod io_extras {
+ #[doc(hidden)]
+ /// IO utilities public only for use by in-crate macros. These should not be used externally
+ pub mod io_extras {
        use core2::io::{self, Read, Write};
  
        /// A writer which will move data into the void.
  }
  
  #[cfg(feature = "std")]
+ #[doc(hidden)]
+ /// IO utilities public only for use by in-crate macros. These should not be used externally
  mod io_extras {
        pub fn read_to_end<D: ::std::io::Read>(mut d: D) -> Result<Vec<u8>, ::std::io::Error> {
                let mut buf = Vec::new();
  }
  
  mod prelude {
 +      #![allow(unused_imports)]
 +
        pub use alloc::{vec, vec::Vec, string::String, collections::VecDeque, boxed::Box};
  
        pub use alloc::borrow::ToOwned;
        pub use alloc::string::ToString;
  
 +      pub use core::convert::{AsMut, AsRef, TryFrom, TryInto};
 +      pub use core::default::Default;
 +      pub use core::marker::Sized;
 +
        pub(crate) use crate::util::hash_tables::*;
  }
  
index 312fe7d9f98424ea2a0ad3dc3231cd765d50e624,e1f4762ecbb8dde0b2600cd9b5fe5a85aa453fdc..df030d0b01eb4452ccf8e7b391e8c738bb9af453
@@@ -354,25 -354,25 +354,25 @@@ macro_rules! _check_missing_tlv 
  #[doc(hidden)]
  #[macro_export]
  macro_rules! _decode_tlv {
-       ($reader: expr, $field: ident, (default_value, $default: expr)) => {{
-               $crate::_decode_tlv!($reader, $field, required)
+       ($outer_reader: expr, $reader: expr, $field: ident, (default_value, $default: expr)) => {{
+               $crate::_decode_tlv!($outer_reader, $reader, $field, required)
        }};
-       ($reader: expr, $field: ident, (static_value, $value: expr)) => {{
+       ($outer_reader: expr, $reader: expr, $field: ident, (static_value, $value: expr)) => {{
        }};
-       ($reader: expr, $field: ident, required) => {{
+       ($outer_reader: expr, $reader: expr, $field: ident, required) => {{
                $field = $crate::util::ser::Readable::read(&mut $reader)?;
        }};
-       ($reader: expr, $field: ident, (required: $trait: ident $(, $read_arg: expr)?)) => {{
+       ($outer_reader: expr, $reader: expr, $field: ident, (required: $trait: ident $(, $read_arg: expr)?)) => {{
                $field = $trait::read(&mut $reader $(, $read_arg)*)?;
        }};
-       ($reader: expr, $field: ident, required_vec) => {{
+       ($outer_reader: expr, $reader: expr, $field: ident, required_vec) => {{
                let f: $crate::util::ser::WithoutLength<Vec<_>> = $crate::util::ser::Readable::read(&mut $reader)?;
                $field = f.0;
        }};
-       ($reader: expr, $field: ident, option) => {{
+       ($outer_reader: expr, $reader: expr, $field: ident, option) => {{
                $field = Some($crate::util::ser::Readable::read(&mut $reader)?);
        }};
-       ($reader: expr, $field: ident, optional_vec) => {{
+       ($outer_reader: expr, $reader: expr, $field: ident, optional_vec) => {{
                let f: $crate::util::ser::WithoutLength<Vec<_>> = $crate::util::ser::Readable::read(&mut $reader)?;
                $field = Some(f.0);
        }};
        // without backwards compat. We'll error if the field is missing, and return `Ok(None)` if the
        // field is present but we can no longer understand it.
        // Note that this variant can only be used within a `MaybeReadable` read.
-       ($reader: expr, $field: ident, upgradable_required) => {{
+       ($outer_reader: expr, $reader: expr, $field: ident, upgradable_required) => {{
                $field = match $crate::util::ser::MaybeReadable::read(&mut $reader)? {
                        Some(res) => res,
-                       _ => return Ok(None)
+                       None => {
+                               // If we successfully read a value but we don't know how to parse it, we give up
+                               // and immediately return `None`. However, we need to make sure we read the correct
+                               // number of bytes for this TLV stream, which is implicitly the end of the stream.
+                               // Thus, we consume everything left in the `$outer_reader` here, ensuring that if
+                               // we're being read as a part of another TLV stream we don't spuriously fail to
+                               // deserialize the outer object due to a TLV length mismatch.
+                               $crate::io_extras::copy($outer_reader, &mut $crate::io_extras::sink()).unwrap();
+                               return Ok(None)
+                       },
                };
        }};
        // `upgradable_option` indicates we're reading an Option-al TLV that may have been upgraded
        // without backwards compat. $field will be None if the TLV is missing or if the field is present
        // but we can no longer understand it.
-       ($reader: expr, $field: ident, upgradable_option) => {{
+       ($outer_reader: expr, $reader: expr, $field: ident, upgradable_option) => {{
                $field = $crate::util::ser::MaybeReadable::read(&mut $reader)?;
+               if $field.is_none() {
+                       #[cfg(not(debug_assertions))] {
+                               // In general, MaybeReadable implementations are required to consume all the bytes
+                               // of the object even if they don't understand it, but due to a bug in the
+                               // serialization format for `impl_writeable_tlv_based_enum_upgradable` we sometimes
+                               // don't know how many bytes that is. In such cases, we'd like to spuriously allow
+                               // TLV length mismatches, which we do here by calling `eat_remaining` so that the
+                               // `s.bytes_remain()` check in `_decode_tlv_stream_range` doesn't fail.
+                               $reader.eat_remaining()?;
+                       }
+               }
        }};
-       ($reader: expr, $field: ident, (option: $trait: ident $(, $read_arg: expr)?)) => {{
+       ($outer_reader: expr, $reader: expr, $field: ident, (option: $trait: ident $(, $read_arg: expr)?)) => {{
                $field = Some($trait::read(&mut $reader $(, $read_arg)*)?);
        }};
-       ($reader: expr, $field: ident, (option, encoding: ($fieldty: ty, $encoding: ident, $encoder:ty))) => {{
-               $crate::_decode_tlv!($reader, $field, (option, encoding: ($fieldty, $encoding)));
+       ($outer_reader: expr, $reader: expr, $field: ident, (option, encoding: ($fieldty: ty, $encoding: ident, $encoder:ty))) => {{
+               $crate::_decode_tlv!($outer_reader, $reader, $field, (option, encoding: ($fieldty, $encoding)));
        }};
-       ($reader: expr, $field: ident, (option, encoding: ($fieldty: ty, $encoding: ident))) => {{
+       ($outer_reader: expr, $reader: expr, $field: ident, (option, encoding: ($fieldty: ty, $encoding: ident))) => {{
                $field = {
                        let field: $encoding<$fieldty> = ser::Readable::read(&mut $reader)?;
                        Some(field.0)
                };
        }};
-       ($reader: expr, $field: ident, (option, encoding: $fieldty: ty)) => {{
-               $crate::_decode_tlv!($reader, $field, option);
+       ($outer_reader: expr, $reader: expr, $field: ident, (option, encoding: $fieldty: ty)) => {{
+               $crate::_decode_tlv!($outer_reader, $reader, $field, option);
        }};
  }
  
@@@ -539,7 -559,7 +559,7 @@@ macro_rules! _decode_tlv_stream_range 
                        let mut s = ser::FixedLengthReader::new(&mut stream_ref, length.0);
                        match typ.0 {
                                $(_t if $crate::_decode_tlv_stream_match_check!(_t, $type, $fieldty) => {
-                                       $crate::_decode_tlv!(s, $field, $fieldty);
+                                       $crate::_decode_tlv!($stream, s, $field, $fieldty);
                                        if s.bytes_remain() {
                                                s.eat_remaining()?; // Return ShortRead if there's actually not enough bytes
                                                return Err(DecodeError::InvalidValue);
@@@ -1033,7 -1053,7 +1053,7 @@@ macro_rules! impl_writeable_tlv_based_e
                                        $($variant_id => {
                                                // Because read_tlv_fields creates a labeled loop, we cannot call it twice
                                                // in the same function body. Instead, we define a closure and call it.
 -                                              let f = || {
 +                                              let mut f = || {
                                                        $crate::_init_and_read_len_prefixed_tlv_fields!(reader, {
                                                                $(($type, $field, $fieldty)),*
                                                        });
  /// when [`MaybeReadable`] is practical instead of just [`Readable`] as it provides an upgrade path for
  /// new variants to be added which are simply ignored by existing clients.
  ///
+ /// Note that only struct and unit variants (not tuple variants) will support downgrading, thus any
+ /// new odd variants MUST be non-tuple (i.e. described using `$variant_id` and `$variant_name` not
+ /// `$tuple_variant_id` and `$tuple_variant_name`).
+ ///
  /// [`MaybeReadable`]: crate::util::ser::MaybeReadable
  /// [`Writeable`]: crate::util::ser::Writeable
  /// [`DecodeError::UnknownRequiredFeature`]: crate::ln::msgs::DecodeError::UnknownRequiredFeature
@@@ -1087,7 -1111,7 +1111,7 @@@ macro_rules! impl_writeable_tlv_based_e
                                        $($variant_id => {
                                                // Because read_tlv_fields creates a labeled loop, we cannot call it twice
                                                // in the same function body. Instead, we define a closure and call it.
 -                                              let f = || {
 +                                              let mut f = || {
                                                        $crate::_init_and_read_len_prefixed_tlv_fields!(reader, {
                                                                $(($type, $field, $fieldty)),*
                                                        });
                                        $($($tuple_variant_id => {
                                                Ok(Some($st::$tuple_variant_name(Readable::read(reader)?)))
                                        }),*)*
-                                       _ if id % 2 == 1 => Ok(None),
+                                       _ if id % 2 == 1 => {
+                                               // Assume that a $variant_id was written, not a $tuple_variant_id, and read
+                                               // the length prefix and discard the correct number of bytes.
+                                               let tlv_len: $crate::util::ser::BigSize = $crate::util::ser::Readable::read(reader)?;
+                                               let mut rd = $crate::util::ser::FixedLengthReader::new(reader, tlv_len.0);
+                                               rd.eat_remaining().map_err(|_| $crate::ln::msgs::DecodeError::ShortRead)?;
+                                               Ok(None)
+                                       },
                                        _ => Err($crate::ln::msgs::DecodeError::UnknownRequiredFeature),
                                }
                        }
  
  #[cfg(test)]
  mod tests {
 -      use crate::io::{self, Cursor};
 +      #[allow(unused_imports)]
        use crate::prelude::*;
 +
 +      use crate::io::{self, Cursor};
        use crate::ln::msgs::DecodeError;
        use crate::util::ser::{Writeable, HighZeroBytesDroppedBigSize, VecWriter};
        use bitcoin::hashes::hex::FromHex;