Allow the encoder to split headers across frames

[Tratcher]

The HPackEncoder has a potential infinite loop today.
https://github.com/aspnet/KestrelHttpServer/blob/55e5e564226e7b27742c91f46ea71841071e4ac3/src/Kestrel.Core/Internal/Http2/HPack/HPackEncoder.cs#L35-L42

EncodeHeader returns false if it does not have enough space to encode the value. If this header was larger than max frame size then no data will be available for the frame at all and length will be 0. The server will be stuck in an infinite loop sending zero length continuation frames.

aspnet/KestrelHttpServer#2893 added an exception to break the infinate loop, but it did so by throwing and very ungracefully aborting the socket.

Proposal: The spec allows splitting encoded headers across frames. This would allow the encoder to pack frames more densely and always make progress.

Priority: Not urgent, the response header would need to be larger than the max frame size (16kb). The client and server can also raise that frame size. Implementing real response header compression would allow even larger values before hitting the frame size limit.

[gfr-g]

Hello, I would like to take a look at the issue.

Context

Header lists are collections of zero or more header fields. When transmitted over a connection, a header list is serialized into a header block using HTTP header compression [COMPRESSION]. The serialized header block is then divided into one or more octet sequences, called header block fragments, and transmitted within the payload of HEADERS (Section 6.2), PUSH_PROMISE (Section 6.6), or CONTINUATION (Section 6.10) frames.

Brief recap

As already stated by @Tratcher, if an header doesn't completely fit inside the payload of the current frame, it will be written in the next continuation frame, leaving part of the current frame payload "empty" (or triggering the bug in case the header itself is bigger than the whole payload of a frame).
The HPackEncoder use a bool semantic for its methods to communicate with the caller whether or not it was able to write a given header inside the current frame payload.

Speculation on possible solutions

1.

• change the "encoding methods" so that they keep writing until the payload of the current frame is full

• change the return types from bool to something richer that can carry two pieces of information: the state of the encoding (header (or headers) fully or partially written) and eventually the encoded bytes remaining from a partial write, so that the Http2FrameWriter can initialize the payload correctly on the next continuation frame.

Impact
I believe that at least the HPackEncoder, the HPackHeaderWriter and the Http2FrameWriter have to be changed. The memory footprint should increase in case of a partial write because the encoded bytes that didn't fit in the current frame have to be saved and copied back into the continuation frame payload.

2.

Another solution would be to change the current design and use something like an observer pattern where the HPackHeaderWriter is the observable and the HTTP2FrameWriter is the observer. The HPackHeaderWriter encodes and writes the headers until it reaches the end of the payload. At that point it stops encoding and signals the HttP2FrameWriter so that it can send the current frame. When this finishes, the encode resume writing on the continuation frame payload from the beginning.

Impact
Code changes should be bigger but it should solve the problem of copying the encoded bytes that didn't fit in the current frame back and forth but it would add some overhead for having an actual instance of the HPackHeaderWriter (right now is static) per connection, regardless of whether or not the headers fully fit in one frame (which is, I believe, the most common case given a 16kb payload).

There might be way better ways to solve the issue, I'm looking forward to hear from the team, if the team still see value in improving this part.

Thank you.

[Tratcher]

As you noticed, this issue is pretty complicated for a fairly minor encoding optimization. You're welcome to explore it and see if it can be done without being too disruptive/complex/slow. You may also want to wait another week or so because there's some ongoing refactoring in this area for #30235.

[gfr-g]

I agree. Thanks for the heads up. While I wait for the refactoring I will think about it a little more, but if I cannot come up with something smaller I'll focus on other issues.

[ladeak]

I can confirm the issue is still present:

System.Net.Http.HPack.HPackEncodingException: Failed to HPACK encode the headers.
         at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.HPackHeaderWriter.EncodeHeadersCore(DynamicHPackEncoder hpackEncoder, Http2HeadersEnumerator headersEnumerator, Span`1 buffer, Boolean throwIfNoneEncoded, Int32& length) in D:\repos\aspnetcore\src\Servers\Kestrel\shared\HPackHeaderWriter.cs:line 121
         at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.HPackHeaderWriter.ContinueEncodeHeaders(DynamicHPackEncoder hpackEncoder, Http2HeadersEnumerator headersEnumerator, Span`1 buffer, Int32& length) in D:\repos\aspnetcore\src\Servers\Kestrel\shared\HPackHeaderWriter.cs:line 76
         at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.Http2FrameWriter.FinishWritingHeaders(Int32 streamId, Int32 payloadLength, Boolean done) in D:\repos\aspnetcore\src\Servers\Kestrel\Core\src\Internal\Http2\Http2FrameWriter.cs:line 561
         at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.Http2FrameWriter.WriteResponseHeadersUnsynchronized(Int32 streamId, Int32 statusCode, Http2HeadersFrameFlags headerFrameFlags, HttpResponseHeaders headers) in D:\repos\aspnetcore\src\Servers\Kestrel\Core\src\Internal\Http2\Http2FrameWriter.cs:line 490

