From f1b9bd34b83f9e8161b6d7bbc56a420cc572502a Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 1 Jul 2022 21:14:19 +0000 Subject: [PATCH] Do not execute the default_value expr until we need it in TLV deser This fixes an insta-panic in `ChannelMonitor` deserialization where we always `unwrap` a previous value to determine the default value of a later field. However, because we always ran the `unwrap` before the previous field is read, we'd always panic. The fix is rather simple - use a `OptionDeserWrapper` for `default_value` fields and only fill in the default value if no value was read while walking the TLV stream. The only complexity comes from our desire to support `read_tlv_field` calls that use an explicit field rather than an `Option` of some sort, which requires some statement which can assign both an `OptionDeserWrapper` variable and a `T` variable. We settle on `x = t.into()` and implement `From for OptionDeserWrapper` which works, though it requires users to specify types explicitly due to Rust determining expression types prior to macro execution, completely guessing with no knowlege for integer expressions (see https://github.com/rust-lang/rust/issues/91369). --- lightning/src/ln/channelmanager.rs | 2 +- lightning/src/util/config.rs | 4 ++-- lightning/src/util/ser.rs | 6 ++++++ lightning/src/util/ser_macros.rs | 8 ++++---- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 871ebb92d..14c9f8b02 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -6198,7 +6198,7 @@ impl_writeable_tlv_based!(ChannelDetails, { (18, outbound_capacity_msat, required), // Note that by the time we get past the required read above, outbound_capacity_msat will be // filled in, so we can safely unwrap it here. - (19, next_outbound_htlc_limit_msat, (default_value, outbound_capacity_msat.0.unwrap())), + (19, next_outbound_htlc_limit_msat, (default_value, outbound_capacity_msat.0.unwrap() as u64)), (20, inbound_capacity_msat, required), (22, confirmations_required, option), (24, force_close_spend_delay, option), diff --git a/lightning/src/util/config.rs b/lightning/src/util/config.rs index be7accc18..2f65e958f 100644 --- a/lightning/src/util/config.rs +++ b/lightning/src/util/config.rs @@ -407,9 +407,9 @@ impl ::util::ser::Readable for LegacyChannelConfig { let mut forwarding_fee_base_msat = 0; read_tlv_fields!(reader, { (0, forwarding_fee_proportional_millionths, required), - (1, max_dust_htlc_exposure_msat, (default_value, 5_000_000)), + (1, max_dust_htlc_exposure_msat, (default_value, 5_000_000u64)), (2, cltv_expiry_delta, required), - (3, force_close_avoidance_max_fee_satoshis, (default_value, 1000)), + (3, force_close_avoidance_max_fee_satoshis, (default_value, 1000u64)), (4, announced_channel, required), (6, commit_upfront_shutdown_pubkey, required), (8, forwarding_fee_base_msat, required), diff --git a/lightning/src/util/ser.rs b/lightning/src/util/ser.rs index a45806eef..fc795f9dd 100644 --- a/lightning/src/util/ser.rs +++ b/lightning/src/util/ser.rs @@ -266,6 +266,12 @@ impl Readable for OptionDeserWrapper { Ok(Self(Some(Readable::read(reader)?))) } } +/// When handling default_values, we want to map the default-value T directly +/// to a OptionDeserWrapper in a way that works for `field: T = t;` as +/// well. Thus, we assume `Into for T` does nothing and use that. +impl From for OptionDeserWrapper { + fn from(t: T) -> OptionDeserWrapper { OptionDeserWrapper(Some(t)) } +} /// Wrapper to write each element of a Vec with no length prefix pub(crate) struct VecWriteWrapper<'a, T: Writeable>(pub &'a Vec); diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs index 816426b11..351c2a1f5 100644 --- a/lightning/src/util/ser_macros.rs +++ b/lightning/src/util/ser_macros.rs @@ -99,7 +99,7 @@ macro_rules! check_tlv_order { #[allow(unused_comparisons)] // Note that $type may be 0 making the second comparison always true let invalid_order = ($last_seen_type.is_none() || $last_seen_type.unwrap() < $type) && $typ.0 > $type; if invalid_order { - $field = $default; + $field = $default.into(); } }}; ($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, required) => {{ @@ -128,7 +128,7 @@ macro_rules! check_missing_tlv { #[allow(unused_comparisons)] // Note that $type may be 0 making the second comparison always true let missing_req_type = $last_seen_type.is_none() || $last_seen_type.unwrap() < $type; if missing_req_type { - $field = $default; + $field = $default.into(); } }}; ($last_seen_type: expr, $type: expr, $field: ident, required) => {{ @@ -349,7 +349,7 @@ macro_rules! read_tlv_fields { macro_rules! init_tlv_based_struct_field { ($field: ident, (default_value, $default: expr)) => { - $field + $field.0.unwrap() }; ($field: ident, option) => { $field @@ -364,7 +364,7 @@ macro_rules! init_tlv_based_struct_field { macro_rules! init_tlv_field_var { ($field: ident, (default_value, $default: expr)) => { - let mut $field = $default; + let mut $field = ::util::ser::OptionDeserWrapper(None); }; ($field: ident, required) => { let mut $field = ::util::ser::OptionDeserWrapper(None); -- 2.39.5