Skip to content

Fix unknown handling in impl_writeable_tlv_based_enum_upgradable #2969

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 8, 2024
Merged

Fix unknown handling in impl_writeable_tlv_based_enum_upgradable #2969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc78b77
Fix unknown handling in `impl_writeable_tlv_based_enum_upgradable`
TheBlueMatt Mar 26, 2024
d6770d4
Ensure we read the full TLV stream length when maybe-reading `None`
TheBlueMatt Mar 28, 2024
f852d16
Allow `MaybeReadable` to not fully read in `upgradable_option`
TheBlueMatt Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lightning/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +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.
Expand Down Expand Up @@ -154,6 +156,8 @@ mod io_extras {
}

#[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();
Expand Down
69 changes: 50 additions & 19 deletions lightning/src/util/ser_macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,58 +354,78 @@ 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);
}};
// `upgradable_required` indicates we're reading a required TLV that may have been upgraded
// 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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we use $crate::io_extras::copy but $reader.eat_remaining()? below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're not reading the inner reader (which is always a LengthLimitingReader for just the specific TLV's value and thus supports eat_remaining()) but rather the outer reader, which may be of any type and thus we can't assume eat_remaining is available.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. Makes sense.

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);
}};
}

Expand Down Expand Up @@ -539,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);
Expand Down Expand Up @@ -1065,6 +1085,10 @@ macro_rules! impl_writeable_tlv_based_enum {
/// 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
Expand Down Expand Up @@ -1102,7 +1126,14 @@ macro_rules! impl_writeable_tlv_based_enum_upgradable {
$($($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),
}
}
Expand Down
Loading