Navigation Menu

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

net/http: No error with http2 client on empty reply body and content-length != 0 #46071

Closed
MichaelEischer opened this issue May 9, 2021 · 10 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@MichaelEischer
Copy link

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

$ go version
go version go1.16.4 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/michael/Library/Caches/go-build"
GOENV="/Users/michael/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/michael/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/michael/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/michael/go1.16.4"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/michael/go1.16.4/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.16.4"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/michael/Projekte/restic/test/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/vs/zdx8nt1n4xlc2fgvtw589_100000gn/T/go-build1480369913=/tmp/go-build -gno-record-gcc-switches -fno-common"
uname -v: Darwin Kernel Version 19.6.0: Mon Apr 12 20:57:45 PDT 2021; root:xnu-6153.141.28.1~1/RELEASE_X86_64
ProductName:	Mac OS X
ProductVersion:	10.15.7
BuildVersion:	19H1030
lldb --version: lldb-1200.0.44.2
Apple Swift version 5.3.2 (swiftlang-1200.0.45 clang-1200.0.32.28)

What did you do?

Issue a GET request to a HTTP/2 server which then replies with the Content-Length header but afterwards fails to send data to the client for some reason. That is the reply has an empty body. See the Go playground link for a minimal example.

https://play.golang.org/p/jbhV10xhWKI

What did you expect to see?

Now, the client should receive an "Unexpected EOF" error when reading the reply:

2021/05/09 20:19:26 unexpected EOF

What did you see instead?

Header: map[Content-Length:[42] Date:[Sun, 09 May 2021 18:17:42 GMT]]
Body: {%!q(*bytes.Reader=&{[] 0 -1})}
Content-Length: 0
result ""

Note that res.ContentLength is set to zero despite the Content-Length header in the reply. This lets clients erroneously assume that they successfully received a reply to their request.

Executing the example on the Go playground currently deadlocks.

Go playground error

fatal error: all goroutines are asleep - deadlock!

goroutine 1 [select]:
net/http.(*Transport).getConn(0xc0000e83c0, 0xc0000bc2c0, 0x0, 0xc0000c80c0, 0x5, 0xc0000b6c50, 0xf, 0x0, 0x0, 0x0, ...)
	/usr/local/go-faketime/src/net/http/transport.go:1368 +0x589
net/http.(*Transport).roundTrip(0xc0000e83c0, 0xc00012a000, 0x30, 0x6f8940, 0x7fc92cc09500)
	/usr/local/go-faketime/src/net/http/transport.go:579 +0x7eb
net/http.(*Transport).RoundTrip(0xc0000e83c0, 0xc00012a000, 0xc0000e83c0, 0x0, 0x0)
	/usr/local/go-faketime/src/net/http/roundtrip.go:17 +0x35
net/http.send(0xc00012a000, 0x76c920, 0xc0000e83c0, 0x0, 0x0, 0x0, 0xc0000b8058, 0x417ed7, 0x1, 0x0)
	/usr/local/go-faketime/src/net/http/client.go:251 +0x454
net/http.(*Client).send(0xc0000a0d20, 0xc00012a000, 0x0, 0x0, 0x0, 0xc0000b8058, 0x0, 0x1, 0xc00012a000)
	/usr/local/go-faketime/src/net/http/client.go:175 +0xff
net/http.(*Client).do(0xc0000a0d20, 0xc00012a000, 0x0, 0x0, 0x0)
	/usr/local/go-faketime/src/net/http/client.go:717 +0x45f
net/http.(*Client).Do(...)
	/usr/local/go-faketime/src/net/http/client.go:585
net/http.(*Client).Get(0xc0000a0d20, 0xc0000c80c0, 0x17, 0x7fc953853888, 0x4, 0x406885)
	/usr/local/go-faketime/src/net/http/client.go:474 +0xbe
main.main()
	/tmp/sandbox703608843/prog.go:24 +0x136

goroutine 18 [IO wait]:
internal/poll.runtime_pollWait(0x7fc95385beb8, 0x72, 0x0)
	/usr/local/go-faketime/src/runtime/netpoll.go:222 +0x55
internal/poll.(*pollDesc).wait(0xc0000fa018, 0x72, 0x0, 0x0, 0x70e0ec)
	/usr/local/go-faketime/src/internal/poll/fd_poll_runtime.go:87 +0x45
internal/poll.(*pollDesc).waitRead(...)
	/usr/local/go-faketime/src/internal/poll/fd_poll_runtime.go:92
internal/poll.(*FD).Accept(0xc0000fa000, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0)
	/usr/local/go-faketime/src/internal/poll/fd_unix.go:401 +0x212
net.(*netFD).accept(0xc0000fa000, 0x50, 0x6e0440, 0xc000096df0)
	/usr/local/go-faketime/src/net/fd_unix.go:172 +0x45
