Not to consume buf when decoding non_huff header name(which consumes the buf) succeeded but header value failed (DecoderError::NeedMore)

[hikaricai]

Decoding error when processing continuation frames

[hikaricai]

Anyone？

[olix0r]

@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.

[hikaricai]

@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.

[hikaricai]

I have just updated the commit, could anyone helps to review?

[olix0r]

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.

[hikaricai]

Could anyone helps to review? I need this patch to fix bug in my project.

[seanmonstar]

Sorry about that, I'll take a look now.

[seanmonstar]

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?

[seanmonstar]

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 usizes and Option<Bytes>?

[hikaricai]

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.

[hikaricai]

@seanmonstar Is the new commit clearly enough?

The error situation is shown in the new test case

[seanmonstar]

Looks great now, thank you! <3