Merge branch '1473-refnames-bug' into 'master'

Avoid unsafe use of scanner.Bytes() in ref name RPC's

Closes #1473

See merge request gitlab-org/gitaly!1054

5 files changed, 169 insertions, 14 deletions

diff --git a/changelogs/unreleased/1473-refnames-bug.yml b/changelogs/unreleased/1473-refnames-bug.yml
new file mode 100644
index 000000000..b44475a43
--- /dev/null
+++ b/changelogs/unreleased/1473-refnames-bug.yml
@@ -0,0 +1,5 @@
+---
+title: Avoid unsafe use of scanner.Bytes() in ref name RPC's
+merge_request: 1054
+author:
+type: fixed
diff --git a/internal/service/ref/refnames.go b/internal/service/ref/refnames.go
index 6c50c84ef..43c20969e 100644
--- a/internal/service/ref/refnames.go
+++ b/internal/service/ref/refnames.go
@@ -23,7 +23,7 @@ type findAllBranchNamesSender struct {
 
 func (ts *findAllBranchNamesSender) Reset() { ts.branchNames = nil }
 func (ts *findAllBranchNamesSender) Append(it chunk.Item) {
-	ts.branchNames = append(ts.branchNames, it.([]byte))
+	ts.branchNames = append(ts.branchNames, []byte(it.(string)))
 }
 func (ts *findAllBranchNamesSender) Send() error {
 	return ts.stream.Send(&gitalypb.FindAllBranchNamesResponse{Names: ts.branchNames})
@@ -43,7 +43,7 @@ type findAllTagNamesSender struct {
 
 func (ts *findAllTagNamesSender) Reset() { ts.tagNames = nil }
 func (ts *findAllTagNamesSender) Append(it chunk.Item) {
-	ts.tagNames = append(ts.tagNames, it.([]byte))
+	ts.tagNames = append(ts.tagNames, []byte(it.(string)))
 }
 func (ts *findAllTagNamesSender) Send() error {
 	return ts.stream.Send(&gitalypb.FindAllTagNamesResponse{Names: ts.tagNames})
@@ -64,7 +64,10 @@ func listRefNames(ctx context.Context, chunker *chunk.Chunker, prefix string, re
 
 	scanner := bufio.NewScanner(cmd)
 	for scanner.Scan() {
-		if err := chunker.Send(scanner.Bytes()); err != nil {
+		// Important: don't use scanner.Bytes() because the slice will become
+		// invalid on the next loop iteration. Instead, use scanner.Text() to
+		// force a copy.
+		if err := chunker.Send(scanner.Text()); err != nil {
 			return err
 		}
 	}
diff --git a/internal/service/ref/refnames_containing.go b/internal/service/ref/refnames_containing.go
index 0ec9e7554..1fc4595dd 100644
--- a/internal/service/ref/refnames_containing.go
+++ b/internal/service/ref/refnames_containing.go
@@ -1,8 +1,8 @@
 package ref
 
 import (
-	"bytes"
 	"fmt"
+	"strings"
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
@@ -82,5 +82,5 @@ func (ts *tagNamesContainingCommitSender) Send() error {
 }
 
 func stripPrefix(it chunk.Item, prefix string) []byte {
-	return bytes.TrimPrefix(it.([]byte), []byte(prefix))
+	return []byte(strings.TrimPrefix(it.(string), prefix))
 }
diff --git a/internal/service/ref/refs_test.go b/internal/service/ref/refs_test.go
index 4648b56d0..c4431c376 100644
--- a/internal/service/ref/refs_test.go
+++ b/internal/service/ref/refs_test.go
@@ -1,8 +1,10 @@
 package ref
 
 import (
+	"bufio"
 	"bytes"
 	"context"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"strings"
@@ -12,6 +14,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/git/log"
+	"gitlab.com/gitlab-org/gitaly/internal/git/updateref"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"google.golang.org/grpc/codes"
@@ -41,9 +44,7 @@ func TestSuccessfulFindAllBranchNames(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 	c, err := client.FindAllBranchNames(ctx, rpcRequest)
-	if err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, err)
 
 	var names [][]byte
 	for {
@@ -51,15 +52,72 @@ func TestSuccessfulFindAllBranchNames(t *testing.T) {
 		if err == io.EOF {
 			break
 		}
-		if err != nil {
-			t.Fatal(err)
-		}
+		require.NoError(t, err)
+
 		names = append(names, r.GetNames()...)
 	}
-	for _, branch := range []string{"master", "100%branch", "improve/awesome", "'test'"} {
-		if !containsRef(names, "refs/heads/"+branch) {
-			t.Fatalf("Expected to find branch %q in all branch names", branch)
+
+	expectedBranches, err := ioutil.ReadFile("testdata/branches.txt")
+	require.NoError(t, err)
+
+	for _, branch := range bytes.Split(bytes.TrimSpace(expectedBranches), []byte("\n")) {
+		require.Contains(t, names, branch)
+	}
+}
+
+func TestFindAllBranchNamesVeryLargeResponse(t *testing.T) {
+	server, serverSocketPath := runRefServiceServer(t)
+	defer server.Stop()
+
+	client, conn := newRefServiceClient(t, serverSocketPath)
+	defer conn.Close()
+
+	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	updater, err := updateref.New(ctx, testRepo)
+	require.NoError(t, err)
+
+	// We want to create enough refs to overflow the default bufio.Scanner
+	// buffer. Such an overflow will cause scanner.Bytes() to become invalid
+	// at some point. That is expected behavior, but our tests did not
+	// trigger it, so we got away with naively using scanner.Bytes() and
+	// causing a bug: https://gitlab.com/gitlab-org/gitaly/issues/1473.
+	refSizeLowerBound := 100
+	numRefs := 2 * bufio.MaxScanTokenSize / refSizeLowerBound
+
+	var testRefs []string
+	for i := 0; i < numRefs; i++ {
+		refName := fmt.Sprintf("refs/heads/test-%0100d", i)
+		require.True(t, len(refName) > refSizeLowerBound, "ref %q must be larger than %d", refName, refSizeLowerBound)
+
+		require.NoError(t, updater.Create(refName, "HEAD"))
+		testRefs = append(testRefs, refName)
+	}
+
+	require.NoError(t, updater.Wait())
+
+	rpcRequest := &gitalypb.FindAllBranchNamesRequest{Repository: testRepo}
+
+	c, err := client.FindAllBranchNames(ctx, rpcRequest)
+	require.NoError(t, err)
+
+	var names [][]byte
+	for {
+		r, err := c.Recv()
+		if err == io.EOF {
+			break
 		}
+		require.NoError(t, err)
+
+		names = append(names, r.GetNames()...)
+	}
+
+	for _, branch := range testRefs {
+		require.Contains(t, names, []byte(branch), "branch missing from response: %q", branch)
 	}
 }
 
diff --git a/internal/service/ref/testdata/branches.txt b/internal/service/ref/testdata/branches.txt
new file mode 100644
index 000000000..a37ad579e
--- /dev/null
+++ b/internal/service/ref/testdata/branches.txt
@@ -0,0 +1,89 @@
+refs/heads/'test'
+refs/heads/100%branch
+refs/heads/1942eed5cc108b19c7405106e81fa96125d0be22
+refs/heads/2-mb-file
+refs/heads/add-balsamiq-file
+refs/heads/add-ipython-files
+refs/heads/add-pdf-file
+refs/heads/add-pdf-text-binary
+refs/heads/add_images_and_changes
+refs/heads/after-create-delete-modify-move
+refs/heads/author-committer-different
+refs/heads/before-create-delete-modify-move
+refs/heads/big-files
+refs/heads/binary-encoding
+refs/heads/blob-with-tricky-encoding
+refs/heads/branch-merged
+refs/heads/conflict-binary-file
+refs/heads/conflict-contains-conflict-markers
+refs/heads/conflict-missing-side
+refs/heads/conflict-non-utf8
+refs/heads/conflict-resolvable
+refs/heads/conflict-start
+refs/heads/conflict-too-large
+refs/heads/conflict_branch_a
+refs/heads/conflict_branch_b
+refs/heads/crlf-diff
+refs/heads/csv
+refs/heads/custom-encoding
+refs/heads/deleted-image-test
+refs/heads/empty-branch
+refs/heads/ends-with.json
+refs/heads/expand-collapse-diffs
+refs/heads/expand-collapse-files
+refs/heads/expand-collapse-lines
+refs/heads/feature
+refs/heads/feature-and-encoding
+refs/heads/feature-encoding-conflict
+refs/heads/feature.custom-highlighting
+refs/heads/feature_conflict
+refs/heads/few-commits
+refs/heads/file-mode-change
+refs/heads/fix
+refs/heads/flat-path
+refs/heads/flat-path-2
+refs/heads/flatten-dirs
+refs/heads/gitaly-diff-stuff
+refs/heads/gitaly-non-utf8-commit
+refs/heads/gitaly-rename-test
+refs/heads/gitaly-stuff
+refs/heads/gitaly-test-ref
+refs/heads/gitaly-windows-1251
+refs/heads/improve/awesome
+refs/heads/jv-conflict-1
+refs/heads/jv-conflict-2
+refs/heads/many_files
+refs/heads/markdown
+refs/heads/master
+refs/heads/merge-commit-analyze-after
+refs/heads/merged-target
+refs/heads/moar-lfs-ptrs
+refs/heads/not-merged-branch
+refs/heads/not-mergéd-branch
+refs/heads/orphaned-branch
+refs/heads/pages-deploy
+refs/heads/pages-deploy-target
+refs/heads/png-lfs
+refs/heads/rd-add-file-larger-than-1-mb
+refs/heads/rebase-encoding-failure-trigger
+refs/heads/signed-commits
+refs/heads/spooky-stuff
+refs/heads/squash-encoding-error-trigger
+refs/heads/squash-large-files
+refs/heads/submodule_inside_folder
+refs/heads/symlink-expand-diff
+refs/heads/test-do-not-touch
+refs/heads/two-commits
+refs/heads/use-gitlab-shell-v-6-0-1
+refs/heads/use-gitlab-shell-v-6-0-3
+refs/heads/utf-16
+refs/heads/utf-16-2
+refs/heads/utf-dir
+refs/heads/v1.1.0
+refs/heads/very-large-diff-ordered
+refs/heads/video
+refs/heads/wip
+refs/heads/with-codeowners
+refs/heads/with-executables
+refs/heads/Ääh-test-utf-8
+refs/heads/ʕ•ᴥ•ʔ