Require LengthLimitedRead for structs that always drain reader
#3640
Conversation
|
👋 Thanks for assigning @TheBlueMatt 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.20% 90.02% +0.82%
==========================================
Files 152 155 +3
Lines 118661 126135 +7474
Branches 118661 126135 +7474
==========================================
+ Hits 105849 113559 +7710
+ Misses 10227 10101 -126
+ Partials 2585 2475 -110 ☔ View full report in Codecov by Sentry. |
|
👋 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 |
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.
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.
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.
valentinewallace
force-pushed
the
2025-03-fixed-len-readable
branch
2 times, most recently
from
3b4e346 to
6e79392
Compare
valentinewallace
force-pushed
the
2025-03-fixed-len-readable
branch
3 times, most recently
from
16ea4dd to
3ddcf9c
Compare
When deserializing these structs, they will consume the reader until it is out of bytes. Therefore, it's safer if they are only provided with readers that limit how much of the byte stream will be read. Most of the relevant structs were updated in the previous commit when we transitioned impl_writeable_msg to use LengthReadable, here we finish the job.
valentinewallace
force-pushed
the
2025-03-fixed-len-readable
branch
from
3ddcf9c to
c6ee09b
Compare
Closes #3292