codec: Fix panic in LengthDelimitedCodec::encode
#682
Conversation
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
src/codec/length_delimited.rs
Outdated
| // If there's no room to write the length field in `dst`, reserve more | ||
| // capacity so we don't overflow the buffer. | ||
| if dst.remaining_mut() < self.builder.length_field_len { | ||
| dst.reserve(self.builder.length_field_len); |
There was a problem hiding this comment.
As a potential optimization, we might want to reserve enough buffer capacity for both the length field and the frame here.
The call to BytesMut::extend_from_slice on line 589 later on won't panic, as that method will resize the buffer if there's insufficient capacity to fit the frame. However, if we grow the buffer enough to fit the frame here, we might be able to do a single allocation rather than two?
There was a problem hiding this comment.
I think doing the one allocation is preferential to (potentially) two.
There was a problem hiding this comment.
I agree, since the total needed size is known, it should be reserved up front.
Also, you do not need to guard the reserve function.
There was a problem hiding this comment.
Okay, thanks @carllerche and @tobz for confirming my suspicion that reserving all the necessary capacity here was the Right Thing 😄 --- I've made that change in 256fa57.
Also, I think that since we're reserving sufficient capacity here, the BytesMut::extend_from_slice on line 583 could just be changed to a BufMut::push_slice --- @carllerche, is there significant additional overhead in calling extend_from_slice when the BytesMut has sufficient capacity for the slice?
hawkw
changed the title
LengthDelimitedCodec::encode
src/codec/length_delimited.rs
Outdated
| // If there's no room to write the length field in `dst`, reserve more | ||
| // capacity so we don't overflow the buffer. | ||
| if dst.remaining_mut() < self.builder.length_field_len { | ||
| dst.reserve(self.builder.length_field_len); |
There was a problem hiding this comment.
I think doing the one allocation is preferential to (potentially) two.
src/codec/length_delimited.rs
Outdated
| // If there's no room to write the length field in `dst`, reserve more | ||
| // capacity so we don't overflow the buffer. | ||
| if dst.remaining_mut() < self.builder.length_field_len { | ||
| dst.reserve(self.builder.length_field_len); |
There was a problem hiding this comment.
I agree, since the total needed size is known, it should be reserved up front.
Also, you do not need to guard the reserve function.
Fixes: #681
Motivation
Currently, a potential panic exists in
LengthDelimitedCodec::encode.Writing the length field to the
dstbuffer can exceed the buffercapacity, as
BufMut::put_uint_{le,be}doesn't reserve more capacity.Solution
This branch adds a check that there's sufficient remaining buffer
capacity to hold the length field, and resizes the buffer to hold the
length field if there is no capacity left.
I've also added a test that reproduces the issue. The test panics on
master, but passes after making this change.
Signed-off-by: Eliza Weisman eliza@buoyant.io