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: flow control lost if server handler calls req.Body.Close() #42983

Closed
jim-minter opened this issue Dec 3, 2020 · 3 comments
Closed

x/net/http2: flow control lost if server handler calls req.Body.Close() #42983

jim-minter opened this issue Dec 3, 2020 · 3 comments

Comments

@jim-minter
Copy link

@jim-minter jim-minter commented Dec 3, 2020

What version of Go are you using (go version)?

$ go version
go version go1.15.2 linux/amd64

Does this issue reproduce with the latest release?

Don't know; I expect it probably does.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/user/.cache/go-build"
GOENV="/home/user/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/user/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/user/go"
GOPRIVATE=""
GOPROXY="direct"
GOROOT="/usr/lib/golang"
GOSUMDB="off"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/golang/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build201633544=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I noticed that if an HTTP/2 server calls req.Body.Close() and then data are received, all the flow control associated with that data are not returned to the client. If this keeps happening, eventually the connection will hang because the client runs out of quota to send to the server.

This may just be a case of "don't do that, then". I think this issue is somewhat similar to #16481 .

package main

import (
	"crypto/rand"
	"crypto/rsa"
	"crypto/tls"
	"crypto/x509"
	"crypto/x509/pkix"
	"fmt"
	"io"
	"math/big"
	"net"
	"net/http"
	"os"
	"strings"
	"time"
)

func tlsListener() (net.Listener, error) {
	key, err := rsa.GenerateKey(rand.Reader, 1024)
	if err != nil {
		return nil, err
	}

	template := &x509.Certificate{
		SerialNumber:          big.NewInt(1),
		NotBefore:             time.Now(),
		NotAfter:              time.Now().AddDate(0, 0, 1),
		Subject:               pkix.Name{CommonName: "localhost"},
		BasicConstraintsValid: true,
		KeyUsage:              x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment,
		DNSNames:              []string{"localhost"},
		ExtKeyUsage:           []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
	}

	cert, err := x509.CreateCertificate(rand.Reader, template, template, &key.PublicKey, key)
	if err != nil {
		return nil, err
	}

	l, err := net.Listen("tcp", ":8443")
	if err != nil {
		return nil, err
	}

	return tls.NewListener(l, &tls.Config{
		Certificates: []tls.Certificate{
			{
				Certificate: [][]byte{
					cert,
				},
				PrivateKey: key,
			},
		},
		NextProtos: []string{"h2"},
	}), nil
}

func run() error {
	l, err := tlsListener()
	if err != nil {
		return err
	}

	s := &http.Server{
		Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
			if r.URL.Path == "/bad" {
				r.Body.Close()
			}
			if r.URL.Path == "/echo" {
				io.Copy(w, r.Body)
			}
		}),
	}

	go func() {
		err := s.Serve(l)
		if err != nil {
			panic(err)
		}
	}()

	c := &http.Client{
		Transport: &http.Transport{
			TLSClientConfig: &tls.Config{
				InsecureSkipVerify: true,
			},
			ForceAttemptHTTP2: true, // because we're setting TLSClientConfig
		},
	}

	for {
		c.Post("https://localhost:8443/bad", "", strings.NewReader(strings.Repeat("x", 4096)))

		resp, err := c.Post("https://localhost:8443/echo", "", strings.NewReader("hello\n"))
		if err != nil {
			return err
		}

		fmt.Println(resp.Proto)
		io.Copy(os.Stdout, resp.Body)
		resp.Body.Close()
	}
}

func main() {
	if err := run(); err != nil {
		panic(err)
	}
}

What did you expect to see?

Ideally, I'd expect the above program not to eventually hang. I believe the leak is in func (p *http2pipe) Write(d []byte) (n int, err error). Specifically in:

	if p.breakErr != nil {
		return len(d), nil // discard when there is no reader
	}

The flow control is never returned.

What did you see instead?

It eventually hangs.

@networkimprov
Copy link

@networkimprov networkimprov commented Dec 5, 2020

@fraenkel
Copy link
Contributor

@fraenkel fraenkel commented Dec 6, 2020

I ran this test on my machine and it never hung, so I don't know how to reproduce the issue.

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 8, 2021

Timed out in state WaitingForInfo. Closing.

(I am just a bot, though. Please speak up if this is a mistake or you have the requested information.)

@gopherbot gopherbot closed this Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants