Merge branch 'alipniagov-comment-on-buffer-size' into 'master'

Do not resize when image is less than 8 bytes, add comments

See merge request gitlab-org/gitlab-workhorse!666

changelogs/unreleased/alipniagov-comment-on-buffer-size.yml

+---
+title: Do not resize when image is less than 8 bytes
+merge_request: 666
+author:
+type: other

internal/imageresizer/image_resizer.go

@@ -5,7 +5,6 @@ import (
 	"context"
 	"fmt"
 	"io"
-	"math"
 	"net"
 	"net/http"
 	"os"
@@ -272,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)
 	}
@@ -282,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)
 	}

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 {