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: Priorities graph may panic #66515

Open
elindsey opened this issue Mar 25, 2024 · 1 comment
Open

x/net/http2: Priorities graph may panic #66515

elindsey opened this issue Mar 25, 2024 · 1 comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@elindsey
Copy link

elindsey commented Mar 25, 2024

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

@elindsey elindsey changed the title x/net/http2: Priorities race condition may panic x/net/http2: Priorities graph may panic Mar 25, 2024
@thanm thanm added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 26, 2024
@thanm
Copy link
Contributor

thanm commented Mar 26, 2024

Thanks for the report.

@neild per owners.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

2 participants