net/http: update bundled http2

Updates http2 to x/net/http2 git rev 1087133bc4a for:

  http2: reject DATA frame before HEADERS frame
  https://golang.org/cl/56770

  http2: respect peer's SETTINGS_MAX_HEADER_LIST_SIZE in ClientConn
  https://golang.org/cl/29243

  http2: reset client stream after processing response headers
  https://golang.org/cl/70510

Also updated TestRequestLimit_h2 as the behavior changed slightly due
to https://golang.org/cl/29243.

Fixes #13959
Fixes #20521
Fixes #21466

Change-Id: Iac659694f3a48b8bd485546a4f96a932e3056026
Reviewed-on: https://go-review.googlesource.com/71611
Run-TryBot: Tom Bergan <tombergan@google.com>
Reviewed-by: Joe Tsai <thebrokentoaster@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

src/net/http/h2_bundle.go

@@ -6606,7 +6606,7 @@ type http2Transport struct {
 
 	// MaxHeaderListSize is the http2 SETTINGS_MAX_HEADER_LIST_SIZE to
 	// send in the initial settings frame. It is how many bytes
-	// of response headers are allow. Unlike the http2 spec, zero here
+	// of response headers are allowed. Unlike the http2 spec, zero here
 	// means to use a default limit (currently 10MB). If you actually
 	// want to advertise an ulimited value to the peer, Transport
 	// interprets the highest possible value here (0xffffffff or 1<<32-1)
@@ -6691,9 +6691,10 @@ type http2ClientConn struct {
 	fr              *http2Framer
 	lastActive      time.Time
 	// Settings from peer: (also guarded by mu)
-	maxFrameSize         uint32
-	maxConcurrentStreams uint32
-	initialWindowSize    uint32
+	maxFrameSize          uint32
+	maxConcurrentStreams  uint32
+	peerMaxHeaderListSize uint64
+	initialWindowSize     uint32
 
 	hbuf    bytes.Buffer // HPACK encoder writes into this
 	henc    *hpack.Encoder
@@ -7038,17 +7039,18 @@ func (t *http2Transport) NewClientConn(c net.Conn) (*http2ClientConn, error) {
 
 func (t *http2Transport) newClientConn(c net.Conn, singleUse bool) (*http2ClientConn, error) {
 	cc := &http2ClientConn{
-		t:                    t,
-		tconn:                c,
-		readerDone:           make(chan struct{}),
-		nextStreamID:         1,
-		maxFrameSize:         16 << 10, // spec default
-		initialWindowSize:    65535,    // spec default
-		maxConcurrentStreams: 1000,     // "infinite", per spec. 1000 seems good enough.
-		streams:              make(map[uint32]*http2clientStream),
-		singleUse:            singleUse,
-		wantSettingsAck:      true,
-		pings:                make(map[[8]byte]chan struct{}),
+		t:                     t,
+		tconn:                 c,
+		readerDone:            make(chan struct{}),
+		nextStreamID:          1,
+		maxFrameSize:          16 << 10,           // spec default
+		initialWindowSize:     65535,              // spec default
+		maxConcurrentStreams:  1000,               // "infinite", per spec. 1000 seems good enough.
+		peerMaxHeaderListSize: 0xffffffffffffffff, // "infinite", per spec. Use 2^64-1 instead.
+		streams:               make(map[uint32]*http2clientStream),
+		singleUse:             singleUse,
+		wantSettingsAck:       true,
+		pings:                 make(map[[8]byte]chan struct{}),
 	}
 	if d := t.idleConnTimeout(); d != 0 {
 		cc.idleTimeout = d
@@ -7604,8 +7606,13 @@ func (cs *http2clientStream) writeRequestBody(body io.Reader, bodyCloser io.Clos
 	var trls []byte
 	if hasTrailers {
 		cc.mu.Lock()
-		defer cc.mu.Unlock()
-		trls = cc.encodeTrailers(req)
+		trls, err = cc.encodeTrailers(req)
+		cc.mu.Unlock()
+		if err != nil {
+			cc.writeStreamReset(cs.ID, http2ErrCodeInternal, err)
+			cc.forgetStreamID(cs.ID)
+			return err
+		}
 	}
 
 	cc.wmu.Lock()
@@ -7708,62 +7715,86 @@ func (cc *http2ClientConn) encodeHeaders(req *Request, addGzipHeader bool, trail
 		}
 	}
 
-	// 8.1.2.3 Request Pseudo-Header Fields
-	// The :path pseudo-header field includes the path and query parts of the
-	// target URI (the path-absolute production and optionally a '?' character
-	// followed by the query production (see Sections 3.3 and 3.4 of
-	// [RFC3986]).
-	cc.writeHeader(":authority", host)
-	cc.writeHeader(":method", req.Method)
-	if req.Method != "CONNECT" {
-		cc.writeHeader(":path", path)
-		cc.writeHeader(":scheme", req.URL.Scheme)
-	}
-	if trailers != "" {
-		cc.writeHeader("trailer", trailers)
-	}
-
-	var didUA bool
-	for k, vv := range req.Header {
-		lowKey := strings.ToLower(k)
-		switch lowKey {
-		case "host", "content-length":
-			// Host is :authority, already sent.
-			// Content-Length is automatic, set below.
-			continue
-		case "connection", "proxy-connection", "transfer-encoding", "upgrade", "keep-alive":
-			// Per 8.1.2.2 Connection-Specific Header
-			// Fields, don't send connection-specific
-			// fields. We have already checked if any
-			// are error-worthy so just ignore the rest.
-			continue
-		case "user-agent":
-			// Match Go's http1 behavior: at most one
-			// User-Agent. If set to nil or empty string,
-			// then omit it. Otherwise if not mentioned,
-			// include the default (below).
-			didUA = true
-			if len(vv) < 1 {
+	enumerateHeaders := func(f func(name, value string)) {
+		// 8.1.2.3 Request Pseudo-Header Fields
+		// The :path pseudo-header field includes the path and query parts of the
+		// target URI (the path-absolute production and optionally a '?' character
+		// followed by the query production (see Sections 3.3 and 3.4 of
+		// [RFC3986]).
+		f(":authority", host)
+		f(":method", req.Method)
+		if req.Method != "CONNECT" {
+			f(":path", path)
+			f(":scheme", req.URL.Scheme)
+		}
+		if trailers != "" {
+			f("trailer", trailers)
+		}
+
+		var didUA bool
+		for k, vv := range req.Header {
+			if strings.EqualFold(k, "host") || strings.EqualFold(k, "content-length") {
+				// Host is :authority, already sent.
+				// Content-Length is automatic, set below.
 				continue
-			}
-			vv = vv[:1]
-			if vv[0] == "" {
+			} else if strings.EqualFold(k, "connection") || strings.EqualFold(k, "proxy-connection") ||
+				strings.EqualFold(k, "transfer-encoding") || strings.EqualFold(k, "upgrade") ||
+				strings.EqualFold(k, "keep-alive") {
+				// Per 8.1.2.2 Connection-Specific Header
+				// Fields, don't send connection-specific
+				// fields. We have already checked if any
+				// are error-worthy so just ignore the rest.
 				continue
+			} else if strings.EqualFold(k, "user-agent") {
+				// Match Go's http1 behavior: at most one
+				// User-Agent. If set to nil or empty string,
+				// then omit it. Otherwise if not mentioned,
+				// include the default (below).
+				didUA = true
+				if len(vv) < 1 {
+					continue
+				}
+				vv = vv[:1]
+				if vv[0] == "" {
+					continue
+				}
+
+			}
+
+			for _, v := range vv {
+				f(k, v)
 			}
 		}
-		for _, v := range vv {
-			cc.writeHeader(lowKey, v)
+		if http2shouldSendReqContentLength(req.Method, contentLength) {
+			f("content-length", strconv.FormatInt(contentLength, 10))
+		}
+		if addGzipHeader {
+			f("accept-encoding", "gzip")
+		}
+		if !didUA {
+			f("user-agent", http2defaultUserAgent)
 		}
 	}
-	if http2shouldSendReqContentLength(req.Method, contentLength) {
-		cc.writeHeader("content-length", strconv.FormatInt(contentLength, 10))
-	}
-	if addGzipHeader {
-		cc.writeHeader("accept-encoding", "gzip")
-	}
-	if !didUA {
-		cc.writeHeader("user-agent", http2defaultUserAgent)
+
+	// Do a first pass over the headers counting bytes to ensure
+	// we don't exceed cc.peerMaxHeaderListSize. This is done as a
+	// separate pass before encoding the headers to prevent
+	// modifying the hpack state.
+	hlSize := uint64(0)
+	enumerateHeaders(func(name, value string) {
+		hf := hpack.HeaderField{Name: name, Value: value}
+		hlSize += uint64(hf.Size())
+	})
+
+	if hlSize > cc.peerMaxHeaderListSize {
+		return nil, http2errRequestHeaderListSize
 	}
+
+	// Header list size is ok. Write the headers.
+	enumerateHeaders(func(name, value string) {
+		cc.writeHeader(strings.ToLower(name), value)
+	})
+
 	return cc.hbuf.Bytes(), nil
 }
 
@@ -7790,17 +7821,29 @@ func http2shouldSendReqContentLength(method string, contentLength int64) bool {
 }
 
 // requires cc.mu be held.
-func (cc *http2ClientConn) encodeTrailers(req *Request) []byte {
+func (cc *http2ClientConn) encodeTrailers(req *Request) ([]byte, error) {
 	cc.hbuf.Reset()
+
+	hlSize := uint64(0)
 	for k, vv := range req.Trailer {
-		// Transfer-Encoding, etc.. have already been filter at the
+		for _, v := range vv {
+			hf := hpack.HeaderField{Name: k, Value: v}
+			hlSize += uint64(hf.Size())
+		}
+	}
+	if hlSize > cc.peerMaxHeaderListSize {
+		return nil, http2errRequestHeaderListSize
+	}
+
+	for k, vv := range req.Trailer {
+		// Transfer-Encoding, etc.. have already been filtered at the
 		// start of RoundTrip
 		lowKey := strings.ToLower(k)
 		for _, v := range vv {
 			cc.writeHeader(lowKey, v)
 		}
 	}
-	return cc.hbuf.Bytes()
+	return cc.hbuf.Bytes(), nil
 }
 
 func (cc *http2ClientConn) writeHeader(name, value string) {
@@ -8012,7 +8055,17 @@ func (rl *http2clientConnReadLoop) run() error {
 
 func (rl *http2clientConnReadLoop) processHeaders(f *http2MetaHeadersFrame) error {
 	cc := rl.cc
-	cs := cc.streamByID(f.StreamID, f.StreamEnded())
+	if f.StreamEnded() {
+		// Issue 20521: If the stream has ended, streamByID() causes
+		// clientStream.done to be closed, which causes the request's bodyWriter
+		// to be closed with an errStreamClosed, which may be received by
+		// clientConn.RoundTrip before the result of processing these headers.
+		// Deferring stream closure allows the header processing to occur first.
+		// clientConn.RoundTrip may still receive the bodyWriter error first, but
+		// the fix for issue 16102 prioritises any response.
+		defer cc.streamByID(f.StreamID, true)
+	}
+	cs := cc.streamByID(f.StreamID, false)
 	if cs == nil {
 		// We'd get here if we canceled a request while the
 		// server had its response still in flight. So if this
@@ -8308,6 +8361,14 @@ func (rl *http2clientConnReadLoop) processData(f *http2DataFrame) error {
 		}
 		return nil
 	}
+	if !cs.firstByte {
+		cc.logf("protocol error: received DATA before a HEADERS frame")
+		rl.endStreamError(cs, http2StreamError{
+			StreamID: f.StreamID,
+			Code:     http2ErrCodeProtocol,
+		})
+		return nil
+	}
 	if f.Length > 0 {
 		// Check connection-level flow control.
 		cc.mu.Lock()
@@ -8422,6 +8483,8 @@ func (rl *http2clientConnReadLoop) processSettings(f *http2SettingsFrame) error
 			cc.maxFrameSize = s.Val
 		case http2SettingMaxConcurrentStreams:
 			cc.maxConcurrentStreams = s.Val
+		case http2SettingMaxHeaderListSize:
+			cc.peerMaxHeaderListSize = uint64(s.Val)
 		case http2SettingInitialWindowSize:
 			// Values above the maximum flow-control
 			// window size of 2^31-1 MUST be treated as a
@@ -8588,6 +8651,7 @@ func (cc *http2ClientConn) writeStreamReset(streamID uint32, code http2ErrCode,
 
 var (
 	http2errResponseHeaderListSize = errors.New("http2: response header list larger than advertised limit")
+	http2errRequestHeaderListSize  = errors.New("http2: request header list larger than peer's advertised limit")
 	http2errPseudoTrailers         = errors.New("http2: invalid pseudo header in trailers")
 )
 

src/net/http/serve_test.go

@@ -2765,15 +2765,28 @@ func testRequestLimit(t *testing.T, h2 bool) {
 		req.Header.Set(fmt.Sprintf("header%05d", i), fmt.Sprintf("val%05d", i))
 	}
 	res, err := cst.c.Do(req)
-	if err != nil {
+	if res != nil {
+		defer res.Body.Close()
+	}
+	if h2 {
+		// In HTTP/2, the result depends on a race. If the client has received the
+		// server's SETTINGS before RoundTrip starts sending the request, then RoundTrip
+		// will fail with an error. Otherwise, the client should receive a 431 from the
+		// server.
+		if err == nil && res.StatusCode != 431 {
+			t.Fatalf("expected 431 response status; got: %d %s", res.StatusCode, res.Status)
+		}
+	} else {
+		// In HTTP/1, we expect a 431 from the server.
 		// Some HTTP clients may fail on this undefined behavior (server replying and
 		// closing the connection while the request is still being written), but
 		// we do support it (at least currently), so we expect a response below.
-		t.Fatalf("Do: %v", err)
-	}
-	defer res.Body.Close()
-	if res.StatusCode != 431 {
-		t.Fatalf("expected 431 response status; got: %d %s", res.StatusCode, res.Status)
+		if err != nil {
+			t.Fatalf("Do: %v", err)
+		}
+		if res.StatusCode != 431 {
+			t.Fatalf("expected 431 response status; got: %d %s", res.StatusCode, res.Status)
+		}
 	}
 }