diff --git a/changelog/unreleased/bugfix-decomposedfs-stale-trash-symlink-on-delete-failure.md b/changelog/unreleased/bugfix-decomposedfs-stale-trash-symlink-on-delete-failure.md new file mode 100644 index 00000000000..13d988262f2 --- /dev/null +++ b/changelog/unreleased/bugfix-decomposedfs-stale-trash-symlink-on-delete-failure.md @@ -0,0 +1,13 @@ +Bugfix: Remove stale trash symlink when decomposedfs Delete fails mid-operation + +`Tree.Delete` creates a trash symlink before renaming the node. If any +subsequent step failed (`os.Rename`, `MetadataBackend().Rename`, or +`os.Remove` of the parent-dir entry) the symlink was not removed during +rollback. The dangling symlink permanently blocked future delete attempts +on the same node with a `file exists` error on the next `os.Symlink` call. + +All three failure paths now remove the trash symlink as part of rollback. +The third path additionally reverts the node rename and metadata rename in +reverse order. + +https://github.com/owncloud/reva/pull/723 diff --git a/pkg/storage/utils/decomposedfs/tree/tree.go b/pkg/storage/utils/decomposedfs/tree/tree.go index 1b2c397a0e3..0ff07d9f49b 100644 --- a/pkg/storage/utils/decomposedfs/tree/tree.go +++ b/pkg/storage/utils/decomposedfs/tree/tree.go @@ -507,14 +507,13 @@ func (t *Tree) Delete(ctx context.Context, n *node.Node) (err error) { trashPath := nodePath + node.TrashIDDelimiter + deletionTime err = os.Rename(nodePath, trashPath) if err != nil { - // To roll back changes - // TODO remove symlink - // Roll back changes + _ = os.Remove(trashLink) _ = n.RemoveXattr(ctx, prefixes.TrashOriginAttr, true) return } err = t.lookup.MetadataBackend().Rename(nodePath, trashPath) if err != nil { + _ = os.Remove(trashLink) _ = n.RemoveXattr(ctx, prefixes.TrashOriginAttr, true) _ = os.Rename(trashPath, nodePath) return @@ -525,10 +524,9 @@ func (t *Tree) Delete(ctx context.Context, n *node.Node) (err error) { // finally remove the entry from the parent dir if err = os.Remove(path); err != nil { - // To roll back changes - // TODO revert the rename - // TODO remove symlink - // Roll back changes + _ = t.lookup.MetadataBackend().Rename(trashPath, nodePath) + _ = os.Rename(trashPath, nodePath) + _ = os.Remove(trashLink) _ = n.RemoveXattr(ctx, prefixes.TrashOriginAttr, true) return } diff --git a/pkg/storage/utils/decomposedfs/tree/tree_test.go b/pkg/storage/utils/decomposedfs/tree/tree_test.go index 467f4246fed..473df7c0e92 100644 --- a/pkg/storage/utils/decomposedfs/tree/tree_test.go +++ b/pkg/storage/utils/decomposedfs/tree/tree_test.go @@ -19,23 +19,33 @@ package tree_test import ( + "errors" "os" "path" "path/filepath" provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1" "github.com/google/uuid" + "github.com/rs/zerolog" + "github.com/owncloud/reva/v2/pkg/storage/fs/posix/timemanager" "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/lookup" + "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/metadata" "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/metadata/prefixes" "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/node" helpers "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/testhelpers" "github.com/owncloud/reva/v2/pkg/storage/utils/decomposedfs/tree" + "github.com/owncloud/reva/v2/pkg/store" "github.com/stretchr/testify/mock" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) +// renameFailBackend wraps a metadata.Backend and injects a failure on Rename (OCISDEV-817) +type renameFailBackend struct{ metadata.Backend } + +func (b renameFailBackend) Rename(_, _ string) error { return errors.New("injected rename error") } + var _ = Describe("Tree", func() { var ( env *helpers.TestEnv @@ -135,6 +145,63 @@ var _ = Describe("Tree", func() { env.Blobstore.AssertNotCalled(GinkgoT(), "Delete", mock.AnythingOfType("*node.Node")) }) }) + + // OCISDEV-817: trash symlink must be removed when MetadataBackend.Rename fails mid-delete. + // This is the exact path observed in the SE-1149 production incident. + Context("when MetadataBackend.Rename fails", func() { + It("rolls back the trash symlink", func() { + log := &zerolog.Logger{} + lu := lookup.New(renameFailBackend{env.Lookup.MetadataBackend()}, env.Options, &timemanager.Manager{}) + failTree := tree.New(lu, env.Blobstore, env.Options, store.Create(), log) + + file, err := lu.NodeFromResource(env.Ctx, &provider.Reference{ + ResourceId: env.SpaceRootRes, + Path: "dir1/file1", + }) + Expect(err).ToNot(HaveOccurred()) + + Expect(failTree.Delete(env.Ctx, file)).To(HaveOccurred()) + + trashLink := path.Join(env.Root, "spaces", lookup.Pathify(file.SpaceRoot.ID, 1, 2), "trash", lookup.Pathify(file.ID, 4, 2)) + _, statErr := os.Lstat(trashLink) + Expect(os.IsNotExist(statErr)).To(BeTrue()) + }) + }) + + // OCISDEV-817: trash symlink must be removed when os.Rename fails mid-delete. + // chmod 0500 on the node's parent dir blocks rename but allows xattr writes (tested on macOS + Linux). + Context("when os.Rename fails", func() { + It("rolls back the trash symlink", func() { + nodeDir := filepath.Dir(n.InternalPath()) + Expect(os.Chmod(nodeDir, 0o500)).To(Succeed()) + defer os.Chmod(nodeDir, 0o700) + + Expect(t.Delete(env.Ctx, n)).To(HaveOccurred()) + + trashLink := path.Join(env.Root, "spaces", lookup.Pathify(n.SpaceRoot.ID, 1, 2), "trash", lookup.Pathify(n.ID, 4, 2)) + _, statErr := os.Lstat(trashLink) + Expect(os.IsNotExist(statErr)).To(BeTrue()) + }) + }) + + // OCISDEV-817: trash symlink and node rename must be rolled back when removing + // the parent-dir entry fails (path #3). chmod 0500 on n.ParentPath() blocks + // os.Remove(path) while all prior steps succeed. + Context("when os.Remove of parent entry fails", func() { + It("rolls back the trash symlink and node rename", func() { + Expect(os.Chmod(n.ParentPath(), 0o500)).To(Succeed()) + defer os.Chmod(n.ParentPath(), 0o700) + + Expect(t.Delete(env.Ctx, n)).To(HaveOccurred()) + + trashLink := path.Join(env.Root, "spaces", lookup.Pathify(n.SpaceRoot.ID, 1, 2), "trash", lookup.Pathify(n.ID, 4, 2)) + _, statErr := os.Lstat(trashLink) + Expect(os.IsNotExist(statErr)).To(BeTrue()) + + _, err := os.Stat(n.InternalPath()) + Expect(err).ToNot(HaveOccurred()) + }) + }) }) Context("that was deleted", func() {