Skip to content
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

x/net/http2: Unhandled Setting: [MAX_HEADER_LIST_SIZE = 1048896] #13959

Closed
bradfitz opened this issue Jan 14, 2016 · 13 comments
Closed

x/net/http2: Unhandled Setting: [MAX_HEADER_LIST_SIZE = 1048896] #13959

bradfitz opened this issue Jan 14, 2016 · 13 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@bradfitz
Copy link
Contributor

The Go http2 server sends MAX_HEADER_LIST_SIZE in its initial SETTINGS frame, but the Transport doesn't use it.

Fix.

/cc @bmizerany

@bradfitz bradfitz self-assigned this Jan 14, 2016
@bradfitz bradfitz modified the milestones: Go1.6Maybe, Go1.7 Jan 14, 2016
@bradfitz
Copy link
Contributor Author

This isn't worth it for Go 1.6.

Similar to #14048 (validate transmitted header field names & values), this would involve making the Transport fail earlier before transmitting anything, telling the user that the server is likely to reject it. (except in #14048's case, the rejection is due to a spec violation, not an "advisory" soft limit, as this is).

@bradfitz
Copy link
Contributor Author

This is tricky because it requires us to back up the hpack encoder state if we find out we wrote too much in encodeHeaders. I guess encodeHeaders needs to ask hpack.Encoder return a snapshot its state, then we write everything into the bytes.Buffer and count the size as we go. Or we need to do first pass of everything, just counting size[1], then another pass doing the encoding.

[1] Remember that this isn't about the hpack-encoded bytes size, but the Sum(EachField(len(key)+len(value)+32)-ish thing:

This advisory setting informs a peer of the maximum size of header list that the sender is prepared to accept, in octets. The value is based on the uncompressed size of header fields, including the length of the name and value in octets plus an overhead of 32 octets for each header field.

I'm demoting this to a Go1.7Maybe.

@bradfitz bradfitz modified the milestones: Go1.7Maybe, Go1.7 May 19, 2016
@bradfitz bradfitz added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels May 20, 2016
@bradfitz
Copy link
Contributor Author

(I'm going away for a month. If anybody else wants to fix this before Jun 23rd, feel free.)

@bradfitz bradfitz modified the milestones: Go1.8, Go1.7Maybe Jun 27, 2016
@appleby
Copy link
Contributor

appleby commented Sep 9, 2016

Is this issue still up for grabs?

@bradfitz
Copy link
Contributor Author

bradfitz commented Sep 9, 2016

Yes.

@appleby
Copy link
Contributor

appleby commented Sep 9, 2016

Ok, I'll have a look. My idea is to update (*ClientConn) encodeHeaders to implement your second suggestion from above, i.e., do a first pass counting bytes, then a second pass to write the headers. Does that sound reasonable?

@appleby
Copy link
Contributor

appleby commented Sep 9, 2016

Above comment refers to client-side only. Looking into the server side, I see there is a corresponding encodeHeaders function, but it seems there are also a few calls to encKV sprinkled throughout write.go. These additional calls to encKV are responsible for only a few tens of bytes I would guess, and at first glance it looks like they could all be shoehorned into encodeHeaders anyway.

@gopherbot
Copy link

CL https://golang.org/cl/29242 mentions this issue.

@gopherbot
Copy link

CL https://golang.org/cl/29243 mentions this issue.

@appleby
Copy link
Contributor

appleby commented Sep 16, 2016

I've mailed a changelist covering only the client portion in order to get feedback before I start on the server side (assuming we even want to handle this on the server).

@bradfitz bradfitz modified the milestones: Go1.9, Go1.8 Nov 29, 2016
@bradfitz bradfitz modified the milestones: Unreleased, Go1.9 Jun 14, 2017
@bradfitz bradfitz removed their assignment Jun 14, 2017
@bradfitz
Copy link
Contributor Author

/cc @tombergan

gopherbot pushed a commit to golang/net that referenced this issue Aug 28, 2017
Add a new peerMaxHeaderListSize member to ClientConn which records the
SETTINGS_MAX_HEADER_LIST_SIZE requested by the client's peer, and
respect this limit in (*ClientConn) encodeHeaders / encodeTrailers.

Attempting to send more than peerMaxHeaderListSize bytes of headers or
trailers will result in RoundTrip returning errRequestHeaderListSize.

Updates golang/go#13959

Change-Id: Ic707179782acdf8ae543429ea1af7f4f30e67e59
Reviewed-on: https://go-review.googlesource.com/29243
Run-TryBot: Tom Bergan <tombergan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Tom Bergan <tombergan@google.com>
@appleby
Copy link
Contributor

appleby commented Aug 30, 2017

The client-side fix has been committed. Thanks @tombergan for reviewing.

It will be at least a month before I have a chance to look into the server-side (laptop dead, parts on the slow boat from China), so If anyone else wants to tackle it before then, feel free.

@gopherbot
Copy link

Change https://golang.org/cl/71611 mentions this issue: net/http: update bundled http2

c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
Add a new peerMaxHeaderListSize member to ClientConn which records the
SETTINGS_MAX_HEADER_LIST_SIZE requested by the client's peer, and
respect this limit in (*ClientConn) encodeHeaders / encodeTrailers.

Attempting to send more than peerMaxHeaderListSize bytes of headers or
trailers will result in RoundTrip returning errRequestHeaderListSize.

Updates golang/go#13959

Change-Id: Ic707179782acdf8ae543429ea1af7f4f30e67e59
Reviewed-on: https://go-review.googlesource.com/29243
Run-TryBot: Tom Bergan <tombergan@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Tom Bergan <tombergan@google.com>
@golang golang locked and limited conversation to collaborators Oct 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants