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
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.

Comments

@jim-minter
Copy link

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

cc @fraenkel

@fraenkel
Copy link
Contributor

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.

@cagedmantis cagedmantis added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Dec 8, 2020
@gopherbot
Copy link

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.)

@golang golang locked and limited conversation to collaborators Jan 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

5 participants