net.(*TCPListener).accept(0xc0000ae0a8, 0x50, 0xc00018c000, 0x7fc953855c20)
	/usr/local/go-faketime/src/net/tcpsock_posix.go:139 +0x32
net.(*TCPListener).Accept(0xc0000ae0a8, 0x6e0440, 0xc000080000, 0x0, 0xc000096e20)
	/usr/local/go-faketime/src/net/tcpsock.go:261 +0x65
crypto/tls.(*listener).Accept(0xc0000ae5b8, 0x6ebd00, 0xc0001820c0, 0x6b9aa0, 0x8ec100)
	/usr/local/go-faketime/src/crypto/tls/tls.go:67 +0x37
net/http.(*Server).Serve(0xc000110000, 0x770ad0, 0xc0000ae5b8, 0x0, 0x0)
	/usr/local/go-faketime/src/net/http/server.go:2981 +0x285
net/http/httptest.(*Server).goServe.func1(0xc0000fe000)
	/usr/local/go-faketime/src/net/http/httptest/server.go:308 +0x6e
created by net/http/httptest.(*Server).goServe
	/usr/local/go-faketime/src/net/http/httptest/server.go:306 +0x5c

goroutine 19 [IO wait]:
internal/poll.runtime_pollWait(0x7fc95385bdd0, 0x77, 0x0)
	/usr/local/go-faketime/src/runtime/netpoll.go:222 +0x55
internal/poll.(*pollDesc).wait(0xc0000fa218, 0x77, 0xc0000b6000, 0x1, 0x0)
	/usr/local/go-faketime/src/internal/poll/fd_poll_runtime.go:87 +0x45
internal/poll.(*pollDesc).waitWrite(...)
	/usr/local/go-faketime/src/internal/poll/fd_poll_runtime.go:96
internal/poll.(*FD).WaitWrite(...)
	/usr/local/go-faketime/src/internal/poll/fd_unix.go:528
net.(*netFD).connect(0xc0000fa200, 0x771290, 0xc0000b6020, 0x0, 0x0, 0x76cca0, 0xc0000fc2e0, 0x0, 0x0, 0x0, ...)
	/usr/local/go-faketime/src/net/fd_unix.go:141 +0x27b
net.(*netFD).dial(0xc0000fa200, 0x771290, 0xc0000b6020, 0x772058, 0x0, 0x772058, 0xc0000a1380, 0x0, 0x7fc953853778, 0x10)
	/usr/local/go-faketime/src/net/sock_posix.go:149 +0x10b
net.socket(0x771290, 0xc0000b6020, 0x70d741, 0x3, 0x2, 0x1, 0x0, 0x0, 0x772058, 0x0, ...)
	/usr/local/go-faketime/src/net/sock_posix.go:70 +0x1c5
net.internetSocket(0x771290, 0xc0000b6020, 0x70d741, 0x3, 0x772058, 0x0, 0x772058, 0xc0000a1380, 0x1, 0x0, ...)
	/usr/local/go-faketime/src/net/ipsock_posix.go:141 +0x145
net.(*sysDialer).doDialTCP(0xc0000fa180, 0x771290, 0xc0000b6020, 0x0, 0xc0000a1380, 0xc000093601, 0x6e6b00, 0x1)
	/usr/local/go-faketime/src/net/tcpsock_posix.go:65 +0xc5
net.(*sysDialer).dialTCP(0xc0000fa180, 0x771290, 0xc0000b6020, 0x0, 0xc0000a1380, 0xc0000b6c50, 0xc000093768, 0x40d77b)
	/usr/local/go-faketime/src/net/tcpsock_posix.go:61 +0xd7
net.(*sysDialer).dialSingle(0xc0000fa180, 0x771290, 0xc0000b6020, 0x76e9f8, 0xc0000a1380, 0x0, 0x0, 0x0, 0x0)
	/usr/local/go-faketime/src/net/dial.go:580 +0x5e8
net.(*sysDialer).dialSerial(0xc0000fa180, 0x771290, 0xc0000b6020, 0xc00009eea0, 0x1, 0x1, 0x0, 0x0, 0x0, 0x0)
	/usr/local/go-faketime/src/net/dial.go:548 +0x15e
net.(*Dialer).DialContext(0x8f8fa0, 0x771290, 0xc0000b6020, 0x70d741, 0x3, 0xc0000b6c50, 0xf, 0x0, 0x0, 0x0, ...)
	/usr/local/go-faketime/src/net/dial.go:425 +0x6e5
net/http.(*Transport).dial(0xc0000e83c0, 0x771290, 0xc0000b6020, 0x70d741, 0x3, 0xc0000b6c50, 0xf, 0x0, 0xc000093ad0, 0x58d33d, ...)
	/usr/local/go-faketime/src/net/http/transport.go:1171 +0x16f
