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<T>` variable and a `T` variable.
We settle on `x = t.into()` and implement `From<T> for
OptionDeserWrapper<T>` 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).

diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 871ebb92d32270d1d1730dc1c947f1015d2596b8..14c9f8b02be98408d69e9121534635eea2e525b2 100644 (file)
--- 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 be7accc18dacfc837d89bddcec937e0e9eb4b349..2f65e958f8b60d0f0a0f72d3cd3b7ca1b009085a 100644 (file)
--- 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 a45806eef54a1434eada0da1d3f1d179d48f9285..fc795f9dd6713b84be341af6525a7ab29299e2c1 100644 (file)
--- a/lightning/src/util/ser.rs
+++ b/lightning/src/util/ser.rs
@@ -266,6 +266,12 @@ impl<T: Readable> Readable for OptionDeserWrapper<T> {
                Ok(Self(Some(Readable::read(reader)?)))
        }
 }
+/// When handling default_values, we want to map the default-value T directly
+/// to a OptionDeserWrapper<T> in a way that works for `field: T = t;` as
+/// well. Thus, we assume `Into<T> for T` does nothing and use that.
+impl<T: Readable> From<T> for OptionDeserWrapper<T> {
+       fn from(t: T) -> OptionDeserWrapper<T> { 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<T>);

diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs
index 816426b1133cb21a1e703129e6aecfa884f2c00c..351c2a1f5e25e4d21ad0c999e2f064bf366a6f35 100644 (file)
--- 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);