From f19b69af252200640bae4e36678e13069f8cc7ea Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 1 Jun 2024 21:56:56 +0200 Subject: [PATCH 01/11] restore: allow overwrite to replace empty directories and symlinks With an already existing file tree an old directory or symlink may exist in a place where restore wants to create a new file. Thus, check for unexpected file types and clean up if necessary. --- internal/restorer/fileswriter.go | 106 +++++++++++++++++++------- internal/restorer/fileswriter_test.go | 77 +++++++++++++++++++ 2 files changed, 156 insertions(+), 27 deletions(-) diff --git a/internal/restorer/fileswriter.go b/internal/restorer/fileswriter.go index 50f06c83d..39ad65da8 100644 --- a/internal/restorer/fileswriter.go +++ b/internal/restorer/fileswriter.go @@ -1,11 +1,15 @@ package restorer import ( + "fmt" + stdfs "io/fs" "os" "sync" + "syscall" "github.com/cespare/xxhash/v2" "github.com/restic/restic/internal/debug" + "github.com/restic/restic/internal/errors" "github.com/restic/restic/internal/fs" ) @@ -39,13 +43,26 @@ func newFilesWriter(count int) *filesWriter { } } -func createFile(path string, createSize int64, sparse bool) (*os.File, error) { - f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY, 0600) +func openFile(path string) (*os.File, error) { + f, err := os.OpenFile(path, os.O_WRONLY|fs.O_NOFOLLOW, 0600) if err != nil { - if !fs.IsAccessDenied(err) { - return nil, err - } + return nil, err + } + fi, err := f.Stat() + if err != nil { + _ = f.Close() + return nil, err + } + if !fi.Mode().IsRegular() { + _ = f.Close() + return nil, fmt.Errorf("unexpected file type %v at %q", fi.Mode().Type(), path) + } + return f, nil +} +func createFile(path string, createSize int64, sparse bool) (*os.File, error) { + f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|fs.O_NOFOLLOW, 0600) + if err != nil && fs.IsAccessDenied(err) { // If file is readonly, clear the readonly flag by resetting the // permissions of the file and try again // as the metadata will be set again in the second pass and the @@ -53,40 +70,75 @@ func createFile(path string, createSize int64, sparse bool) (*os.File, error) { if err = fs.ResetPermissions(path); err != nil { return nil, err } - if f, err = os.OpenFile(path, os.O_WRONLY, 0600); err != nil { + if f, err = os.OpenFile(path, os.O_WRONLY|fs.O_NOFOLLOW, 0600); err != nil { + return nil, err + } + } else if err != nil && (errors.Is(err, syscall.ELOOP) || errors.Is(err, syscall.EISDIR)) { + // symlink or directory, try to remove it later on + f = nil + } else if err != nil { + return nil, err + } + + var fi stdfs.FileInfo + if f != nil { + // stat to check that we've opened a regular file + fi, err = f.Stat() + if err != nil { + _ = f.Close() + return nil, err + } + } + if f == nil || !fi.Mode().IsRegular() { + // close handle if we still have it + if f != nil { + if err := f.Close(); err != nil { + return nil, err + } + } + + // not what we expected, try to get rid of it + if err := os.Remove(path); err != nil { + return nil, err + } + // create a new file, pass O_EXCL to make sure there are no surprises + f, err = os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_EXCL|fs.O_NOFOLLOW, 0600) + if err != nil { + return nil, err + } + fi, err = f.Stat() + if err != nil { + _ = f.Close() return nil, err } } + return ensureSize(f, fi, createSize, sparse) +} + +func ensureSize(f *os.File, fi stdfs.FileInfo, createSize int64, sparse bool) (*os.File, error) { if sparse { - err = truncateSparse(f, createSize) + err := truncateSparse(f, createSize) if err != nil { _ = f.Close() return nil, err } - } else { - info, err := f.Stat() + } else if fi.Size() > createSize { + // file is too long must shorten it + err := f.Truncate(createSize) if err != nil { _ = f.Close() return nil, err } - if info.Size() > createSize { - // file is too long must shorten it - err = f.Truncate(createSize) - if err != nil { - _ = f.Close() - return nil, err - } - } else if createSize > 0 { - err := fs.PreallocateFile(f, createSize) - if err != nil { - // Just log the preallocate error but don't let it cause the restore process to fail. - // Preallocate might return an error if the filesystem (implementation) does not - // support preallocation or our parameters combination to the preallocate call - // This should yield a syscall.ENOTSUP error, but some other errors might also - // show up. - debug.Log("Failed to preallocate %v with size %v: %v", path, createSize, err) - } + } else if createSize > 0 { + err := fs.PreallocateFile(f, createSize) + if err != nil { + // Just log the preallocate error but don't let it cause the restore process to fail. + // Preallocate might return an error if the filesystem (implementation) does not + // support preallocation or our parameters combination to the preallocate call + // This should yield a syscall.ENOTSUP error, but some other errors might also + // show up. + debug.Log("Failed to preallocate %v with size %v: %v", f.Name(), createSize, err) } } return f, nil @@ -110,7 +162,7 @@ func (w *filesWriter) writeToFile(path string, blob []byte, offset int64, create if err != nil { return nil, err } - } else if f, err = os.OpenFile(path, os.O_WRONLY, 0600); err != nil { + } else if f, err = openFile(path); err != nil { return nil, err } diff --git a/internal/restorer/fileswriter_test.go b/internal/restorer/fileswriter_test.go index 7beb9a2dc..74bf479bb 100644 --- a/internal/restorer/fileswriter_test.go +++ b/internal/restorer/fileswriter_test.go @@ -1,9 +1,13 @@ package restorer import ( + "fmt" "os" + "path/filepath" + "syscall" "testing" + "github.com/restic/restic/internal/errors" rtest "github.com/restic/restic/internal/test" ) @@ -34,3 +38,76 @@ func TestFilesWriterBasic(t *testing.T) { rtest.OK(t, err) rtest.Equals(t, []byte{2, 2}, buf) } + +func TestCreateFile(t *testing.T) { + basepath := filepath.Join(t.TempDir(), "test") + + scenarios := []struct { + name string + create func(t testing.TB, path string) + err error + }{ + { + "file", + func(t testing.TB, path string) { + rtest.OK(t, os.WriteFile(path, []byte("test-test-test-data"), 0o400)) + }, + nil, + }, + { + "empty dir", + func(t testing.TB, path string) { + rtest.OK(t, os.Mkdir(path, 0o400)) + }, + nil, + }, + { + "symlink", + func(t testing.TB, path string) { + rtest.OK(t, os.Symlink("./something", path)) + }, + nil, + }, + { + "filled dir", + func(t testing.TB, path string) { + rtest.OK(t, os.Mkdir(path, 0o700)) + rtest.OK(t, os.WriteFile(filepath.Join(path, "file"), []byte("data"), 0o400)) + }, + syscall.ENOTEMPTY, + }, + } + + tests := []struct { + size int64 + isSparse bool + }{ + {5, false}, + {21, false}, + {100, false}, + {5, true}, + {21, true}, + {100, true}, + } + + for i, sc := range scenarios { + t.Run(sc.name, func(t *testing.T) { + for _, test := range tests { + path := basepath + fmt.Sprintf("%v", i) + sc.create(t, path) + f, err := createFile(path, test.size, test.isSparse) + if sc.err == nil { + rtest.OK(t, err) + fi, err := f.Stat() + rtest.OK(t, err) + rtest.Assert(t, fi.Mode().IsRegular(), "wrong filetype %v", fi.Mode()) + rtest.Assert(t, fi.Size() <= test.size, "unexpected file size expected %v, got %v", test.size, fi.Size()) + rtest.OK(t, f.Close()) + } else { + rtest.Assert(t, errors.Is(err, sc.err), "unexpected error got %v expected %v", err, sc.err) + } + rtest.OK(t, os.RemoveAll(path)) + } + }) + } +} From d265ec64f2411bfb259f83f33b4f39fe4e592725 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 1 Jun 2024 22:21:16 +0200 Subject: [PATCH 02/11] restore: correctly handle existing hardlinks With hardlinks there's no efficient way to detect which files are linked with each other. Thus, just start from scratch when restore has to modify a hardlinked file. --- internal/restorer/fileswriter.go | 13 ++++++++- internal/restorer/fileswriter_test.go | 41 ++++++++++++++++++--------- 2 files changed, 39 insertions(+), 15 deletions(-) diff --git a/internal/restorer/fileswriter.go b/internal/restorer/fileswriter.go index 39ad65da8..9e9b6b01c 100644 --- a/internal/restorer/fileswriter.go +++ b/internal/restorer/fileswriter.go @@ -89,7 +89,18 @@ func createFile(path string, createSize int64, sparse bool) (*os.File, error) { return nil, err } } - if f == nil || !fi.Mode().IsRegular() { + + mustReplace := f == nil || !fi.Mode().IsRegular() + if !mustReplace { + ex := fs.ExtendedStat(fi) + if ex.Links > 1 { + // there is no efficient way to find out which other files might be linked to this file + // thus nuke the existing file and start with a fresh one + mustReplace = true + } + } + + if mustReplace { // close handle if we still have it if f != nil { if err := f.Close(); err != nil { diff --git a/internal/restorer/fileswriter_test.go b/internal/restorer/fileswriter_test.go index 74bf479bb..b4252a96e 100644 --- a/internal/restorer/fileswriter_test.go +++ b/internal/restorer/fileswriter_test.go @@ -45,36 +45,46 @@ func TestCreateFile(t *testing.T) { scenarios := []struct { name string create func(t testing.TB, path string) + check func(t testing.TB, path string) err error }{ { - "file", - func(t testing.TB, path string) { + name: "file", + create: func(t testing.TB, path string) { rtest.OK(t, os.WriteFile(path, []byte("test-test-test-data"), 0o400)) }, - nil, }, { - "empty dir", - func(t testing.TB, path string) { + name: "empty dir", + create: func(t testing.TB, path string) { rtest.OK(t, os.Mkdir(path, 0o400)) }, - nil, }, { - "symlink", - func(t testing.TB, path string) { + name: "symlink", + create: func(t testing.TB, path string) { rtest.OK(t, os.Symlink("./something", path)) }, - nil, }, { - "filled dir", - func(t testing.TB, path string) { + name: "filled dir", + create: func(t testing.TB, path string) { rtest.OK(t, os.Mkdir(path, 0o700)) rtest.OK(t, os.WriteFile(filepath.Join(path, "file"), []byte("data"), 0o400)) }, - syscall.ENOTEMPTY, + err: syscall.ENOTEMPTY, + }, + { + name: "hardlinks", + create: func(t testing.TB, path string) { + rtest.OK(t, os.WriteFile(path, []byte("test-test-test-data"), 0o400)) + rtest.OK(t, os.Link(path, path+"h")) + }, + check: func(t testing.TB, path string) { + data, err := os.ReadFile(path + "h") + rtest.OK(t, err) + rtest.Equals(t, "test-test-test-data", string(data), "unexpected content change") + }, }, } @@ -92,8 +102,8 @@ func TestCreateFile(t *testing.T) { for i, sc := range scenarios { t.Run(sc.name, func(t *testing.T) { - for _, test := range tests { - path := basepath + fmt.Sprintf("%v", i) + for j, test := range tests { + path := basepath + fmt.Sprintf("%v%v", i, j) sc.create(t, path) f, err := createFile(path, test.size, test.isSparse) if sc.err == nil { @@ -103,6 +113,9 @@ func TestCreateFile(t *testing.T) { rtest.Assert(t, fi.Mode().IsRegular(), "wrong filetype %v", fi.Mode()) rtest.Assert(t, fi.Size() <= test.size, "unexpected file size expected %v, got %v", test.size, fi.Size()) rtest.OK(t, f.Close()) + if sc.check != nil { + sc.check(t, path) + } } else { rtest.Assert(t, errors.Is(err, sc.err), "unexpected error got %v expected %v", err, sc.err) } From c598a751c25fb9d2f4050b3717639e0c6d723cc4 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 2 Jun 2024 16:54:19 +0200 Subject: [PATCH 03/11] restore: fine-grained sparse support for windows --- internal/restorer/sparsewrite.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/internal/restorer/sparsewrite.go b/internal/restorer/sparsewrite.go index 2c1f234de..ae354f64f 100644 --- a/internal/restorer/sparsewrite.go +++ b/internal/restorer/sparsewrite.go @@ -1,6 +1,3 @@ -//go:build !windows -// +build !windows - package restorer import ( From c7902b77248a13729bcf6f8c93f0b9a47cfd27b6 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 7 Jun 2024 22:43:31 +0200 Subject: [PATCH 04/11] restorer: cleanup overwrite tests --- internal/restorer/restorer_test.go | 70 ++++++++++++------------------ 1 file changed, 27 insertions(+), 43 deletions(-) diff --git a/internal/restorer/restorer_test.go b/internal/restorer/restorer_test.go index 25ce668db..b1fb0ff92 100644 --- a/internal/restorer/restorer_test.go +++ b/internal/restorer/restorer_test.go @@ -895,6 +895,31 @@ func TestRestorerSparseFiles(t *testing.T) { len(zeros), blocks, 100*sparsity) } +func saveSnapshotsAndOverwrite(t *testing.T, baseSnapshot Snapshot, overwriteSnapshot Snapshot, options Options) string { + repo := repository.TestRepository(t) + tempdir := filepath.Join(rtest.TempDir(t), "target") + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // base snapshot + sn, id := saveSnapshot(t, repo, baseSnapshot, noopGetGenericAttributes) + t.Logf("base snapshot saved as %v", id.Str()) + + res := NewRestorer(repo, sn, options) + rtest.OK(t, res.RestoreTo(ctx, tempdir)) + + // overwrite snapshot + sn, id = saveSnapshot(t, repo, overwriteSnapshot, noopGetGenericAttributes) + t.Logf("overwrite snapshot saved as %v", id.Str()) + res = NewRestorer(repo, sn, options) + rtest.OK(t, res.RestoreTo(ctx, tempdir)) + + _, err := res.VerifyFiles(ctx, tempdir) + rtest.OK(t, err) + + return tempdir +} + func TestRestorerSparseOverwrite(t *testing.T) { baseSnapshot := Snapshot{ Nodes: map[string]Node{ @@ -908,29 +933,7 @@ func TestRestorerSparseOverwrite(t *testing.T) { }, } - repo := repository.TestRepository(t) - tempdir := filepath.Join(rtest.TempDir(t), "target") - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - // base snapshot - sn, id := saveSnapshot(t, repo, baseSnapshot, noopGetGenericAttributes) - t.Logf("base snapshot saved as %v", id.Str()) - - res := NewRestorer(repo, sn, Options{Sparse: true}) - err := res.RestoreTo(ctx, tempdir) - rtest.OK(t, err) - - // sparse snapshot - sn, id = saveSnapshot(t, repo, sparseSnapshot, noopGetGenericAttributes) - t.Logf("base snapshot saved as %v", id.Str()) - - res = NewRestorer(repo, sn, Options{Sparse: true, Overwrite: OverwriteAlways}) - err = res.RestoreTo(ctx, tempdir) - rtest.OK(t, err) - files, err := res.VerifyFiles(ctx, tempdir) - rtest.OK(t, err) - rtest.Equals(t, 1, files, "unexpected number of verified files") + saveSnapshotsAndOverwrite(t, baseSnapshot, sparseSnapshot, Options{Sparse: true, Overwrite: OverwriteAlways}) } func TestRestorerOverwriteBehavior(t *testing.T) { @@ -993,26 +996,7 @@ func TestRestorerOverwriteBehavior(t *testing.T) { for _, test := range tests { t.Run("", func(t *testing.T) { - repo := repository.TestRepository(t) - tempdir := filepath.Join(rtest.TempDir(t), "target") - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - // base snapshot - sn, id := saveSnapshot(t, repo, baseSnapshot, noopGetGenericAttributes) - t.Logf("base snapshot saved as %v", id.Str()) - - res := NewRestorer(repo, sn, Options{}) - rtest.OK(t, res.RestoreTo(ctx, tempdir)) - - // overwrite snapshot - sn, id = saveSnapshot(t, repo, overwriteSnapshot, noopGetGenericAttributes) - t.Logf("overwrite snapshot saved as %v", id.Str()) - res = NewRestorer(repo, sn, Options{Overwrite: test.Overwrite}) - rtest.OK(t, res.RestoreTo(ctx, tempdir)) - - _, err := res.VerifyFiles(ctx, tempdir) - rtest.OK(t, err) + tempdir := saveSnapshotsAndOverwrite(t, baseSnapshot, overwriteSnapshot, Options{Overwrite: test.Overwrite}) for filename, content := range test.Files { data, err := os.ReadFile(filepath.Join(tempdir, filepath.FromSlash(filename))) From ac729db3ce8e2073d472272ec7b2a8845d5ac120 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 7 Jun 2024 22:44:47 +0200 Subject: [PATCH 05/11] restorer: fix overwriting of special file types An attempt to replace an existing file with a hardlink previously ended with a missing file. Remove an existing file before trying to restore a special node. This generalizes the existing behavior for symlinks to all special node types. --- internal/restic/node.go | 5 --- internal/restorer/restorer.go | 5 ++- internal/restorer/restorer_test.go | 67 ++++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 6 deletions(-) diff --git a/internal/restic/node.go b/internal/restic/node.go index 5bdc5ba27..51c6071b7 100644 --- a/internal/restic/node.go +++ b/internal/restic/node.go @@ -348,11 +348,6 @@ func (node Node) writeNodeContent(ctx context.Context, repo BlobLoader, f *os.Fi } func (node Node) createSymlinkAt(path string) error { - - if err := os.Remove(path); err != nil && !errors.Is(err, os.ErrNotExist) { - return errors.Wrap(err, "Symlink") - } - if err := fs.Symlink(node.LinkTarget, path); err != nil { return errors.WithStack(err) } diff --git a/internal/restorer/restorer.go b/internal/restorer/restorer.go index 6002d6f0e..313174fc3 100644 --- a/internal/restorer/restorer.go +++ b/internal/restorer/restorer.go @@ -221,6 +221,9 @@ func (res *Restorer) traverseTree(ctx context.Context, target, location string, func (res *Restorer) restoreNodeTo(ctx context.Context, node *restic.Node, target, location string) error { debug.Log("restoreNode %v %v %v", node.Name, target, location) + if err := fs.Remove(target); err != nil && !errors.Is(err, os.ErrNotExist) { + return errors.Wrap(err, "RemoveNode") + } err := node.CreateAt(ctx, target, res.repo) if err != nil { @@ -242,7 +245,7 @@ func (res *Restorer) restoreNodeMetadataTo(node *restic.Node, target, location s } func (res *Restorer) restoreHardlinkAt(node *restic.Node, target, path, location string) error { - if err := fs.Remove(path); !os.IsNotExist(err) { + if err := fs.Remove(path); err != nil && !errors.Is(err, os.ErrNotExist) { return errors.Wrap(err, "RemoveCreateHardlink") } err := fs.Link(target, path) diff --git a/internal/restorer/restorer_test.go b/internal/restorer/restorer_test.go index b1fb0ff92..5c23a88e4 100644 --- a/internal/restorer/restorer_test.go +++ b/internal/restorer/restorer_test.go @@ -37,6 +37,11 @@ type File struct { attributes *FileAttributes } +type Symlink struct { + Target string + ModTime time.Time +} + type Dir struct { Nodes map[string]Node Mode os.FileMode @@ -103,6 +108,20 @@ func saveDir(t testing.TB, repo restic.BlobSaver, nodes map[string]Node, inode u GenericAttributes: getGenericAttributes(node.attributes, false), }) rtest.OK(t, err) + case Symlink: + symlink := n.(Symlink) + err := tree.Insert(&restic.Node{ + Type: "symlink", + Mode: os.ModeSymlink | 0o777, + ModTime: symlink.ModTime, + Name: name, + UID: uint32(os.Getuid()), + GID: uint32(os.Getgid()), + LinkTarget: symlink.Target, + Inode: inode, + Links: 1, + }) + rtest.OK(t, err) case Dir: id := saveDir(t, repo, node.Nodes, inode, getGenericAttributes) @@ -1013,6 +1032,54 @@ func TestRestorerOverwriteBehavior(t *testing.T) { } } +func TestRestorerOverwriteSpecial(t *testing.T) { + baseTime := time.Now() + baseSnapshot := Snapshot{ + Nodes: map[string]Node{ + "dirtest": Dir{ModTime: baseTime}, + "link": Symlink{Target: "foo", ModTime: baseTime}, + "file": File{Data: "content: file\n", Inode: 42, Links: 2, ModTime: baseTime}, + "hardlink": File{Data: "content: file\n", Inode: 42, Links: 2, ModTime: baseTime}, + }, + } + overwriteSnapshot := Snapshot{ + Nodes: map[string]Node{ + "dirtest": Symlink{Target: "foo", ModTime: baseTime}, + "link": File{Data: "content: link\n", Inode: 42, Links: 2, ModTime: baseTime.Add(time.Second)}, + "file": Symlink{Target: "foo2", ModTime: baseTime}, + "hardlink": File{Data: "content: link\n", Inode: 42, Links: 2, ModTime: baseTime.Add(time.Second)}, + }, + } + + files := map[string]string{ + "link": "content: link\n", + "hardlink": "content: link\n", + } + links := map[string]string{ + "dirtest": "foo", + "file": "foo2", + } + + tempdir := saveSnapshotsAndOverwrite(t, baseSnapshot, overwriteSnapshot, Options{Overwrite: OverwriteAlways}) + + for filename, content := range files { + data, err := os.ReadFile(filepath.Join(tempdir, filepath.FromSlash(filename))) + if err != nil { + t.Errorf("unable to read file %v: %v", filename, err) + continue + } + + if !bytes.Equal(data, []byte(content)) { + t.Errorf("file %v has wrong content: want %q, got %q", filename, content, data) + } + } + for filename, target := range links { + link, err := fs.Readlink(filepath.Join(tempdir, filepath.FromSlash(filename))) + rtest.OK(t, err) + rtest.Equals(t, link, target, "wrong symlink target") + } +} + func TestRestoreModified(t *testing.T) { // overwrite files between snapshots and also change their filesize snapshots := []Snapshot{ From ebbd4e26d7027c92498184083d816a28684178d7 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 7 Jun 2024 23:02:46 +0200 Subject: [PATCH 06/11] restorer: allow directory to replace existing file --- internal/restorer/restorer.go | 26 +++++++++++++++++++------- internal/restorer/restorer_test.go | 2 ++ 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/internal/restorer/restorer.go b/internal/restorer/restorer.go index 313174fc3..19555afb8 100644 --- a/internal/restorer/restorer.go +++ b/internal/restorer/restorer.go @@ -259,6 +259,23 @@ func (res *Restorer) restoreHardlinkAt(node *restic.Node, target, path, location return res.restoreNodeMetadataTo(node, path, location) } +func (res *Restorer) ensureDir(target string) error { + fi, err := fs.Lstat(target) + if err != nil && !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("failed to check for directory: %w", err) + } + if err == nil && !fi.IsDir() { + // try to cleanup unexpected file + if err := fs.Remove(target); err != nil { + return fmt.Errorf("failed to remove stale item: %w", err) + } + } + + // create parent dir with default permissions + // second pass #leaveDir restores dir metadata after visiting/restoring all children + return fs.MkdirAll(target, 0700) +} + // RestoreTo creates the directories and files in the snapshot below dst. // Before an item is created, res.Filter is called. func (res *Restorer) RestoreTo(ctx context.Context, dst string) error { @@ -284,17 +301,12 @@ func (res *Restorer) RestoreTo(ctx context.Context, dst string) error { enterDir: func(_ *restic.Node, target, location string) error { debug.Log("first pass, enterDir: mkdir %q, leaveDir should restore metadata", location) res.opts.Progress.AddFile(0) - // create dir with default permissions - // #leaveDir restores dir metadata after visiting all children - return fs.MkdirAll(target, 0700) + return res.ensureDir(target) }, visitNode: func(node *restic.Node, target, location string) error { debug.Log("first pass, visitNode: mkdir %q, leaveDir on second pass should restore metadata", location) - // create parent dir with default permissions - // second pass #leaveDir restores dir metadata after visiting/restoring all children - err := fs.MkdirAll(filepath.Dir(target), 0700) - if err != nil { + if err := res.ensureDir(filepath.Dir(target)); err != nil { return err } diff --git a/internal/restorer/restorer_test.go b/internal/restorer/restorer_test.go index 5c23a88e4..3becf7c7a 100644 --- a/internal/restorer/restorer_test.go +++ b/internal/restorer/restorer_test.go @@ -1040,6 +1040,7 @@ func TestRestorerOverwriteSpecial(t *testing.T) { "link": Symlink{Target: "foo", ModTime: baseTime}, "file": File{Data: "content: file\n", Inode: 42, Links: 2, ModTime: baseTime}, "hardlink": File{Data: "content: file\n", Inode: 42, Links: 2, ModTime: baseTime}, + "newdir": File{Data: "content: dir\n", ModTime: baseTime}, }, } overwriteSnapshot := Snapshot{ @@ -1048,6 +1049,7 @@ func TestRestorerOverwriteSpecial(t *testing.T) { "link": File{Data: "content: link\n", Inode: 42, Links: 2, ModTime: baseTime.Add(time.Second)}, "file": Symlink{Target: "foo2", ModTime: baseTime}, "hardlink": File{Data: "content: link\n", Inode: 42, Links: 2, ModTime: baseTime.Add(time.Second)}, + "newdir": Dir{ModTime: baseTime}, }, } From 4d6042fe951ba2c72eda68a65dee7c67c56ec77d Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Thu, 13 Jun 2024 22:21:00 +0200 Subject: [PATCH 07/11] restore: remove unexpected xattrs from files --- internal/restic/node_xattr.go | 22 +++++++++++++ internal/restic/node_xattr_all_test.go | 44 ++++++++++++++++++++++++++ 2 files changed, 66 insertions(+) create mode 100644 internal/restic/node_xattr_all_test.go diff --git a/internal/restic/node_xattr.go b/internal/restic/node_xattr.go index a55fcb2db..5a5a253d9 100644 --- a/internal/restic/node_xattr.go +++ b/internal/restic/node_xattr.go @@ -40,6 +40,11 @@ func setxattr(path, name string, data []byte) error { return handleXattrErr(xattr.LSet(path, name, data)) } +// removexattr removes the attribute name from path. +func removexattr(path, name string) error { + return handleXattrErr(xattr.LRemove(path, name)) +} + func handleXattrErr(err error) error { switch e := err.(type) { case nil: @@ -70,12 +75,29 @@ func (node *Node) fillGenericAttributes(_ string, _ os.FileInfo, _ *statT) (allo } func (node Node) restoreExtendedAttributes(path string) error { + expectedAttrs := map[string]struct{}{} for _, attr := range node.ExtendedAttributes { err := setxattr(path, attr.Name, attr.Value) if err != nil { return err } + expectedAttrs[attr.Name] = struct{}{} } + + // remove unexpected xattrs + xattrs, err := listxattr(path) + if err != nil { + return err + } + for _, name := range xattrs { + if _, ok := expectedAttrs[name]; ok { + continue + } + if err := removexattr(path, name); err != nil { + return err + } + } + return nil } diff --git a/internal/restic/node_xattr_all_test.go b/internal/restic/node_xattr_all_test.go new file mode 100644 index 000000000..4e93330bc --- /dev/null +++ b/internal/restic/node_xattr_all_test.go @@ -0,0 +1,44 @@ +//go:build darwin || freebsd || linux || solaris || windows +// +build darwin freebsd linux solaris windows + +package restic + +import ( + "os" + "path/filepath" + "testing" + + rtest "github.com/restic/restic/internal/test" +) + +func setAndVerifyXattr(t *testing.T, file string, attrs []ExtendedAttribute) { + node := Node{ + ExtendedAttributes: attrs, + } + rtest.OK(t, node.restoreExtendedAttributes(file)) + + nodeActual := Node{} + rtest.OK(t, nodeActual.fillExtendedAttributes(file, false)) + + rtest.Assert(t, nodeActual.sameExtendedAttributes(node), "xattr mismatch got %v expected %v", nodeActual.ExtendedAttributes, node.ExtendedAttributes) +} + +func TestOverwriteXattr(t *testing.T) { + dir := t.TempDir() + file := filepath.Join(dir, "file") + rtest.OK(t, os.WriteFile(file, []byte("hello world"), 0o600)) + + setAndVerifyXattr(t, file, []ExtendedAttribute{ + { + Name: "user.foo", + Value: []byte("bar"), + }, + }) + + setAndVerifyXattr(t, file, []ExtendedAttribute{ + { + Name: "user.other", + Value: []byte("some"), + }, + }) +} From 3d73ae9988aeae1f7b23877ebdb46404932cc788 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Thu, 13 Jun 2024 22:32:53 +0200 Subject: [PATCH 08/11] update restore changelog --- changelog/unreleased/issue-4817 | 1 + 1 file changed, 1 insertion(+) diff --git a/changelog/unreleased/issue-4817 b/changelog/unreleased/issue-4817 index ddbd5672b..e9c2d01a5 100644 --- a/changelog/unreleased/issue-4817 +++ b/changelog/unreleased/issue-4817 @@ -20,3 +20,4 @@ https://github.com/restic/restic/issues/407 https://github.com/restic/restic/issues/2662 https://github.com/restic/restic/pull/4837 https://github.com/restic/restic/pull/4838 +https://github.com/restic/restic/pull/4864 From ca41c8fd11e18cc82f3c280e3573433c40fa62fa Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Thu, 13 Jun 2024 22:40:35 +0200 Subject: [PATCH 09/11] restore: use fs function wrappers This ensures proper path handling on Windows. --- internal/restorer/fileswriter.go | 10 +++++----- internal/restorer/restorer.go | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/restorer/fileswriter.go b/internal/restorer/fileswriter.go index 9e9b6b01c..034ed2725 100644 --- a/internal/restorer/fileswriter.go +++ b/internal/restorer/fileswriter.go @@ -44,7 +44,7 @@ func newFilesWriter(count int) *filesWriter { } func openFile(path string) (*os.File, error) { - f, err := os.OpenFile(path, os.O_WRONLY|fs.O_NOFOLLOW, 0600) + f, err := fs.OpenFile(path, fs.O_WRONLY|fs.O_NOFOLLOW, 0600) if err != nil { return nil, err } @@ -61,7 +61,7 @@ func openFile(path string) (*os.File, error) { } func createFile(path string, createSize int64, sparse bool) (*os.File, error) { - f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|fs.O_NOFOLLOW, 0600) + f, err := fs.OpenFile(path, fs.O_CREATE|fs.O_WRONLY|fs.O_NOFOLLOW, 0600) if err != nil && fs.IsAccessDenied(err) { // If file is readonly, clear the readonly flag by resetting the // permissions of the file and try again @@ -70,7 +70,7 @@ func createFile(path string, createSize int64, sparse bool) (*os.File, error) { if err = fs.ResetPermissions(path); err != nil { return nil, err } - if f, err = os.OpenFile(path, os.O_WRONLY|fs.O_NOFOLLOW, 0600); err != nil { + if f, err = fs.OpenFile(path, fs.O_WRONLY|fs.O_NOFOLLOW, 0600); err != nil { return nil, err } } else if err != nil && (errors.Is(err, syscall.ELOOP) || errors.Is(err, syscall.EISDIR)) { @@ -109,11 +109,11 @@ func createFile(path string, createSize int64, sparse bool) (*os.File, error) { } // not what we expected, try to get rid of it - if err := os.Remove(path); err != nil { + if err := fs.Remove(path); err != nil { return nil, err } // create a new file, pass O_EXCL to make sure there are no surprises - f, err = os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_EXCL|fs.O_NOFOLLOW, 0600) + f, err = fs.OpenFile(path, fs.O_CREATE|fs.O_WRONLY|fs.O_EXCL|fs.O_NOFOLLOW, 0600) if err != nil { return nil, err } diff --git a/internal/restorer/restorer.go b/internal/restorer/restorer.go index 19555afb8..85132c8b4 100644 --- a/internal/restorer/restorer.go +++ b/internal/restorer/restorer.go @@ -541,7 +541,7 @@ func (s *fileState) HasMatchingBlob(i int) bool { // Reusing buffers prevents the verifier goroutines allocating all of RAM and // flushing the filesystem cache (at least on Linux). func (res *Restorer) verifyFile(target string, node *restic.Node, failFast bool, trustMtime bool, buf []byte) (*fileState, []byte, error) { - f, err := os.OpenFile(target, fs.O_RDONLY|fs.O_NOFOLLOW, 0) + f, err := fs.OpenFile(target, fs.O_RDONLY|fs.O_NOFOLLOW, 0) if err != nil { return nil, buf, err } From 9572b7224f6ac16c0b8a9a3bbf8bd8a7c29f65e9 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Thu, 13 Jun 2024 22:52:31 +0200 Subject: [PATCH 10/11] restorer: windows test fixes --- internal/restic/node_xattr_all_test.go | 14 +++++++++++++- internal/restorer/fileswriter_other_test.go | 10 ++++++++++ internal/restorer/fileswriter_test.go | 9 +++++++-- internal/restorer/fileswriter_windows_test.go | 7 +++++++ 4 files changed, 37 insertions(+), 3 deletions(-) create mode 100644 internal/restorer/fileswriter_other_test.go create mode 100644 internal/restorer/fileswriter_windows_test.go diff --git a/internal/restic/node_xattr_all_test.go b/internal/restic/node_xattr_all_test.go index 4e93330bc..56ce5e286 100644 --- a/internal/restic/node_xattr_all_test.go +++ b/internal/restic/node_xattr_all_test.go @@ -6,18 +6,30 @@ package restic import ( "os" "path/filepath" + "runtime" + "strings" "testing" rtest "github.com/restic/restic/internal/test" ) func setAndVerifyXattr(t *testing.T, file string, attrs []ExtendedAttribute) { + if runtime.GOOS == "windows" { + // windows seems to convert the xattr name to upper case + for i := range attrs { + attrs[i].Name = strings.ToUpper(attrs[i].Name) + } + } + node := Node{ + Type: "file", ExtendedAttributes: attrs, } rtest.OK(t, node.restoreExtendedAttributes(file)) - nodeActual := Node{} + nodeActual := Node{ + Type: "file", + } rtest.OK(t, nodeActual.fillExtendedAttributes(file, false)) rtest.Assert(t, nodeActual.sameExtendedAttributes(node), "xattr mismatch got %v expected %v", nodeActual.ExtendedAttributes, node.ExtendedAttributes) diff --git a/internal/restorer/fileswriter_other_test.go b/internal/restorer/fileswriter_other_test.go new file mode 100644 index 000000000..530a190e5 --- /dev/null +++ b/internal/restorer/fileswriter_other_test.go @@ -0,0 +1,10 @@ +//go:build !windows +// +build !windows + +package restorer + +import "syscall" + +func notEmptyDirError() error { + return syscall.ENOTEMPTY +} diff --git a/internal/restorer/fileswriter_test.go b/internal/restorer/fileswriter_test.go index b4252a96e..383a9e0d7 100644 --- a/internal/restorer/fileswriter_test.go +++ b/internal/restorer/fileswriter_test.go @@ -4,7 +4,7 @@ import ( "fmt" "os" "path/filepath" - "syscall" + "runtime" "testing" "github.com/restic/restic/internal/errors" @@ -72,7 +72,7 @@ func TestCreateFile(t *testing.T) { rtest.OK(t, os.Mkdir(path, 0o700)) rtest.OK(t, os.WriteFile(filepath.Join(path, "file"), []byte("data"), 0o400)) }, - err: syscall.ENOTEMPTY, + err: notEmptyDirError(), }, { name: "hardlinks", @@ -81,6 +81,11 @@ func TestCreateFile(t *testing.T) { rtest.OK(t, os.Link(path, path+"h")) }, check: func(t testing.TB, path string) { + if runtime.GOOS == "windows" { + // hardlinks are not supported on windows + return + } + data, err := os.ReadFile(path + "h") rtest.OK(t, err) rtest.Equals(t, "test-test-test-data", string(data), "unexpected content change") diff --git a/internal/restorer/fileswriter_windows_test.go b/internal/restorer/fileswriter_windows_test.go new file mode 100644 index 000000000..ec2b062f0 --- /dev/null +++ b/internal/restorer/fileswriter_windows_test.go @@ -0,0 +1,7 @@ +package restorer + +import "syscall" + +func notEmptyDirError() error { + return syscall.ERROR_DIR_NOT_EMPTY +} From deca7d08ac3981105ecf4a04c9261e95d579e99b Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 14 Jun 2024 20:17:06 +0200 Subject: [PATCH 11/11] restorer: cleanup unexpected xattrs on windows --- internal/restic/node_windows.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/internal/restic/node_windows.go b/internal/restic/node_windows.go index 0d96bdb98..48ce07295 100644 --- a/internal/restic/node_windows.go +++ b/internal/restic/node_windows.go @@ -155,6 +155,26 @@ func restoreExtendedAttributes(nodeType, path string, eas []fs.ExtendedAttribute } defer closeFileHandle(fileHandle, path) // Replaced inline defer with named function call + // clear old unexpected xattrs by setting them to an empty value + oldEAs, err := fs.GetFileEA(fileHandle) + if err != nil { + return err + } + + for _, oldEA := range oldEAs { + found := false + for _, ea := range eas { + if strings.EqualFold(ea.Name, oldEA.Name) { + found = true + break + } + } + + if !found { + eas = append(eas, fs.ExtendedAttribute{Name: oldEA.Name, Value: nil}) + } + } + if err = fs.SetFileEA(fileHandle, eas); err != nil { return errors.Errorf("set EA failed for path %v, with: %v", path, err) }