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: client sometimes sends extra zero-len DATA frame after request DATA #32254

Closed
the729 opened this issue May 26, 2019 · 13 comments

Comments

@the729
Copy link

commented May 26, 2019

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

$ go version
go version go1.10.1 linux/amd64

Does this issue reproduce with the latest release?

Yes. I have tried with the latest master branch of golang.org/x/net/http2 (commit f3200d17e092c607f615320ecaad13d87ad9a2b3).

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/the729/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/the729/go"
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
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-build647013263=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Code: https://play.golang.org/p/SOgAnCu0ybi

Run with GODBUG=http2debug=2

What did you expect to see?

Client should write a HEADERS frame and a single DATA frame. The payload "hello, world!" should be in the DATA frame, with END_STREAM set.

What did you see instead?

2019/05/26 17:26:12 http2: Framer 0xc42039f960: wrote HEADERS flags=END_HEADERS stream=1 len=51
2019/05/26 17:26:12 http2: Framer 0xc42039f960: wrote DATA stream=1 len=13 data="hello, world!"
2019/05/26 17:26:12 http2: Framer 0xc42039f960: wrote DATA flags=END_STREAM stream=1 len=0 data=""

There are 2 DATA frames. First one with the actual payload, second one with END_STREAM flag.

Strictly speaking, this does not violate the specs. However, there are several reasons that we should save the extra frame:

  • less actual bytes transmitted, faster roundtrip
  • some non-standard http2 server half-close stream after receiving entire body indicated by Content-Length header, thus the extra frame will trigger a RST_STREAM with ErrCode=STREAM_CLOSED
@bradfitz

This comment has been minimized.

Copy link
Member

commented May 27, 2019

It's doing whatever the underlying io.Reader does.

If the underlying reader returns (non-zero, io.EOF), then it sends one 1 DATA frame.

(We tried to change the strings.Reader and bytes.Reader implementations several times in the past to do eager EOF returns but it broke too many tests to be worth it.)

But I suppose we could know better in the case where the request body's content-length was predeclared (as it is when you use http.NewRequest on a few known types like strings.Reader). Once we've read enough bytes then we could assume that no bytes will remain on that reader, but that's also where we currently double-check users' Reader implementations and abort the http2 stream if their declared request content-length doesn't match reality. But I suppose we could trust standard library types and only sanity check unknown types.

But that's a lot of special casing for minimal benefit.

@bradfitz bradfitz added this to the Unplanned milestone May 27, 2019
@bradfitz bradfitz changed the title x/net/http2: client sends extra zero-len DATA frame after sending actual payloads x/net/http2: client sometimes sends extra zero-len DATA frame after request DATA May 27, 2019
@the729

This comment has been minimized.

Copy link
Author

commented May 27, 2019

The spec (https://http2.github.io/http2-spec/#malformed) says:

A request or response is also malformed if the value of a content-length header field does not equal the sum of the DATA frame payload lengths that form the body.
Malformed requests or responses that are detected MUST be treated as a stream error (Section 5.4.2) of type PROTOCOL_ERROR.

So we can do the following:

  • If request does not have content-length header, do not change the behavior.
  • Read from io.Reader until we get enough bytes according to content-length, then try to read one more byte.
  • If we get EOF without the byte, we are good.
  • If we get the byte with or without EOF, report PROTOCOL_ERROR.

Another way is that we can wrap io.Reader into a new io.Reader with content-length constraint. This can be done as a separate package outside x/net, which will have minimal side-effects.

@bradfitz Which way looks good to you? I can do the implementation.

@the729

This comment has been minimized.

Copy link
Author

commented May 27, 2019

I see that io.LimitReader will do the trick.

@the729 the729 closed this May 27, 2019
@the729

This comment has been minimized.

Copy link
Author

commented Jun 5, 2019

I reopened the issue because io.LimitedReader does not do the trick, as it also returns EOF lazily.
I propose implementing a modified version of LimitedReader in http2/transport.go so that it returns eager EOF, as long as content-length is specified.

@the729 the729 reopened this Jun 5, 2019
@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

But then how/when do we report an error if the user's Request.Body Reader returns more bytes than the declared ContentLength?

@the729

This comment has been minimized.

Copy link
Author

commented Jun 5, 2019

@bradfitz A simplified solution would be to drop the extra bytes silently without producing any actual error except a debug log.

A complete solution would be the following:

  1. Implement a limitedReader with eager EOF that can also produce a special ErrUnexpectedData when it meets extra bytes.
  2. In ClientConn.roundTrip(): wrap body with limitedReader, if contentLen>=0
  3. In clientStream.writeRequestBody(): after body.Read, if err==ErrUnexpectedData, write a stream reset frame with PROTOCOL_ERROR.

Does these sounds OK?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

That doesn't sound okay to me. I'm not even sure how (1) would work, and even if it did, it'd be silently sending truncated data to the peer, rather than aborting the stream which is meant to prevent them fro working with corrupted data.

@the729

This comment has been minimized.

Copy link
Author

commented Jun 5, 2019

Fair enough.
Then we can go for (2). What's your concern? Is it the idea of addressing some fundamental issues (i.e. lazy EOF) in an upper level less elegant? 😊

@gopherbot

This comment has been minimized.

Copy link

commented Jun 7, 2019

Change https://golang.org/cl/181157 mentions this issue: http2: end stream eagerly after sending the request body

@the729

This comment has been minimized.

Copy link
Author

commented Jun 7, 2019

@bradfitz Hi Brad, I thought it may be easier to just talk about the code, so I filed https://golang.org/cl/181157

What I did is also simpler than discussed before. It fixes the issue, while still allows sending any body without interruption or truncation, even if the actual length does not match specified content-length. This is exactly the behavior before.

I thought it is the receiving peer's responsibility to handle the malformed request body rather than the sending peer.

the729 added a commit to HuanTeng/golang-x-net that referenced this issue Jun 7, 2019
Check EOF eagerly on the request body when its content-length
is specified and it is expected to end. Thus, the data frame
containing the last chunk of data of the body will be marked with
END_STREAM eagerly.

However, if the specified content-length is incorrect, the whole
request body will still be sent, regardless of its actual length.
This will render the request malformed according to the spec.
We believe it is the receiving peer's responsibility to handle
the error.

Fix golang/go#32254

Change-Id: Id24c043c7cc3a41421dfd099a139f1b1e08056b9
@the729

This comment has been minimized.

Copy link
Author

commented Jul 12, 2019

@dmitshur Since Brad is on leave, can you please review https://golang.org/cl/181157 ? Thanks!

@the729

This comment has been minimized.

Copy link
Author

commented Sep 11, 2019

ping

@gopherbot

This comment has been minimized.

Copy link

commented Oct 9, 2019

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

gopherbot pushed a commit that referenced this issue Oct 9, 2019
Updates x/net/http2 to git rev d66e71096ffb9f08f36d9aefcae80ce319de6d68

    http2: end stream eagerly after sending the request body
    https://golang.org/cl/181157 (fixes #32254)

    all: fix typos
    https://golang.org/cl/193799

    http2: fix memory leak in random write scheduler
    https://golang.org/cl/198462 (fixes #33812)

    http2: do not sniff body if Content-Encoding is set
    https://golang.org/cl/199841 (updates #31753)

Also unskips tests from CL 199799.

Change-Id: I241c0b1cd18cad5041485be92809137a973e33bd
Reviewed-on: https://go-review.googlesource.com/c/go/+/200102
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.