-
-
Notifications
You must be signed in to change notification settings - Fork 270
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Not to consume buf when decoding non_huff header name(which consumes the buf) succeeded but header value failed (DecoderError::NeedMore) #589
Conversation
Anyone? |
@hikaricai it would probably be easier to review this if we had a test that failed without this change. At a quick glance, this is going to incur an additional (unnecessary?) allocation on each call. |
@olix0r thanks, I will write a test case. It seems that the "fn consum" is used only if the decoding is success, but the "fn decode_string" may only be part of decoding. For example, decoding both header name and header value. I met decoding failure when receiving continuation header which just spilt header name and header value. |
Decoding error when processing continuation header which contains normal header name at boundary
I have just updated the commit, could anyone helps to review? |
Not sure if @carllerche has bandwidth to review, but I think he knows this code best. Also I think @seanmonstar and @nox have worked on this module ~recently. If no one's available, I'll try to take a look at this but it will take me some time to load up context. |
Could anyone helps to review? I need this patch to fix bug in my project. |
Sorry about that, I'll take a look now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind helping me understand what sort of situation this happens in? So, you receive a HEADERS frame, and only the name and part of the value are in it, and the rest of the value is in a CONTINUATION frame? And currently, h2 doesn't handle that?
src/hpack/decoder.rs
Outdated
fn try_decode_string( | ||
&mut self, | ||
buf: &mut Cursor<&mut BytesMut>, | ||
) -> Result<((usize, usize), Option<Bytes>), DecoderError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's hard to know what these values are supposed to be. What if instead of returning a tuple, it return some struct DecodedString { .. }
that named the usize
s and Option<Bytes>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for reply.
I am using tonic as grpc client and would receive a large HEADERS frame(grpc-status-details-bin
) whose name is non_huff format and value is partial from C++ server. When decoding this header, the name is consumed because it is not huff, and then the value decoding is failed for DecoderError::NeedMore
, and then the decode function returns with error but the buf is consumed.
I see that the CONTINUATION header currently tested in CI but both name and value are huff_encoded.
My first patch is simply replacing consume()
with buf copy, and @olix0r pointed that the copy can be optimized. So i tried to save the decoding context and consume the buf only if both header name and header buf is successfully decoded.
I will update my patch to easily show the meaning of those values.
@seanmonstar Is the new commit clearly enough? The error situation is shown in the new test case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great now, thank you! <3
Decoding error when processing continuation frames