8000 http2: limit maximum handler goroutines to MaxConcurrentStreams · golang/net@b225e7c · GitHub
[go: up one dir, main page]
Skip to content

Commit b225e7c

Browse files
neildgopherbot
neild
authored and
gopherbot
committed
http2: limit maximum handler goroutines to MaxConcurrentStreams
When the peer opens a new stream while we have MaxConcurrentStreams handler goroutines running, defer starting a handler until one of the existing handlers exits. Fixes golang/go#63417 Fixes CVE-2023-39325 Change-Id: If0531e177b125700f3e24c5ebd24b1023098fa6d Reviewed-on: https://team-review.git.corp.google.com/c/golang/go-private/+/2045854 TryBot-Result: Security TryBots <security-trybots@go-security-trybots.iam.gserviceaccount.com> Reviewed-by: Ian Cottrell <iancottrell@google.com> Reviewed-by: Tatiana Bradley <tatianabradley@google.com> Run-TryBot: Damien Neil <dneil@google.com> Reviewed-on: https://go-review.googlesource.com/c/net/+/534215 Reviewed-by: Michael Pratt <mpratt@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org> Reviewed-by: Damien Neil <dneil@google.com>
1 parent 88194ad commit b225e7c

File tree

2 files changed

+177
-2
lines changed

2 files changed

+177
-2
lines changed

‎http2/server.go

Lines changed: 64 additions & 2 deletions
< 10000 td data-grid-cell-id="diff-f930c2ef3e066fdc1a1fdd23284f6d7093374d662fcd5a29a23d5db515ef5191-2280-2320-2" data-line-anchor="diff-f930c2ef3e066fdc1a1fdd23284f6d7093374d662fcd5a29a23d5db515ef5191R2320" data-selected="false" role="gridcell" style="background-color:var(--diffBlob-additionLine-bgColor, var(--diffBlob-addition-bgColor-line));padding-right:24px" tabindex="-1" valign="top" class="focusable-grid-cell diff-text-cell right-side-diff-cell left-side">+
sc.curHandlers--
Original file line numberDiff line numberDiff line change
@@ -581,9 +581,11 @@ type serverConn struct {
581581
advMaxStreams uint32 // our SETTINGS_MAX_CONCURRENT_STREAMS advertised the client
582582
curClientStreams uint32 // number of open streams initiated by the client
583583
curPushedStreams uint32 // number of open streams initiated by server push
584+
curHandlers uint32 // number of running handler goroutines
584585
maxClientStreamID uint32 // max ever seen from client (odd), or 0 if there have been no client requests
585586
maxPushPromiseID uint32 // ID of the last push promise (even), or 0 if there have been no pushes
586587
streams map[uint32]*stream
588+
unstartedHandlers []unstartedHandler
587589
initialStreamSendWindowSize int32
588590
maxFrameSize int32
589591
peerMaxHeaderListSize uint32 // zero means unknown (default)
@@ -981,6 +983,8 @@ func (sc *serverConn) serve() {
981983
return
982984
case gracefulShutdownMsg:
983985
sc.startGracefulShutdownInternal()
986+
case handlerDoneMsg:
987+
sc.handlerDone()
984988
default:
985989
panic("unknown timer")
986990
}
@@ -1020,6 +1024,7 @@ var (
10201024
idleTimerMsg = new(serverMessage)
10211025
shutdownTimerMsg = new(serverMessage)
10221026
gracefulShutdownMsg = new(serverMessage)
1027+
handlerDoneMsg = new(serverMessage)
10231028
)
10241029

10251030
func (sc *serverConn) onSettingsTimer() { sc.sendServeMsg(settingsTimerMsg) }
@@ -2017,8 +2022,7 @@ func (sc *serverConn) processHeaders(f *MetaHeadersFrame) error {
20172022
st.readDeadline = time.AfterFunc(sc.hs.ReadTimeout, st.onReadTimeout)
20182023
}
20192024

2020-
go sc.runHandler(rw, req, handler)
2021-
return nil
2025+
return sc.scheduleHandler(id, rw, req, handler)
20222026
}
20232027

