From: Matt Corallo <git@bluematt.me>
Date: Fri, 1 Jul 2022 21:14:19 +0000 (+0000)
Subject: Do not execute the default_value expr until we need it in TLV deser
X-Git-Tag: v0.0.110~25^2
X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=f1b9bd34b83f9e8161b6d7bbc56a420cc572502a;p=rust-lightning

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