Merge branch 'fix-exif-on-multipart-uploads' into 'master'

Fix EXIF cleaning for S3 compatible Object Storage

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

changelogs/unreleased/fix-exif-on-multipart-uploads.yml

+---
+title: Fix EXIF cleaning for S3 compatible Object Storage
+merge_request:
+author:
+type: fixed

internal/upload/exif/exif.go

@@ -19,15 +19,11 @@ type cleaner struct {
 	cmd    *exec.Cmd
 	stdout io.Reader
 	stderr bytes.Buffer
-	waitDone chan struct{}
-	waitErr  error
+	eof    bool
 }
 
-func NewCleaner(ctx context.Context, stdin io.Reader) (io.Reader, error) {
-	c := &cleaner{
-		ctx:      ctx,
-		waitDone: make(chan struct{}),
-	}
+func NewCleaner(ctx context.Context, stdin io.Reader) (io.ReadCloser, error) {
+	c := &cleaner{ctx: ctx}
 
 	if err := c.startProcessing(stdin); err != nil {
 		return nil, err
@@ -36,17 +32,32 @@ func NewCleaner(ctx context.Context, stdin io.Reader) (io.Reader, error) {
 	return c, nil
 }
 
+func (c *cleaner) Close() error {
+	if c.cmd == nil {
+		return nil
+	}
+
+	return c.cmd.Wait()
+}
+
 func (c *cleaner) Read(p []byte) (int, error) {
+	if c.eof {
+		return 0, io.EOF
+	}
+
 	n, err := c.stdout.Read(p)
 	if err == io.EOF {
-		if waitErr := c.wait(); waitErr != nil {
+		if waitErr := c.cmd.Wait(); waitErr != nil {
 			log.WithContextFields(c.ctx, log.Fields{
 				"command": c.cmd.Args,
 				"stderr":  c.stderr.String(),
 				"error":   waitErr.Error(),
 			}).Print("exiftool command failed")
+
 			return n, ErrRemovingExif
 		}
+
+		c.eof = true
 	}
 
 	return n, err
@@ -85,19 +96,10 @@ func (c *cleaner) startProcessing(stdin io.Reader) error {
 	if err = c.cmd.Start(); err != nil {
 		return fmt.Errorf("start %v: %v", c.cmd.Args, err)
 	}
-	go func() {
-		c.waitErr = c.cmd.Wait()
-		close(c.waitDone)
-	}()
 
 	return nil
 }
 
-func (c *cleaner) wait() error {
-	<-c.waitDone
-	return c.waitErr
-}
-
 func IsExifFile(filename string) bool {
 	filenameMatch := regexp.MustCompile(`(?i)\.(jpg|jpeg|tiff)$`)
 

internal/upload/exif/exif_test.go

@@ -75,3 +75,21 @@ func TestNewCleanerWithInvalidFile(t *testing.T) {
 	require.Error(t, err, "Expected error when reading output")
 	require.Equal(t, int64(0), size, "Size of invalid image should be 0")
 }
+
+func TestNewCleanerReadingAfterEOF(t *testing.T) {
+	input, err := os.Open("testdata/sample_exif.jpg")
+	require.NoError(t, err)
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	cleaner, err := NewCleaner(ctx, input)
+	require.NoError(t, err, "Expected no error when creating cleaner command")
+
+	_, err = io.Copy(ioutil.Discard, cleaner)
+	require.NoError(t, err, "Expected no error when reading output")
+
+	buf := make([]byte, 1)
+	size, err := cleaner.Read(buf)
+	require.Equal(t, 0, size, "The output was already consumed by previous reads")
+	require.Equal(t, io.EOF, err, "We return EOF")
+}

internal/upload/rewrite.go

@@ -174,12 +174,12 @@ func handleExifUpload(ctx context.Context, r io.Reader, filename string) (io.Rea
 		"filename": filename,
 	}).Print("running exiftool to remove any metadata")
 
-	r, err := exif.NewCleaner(ctx, r)
+	cleaner, err := exif.NewCleaner(ctx, r)
 	if err != nil {
 		return nil, err
 	}
 
-	return ioutil.NopCloser(r), nil
+	return cleaner, nil
 }
 
 func handleLsifUpload(ctx context.Context, reader io.Reader, tempPath, filename string, preauth *api.Response) (io.ReadCloser, error) {