From 56f012ceb6afa978274c2980e466e55bf0d2546d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Pablo=20Villaf=C3=A1=C3=B1ez?= Date: Fri, 29 Nov 2024 14:16:07 +0100 Subject: [PATCH 1/3] fix: use a tmp file and rename to prevent a possible race condition --- .../pkg/thumbnail/storage/filesystem.go | 25 +++++++++++++++---- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/services/thumbnails/pkg/thumbnail/storage/filesystem.go b/services/thumbnails/pkg/thumbnail/storage/filesystem.go index 0188fc8b438..621b2194fb7 100644 --- a/services/thumbnails/pkg/thumbnail/storage/filesystem.go +++ b/services/thumbnails/pkg/thumbnail/storage/filesystem.go @@ -62,14 +62,29 @@ func (s FileSystem) Put(key string, img []byte) error { } if _, err := os.Stat(imgPath); os.IsNotExist(err) { - f, err := os.Create(imgPath) + f, err := os.CreateTemp(dir, "tmpthumb") if err != nil { - return errors.Wrapf(err, "could not create file \"%s\"", key) + return errors.Wrapf(err, "could not create temporary file for \"%s\"", key) } - defer f.Close() - if _, err = f.Write(img); err != nil { - return errors.Wrapf(err, "could not write to file \"%s\"", key) + _, writeErr := f.Write(img) // write the thumbnail in the temporary file + f.Close() // close the file regardless of the error + + // if there was a problem writing, remove the temporary file + if writeErr != nil { + if remErr := os.Remove(f.Name()); remErr != nil { + return errors.Wrapf(remErr, "could not cleanup temporary file for \"%s\"", key) + } + return errors.Wrapf(writeErr, "could not write to temporary file for \"%s\"", key) + } + + // rename the temporary file to the final file + if renErr := os.Rename(f.Name(), imgPath); renErr != nil { + // if we couldn't rename, remove the temporary file + if remErr := os.Remove(f.Name()); remErr != nil { + return errors.Wrapf(remErr, "rename failed and could not cleanup temporary file for \"%s\"", key) + } + return errors.Wrapf(err, "could not rename temporary file to \"%s\"", key) } } From ce3d9c13f560bb92a72c5652b56161e51c1eb1dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Pablo=20Villaf=C3=A1=C3=B1ez?= Date: Fri, 29 Nov 2024 15:27:00 +0100 Subject: [PATCH 2/3] chore: add changelog entry --- changelog/unreleased/thumbnail-create-and-rename.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 changelog/unreleased/thumbnail-create-and-rename.md diff --git a/changelog/unreleased/thumbnail-create-and-rename.md b/changelog/unreleased/thumbnail-create-and-rename.md new file mode 100644 index 00000000000..f349b49c3ab --- /dev/null +++ b/changelog/unreleased/thumbnail-create-and-rename.md @@ -0,0 +1,8 @@ +Bugfix: Fix possible race condition when a thumbnails is stored in the FS + +A race condition could cause the thumbnail service to return a thumbnail +with 0 bytes or with partial content. In order to fix this, the service will +create a temporary file with the contents and then rename that file to its +final location. + +https://github.com/owncloud/ocis/pull/10693 From f3d91f8c69cc0006ee64c355ac9392d709d4ac6e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Pablo=20Villaf=C3=A1=C3=B1ez?= Date: Tue, 3 Dec 2024 10:52:05 +0100 Subject: [PATCH 3/3] fix: ensure thumbnail is written into disk --- .../thumbnails/pkg/thumbnail/storage/filesystem.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/services/thumbnails/pkg/thumbnail/storage/filesystem.go b/services/thumbnails/pkg/thumbnail/storage/filesystem.go index 621b2194fb7..8f2f561fb0e 100644 --- a/services/thumbnails/pkg/thumbnail/storage/filesystem.go +++ b/services/thumbnails/pkg/thumbnail/storage/filesystem.go @@ -66,25 +66,28 @@ func (s FileSystem) Put(key string, img []byte) error { if err != nil { return errors.Wrapf(err, "could not create temporary file for \"%s\"", key) } - - _, writeErr := f.Write(img) // write the thumbnail in the temporary file - f.Close() // close the file regardless of the error + defer f.Close() // if there was a problem writing, remove the temporary file - if writeErr != nil { + if _, writeErr := f.Write(img); writeErr != nil { if remErr := os.Remove(f.Name()); remErr != nil { return errors.Wrapf(remErr, "could not cleanup temporary file for \"%s\"", key) } return errors.Wrapf(writeErr, "could not write to temporary file for \"%s\"", key) } + // if there wasn't a problem, ensure the data is written into disk + if synErr := f.Sync(); synErr != nil { + return errors.Wrapf(synErr, "could not sync temporary file data into the disk for \"%s\"", key) + } + // rename the temporary file to the final file if renErr := os.Rename(f.Name(), imgPath); renErr != nil { // if we couldn't rename, remove the temporary file if remErr := os.Remove(f.Name()); remErr != nil { return errors.Wrapf(remErr, "rename failed and could not cleanup temporary file for \"%s\"", key) } - return errors.Wrapf(err, "could not rename temporary file to \"%s\"", key) + return errors.Wrapf(renErr, "could not rename temporary file to \"%s\"", key) } }