http2: don't crash when writing RST_STREAM on idle or closed streams
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>
diff --git a/http2/writesched.go b/http2/writesched.go
index fb5da35..caa77c7 100644
--- a/http2/writesched.go
+++ b/http2/writesched.go
@@ -25,7 +25,9 @@
// https://tools.ietf.org/html/rfc7540#section-5.1
AdjustStream(streamID uint32, priority PriorityParam)
- // Push queues a frame in the scheduler.
+ // Push queues a frame in the scheduler. In most cases, this will not be
+ // called with wr.StreamID()!=0 unless that stream is currently open. The one
+ // exception is RST_STREAM frames, which may be sent on idle or closed streams.
Push(wr FrameWriteRequest)
// Pop dequeues the next frame to write. Returns false if no frames can
diff --git a/http2/writesched_priority.go b/http2/writesched_priority.go
index 40108b0..0113272 100644
--- a/http2/writesched_priority.go
+++ b/http2/writesched_priority.go
@@ -388,7 +388,15 @@
} else {
n = ws.nodes[id]
if n == nil {
- panic("add on non-open stream")
+ // id is an idle or closed stream. wr should not be a HEADERS or
+ // DATA frame. However, wr can be a RST_STREAM. In this case, we
+ // push wr onto the root, rather than creating a new priorityNode,
+ // since RST_STREAM is tiny and the stream's priority is unknown
+ // anyway. See issue #17919.
+ if wr.DataSize() > 0 {
+ panic("add DATA on non-open stream")
+ }
+ n = &ws.root
}
}
n.q.push(wr)
diff --git a/http2/writesched_priority_test.go b/http2/writesched_priority_test.go
index 174b447..2b23204 100644
--- a/http2/writesched_priority_test.go
+++ b/http2/writesched_priority_test.go
@@ -524,3 +524,18 @@
t.Error(err)
}
}
+
+func TestPriorityRstStreamOnNonOpenStreams(t *testing.T) {
+ ws := NewPriorityWriteScheduler(&PriorityWriteSchedulerConfig{
+ MaxClosedNodesInTree: 0,
+ MaxIdleNodesInTree: 0,
+ })
+ ws.OpenStream(1, OpenStreamOptions{})
+ ws.CloseStream(1)
+ ws.Push(FrameWriteRequest{write: streamError(1, ErrCodeProtocol)})
+ ws.Push(FrameWriteRequest{write: streamError(2, ErrCodeProtocol)})
+
+ if err := checkPopAll(ws, []uint32{1, 2}); err != nil {
+ t.Error(err)
+ }
+}
