Closed
Conversation
HTTP2 headers are sent in (potentially) many frames, but all must be sent sequentially with no traffic intervening. This was not clear when I wrote the HPACK parser, and still indeed quite contentious on the HTTP2 mailing lists. Now that matter is well settled (years ago!) take advantage of the fact by delaying parsing until all bytes are available. A future change will leverage this to avoid having to store and verify partial parse state, completely eliminating indirect calls within the parser.
This reverts commit 258d712.
ctiller
added
disposition/Work In Progress
release notes: no
Member
Author
|
I should fix that buffering issue... will send something out momentarily.
I'd like to avoid the complexity of having to even try to restart mid
header - it was adding a lot here, and we don't need it.
…On Thu, Jul 29, 2021 at 8:36 AM Eric Anderson ***@***.***> wrote:
I don't see the code in #26700 <#26700>
to deal with over-buffering. So it is unclear how much issue "parse
everything at once" will cause. I'd suggest you still have a streaming
parser, but decode a header in its entirety at a time and just re-start
that one header's parsing when you receive more data.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#26787 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACNG45JVURB4PNBQJFWLSJDT2FYRLANCNFSM5BAMMDBA>
.
|
Member
Well, MAX_HEADER_LIST_SIZE is advisory and the only option for dealing with overbuffering without stream-parsing headers is killing the connection, which combined together can produce very bad results. From what I've seen, it doesn't require much state to restart on header boundary. The main thing it requires is the various parts of the parsing to be able to fail due to too little buffer and ifs to propagate that out through the layers. |
Member
Author
|
We do kill the connection if HEADER_LIST_SIZE is exceeded, and it seems
there's a reasonable upper bound on the amount you need to buffer before
it's provable that you've exceeded HEADER_LIST_SIZE.
…On Thu, Jul 29, 2021 at 8:49 AM Eric Anderson ***@***.***> wrote:
even try to restart mid
header - it was adding a lot here, and we don't need it.
Well, MAX_HEADER_LIST_SIZE is advisory and the only option for dealing
with overbuffering without stream-parsing headers is killing the
connection, which combined together can produce very bad results.
From what I've seen, it doesn't require much state to restart on header
boundary. The main thing it requires is the various parts of the parsing to
be able to fail due to too little buffer and ifs to propagate that out
through the layers.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#26787 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACNG45LPWMISUGRDKT3SMJLT2FZ7JANCNFSM5BAMMDBA>
.
|
Member
Author
|
Oh scratch that, yep you're right.
…On Thu, Jul 29, 2021 at 9:01 AM Craig Tiller ***@***.***> wrote:
We do kill the connection if HEADER_LIST_SIZE is exceeded, and it seems
there's a reasonable upper bound on the amount you need to buffer before
it's provable that you've exceeded HEADER_LIST_SIZE.
On Thu, Jul 29, 2021 at 8:49 AM Eric Anderson ***@***.***>
wrote:
> even try to restart mid
> header - it was adding a lot here, and we don't need it.
>
> Well, MAX_HEADER_LIST_SIZE is advisory and the only option for dealing
> with overbuffering without stream-parsing headers is killing the
> connection, which combined together can produce very bad results.
>
> From what I've seen, it doesn't require much state to restart on header
> boundary. The main thing it requires is the various parts of the parsing to
> be able to fail due to too little buffer and ifs to propagate that out
> through the layers.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#26787 (comment)>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ACNG45LPWMISUGRDKT3SMJLT2FZ7JANCNFSM5BAMMDBA>
> .
>
|
Member
|
So for compatibility with C core clients should consider MAX_HEADER_LIST_SIZE mandatory. I understand you are allowed to kill the connection when things become excessive, but that seems draconian. |
Member
|
One middle-ground solution is to let, say, 2x MAX_HEADER_LIST_SIZE buffering HEADERS and respond with a clean error. If the 2x limit is reached then kill the connection. But when MAX_HEADER_LIST_SIZE is 8 KB, 2x doesn't go very far. |
Member
Author
|
Yeah, I was going to suggest 4x originally... I'm leaning towards that
again tbh, since it does get reasonably more complicated to stream here.
…On Thu, Jul 29, 2021 at 9:08 AM Eric Anderson ***@***.***> wrote:
One middle-ground solution is to let, say, 2x MAX_HEADER_LIST_SIZE
buffering HEADERS and respond with a clean error. If the 2x limit is
reached then kill the connection. But when MAX_HEADER_LIST_SIZE is 8 KB, 2x
doesn't go very far.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#26787 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACNG45PHVQ6GT7S42JHXWYDT2F4GVANCNFSM5BAMMDBA>
.
|
Member
Author
|
We don't currently kill the connection, but I think if we saw enough bytes
coming in on a header block, I'd be ok with that.
…On Thu, Jul 29, 2021 at 9:51 AM Craig Tiller ***@***.***> wrote:
Yeah, I was going to suggest 4x originally... I'm leaning towards that
again tbh, since it does get reasonably more complicated to stream here.
On Thu, Jul 29, 2021 at 9:08 AM Eric Anderson ***@***.***>
wrote:
> One middle-ground solution is to let, say, 2x MAX_HEADER_LIST_SIZE
> buffering HEADERS and respond with a clean error. If the 2x limit is
> reached then kill the connection. But when MAX_HEADER_LIST_SIZE is 8 KB, 2x
> doesn't go very far.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#26787 (comment)>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ACNG45PHVQ6GT7S42JHXWYDT2F4GVANCNFSM5BAMMDBA>
> .
>
|
Member
Author
|
This is now a streaming parser. |
Member
Author
|
And I'd claim ready for review for real this time. |
Member
|
Nicely readable. Something to be wary of: a very long header entry. E.g., let's say a header value is MAX_INT in length. If an entry is greater than MAX_HEADER_LIST_SIZE and greater than the HPACK table size, then it can be skipped (and the HPACK table would be cleared). |
Merged
Member
Author
|
@yashykt ping on this review |
Member
Author
|
Ping? |
yashykt
approved these changes
Aug 9, 2021
Member
yashykt
left a comment
yashykt
left a comment
There was a problem hiding this comment.
net effect: -347 LOC! Love it!
Member
Author
|
Merged with #26851 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR leverages the work done in #26700 to eliminate all parsing state stored on HPackParser, and the need for a state machine driven parser at all.