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: RSTStream stream error write scheduling might panic with priority scheduler #17919

Closed
bradfitz opened this issue Nov 15, 2016 · 1 comment
Closed

x/net/http2: RSTStream stream error write scheduling might panic with priority scheduler #17919

bradfitz opened this issue Nov 15, 2016 · 1 comment
Labels
FrozenDueToAge
Milestone
Unreleased

Comments

@bradfitz
Copy link
Contributor

Follow-up to https://go-review.googlesource.com/33238 for #17243

With the priority write scheduler, it's possible that the Server could generate a RSTStream write that would then crash the write scheduler I think. Perhaps if the user sends a bogus frame to an un-opened stream or something and then we reply with RST_STREAM?

Unsure.

Doesn't affect Go 1.8, which will ship with the random write scheduler still.
@bradfitz bradfitz added this to the Unreleased milestone Nov 15, 2016
@gopherbot
Copy link

CL https://golang.org/cl/33248 mentions this issue.

@golang golang locked and limited conversation to collaborators Nov 15, 2017
c3mb0 pushed a commit to c3mb0/net that referenced this issue Apr 2, 2018
@tombergan @bradfitz
http2: don't crash when writing RST_STREAM on idle or closed streams
f4a3dfc 
https://go-review.googlesource.com/33238 fixed scheduling of RST_STREAMs so
they are added to the appropriate stream queue. However, RST_STREAM may be
sent before OpenStream or after CloseStream in certain error cases, such as:

https://github.com/golang/net/blob/00ed5e97ea3a5ac46658b98e50259941947cec04/http2/server.go#L1395
https://github.com/golang/net/blob/00ed5e97ea3a5ac46658b98e50259941947cec04/http2/frame.go#L866
https://github.com/golang/net/blob/00ed5e97ea3a5ac46658b98e50259941947cec04/http2/frame.go#L947

In these cases, the failing stream has no queue and in priorityWriteScheduler.Push
will panic. A simple fix is to add RST_STREAMs to the root queue when the stream
queue doesn't exist. This is correct because:

- RST_STREAM is tiny and doesn't use flow control bytes, so prioritization is
  not important.

- The server should not send any frames on a stream after sending RST_STREAM,
  but even if that happens, the RST_STREAM will be ordered before those other
  frames because the control queue has the highest priority.

Fixes golang/go#17919

Change-Id: I2e8101f015822ef975303a1fe87634b36afbdc6b
Reviewed-on: https://go-review.googlesource.com/33248
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
3 participants
@bradfitz @gopherbot @tombergan