Rename LengthRead to LengthLimitedRead

In general, the codebase currently tends to use the Readable trait with two
different semantics -- some objects know when to stop reading because they have
a length prefix and some automatically read to the end of the reader.

This differing usage can lead to bugs. For objects that read-to-end, it would
be much safer if the compiler enforced that they could only be read from a
length-limiting reader.

We already have a LengthRead trait that requires the underlying reader to
provide the length, which is similar to what we want. So rather than adding an
additional trait, here we rename LengthRead and update its semantics to require
a fixed length reader.

Upcoming commits will switch read-to-end structs that are currently read with
Readable to use LengthReadable with LengthLimitedRead instead.

Author: valentinewallace

lightning/src/crypto/streams.rs

@@ -4,7 +4,7 @@ use crate::crypto::chacha20poly1305rfc::ChaCha20Poly1305RFC;
 use crate::io::{self, Read, Write};
 use crate::ln::msgs::DecodeError;
 use crate::util::ser::{
-	FixedLengthReader, LengthRead, LengthReadableArgs, Readable, Writeable, Writer,
+	FixedLengthReader, LengthLimitedRead, LengthReadableArgs, Readable, Writeable, Writer,
 };
 
 pub(crate) struct ChaChaReader<'a, R: io::Read> {
@@ -56,10 +56,10 @@ pub(crate) struct ChaChaPolyReadAdapter<R: Readable> {
 }
 
 impl<T: Readable> LengthReadableArgs<[u8; 32]> for ChaChaPolyReadAdapter<T> {
-	// Simultaneously read and decrypt an object from a LengthRead, storing it in Self::readable.
-	// LengthRead must be used instead of std::io::Read because we need the total length to separate
-	// out the tag at the end.
-	fn read<R: LengthRead>(r: &mut R, secret: [u8; 32]) -> Result<Self, DecodeError> {
+	// Simultaneously read and decrypt an object from a LengthLimitedRead storing it in
+	// Self::readable. LengthLimitedRead must be used instead of std::io::Read because we need the
+	// total length to separate out the tag at the end.
+	fn read<R: LengthLimitedRead>(r: &mut R, secret: [u8; 32]) -> Result<Self, DecodeError> {
 		if r.total_bytes() < 16 {
 			return Err(DecodeError::InvalidValue);
 		}

lightning/src/ln/msgs.rs

@@ -59,7 +59,7 @@ use crate::io_extras::read_to_end;
 
 use crate::crypto::streams::ChaChaPolyReadAdapter;
 use crate::util::logger;
-use crate::util::ser::{BigSize, FixedLengthReader, HighZeroBytesDroppedBigSize, Hostname, LengthRead, LengthReadable, LengthReadableArgs, Readable, ReadableArgs, TransactionU16LenLimited, WithoutLength, Writeable, Writer};
+use crate::util::ser::{BigSize, FixedLengthReader, HighZeroBytesDroppedBigSize, Hostname, LengthLimitedRead, LengthReadable, LengthReadableArgs, Readable, ReadableArgs, TransactionU16LenLimited, WithoutLength, Writeable, Writer};
 use crate::util::base32;
 
 use crate::routing::gossip::{NodeAlias, NodeId};
@@ -2323,7 +2323,7 @@ impl Writeable for TrampolineOnionPacket {
 }
 
 impl LengthReadable for TrampolineOnionPacket {
-	fn read_from_fixed_length_buffer<R: LengthRead>(r: &mut R) -> Result<Self, DecodeError> {
+	fn read_from_fixed_length_buffer<R: LengthLimitedRead>(r: &mut R) -> Result<Self, DecodeError> {
 		let version = Readable::read(r)?;
 		let public_key = Readable::read(r)?;
 

lightning/src/onion_message/packet.rs

@@ -23,7 +23,7 @@ use crate::ln::msgs::DecodeError;
 use crate::ln::onion_utils;
 use crate::util::logger::Logger;
 use crate::util::ser::{
-	BigSize, FixedLengthReader, LengthRead, LengthReadable, LengthReadableArgs, Readable,
+	BigSize, FixedLengthReader, LengthLimitedRead, LengthReadable, LengthReadableArgs, Readable,
 	ReadableArgs, Writeable, Writer,
 };
 
@@ -83,7 +83,7 @@ impl Writeable for Packet {
 }
 
 impl LengthReadable for Packet {
-	fn read_from_fixed_length_buffer<R: LengthRead>(r: &mut R) -> Result<Self, DecodeError> {
+	fn read_from_fixed_length_buffer<R: LengthLimitedRead>(r: &mut R) -> Result<Self, DecodeError> {
 		const READ_BUFFER_SIZE: usize = 4096;
 
 		let version = Readable::read(r)?;

lightning/src/util/ser.rs

@@ -226,7 +226,7 @@ impl<'a, R: Read> Read for FixedLengthReader<'a, R> {
 	}
 }
 
-impl<'a, R: Read> LengthRead for FixedLengthReader<'a, R> {
+impl<'a, R: Read> LengthLimitedRead for FixedLengthReader<'a, R> {
 	#[inline]
 	fn total_bytes(&self) -> u64 {
 		self.total_bytes
@@ -344,8 +344,10 @@ where
 	fn read<R: Read>(reader: &mut R, params: P) -> Result<Self, DecodeError>;
 }
 
-/// A [`io::Read`] that also provides the total bytes available to be read.
-pub trait LengthRead: Read {
+/// A [`io::Read`] that limits the amount of bytes that can be read. Implementations should ensure
+/// that the object being read will only consume a fixed number of bytes from the underlying
+/// [`io::Read`], see [`FixedLengthReader`] for an example.
+pub trait LengthLimitedRead: Read {
 	/// The total number of bytes available to be read.
 	fn total_bytes(&self) -> u64;
 }
@@ -357,26 +359,30 @@ pub(crate) trait LengthReadableArgs<P>
 where
 	Self: Sized,
 {
-	/// Reads a `Self` in from the given [`LengthRead`].
-	fn read<R: LengthRead>(reader: &mut R, params: P) -> Result<Self, DecodeError>;
+	/// Reads a `Self` in from the given [`LengthLimitedRead`].
+	fn read<R: LengthLimitedRead>(reader: &mut R, params: P) -> Result<Self, DecodeError>;
 }
 
-/// A trait that allows the implementer to be read in from a [`LengthRead`], requiring
-/// the reader to provide the total length of the read.
+/// A trait that allows the implementer to be read in from a [`LengthLimitedRead`], requiring the
+/// reader to provide the total length of the read.
 ///
 /// Any type that implements [`Readable`] also automatically has a [`LengthReadable`]
 /// implementation, but some types, most notably onion packets, only implement [`LengthReadable`].
 pub trait LengthReadable
 where
 	Self: Sized,
 {
-	/// Reads a `Self` in from the given [`LengthRead`].
-	fn read_from_fixed_length_buffer<R: LengthRead>(reader: &mut R) -> Result<Self, DecodeError>;
+	/// Reads a `Self` in from the given [`LengthLimitedRead`].
+	fn read_from_fixed_length_buffer<R: LengthLimitedRead>(
+		reader: &mut R,
+	) -> Result<Self, DecodeError>;
 }
 
 impl<T: Readable> LengthReadable for T {
 	#[inline]
-	fn read_from_fixed_length_buffer<R: LengthRead>(reader: &mut R) -> Result<T, DecodeError> {
+	fn read_from_fixed_length_buffer<R: LengthLimitedRead>(
+		reader: &mut R,
+	) -> Result<T, DecodeError> {
 		Readable::read(reader)
 	}
 }
@@ -405,7 +411,9 @@ impl<T: Readable> MaybeReadable for T {
 pub struct RequiredWrapper<T>(pub Option<T>);
 impl<T: LengthReadable> LengthReadable for RequiredWrapper<T> {
 	#[inline]
-	fn read_from_fixed_length_buffer<R: LengthRead>(reader: &mut R) -> Result<Self, DecodeError> {
+	fn read_from_fixed_length_buffer<R: LengthLimitedRead>(
+		reader: &mut R,
+	) -> Result<Self, DecodeError> {
 		Ok(Self(Some(LengthReadable::read_from_fixed_length_buffer(reader)?)))
 	}
 }