Merge branch 'fix-pktline-splitter' into 'master'

Fix panic in git pktline splitter

See merge request gitlab-org/gitaly!893

6 files changed, 141 insertions, 88 deletions

diff --git a/changelogs/unreleased/fix-pktline-splitter.yml b/changelogs/unreleased/fix-pktline-splitter.yml
new file mode 100644
index 000000000..2ae91fff1
--- /dev/null
+++ b/changelogs/unreleased/fix-pktline-splitter.yml
@@ -0,0 +1,5 @@
+---
+title: Fix panic in git pktline splitter
+merge_request: 893
+author:
+type: fixed
diff --git a/internal/git/pktline/pktline.go b/internal/git/pktline/pktline.go
new file mode 100644
index 000000000..c53463a0a
--- /dev/null
+++ b/internal/git/pktline/pktline.go
@@ -0,0 +1,102 @@
+package pktline
+
+// Utility functions for working with the Git pkt-line format. See
+// https://github.com/git/git/blob/master/Documentation/technical/protocol-common.txt
+
+// TODO add tests https://gitlab.com/gitlab-org/gitaly/issues/1340
+
+import (
+	"bufio"
+	"bytes"
+	"fmt"
+	"io"
+	"strconv"
+)
+
+const (
+	maxPktSize = 0xffff
+)
+
+var (
+	flush = []byte("0000")
+)
+
+// NewScanner returns a bufio.Scanner that splits on Git pktline boundaries
+func NewScanner(r io.Reader) *bufio.Scanner {
+	scanner := bufio.NewScanner(r)
+	scanner.Buffer(make([]byte, maxPktSize), maxPktSize)
+	scanner.Split(pktLineSplitter)
+	return scanner
+}
+
+// Data returns the packet pkt without its length header. The length
+// header is not validated. Returns nil when pkt is a magic packet such
+// as '0000'.
+func Data(pkt []byte) []byte {
+	if len(pkt) == 4 {
+		return nil
+	}
+
+	return pkt[4:]
+}
+
+// IsFlush detects the special flush packet '0000'
+func IsFlush(pkt []byte) bool {
+	return bytes.Equal(pkt, flush)
+}
+
+// WriteString writes a string with pkt-line framing
+func WriteString(w io.Writer, str string) (int, error) {
+	pktLen := len(str) + 4
+	if pktLen > maxPktSize {
+		return 0, fmt.Errorf("string too large: %d bytes", len(str))
+	}
+
+	return fmt.Fprintf(w, "%04x%s", pktLen, str)
+}
+
+// WriteFlush write a pkt flush packet.
+func WriteFlush(w io.Writer) error {
+	_, err := w.Write(flush)
+	return err
+}
+
+func pktLineSplitter(data []byte, atEOF bool) (advance int, token []byte, err error) {
+	if len(data) < 4 {
+		if atEOF && len(data) > 0 {
+			return 0, nil, fmt.Errorf("pktLineSplitter: incomplete length prefix on %q", data)
+		}
+		return 0, nil, nil // want more data
+	}
+
+	// We have at least 4 bytes available so we can decode the 4-hex digit
+	// length prefix of the packet line.
+	pktLength64, err := strconv.ParseInt(string(data[:4]), 16, 0)
+	if err != nil {
+		return 0, nil, fmt.Errorf("pktLineSplitter: decode length: %v", err)
+	}
+
+	// Cast is safe because we requested an int-size number from strconv.ParseInt
+	pktLength := int(pktLength64)
+
+	if pktLength < 0 {
+		return 0, nil, fmt.Errorf("pktLineSplitter: invalid length: %d", pktLength)
+	}
+
+	if pktLength < 4 {
+		// Special case: magic empty packet 0000, 0001, 0002 or 0003.
+		return 4, data[:4], nil
+	}
+
+	if len(data) < pktLength {
+		// data contains incomplete packet
+
+		if atEOF {
+			return 0, nil, fmt.Errorf("pktLineSplitter: less than %d bytes in input %q", pktLength, data)
+		}
+
+		return 0, nil, nil // want more data
+	}
+
+	return pktLength, data[:pktLength], nil
+}
diff --git a/internal/service/smarthttp/inforefs.go b/internal/service/smarthttp/inforefs.go
index 101d18b96..76defd99f 100644
--- a/internal/service/smarthttp/inforefs.go
+++ b/internal/service/smarthttp/inforefs.go
@@ -9,6 +9,7 @@ import (
 	log "github.com/sirupsen/logrus"
 	pb "gitlab.com/gitlab-org/gitaly-proto/go"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
+	"gitlab.com/gitlab-org/gitaly/internal/git/pktline"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/streamio"
 
@@ -55,11 +56,11 @@ func handleInfoRefs(ctx context.Context, service string, req *pb.InfoRefsRequest
 		return status.Errorf(codes.Internal, "GetInfoRefs: cmd: %v", err)
 	}
 
-	if err := pktLine(w, fmt.Sprintf("# service=git-%s\n", service)); err != nil {
+	if _, err := pktline.WriteString(w, fmt.Sprintf("# service=git-%s\n", service)); err != nil {
 		return status.Errorf(codes.Internal, "GetInfoRefs: pktLine: %v", err)
 	}
 
-	if err := pktFlush(w); err != nil {
+	if err := pktline.WriteFlush(w); err != nil {
 		return status.Errorf(codes.Internal, "GetInfoRefs: pktFlush: %v", err)
 	}
 
@@ -73,13 +74,3 @@ func handleInfoRefs(ctx context.Context, service string, req *pb.InfoRefsRequest
 
 	return nil
 }
-
-func pktLine(w io.Writer, s string) error {
-	_, err := fmt.Fprintf(w, "%04x%s", len(s)+4, s)
-	return err
-}
-
-func pktFlush(w io.Writer) error {
-	_, err := fmt.Fprint(w, "0000")
-	return err
-}
diff --git a/internal/service/smarthttp/scan_deepen.go b/internal/service/smarthttp/scan_deepen.go
index 9ec501878..b6d997b57 100644
--- a/internal/service/smarthttp/scan_deepen.go
+++ b/internal/service/smarthttp/scan_deepen.go
@@ -1,21 +1,19 @@
 package smarthttp
 
 import (
-	"bufio"
 	"bytes"
-	"fmt"
 	"io"
 	"io/ioutil"
-	"strconv"
+
+	"gitlab.com/gitlab-org/gitaly/internal/git/pktline"
 )
 
 func scanDeepen(body io.Reader) bool {
 	result := false
 
-	scanner := bufio.NewScanner(body)
-	scanner.Split(pktLineSplitter)
+	scanner := pktline.NewScanner(body)
 	for scanner.Scan() {
-		if bytes.HasPrefix(scanner.Bytes(), []byte("deepen")) && scanner.Err() == nil {
+		if bytes.HasPrefix(pktline.Data(scanner.Bytes()), []byte("deepen")) && scanner.Err() == nil {
 			result = true
 			break
 		}
@@ -26,41 +24,3 @@ func scanDeepen(body io.Reader) bool {
 	io.Copy(ioutil.Discard, body)
 	return result
 }
-
-func pktLineSplitter(data []byte, atEOF bool) (advance int, token []byte, err error) {
-	if len(data) < 4 {
-		if atEOF && len(data) > 0 {
-			return 0, nil, fmt.Errorf("pktLineSplitter: incomplete length prefix on %q", data)
-		}
-		return 0, nil, nil // want more data
-	}
-
-	if bytes.HasPrefix(data, []byte("0000")) {
-		// special case: "0000" terminator packet: return empty token
-		return 4, data[:0], nil
-	}
-
-	// We have at least 4 bytes available so we can decode the 4-hex digit
-	// length prefix of the packet line.
-	pktLength64, err := strconv.ParseInt(string(data[:4]), 16, 0)
-	if err != nil {
-		return 0, nil, fmt.Errorf("pktLineSplitter: decode length: %v", err)
-	}
-
-	// Cast is safe because we requested an int-size number from strconv.ParseInt
-	pktLength := int(pktLength64)
-
-	if pktLength < 0 {
-		return 0, nil, fmt.Errorf("pktLineSplitter: invalid length: %d", pktLength)
-	}
-
-	if len(data) < pktLength {
-		if atEOF {
-			return 0, nil, fmt.Errorf("pktLineSplitter: less than %d bytes in input %q", pktLength, data)
-		}
-		return 0, nil, nil // want more data
-	}
-
-	// return "pkt" token without length prefix
-	return pktLength, data[4:pktLength], nil
-}
diff --git a/internal/service/smarthttp/scan_deepen_test.go b/internal/service/smarthttp/scan_deepen_test.go
index c5a8103d6..0a5f90ef3 100644
--- a/internal/service/smarthttp/scan_deepen_test.go
+++ b/internal/service/smarthttp/scan_deepen_test.go
@@ -13,7 +13,10 @@ func TestSuccessfulScanDeepen(t *testing.T) {
 		output bool
 	}{
 		{"000dsomething000cdeepen 10000", true},
-		{"000dsomething0000000cdeepen 1", true},
+		{"000dsomething0000000cdeepen 1", true}, // 0000 packet
+		{"000dsomething0001000cdeepen 1", true}, // 0001 packet
+		{"000dsomething0002000cdeepen 1", true}, // 0002 packet
+		{"000dsomething0003000cdeepen 1", true}, // 0003 packet
 		{"000dsomething0000000cdeepen 1" + strings.Repeat("garbage", 1000000), true},
 		{"ffff" + strings.Repeat("x", 65531) + "000cdeepen 1", true},
 		{"000dsomething0000", false},
diff --git a/internal/service/smarthttp/upload_pack_test.go b/internal/service/smarthttp/upload_pack_test.go
index d3f20b2fa..502c231d6 100644
--- a/internal/service/smarthttp/upload_pack_test.go
+++ b/internal/service/smarthttp/upload_pack_test.go
@@ -7,10 +7,10 @@ import (
 	"io"
 	"os"
 	"path"
-	"strconv"
 	"testing"
 	"time"
 
+	"gitlab.com/gitlab-org/gitaly/internal/git/pktline"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 
@@ -65,10 +65,10 @@ func TestSuccessfulUploadPackRequest(t *testing.T) {
 	// UploadPack request is a "want" packet line followed by a packet flush, then many "have" packets followed by a packet flush.
 	// This is explained a bit in https://git-scm.com/book/en/v2/Git-Internals-Transfer-Protocols#_downloading_data
 	requestBuffer := &bytes.Buffer{}
-	pktLine(requestBuffer, fmt.Sprintf("want %s %s\n", newHead, clientCapabilities))
-	pktFlush(requestBuffer)
-	pktLine(requestBuffer, fmt.Sprintf("have %s\n", oldHead))
-	pktFlush(requestBuffer)
+	pktline.WriteString(requestBuffer, fmt.Sprintf("want %s %s\n", newHead, clientCapabilities))
+	pktline.WriteFlush(requestBuffer)
+	pktline.WriteString(requestBuffer, fmt.Sprintf("have %s\n", oldHead))
+	pktline.WriteFlush(requestBuffer)
 
 	req := &pb.PostUploadPackRequest{
 		Repository: &pb.Repository{
@@ -120,10 +120,10 @@ func TestUploadPackRequestWithGitConfigOptions(t *testing.T) {
 	requestBodyCopy := &bytes.Buffer{}
 	tee := io.MultiWriter(requestBody, requestBodyCopy)
 
-	pktLine(tee, fmt.Sprintf("want %s %s\n", want, clientCapabilities))
-	pktFlush(tee)
-	pktLine(tee, fmt.Sprintf("have %s\n", have))
-	pktFlush(tee)
+	pktline.WriteString(tee, fmt.Sprintf("want %s %s\n", want, clientCapabilities))
+	pktline.WriteFlush(tee)
+	pktline.WriteString(tee, fmt.Sprintf("have %s\n", have))
+	pktline.WriteFlush(tee)
 
 	rpcRequest := &pb.PostUploadPackRequest{
 		Repository: &pb.Repository{
@@ -157,9 +157,9 @@ func TestSuccessfulUploadPackDeepenRequest(t *testing.T) {
 	testRepo := testhelper.TestRepository()
 
 	requestBody := &bytes.Buffer{}
-	pktLine(requestBody, fmt.Sprintf("want e63f41fe459e62e1228fcef60d7189127aeba95a %s\n", clientCapabilities))
-	pktLine(requestBody, "deepen 1")
-	pktFlush(requestBody)
+	pktline.WriteString(requestBody, fmt.Sprintf("want e63f41fe459e62e1228fcef60d7189127aeba95a %s\n", clientCapabilities))
+	pktline.WriteString(requestBody, "deepen 1")
+	pktline.WriteFlush(requestBody)
 
 	rpcRequest := &pb.PostUploadPackRequest{Repository: testRepo}
 	response, err := makePostUploadPackRequest(t, serverSocketPath, rpcRequest, requestBody)
@@ -222,32 +222,24 @@ func makePostUploadPackRequest(t *testing.T, serverSocketPath string, in *pb.Pos
 func extractPackDataFromResponse(t *testing.T, buf *bytes.Buffer) ([]byte, int, int) {
 	var pack []byte
 
-	// The response should have the following format, where <length> is always four hexadecimal digits.
-	// <length><data>
-	// <length><data>
+	// The response should have the following format.
+	// PKT-LINE
+	// PKT-LINE
 	// ...
 	// 0000
-	for {
-		pktLenStr := buf.Next(4)
-		if len(pktLenStr) != 4 {
-			return nil, 0, 0
-		}
-		if string(pktLenStr) == pktFlushStr {
+	scanner := pktline.NewScanner(buf)
+	for scanner.Scan() {
+		pkt := scanner.Bytes()
+		if pktline.IsFlush(pkt) {
 			break
 		}
 
-		pktLen, err := strconv.ParseUint(string(pktLenStr), 16, 16)
-		require.NoError(t, err)
-
-		restPktLen := int(pktLen) - 4
-		pkt := buf.Next(restPktLen)
-		require.Equal(t, restPktLen, len(pkt), "Incomplete packet read")
-
-		// The first byte of the packet is the band designator. We only care about data in band 1.
-		if pkt[0] == 1 {
-			pack = append(pack, pkt[1:]...)
+		// The first data byte of the packet is the band designator. We only care about data in band 1.
+		if data := pktline.Data(pkt); len(data) > 0 && data[0] == 1 {
+			pack = append(pack, data[1:]...)
 		}
 	}
+	require.NoError(t, scanner.Err())
 
 	// The packet is structured as follows:
 	// 4 bytes for signature, here it's "PACK"