Buffer request in tempfile, not memory

internal/git/upload-pack.go

@@ -6,15 +6,13 @@ import (
 	"io"
 	"io/ioutil"
 	"net/http"
+	"os"
 
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/api"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
 )
 
 func handleUploadPack(w *GitHttpResponseWriter, r *http.Request, a *api.Response) (writtenIn int64, err error) {
-	var isShallowClone bool
-	var body io.Reader
-
 	buffer := &bytes.Buffer{}
 	// Only sniff on the first 4096 bytes: we assume that if we find no
 	// 'deepen' message in the first 4096 bytes there won't be one later
@@ -24,19 +22,18 @@ func handleUploadPack(w *GitHttpResponseWriter, r *http.Request, a *api.Response
 		fail500(w)
 		return writtenIn, &copyError{fmt.Errorf("buffer git-upload-pack body: %v", err)}
 	}
+	isShallowClone := scanDeepen(bytes.NewReader(buffer.Bytes()))
 
-	isShallowClone = scanDeepen(bytes.NewReader(buffer.Bytes()))
-	body = io.MultiReader(buffer, r.Body)
-
-	// Read out the full HTTP request body so that we can reply
-	buf, err := ioutil.ReadAll(body)
-
+	// We must drain the request body before writing the response, to avoid
+	// upsetting NGINX.
+	remainder, err := bufferInTempfile(r.Body)
 	if err != nil {
 		fail500(w)
-		return writtenIn, &copyError{fmt.Errorf("full buffer git-upload-pack body: %v", err)}
+		return writtenIn, fmt.Errorf("bufferInTempfile: %v", err)
 	}
+	defer remainder.Close()
 
-	body = ioutil.NopCloser(bytes.NewBuffer(buf))
+	body := ioutil.NopCloser(io.MultiReader(buffer, remainder))
 	r.Body.Close()
 
 	action := getService(r)
@@ -52,11 +49,10 @@ func handleUploadPack(w *GitHttpResponseWriter, r *http.Request, a *api.Response
 	defer helper.CleanUpProcessGroup(cmd) // Ensure brute force subprocess clean-up
 
 	stdoutError := make(chan error, 1)
-
-	// Start writing the response
-	writePostRPCHeader(w, action)
-
 	go func() {
+		writePostRPCHeader(w, action)
+		// Start reading from stdout already to avoid blocking while writing to
+		// stdin below.
 		_, err := io.Copy(w, stdout)
 		// This error may be lost if some other error prevents us from <-ing on this channel.
 		stdoutError <- err
@@ -87,3 +83,24 @@ func handleUploadPack(w *GitHttpResponseWriter, r *http.Request, a *api.Response
 func fail500(w http.ResponseWriter) {
 	helper.Fail500(w, nil, nil)
 }
+
+func bufferInTempfile(r io.Reader) (io.ReadCloser, error) {
+	buffer, err := ioutil.TempFile("", "gitlab-workhorse-git-request-body")
+	if err != nil {
+		return nil, err
+	}
+
+	if err := os.Remove(buffer.Name()); err != nil {
+		return nil, err
+	}
+
+	if _, err := io.Copy(buffer, r); err != nil {
+		return nil, err
+	}
+
+	if _, err := buffer.Seek(0, 0); err != nil {
+		return nil, err
+	}
+
+	return buffer, nil
+}