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

Jump to bottom

x/net/http2: stream reset on invalid content-length header drops conn flow control #54185

Closed
jronak opened this issue Aug 1, 2022 · 3 comments
Closed

x/net/http2: stream reset on invalid content-length header drops conn flow control #54185

jronak opened this issue Aug 1, 2022 · 3 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@jronak
Copy link

jronak commented Aug 1, 2022 •
edited

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

$ go version
go version go1.18.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/ronakj/Library/Caches/go-build"
GOENV="/Users/ronakj/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/ronakj/gocode/pkg/mod"
GONOPROXY="none"
GONOSUMDB="*"
GOOS="darwin"
GOPATH="/Users/ronakj/gocode"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.18.3"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/ronakj/project/http2-issue-repro/go.mod"
GOWORK=""
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/4d/2jw_2tc15x339gr53x6k64hm0000gn/T/go-build1453600270=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I'm using Go HTTP2 server for proxying gRPC requests, where a client could potentially forward a request with a smaller content-length header.

What did you expect to see?

Stream with an invalid content-length header must result in an RST_STREAM. This must not affect other valid streams and connection level flow control.

What did you see instead?

The client slowly notices degradation in performance and eventually, the client is hung with all requests failing with deadline exceeded errors.

This issue happens as the client flow control eventually runs out of available outward flow control bytes as the HTTP2 server does not return WINDOW_UPDATE when the stream resets due to an invalid content-length header.

Issue repro
@jronak jronak mentioned this issue Aug 1, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/420615 mentions this issue: http2: fix conn flow control when stream closes on bad content-length

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 3, 2022
@dmitshur dmitshur added this to the Backlog milestone Aug 3, 2022
@dmitshur
Copy link
Contributor

dmitshur commented Aug 3, 2022

CC @neild, @tombergan.

@jronak jronak mentioned this issue Aug 7, 2022
Closed
@gopherbot
Copy link

Change https://go.dev/cl/421974 mentions this issue: http2: fix conn flow control when stream closes on bad content-length

WeiminShang added a commit to WeiminShang/net that referenced this issue Nov 16, 2022
@WeiminShang @neild
http2: fix conn flow control when stream closes on bad content-length
2b1bcd8 
HTTP2 server does not send WINDOW_UPDATE when the client
sends more data than declared in the content-length header.

Client flow control can eventually run out of available bytes which
hangs the client connection as it cannot write DATA frames for any
stream any longer.

Fixes: golang/go#54185

Change-Id: I48ae3212fb31ce302715abe129adf5c9625faf12
GitHub-Last-Rev: 1351d3b416b6ecbfc396b7a7d43dba5115b4aaf7
GitHub-Pull-Request: golang/net#143
Reviewed-on: https://go-review.googlesource.com/c/net/+/421974
Reviewed-by: Than McIntosh <thanm@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Aug 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
3 participants
@dmitshur @jronak @gopherbot