20242028
func (sc *serverConn) upgradeRequest(req *http.Request) {
@@ -2038,6 +2042,10 @@ func (sc *serverConn) upgradeRequest(req *http.Request) {
20382042
sc.conn.SetReadDeadline(time.Time{})
20392043
}
20402044

2045+
// This is the first request on the connection,
2046+
// so start the handler directly rather than going
2047+
// through scheduleHandler.
2048+
sc.curHandlers++
20412049
go sc.runHandler(rw, req, sc.handler.ServeHTTP)
20422050
}
20432051

@@ -2278,8 +2286,62 @@ func (sc *serverConn) newResponseWriter(st *stream, req *http.Request) *response
22782286
return &responseWriter{rws: rws}
22792287
}
22802288

2289+
type unstartedHandler struct {
2290+
streamID uint32
2291+
rw *responseWriter
2292+
req *http.Request
2293+
handler func(http.ResponseWriter, *http.Request)
2294+
}
2295+
2296+
// scheduleHandler starts a handler goroutine,
2297+
// or schedules one to start as soon as an existing handler finishes.
2298+
func (sc *serverConn) scheduleHandler(streamID uint32, rw *responseWriter, req *http.Request, handler func(http.ResponseWriter, *http.Request)) error {
2299+
sc.serveG.check()
2300+
maxHandlers := sc.advMaxStreams
2301+
if sc.curHandlers < maxHandlers {
2302+
sc.curHandlers++
2303+
go sc.runHandler(rw, req, handler)
2304+
return nil
2305+
}
2306+
if len(sc.unstartedHandlers) > int(4*sc.advMaxStreams) {
2307+
return sc.countError("too_many_early_resets", ConnectionError(ErrCodeEnhanceYourCalm))
2308+
}
2309+
sc.unstartedHandlers = append(sc.unstartedHandlers, unstartedHandler{
2310+
streamID: streamID,
2311+
rw: rw,
2312+
req: req,
2313+
handler: handler,
2314+
})
2315+
return nil
2316+
}
2317+
2318+
func (sc *serverConn) handlerDone() {
2319+
sc.serveG.check()
2320
2321+
i := 0
2322+
maxHandlers := sc.advMaxStreams
2323+
for ; i < len(sc.unstartedHandlers); i++ {
2324+
u := sc.unstartedHandlers[i]
2325+
if sc.streams[u.streamID] == nil {
2326+
// This stream was reset before its goroutine had a chance to start.
2327+
continue
2328+
}
2329+
if sc.curHandlers >= maxHandlers {
2330+
break
2331+
}
2332+
sc.curHandlers++
2333+
go sc.runHandler(u.rw, u.req, u.handler)
2334+
sc.unstartedHandlers[i] = unstartedHandler{} // don't retain references
2335+
}
2336+
sc.unstartedHandlers = sc.unstartedHandlers[i:]
2337+
if len(sc.unstartedHandlers) == 0 {
2338+
sc.unstartedHandlers = nil
2339+
}
2340+
}
2341+
22812342
// Run on its own goroutine.
22822343
func (sc *serverConn) runHandler(rw *responseWriter, req *http.Request, handler func(http.ResponseWriter, *http.Request)) {
2344+
defer sc.sendServeMsg(handlerDoneMsg)
22832345
didPanic := true
22842346
defer func() {
22852347
rw.rws.stream.cancelCtx()

‎http2/server_test.go

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4664,3 +4664,116 @@ func TestServerWriteDoesNotRetainBufferAfterServerClose(t *testing.T) {
46644664
st.ts.Config.Close()
46654665
<-donec
46664666
}
4667+
4668+
func TestServerMaxHandlerGoroutines(t *testing.T) {
4669+
const maxHandlers = 10
4670+
handlerc := make(chan chan bool)
4671+
donec := make(chan struct{})
4672+
defer close(donec)
4673+
st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) {
4674+
stopc := make(chan bool, 1)
4675+
select {
4676+
case handlerc <- stopc:
4677+
case <-donec:
4678+
}
4679+
select {
4680+
case shouldPanic := <-stopc:
4681+
if shouldPanic {
4682+
panic(http.ErrAbortHandler)
4683+
}
4684+
case <-donec:
4685+
}
4686+
}, func(s *Server) {
4687+
s.MaxConcurrentStreams = maxHandlers
4688+
})
4689+
defer st.Close()
4690+
4691+
st.writePreface()
4692+
st.writeInitialSettings()
4693+
st.writeSettingsAck()
4694+
4695+
// Make maxHandlers concurrent requests.
4696+
// Reset them all, but only after the handler goroutines have started.
4697+
var stops []chan bool
4698+
streamID := uint32(1)
4699+
for i := 0; i < maxHandlers; i++ {
4700+
st.writeHeaders(HeadersFrameParam{
4701+
StreamID: streamID,
4702+
BlockFragment: st.encodeHeader(),
4703+
EndStream: true,
4704+
EndHeaders: true,
4705+
})
4706+
stops = append(stops, <-handlerc)
4707+
st.fr.WriteRSTStream(streamID, ErrCodeCancel)
4708+
streamID += 2
4709+
}
4710+
4711+
// Start another request, and immediately reset it.
4712+
st.writeHeaders(HeadersFrameParam{
4713+
StreamID: streamID,
4714+
BlockFragment: st.encodeHeader(),
4715+
EndStream: true,
4716+
EndHeaders: true,
4717+
})
4718+
st.fr.WriteRSTStream(streamID, ErrCodeCancel)
4719+
streamID += 2
4720+
4721+
// Start another two requests. Don't reset these.
4722+
for i := 0; i < 2; i++ {
4723+
st.writeHeaders(HeadersFrameParam{
4724+
StreamID: streamID,
4725+
BlockFragment: st.encodeHeader(),
4726+
EndStream: true,
4727+
EndHeaders: true,
4728+
})
4729+
streamID += 2
4730+
}
4731+
4732+
// The initial maxHandlers handlers are still executing,
4733+
// so the last two requests don't start any new handlers.
4734+
select {
4735+
case <-handlerc:
4736+
t.Errorf("handler unexpectedly started while maxHandlers are already running")
4737+
case <-time.After(1 * time.Millisecond):
4738+
}
4739+
4740+
// Tell two handlers to exit.
4741+
// The pending requests which weren't reset start handlers.
4742+
stops[0] <- false // normal exit
4743+
stops[1] <- true // panic
4744+
stops = stops[2:]
4745+
stops = append(stops, <-handlerc)
4746+
stops = append(stops, <-handlerc)
4747+
4748+
// Make a bunch more requests.
4749+
// Eventually, the server tells us to go away.
4750+
for i := 0; i < 5*maxHandlers; i++ {
4751+
st.writeHeaders(HeadersFrameParam{
4752+
StreamID: streamID,
4753+
BlockFragment: st.encodeHeader(),
4754+
EndStream: true,
4755+
EndHeaders: true,
4756+
})
4757+
st.fr.WriteRSTStream(streamID, ErrCodeCancel)
4758+
streamID += 2
4759+
}
4760+
Frames:
4761+
for {
4762+
f, err := st.readFrame()
4763+
if err != nil {
4764+
st.t.Fatal(err)
4765+
}
4766+
switch f := f.(type) {
4767+
case *GoAwayFrame:
4768+
if f.ErrCode != ErrCodeEnhanceYourCalm {
4769+
t.Errorf("err code = %v; want %v", f.ErrCode, ErrCodeEnhanceYourCalm)
4770+
}
4771+
break Frames
4772+
default:
4773+
}
4774+
}
4775+
4776+
for _, s := range stops {
4777+
close(s)
4778+
}
4779+
}

0 commit comments

Comments
 (0)
0