From 76583344c05ac4e2ee52fabbabcc22cc94d9b74f Mon Sep 17 00:00:00 2001 From: Claudio Costa Date: Mon, 11 Jul 2022 09:34:49 +0200 Subject: [PATCH] [MM-45504] Improve GIF preprocessing logic (#20595) * Improve GIF preprocessing logic * Prefer io to ioutil * Avoid color palette allocation --- app/post_metadata.go | 2 +- app/upload.go | 25 ++++++++-- app/upload_test.go | 18 ++++++++ i18n/en.json | 4 ++ model/file_info.go | 7 +-- utils/imgutils/gif.go | 93 ++++++++++++++++++-------------------- utils/imgutils/gif_test.go | 72 ++++++++++++++++------------- 7 files changed, 133 insertions(+), 88 deletions(-) diff --git a/app/post_metadata.go b/app/post_metadata.go index b7cde0355c..ee552e3998 100644 --- 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 } diff --git a/app/upload.go b/app/upload.go index 93ff614619..d84829cef5 100644 --- 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 diff --git a/app/upload_test.go b/app/upload_test.go index ffec8c803b..be4482b419 100644 --- 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) { diff --git a/i18n/en.json b/i18n/en.json index 5e25b80e9e..e9be9e4661 100644 --- 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." diff --git a/model/file_info.go b/model/file_info.go index 366b544676..723634b6fc 100644 --- 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 } diff --git a/utils/imgutils/gif.go b/utils/imgutils/gif.go index 38ce0cfdae..44c70590d0 100644 --- 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 +} diff --git a/utils/imgutils/gif_test.go b/utils/imgutils/gif_test.go index ed65a16f80..cef6738d1b 100644 --- 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) })