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: serverConn.processHeaders has two error paths that don't clean up #48675

Open
bradfitz opened this issue Sep 29, 2021 · 2 comments
Open

Comments

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Sep 29, 2021

I continue to hunt a stream accounting bug in either the Go HTTP/2 transport and/or server. Reading code again now, I noticed:

(*serverConn).processHeaders does:

st := sc.newStream(id, 0, initialState)

... where that newStream call is what's responsible for incrementing the stream count:

sc.curClientStreams++

(that's what's ultimately causing the return sc.countError("over_max_streams", streamError(id, ErrCodeProtocol)) I'm hitting)

But just after that newStream call are two error exit paths:

st := sc.newStream(id, 0, initialState)

if f.HasPriority() {
        if err := sc.checkPriority(f.StreamID, f.Priority); err != nil {
                return err
        }
        sc.writeSched.AdjustStream(st.id, f.Priority)
}

rw, req, err := sc.newWriterAndRequest(st, f)
if err != nil {
        return err
}
// ... [no returns]
go sc.runHandler(rw, req, handler)
return nil

Those two return err errors are suspect. If those happen, then curClientStreams can be left incremented, never to be decremented again.

The decrement happens in (*serverConn).closeStream, which is called from:

  • processResetStream, reading a RSTStream frame from the client (e.g. they canceled the request)
  • wroteFrame, called after a frame write is successful, including handler panics. But if we return err above before starting the handler, we can't ever write anything, as the handler never runs.

I'm not sure whether this is what we're hitting yet, but it looks suspicious.

/cc @neild

@gopherbot
Copy link

@gopherbot gopherbot commented Sep 29, 2021

Change https://golang.org/cl/353031 mentions this issue: http2: add error counter to one missing processHeaders error path

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Sep 29, 2021

I still think this is a problem, but I don't think it's the over_max_streams problem I'm hitting. I checked all the exit paths and they all have error counter metrics on them, except for one (https://golang.org/cl/353031), which I doubt we're hitting, as CONNECT requests should be unable to make it to our backend.

gopherbot pushed a commit to golang/net that referenced this issue Sep 29, 2021
And unrelated typo fix.

Updates golang/go#48675

Change-Id: Id90e0e76e514ba550fa2d3c9e1104ebfd4c23a9b
Reviewed-on: https://go-review.googlesource.com/c/net/+/353031
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: David Crawshaw <crawshaw@golang.org>
Trust: Josh Bleecher Snyder <josharian@gmail.com>
@mknyszek mknyszek added this to the Unreleased milestone Oct 4, 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
3 participants