From 799ec680bdc0f490b85f8e863798f0cbef7d89cc Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Tue, 19 Dec 2023 10:00:01 +0100 Subject: [PATCH] [MM-29240] Improve buffer handling of UploadEmojiImage (#25536) --- server/channels/app/app_iface.go | 1 - server/channels/app/emoji.go | 100 ++++++------ server/channels/app/emoji_bench_test.go | 144 ++++++++++++++++++ .../app/opentracing/opentracing_layer.go | 22 --- server/channels/utils/emoji.go | 4 +- server/i18n/en.json | 4 + 6 files changed, 196 insertions(+), 79 deletions(-) create mode 100644 server/channels/app/emoji_bench_test.go diff --git a/server/channels/app/app_iface.go b/server/channels/app/app_iface.go index cb945dcb4ac..1e019364cd2 100644 --- a/server/channels/app/app_iface.go +++ b/server/channels/app/app_iface.go @@ -1163,7 +1163,6 @@ type AppIface interface { UpdateUserRoles(c request.CTX, userID string, newRoles string, sendWebSocketEvent bool) (*model.User, *model.AppError) UpdateUserRolesWithUser(c request.CTX, user *model.User, newRoles string, sendWebSocketEvent bool) (*model.User, *model.AppError) UploadData(c request.CTX, us *model.UploadSession, rd io.Reader) (*model.FileInfo, *model.AppError) - UploadEmojiImage(c request.CTX, id string, imageData *multipart.FileHeader) *model.AppError UploadFileForUserAndTeam(c request.CTX, data []byte, channelID string, filename string, rawUserId string, rawTeamId string) (*model.FileInfo, *model.AppError) UpsertDraft(c request.CTX, draft *model.Draft, connectionID string) (*model.Draft, *model.AppError) UpsertGroupMember(groupID string, userID string) (*model.GroupMember, *model.AppError) diff --git a/server/channels/app/emoji.go b/server/channels/app/emoji.go index 10123be66f0..3ddb566eb04 100644 --- a/server/channels/app/emoji.go +++ b/server/channels/app/emoji.go @@ -38,11 +38,11 @@ const ( func (a *App) CreateEmoji(c request.CTX, sessionUserId string, emoji *model.Emoji, multiPartImageData *multipart.Form) (*model.Emoji, *model.AppError) { if !*a.Config().ServiceSettings.EnableCustomEmoji { - return nil, model.NewAppError("UploadEmojiImage", "api.emoji.disabled.app_error", nil, "", http.StatusForbidden) + return nil, model.NewAppError("CreateEmoji", "api.emoji.disabled.app_error", nil, "", http.StatusForbidden) } if *a.Config().FileSettings.DriverName == "" { - return nil, model.NewAppError("GetEmoji", "api.emoji.storage.app_error", nil, "", http.StatusForbidden) + return nil, model.NewAppError("CreateEmoji", "api.emoji.storage.app_error", nil, "", http.StatusForbidden) } // wipe the emoji id so that existing emojis can't get overwritten @@ -56,11 +56,11 @@ func (a *App) CreateEmoji(c request.CTX, sessionUserId string, emoji *model.Emoj } if emoji.CreatorId != sessionUserId { - return nil, model.NewAppError("createEmoji", "api.emoji.create.other_user.app_error", nil, "", http.StatusForbidden) + return nil, model.NewAppError("CreateEmoji", "api.emoji.create.other_user.app_error", nil, "", http.StatusForbidden) } if existingEmoji, err := a.Srv().Store().Emoji().GetByName(c, emoji.Name, true); err == nil && existingEmoji != nil { - return nil, model.NewAppError("createEmoji", "api.emoji.create.duplicate.app_error", nil, "", http.StatusBadRequest).Wrap(err) + return nil, model.NewAppError("CreateEmoji", "api.emoji.create.duplicate.app_error", nil, "", http.StatusBadRequest).Wrap(err) } imageData := multiPartImageData.File["image"] @@ -68,11 +68,18 @@ func (a *App) CreateEmoji(c request.CTX, sessionUserId string, emoji *model.Emoj return nil, model.NewAppError("Context", "api.context.invalid_body_param.app_error", map[string]any{"Name": "createEmoji"}, "", http.StatusBadRequest) } - if appErr := a.UploadEmojiImage(c, emoji.Id, imageData[0]); appErr != nil { + filename := imageData[0].Filename + file, err := imageData[0].Open() + if err != nil { + return nil, model.NewAppError("CreateEmoji", "api.emoji.upload.open.app_error", nil, "", http.StatusBadRequest).Wrap(err) + } + defer file.Close() + + if appErr := a.uploadEmojiImage(c, emoji.Id, filename, file); appErr != nil { return nil, appErr } - emoji, err := a.Srv().Store().Emoji().Save(emoji) + emoji, err = a.Srv().Store().Emoji().Save(emoji) if err != nil { return nil, model.NewAppError("CreateEmoji", "app.emoji.create.internal_error", nil, "", http.StatusInternalServerError).Wrap(err) } @@ -96,26 +103,9 @@ func (a *App) GetEmojiList(c request.CTX, page, perPage int, sort string) ([]*mo return list, nil } -func (a *App) UploadEmojiImage(c request.CTX, id string, imageData *multipart.FileHeader) *model.AppError { - if !*a.Config().ServiceSettings.EnableCustomEmoji { - return model.NewAppError("UploadEmojiImage", "api.emoji.disabled.app_error", nil, "", http.StatusForbidden) - } - - if *a.Config().FileSettings.DriverName == "" { - return model.NewAppError("UploadEmojiImage", "api.emoji.storage.app_error", nil, "", http.StatusForbidden) - } - - file, err := imageData.Open() - if err != nil { - return model.NewAppError("uploadEmojiImage", "api.emoji.upload.open.app_error", nil, "", http.StatusBadRequest).Wrap(err) - } - defer file.Close() - - buf := bytes.NewBuffer(nil) - io.Copy(buf, file) - +func (a *App) uploadEmojiImage(c request.CTX, id string, filename string, file io.ReadSeeker) *model.AppError { // make sure the file is an image and is within the required dimensions - config, _, err := image.DecodeConfig(bytes.NewReader(buf.Bytes())) + config, _, err := image.DecodeConfig(file) if err != nil { return model.NewAppError("uploadEmojiImage", "api.emoji.upload.image.app_error", nil, "", http.StatusBadRequest).Wrap(err) } @@ -127,37 +117,40 @@ func (a *App) UploadEmojiImage(c request.CTX, id string, imageData *multipart.Fi }, "", http.StatusBadRequest) } - if config.Width > MaxEmojiWidth || config.Height > MaxEmojiHeight { - data := buf.Bytes() - newbuf := bytes.NewBuffer(nil) - info, err := getInfoForBytes(imageData.Filename, bytes.NewReader(data), len(data)) + _, err = file.Seek(0, io.SeekStart) + if err != nil { + return model.NewAppError("uploadEmojiImage", "api.emoji.upload.seek.app_error", nil, "", http.StatusInternalServerError).Wrap(err) + } + + if config.Width <= MaxEmojiWidth && config.Height <= MaxEmojiHeight { + // No need to resize the image + _, appErr := a.WriteFile(file, getEmojiImagePath(id)) + return appErr + } + + // Create a buffer for the resized image + buf := &bytes.Buffer{} + + info := model.NewInfo(filename) + if info.MimeType == "image/gif" { + g, err := gif.DecodeAll(file) if err != nil { - return err + return model.NewAppError("uploadEmojiImage", "api.emoji.upload.large_image.gif_decode_error", nil, "", http.StatusBadRequest).Wrap(err) } - if info.MimeType == "image/gif" { - gif_data, err := gif.DecodeAll(bytes.NewReader(data)) - if err != nil { - return model.NewAppError("uploadEmojiImage", "api.emoji.upload.large_image.gif_decode_error", nil, "", http.StatusBadRequest).Wrap(err) - } + resizeEmojiGif(g) + if err := gif.EncodeAll(buf, g); err != nil { + return model.NewAppError("uploadEmojiImage", "api.emoji.upload.large_image.gif_encode_error", nil, "", http.StatusBadRequest).Wrap(err) + } + } else { + img, _, err := image.Decode(file) + if err != nil { + return model.NewAppError("uploadEmojiImage", "api.emoji.upload.large_image.decode_error", nil, "", http.StatusBadRequest).Wrap(err) + } - resized_gif := resizeEmojiGif(gif_data) - if err := gif.EncodeAll(newbuf, resized_gif); err != nil { - return model.NewAppError("uploadEmojiImage", "api.emoji.upload.large_image.gif_encode_error", nil, "", http.StatusBadRequest).Wrap(err) - } - - buf = newbuf - } else { - img, _, err := image.Decode(bytes.NewReader(data)) - if err != nil { - return model.NewAppError("uploadEmojiImage", "api.emoji.upload.large_image.decode_error", nil, "", http.StatusBadRequest).Wrap(err) - } - - resizedImg := resizeEmoji(img, config.Width, config.Height) - if err := a.ch.imgEncoder.EncodePNG(newbuf, resizedImg); err != nil { - return model.NewAppError("uploadEmojiImage", "api.emoji.upload.large_image.encode_error", nil, "", http.StatusBadRequest).Wrap(err) - } - buf = newbuf + resizedImg := resizeEmoji(img, config.Width, config.Height) + if err := a.ch.imgEncoder.EncodePNG(buf, resizedImg); err != nil { + return model.NewAppError("uploadEmojiImage", "api.emoji.upload.large_image.encode_error", nil, "", http.StatusBadRequest).Wrap(err) } } @@ -314,7 +307,7 @@ func (a *App) GetEmojiStaticURL(c request.CTX, emojiName string) (string, *model } } -func resizeEmojiGif(gifImg *gif.GIF) *gif.GIF { +func resizeEmojiGif(gifImg *gif.GIF) { // Create a new RGBA image to hold the incremental frames. firstFrame := gifImg.Image[0].Bounds() b := image.Rect(0, 0, firstFrame.Dx(), firstFrame.Dy()) @@ -331,7 +324,6 @@ func resizeEmojiGif(gifImg *gif.GIF) *gif.GIF { // Set new gif width and height gifImg.Config.Width = resizedImage.Bounds().Dx() gifImg.Config.Height = resizedImage.Bounds().Dy() - return gifImg } func getEmojiImagePath(id string) string { diff --git a/server/channels/app/emoji_bench_test.go b/server/channels/app/emoji_bench_test.go new file mode 100644 index 00000000000..4763acdfc67 --- /dev/null +++ b/server/channels/app/emoji_bench_test.go @@ -0,0 +1,144 @@ +// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +package app + +import ( + "strings" + "testing" + + "github.com/mattermost/mattermost/server/public/model" + "github.com/mattermost/mattermost/server/public/shared/request" + "github.com/mattermost/mattermost/server/v8/channels/utils" + "github.com/stretchr/testify/require" +) + +func BenchmarkUploadEmojiImage(b *testing.B) { + th := Setup(b) + b.Cleanup(func() { + b.StopTimer() + th.TearDown() + }) + + rctx := request.TestContext(b) + + b.Run("gif", func(b *testing.B) { + filename := "image.gif" + b.Run("small", func(b *testing.B) { + file := strings.NewReader(string(utils.CreateTestGif(b, 10, 10))) + b.ResetTimer() + for i := 0; i < b.N; i++ { + id := model.NewId() + appErr := th.App.uploadEmojiImage(rctx, id, filename, file) + require.Nil(b, appErr) + _, err := file.Seek(0, 0) + require.NoError(b, err) + } + }) + b.Run("max size", func(b *testing.B) { + file := strings.NewReader(string(utils.CreateTestGif(b, MaxEmojiWidth, MaxEmojiHeight))) + b.ResetTimer() + for i := 0; i < b.N; i++ { + id := model.NewId() + appErr := th.App.uploadEmojiImage(rctx, id, filename, file) + require.Nil(b, appErr) + _, err := file.Seek(0, 0) + require.NoError(b, err) + } + }) + b.Run("too wide", func(b *testing.B) { + file := strings.NewReader(string(utils.CreateTestGif(b, MaxEmojiOriginalWidth, MaxEmojiHeight))) + b.ResetTimer() + for i := 0; i < b.N; i++ { + id := model.NewId() + appErr := th.App.uploadEmojiImage(rctx, id, filename, file) + require.Nil(b, appErr) + _, err := file.Seek(0, 0) + require.NoError(b, err) + } + }) + b.Run("too tall", func(b *testing.B) { + file := strings.NewReader(string(utils.CreateTestGif(b, MaxEmojiWidth, MaxEmojiOriginalWidth))) + b.ResetTimer() + for i := 0; i < b.N; i++ { + id := model.NewId() + appErr := th.App.uploadEmojiImage(rctx, id, filename, file) + require.Nil(b, appErr) + _, err := file.Seek(0, 0) + require.NoError(b, err) + } + }) + b.Run("too tall and too wide", func(b *testing.B) { + file := strings.NewReader(string(utils.CreateTestGif(b, MaxEmojiOriginalWidth, MaxEmojiOriginalWidth))) + b.ResetTimer() + for i := 0; i < b.N; i++ { + id := model.NewId() + appErr := th.App.uploadEmojiImage(rctx, id, filename, file) + require.Nil(b, appErr) + _, err := file.Seek(0, 0) + require.NoError(b, err) + } + }) + }) + + b.Run("png", func(b *testing.B) { + filename := "image.png" + + b.Run("small", func(b *testing.B) { + file := strings.NewReader(string(utils.CreateTestPng(b, 10, 10))) + b.ResetTimer() + for i := 0; i < b.N; i++ { + id := model.NewId() + appErr := th.App.uploadEmojiImage(rctx, id, filename, file) + require.Nil(b, appErr) + _, err := file.Seek(0, 0) + require.NoError(b, err) + } + }) + + b.Run("max size", func(b *testing.B) { + file := strings.NewReader(string(utils.CreateTestPng(b, MaxEmojiWidth, MaxEmojiHeight))) + b.ResetTimer() + for i := 0; i < b.N; i++ { + id := model.NewId() + appErr := th.App.uploadEmojiImage(rctx, id, filename, file) + require.Nil(b, appErr) + _, err := file.Seek(0, 0) + require.NoError(b, err) + } + }) + b.Run("too wide", func(b *testing.B) { + file := strings.NewReader(string(utils.CreateTestPng(b, MaxEmojiOriginalWidth, MaxEmojiHeight))) + b.ResetTimer() + for i := 0; i < b.N; i++ { + id := model.NewId() + appErr := th.App.uploadEmojiImage(rctx, id, filename, file) + require.Nil(b, appErr) + _, err := file.Seek(0, 0) + require.NoError(b, err) + } + }) + b.Run("too tall", func(b *testing.B) { + file := strings.NewReader(string(utils.CreateTestPng(b, MaxEmojiWidth, MaxEmojiOriginalWidth))) + b.ResetTimer() + for i := 0; i < b.N; i++ { + id := model.NewId() + appErr := th.App.uploadEmojiImage(rctx, id, filename, file) + require.Nil(b, appErr) + _, err := file.Seek(0, 0) + require.NoError(b, err) + } + }) + b.Run("too tall and too wide", func(b *testing.B) { + file := strings.NewReader(string(utils.CreateTestPng(b, MaxEmojiOriginalWidth, MaxEmojiOriginalWidth))) + b.ResetTimer() + for i := 0; i < b.N; i++ { + id := model.NewId() + appErr := th.App.uploadEmojiImage(rctx, id, filename, file) + require.Nil(b, appErr) + _, err := file.Seek(0, 0) + require.NoError(b, err) + } + }) + }) +} diff --git a/server/channels/app/opentracing/opentracing_layer.go b/server/channels/app/opentracing/opentracing_layer.go index 2f5f88efcbc..98bf87fc4f9 100644 --- a/server/channels/app/opentracing/opentracing_layer.go +++ b/server/channels/app/opentracing/opentracing_layer.go @@ -18336,28 +18336,6 @@ func (a *OpenTracingAppLayer) UploadData(c request.CTX, us *model.UploadSession, return resultVar0, resultVar1 } -func (a *OpenTracingAppLayer) UploadEmojiImage(c request.CTX, id string, imageData *multipart.FileHeader) *model.AppError { - origCtx := a.ctx - span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.UploadEmojiImage") - - a.ctx = newCtx - a.app.Srv().Store().SetContext(newCtx) - defer func() { - a.app.Srv().Store().SetContext(origCtx) - a.ctx = origCtx - }() - - defer span.Finish() - resultVar0 := a.app.UploadEmojiImage(c, id, imageData) - - if resultVar0 != nil { - span.LogFields(spanlog.Error(resultVar0)) - ext.Error.Set(span, true) - } - - return resultVar0 -} - func (a *OpenTracingAppLayer) UploadFile(c request.CTX, data []byte, channelID string, filename string) (*model.FileInfo, *model.AppError) { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.UploadFile") diff --git a/server/channels/utils/emoji.go b/server/channels/utils/emoji.go index bfad8ee1f4b..82ef927bf83 100644 --- a/server/channels/utils/emoji.go +++ b/server/channels/utils/emoji.go @@ -15,7 +15,7 @@ import ( "github.com/stretchr/testify/require" ) -func CreateTestGif(t *testing.T, width int, height int) []byte { +func CreateTestGif(t testing.TB, width int, height int) []byte { var buffer bytes.Buffer err := gif.Encode(&buffer, image.NewRGBA(image.Rect(0, 0, width, height)), nil) @@ -50,7 +50,7 @@ func CreateTestJpeg(t *testing.T, width int, height int) []byte { return buffer.Bytes() } -func CreateTestPng(t *testing.T, width int, height int) []byte { +func CreateTestPng(t testing.TB, width int, height int) []byte { var buffer bytes.Buffer err := png.Encode(&buffer, image.NewRGBA(image.Rect(0, 0, width, height))) diff --git a/server/i18n/en.json b/server/i18n/en.json index 19255506c46..b74c53e462e 100644 --- a/server/i18n/en.json +++ b/server/i18n/en.json @@ -1852,6 +1852,10 @@ "id": "api.emoji.upload.open.app_error", "translation": "Unable to create the emoji. An error occurred when trying to open the attached image." }, + { + "id": "api.emoji.upload.seek.app_error", + "translation": "Unable to seek to file start." + }, { "id": "api.error_get_first_admin_complete_setup", "translation": "Error trying to retrieve first admin complete setup from the store."