Skip to content

Commit

Permalink
Do not execute the default_value expr until we need it in TLV deser
Browse files Browse the repository at this point in the history
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).
  • Loading branch information
TheBlueMatt committed Jul 5, 2022
1 parent d246e61 commit f1b9bd3
Show file tree
Hide file tree
Showing 4 changed files with 13 additions and 7 deletions.
2 changes: 1 addition & 1 deletion lightning/src/ln/channelmanager.rs
Expand Up @@ -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),
Expand Down
4 changes: 2 additions & 2 deletions lightning/src/util/config.rs
Expand Up @@ -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),
Expand Down
6 changes: 6 additions & 0 deletions lightning/src/util/ser.rs
Expand Up @@ -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>);
Expand Down
8 changes: 4 additions & 4 deletions lightning/src/util/ser_macros.rs
Expand Up @@ -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) => {{
Expand Down Expand Up @@ -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) => {{
Expand Down Expand Up @@ -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
Expand All @@ -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);
Expand Down

0 comments on commit f1b9bd3

Please sign in to comment.