ssh: Fix racy cleanup of read monitor's pipe

The read monitor is implemented via a Unix pipe, which means that its
file descriptors need to always be closed by us or otherwise we have the
potential for a resource leak. This is done by spawning a Goroutine that
waits for the context to be cancelled. Unfortunately, when the context
is cancelled when spawning a command that is about to use these file
descriptors then we now run into a race: `exec.Cmd.Start()` may just be
about to duplicate those file descriptors to pass them to the child
process while we're in the process of closing them.

Fix this race by cleaning them up inside of the command's finalizer
instead. Like this we ensure that the descriptors are only closed after
we have already reaped the child process.

Changelog: fixed

3 files changed, 22 insertions, 20 deletions

diff --git a/internal/git/pktline/read_monitor.go b/internal/git/pktline/read_monitor.go
index e288b2530..9b371a85f 100644
--- a/internal/git/pktline/read_monitor.go
+++ b/internal/git/pktline/read_monitor.go
@@ -40,27 +40,22 @@ type ReadMonitor struct {
 // to the pipe. The stream will be monitored for a pktline-formatted packet
 // matching pkt. If it isn't seen within the timeout, cancelFn will be called.
 //
-// Resources will be freed when the context is done, but you should close the
-// returned *os.File earlier if possible.
-func NewReadMonitor(ctx context.Context, r io.Reader) (*os.File, *ReadMonitor, error) {
+// The returned function will release allocated resources. You must make sure to call this
+// function.
+func NewReadMonitor(ctx context.Context, r io.Reader) (*os.File, *ReadMonitor, func(), error) {
 	pr, pw, err := os.Pipe()
 	if err != nil {
-		return nil, nil, err
+		return nil, nil, nil, err
 	}
 
-	// Ensure all resources are closed once the context is done
-	go func() {
-		<-ctx.Done()
-
-		pr.Close()
-		pw.Close()
-	}()
-
 	return pr, &ReadMonitor{
-		pr:         pr,
-		pw:         pw,
-		underlying: r,
-	}, nil
+			pr:         pr,
+			pw:         pw,
+			underlying: r,
+		}, func() {
+			pr.Close()
+			pw.Close()
+		}, nil
 }
 
 // Monitor should be called at most once. It scans the stream, looking for the
diff --git a/internal/git/pktline/read_monitor_test.go b/internal/git/pktline/read_monitor_test.go
index e78bdd8c5..65a0ec3ab 100644
--- a/internal/git/pktline/read_monitor_test.go
+++ b/internal/git/pktline/read_monitor_test.go
@@ -24,7 +24,7 @@ func TestReadMonitorTimeout(t *testing.T) {
 		waitPipeR, // this pipe reader lets us block the multi reader
 	)
 
-	r, monitor, err := NewReadMonitor(ctx, in)
+	r, monitor, cleanup, err := NewReadMonitor(ctx, in)
 	require.NoError(t, err)
 
 	timeoutTicker := helper.NewManualTicker()
@@ -41,6 +41,8 @@ func TestReadMonitorTimeout(t *testing.T) {
 	require.Equal(t, ctx.Err(), context.Canceled)
 	require.True(t, elapsed < time.Second, "Expected context to be cancelled quickly, but it was not")
 
+	cleanup()
+
 	// Verify that pipe is closed
 	_, err = io.ReadAll(r)
 	require.Error(t, err)
@@ -61,8 +63,9 @@ func TestReadMonitorSuccess(t *testing.T) {
 		strings.NewReader(postTimeoutPayload),
 	)
 
-	r, monitor, err := NewReadMonitor(ctx, in)
+	r, monitor, cleanup, err := NewReadMonitor(ctx, in)
 	require.NoError(t, err)
+	defer cleanup()
 
 	timeoutTicker := helper.NewManualTicker()
 
diff --git a/internal/gitaly/service/ssh/monitor_stdin_command.go b/internal/gitaly/service/ssh/monitor_stdin_command.go
index 3d78218ba..5bad1df27 100644
--- a/internal/gitaly/service/ssh/monitor_stdin_command.go
+++ b/internal/gitaly/service/ssh/monitor_stdin_command.go
@@ -11,16 +11,20 @@ import (
 )
 
 func monitorStdinCommand(ctx context.Context, gitCmdFactory git.CommandFactory, stdin io.Reader, stdout, stderr io.Writer, sc git.SubCmd, opts ...git.CmdOpt) (*command.Command, *pktline.ReadMonitor, error) {
-	stdinPipe, monitor, err := pktline.NewReadMonitor(ctx, stdin)
+	stdinPipe, monitor, cleanup, err := pktline.NewReadMonitor(ctx, stdin)
 	if err != nil {
 		return nil, nil, fmt.Errorf("create monitor: %v", err)
 	}
 
 	cmd, err := gitCmdFactory.NewWithoutRepo(ctx, sc, append([]git.CmdOpt{
-		git.WithStdin(stdinPipe), git.WithStdout(stdout), git.WithStderr(stderr),
+		git.WithStdin(stdinPipe),
+		git.WithStdout(stdout),
+		git.WithStderr(stderr),
+		git.WithFinalizer(func(*command.Command) { cleanup() }),
 	}, opts...)...)
 	stdinPipe.Close() // this now belongs to cmd
 	if err != nil {
+		cleanup()
 		return nil, nil, fmt.Errorf("start cmd: %v", err)
 	}