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

Comments

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jan 14, 2016

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

This comment has been minimized.

Copy link
Contributor Author

@bradfitz bradfitz commented Jan 21, 2016

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

This comment has been minimized.

Copy link
Contributor Author

@bradfitz bradfitz commented May 19, 2016

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

This comment has been minimized.

Copy link
Contributor Author

@bradfitz bradfitz commented May 20, 2016

(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

This comment has been minimized.

Copy link
Contributor

@appleby appleby commented Sep 9, 2016

Is this issue still up for grabs?

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

@bradfitz bradfitz commented Sep 9, 2016

Yes.

@appleby

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 16, 2016

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 16, 2016

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

@appleby

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

@bradfitz bradfitz commented Jun 14, 2017

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 18, 2017

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

@gopherbot gopherbot closed this in 77c041c Oct 18, 2017
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.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.