Skip to content

Commit

Permalink
CRI stream server: Fix goroutine leak in Exec
Browse files Browse the repository at this point in the history
In the CRI streaming server, a goroutine (`handleResizeEvents`) is launched
to handle terminal resize events if a TTY is asked for with an exec; this
is the sender of terminal resize events. Another goroutine is launched
shortly after successful process startup to actually do something with
these events, however the issue arises if the exec process fails to start
for any reason that would have `process.Start` return non-nil. The receiver
goroutine never gets launched so the sender is stuck blocked on a channel send
infinitely.

This could be used in a malicious manner by repeatedly launching execs
with a command that doesn't exist in the image, as a single goroutine
will get leaked on every invocation which will slowly grow containerd's
memory usage.

Signed-off-by: Danny Canter <danny@dcantah.dev>
(cherry picked from commit f012617)
  • Loading branch information
dcantah authored and dmcgowan committed Dec 7, 2022
1 parent 2f59a97 commit 6cd1152
Showing 1 changed file with 12 additions and 3 deletions.
15 changes: 12 additions & 3 deletions pkg/cri/streaming/remotecommand/httpstream.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ limitations under the License.
package remotecommand

import (
gocontext "context"
"encoding/json"
"errors"
"fmt"
Expand Down Expand Up @@ -132,7 +133,7 @@ func createStreams(req *http.Request, w http.ResponseWriter, opts *Options, supp

if ctx.resizeStream != nil {
ctx.resizeChan = make(chan remotecommand.TerminalSize)
go handleResizeEvents(ctx.resizeStream, ctx.resizeChan)
go handleResizeEvents(req.Context(), ctx.resizeStream, ctx.resizeChan)
}

return ctx, true
Expand Down Expand Up @@ -425,7 +426,7 @@ WaitForStreams:
// supportsTerminalResizing returns false because v1ProtocolHandler doesn't support it.
func (*v1ProtocolHandler) supportsTerminalResizing() bool { return false }

func handleResizeEvents(stream io.Reader, channel chan<- remotecommand.TerminalSize) {
func handleResizeEvents(ctx gocontext.Context, stream io.Reader, channel chan<- remotecommand.TerminalSize) {
defer runtime.HandleCrash()
defer close(channel)

Expand All @@ -435,7 +436,15 @@ func handleResizeEvents(stream io.Reader, channel chan<- remotecommand.TerminalS
if err := decoder.Decode(&size); err != nil {
break
}
channel <- size

select {
case channel <- size:
case <-ctx.Done():
// To avoid leaking this routine, exit if the http request finishes. This path
// would generally be hit if starting the process fails and nothing is started to
// ingest these resize events.
return
}
}
}

Expand Down

0 comments on commit 6cd1152

Please sign in to comment.