Merge branch 'jt-sshreceivepack-deadlock' into 'master'

ssh: Fix blocked `Wait()` when git process crashes

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5164

Merged-by: Will Chandler <wchandler@gitlab.com>
Approved-by: James Fargher <proglottis@gmail.com>
Approved-by: Will Chandler <wchandler@gitlab.com>
Co-authored-by: Justin Tobler <jtobler@gitlab.com>

2 files changed, 50 insertions, 1 deletions

diff --git a/internal/gitaly/service/ssh/receive_pack.go b/internal/gitaly/service/ssh/receive_pack.go
index ea292e6f3..9f6ea5f04 100644
--- a/internal/gitaly/service/ssh/receive_pack.go
+++ b/internal/gitaly/service/ssh/receive_pack.go
@@ -4,6 +4,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"os"
 	"strings"
 	"sync"
 
@@ -76,12 +77,35 @@ func (s *server) sshReceivePack(stream gitalypb.SSHService_SSHReceivePackServer,
 		return err
 	}
 
+	// When an `exec.Cmd` has its `cmd.Stdin` configured with an `io.Reader`
+	// that is not also of type `os.File` a goroutine is automatically
+	// configured that performs an `io.Copy()` between the reader and a newly
+	// created pipe. A problem with this can arise when `cmd.Wait()` is invoked
+	// because it waits not only for the process to complete but also all the
+	// goroutine to end. If the configured `cmd.Stdin` is only of type
+	// `io.Reader` and never closed, the goroutine will never end. This leads to
+	// `cmd.Wait()` being blocked indefinitely.
+	//
+	// Within Gitaly this problem can manifest itself when a git process crashes
+	// before `stdin` reaches EOF. To date this has only been noticed as a
+	// problem for the `SSHReceivePack` RPC, so a pipe and goroutine have been
+	// created explicitly to prevent `cmd.Wait()` from blocking indefinitely.
+	pr, pw, err := os.Pipe()
+	if err != nil {
+		return fmt.Errorf("creating pipe: %w", err)
+	}
+
+	go func() {
+		_, _ = io.Copy(pw, stdin)
+		_ = pw.Close()
+	}()
+
 	cmd, err := s.gitCmdFactory.New(ctx, req.GetRepository(),
 		git.Command{
 			Name: "receive-pack",
 			Args: []string{repoPath},
 		},
-		git.WithStdin(stdin),
+		git.WithStdin(pr),
 		git.WithStdout(stdout),
 		git.WithStderr(stderr),
 		git.WithReceivePackHooks(req, "ssh"),
diff --git a/internal/gitaly/service/ssh/receive_pack_test.go b/internal/gitaly/service/ssh/receive_pack_test.go
index 78256747e..bafba6c3b 100644
--- a/internal/gitaly/service/ssh/receive_pack_test.go
+++ b/internal/gitaly/service/ssh/receive_pack_test.go
@@ -201,6 +201,31 @@ func TestReceivePack_success(t *testing.T) {
 	}, payload)
 }
 
+func TestReceivePack_invalidGitconfig(t *testing.T) {
+	t.Parallel()
+
+	ctx := testhelper.Context(t)
+	cfg := testcfg.Build(t)
+	cfg.SocketPath = runSSHServer(t, cfg)
+
+	testcfg.BuildGitalySSH(t, cfg)
+	testcfg.BuildGitalyHooks(t, cfg)
+
+	repo, repoPath := gittest.CreateRepository(t, ctx, cfg)
+	gittest.WriteCommit(t, cfg, repoPath, gittest.WithBranch("main"))
+	require.NoError(t, os.WriteFile(filepath.Join(repoPath, "config"), []byte("x x x foobar"), 0o644))
+
+	lHead, rHead, err := testCloneAndPush(t, ctx, cfg, cfg.SocketPath, repo, repoPath, pushParams{
+		storageName:   cfg.Storages[0].Name,
+		glID:          "123",
+		glUsername:    "user",
+		glRepository:  "something",
+		glProjectPath: "something",
+	})
+	require.Error(t, err)
+	require.Equal(t, lHead, rHead, "local and remote head not equal. push failed")
+}
+
 func TestReceivePack_client(t *testing.T) {
 	t.Parallel()