From 2b6cbbdf3cde07745e5498dcdfe9a360308538cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Wed, 18 Sep 2024 12:14:49 +0200 Subject: [PATCH] write upload session info atomically MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jörn Friedrich Dreyer --- .../write-upload-sessions-atomically.md | 5 +++++ .../utils/decomposedfs/upload/session.go | 20 ++++++++++++++++--- .../utils/decomposedfs/upload/store.go | 9 ++++++++- 3 files changed, 30 insertions(+), 4 deletions(-) create mode 100644 changelog/unreleased/write-upload-sessions-atomically.md diff --git a/changelog/unreleased/write-upload-sessions-atomically.md b/changelog/unreleased/write-upload-sessions-atomically.md new file mode 100644 index 0000000000..35229c9d84 --- /dev/null +++ b/changelog/unreleased/write-upload-sessions-atomically.md @@ -0,0 +1,5 @@ +Bugfix: Write upload session info atomically + +We now use a lock and atomic write on upload session metadata to prevent empty reads. A virus scan event might cause the file to be truncated and then a finished event might try to read the file, just getting an empty string. + +https://github.com/cs3org/reva/pull/4850 diff --git a/pkg/storage/utils/decomposedfs/upload/session.go b/pkg/storage/utils/decomposedfs/upload/session.go index 1c1386d9b4..3c3bef13ef 100644 --- a/pkg/storage/utils/decomposedfs/upload/session.go +++ b/pkg/storage/utils/decomposedfs/upload/session.go @@ -26,6 +26,8 @@ import ( "strconv" "time" + "github.com/google/renameio/v2" + "github.com/rogpeppe/go-internal/lockedfile" tusd "github.com/tus/tusd/v2/pkg/handler" userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" @@ -74,7 +76,13 @@ func (s *OcisSession) executantUser() *userpb.User { // Purge deletes the upload session metadata and written binary data func (s *OcisSession) Purge(ctx context.Context) error { - if err := os.Remove(sessionPath(s.store.root, s.info.ID)); err != nil { + sessionPath := sessionPath(s.store.root, s.info.ID) + f, err := lockedfile.OpenFile(sessionPath+".lock", os.O_RDWR|os.O_TRUNC|os.O_CREATE, 0600) + if err != nil { + return err + } + defer f.Close() + if err := os.Remove(sessionPath); err != nil { return err } if err := os.Remove(s.binPath()); err != nil { @@ -93,6 +101,8 @@ func (s *OcisSession) TouchBin() error { } // Persist writes the upload session metadata to disk +// events can update the scan outcome and the finished event might read an empty file because of race conditions +// so we need to lock the file while writing and use atomic writes func (s *OcisSession) Persist(ctx context.Context) error { uploadPath := sessionPath(s.store.root, s.info.ID) // create folder structure (if needed) @@ -105,8 +115,12 @@ func (s *OcisSession) Persist(ctx context.Context) error { if err != nil { return err } - - return os.WriteFile(uploadPath, d, 0600) + f, err := lockedfile.OpenFile(uploadPath+".lock", os.O_RDWR|os.O_TRUNC|os.O_CREATE, 0600) + if err != nil { + return err + } + defer f.Close() + return renameio.WriteFile(uploadPath, d, 0600) } // ToFileInfo returns tus compatible FileInfo so the tus handler can access the upload offset diff --git a/pkg/storage/utils/decomposedfs/upload/store.go b/pkg/storage/utils/decomposedfs/upload/store.go index 74744e6da9..fa036a7023 100644 --- a/pkg/storage/utils/decomposedfs/upload/store.go +++ b/pkg/storage/utils/decomposedfs/upload/store.go @@ -119,7 +119,7 @@ func (store OcisStore) List(ctx context.Context) ([]*OcisSession, error) { // Get returns the upload session for the given upload id func (store OcisStore) Get(ctx context.Context, id string) (*OcisSession, error) { - sessionPath := filepath.Join(store.root, "uploads", id+".info") + sessionPath := sessionPath(store.root, id) match := _idRegexp.FindStringSubmatch(sessionPath) if match == nil || len(match) < 2 { return nil, fmt.Errorf("invalid upload path") @@ -129,6 +129,11 @@ func (store OcisStore) Get(ctx context.Context, id string) (*OcisSession, error) store: store, info: tusd.FileInfo{}, } + lock, err := lockedfile.Open(sessionPath) + if err != nil { + return nil, err + } + defer lock.Close() data, err := os.ReadFile(sessionPath) if err != nil { if errors.Is(err, iofs.ErrNotExist) { @@ -137,6 +142,8 @@ func (store OcisStore) Get(ctx context.Context, id string) (*OcisSession, error) } return nil, err } + lock.Close() // release lock asap + if err := json.Unmarshal(data, &session.info); err != nil { return nil, err }