[MM-45504] Improve GIF preprocessing logic (#20595)

* Improve GIF preprocessing logic * Prefer io to ioutil * Avoid color palette allocation
2022-07-11 09:34:49 +02:00
--- a/app/post_metadata.go
+++ b/app/post_metadata.go
@@ -744,7 +744,7 @@ func parseImages(body io.Reader) (*model.PostImage, error) {

 	if format == "gif" {
 		// Decoding the config may have read some of the image data, so re-read the data that has already been read first
-		frameCount, err := imgutils.CountFrames(io.MultiReader(buf, body))
+		frameCount, err := imgutils.CountGIFFrames(io.MultiReader(buf, body))
 		if err != nil {
 			return nil, err
 		}
--- a/app/upload.go
+++ b/app/upload.go
@@ -6,6 +6,7 @@ package app
 import (
 	"errors"
 	"io"
+	"mime"
 	"net/http"
 	"path/filepath"
 	"strings"
@@ -21,6 +22,23 @@ import (

 const minFirstPartSize = 5 * 1024 * 1024 // 5MB

+func (a *App) genFileInfoFromReader(name string, file io.ReadSeeker, size int64) (*model.FileInfo, error) {
+	ext := strings.ToLower(filepath.Ext(name))
+	info := &model.FileInfo{
+		Name:     name,
+		MimeType: mime.TypeByExtension(ext),
+	}
+	if info.IsImage() {
+		config, _, err := a.ch.imgDecoder.DecodeConfig(file)
+		if err != nil {
+			return nil, err
+		}
+		info.Width = config.Width
+		info.Height = config.Height
+	}
+	return info, nil
+}
+
 func (a *App) runPluginsHook(c *request.Context, info *model.FileInfo, file io.Reader) *model.AppError {
 	pluginsEnvironment := a.GetPluginsEnvironment()
 	if pluginsEnvironment == nil {
@@ -244,10 +262,11 @@ func (a *App) UploadData(c *request.Context, us *model.UploadSession, rd io.Read
 		return nil, model.NewAppError("UploadData", "app.upload.upload_data.read_file.app_error", nil, err.Error(), http.StatusInternalServerError)
 	}

-	info, err := model.GetInfoForBytes(us.Filename, file, int(us.FileSize))
+	// generate file info
+	info, genErr := a.genFileInfoFromReader(us.Filename, file, us.FileSize)
 	file.Close()
-	if err != nil {
-		return nil, err
+	if genErr != nil {
+		return nil, model.NewAppError("UploadData", "app.upload.upload_data.gen_info.app_error", nil, genErr.Error(), http.StatusInternalServerError)
 	}

 	info.CreatorId = us.UserId
--- a/app/upload_test.go
+++ b/app/upload_test.go
@@ -17,6 +17,7 @@ import (

 	"github.com/mattermost/mattermost-server/v6/model"
 	"github.com/mattermost/mattermost-server/v6/utils/fileutils"
+	"github.com/mattermost/mattermost-server/v6/utils/imgutils"
 )

 func TestCreateUploadSession(t *testing.T) {
@@ -232,6 +233,23 @@ func TestUploadData(t *testing.T) {
 		require.NotEmpty(t, info.ThumbnailPath)
 		require.NotEmpty(t, info.PreviewPath)
 	})
+
+	t.Run("huge GIF", func(t *testing.T) {
+		gifData := imgutils.GenGIFData(65535, 65535, 10)
+
+		us.Id = model.NewId()
+		us.Filename = "test.gif"
+		us.FileSize = int64(len(gifData))
+		var appErr *model.AppError
+		us, appErr = th.App.CreateUploadSession(us)
+		require.Nil(t, appErr)
+		require.NotEmpty(t, us)
+
+		info, appErr := th.App.UploadData(th.Context, us, bytes.NewReader(gifData))
+		require.NotNil(t, appErr)
+		require.Equal(t, "app.upload.upload_data.large_image.app_error", appErr.Id)
+		require.Empty(t, info)
+	})
 }

 func TestUploadDataConcurrent(t *testing.T) {
--- a/i18n/en.json
+++ b/i18n/en.json
@@ -6367,6 +6367,10 @@
    "id": "app.upload.upload_data.first_part_too_small.app_error",
    "translation": "Failed to upload data. First part must be at least {{.Size}} Bytes."
  },
+  {
+    "id": "app.upload.upload_data.gen_info.app_error",
+    "translation": "Failed to generate file info from uploaded data."
+  },
  {
    "id": "app.upload.upload_data.large_image.app_error",
    "translation": "{{.Filename}} dimensions ({{.Width}} by {{.Height}} pixels) exceed the limits."
--- a/model/file_info.go
+++ b/model/file_info.go
@@ -5,12 +5,13 @@ package model

 import (
 	"image"
-	"image/gif"
 	"io"
 	"mime"
 	"net/http"
 	"path/filepath"
 	"strings"
+
+	"github.com/mattermost/mattermost-server/v6/utils/imgutils"
 )

 const (
@@ -156,13 +157,13 @@ func GetInfoForBytes(name string, data io.ReadSeeker, size int) (*FileInfo, *App
 			if info.MimeType == "image/gif" {
 				// Just show the gif itself instead of a preview image for animated gifs
 				data.Seek(0, io.SeekStart)
-				gifConfig, err := gif.DecodeAll(data)
+				frameCount, err := imgutils.CountGIFFrames(data)
 				if err != nil {
 					// Still return the rest of the info even though it doesn't appear to be an actual gif
 					info.HasPreviewImage = true
 					return info, NewAppError("GetInfoForBytes", "model.file_info.get.gif.app_error", nil, err.Error(), http.StatusBadRequest)
 				}
-				info.HasPreviewImage = len(gifConfig.Image) == 1
+				info.HasPreviewImage = frameCount == 1
 			} else {
 				info.HasPreviewImage = true
 			}
--- a/utils/imgutils/gif.go
+++ b/utils/imgutils/gif.go
@@ -12,10 +12,9 @@ package imgutils
 import (
 	"bufio"
 	"compress/lzw"
+	"encoding/binary"
 	"errors"
 	"fmt"
-	"image"
-	"image/color"
 	"io"
 )

@@ -100,7 +99,7 @@ type decoder struct {
 	hasTransparentIndex bool

 	// Computed.
-	globalColorTable color.Palette
+	hasGlobalColorTable bool

 	// Used when decoding.
 	imageCount int
@@ -274,26 +273,22 @@ func (d *decoder) readHeaderAndScreenDescriptor() error {
 	if fields := d.tmp[10]; fields&fColorTable != 0 {
 		d.backgroundIndex = d.tmp[11]
 		// readColorTable overwrites the contents of d.tmp, but that's OK.
-		if d.globalColorTable, err = d.readColorTable(fields); err != nil {
+		if err = d.readColorTable(fields); err != nil {
 			return err
 		}
+		d.hasGlobalColorTable = true
 	}
 	// d.tmp[12] is the Pixel Aspect Ratio, which is ignored.
 	return nil
 }

-func (d *decoder) readColorTable(fields byte) (color.Palette, error) {
+func (d *decoder) readColorTable(fields byte) error {
 	n := 1 << (1 + uint(fields&fColorTableBitsMask))
 	err := readFull(d.r, d.tmp[:3*n])
 	if err != nil {
-		return nil, fmt.Errorf("gif: reading color table: %s", err)
+		return fmt.Errorf("gif: reading color table: %s", err)
 	}
-	j, p := 0, make(color.Palette, n)
-	for i := range p {
-		p[i] = color.RGBA{d.tmp[j+0], d.tmp[j+1], d.tmp[j+2], 0xFF}
-		j += 3
-	}
-	return p, nil
+	return nil
 }

 func (d *decoder) readExtension() error {
@@ -371,41 +366,17 @@ func (d *decoder) readGraphicControl() error {
 }

 func (d *decoder) readImageDescriptor() error {
-	m, err := d.newImageFromDescriptor()
+	err := d.checkImageFromDescriptor()
 	if err != nil {
 		return err
 	}
 	useLocalColorTable := d.imageFields&fColorTable != 0
 	if useLocalColorTable {
-		m.Palette, err = d.readColorTable(d.imageFields)
-		if err != nil {
+		if err = d.readColorTable(d.imageFields); err != nil {
 			return err
 		}
-	} else {
-		if d.globalColorTable == nil {
-			return errors.New("gif: no color table")
-		}
-		m.Palette = d.globalColorTable
-	}
-	if d.hasTransparentIndex {
-		if !useLocalColorTable {
-			// Clone the global color table.
-			m.Palette = append(color.Palette(nil), d.globalColorTable...)
-		}
-		if ti := int(d.transparentIndex); ti < len(m.Palette) {
-			m.Palette[ti] = color.RGBA{}
-		} else {
-			// The transparentIndex is out of range, which is an error
-			// according to the spec, but Firefox and Google Chrome
-			// seem OK with this, so we enlarge the palette with
-			// transparent colors. See golang.org/issue/15059.
-			p := make(color.Palette, ti+1)
-			copy(p, m.Palette)
-			for i := len(m.Palette); i < len(p); i++ {
-				p[i] = color.RGBA{}
-			}
-			m.Palette = p
-		}
+	} else if !d.hasGlobalColorTable {
+		return errors.New("gif: no color table")
 	}
 	litWidth, err := readByte(d.r)
 	if err != nil {
@@ -418,12 +389,14 @@ func (d *decoder) readImageDescriptor() error {
 	br := &blockReader{d: d}
 	lzwr := lzw.NewReader(br, lzw.LSB, int(litWidth))
 	defer lzwr.Close()
-	if err = readFull(lzwr, m.Pix); err != nil {
+
+	if _, err := io.Copy(io.Discard, lzwr); err != nil {
 		if err != io.ErrUnexpectedEOF {
 			return fmt.Errorf("gif: reading image data: %v", err)
 		}
 		return errNotEnough
 	}
+
 	// In theory, both lzwr and br should be exhausted. Reading from them
 	// should yield (0, io.EOF).
 	//
@@ -455,9 +428,9 @@ func (d *decoder) readImageDescriptor() error {
 	return nil
 }

-func (d *decoder) newImageFromDescriptor() (*image.Paletted, error) {
+func (d *decoder) checkImageFromDescriptor() error {
 	if err := readFull(d.r, d.tmp[:9]); err != nil {
-		return nil, fmt.Errorf("gif: can't read image descriptor: %s", err)
+		return fmt.Errorf("gif: can't read image descriptor: %s", err)
 	}
 	left := int(d.tmp[0]) + int(d.tmp[1])<<8
 	top := int(d.tmp[2]) + int(d.tmp[3])<<8
@@ -482,12 +455,10 @@ func (d *decoder) newImageFromDescriptor() (*image.Paletted, error) {
 	// imageBounds.Max (d.width, d.height) and not frameBounds.Min (left, top)
 	// against imageBounds.Min (0, 0).
 	if left+width > d.width || top+height > d.height {
-		return nil, errors.New("gif: frame bounds larger than image bounds")
+		return errors.New("gif: frame bounds larger than image bounds")
 	}
-	return image.NewPaletted(image.Rectangle{
-		Min: image.Point{left, top},
-		Max: image.Point{left + width, top + height},
-	}, nil), nil
+
+	return nil
 }

 func (d *decoder) readBlock() (int, error) {
@@ -501,10 +472,34 @@ func (d *decoder) readBlock() (int, error) {
 	return int(n), nil
 }

-func CountFrames(r io.Reader) (int, error) {
+func CountGIFFrames(r io.Reader) (int, error) {
 	var d decoder
 	if err := d.decode(r, false); err != nil {
 		return -1, err
 	}
 	return d.imageCount, nil
 }
+
+func GenGIFData(width, height uint16, nFrames int) []byte {
+	header := []byte{
+		'G', 'I', 'F', '8', '9', 'a', // header
+		0, 0, 0, 0, // width and height
+		128, 0, 0, // other header information
+		0, 0, 0, 1, 1, 1, // color table
+	}
+	binary.LittleEndian.PutUint16(header[6:], width)
+	binary.LittleEndian.PutUint16(header[8:], height)
+	frame := []byte{
+		0x2c,                   // block introducer
+		0, 0, 0, 0, 1, 0, 1, 0, // position and dimensions of the frame
+		0,                      // other frame information
+		0x2, 0x2, 0x4c, 0x1, 0, // encoded pixel data
+	}
+	trailer := []byte{0x3b}
+	gifData := header
+	for i := 0; i < nFrames; i++ {
+		gifData = append(gifData, frame...)
+	}
+	gifData = append(gifData, trailer...)
+	return gifData
+}
--- a/utils/imgutils/gif_test.go
+++ b/utils/imgutils/gif_test.go
@@ -5,76 +5,84 @@ package imgutils

 import (
 	"bytes"
+	"image"
+	_ "image/gif"
+	"io"
+	"os"
+	"path/filepath"
 	"testing"

+	"github.com/mattermost/mattermost-server/v6/utils/fileutils"
+
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-
-	"github.com/mattermost/mattermost-server/v6/utils/testutils"
 )

-func TestCountFrames(t *testing.T) {
-	header := []byte{
-		'G', 'I', 'F', '8', '9', 'a', // header
-		1, 0, 1, 0, // width and height of 1 by 1
-		128, 0, 0, // other header information
-		0, 0, 0, 1, 1, 1, // color table
+func readTestFile(t *testing.T, name string) ([]byte, error) {
+	t.Helper()
+	path, _ := fileutils.FindDir("tests")
+	file, err := os.Open(filepath.Join(path, name))
+	if err != nil {
+		return nil, err
 	}
-	frame := []byte{
-		0x2c,                   // block introducer
-		0, 0, 0, 0, 1, 0, 1, 0, // position and dimensions of the frame
-		0,                      // other frame information
-		0x2, 0x2, 0x4c, 0x1, 0, // encoded pixel data
-	}
-	trailer := []byte{0x3b}
+	defer file.Close()

+	data := &bytes.Buffer{}
+	if _, err := io.Copy(data, file); err != nil {
+		return nil, err
+	}
+	return data.Bytes(), nil
+}
+
+func TestGenGIFData(t *testing.T) {
+	data := GenGIFData(600, 400, 1)
+	img, format, err := image.DecodeConfig(bytes.NewReader(data))
+	require.NoError(t, err)
+	require.Equal(t, 600, img.Width)
+	require.Equal(t, 400, img.Height)
+	require.Equal(t, "gif", format)
+}
+
+func TestCountGIFFrames(t *testing.T) {
 	t.Run("should count the frames of a static gif", func(t *testing.T) {
-		var b []byte
-		b = append(b, header...)
-		b = append(b, frame...)
-		b = append(b, trailer...)
+		gifData := GenGIFData(400, 400, 1)

-		count, err := CountFrames(bytes.NewReader(b))
+		count, err := CountGIFFrames(bytes.NewReader(gifData))

 		assert.NoError(t, err)
 		assert.Equal(t, 1, count)
 	})

 	t.Run("should count the frames of an animated gif", func(t *testing.T) {
-		var b []byte
-		b = append(b, header...)
-		for i := 0; i < 100; i++ {
-			b = append(b, frame...)
-		}
-		b = append(b, trailer...)
+		gifData := GenGIFData(400, 400, 100)

-		count, err := CountFrames(bytes.NewReader(b))
+		count, err := CountGIFFrames(bytes.NewReader(gifData))

 		assert.NoError(t, err)
 		assert.Equal(t, 100, count)
 	})

 	t.Run("should count the frames of an actual animated gif", func(t *testing.T) {
-		b, err := testutils.ReadTestFile("testgif.gif")
+		b, err := readTestFile(t, "testgif.gif")
 		require.NoError(t, err)

-		count, err := CountFrames(bytes.NewReader(b))
+		count, err := CountGIFFrames(bytes.NewReader(b))

 		assert.NoError(t, err)
 		assert.Equal(t, 4, count)
 	})

 	t.Run("should return an error for a non-gif image", func(t *testing.T) {
-		b, err := testutils.ReadTestFile("test.png")
+		b, err := readTestFile(t, "test.png")
 		require.NoError(t, err)

-		_, err = CountFrames(bytes.NewReader(b))
+		_, err = CountGIFFrames(bytes.NewReader(b))

 		assert.Error(t, err)
 	})

 	t.Run("should return an error for garbage data", func(t *testing.T) {
-		_, err := CountFrames(bytes.NewReader([]byte("garbage data")))
+		_, err := CountGIFFrames(bytes.NewReader([]byte("garbage data")))

 		assert.Error(t, err)
 	})