From 71e0173d8db17116a1a17517b40512be8c05af50 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 21 Sep 2021 17:48:40 +0000 Subject: [PATCH] Fix future unknown `Event` variant backwards compatibility In 8ffc2d1742ff1171a87b0410b21cbbd557ff8247, in 0.0.100, we added a backwards compatibility feature to the reading of `Event`s - if the type was unknown and odd, we'd simply ignore the event and treat it as no event. However, we failed to read the length-prefixed TLV stream when doing so, resulting in us reading some of the skipped-event data as the next event or other data in the ChannelManager. We fix this by reading the varint length prefix written, then skipping that many bytes when we come across an unknown odd event type. --- lightning/src/util/events.rs | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs index ca4e96d60..be9296161 100644 --- a/lightning/src/util/events.rs +++ b/lightning/src/util/events.rs @@ -19,7 +19,7 @@ use ln::msgs; use ln::msgs::DecodeError; use ln::{PaymentPreimage, PaymentHash, PaymentSecret}; use routing::network_graph::NetworkUpdate; -use util::ser::{Writeable, Writer, MaybeReadable, Readable, VecReadWrapper, VecWriteWrapper}; +use util::ser::{BigSize, FixedLengthReader, Writeable, Writer, MaybeReadable, Readable, VecReadWrapper, VecWriteWrapper}; use bitcoin::blockdata::script::Script; @@ -335,6 +335,9 @@ impl Writeable for Event { (2, reason, required) }); }, + // Note that, going forward, all new events must only write data inside of + // `write_tlv_fields`. Versions 0.0.101+ will ignore odd-numbered events that write + // data via `write_tlv_fields`. } Ok(()) } @@ -342,6 +345,8 @@ impl Writeable for Event { impl MaybeReadable for Event { fn read(reader: &mut R) -> Result, msgs::DecodeError> { match Readable::read(reader)? { + // Note that we do not write a length-prefixed TLV for FundingGenerationReady events, + // unlike all other events, thus we return immediately here. 0u8 => Ok(None), 1u8 => { let f = || { @@ -459,7 +464,19 @@ impl MaybeReadable for Event { Ok(Some(Event::ChannelClosed { channel_id, reason: reason.unwrap() })) }, // Versions prior to 0.0.100 did not ignore odd types, instead returning InvalidValue. - x if x % 2 == 1 => Ok(None), + // Version 0.0.100 failed to properly ignore odd types, possibly resulting in corrupt + // reads. + x if x % 2 == 1 => { + // If the event is of unknown type, assume it was written with `write_tlv_fields`, + // which prefixes the whole thing with a length BigSize. Because the event is + // odd-type unknown, we should treat it as `Ok(None)` even if it has some TLV + // fields that are even. Thus, we avoid using `read_tlv_fields` and simply read + // exactly the number of bytes specified, ignoring them entirely. + let tlv_len: BigSize = Readable::read(reader)?; + FixedLengthReader::new(reader, tlv_len.0) + .eat_remaining().map_err(|_| msgs::DecodeError::ShortRead)?; + Ok(None) + }, _ => Err(msgs::DecodeError::InvalidValue) } } -- 2.39.5