net/http.(*Transport).dialConn(0xc0000e83c0, 0x771290, 0xc0000b6020, 0x0, 0xc0000c80c0, 0x5, 0xc0000b6c50, 0xf, 0x0, 0xc0000ca7e0, ...)
	/usr/local/go-faketime/src/net/http/transport.go:1600 +0x1b85
net/http.(*Transport).dialConnFor(0xc0000e83c0, 0xc0000dc2c0)
	/usr/local/go-faketime/src/net/http/transport.go:1442 +0xc6
created by net/http.(*Transport).queueForDial
	/usr/local/go-faketime/src/net/http/transport.go:1411 +0x40f

Disabling the use of HTTP2 (comment out the ts.EnableHTTP2 = true line) or sending at least one byte (see w.Write([]byte("a"))) leads to the expected error.

@heschi heschi added the NeedsFix The path to resolution is known, but the work has not been done. label May 10, 2021
@heschi heschi modified the milestones: Go1.18, Go1.17 May 10, 2021
@heschi
Copy link
Contributor

heschi commented May 10, 2021

cc @bradfitz

@heschi heschi modified the milestones: Go1.17, Backlog May 10, 2021
@heschi
Copy link
Contributor

heschi commented May 10, 2021

cc @empijei @neild

@networkimprov
Copy link

cc @fraenkel

@fraenkel
Copy link
Contributor

https://datatracker.ietf.org/doc/html/rfc7540#section-8.1.2.6

A response that is defined to have no payload,
can have a non-zero content-length header field, even
   though no content is included in DATA frames.

@bradfitz
Copy link
Contributor

Looks like a bug. And I'm surprised, as I thought it handled this.

@fraenkel, that's about HEAD responses primarily, no? Doesn't apply to this case where it's just a GET?

@fraenkel
Copy link
Contributor

Unfortunately it's under malformed requests and responses, not tied to a method.

@MichaelEischer
Copy link
Author

@fraenkel The quote is missing the reference to RFC7230:

A response that is defined to have no payload, as described in [RFC7230],
Section 3.3.2, can have a non-zero content-length header field, even
though no content is included in DATA frames.

In RFC 7230 section 3.3 the last paragraph seems to describe all cases when a response has a message body, see https://datatracker.ietf.org/doc/html/rfc7230#section-3.3 . And a 200 OK reply to a GET request as in the playground example in the issue description, is allowed to have a payload. So I don't think that the response that is defined to have no payload exception applies here?

@fraenkel
Copy link
Contributor

Thanks for catching that. It's would seem it's not just method bit also status code.

@bradfitz
Copy link
Contributor

Yeah, when I said "HEAD primarily" I was being lazy and referring to:

// bodyAllowedForStatus reports whether a given response status code
// permits a body. See RFC 7230, section 3.3.
func bodyAllowedForStatus(status int) bool {
        switch {
        case status >= 100 && status <= 199:
                return false
        case status == 204:
                return false
        case status == 304:
                return false
        }
        return true
}

Which we use in various places in both net/http and http2 already, e.g.:

                if clen == "" && rws.handlerDone && bodyAllowedForStatus(rws.status) && (len(p) > 0 || !isHeadResp) {
                        clen = strconv.Itoa(len(p))
                }
        if w.req.Method != "HEAD" && w.contentLength != -1 && w.bodyAllowed() && w.contentLength != w.written {
                // Did not write enough. Avoid getting out of sync.                                                        
                return false
        }

I see net/http has checks for writing too much:

	if w.contentLength != -1 && w.written > w.contentLength {
                return 0, ErrContentLength
        }

But I don't see anything in net/http.(*response).finishReuest about writing too little.

@gopherbot
Copy link

Change https://golang.org/cl/354141 mentions this issue: http2: return unexpected eof on empty response with non-zero content length

AlexanderYastrebov added a commit to AlexanderYastrebov/net that referenced this issue Oct 7, 2021
AlexanderYastrebov added a commit to AlexanderYastrebov/net that referenced this issue Oct 7, 2021
dteh pushed a commit to dteh/fhttp that referenced this issue Jun 22, 2022
…length

Fixes golang/go#46071

Change-Id: I8b8262ba8e28a129f6aacfca43b7b8d9e605a0ce
GitHub-Last-Rev: 11b92cc8ba6e1d08716aac816d33b659563a893f
GitHub-Pull-Request: golang/net#114
Reviewed-on: https://go-review.googlesource.com/c/net/+/354141
Reviewed-by: Damien Neil <dneil@google.com>
Trust: Damien Neil <dneil@google.com>
Trust: Alexander Rakoczy <alex@golang.org>
Run-TryBot: Alexander Rakoczy <alex@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Oct 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
6 participants