[ladeak]

Looking at this issue, I also came to a similar conclusion as suggested by @gfr-g 's in the 1st proposal.

I'm not sure on changing HPackEncoder but I see it is in System namespace, so I wonder if it is copied over to this project, and if so, changing it is allowed at all. Hence, I would try to avoid touching it.

My thinking is: HPackHeaderWriter.EncodeHeadersCore when fails to encode header tries to allocate (or rent from ArrayPool) a temp buffer, where it tries to write the complete header (when not possible, tries to get a larger temp buffer). Once it manages to write the complete header, it fills the current frame with as much data as possible, and returns false to the invoker along with a reference to the remainder of this temp buffer. Next a CONTIUATION frame is being written by Http2FrameWriter.FinishWritingHeaders. This could take the buffer and flush it directly to the frame, until there is nothing left in the buffer.

Does my thinking sound reasonable? Any objections to go ahead with a PR? Any other suggestions / considerations to take before I get my hands dirty?

[amcasey]

HPackEncoder is shared between dotnet/runtime and dotnet/aspnetcore. It can be changed but, obviously, the bar is higher. Without having looked into the problem, I don't know whether that's an appropriate place to make changes, so it may not even be relevant.

[ladeak]

Does it worth exploring and opening a PR that touches HPackHeaderWriter (implemented by asp.net repo)? I am not convinced it is going to be a 'clean' solution. But happy to give an initial implementation, so then it can be decided if the approach should be further pursued.

[amcasey]

@ladeak Thanks for looking into this! I wish I could give you a more concrete answer, but I'm not familiar enough with this code to have an intuition about whether your approach is correct. Some thoughts:

1. The case where headers fit in a single frame cannot get slower - it's performance critical.
2. The team doesn't have a lot of bandwidth to dig into this and provide pointers, so any solution will involve a bunch of trial and error on your part (and providing things like tests and benchmarks to reassure the team that the solution is good). You've done a great job of doing this on other work items, but I thought it was only fair to warn you.
3. Don't be afraid to change code in System if that's the correct fix. Having said that, the shared code superficially looks like a bunch of low-level helpers that don't include the sort of logic affected by this bug/fix.
4. Do look for consumers of the same helpers dotnet/runtime - either they have the same bug (e.g. in HttpClient), in which case they should be notified, or they don't, in which case we may be able to borrow their solution.
5. This feels like two distinct problems: either the collection of headers is too large or an individual header is too large. I'm not sure there's anything we can do if it's an individual header.
6. Do look at the spec to see how it indicates this case should be handled.

Hope that helps. Please let us know if still like to tag this on so we can update the issue appropriately.

[Tratcher]

5. This feels like two distinct problems: either the collection of headers is too large or an individual header is too large. I'm not sure there's anything we can do if it's an individual header.

When the collection of headers is too large it should already be splitting into continuation frames. The problem now is individual headers, which can be changed to split across frames.

[ladeak]

Thank you for the feedback.

I can confirm what @Tratcher says is correct.
I can also see HttpClient works correctly - I will check how it handles the frames.
I think it can be made faster without making the existing cases slower - and I think even without changing dotnet/runtime.

[ladeak]

HttpClient works because it writes everything to an ArrayBuffer and then splits that into frames in PerformWriteAsync method:
https://source.dot.net/#System.Net.Http/System/Net/Http/SocketsHttpHandler/Http2Connection.cs,1625

I don't think this is an option available here, as I think (not measured) the copy of serialized header bytes will cause a perf slowdown.

[amcasey]

Having a slow path that does the extra allocation might be a way forward, but we'd have to consider whether the client could force that code path to be taken and possibly DoS the server.

[ladeak]

Another option could be being smart and splitting the encoded header itself and only writing the current corresponding part of it to the stream, but it means encoding multiple time (no extra allocation but does not help against a DoS).

[ladeak]

I am working towards a solution. Currently working on some tests and benchmarks. What I realize (and wonder) if H/3 faces a similar error. I have not yet read the HTTP3 specs yet, but QPackHeaderWriter also throws in a similar manner:

throw new QPackEncodingException("TODO sync with corefx" /* CoreStrings.HPackErrorNotEnoughBuffer */);

[amcasey]

I am working towards a solution. Currently working on some tests and benchmarks. What I realize (and wonder) if H/3 faces a similar error. I have not yet read the HTTP3 specs yet, but QPackHeaderWriter also throws in a similar manner:

throw new QPackEncodingException("TODO sync with corefx" /* CoreStrings.HPackErrorNotEnoughBuffer */);

I agree that http/3 is probably affected. I suspect this bug pre-dates http/3 support. 😆