Merge pull request #854 from TheBlueMatt/2021-03-fix-lens

Fix serialization expected lengths and check them in test/fuzzing

Author: TheBlueMatt

lightning/src/ln/chan_utils.rs

@@ -39,6 +39,7 @@ use std::io::Read;
 use std::ops::Deref;
 use chain;
 
+// Maximum size of a serialized HTLCOutputInCommitment
 const HTLC_OUTPUT_IN_COMMITMENT_SIZE: usize = 1 + 8 + 4 + 32 + 5;
 
 pub(crate) const MAX_HTLCS: u16 = 483;
@@ -320,7 +321,8 @@ pub struct TxCreationKeys {
 	/// Broadcaster's Payment Key (which isn't allowed to be spent from for some delay)
 	pub broadcaster_delayed_payment_key: PublicKey,
 }
-impl_writeable!(TxCreationKeys, 33*6,
+
+impl_writeable!(TxCreationKeys, 33*5,
 	{ per_commitment_point, revocation_key, broadcaster_htlc_key, countersignatory_htlc_key, broadcaster_delayed_payment_key });
 
 /// One counterparty's public keys which do not change over the life of a channel.
@@ -427,7 +429,10 @@ pub struct HTLCOutputInCommitment {
 	pub transaction_output_index: Option<u32>,
 }
 
-impl_writeable!(HTLCOutputInCommitment, HTLC_OUTPUT_IN_COMMITMENT_SIZE, {
+impl_writeable_len_match!(HTLCOutputInCommitment, {
+		{ HTLCOutputInCommitment { transaction_output_index: None, .. }, HTLC_OUTPUT_IN_COMMITMENT_SIZE - 4 },
+		{ _, HTLC_OUTPUT_IN_COMMITMENT_SIZE }
+	}, {
 	offered,
 	amount_msat,
 	cltv_expiry,

lightning/src/ln/msgs.rs

@@ -1396,7 +1396,7 @@ impl Readable for Pong {
 
 impl Writeable for UnsignedChannelAnnouncement {
 	fn write<W: Writer>(&self, w: &mut W) -> Result<(), ::std::io::Error> {
-		w.size_hint(2 + 2*32 + 4*33 + self.features.byte_count() + self.excess_data.len());
+		w.size_hint(2 + 32 + 8 + 4*33 + self.features.byte_count() + self.excess_data.len());
 		self.features.write(w)?;
 		self.chain_hash.write(w)?;
 		self.short_channel_id.write(w)?;
@@ -1430,7 +1430,7 @@ impl Readable for UnsignedChannelAnnouncement {
 
 impl_writeable_len_match!(ChannelAnnouncement, {
 		{ ChannelAnnouncement { contents: UnsignedChannelAnnouncement {ref features, ref excess_data, ..}, .. },
-			2 + 2*32 + 4*33 + features.byte_count() + excess_data.len() + 4*64 }
+			2 + 32 + 8 + 4*33 + features.byte_count() + excess_data.len() + 4*64 }
 	}, {
 	node_signature_1,
 	node_signature_2,
@@ -1491,8 +1491,8 @@ impl Readable for UnsignedChannelUpdate {
 }
 
 impl_writeable_len_match!(ChannelUpdate, {
-		{ ChannelUpdate { contents: UnsignedChannelUpdate {ref excess_data, ..}, .. },
-			64 + excess_data.len() + 64 }
+		{ ChannelUpdate { contents: UnsignedChannelUpdate {ref excess_data, ref htlc_maximum_msat, ..}, .. },
+			64 + 64 + excess_data.len() + if let OptionalField::Present(_) = htlc_maximum_msat { 8 } else { 0 } }
 	}, {
 	signature,
 	contents
@@ -1528,7 +1528,7 @@ impl Readable for ErrorMessage {
 
 impl Writeable for UnsignedNodeAnnouncement {
 	fn write<W: Writer>(&self, w: &mut W) -> Result<(), ::std::io::Error> {
-		w.size_hint(64 + 76 + self.features.byte_count() + self.addresses.len()*38 + self.excess_address_data.len() + self.excess_data.len());
+		w.size_hint(76 + self.features.byte_count() + self.addresses.len()*38 + self.excess_address_data.len() + self.excess_data.len());
 		self.features.write(w)?;
 		self.timestamp.write(w)?;
 		self.node_id.write(w)?;
@@ -1611,7 +1611,7 @@ impl Readable for UnsignedNodeAnnouncement {
 	}
 }
 
-impl_writeable_len_match!(NodeAnnouncement, {
+impl_writeable_len_match!(NodeAnnouncement, <=, {
 		{ NodeAnnouncement { contents: UnsignedNodeAnnouncement { ref features, ref addresses, ref excess_address_data, ref excess_data, ..}, .. },
 			64 + 76 + features.byte_count() + addresses.len()*(NetAddress::MAX_LEN as usize + 1) + excess_address_data.len() + excess_data.len() }
 	}, {

lightning/src/util/config.rs

@@ -223,7 +223,7 @@ impl Default for ChannelConfig {
 }
 
 //Add write and readable traits to channelconfig
-impl_writeable!(ChannelConfig, 8+2+1+1, {
+impl_writeable!(ChannelConfig, 4+2+1+1, {
 	fee_proportional_millionths,
 	cltv_expiry_delta,
 	announced_channel,

lightning/src/util/ser_macros.rs

@@ -120,6 +120,16 @@ macro_rules! impl_writeable {
 				if $len != 0 {
 					w.size_hint($len);
 				}
+				#[cfg(any(test, feature = "fuzztarget"))]
+				{
+					// In tests, assert that the hard-coded length matches the actual one
+					if $len != 0 {
+						use util::ser::LengthCalculatingWriter;
+						let mut len_calc = LengthCalculatingWriter(0);
+						$( self.$field.write(&mut len_calc)?; )*
+						assert_eq!(len_calc.0, $len);
+					}
+				}
 				$( self.$field.write(w)?; )*
 				Ok(())
 			}
@@ -135,24 +145,36 @@ macro_rules! impl_writeable {
 	}
 }
 macro_rules! impl_writeable_len_match {
-	($st:ident, {$({$m: pat, $l: expr}),*}, {$($field:ident),*}) => {
-		impl Writeable for $st {
+	($struct: ident, $cmp: tt, {$({$match: pat, $length: expr}),*}, {$($field:ident),*}) => {
+		impl Writeable for $struct {
 			fn write<W: Writer>(&self, w: &mut W) -> Result<(), ::std::io::Error> {
-				w.size_hint(match *self {
-					$($m => $l,)*
-				});
+				let len = match *self {
+					$($match => $length,)*
+				};
+				w.size_hint(len);
+				#[cfg(any(test, feature = "fuzztarget"))]
+				{
+					// In tests, assert that the hard-coded length matches the actual one
+					use util::ser::LengthCalculatingWriter;
+					let mut len_calc = LengthCalculatingWriter(0);
+					$( self.$field.write(&mut len_calc)?; )*
+					assert!(len_calc.0 $cmp len);
+				}
 				$( self.$field.write(w)?; )*
 				Ok(())
 			}
 		}
 
-		impl ::util::ser::Readable for $st {
+		impl ::util::ser::Readable for $struct {
 			fn read<R: ::std::io::Read>(r: &mut R) -> Result<Self, DecodeError> {
 				Ok(Self {
 					$($field: Readable::read(r)?),*
 				})
 			}
 		}
+	};
+	($struct: ident, {$({$match: pat, $length: expr}),*}, {$($field:ident),*}) => {
+		impl_writeable_len_match!($struct, ==, { $({ $match, $length }),* }, { $($field),* });
 	}
 }