From 6d52522b162a49d38d53e3adbe89e300e71717f1 Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Fri, 1 Apr 2022 11:59:32 +0200 Subject: [PATCH 1/3] return precondition failed if already locked (decomposed fs) --- changelog/unreleased/set-lock-precondition.md | 6 ++++++ pkg/storage/utils/decomposedfs/node/locks.go | 21 +++++++------------ 2 files changed, 13 insertions(+), 14 deletions(-) create mode 100644 changelog/unreleased/set-lock-precondition.md diff --git a/changelog/unreleased/set-lock-precondition.md b/changelog/unreleased/set-lock-precondition.md new file mode 100644 index 0000000000..7c93740ced --- /dev/null +++ b/changelog/unreleased/set-lock-precondition.md @@ -0,0 +1,6 @@ +Bugfix: Decomposed FS: return precondition failed if already locked + +We've fixed the return code from permission denied to precondition failed if a +user tries to lock an already locked file. + +https://github.com/cs3org/reva/pull/323 diff --git a/pkg/storage/utils/decomposedfs/node/locks.go b/pkg/storage/utils/decomposedfs/node/locks.go index 455c4eb8d0..018bb35c0d 100644 --- a/pkg/storage/utils/decomposedfs/node/locks.go +++ b/pkg/storage/utils/decomposedfs/node/locks.go @@ -39,26 +39,14 @@ import ( // SetLock sets a lock on the node func (n *Node) SetLock(ctx context.Context, lock *provider.Lock) error { lockFilePath := n.LockFilePath() - // check existing lock - - if l, _ := n.ReadLock(ctx); l != nil { - lockID, _ := ctxpkg.ContextGetLockID(ctx) - if l.LockId != lockID { - return errtypes.Locked(l.LockId) - } - - err := os.Remove(lockFilePath) - if err != nil { - return err - } - } // ensure parent path exists if err := os.MkdirAll(filepath.Dir(lockFilePath), 0700); err != nil { return errors.Wrap(err, "Decomposedfs: error creating parent folder for lock") } - fileLock, err := filelocks.AcquireWriteLock(n.InternalPath()) + // get file lock, so that nobody can create the lock in the meantime + fileLock, err := filelocks.AcquireWriteLock(n.InternalPath()) if err != nil { return err } @@ -72,6 +60,11 @@ func (n *Node) SetLock(ctx context.Context, lock *provider.Lock) error { } }() + // check if already locked + if l, _ := n.ReadLock(ctx); l != nil { + return errtypes.PreconditionFailed("already locked") + } + // O_EXCL to make open fail when the file already exists f, err := os.OpenFile(lockFilePath, os.O_EXCL|os.O_CREATE|os.O_WRONLY, 0600) if err != nil { From 3ed35a881a65f7c3d017a685c408bab65214bcdb Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Fri, 1 Apr 2022 13:09:39 +0200 Subject: [PATCH 2/3] ensure right error code when already locked --- changelog/unreleased/set-lock-precondition.md | 2 +- .../utils/decomposedfs/decomposedfs.go | 2 +- pkg/storage/utils/decomposedfs/node/locks.go | 42 ++++++++++++------- .../utils/decomposedfs/node/locks_test.go | 42 +++++-------------- 4 files changed, 40 insertions(+), 48 deletions(-) diff --git a/changelog/unreleased/set-lock-precondition.md b/changelog/unreleased/set-lock-precondition.md index 7c93740ced..467f5638dd 100644 --- a/changelog/unreleased/set-lock-precondition.md +++ b/changelog/unreleased/set-lock-precondition.md @@ -3,4 +3,4 @@ Bugfix: Decomposed FS: return precondition failed if already locked We've fixed the return code from permission denied to precondition failed if a user tries to lock an already locked file. -https://github.com/cs3org/reva/pull/323 +https://github.com/cs3org/reva/pull/2709 diff --git a/pkg/storage/utils/decomposedfs/decomposedfs.go b/pkg/storage/utils/decomposedfs/decomposedfs.go index 71dbbaf966..7dbffac201 100644 --- a/pkg/storage/utils/decomposedfs/decomposedfs.go +++ b/pkg/storage/utils/decomposedfs/decomposedfs.go @@ -596,7 +596,7 @@ func (fs *Decomposedfs) GetLock(ctx context.Context, ref *provider.Reference) (* case !ok: return nil, errtypes.PermissionDenied(filepath.Join(node.ParentID, node.Name)) } - return node.ReadLock(ctx) + return node.ReadLock(ctx, false) } // SetLock puts a lock on the given reference diff --git a/pkg/storage/utils/decomposedfs/node/locks.go b/pkg/storage/utils/decomposedfs/node/locks.go index 018bb35c0d..1bbeb9e7bc 100644 --- a/pkg/storage/utils/decomposedfs/node/locks.go +++ b/pkg/storage/utils/decomposedfs/node/locks.go @@ -61,8 +61,16 @@ func (n *Node) SetLock(ctx context.Context, lock *provider.Lock) error { }() // check if already locked - if l, _ := n.ReadLock(ctx); l != nil { - return errtypes.PreconditionFailed("already locked") + l, err := n.ReadLock(ctx, true) // we already have a write file lock, so ReadLock() would fail to acquire a read file lock -> skip it + switch err.(type) { + case errtypes.NotFound: + // file not locked, continue + case nil: + if l != nil { + return errtypes.PreconditionFailed("already locked") + } + default: + return errors.Wrap(err, "Decomposedfs: could check if file already is locked") } // O_EXCL to make open fail when the file already exists @@ -80,26 +88,30 @@ func (n *Node) SetLock(ctx context.Context, lock *provider.Lock) error { } // ReadLock reads the lock id for a node -func (n Node) ReadLock(ctx context.Context) (*provider.Lock, error) { +func (n Node) ReadLock(ctx context.Context, skipFileLock bool) (*provider.Lock, error) { // ensure parent path exists if err := os.MkdirAll(filepath.Dir(n.InternalPath()), 0700); err != nil { return nil, errors.Wrap(err, "Decomposedfs: error creating parent folder for lock") } - fileLock, err := filelocks.AcquireReadLock(n.InternalPath()) - if err != nil { - return nil, err - } - - defer func() { - rerr := filelocks.ReleaseLock(fileLock) + // the caller of ReadLock already may hold a file lock + if !skipFileLock { + fileLock, err := filelocks.AcquireReadLock(n.InternalPath()) - // if err is non nil we do not overwrite that - if err == nil { - err = rerr + if err != nil { + return nil, err } - }() + + defer func() { + rerr := filelocks.ReleaseLock(fileLock) + + // if err is non nil we do not overwrite that + if err == nil { + err = rerr + } + }() + } f, err := os.Open(n.LockFilePath()) if err != nil { @@ -234,7 +246,7 @@ func (n *Node) Unlock(ctx context.Context, lock *provider.Lock) error { // CheckLock compares the context lock with the node lock func (n *Node) CheckLock(ctx context.Context) error { lockID, _ := ctxpkg.ContextGetLockID(ctx) - lock, _ := n.ReadLock(ctx) + lock, _ := n.ReadLock(ctx, false) if lock != nil { switch lockID { case "": diff --git a/pkg/storage/utils/decomposedfs/node/locks_test.go b/pkg/storage/utils/decomposedfs/node/locks_test.go index 3e785a6367..250d8ff036 100644 --- a/pkg/storage/utils/decomposedfs/node/locks_test.go +++ b/pkg/storage/utils/decomposedfs/node/locks_test.go @@ -29,6 +29,7 @@ import ( userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" ctxpkg "github.com/cs3org/reva/v2/pkg/ctx" + "github.com/cs3org/reva/v2/pkg/errtypes" "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/node" helpers "github.com/cs3org/reva/v2/pkg/storage/utils/decomposedfs/testhelpers" ) @@ -102,25 +103,14 @@ var _ = Describe("Node locks", func() { Expect(err).ToNot(HaveOccurred()) }) - It("refuses to lock if already locked an existing lock was not provided", func() { + It("refuses to set a lock if already locked", func() { err := n.SetLock(env.Ctx, lockByUser) Expect(err).ToNot(HaveOccurred()) err = n.SetLock(env.Ctx, lockByUser) Expect(err).To(HaveOccurred()) - - env.Ctx = ctxpkg.ContextSetLockID(env.Ctx, wrongLockByUser.LockId) - err = n.SetLock(env.Ctx, lockByUser) - Expect(err).To(HaveOccurred()) - }) - - It("relocks if the existing lock was provided", func() { - err := n.SetLock(env.Ctx, lockByUser) - Expect(err).ToNot(HaveOccurred()) - - env.Ctx = ctxpkg.ContextSetLockID(env.Ctx, lockByUser.LockId) - err = n.SetLock(env.Ctx, lockByUser) - Expect(err).ToNot(HaveOccurred()) + _, ok := err.(errtypes.PreconditionFailed) + Expect(ok).To(BeTrue()) }) }) @@ -136,26 +126,16 @@ var _ = Describe("Node locks", func() { Expect(err).ToNot(HaveOccurred()) }) - It("refuses to lock if already locked an existing lock was not provided", func() { + It("refuses to set a lock if already locked", func() { err := n.SetLock(env.Ctx, lockByApp) Expect(err).ToNot(HaveOccurred()) err = n.SetLock(env.Ctx, lockByApp) Expect(err).To(HaveOccurred()) - - env.Ctx = ctxpkg.ContextSetLockID(env.Ctx, wrongLockByApp.LockId) - err = n.SetLock(env.Ctx, lockByApp) - Expect(err).To(HaveOccurred()) + _, ok := err.(errtypes.PreconditionFailed) + Expect(ok).To(BeTrue()) }) - It("relocks if the existing lock was provided", func() { - err := n.SetLock(env.Ctx, lockByApp) - Expect(err).ToNot(HaveOccurred()) - - env.Ctx = ctxpkg.ContextSetLockID(env.Ctx, lockByApp.LockId) - err = n.SetLock(env.Ctx, lockByApp) - Expect(err).ToNot(HaveOccurred()) - }) }) Context("with an existing lock for a user", func() { @@ -166,13 +146,13 @@ var _ = Describe("Node locks", func() { Describe("ReadLock", func() { It("returns the lock", func() { - l, err := n.ReadLock(env.Ctx) + l, err := n.ReadLock(env.Ctx, false) Expect(err).ToNot(HaveOccurred()) Expect(l).To(Equal(lockByUser)) }) It("reports an error when the node wasn't locked", func() { - _, err := n2.ReadLock(env.Ctx) + _, err := n2.ReadLock(env.Ctx, false) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("no lock found")) }) @@ -270,13 +250,13 @@ var _ = Describe("Node locks", func() { Describe("ReadLock", func() { It("returns the lock", func() { - l, err := n.ReadLock(env.Ctx) + l, err := n.ReadLock(env.Ctx, false) Expect(err).ToNot(HaveOccurred()) Expect(l).To(Equal(lockByApp)) }) It("reports an error when the node wasn't locked", func() { - _, err := n2.ReadLock(env.Ctx) + _, err := n2.ReadLock(env.Ctx, false) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("no lock found")) }) From 6733bcfd37def173467a2507fa5aecbcf6db3e96 Mon Sep 17 00:00:00 2001 From: Willy Kloucek Date: Fri, 1 Apr 2022 15:13:45 +0200 Subject: [PATCH 3/3] use precondition failed in webdav too --- internal/http/services/owncloud/ocdav/locks.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/http/services/owncloud/ocdav/locks.go b/internal/http/services/owncloud/ocdav/locks.go index ba02badf09..e4bfb00446 100644 --- a/internal/http/services/owncloud/ocdav/locks.go +++ b/internal/http/services/owncloud/ocdav/locks.go @@ -488,7 +488,7 @@ func (s *svc) lockReference(ctx context.Context, w http.ResponseWriter, r *http. // this actually is a name based lock ... ugh token, err = s.LockSystem.Create(ctx, now, ld) if err != nil { - if _, ok := err.(errtypes.Locked); ok { + if _, ok := err.(errtypes.PreconditionFailed); ok { return http.StatusLocked, err } return http.StatusInternalServerError, err