Skip to content

Commit f1b9bd3

Browse files
committed
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 rust-lang/rust#91369).
1 parent d246e61 commit f1b9bd3

File tree

4 files changed

+13
-7
lines changed

4 files changed

+13
-7
lines changed

lightning/src/ln/channelmanager.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -6198,7 +6198,7 @@ impl_writeable_tlv_based!(ChannelDetails, {
61986198
(18, outbound_capacity_msat, required),
61996199
// Note that by the time we get past the required read above, outbound_capacity_msat will be
62006200
// filled in, so we can safely unwrap it here.
6201-
(19, next_outbound_htlc_limit_msat, (default_value, outbound_capacity_msat.0.unwrap())),
6201+
(19, next_outbound_htlc_limit_msat, (default_value, outbound_capacity_msat.0.unwrap() as u64)),
62026202
(20, inbound_capacity_msat, required),
62036203
(22, confirmations_required, option),
62046204
(24, force_close_spend_delay, option),

lightning/src/util/config.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -407,9 +407,9 @@ impl ::util::ser::Readable for LegacyChannelConfig {
407407
let mut forwarding_fee_base_msat = 0;
408408
read_tlv_fields!(reader, {
409409
(0, forwarding_fee_proportional_millionths, required),
410-
(1, max_dust_htlc_exposure_msat, (default_value, 5_000_000)),
410+
(1, max_dust_htlc_exposure_msat, (default_value, 5_000_000u64)),
411411
(2, cltv_expiry_delta, required),
412-
(3, force_close_avoidance_max_fee_satoshis, (default_value, 1000)),
412+
(3, force_close_avoidance_max_fee_satoshis, (default_value, 1000u64)),
413413
(4, announced_channel, required),
414414
(6, commit_upfront_shutdown_pubkey, required),
415415
(8, forwarding_fee_base_msat, required),

lightning/src/util/ser.rs

+6
Original file line numberDiff line numberDiff line change
@@ -266,6 +266,12 @@ impl<T: Readable> Readable for OptionDeserWrapper<T> {
266266
Ok(Self(Some(Readable::read(reader)?)))
267267
}
268268
}
269+
/// When handling default_values, we want to map the default-value T directly
270+
/// to a OptionDeserWrapper<T> in a way that works for `field: T = t;` as
271+
/// well. Thus, we assume `Into<T> for T` does nothing and use that.
272+
impl<T: Readable> From<T> for OptionDeserWrapper<T> {
273+
fn from(t: T) -> OptionDeserWrapper<T> { OptionDeserWrapper(Some(t)) }
274+
}
269275

270276
/// Wrapper to write each element of a Vec with no length prefix
271277
pub(crate) struct VecWriteWrapper<'a, T: Writeable>(pub &'a Vec<T>);

lightning/src/util/ser_macros.rs

+4-4
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ macro_rules! check_tlv_order {
9999
#[allow(unused_comparisons)] // Note that $type may be 0 making the second comparison always true
100100
let invalid_order = ($last_seen_type.is_none() || $last_seen_type.unwrap() < $type) && $typ.0 > $type;
101101
if invalid_order {
102-
$field = $default;
102+
$field = $default.into();
103103
}
104104
}};
105105
($last_seen_type: expr, $typ: expr, $type: expr, $field: ident, required) => {{
@@ -128,7 +128,7 @@ macro_rules! check_missing_tlv {
128128
#[allow(unused_comparisons)] // Note that $type may be 0 making the second comparison always true
129129
let missing_req_type = $last_seen_type.is_none() || $last_seen_type.unwrap() < $type;
130130
if missing_req_type {
131-
$field = $default;
131+
$field = $default.into();
132132
}
133133
}};
134134
($last_seen_type: expr, $type: expr, $field: ident, required) => {{
@@ -349,7 +349,7 @@ macro_rules! read_tlv_fields {
349349

350350
macro_rules! init_tlv_based_struct_field {
351351
($field: ident, (default_value, $default: expr)) => {
352-
$field
352+
$field.0.unwrap()
353353
};
354354
($field: ident, option) => {
355355
$field
@@ -364,7 +364,7 @@ macro_rules! init_tlv_based_struct_field {
364364

365365
macro_rules! init_tlv_field_var {
366366
($field: ident, (default_value, $default: expr)) => {
367-
let mut $field = $default;
367+
let mut $field = ::util::ser::OptionDeserWrapper(None);
368368
};
369369
($field: ident, required) => {
370370
let mut $field = ::util::ser::OptionDeserWrapper(None);

0 commit comments

Comments
 (0)