Add latest changes from gitlab-org/gitlab@master

2 files changed, 42 insertions, 18 deletions

diff --git a/workhorse/internal/imageresizer/image_resizer.go b/workhorse/internal/imageresizer/image_resizer.go
index feefd9c6dee..77318ed1c46 100644
--- a/workhorse/internal/imageresizer/image_resizer.go
+++ b/workhorse/internal/imageresizer/image_resizer.go
@@ -5,7 +5,6 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"math"
 	"net"
 	"net/http"
 	"os"
@@ -17,6 +16,7 @@ import (
 	"time"
 
 	"github.com/prometheus/client_golang/prometheus"
+	"github.com/prometheus/client_golang/prometheus/promauto"
 
 	"gitlab.com/gitlab-org/labkit/correlation"
 	"gitlab.com/gitlab-org/labkit/log"
@@ -96,7 +96,7 @@ const (
 )
 
 var (
-	imageResizeConcurrencyLimitExceeds = prometheus.NewCounter(
+	imageResizeConcurrencyLimitExceeds = promauto.NewCounter(
 		prometheus.CounterOpts{
 			Namespace: namespace,
 			Subsystem: subsystem,
@@ -104,7 +104,7 @@ var (
 			Help:      "Amount of image resizing requests that exceeded the maximum allowed scaler processes",
 		},
 	)
-	imageResizeProcesses = prometheus.NewGauge(
+	imageResizeProcesses = promauto.NewGauge(
 		prometheus.GaugeOpts{
 			Namespace: namespace,
 			Subsystem: subsystem,
@@ -112,7 +112,7 @@ var (
 			Help:      "Amount of image scaler processes working now",
 		},
 	)
-	imageResizeMaxProcesses = prometheus.NewGauge(
+	imageResizeMaxProcesses = promauto.NewGauge(
 		prometheus.GaugeOpts{
 			Namespace: namespace,
 			Subsystem: subsystem,
@@ -120,7 +120,7 @@ var (
 			Help:      "The maximum amount of image scaler processes allowed to run concurrently",
 		},
 	)
-	imageResizeRequests = prometheus.NewCounterVec(
+	imageResizeRequests = promauto.NewCounterVec(
 		prometheus.CounterOpts{
 			Namespace: namespace,
 			Subsystem: subsystem,
@@ -129,7 +129,7 @@ var (
 		},
 		[]string{"status"},
 	)
-	imageResizeDurations = prometheus.NewHistogramVec(
+	imageResizeDurations = promauto.NewHistogramVec(
 		prometheus.HistogramOpts{
 			Namespace: namespace,
 			Subsystem: subsystem,
@@ -154,14 +154,6 @@ const (
 	maxMagicLen = 8                   // 8 first bytes is enough to detect PNG or JPEG
 )
 
-func init() {
-	prometheus.MustRegister(imageResizeConcurrencyLimitExceeds)
-	prometheus.MustRegister(imageResizeProcesses)
-	prometheus.MustRegister(imageResizeMaxProcesses)
-	prometheus.MustRegister(imageResizeRequests)
-	prometheus.MustRegister(imageResizeDurations)
-}
-
 func NewResizer(cfg config.Config) *Resizer {
 	imageResizeMaxProcesses.Set(float64(cfg.ImageResizerConfig.MaxScalerProcs))
 
@@ -279,6 +271,10 @@ func (r *Resizer) tryResizeImage(req *http.Request, f *imageFile, params *resize
 		return f.reader, nil, fmt.Errorf("%d bytes exceeds maximum file size of %d bytes", f.contentLength, cfg.MaxFilesize)
 	}
 
+	if f.contentLength < maxMagicLen {
+		return f.reader, nil, fmt.Errorf("file is too small to resize: %d bytes", f.contentLength)
+	}
+
 	if !r.numScalerProcs.tryIncrement(int32(cfg.MaxScalerProcs)) {
 		return f.reader, nil, fmt.Errorf("too many running scaler processes (%d / %d)", r.numScalerProcs.n, cfg.MaxScalerProcs)
 	}
@@ -289,11 +285,16 @@ func (r *Resizer) tryResizeImage(req *http.Request, f *imageFile, params *resize
 		r.numScalerProcs.decrement()
 	}()
 
-	// Prevents EOF if the file is smaller than 8 bytes
-	bufferSize := int(math.Min(maxMagicLen, float64(f.contentLength)))
-	buffered := bufio.NewReaderSize(f.reader, bufferSize)
+	// Creating buffered Reader is required for us to Peek into first bytes of the image file to detect the format
+	// without advancing the reader (we need to read from the file start in the Scaler binary).
+	// We set `8` as the minimal buffer size by the length of PNG magic bytes sequence (JPEG needs only 2).
+	// In fact, `NewReaderSize` will immediately override it with `16` using its `minReadBufferSize` -
+	// here we are just being explicit about the buffer size required for our code to operate correctly.
+	// Having a reader with such tiny buffer will not hurt the performance during further operations,
+	// because Golang `bufio.Read` avoids double copy: https://golang.org/src/bufio/bufio.go?s=1768:1804#L212
+	buffered := bufio.NewReaderSize(f.reader, maxMagicLen)
 
-	headerBytes, err := buffered.Peek(bufferSize)
+	headerBytes, err := buffered.Peek(maxMagicLen)
 	if err != nil {
 		return buffered, nil, fmt.Errorf("peek stream: %v", err)
 	}
diff --git a/workhorse/internal/imageresizer/image_resizer_test.go b/workhorse/internal/imageresizer/image_resizer_test.go
index 49cd88200aa..bacc97738b8 100644
--- a/workhorse/internal/imageresizer/image_resizer_test.go
+++ b/workhorse/internal/imageresizer/image_resizer_test.go
@@ -198,6 +198,29 @@ func TestServeOriginalImageWhenSourceImageFormatIsNotAllowed(t *testing.T) {
 	require.Equal(t, svgImage, responseData, "expected original image")
 }
 
+func TestServeOriginalImageWhenSourceImageIsTooSmall(t *testing.T) {
+	content := []byte("PNG") // 3 bytes only, invalid as PNG/JPEG image
+
+	img, err := ioutil.TempFile("", "*.png")
+	require.NoError(t, err)
+
+	defer img.Close()
+	defer os.Remove(img.Name())
+
+	_, err = img.Write(content)
+	require.NoError(t, err)
+
+	cfg := config.DefaultImageResizerConfig
+	params := resizeParams{Location: img.Name(), ContentType: "image/png", Width: 64}
+
+	resp := requestScaledImage(t, nil, params, cfg)
+	require.Equal(t, http.StatusOK, resp.StatusCode)
+
+	responseData, err := ioutil.ReadAll(resp.Body)
+	require.NoError(t, err)
+	require.Equal(t, content, responseData, "expected original image")
+}
+
 // The Rails applications sends a Base64 encoded JSON string carrying
 // these parameters in an HTTP response header
 func encodeParams(t *testing.T, p *resizeParams) string {