x/net/http2: Priorities graph may panic

[elindsey]

Go version

go1.22.1

Output of go env in your module/workspace:

$ go env
GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/elilindsey/Library/Caches/go-build'
GOENV='/Users/elilindsey/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/elilindsey/code/go/pkg/mod'
GONOPROXY='stash.corp.netflix.com'
GONOSUMDB='stash.corp.netflix.com'
GOOS='darwin'
GOPATH='/Users/elilindsey/code/go'
GOPRIVATE='stash.corp.netflix.com'
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/opt/homebrew/Cellar/go/1.22.1/libexec'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/opt/homebrew/Cellar/go/1.22.1/libexec/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.22.1'
GCCGO='gccgo'
AR='ar'
CC='cc'
CXX='c++'
CGO_ENABLED='1'
GOMOD='/Users/elilindsey/code/scratchgo/bindl4/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/9v/g2961bl15zv1bx2qk1sclrtr0000gn/T/go-build3463040716=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

H2 priorities code may panic if a stream is deleted after it's established but before it sends data. This ordering is valid because the RFC allows sending PRIORITY for streams in any state.

Failing test case:

package http2

import (
    "testing"
)

func TestPrioritiesIdleStreamPanic(t *testing.T) {
    ws := NewPriorityWriteScheduler(nil)
    // Preseed 10 entries
    for id := 1; id < 20; id += 2 {
        ws.AdjustStream(uint32(id), PriorityParam{
            StreamDep: uint32(id - 2),
            Exclusive: false,
            Weight:    16,
        })
    }
    // Convert id 1 to a "real" stream
    ws.OpenStream(uint32(1), OpenStreamOptions{})
    // Add a new idle stream
    ws.AdjustStream(uint32(21), PriorityParam{
        StreamDep: uint32(19),
        Exclusive: false,
        Weight:    16,
    })
    // Now, push data
    ws.Push(FrameWriteRequest{
        stream: &stream{
            id: uint32(1),
        },
        write: &writeData{
            streamID:  uint32(1),
            p:         []byte{0x1, 0x1, 0x1},
            endStream: false,
        },
        done: nil,
    })
}

A potential patch:

diff --git a/vendor/golang.org/x/net/http2/writesched_priority.go b/vendor/golang.org/x/net/http2/writesched_priority.go
index 0a242c6..671902e 100644
--- a/vendor/golang.org/x/net/http2/writesched_priority.go
+++ b/vendor/golang.org/x/net/http2/writesched_priority.go
@@ -254,13 +254,28 @@ type priorityWriteScheduler struct {
        queuePool writeQueuePool
 }

+func (ws *priorityWriteScheduler) convertIdleToOpenNode(n *priorityNode) {
+       if n.state != priorityNodeIdle {
+               panic(fmt.Sprintf("stream %d already opened", n.id))
+       }
+       n.state = priorityNodeOpen
+       for i := range ws.idleNodes {
+               if ws.idleNodes[i] == n {
+                       x := ws.idleNodes[:i]
+                       y := ws.idleNodes[i+1:]
+                       copy(ws.idleNodes, x)
+                       ws.idleNodes = append(ws.idleNodes, y...)
+                       ws.idleNodes = ws.idleNodes[:len(x)+len(y)]
+                       return
+               }
+       }
+       panic("idle node not found in idle list")
+}
+
 func (ws *priorityWriteScheduler) OpenStream(streamID uint32, options OpenStreamOptions) {
        // The stream may be currently idle but cannot be opened or closed.
        if curr := ws.nodes[streamID]; curr != nil {
-               if curr.state != priorityNodeIdle {
-                       panic(fmt.Sprintf("stream %d already opened", streamID))
-               }
-               curr.state = priorityNodeOpen
+               ws.convertIdleToOpenNode(curr)
                return
        }

What did you see happen?

Panic

What did you expect to see?

Not panic

[thanm]

Thanks for the report.

@neild per owners.