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/33248Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>

http2/writesched.go

@@ -25,7 +25,9 @@ type WriteScheduler interface {
 	// 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

http2/writesched_priority.go

@@ -388,7 +388,15 @@ func (ws *priorityWriteScheduler) Push(wr FrameWriteRequest) {
 	} 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)

http2/writesched_priority_test.go

@@ -524,3 +524,18 @@ func TestPriorityWeights(t *testing.T) {
 		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)
+	}
+}