Require LengthLimitedRead for structs that always drain reader
#3640
Conversation
|
👋 Thanks for assigning @arik-so as a reviewer! |
valentinewallace
force-pushed
the
2025-03-fixed-len-readable
branch
from
6f66ebb to
47916c1
Compare
|
Looking for concept ACKs before fixing fuzz! |
|
Also do we also want to do the same for |
valentinewallace
force-pushed
the
2025-03-fixed-len-readable
branch
from
47916c1 to
792fcf0
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3640 +/- ##
==========================================
+ Coverage 89.24% 90.37% +1.12%
==========================================
Files 155 155
Lines 119269 127304 +8035
Branches 119269 127304 +8035
==========================================
+ Hits 106442 115050 +8608
+ Misses 10231 9722 -509
+ Partials 2596 2532 -64 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
|
nit: might point out in the last commit message that the remaining structs are the ones not covered by impl_writeable_msg |
valentinewallace
force-pushed
the
2025-03-fixed-len-readable
branch
2 times, most recently
from
36698bb to
b95f2ce
Compare
|
nit: squash the formatting commit in between getting the second ACK and merging. |
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
lightning/src/util/ser.rs
Outdated
| /// 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 [`FixedLengthReader`]. | ||
| fn read_from_fixed_length_buffer<'a, R: Read>( |
There was a problem hiding this comment.
We should either use a trait here, rather than forcing a FixedLengthReader, or we should remove LengthRead entirely (which looks ~unused now?). It'd be kinda nice to use a trait, I think, but maybe you thought it wouldn't be strict enough? I guess we could kinda just rename LengthRead as LengthLimiedRead and describe the semantics?
There was a problem hiding this comment.
Haha I thought you said several times offline that LengthRead as-is wasn't sufficient so I went with the stricter option... I'll move forward with just renaming the current trait and adding docs.
There was a problem hiding this comment.
Errr, I meant LengthReadable isn't sufficient. I don't think we have to fix it by forcing a LengthLimitingReader, but doing so is fine, I just wanted to highlight that we are leaving LengthRead unused here, and should remove it, but also that we could use that with suffucient method/trait renames.
fuzz/src/chanmon_consistency.rs
Outdated
| @@ -1101,7 +1101,9 @@ pub fn do_test<Out: Output>(data: &[u8], underlying_out: Out, anchors: bool) { | |||
| // update_fail_htlc as we do when we reject a payment. | |||
| let mut msg_ser = update_add.encode(); | |||
| msg_ser[1000] ^= 0xff; | |||
| let new_msg = UpdateAddHTLC::read(&mut Cursor::new(&msg_ser)).unwrap(); | |||
| let mut cursor = Cursor::new(&msg_ser); | |||
There was a problem hiding this comment.
nit: we don't actually need a Cursor, we can just &mut &msg_ser[..]. Same elsewhere in this commit. Would be nice to slowly phase out Cursor as we touch it.
There was a problem hiding this comment.
I started making this change, had to implement LengthLimitedRead for &[u8]. It ended up being weird because the Read implementation for a slice advances the slice to point to the yet-unread part, which means that LengthLimitedRead::total_bytes will be inaccurate after any part of the struct is read...
I'm not sure this is the way to go, when reading a TrampolinePacket for example it "successfully" read but due to this subtlety it didn't read the whole packet.
There was a problem hiding this comment.
Hmm, I almost prefer it that way? Like, having a "how many bytes are in the buffer" call is weird cause the caller may have already read many bytes. "How many bytes are left in the buffer" is a way more sensible question to ask, IMO.
| @@ -1033,8 +1034,10 @@ impl OfferContents { | |||
| } | |||
| } | |||
|
|
|||
| impl Readable for Offer { | |||
| fn read<R: io::Read>(reader: &mut R) -> Result<Self, DecodeError> { | |||
| impl LengthReadable for Offer { | |||
There was a problem hiding this comment.
Don't we need to do this for all the other bolt 12 structs too?
There was a problem hiding this comment.
Did this and a few other places in ser.rs.
valentinewallace
changed the title
FixedLengthReadable for structs that always drain readerLengthLimitedRead for structs that always drain reader
valentinewallace
force-pushed
the
2025-03-fixed-len-readable
branch
from
b95f2ce to
db17178
Compare
|
Will fix the build and a few more instances where |
valentinewallace
force-pushed
the
2025-03-fixed-len-readable
branch
6 times, most recently
from
3ddcf9c to
c6ee09b
Compare
valentinewallace
force-pushed
the
2025-03-fixed-len-readable
branch
from
c6ee09b to
3fa78e5
Compare
| @@ -104,7 +104,7 @@ | |||
| //! impl CustomMessageReader for BarHandler { | |||
| //! // ... | |||
| //! # type CustomMessage = Bar; | |||
| //! # fn read<R: io::Read>( | |||
| //! # fn read<R: LengthLimitedRead>( | |||
There was a problem hiding this comment.
Does it make sense to rename CustomMessageReader::read to match the LengthReadable version?
There was a problem hiding this comment.
I thought the reason we renamed the method last time was to avoid conflicting implementations between LengthReadable::read and Readable::read, which doesn't seem to be the case here? But I'm happy to rename it if you prefer.
There was a problem hiding this comment.
Its also clearer to the reader, but we can leave it too. It should remain clear given the buffer type is restricted.
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 length limiting reader. Upcoming commits will switch read-to-end structs that are currently read with Readable to use LengthReadable with LengthLimitedRead instead.
We want to slowly phase out the use of Cursor in the codebase, which means we want to be able to call LengthLimitedRead::read(&mut &slice_of_bytes[..]) instead of wrapping the slice in a Cursor. However, this breaks the current LengthLimitedRead::total_bytes method because the underlying Read implementation for slice advances the slice as it is read, so slice.len() can't be used to get the total bytes after any part of the struct is read. Therefore here we also switch the ::total_bytes method to ::remaining_bytes, which seems like a more sensible method anyway since the reader is being consumed.
valentinewallace
force-pushed
the
2025-03-fixed-len-readable
branch
from
2f4c0de to
6d13257
Compare
See prior two commits. When deserializing objects via this macro, there is no length prefix so the deser code will read the provided reader until it runs out of bytes. Readable is not an appropriate trait for this situation because it should only be used for structs that are prefixed with a length and know when to stop reading. LengthReadable instead requires that the caller supply only the bytes that are reserved for this struct.
Easier to review the previous commit this way.
Continuing the work of the last few commits, here we require that some LN gossip messages be read from a length limiting reader. The messages' updates are separated into their own commit because they definitely should use a length limited reader but they also require breaking out of the ser macros. We can't use the macros because each message contains a non-TLV field that will always consume the rest of the reader, while the ser macros only support reading non-TLV fields that know when to stop reading.
When deserializing lightning messages, many consume the reader until it is out of bytes, or could in the future if new fields or TLVs are added. Therefore, it's safer if they are only provided with readers that limit how much of the byte stream will be read. Many of the relevant LN messages were updated in a prior commit when we transitioned impl_writeable_msg to require length limiting readers, here we finish the job.
When reading a raw LSP message specifically, it will consume the provided reader until it is out of bytes. Readable is not an appropriate trait for this situation, and over the past few commits we've been transitioning any struct that always reads-to-end to require a length limited reader. This is less error-prone because there is no risk of a bug being introduced because a custom message read more data than it was supposed to from the provided reader.
Continuing the work over the past few commits, we want to transition any structs that always consume the entire provided reader when being deserialized to require a length-limiting reader. To do this we actually require all the offers' underlying ser wrappers to be read from a length-limiting reader as well, since these wrappers will always read-to-end in general. Some of the ser macros needed updating for this, which is fine because all TLV values were already read from a length-limiting reader.
valentinewallace
force-pushed
the
2025-03-fixed-len-readable
branch
from
6d13257 to
94f8952
Compare
|
Got rid of an unused lifetime (surprised there's no warning for that): diff --git a/lightning/src/ln/wire.rs b/lightning/src/ln/wire.rs
index 20e1ca789..1bb7c7448 100644
--- a/lightning/src/ln/wire.rs
+++ b/lightning/src/ln/wire.rs
@@ -238,7 +238,7 @@ impl<T: core::fmt::Debug + Type + TestEq> Message<T> {
/// # Errors
///
/// Returns an error if the message payload could not be decoded as the specified type.
-pub(crate) fn read<'a, R: LengthLimitedRead, T, H: core::ops::Deref>(
+pub(crate) fn read<R: LengthLimitedRead, T, H: core::ops::Deref>(
buffer: &mut R, custom_reader: H,
) -> Result<Message<T>, (msgs::DecodeError, Option<u16>)>
where
@@ -249,7 +249,7 @@ where
do_read(buffer, message_type, custom_reader).map_err(|e| (e, Some(message_type)))
}
-fn do_read<'a, R: LengthLimitedRead, T, H: core::ops::Deref>(
+fn do_read<R: LengthLimitedRead, T, H: core::ops::Deref>(
buffer: &mut R, message_type: u16, custom_reader: H,
) -> Result<Message<T>, msgs::DecodeError>
where |
There was a problem hiding this comment.
should this commit remain or be squashed out pre-merge?
There was a problem hiding this comment.
I assume you're referring to the rustfmtone? No reason to squash, IMO. Anything that helps a reviewer now helps a reviewer trying to understand the code in three years and it doesn't violate our "each commit should, on its own, build and pass tests" rule.
I felt weird using |
|
fair enough! |
Closes #3292