From 60960d2405f513b9b7a7abb2c00c535c471524a8 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 18 Oct 2024 19:27:42 +0200 Subject: [PATCH 1/9] fs/vss: properly create node from vss path Previously, NodeFromFileInfo used the original file path to create the node, which also meant that extended metadata was read from there instead of within the vss snapshot. --- internal/fs/fs_local_vss.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal/fs/fs_local_vss.go b/internal/fs/fs_local_vss.go index 908e744ee..54139ab2e 100644 --- a/internal/fs/fs_local_vss.go +++ b/internal/fs/fs_local_vss.go @@ -10,6 +10,7 @@ import ( "github.com/restic/restic/internal/errors" "github.com/restic/restic/internal/options" + "github.com/restic/restic/internal/restic" ) // VSSConfig holds extended options of windows volume shadow copy service. @@ -140,6 +141,10 @@ func (fs *LocalVss) Lstat(name string) (os.FileInfo, error) { return os.Lstat(fs.snapshotPath(name)) } +func (fs *LocalVss) NodeFromFileInfo(path string, fi os.FileInfo, ignoreXattrListError bool) (*restic.Node, error) { + return fs.FS.NodeFromFileInfo(fs.snapshotPath(path), fi, ignoreXattrListError) +} + // isMountPointIncluded is true if given mountpoint included by user. func (fs *LocalVss) isMountPointIncluded(mountPoint string) bool { if fs.excludeVolumes == nil { From b988754a6dbcd0a3a93c400d86fb9fe4f499e086 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 18 Oct 2024 19:30:05 +0200 Subject: [PATCH 2/9] fs/vss: reuse functions from underlying FS OpenFile, Stat and Lstat should reuse the underlying FS implementation to avoid diverging behavior. --- internal/fs/fs_local_vss.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/fs/fs_local_vss.go b/internal/fs/fs_local_vss.go index 54139ab2e..1915e2a7c 100644 --- a/internal/fs/fs_local_vss.go +++ b/internal/fs/fs_local_vss.go @@ -128,17 +128,17 @@ func (fs *LocalVss) DeleteSnapshots() { // OpenFile wraps the Open method of the underlying file system. func (fs *LocalVss) OpenFile(name string, flag int, perm os.FileMode) (File, error) { - return os.OpenFile(fs.snapshotPath(name), flag, perm) + return fs.FS.OpenFile(fs.snapshotPath(name), flag, perm) } // Stat wraps the Stat method of the underlying file system. func (fs *LocalVss) Stat(name string) (os.FileInfo, error) { - return os.Stat(fs.snapshotPath(name)) + return fs.FS.Stat(fs.snapshotPath(name)) } // Lstat wraps the Lstat method of the underlying file system. func (fs *LocalVss) Lstat(name string) (os.FileInfo, error) { - return os.Lstat(fs.snapshotPath(name)) + return fs.FS.Lstat(fs.snapshotPath(name)) } func (fs *LocalVss) NodeFromFileInfo(path string, fi os.FileInfo, ignoreXattrListError bool) (*restic.Node, error) { From 26b77a543d5855ad0a8c74c0813adda840e78d99 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 18 Oct 2024 21:41:02 +0200 Subject: [PATCH 3/9] archiver: use correct filepath in fileSaver for vss When using the VSS FS, then `f.Name()` contained the filename in the snapshot. This caused a double mapping when calling NodeFromFileInfo. --- internal/archiver/file_saver.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/archiver/file_saver.go b/internal/archiver/file_saver.go index b9d07434a..dccaa9442 100644 --- a/internal/archiver/file_saver.go +++ b/internal/archiver/file_saver.go @@ -156,7 +156,7 @@ func (s *fileSaver) saveFile(ctx context.Context, chnker *chunker.Chunker, snPat debug.Log("%v", snPath) - node, err := s.NodeFromFileInfo(snPath, f.Name(), fi, false) + node, err := s.NodeFromFileInfo(snPath, target, fi, false) if err != nil { _ = f.Close() completeError(err) From 352605d9f0b23ec60bcf53a96ac2a39661f1e48a Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 18 Oct 2024 21:43:23 +0200 Subject: [PATCH 4/9] fs: remove file.Name() from interface The only user was archiver.fileSaver. --- internal/fs/fs_reader.go | 6 +----- internal/fs/interface.go | 1 - 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/internal/fs/fs_reader.go b/internal/fs/fs_reader.go index c2bf23bb7..97d4e1660 100644 --- a/internal/fs/fs_reader.go +++ b/internal/fs/fs_reader.go @@ -232,7 +232,7 @@ func (r *readerFile) Close() error { var _ File = &readerFile{} // fakeFile implements all File methods, but only returns errors for anything -// except Stat() and Name(). +// except Stat() type fakeFile struct { name string os.FileInfo @@ -257,10 +257,6 @@ func (f fakeFile) Stat() (os.FileInfo, error) { return f.FileInfo, nil } -func (f fakeFile) Name() string { - return f.name -} - // fakeDir implements Readdirnames and Readdir, everything else is delegated to fakeFile. type fakeDir struct { entries []os.FileInfo diff --git a/internal/fs/interface.go b/internal/fs/interface.go index 0bb3029dc..2967429c0 100644 --- a/internal/fs/interface.go +++ b/internal/fs/interface.go @@ -34,5 +34,4 @@ type File interface { Readdirnames(n int) ([]string, error) Stat() (os.FileInfo, error) - Name() string } From ca79cb92e3bce9b0c89b028595490087b000e183 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 18 Oct 2024 19:45:48 +0200 Subject: [PATCH 5/9] fs/vss: test that vss functions actually read from snapshot --- internal/fs/fs_local_vss_test.go | 58 ++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/internal/fs/fs_local_vss_test.go b/internal/fs/fs_local_vss_test.go index a59882381..f1a043118 100644 --- a/internal/fs/fs_local_vss_test.go +++ b/internal/fs/fs_local_vss_test.go @@ -5,13 +5,18 @@ package fs import ( "fmt" + "io" + "os" + "path/filepath" "regexp" + "runtime" "strings" "testing" "time" ole "github.com/go-ole/go-ole" "github.com/restic/restic/internal/options" + rtest "github.com/restic/restic/internal/test" ) func matchStrings(ptrs []string, strs []string) bool { @@ -284,3 +289,56 @@ func TestParseProvider(t *testing.T) { }) } } + +func TestVSSFS(t *testing.T) { + if runtime.GOOS != "windows" || HasSufficientPrivilegesForVSS() != nil { + t.Skip("vss fs test can only be run on windows with admin privileges") + } + + cfg, err := ParseVSSConfig(options.Options{}) + rtest.OK(t, err) + + errorHandler := func(item string, err error) { + t.Fatalf("unexpected error (%v)", err) + } + messageHandler := func(msg string, args ...interface{}) { + if strings.HasPrefix(msg, "creating VSS snapshot for") || strings.HasPrefix(msg, "successfully created snapshot") { + return + } + t.Fatalf("unexpected message (%s)", fmt.Sprintf(msg, args)) + } + + localVss := NewLocalVss(errorHandler, messageHandler, cfg) + defer localVss.DeleteSnapshots() + + tempdir := t.TempDir() + tempfile := filepath.Join(tempdir, "file") + rtest.OK(t, os.WriteFile(tempfile, []byte("example"), 0o600)) + + // trigger snapshot creation and + // capture FI while file still exists (should already be within the snapshot) + origFi, err := localVss.Stat(tempfile) + rtest.OK(t, err) + + // remove original file + rtest.OK(t, os.Remove(tempfile)) + + statFi, err := localVss.Stat(tempfile) + rtest.OK(t, err) + rtest.Equals(t, origFi.Mode(), statFi.Mode()) + + lstatFi, err := localVss.Lstat(tempfile) + rtest.OK(t, err) + rtest.Equals(t, origFi.Mode(), lstatFi.Mode()) + + f, err := localVss.OpenFile(tempfile, os.O_RDONLY, 0) + rtest.OK(t, err) + data, err := io.ReadAll(f) + rtest.OK(t, err) + rtest.Equals(t, "example", string(data), "unexpected file content") + rtest.OK(t, f.Close()) + + node, err := localVss.NodeFromFileInfo(tempfile, statFi, false) + rtest.OK(t, err) + rtest.Equals(t, node.Mode, statFi.Mode()) +} From 9f206601af85bb9e88ad3acdbf7b766fe5f52fa4 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 18 Oct 2024 20:05:32 +0200 Subject: [PATCH 6/9] backup: test that vss backups work if underlying data was removed --- cmd/restic/cmd_backup.go | 5 +++ cmd/restic/cmd_backup_integration_test.go | 47 +++++++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/cmd/restic/cmd_backup.go b/cmd/restic/cmd_backup.go index 107e8bbe0..b7eed1318 100644 --- a/cmd/restic/cmd_backup.go +++ b/cmd/restic/cmd_backup.go @@ -97,6 +97,7 @@ type BackupOptions struct { } var backupOptions BackupOptions +var backupFSTestHook func(fs fs.FS) fs.FS // ErrInvalidSourceData is used to report an incomplete backup var ErrInvalidSourceData = errors.New("at least one source file could not be read") @@ -582,6 +583,10 @@ func runBackup(ctx context.Context, opts BackupOptions, gopts GlobalOptions, ter targets = []string{filename} } + if backupFSTestHook != nil { + targetFS = backupFSTestHook(targetFS) + } + // rejectFuncs collect functions that can reject items from the backup based on path and file info rejectFuncs, err := collectRejectFuncs(opts, targets, targetFS) if err != nil { diff --git a/cmd/restic/cmd_backup_integration_test.go b/cmd/restic/cmd_backup_integration_test.go index 5e00b84b0..cc6a2ca22 100644 --- a/cmd/restic/cmd_backup_integration_test.go +++ b/cmd/restic/cmd_backup_integration_test.go @@ -111,6 +111,53 @@ func TestBackupWithRelativePath(t *testing.T) { rtest.Assert(t, latestSn.Parent != nil && latestSn.Parent.Equal(firstSnapshotID), "second snapshot selected unexpected parent %v instead of %v", latestSn.Parent, firstSnapshotID) } +type vssDeleteOriginalFS struct { + fs.FS + testdata string + hasRemoved bool +} + +func (f *vssDeleteOriginalFS) Lstat(name string) (os.FileInfo, error) { + if !f.hasRemoved { + // call Lstat to trigger snapshot creation + _, _ = f.FS.Lstat(name) + // nuke testdata + if err := os.RemoveAll(f.testdata); err != nil { + return nil, err + } + f.hasRemoved = true + } + return f.FS.Lstat(name) +} + +func TestBackupVSS(t *testing.T) { + if runtime.GOOS != "windows" || fs.HasSufficientPrivilegesForVSS() != nil { + t.Skip("vss fs test can only be run on windows with admin privileges") + } + + env, cleanup := withTestEnvironment(t) + defer cleanup() + + testSetupBackupData(t, env) + opts := BackupOptions{UseFsSnapshot: true} + + var testFS *vssDeleteOriginalFS + backupFSTestHook = func(fs fs.FS) fs.FS { + testFS = &vssDeleteOriginalFS{ + FS: fs, + testdata: env.testdata, + } + return testFS + } + defer func() { + backupFSTestHook = nil + }() + + testRunBackup(t, filepath.Dir(env.testdata), []string{"testdata"}, opts, env.gopts) + testListSnapshots(t, env.gopts, 1) + rtest.Equals(t, true, testFS.hasRemoved, "testdata was not removed") +} + func TestBackupParentSelection(t *testing.T) { env, cleanup := withTestEnvironment(t) defer cleanup() From fc6f1b4b068fd7392ffc9dd24a3de9dbc414e2b2 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 18 Oct 2024 21:18:22 +0200 Subject: [PATCH 7/9] redirect test log output to t.Log() --- cmd/restic/integration_helpers_test.go | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/cmd/restic/integration_helpers_test.go b/cmd/restic/integration_helpers_test.go index 978deab3d..8ae3bb78a 100644 --- a/cmd/restic/integration_helpers_test.go +++ b/cmd/restic/integration_helpers_test.go @@ -9,6 +9,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "sync" "testing" @@ -168,6 +169,16 @@ type testEnvironment struct { gopts GlobalOptions } +type logOutputter struct { + t testing.TB +} + +func (l *logOutputter) Write(p []byte) (n int, err error) { + l.t.Helper() + l.t.Log(strings.TrimSuffix(string(p), "\n")) + return len(p), nil +} + // withTestEnvironment creates a test environment and returns a cleanup // function which removes it. func withTestEnvironment(t testing.TB) (env *testEnvironment, cleanup func()) { @@ -200,8 +211,11 @@ func withTestEnvironment(t testing.TB) (env *testEnvironment, cleanup func()) { Quiet: true, CacheDir: env.cache, password: rtest.TestPassword, - stdout: os.Stdout, - stderr: os.Stderr, + // stdout and stderr are written to by Warnf etc. That is the written data + // usually consists of one or multiple lines and therefore can be handled well + // by t.Log. + stdout: &logOutputter{t}, + stderr: &logOutputter{t}, extended: make(options.Options), // replace this hook with "nil" if listing a filetype more than once is necessary From e1faf7b18cedbe9fb1832017a064a1649fdea7f3 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 18 Oct 2024 22:08:10 +0200 Subject: [PATCH 8/9] backup: work around file deletion error in test --- cmd/restic/cmd_backup_integration_test.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/cmd/restic/cmd_backup_integration_test.go b/cmd/restic/cmd_backup_integration_test.go index cc6a2ca22..5926fdd54 100644 --- a/cmd/restic/cmd_backup_integration_test.go +++ b/cmd/restic/cmd_backup_integration_test.go @@ -8,6 +8,7 @@ import ( "path/filepath" "runtime" "testing" + "time" "github.com/restic/restic/internal/fs" "github.com/restic/restic/internal/restic" @@ -122,7 +123,17 @@ func (f *vssDeleteOriginalFS) Lstat(name string) (os.FileInfo, error) { // call Lstat to trigger snapshot creation _, _ = f.FS.Lstat(name) // nuke testdata - if err := os.RemoveAll(f.testdata); err != nil { + var err error + for i := 0; i < 3; i++ { + // The CI sometimes runs into "The process cannot access the file because it is being used by another process" errors + // thus try a few times to remove the data + err = os.RemoveAll(f.testdata) + if err == nil { + break + } + time.Sleep(10 * time.Millisecond) + } + if err != nil { return nil, err } f.hasRemoved = true From ec43594003ec4c04a25847cea399fe54386bcc5c Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 18 Oct 2024 22:36:03 +0200 Subject: [PATCH 9/9] add vss metadata changelog --- changelog/unreleased/issue-5063 | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 changelog/unreleased/issue-5063 diff --git a/changelog/unreleased/issue-5063 b/changelog/unreleased/issue-5063 new file mode 100644 index 000000000..95048ec58 --- /dev/null +++ b/changelog/unreleased/issue-5063 @@ -0,0 +1,10 @@ +Bugfix: Correctly `backup` extended metadata when using VSS on Windows + +On Windows, when creating a backup using the `--use-fs-snapshot` option, +then the extended metadata was not read from the filesystem snapshot. This +could result in errors when files have been removed in the meantime. + +This issue has been resolved. + +https://github.com/restic/restic/issues/5063 +https://github.com/restic/restic/pull/5097