From f8031561f276e7ffc1ebf8ab6f282d18bf3f324c Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 2 Nov 2024 17:41:40 +0100 Subject: [PATCH 01/10] archiver: deduplicate error filtering --- internal/archiver/archiver.go | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/internal/archiver/archiver.go b/internal/archiver/archiver.go index f7a9f275009..efa0d2945dc 100644 --- a/internal/archiver/archiver.go +++ b/internal/archiver/archiver.go @@ -435,6 +435,13 @@ func (arch *Archiver) save(ctx context.Context, snPath, target string, previous return futureNode{}, false, err } + filterError := func(err error) (futureNode, bool, error) { + err = arch.error(abstarget, err) + if err != nil { + return futureNode{}, false, errors.WithStack(err) + } + return futureNode{}, true, nil + } // exclude files by path before running Lstat to reduce number of lstat calls if !arch.SelectByName(abstarget) { debug.Log("%v is excluded by path", target) @@ -445,11 +452,7 @@ func (arch *Archiver) save(ctx context.Context, snPath, target string, previous fi, err := arch.FS.Lstat(target) if err != nil { debug.Log("lstat() for %v returned error: %v", target, err) - err = arch.error(abstarget, err) - if err != nil { - return futureNode{}, false, errors.WithStack(err) - } - return futureNode{}, true, nil + return filterError(err) } if !arch.Select(abstarget, fi, arch.FS) { debug.Log("%v is excluded", target) @@ -497,33 +500,21 @@ func (arch *Archiver) save(ctx context.Context, snPath, target string, previous file, err := arch.FS.OpenFile(target, fs.O_RDONLY|fs.O_NOFOLLOW, 0) if err != nil { debug.Log("Openfile() for %v returned error: %v", target, err) - err = arch.error(abstarget, err) - if err != nil { - return futureNode{}, false, errors.WithStack(err) - } - return futureNode{}, true, nil + return filterError(err) } fi, err = file.Stat() if err != nil { debug.Log("stat() on opened file %v returned error: %v", target, err) _ = file.Close() - err = arch.error(abstarget, err) - if err != nil { - return futureNode{}, false, errors.WithStack(err) - } - return futureNode{}, true, nil + return filterError(err) } // make sure it's still a file if !fi.Mode().IsRegular() { - err = errors.Errorf("file %v changed type, refusing to archive", fi.Name()) + err = errors.Errorf("file %v changed type, refusing to archive", target) _ = file.Close() - err = arch.error(abstarget, err) - if err != nil { - return futureNode{}, false, err - } - return futureNode{}, true, nil + return filterError(err) } // Save will close the file, we don't need to do that From b402e8a6fc7685c6bf8061ff0917ee5735bc19c2 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 2 Nov 2024 17:44:55 +0100 Subject: [PATCH 02/10] fs: stricter enforcement to only call readdir on a directory Use O_DIRECTORY to prevent opening any other than a directory in readdirnames. --- internal/fs/const_unix.go | 3 +++ internal/fs/const_windows.go | 5 +++++ internal/fs/file.go | 5 +++-- internal/fs/file_unix_test.go | 22 ++++++++++++++++++++++ 4 files changed, 33 insertions(+), 2 deletions(-) create mode 100644 internal/fs/file_unix_test.go diff --git a/internal/fs/const_unix.go b/internal/fs/const_unix.go index fe84cda176d..e570c255370 100644 --- a/internal/fs/const_unix.go +++ b/internal/fs/const_unix.go @@ -7,3 +7,6 @@ import "syscall" // O_NOFOLLOW instructs the kernel to not follow symlinks when opening a file. const O_NOFOLLOW int = syscall.O_NOFOLLOW + +// O_DIRECTORY instructs the kernel to only open directories. +const O_DIRECTORY int = syscall.O_DIRECTORY diff --git a/internal/fs/const_windows.go b/internal/fs/const_windows.go index f1b263a54a4..4c29e0b9d59 100644 --- a/internal/fs/const_windows.go +++ b/internal/fs/const_windows.go @@ -3,5 +3,10 @@ package fs +// TODO honor flags when opening files + // O_NOFOLLOW is a noop on Windows. const O_NOFOLLOW int = 0 + +// O_DIRECTORY is a noop on Windows. +const O_DIRECTORY int = 0 diff --git a/internal/fs/file.go b/internal/fs/file.go index 8d60ed15978..c60625a07ab 100644 --- a/internal/fs/file.go +++ b/internal/fs/file.go @@ -64,9 +64,10 @@ func ResetPermissions(path string) error { return nil } -// Readdirnames returns a list of file in a directory. Flags are passed to fs.OpenFile. O_RDONLY is implied. +// Readdirnames returns a list of file in a directory. Flags are passed to fs.OpenFile. +// O_RDONLY and O_DIRECTORY are implied. func Readdirnames(filesystem FS, dir string, flags int) ([]string, error) { - f, err := filesystem.OpenFile(dir, O_RDONLY|flags, 0) + f, err := filesystem.OpenFile(dir, O_RDONLY|O_DIRECTORY|flags, 0) if err != nil { return nil, fmt.Errorf("openfile for readdirnames failed: %w", err) } diff --git a/internal/fs/file_unix_test.go b/internal/fs/file_unix_test.go new file mode 100644 index 00000000000..00d68abb8e5 --- /dev/null +++ b/internal/fs/file_unix_test.go @@ -0,0 +1,22 @@ +//go:build unix + +package fs + +import ( + "path/filepath" + "syscall" + "testing" + + "github.com/restic/restic/internal/errors" + rtest "github.com/restic/restic/internal/test" +) + +func TestReaddirnamesFifo(t *testing.T) { + // should not block when reading from a fifo instead of a directory + tempdir := t.TempDir() + fifoFn := filepath.Join(tempdir, "fifo") + rtest.OK(t, mkfifo(fifoFn, 0o600)) + + _, err := Readdirnames(&Local{}, fifoFn, 0) + rtest.Assert(t, errors.Is(err, syscall.ENOTDIR), "unexpected error %v", err) +} From 623ba92b986c856981f30d0f7351668e9532c8f0 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 2 Nov 2024 17:47:54 +0100 Subject: [PATCH 03/10] fs: drop unused permission parameter from OpenFile --- internal/archiver/archiver.go | 2 +- internal/archiver/archiver_test.go | 16 ++++++++-------- internal/archiver/exclude.go | 2 +- internal/archiver/file_saver_test.go | 2 +- internal/fs/file.go | 2 +- internal/fs/fs_local.go | 4 ++-- internal/fs/fs_local_vss.go | 4 ++-- internal/fs/fs_local_vss_test.go | 2 +- internal/fs/fs_reader.go | 2 +- internal/fs/fs_reader_test.go | 8 ++++---- internal/fs/fs_track.go | 4 ++-- internal/fs/interface.go | 2 +- 12 files changed, 25 insertions(+), 25 deletions(-) diff --git a/internal/archiver/archiver.go b/internal/archiver/archiver.go index efa0d2945dc..fb03c88fced 100644 --- a/internal/archiver/archiver.go +++ b/internal/archiver/archiver.go @@ -497,7 +497,7 @@ func (arch *Archiver) save(ctx context.Context, snPath, target string, previous // reopen file and do an fstat() on the open file to check it is still // a file (and has not been exchanged for e.g. a symlink) - file, err := arch.FS.OpenFile(target, fs.O_RDONLY|fs.O_NOFOLLOW, 0) + file, err := arch.FS.OpenFile(target, fs.O_RDONLY|fs.O_NOFOLLOW) if err != nil { debug.Log("Openfile() for %v returned error: %v", target, err) return filterError(err) diff --git a/internal/archiver/archiver_test.go b/internal/archiver/archiver_test.go index 97d27dc649f..ff072fa3605 100644 --- a/internal/archiver/archiver_test.go +++ b/internal/archiver/archiver_test.go @@ -76,7 +76,7 @@ func saveFile(t testing.TB, repo archiverRepo, filename string, filesystem fs.FS startCallback = true } - file, err := arch.FS.OpenFile(filename, fs.O_RDONLY|fs.O_NOFOLLOW, 0) + file, err := arch.FS.OpenFile(filename, fs.O_RDONLY|fs.O_NOFOLLOW) if err != nil { t.Fatal(err) } @@ -1665,8 +1665,8 @@ type MockFS struct { bytesRead map[string]int // tracks bytes read from all opened files } -func (m *MockFS) OpenFile(name string, flag int, perm os.FileMode) (fs.File, error) { - f, err := m.FS.OpenFile(name, flag, perm) +func (m *MockFS) OpenFile(name string, flag int) (fs.File, error) { + f, err := m.FS.OpenFile(name, flag) if err != nil { return f, err } @@ -2056,12 +2056,12 @@ type TrackFS struct { m sync.Mutex } -func (m *TrackFS) OpenFile(name string, flag int, perm os.FileMode) (fs.File, error) { +func (m *TrackFS) OpenFile(name string, flag int) (fs.File, error) { m.m.Lock() m.opened[name]++ m.m.Unlock() - return m.FS.OpenFile(name, flag, perm) + return m.FS.OpenFile(name, flag) } type failSaveRepo struct { @@ -2228,9 +2228,9 @@ func (fs *StatFS) Lstat(name string) (os.FileInfo, error) { return fs.FS.Lstat(name) } -func (fs *StatFS) OpenFile(name string, flags int, perm os.FileMode) (fs.File, error) { +func (fs *StatFS) OpenFile(name string, flags int) (fs.File, error) { if fi, ok := fs.OverrideLstat[fixpath(name)]; ok { - f, err := fs.FS.OpenFile(name, flags, perm) + f, err := fs.FS.OpenFile(name, flags) if err != nil { return nil, err } @@ -2242,7 +2242,7 @@ func (fs *StatFS) OpenFile(name string, flags int, perm os.FileMode) (fs.File, e return wrappedFile, nil } - return fs.FS.OpenFile(name, flags, perm) + return fs.FS.OpenFile(name, flags) } type fileStat struct { diff --git a/internal/archiver/exclude.go b/internal/archiver/exclude.go index 1e855fc3a37..54ced788ae0 100644 --- a/internal/archiver/exclude.go +++ b/internal/archiver/exclude.go @@ -153,7 +153,7 @@ func isDirExcludedByFile(dir, tagFilename, header string, fs fs.FS, warnf func(m // From this stage, errors mean tagFilename exists but it is malformed. // Warnings will be generated so that the user is informed that the // indented ignore-action is not performed. - f, err := fs.OpenFile(tf, os.O_RDONLY, 0) + f, err := fs.OpenFile(tf, os.O_RDONLY) if err != nil { warnf("could not open exclusion tagfile: %v", err) return false diff --git a/internal/archiver/file_saver_test.go b/internal/archiver/file_saver_test.go index 5b17eca3797..069cdc17152 100644 --- a/internal/archiver/file_saver_test.go +++ b/internal/archiver/file_saver_test.go @@ -72,7 +72,7 @@ func TestFileSaver(t *testing.T) { var results []futureNode for _, filename := range files { - f, err := testFs.OpenFile(filename, os.O_RDONLY, 0) + f, err := testFs.OpenFile(filename, os.O_RDONLY) if err != nil { t.Fatal(err) } diff --git a/internal/fs/file.go b/internal/fs/file.go index c60625a07ab..fa395b62845 100644 --- a/internal/fs/file.go +++ b/internal/fs/file.go @@ -67,7 +67,7 @@ func ResetPermissions(path string) error { // Readdirnames returns a list of file in a directory. Flags are passed to fs.OpenFile. // O_RDONLY and O_DIRECTORY are implied. func Readdirnames(filesystem FS, dir string, flags int) ([]string, error) { - f, err := filesystem.OpenFile(dir, O_RDONLY|O_DIRECTORY|flags, 0) + f, err := filesystem.OpenFile(dir, O_RDONLY|O_DIRECTORY|flags) if err != nil { return nil, fmt.Errorf("openfile for readdirnames failed: %w", err) } diff --git a/internal/fs/fs_local.go b/internal/fs/fs_local.go index 5fac88dbb8a..045edf02f60 100644 --- a/internal/fs/fs_local.go +++ b/internal/fs/fs_local.go @@ -25,8 +25,8 @@ func (fs Local) VolumeName(path string) string { // (O_RDONLY etc.) and perm, (0666 etc.) if applicable. If successful, // methods on the returned File can be used for I/O. // If there is an error, it will be of type *PathError. -func (fs Local) OpenFile(name string, flag int, perm os.FileMode) (File, error) { - f, err := os.OpenFile(fixpath(name), flag, perm) +func (fs Local) OpenFile(name string, flag int) (File, error) { + f, err := os.OpenFile(fixpath(name), flag, 0) if err != nil { return nil, err } diff --git a/internal/fs/fs_local_vss.go b/internal/fs/fs_local_vss.go index dcbda2a847f..aa9f2b89de0 100644 --- a/internal/fs/fs_local_vss.go +++ b/internal/fs/fs_local_vss.go @@ -127,8 +127,8 @@ 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 fs.FS.OpenFile(fs.snapshotPath(name), flag, perm) +func (fs *LocalVss) OpenFile(name string, flag int) (File, error) { + return fs.FS.OpenFile(fs.snapshotPath(name), flag) } // Stat wraps the Stat method of the underlying file system. diff --git a/internal/fs/fs_local_vss_test.go b/internal/fs/fs_local_vss_test.go index f1a0431185d..7856767ba78 100644 --- a/internal/fs/fs_local_vss_test.go +++ b/internal/fs/fs_local_vss_test.go @@ -331,7 +331,7 @@ func TestVSSFS(t *testing.T) { rtest.OK(t, err) rtest.Equals(t, origFi.Mode(), lstatFi.Mode()) - f, err := localVss.OpenFile(tempfile, os.O_RDONLY, 0) + f, err := localVss.OpenFile(tempfile, os.O_RDONLY) rtest.OK(t, err) data, err := io.ReadAll(f) rtest.OK(t, err) diff --git a/internal/fs/fs_reader.go b/internal/fs/fs_reader.go index 97d4e1660f4..ed8b9a3470c 100644 --- a/internal/fs/fs_reader.go +++ b/internal/fs/fs_reader.go @@ -54,7 +54,7 @@ func (fs *Reader) fi() os.FileInfo { // (O_RDONLY etc.) and perm, (0666 etc.) if applicable. If successful, // methods on the returned File can be used for I/O. // If there is an error, it will be of type *os.PathError. -func (fs *Reader) OpenFile(name string, flag int, _ os.FileMode) (f File, err error) { +func (fs *Reader) OpenFile(name string, flag int) (f File, err error) { if flag & ^(O_RDONLY|O_NOFOLLOW) != 0 { return nil, pathError("open", name, fmt.Errorf("invalid combination of flags 0x%x", flag)) diff --git a/internal/fs/fs_reader_test.go b/internal/fs/fs_reader_test.go index 442912fe3af..e7020bc9dc5 100644 --- a/internal/fs/fs_reader_test.go +++ b/internal/fs/fs_reader_test.go @@ -16,7 +16,7 @@ import ( ) func verifyFileContentOpenFile(t testing.TB, fs FS, filename string, want []byte) { - f, err := fs.OpenFile(filename, O_RDONLY, 0) + f, err := fs.OpenFile(filename, O_RDONLY) if err != nil { t.Fatal(err) } @@ -37,7 +37,7 @@ func verifyFileContentOpenFile(t testing.TB, fs FS, filename string, want []byte } func verifyDirectoryContents(t testing.TB, fs FS, dir string, want []string) { - f, err := fs.OpenFile(dir, os.O_RDONLY, 0) + f, err := fs.OpenFile(dir, os.O_RDONLY) if err != nil { t.Fatal(err) } @@ -123,7 +123,7 @@ func TestFSReader(t *testing.T) { { name: "file/Stat", f: func(t *testing.T, fs FS) { - f, err := fs.OpenFile(filename, os.O_RDONLY, 0) + f, err := fs.OpenFile(filename, os.O_RDONLY) if err != nil { t.Fatal(err) } @@ -295,7 +295,7 @@ func TestFSReaderMinFileSize(t *testing.T) { AllowEmptyFile: test.allowEmpty, } - f, err := fs.OpenFile("testfile", os.O_RDONLY, 0) + f, err := fs.OpenFile("testfile", os.O_RDONLY) if err != nil { t.Fatal(err) } diff --git a/internal/fs/fs_track.go b/internal/fs/fs_track.go index 366bbee762a..9912ac45bb9 100644 --- a/internal/fs/fs_track.go +++ b/internal/fs/fs_track.go @@ -16,8 +16,8 @@ type Track struct { } // OpenFile wraps the OpenFile method of the underlying file system. -func (fs Track) OpenFile(name string, flag int, perm os.FileMode) (File, error) { - f, err := fs.FS.OpenFile(fixpath(name), flag, perm) +func (fs Track) OpenFile(name string, flag int) (File, error) { + f, err := fs.FS.OpenFile(fixpath(name), flag) if err != nil { return nil, err } diff --git a/internal/fs/interface.go b/internal/fs/interface.go index 2967429c0d5..58744bd1dc1 100644 --- a/internal/fs/interface.go +++ b/internal/fs/interface.go @@ -9,7 +9,7 @@ import ( // FS bundles all methods needed for a file system. type FS interface { - OpenFile(name string, flag int, perm os.FileMode) (File, error) + OpenFile(name string, flag int) (File, error) Stat(name string) (os.FileInfo, error) Lstat(name string) (os.FileInfo, error) DeviceID(fi os.FileInfo) (deviceID uint64, err error) From 2f2ce9add25c86bea2d2b8f235ae257f7d6a6ba7 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 2 Nov 2024 18:09:39 +0100 Subject: [PATCH 04/10] fs: remove Stat from FS interface --- internal/archiver/archiver.go | 57 ++++++++++++++++++-------------- internal/fs/fs_local.go | 6 ---- internal/fs/fs_local_vss.go | 5 --- internal/fs/fs_local_vss_test.go | 13 +++----- internal/fs/fs_reader.go | 6 ---- internal/fs/interface.go | 1 - 6 files changed, 37 insertions(+), 51 deletions(-) diff --git a/internal/archiver/archiver.go b/internal/archiver/archiver.go index fb03c88fced..e88c15bd970 100644 --- a/internal/archiver/archiver.go +++ b/internal/archiver/archiver.go @@ -605,22 +605,6 @@ func join(elem ...string) string { return path.Join(elem...) } -// statDir returns the file info for the directory. Symbolic links are -// resolved. If the target directory is not a directory, an error is returned. -func (arch *Archiver) statDir(dir string) (os.FileInfo, error) { - fi, err := arch.FS.Stat(dir) - if err != nil { - return nil, errors.WithStack(err) - } - - tpe := fi.Mode() & (os.ModeType | os.ModeCharDevice) - if tpe != os.ModeDir { - return fi, errors.Errorf("path is not a directory: %v", dir) - } - - return fi, nil -} - // saveTree stores a Tree in the repo, returned is the tree. snPath is the path // within the current snapshot. func (arch *Archiver) saveTree(ctx context.Context, snPath string, atree *tree, previous *restic.Tree, complete fileCompleteFunc) (futureNode, int, error) { @@ -631,15 +615,8 @@ func (arch *Archiver) saveTree(ctx context.Context, snPath string, atree *tree, return futureNode{}, 0, errors.Errorf("FileInfoPath for %v is empty", snPath) } - fi, err := arch.statDir(atree.FileInfoPath) - if err != nil { - return futureNode{}, 0, err - } - - debug.Log("%v, dir node data loaded from %v", snPath, atree.FileInfoPath) - // in some cases reading xattrs for directories above the backup source is not allowed - // thus ignore errors for such folders. - node, err = arch.nodeFromFileInfo(snPath, atree.FileInfoPath, fi, true) + var err error + node, err = arch.dirPathToNode(snPath, atree.FileInfoPath) if err != nil { return futureNode{}, 0, err } @@ -710,6 +687,36 @@ func (arch *Archiver) saveTree(ctx context.Context, snPath string, atree *tree, return fn, len(nodes), nil } +func (arch *Archiver) dirPathToNode(snPath, target string) (node *restic.Node, err error) { + meta, err := arch.FS.OpenFile(target, fs.O_RDONLY) + if err != nil { + return nil, err + } + defer func() { + cerr := meta.Close() + if err == nil { + err = cerr + } + }() + + debug.Log("%v, reading dir node data from %v", snPath, target) + fi, err := meta.Stat() + if err != nil { + return nil, errors.WithStack(err) + } + + // in some cases reading xattrs for directories above the backup source is not allowed + // thus ignore errors for such folders. + node, err = arch.nodeFromFileInfo(snPath, target, fi, true) + if err != nil { + return nil, err + } + if node.Type != restic.NodeTypeDir { + return nil, errors.Errorf("path is not a directory: %v", target) + } + return node, err +} + // resolveRelativeTargets replaces targets that only contain relative // directories ("." or "../../") with the contents of the directory. Each // element of target is processed with fs.Clean(). diff --git a/internal/fs/fs_local.go b/internal/fs/fs_local.go index 045edf02f60..4f8b670900c 100644 --- a/internal/fs/fs_local.go +++ b/internal/fs/fs_local.go @@ -34,12 +34,6 @@ func (fs Local) OpenFile(name string, flag int) (File, error) { return f, nil } -// Stat returns a FileInfo describing the named file. If there is an error, it -// will be of type *PathError. -func (fs Local) Stat(name string) (os.FileInfo, error) { - return os.Stat(fixpath(name)) -} - // Lstat returns the FileInfo structure describing the named file. // If the file is a symbolic link, the returned FileInfo // describes the symbolic link. Lstat makes no attempt to follow the link. diff --git a/internal/fs/fs_local_vss.go b/internal/fs/fs_local_vss.go index aa9f2b89de0..e9bc66657c9 100644 --- a/internal/fs/fs_local_vss.go +++ b/internal/fs/fs_local_vss.go @@ -131,11 +131,6 @@ func (fs *LocalVss) OpenFile(name string, flag int) (File, error) { return fs.FS.OpenFile(fs.snapshotPath(name), flag) } -// Stat wraps the Stat method of the underlying file system. -func (fs *LocalVss) Stat(name string) (os.FileInfo, error) { - 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 fs.FS.Lstat(fs.snapshotPath(name)) diff --git a/internal/fs/fs_local_vss_test.go b/internal/fs/fs_local_vss_test.go index 7856767ba78..db8d4b13385 100644 --- a/internal/fs/fs_local_vss_test.go +++ b/internal/fs/fs_local_vss_test.go @@ -317,16 +317,12 @@ func TestVSSFS(t *testing.T) { // trigger snapshot creation and // capture FI while file still exists (should already be within the snapshot) - origFi, err := localVss.Stat(tempfile) + origFi, err := localVss.Lstat(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()) @@ -336,9 +332,10 @@ func TestVSSFS(t *testing.T) { 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) + node, err := f.ToNode(false) rtest.OK(t, err) - rtest.Equals(t, node.Mode, statFi.Mode()) + rtest.Equals(t, node.Mode, lstatFi.Mode()) + + rtest.OK(t, f.Close()) } diff --git a/internal/fs/fs_reader.go b/internal/fs/fs_reader.go index ed8b9a3470c..a4efa8deacb 100644 --- a/internal/fs/fs_reader.go +++ b/internal/fs/fs_reader.go @@ -81,12 +81,6 @@ func (fs *Reader) OpenFile(name string, flag int) (f File, err error) { return nil, pathError("open", name, syscall.ENOENT) } -// Stat returns a FileInfo describing the named file. If there is an error, it -// will be of type *os.PathError. -func (fs *Reader) Stat(name string) (os.FileInfo, error) { - return fs.Lstat(name) -} - // Lstat returns the FileInfo structure describing the named file. // If the file is a symbolic link, the returned FileInfo // describes the symbolic link. Lstat makes no attempt to follow the link. diff --git a/internal/fs/interface.go b/internal/fs/interface.go index 58744bd1dc1..899888fb085 100644 --- a/internal/fs/interface.go +++ b/internal/fs/interface.go @@ -10,7 +10,6 @@ import ( // FS bundles all methods needed for a file system. type FS interface { OpenFile(name string, flag int) (File, error) - Stat(name string) (os.FileInfo, error) Lstat(name string) (os.FileInfo, error) DeviceID(fi os.FileInfo) (deviceID uint64, err error) ExtendedStat(fi os.FileInfo) ExtendedFileInfo From 48dbefc37e1c5b16ee6cd97fed272a8372b9cbdd Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 2 Nov 2024 20:27:38 +0100 Subject: [PATCH 05/10] fs / archiver: convert to handle based interface The actual implementation still relies on file paths, but with the abstraction layer in place, an FS implementation can ensure atomic file accesses in the future. --- internal/archiver/archiver.go | 86 ++++++---- internal/archiver/archiver_test.go | 207 ++++++++++++------------ internal/archiver/archiver_unix_test.go | 8 +- internal/archiver/exclude.go | 8 +- internal/archiver/file_saver.go | 13 +- internal/archiver/file_saver_test.go | 15 +- internal/fs/file.go | 2 +- internal/fs/fs_local.go | 110 +++++++++++-- internal/fs/fs_local_vss.go | 11 +- internal/fs/fs_local_vss_test.go | 2 +- internal/fs/fs_reader.go | 33 ++-- internal/fs/fs_reader_test.go | 8 +- internal/fs/fs_track.go | 6 +- internal/fs/interface.go | 25 ++- internal/fs/node_test.go | 60 ++----- internal/fs/node_unix_test.go | 16 +- internal/fs/node_windows_test.go | 8 +- internal/restic/tree_test.go | 20 +-- 18 files changed, 355 insertions(+), 283 deletions(-) diff --git a/internal/archiver/archiver.go b/internal/archiver/archiver.go index e88c15bd970..ae3edaf44c6 100644 --- a/internal/archiver/archiver.go +++ b/internal/archiver/archiver.go @@ -66,6 +66,11 @@ func (s *ItemStats) Add(other ItemStats) { s.TreeSizeInRepo += other.TreeSizeInRepo } +// ToNoder returns a restic.Node for a File. +type ToNoder interface { + ToNode(ignoreXattrListError bool) (*restic.Node, error) +} + type archiverRepo interface { restic.Loader restic.BlobSaver @@ -257,8 +262,8 @@ func (arch *Archiver) trackItem(item string, previous, current *restic.Node, s I } // nodeFromFileInfo returns the restic node from an os.FileInfo. -func (arch *Archiver) nodeFromFileInfo(snPath, filename string, fi os.FileInfo, ignoreXattrListError bool) (*restic.Node, error) { - node, err := arch.FS.NodeFromFileInfo(filename, fi, ignoreXattrListError) +func (arch *Archiver) nodeFromFileInfo(snPath, filename string, meta ToNoder, ignoreXattrListError bool) (*restic.Node, error) { + node, err := meta.ToNode(ignoreXattrListError) if !arch.WithAtime { node.AccessTime = node.ModTime } @@ -308,20 +313,14 @@ func (arch *Archiver) wrapLoadTreeError(id restic.ID, err error) error { // saveDir stores a directory in the repo and returns the node. snPath is the // path within the current snapshot. -func (arch *Archiver) saveDir(ctx context.Context, snPath string, dir string, fi os.FileInfo, previous *restic.Tree, complete fileCompleteFunc) (d futureNode, err error) { +func (arch *Archiver) saveDir(ctx context.Context, snPath string, dir string, meta fs.File, previous *restic.Tree, complete fileCompleteFunc) (d futureNode, err error) { debug.Log("%v %v", snPath, dir) - treeNode, err := arch.nodeFromFileInfo(snPath, dir, fi, false) + treeNode, names, err := arch.dirToNodeAndEntries(snPath, dir, meta) if err != nil { return futureNode{}, err } - names, err := fs.Readdirnames(arch.FS, dir, fs.O_NOFOLLOW) - if err != nil { - return futureNode{}, err - } - sort.Strings(names) - nodes := make([]futureNode, 0, len(names)) for _, name := range names { @@ -359,6 +358,29 @@ func (arch *Archiver) saveDir(ctx context.Context, snPath string, dir string, fi return fn, nil } +func (arch *Archiver) dirToNodeAndEntries(snPath, dir string, meta fs.File) (node *restic.Node, names []string, err error) { + err = meta.MakeReadable() + if err != nil { + return nil, nil, fmt.Errorf("openfile for readdirnames failed: %w", err) + } + + node, err = arch.nodeFromFileInfo(snPath, dir, meta, false) + if err != nil { + return nil, nil, err + } + if node.Type != restic.NodeTypeDir { + return nil, nil, fmt.Errorf("directory %v changed type, refusing to archive", snPath) + } + + names, err = meta.Readdirnames(-1) + if err != nil { + return nil, nil, fmt.Errorf("readdirnames %v failed: %w", dir, err) + } + sort.Strings(names) + + return node, names, nil +} + // futureNode holds a reference to a channel that returns a FutureNodeResult // or a reference to an already existing result. If the result is available // immediately, then storing a reference directly requires less memory than @@ -448,8 +470,23 @@ func (arch *Archiver) save(ctx context.Context, snPath, target string, previous return futureNode{}, true, nil } + meta, err := arch.FS.OpenFile(target, fs.O_NOFOLLOW, true) + if err != nil { + debug.Log("open metadata for %v returned error: %v", target, err) + return filterError(err) + } + closeFile := true + defer func() { + if closeFile { + cerr := meta.Close() + if err == nil { + err = cerr + } + } + }() + // get file info and run remaining select functions that require file information - fi, err := arch.FS.Lstat(target) + fi, err := meta.Stat() if err != nil { debug.Log("lstat() for %v returned error: %v", target, err) return filterError(err) @@ -470,7 +507,7 @@ func (arch *Archiver) save(ctx context.Context, snPath, target string, previous debug.Log("%v hasn't changed, using old list of blobs", target) arch.trackItem(snPath, previous, previous, ItemStats{}, time.Since(start)) arch.CompleteBlob(previous.Size) - node, err := arch.nodeFromFileInfo(snPath, target, fi, false) + node, err := arch.nodeFromFileInfo(snPath, target, meta, false) if err != nil { return futureNode{}, false, err } @@ -497,28 +534,28 @@ func (arch *Archiver) save(ctx context.Context, snPath, target string, previous // reopen file and do an fstat() on the open file to check it is still // a file (and has not been exchanged for e.g. a symlink) - file, err := arch.FS.OpenFile(target, fs.O_RDONLY|fs.O_NOFOLLOW) + err := meta.MakeReadable() if err != nil { - debug.Log("Openfile() for %v returned error: %v", target, err) + debug.Log("MakeReadable() for %v returned error: %v", target, err) return filterError(err) } - fi, err = file.Stat() + fi, err := meta.Stat() if err != nil { debug.Log("stat() on opened file %v returned error: %v", target, err) - _ = file.Close() return filterError(err) } // make sure it's still a file if !fi.Mode().IsRegular() { err = errors.Errorf("file %v changed type, refusing to archive", target) - _ = file.Close() return filterError(err) } + closeFile = false + // Save will close the file, we don't need to do that - fn = arch.fileSaver.Save(ctx, snPath, target, file, fi, func() { + fn = arch.fileSaver.Save(ctx, snPath, target, meta, func() { arch.StartFile(snPath) }, func() { arch.trackItem(snPath, nil, nil, ItemStats{}, 0) @@ -538,7 +575,7 @@ func (arch *Archiver) save(ctx context.Context, snPath, target string, previous return futureNode{}, false, err } - fn, err = arch.saveDir(ctx, snPath, target, fi, oldSubtree, + fn, err = arch.saveDir(ctx, snPath, target, meta, oldSubtree, func(node *restic.Node, stats ItemStats) { arch.trackItem(snItem, previous, node, stats, time.Since(start)) }) @@ -554,7 +591,7 @@ func (arch *Archiver) save(ctx context.Context, snPath, target string, previous default: debug.Log(" %v other", target) - node, err := arch.nodeFromFileInfo(snPath, target, fi, false) + node, err := arch.nodeFromFileInfo(snPath, target, meta, false) if err != nil { return futureNode{}, false, err } @@ -688,7 +725,7 @@ func (arch *Archiver) saveTree(ctx context.Context, snPath string, atree *tree, } func (arch *Archiver) dirPathToNode(snPath, target string) (node *restic.Node, err error) { - meta, err := arch.FS.OpenFile(target, fs.O_RDONLY) + meta, err := arch.FS.OpenFile(target, 0, true) if err != nil { return nil, err } @@ -700,14 +737,9 @@ func (arch *Archiver) dirPathToNode(snPath, target string) (node *restic.Node, e }() debug.Log("%v, reading dir node data from %v", snPath, target) - fi, err := meta.Stat() - if err != nil { - return nil, errors.WithStack(err) - } - // in some cases reading xattrs for directories above the backup source is not allowed // thus ignore errors for such folders. - node, err = arch.nodeFromFileInfo(snPath, target, fi, true) + node, err = arch.nodeFromFileInfo(snPath, target, meta, true) if err != nil { return nil, err } diff --git a/internal/archiver/archiver_test.go b/internal/archiver/archiver_test.go index ff072fa3605..0b2957bc6d4 100644 --- a/internal/archiver/archiver_test.go +++ b/internal/archiver/archiver_test.go @@ -76,17 +76,12 @@ func saveFile(t testing.TB, repo archiverRepo, filename string, filesystem fs.FS startCallback = true } - file, err := arch.FS.OpenFile(filename, fs.O_RDONLY|fs.O_NOFOLLOW) + file, err := arch.FS.OpenFile(filename, fs.O_NOFOLLOW, false) if err != nil { t.Fatal(err) } - fi, err := file.Stat() - if err != nil { - t.Fatal(err) - } - - res := arch.fileSaver.Save(ctx, "/", filename, file, fi, start, completeReading, complete) + res := arch.fileSaver.Save(ctx, "/", filename, file, start, completeReading, complete) fnr := res.take(ctx) if fnr.err != nil { @@ -556,11 +551,12 @@ func rename(t testing.TB, oldname, newname string) { } } -func nodeFromFI(t testing.TB, fs fs.FS, filename string, fi os.FileInfo) *restic.Node { - node, err := fs.NodeFromFileInfo(filename, fi, false) - if err != nil { - t.Fatal(err) - } +func nodeFromFile(t testing.TB, localFs fs.FS, filename string) *restic.Node { + meta, err := localFs.OpenFile(filename, fs.O_NOFOLLOW, true) + rtest.OK(t, err) + node, err := meta.ToNode(false) + rtest.OK(t, err) + rtest.OK(t, meta.Close()) return node } @@ -688,7 +684,7 @@ func TestFileChanged(t *testing.T) { fs := &fs.Local{} fiBefore := lstat(t, filename) - node := nodeFromFI(t, fs, filename, fiBefore) + node := nodeFromFile(t, fs, filename) if fileChanged(fs, fiBefore, node, 0) { t.Fatalf("unchanged file detected as changed") @@ -729,8 +725,8 @@ func TestFilChangedSpecialCases(t *testing.T) { t.Run("type-change", func(t *testing.T) { fi := lstat(t, filename) - node := nodeFromFI(t, &fs.Local{}, filename, fi) - node.Type = "restic.NodeTypeSymlink" + node := nodeFromFile(t, &fs.Local{}, filename) + node.Type = restic.NodeTypeSymlink if !fileChanged(&fs.Local{}, fi, node, 0) { t.Fatal("node with changed type detected as unchanged") } @@ -834,7 +830,8 @@ func TestArchiverSaveDir(t *testing.T) { wg, ctx := errgroup.WithContext(context.Background()) repo.StartPackUploader(ctx, wg) - arch := New(repo, fs.Track{FS: fs.Local{}}, Options{}) + testFS := fs.Track{FS: fs.Local{}} + arch := New(repo, testFS, Options{}) arch.runWorkers(ctx, wg) arch.summary = &Summary{} @@ -846,15 +843,11 @@ func TestArchiverSaveDir(t *testing.T) { back := rtest.Chdir(t, chdir) defer back() - fi, err := os.Lstat(test.target) - if err != nil { - t.Fatal(err) - } - - ft, err := arch.saveDir(ctx, "/", test.target, fi, nil, nil) - if err != nil { - t.Fatal(err) - } + meta, err := testFS.OpenFile(test.target, fs.O_NOFOLLOW, true) + rtest.OK(t, err) + ft, err := arch.saveDir(ctx, "/", test.target, meta, nil, nil) + rtest.OK(t, err) + rtest.OK(t, meta.Close()) fnr := ft.take(ctx) node, stats := fnr.node, fnr.stats @@ -916,19 +909,16 @@ func TestArchiverSaveDirIncremental(t *testing.T) { wg, ctx := errgroup.WithContext(context.TODO()) repo.StartPackUploader(ctx, wg) - arch := New(repo, fs.Track{FS: fs.Local{}}, Options{}) + testFS := fs.Track{FS: fs.Local{}} + arch := New(repo, testFS, Options{}) arch.runWorkers(ctx, wg) arch.summary = &Summary{} - fi, err := os.Lstat(tempdir) - if err != nil { - t.Fatal(err) - } - - ft, err := arch.saveDir(ctx, "/", tempdir, fi, nil, nil) - if err != nil { - t.Fatal(err) - } + meta, err := testFS.OpenFile(tempdir, fs.O_NOFOLLOW, true) + rtest.OK(t, err) + ft, err := arch.saveDir(ctx, "/", tempdir, meta, nil, nil) + rtest.OK(t, err) + rtest.OK(t, meta.Close()) fnr := ft.take(ctx) node, stats := fnr.node, fnr.stats @@ -1665,8 +1655,8 @@ type MockFS struct { bytesRead map[string]int // tracks bytes read from all opened files } -func (m *MockFS) OpenFile(name string, flag int) (fs.File, error) { - f, err := m.FS.OpenFile(name, flag) +func (m *MockFS) OpenFile(name string, flag int, metadataOnly bool) (fs.File, error) { + f, err := m.FS.OpenFile(name, flag, metadataOnly) if err != nil { return f, err } @@ -2056,12 +2046,12 @@ type TrackFS struct { m sync.Mutex } -func (m *TrackFS) OpenFile(name string, flag int) (fs.File, error) { +func (m *TrackFS) OpenFile(name string, flag int, metadataOnly bool) (fs.File, error) { m.m.Lock() m.opened[name]++ m.m.Unlock() - return m.FS.OpenFile(name, flag) + return m.FS.OpenFile(name, flag, metadataOnly) } type failSaveRepo struct { @@ -2210,48 +2200,39 @@ func snapshot(t testing.TB, repo archiverRepo, fs fs.FS, parent *restic.Snapshot return snapshot, node } -// StatFS allows overwriting what is returned by the Lstat function. -type StatFS struct { +type overrideFS struct { fs.FS - - OverrideLstat map[string]os.FileInfo - OnlyOverrideStat bool + overrideFI os.FileInfo + overrideNode *restic.Node + overrideErr error } -func (fs *StatFS) Lstat(name string) (os.FileInfo, error) { - if !fs.OnlyOverrideStat { - if fi, ok := fs.OverrideLstat[fixpath(name)]; ok { - return fi, nil - } +func (m *overrideFS) OpenFile(name string, flag int, metadataOnly bool) (fs.File, error) { + f, err := m.FS.OpenFile(name, flag, metadataOnly) + if err != nil { + return f, err } - return fs.FS.Lstat(name) -} - -func (fs *StatFS) OpenFile(name string, flags int) (fs.File, error) { - if fi, ok := fs.OverrideLstat[fixpath(name)]; ok { - f, err := fs.FS.OpenFile(name, flags) - if err != nil { - return nil, err - } - - wrappedFile := fileStat{ - File: f, - fi: fi, - } - return wrappedFile, nil + if filepath.Base(name) == "testfile" { + return &overrideFile{f, m}, nil } - - return fs.FS.OpenFile(name, flags) + return f, nil } -type fileStat struct { +type overrideFile struct { fs.File - fi os.FileInfo + ofs *overrideFS +} + +func (f overrideFile) Stat() (os.FileInfo, error) { + return f.ofs.overrideFI, nil } -func (f fileStat) Stat() (os.FileInfo, error) { - return f.fi, nil +func (f overrideFile) ToNode(ignoreXattrListError bool) (*restic.Node, error) { + if f.ofs.overrideNode == nil { + return f.File.ToNode(ignoreXattrListError) + } + return f.ofs.overrideNode, f.ofs.overrideErr } // used by wrapFileInfo, use untyped const in order to avoid having a version @@ -2279,17 +2260,18 @@ func TestMetadataChanged(t *testing.T) { // get metadata fi := lstat(t, "testfile") localFS := &fs.Local{} - want, err := localFS.NodeFromFileInfo("testfile", fi, false) - if err != nil { - t.Fatal(err) - } + meta, err := localFS.OpenFile("testfile", fs.O_NOFOLLOW, true) + rtest.OK(t, err) + want, err := meta.ToNode(false) + rtest.OK(t, err) + rtest.OK(t, meta.Close()) - fs := &StatFS{ - FS: localFS, - OverrideLstat: map[string]os.FileInfo{ - "testfile": fi, - }, + fs := &overrideFS{ + FS: localFS, + overrideFI: fi, + overrideNode: &restic.Node{}, } + *fs.overrideNode = *want sn, node2 := snapshot(t, repo, fs, nil, "testfile") @@ -2309,7 +2291,8 @@ func TestMetadataChanged(t *testing.T) { } // modify the mode by wrapping it in a new struct, uses the consts defined above - fs.OverrideLstat["testfile"] = wrapFileInfo(fi) + fs.overrideFI = wrapFileInfo(fi) + rtest.Assert(t, !fileChanged(fs, fs.overrideFI, node2, 0), "testfile must not be considered as changed") // set the override values in the 'want' node which want.Mode = 0400 @@ -2318,16 +2301,13 @@ func TestMetadataChanged(t *testing.T) { want.UID = 51234 want.GID = 51235 } - // no user and group name - want.User = "" - want.Group = "" + // update mock node accordingly + fs.overrideNode.Mode = 0400 + fs.overrideNode.UID = want.UID + fs.overrideNode.GID = want.GID // make another snapshot _, node3 := snapshot(t, repo, fs, sn, "testfile") - // Override username and group to empty string - in case underlying system has user with UID 51234 - // See https://github.com/restic/restic/issues/2372 - node3.User = "" - node3.Group = "" // make sure that metadata was recorded successfully if !cmp.Equal(want, node3) { @@ -2342,7 +2322,7 @@ func TestMetadataChanged(t *testing.T) { func TestRacyFileSwap(t *testing.T) { files := TestDir{ - "file": TestFile{ + "testfile": TestFile{ Content: "foo bar test file", }, } @@ -2354,14 +2334,11 @@ func TestRacyFileSwap(t *testing.T) { // get metadata of current folder fi := lstat(t, ".") - tempfile := filepath.Join(tempdir, "file") + tempfile := filepath.Join(tempdir, "testfile") - statfs := &StatFS{ - FS: fs.Local{}, - OverrideLstat: map[string]os.FileInfo{ - tempfile: fi, - }, - OnlyOverrideStat: true, + statfs := &overrideFS{ + FS: fs.Local{}, + overrideFI: fi, } ctx, cancel := context.WithCancel(context.Background()) @@ -2388,14 +2365,19 @@ func TestRacyFileSwap(t *testing.T) { } } +type mockToNoder struct { + node *restic.Node + err error +} + +func (m *mockToNoder) ToNode(_ bool) (*restic.Node, error) { + return m.node, m.err +} + func TestMetadataBackupErrorFiltering(t *testing.T) { tempdir := t.TempDir() - repo := repository.TestRepository(t) - filename := filepath.Join(tempdir, "file") - rtest.OK(t, os.WriteFile(filename, []byte("example"), 0o600)) - fi, err := os.Stat(filename) - rtest.OK(t, err) + repo := repository.TestRepository(t) arch := New(repo, fs.Local{}, Options{}) @@ -2406,15 +2388,24 @@ func TestMetadataBackupErrorFiltering(t *testing.T) { return replacementErr } + nonExistNoder := &mockToNoder{ + node: &restic.Node{Type: restic.NodeTypeFile}, + err: fmt.Errorf("not found"), + } + // check that errors from reading extended metadata are properly filtered - node, err := arch.nodeFromFileInfo("file", filename+"invalid", fi, false) + node, err := arch.nodeFromFileInfo("file", filename+"invalid", nonExistNoder, false) rtest.Assert(t, node != nil, "node is missing") rtest.Assert(t, err == replacementErr, "expected %v got %v", replacementErr, err) rtest.Assert(t, filteredErr != nil, "missing inner error") // check that errors from reading irregular file are not filtered filteredErr = nil - node, err = arch.nodeFromFileInfo("file", filename, wrapIrregularFileInfo(fi), false) + nonExistNoder = &mockToNoder{ + node: &restic.Node{Type: restic.NodeTypeIrregular}, + err: fmt.Errorf(`unsupported file type "irregular"`), + } + node, err = arch.nodeFromFileInfo("file", filename, nonExistNoder, false) rtest.Assert(t, node != nil, "node is missing") rtest.Assert(t, filteredErr == nil, "error for irregular node should not have been filtered") rtest.Assert(t, strings.Contains(err.Error(), "irregular"), "unexpected error %q does not warn about irregular file mode", err) @@ -2434,17 +2425,19 @@ func TestIrregularFile(t *testing.T) { tempfile := filepath.Join(tempdir, "testfile") fi := lstat(t, "testfile") - statfs := &StatFS{ - FS: fs.Local{}, - OverrideLstat: map[string]os.FileInfo{ - tempfile: wrapIrregularFileInfo(fi), + override := &overrideFS{ + FS: fs.Local{}, + overrideFI: wrapIrregularFileInfo(fi), + overrideNode: &restic.Node{ + Type: restic.NodeTypeIrregular, }, + overrideErr: fmt.Errorf(`unsupported file type "irregular"`), } ctx, cancel := context.WithCancel(context.Background()) defer cancel() - arch := New(repo, fs.Track{FS: statfs}, Options{}) + arch := New(repo, fs.Track{FS: override}, Options{}) _, excluded, err := arch.save(ctx, "/", tempfile, nil) if err == nil { t.Fatalf("Save() should have failed") diff --git a/internal/archiver/archiver_unix_test.go b/internal/archiver/archiver_unix_test.go index 621f84826b0..deeab645947 100644 --- a/internal/archiver/archiver_unix_test.go +++ b/internal/archiver/archiver_unix_test.go @@ -57,12 +57,8 @@ func wrapIrregularFileInfo(fi os.FileInfo) os.FileInfo { } func statAndSnapshot(t *testing.T, repo archiverRepo, name string) (*restic.Node, *restic.Node) { - fi := lstat(t, name) - fs := &fs.Local{} - want, err := fs.NodeFromFileInfo(name, fi, false) - rtest.OK(t, err) - - _, node := snapshot(t, repo, fs, nil, name) + want := nodeFromFile(t, &fs.Local{}, name) + _, node := snapshot(t, repo, &fs.Local{}, nil, name) return want, node } diff --git a/internal/archiver/exclude.go b/internal/archiver/exclude.go index 54ced788ae0..418517fd91c 100644 --- a/internal/archiver/exclude.go +++ b/internal/archiver/exclude.go @@ -135,9 +135,9 @@ func isExcludedByFile(filename, tagFilename, header string, rc *rejectionCache, return rejected } -func isDirExcludedByFile(dir, tagFilename, header string, fs fs.FS, warnf func(msg string, args ...interface{})) bool { - tf := fs.Join(dir, tagFilename) - _, err := fs.Lstat(tf) +func isDirExcludedByFile(dir, tagFilename, header string, fsInst fs.FS, warnf func(msg string, args ...interface{})) bool { + tf := fsInst.Join(dir, tagFilename) + _, err := fsInst.Lstat(tf) if errors.Is(err, os.ErrNotExist) { return false } @@ -153,7 +153,7 @@ func isDirExcludedByFile(dir, tagFilename, header string, fs fs.FS, warnf func(m // From this stage, errors mean tagFilename exists but it is malformed. // Warnings will be generated so that the user is informed that the // indented ignore-action is not performed. - f, err := fs.OpenFile(tf, os.O_RDONLY) + f, err := fsInst.OpenFile(tf, fs.O_RDONLY, false) if err != nil { warnf("could not open exclusion tagfile: %v", err) return false diff --git a/internal/archiver/file_saver.go b/internal/archiver/file_saver.go index dccaa944245..ca8ec2fbb50 100644 --- a/internal/archiver/file_saver.go +++ b/internal/archiver/file_saver.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "io" - "os" "sync" "github.com/restic/chunker" @@ -29,7 +28,7 @@ type fileSaver struct { CompleteBlob func(bytes uint64) - NodeFromFileInfo func(snPath, filename string, fi os.FileInfo, ignoreXattrListError bool) (*restic.Node, error) + NodeFromFileInfo func(snPath, filename string, meta ToNoder, ignoreXattrListError bool) (*restic.Node, error) } // newFileSaver returns a new file saver. A worker pool with fileWorkers is @@ -71,13 +70,12 @@ type fileCompleteFunc func(*restic.Node, ItemStats) // file is closed by Save. completeReading is only called if the file was read // successfully. complete is always called. If completeReading is called, then // this will always happen before calling complete. -func (s *fileSaver) Save(ctx context.Context, snPath string, target string, file fs.File, fi os.FileInfo, start func(), completeReading func(), complete fileCompleteFunc) futureNode { +func (s *fileSaver) Save(ctx context.Context, snPath string, target string, file fs.File, start func(), completeReading func(), complete fileCompleteFunc) futureNode { fn, ch := newFutureNode() job := saveFileJob{ snPath: snPath, target: target, file: file, - fi: fi, ch: ch, start: start, @@ -100,7 +98,6 @@ type saveFileJob struct { snPath string target string file fs.File - fi os.FileInfo ch chan<- futureNodeResult start func() @@ -109,7 +106,7 @@ type saveFileJob struct { } // saveFile stores the file f in the repo, then closes it. -func (s *fileSaver) saveFile(ctx context.Context, chnker *chunker.Chunker, snPath string, target string, f fs.File, fi os.FileInfo, start func(), finishReading func(), finish func(res futureNodeResult)) { +func (s *fileSaver) saveFile(ctx context.Context, chnker *chunker.Chunker, snPath string, target string, f fs.File, start func(), finishReading func(), finish func(res futureNodeResult)) { start() fnr := futureNodeResult{ @@ -156,7 +153,7 @@ func (s *fileSaver) saveFile(ctx context.Context, chnker *chunker.Chunker, snPat debug.Log("%v", snPath) - node, err := s.NodeFromFileInfo(snPath, target, fi, false) + node, err := s.NodeFromFileInfo(snPath, target, f, false) if err != nil { _ = f.Close() completeError(err) @@ -262,7 +259,7 @@ func (s *fileSaver) worker(ctx context.Context, jobs <-chan saveFileJob) { } } - s.saveFile(ctx, chnker, job.snPath, job.target, job.file, job.fi, job.start, func() { + s.saveFile(ctx, chnker, job.snPath, job.target, job.file, job.start, func() { if job.completeReading != nil { job.completeReading() } diff --git a/internal/archiver/file_saver_test.go b/internal/archiver/file_saver_test.go index 069cdc17152..ce862f6feb6 100644 --- a/internal/archiver/file_saver_test.go +++ b/internal/archiver/file_saver_test.go @@ -30,7 +30,7 @@ func createTestFiles(t testing.TB, num int) (files []string) { return files } -func startFileSaver(ctx context.Context, t testing.TB, fs fs.FS) (*fileSaver, context.Context, *errgroup.Group) { +func startFileSaver(ctx context.Context, t testing.TB, fsInst fs.FS) (*fileSaver, context.Context, *errgroup.Group) { wg, ctx := errgroup.WithContext(ctx) saveBlob := func(ctx context.Context, tpe restic.BlobType, buf *buffer, _ string, cb func(saveBlobResponse)) { @@ -49,8 +49,8 @@ func startFileSaver(ctx context.Context, t testing.TB, fs fs.FS) (*fileSaver, co } s := newFileSaver(ctx, wg, saveBlob, pol, workers, workers) - s.NodeFromFileInfo = func(snPath, filename string, fi os.FileInfo, ignoreXattrListError bool) (*restic.Node, error) { - return fs.NodeFromFileInfo(filename, fi, ignoreXattrListError) + s.NodeFromFileInfo = func(snPath, filename string, meta ToNoder, ignoreXattrListError bool) (*restic.Node, error) { + return meta.ToNode(ignoreXattrListError) } return s, ctx, wg @@ -72,17 +72,12 @@ func TestFileSaver(t *testing.T) { var results []futureNode for _, filename := range files { - f, err := testFs.OpenFile(filename, os.O_RDONLY) + f, err := testFs.OpenFile(filename, os.O_RDONLY, false) if err != nil { t.Fatal(err) } - fi, err := f.Stat() - if err != nil { - t.Fatal(err) - } - - ff := s.Save(ctx, filename, filename, f, fi, startFn, completeReadingFn, completeFn) + ff := s.Save(ctx, filename, filename, f, startFn, completeReadingFn, completeFn) results = append(results, ff) } diff --git a/internal/fs/file.go b/internal/fs/file.go index fa395b62845..81ee4bc7acd 100644 --- a/internal/fs/file.go +++ b/internal/fs/file.go @@ -67,7 +67,7 @@ func ResetPermissions(path string) error { // Readdirnames returns a list of file in a directory. Flags are passed to fs.OpenFile. // O_RDONLY and O_DIRECTORY are implied. func Readdirnames(filesystem FS, dir string, flags int) ([]string, error) { - f, err := filesystem.OpenFile(dir, O_RDONLY|O_DIRECTORY|flags) + f, err := filesystem.OpenFile(dir, O_RDONLY|O_DIRECTORY|flags, false) if err != nil { return nil, fmt.Errorf("openfile for readdirnames failed: %w", err) } diff --git a/internal/fs/fs_local.go b/internal/fs/fs_local.go index 4f8b670900c..5e6c72d0a3e 100644 --- a/internal/fs/fs_local.go +++ b/internal/fs/fs_local.go @@ -20,18 +20,16 @@ func (fs Local) VolumeName(path string) string { return filepath.VolumeName(path) } -// OpenFile is the generalized open call; most users will use Open -// or Create instead. It opens the named file with specified flag -// (O_RDONLY etc.) and perm, (0666 etc.) if applicable. If successful, -// methods on the returned File can be used for I/O. -// If there is an error, it will be of type *PathError. -func (fs Local) OpenFile(name string, flag int) (File, error) { - f, err := os.OpenFile(fixpath(name), flag, 0) - if err != nil { - return nil, err - } - _ = setFlags(f) - return f, nil +// OpenFile opens a file or directory for reading. +// +// If metadataOnly is set, an implementation MUST return a File object for +// arbitrary file types including symlinks. The implementation may internally use +// the given file path or a file handle. In particular, an implementation may +// delay actually accessing the underlying filesystem. +// +// Only the O_NOFOLLOW and O_DIRECTORY flags are supported. +func (fs Local) OpenFile(name string, flag int, metadataOnly bool) (File, error) { + return newLocalFile(name, flag, metadataOnly) } // Lstat returns the FileInfo structure describing the named file. @@ -53,10 +51,6 @@ func (fs Local) ExtendedStat(fi os.FileInfo) ExtendedFileInfo { return ExtendedStat(fi) } -func (fs Local) NodeFromFileInfo(path string, fi os.FileInfo, ignoreXattrListError bool) (*restic.Node, error) { - return nodeFromFileInfo(path, fi, ignoreXattrListError) -} - // Join joins any number of path elements into a single path, adding a // Separator if necessary. Join calls Clean on the result; in particular, all // empty strings are ignored. On Windows, the result is a UNC path if and only @@ -97,3 +91,87 @@ func (fs Local) Base(path string) string { func (fs Local) Dir(path string) string { return filepath.Dir(path) } + +type localFile struct { + name string + flag int + f *os.File + fi os.FileInfo +} + +// See the File interface for a description of each method +var _ File = &localFile{} + +func newLocalFile(name string, flag int, metadataOnly bool) (*localFile, error) { + var f *os.File + if !metadataOnly { + var err error + f, err = os.OpenFile(fixpath(name), flag, 0) + if err != nil { + return nil, err + } + _ = setFlags(f) + } + return &localFile{ + name: name, + flag: flag, + f: f, + }, nil +} + +func (f *localFile) MakeReadable() error { + if f.f != nil { + panic("file is already readable") + } + + newF, err := newLocalFile(f.name, f.flag, false) + if err != nil { + return err + } + // replace state and also reset cached FileInfo + *f = *newF + return nil +} + +func (f *localFile) cacheFI() error { + if f.fi != nil { + return nil + } + var err error + if f.f != nil { + f.fi, err = f.f.Stat() + } else if f.flag&O_NOFOLLOW != 0 { + f.fi, err = os.Lstat(f.name) + } else { + f.fi, err = os.Stat(f.name) + } + return err +} + +func (f *localFile) Stat() (os.FileInfo, error) { + err := f.cacheFI() + // the call to cacheFI MUST happen before reading from f.fi + return f.fi, err +} + +func (f *localFile) ToNode(ignoreXattrListError bool) (*restic.Node, error) { + if err := f.cacheFI(); err != nil { + return nil, err + } + return nodeFromFileInfo(f.name, f.fi, ignoreXattrListError) +} + +func (f *localFile) Read(p []byte) (n int, err error) { + return f.f.Read(p) +} + +func (f *localFile) Readdirnames(n int) ([]string, error) { + return f.f.Readdirnames(n) +} + +func (f *localFile) Close() error { + if f.f != nil { + return f.f.Close() + } + return nil +} diff --git a/internal/fs/fs_local_vss.go b/internal/fs/fs_local_vss.go index e9bc66657c9..fe82b85e13d 100644 --- a/internal/fs/fs_local_vss.go +++ b/internal/fs/fs_local_vss.go @@ -10,7 +10,6 @@ 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. @@ -126,9 +125,9 @@ func (fs *LocalVss) DeleteSnapshots() { fs.snapshots = activeSnapshots } -// OpenFile wraps the Open method of the underlying file system. -func (fs *LocalVss) OpenFile(name string, flag int) (File, error) { - return fs.FS.OpenFile(fs.snapshotPath(name), flag) +// OpenFile wraps the OpenFile method of the underlying file system. +func (fs *LocalVss) OpenFile(name string, flag int, metadataOnly bool) (File, error) { + return fs.FS.OpenFile(fs.snapshotPath(name), flag, metadataOnly) } // Lstat wraps the Lstat method of the underlying file system. @@ -136,10 +135,6 @@ func (fs *LocalVss) Lstat(name string) (os.FileInfo, error) { return fs.FS.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 { diff --git a/internal/fs/fs_local_vss_test.go b/internal/fs/fs_local_vss_test.go index db8d4b13385..33c412fe9dc 100644 --- a/internal/fs/fs_local_vss_test.go +++ b/internal/fs/fs_local_vss_test.go @@ -327,7 +327,7 @@ func TestVSSFS(t *testing.T) { rtest.OK(t, err) rtest.Equals(t, origFi.Mode(), lstatFi.Mode()) - f, err := localVss.OpenFile(tempfile, os.O_RDONLY) + f, err := localVss.OpenFile(tempfile, os.O_RDONLY, false) rtest.OK(t, err) data, err := io.ReadAll(f) rtest.OK(t, err) diff --git a/internal/fs/fs_reader.go b/internal/fs/fs_reader.go index a4efa8deacb..8728b274c71 100644 --- a/internal/fs/fs_reader.go +++ b/internal/fs/fs_reader.go @@ -49,12 +49,7 @@ func (fs *Reader) fi() os.FileInfo { } } -// OpenFile is the generalized open call; most users will use Open -// or Create instead. It opens the named file with specified flag -// (O_RDONLY etc.) and perm, (0666 etc.) if applicable. If successful, -// methods on the returned File can be used for I/O. -// If there is an error, it will be of type *os.PathError. -func (fs *Reader) OpenFile(name string, flag int) (f File, err error) { +func (fs *Reader) OpenFile(name string, flag int, _ bool) (f File, err error) { if flag & ^(O_RDONLY|O_NOFOLLOW) != 0 { return nil, pathError("open", name, fmt.Errorf("invalid combination of flags 0x%x", flag)) @@ -127,17 +122,6 @@ func (fs *Reader) ExtendedStat(fi os.FileInfo) ExtendedFileInfo { } } -func (fs *Reader) NodeFromFileInfo(path string, fi os.FileInfo, _ bool) (*restic.Node, error) { - node := buildBasicNode(path, fi) - - // fill minimal info with current values for uid, gid - node.UID = uint32(os.Getuid()) - node.GID = uint32(os.Getgid()) - node.ChangeTime = node.ModTime - - return node, nil -} - // Join joins any number of path elements into a single path, adding a // Separator if necessary. Join calls Clean on the result; in particular, all // empty strings are ignored. On Windows, the result is a UNC path if and only @@ -235,6 +219,10 @@ type fakeFile struct { // ensure that fakeFile implements File var _ File = fakeFile{} +func (f fakeFile) MakeReadable() error { + return nil +} + func (f fakeFile) Readdirnames(_ int) ([]string, error) { return nil, pathError("readdirnames", f.name, os.ErrInvalid) } @@ -251,6 +239,17 @@ func (f fakeFile) Stat() (os.FileInfo, error) { return f.FileInfo, nil } +func (f fakeFile) ToNode(_ bool) (*restic.Node, error) { + node := buildBasicNode(f.name, f.FileInfo) + + // fill minimal info with current values for uid, gid + node.UID = uint32(os.Getuid()) + node.GID = uint32(os.Getgid()) + node.ChangeTime = node.ModTime + + return node, nil +} + // fakeDir implements Readdirnames and Readdir, everything else is delegated to fakeFile. type fakeDir struct { entries []os.FileInfo diff --git a/internal/fs/fs_reader_test.go b/internal/fs/fs_reader_test.go index e7020bc9dc5..7e7f6e77c8e 100644 --- a/internal/fs/fs_reader_test.go +++ b/internal/fs/fs_reader_test.go @@ -16,7 +16,7 @@ import ( ) func verifyFileContentOpenFile(t testing.TB, fs FS, filename string, want []byte) { - f, err := fs.OpenFile(filename, O_RDONLY) + f, err := fs.OpenFile(filename, O_RDONLY, false) if err != nil { t.Fatal(err) } @@ -37,7 +37,7 @@ func verifyFileContentOpenFile(t testing.TB, fs FS, filename string, want []byte } func verifyDirectoryContents(t testing.TB, fs FS, dir string, want []string) { - f, err := fs.OpenFile(dir, os.O_RDONLY) + f, err := fs.OpenFile(dir, O_RDONLY, false) if err != nil { t.Fatal(err) } @@ -123,7 +123,7 @@ func TestFSReader(t *testing.T) { { name: "file/Stat", f: func(t *testing.T, fs FS) { - f, err := fs.OpenFile(filename, os.O_RDONLY) + f, err := fs.OpenFile(filename, O_RDONLY, true) if err != nil { t.Fatal(err) } @@ -295,7 +295,7 @@ func TestFSReaderMinFileSize(t *testing.T) { AllowEmptyFile: test.allowEmpty, } - f, err := fs.OpenFile("testfile", os.O_RDONLY) + f, err := fs.OpenFile("testfile", O_RDONLY, false) if err != nil { t.Fatal(err) } diff --git a/internal/fs/fs_track.go b/internal/fs/fs_track.go index 9912ac45bb9..9ebdbb8c4a4 100644 --- a/internal/fs/fs_track.go +++ b/internal/fs/fs_track.go @@ -16,8 +16,8 @@ type Track struct { } // OpenFile wraps the OpenFile method of the underlying file system. -func (fs Track) OpenFile(name string, flag int) (File, error) { - f, err := fs.FS.OpenFile(fixpath(name), flag) +func (fs Track) OpenFile(name string, flag int, metadataOnly bool) (File, error) { + f, err := fs.FS.OpenFile(name, flag, metadataOnly) if err != nil { return nil, err } @@ -31,7 +31,7 @@ type trackFile struct { func newTrackFile(stack []byte, filename string, file File) *trackFile { f := &trackFile{file} - runtime.SetFinalizer(f, func(_ *trackFile) { + runtime.SetFinalizer(f, func(_ any) { fmt.Fprintf(os.Stderr, "file %s not closed\n\nStacktrack:\n%s\n", filename, stack) panic("file " + filename + " not closed") }) diff --git a/internal/fs/interface.go b/internal/fs/interface.go index 899888fb085..7ff77713814 100644 --- a/internal/fs/interface.go +++ b/internal/fs/interface.go @@ -9,11 +9,18 @@ import ( // FS bundles all methods needed for a file system. type FS interface { - OpenFile(name string, flag int) (File, error) + // OpenFile opens a file or directory for reading. + // + // If metadataOnly is set, an implementation MUST return a File object for + // arbitrary file types including symlinks. The implementation may internally use + // the given file path or a file handle. In particular, an implementation may + // delay actually accessing the underlying filesystem. + // + // Only the O_NOFOLLOW and O_DIRECTORY flags are supported. + OpenFile(name string, flag int, metadataOnly bool) (File, error) Lstat(name string) (os.FileInfo, error) DeviceID(fi os.FileInfo) (deviceID uint64, err error) ExtendedStat(fi os.FileInfo) ExtendedFileInfo - NodeFromFileInfo(path string, fi os.FileInfo, ignoreXattrListError bool) (*restic.Node, error) Join(elem ...string) string Separator() string @@ -26,11 +33,23 @@ type FS interface { Base(path string) string } -// File is an open file on a file system. +// File is an open file on a file system. When opened as metadataOnly, an +// implementation may opt to perform filesystem operations using the filepath +// instead of actually opening the file. type File interface { + // MakeReadable reopens a File that was opened metadataOnly for reading. + // The method must not be called for files that are opened for reading. + // If possible, the underlying file should be reopened atomically. + // MakeReadable must work for files and directories. + MakeReadable() error + io.Reader io.Closer Readdirnames(n int) ([]string, error) Stat() (os.FileInfo, error) + // ToNode returns a restic.Node for the File. The internally used os.FileInfo + // must be consistent with that returned by Stat(). In particular, the metadata + // returned by consecutive calls to Stat() and ToNode() must match. + ToNode(ignoreXattrListError bool) (*restic.Node, error) } diff --git a/internal/fs/node_test.go b/internal/fs/node_test.go index 58facceb165..65098e30473 100644 --- a/internal/fs/node_test.go +++ b/internal/fs/node_test.go @@ -17,56 +17,26 @@ import ( rtest "github.com/restic/restic/internal/test" ) -func BenchmarkNodeFillUser(t *testing.B) { - tempfile, err := os.CreateTemp("", "restic-test-temp-") - if err != nil { - t.Fatal(err) - } - - fi, err := tempfile.Stat() - if err != nil { - t.Fatal(err) - } - +func BenchmarkNodeFromFileInfo(t *testing.B) { + tempfile, err := os.CreateTemp(t.TempDir(), "restic-test-temp-") + rtest.OK(t, err) path := tempfile.Name() - fs := Local{} - - t.ResetTimer() - - for i := 0; i < t.N; i++ { - _, err := fs.NodeFromFileInfo(path, fi, false) - rtest.OK(t, err) - } - rtest.OK(t, tempfile.Close()) - rtest.RemoveAll(t, tempfile.Name()) -} - -func BenchmarkNodeFromFileInfo(t *testing.B) { - tempfile, err := os.CreateTemp("", "restic-test-temp-") - if err != nil { - t.Fatal(err) - } - - fi, err := tempfile.Stat() - if err != nil { - t.Fatal(err) - } - path := tempfile.Name() fs := Local{} + f, err := fs.OpenFile(path, O_NOFOLLOW, true) + rtest.OK(t, err) + _, err = f.Stat() + rtest.OK(t, err) t.ResetTimer() for i := 0; i < t.N; i++ { - _, err := fs.NodeFromFileInfo(path, fi, false) - if err != nil { - t.Fatal(err) - } + _, err := f.ToNode(false) + rtest.OK(t, err) } - rtest.OK(t, tempfile.Close()) - rtest.RemoveAll(t, tempfile.Name()) + rtest.OK(t, f.Close()) } func parseTime(s string) time.Time { @@ -249,14 +219,14 @@ func TestNodeRestoreAt(t *testing.T) { rtest.OK(t, NodeCreateAt(&test, nodePath)) rtest.OK(t, NodeRestoreMetadata(&test, nodePath, func(msg string) { rtest.OK(t, fmt.Errorf("Warning triggered for path: %s: %s", nodePath, msg)) })) - fi, err := os.Lstat(nodePath) - rtest.OK(t, err) - fs := &Local{} - n2, err := fs.NodeFromFileInfo(nodePath, fi, false) + meta, err := fs.OpenFile(nodePath, O_NOFOLLOW, true) + rtest.OK(t, err) + n2, err := meta.ToNode(false) rtest.OK(t, err) - n3, err := fs.NodeFromFileInfo(nodePath, fi, true) + n3, err := meta.ToNode(true) rtest.OK(t, err) + rtest.OK(t, meta.Close()) rtest.Assert(t, n2.Equals(*n3), "unexpected node info mismatch %v", cmp.Diff(n2, n3)) rtest.Assert(t, test.Name == n2.Name, diff --git a/internal/fs/node_unix_test.go b/internal/fs/node_unix_test.go index 6b47eafba2f..1eb1ee5061d 100644 --- a/internal/fs/node_unix_test.go +++ b/internal/fs/node_unix_test.go @@ -114,16 +114,14 @@ func TestNodeFromFileInfo(t *testing.T) { return } - if fi.Sys() == nil { - t.Skip("fi.Sys() is nil") - return - } - fs := &Local{} - node, err := fs.NodeFromFileInfo(test.filename, fi, false) - if err != nil { - t.Fatal(err) - } + meta, err := fs.OpenFile(test.filename, O_NOFOLLOW, true) + rtest.OK(t, err) + node, err := meta.ToNode(false) + rtest.OK(t, err) + rtest.OK(t, meta.Close()) + + rtest.OK(t, err) switch node.Type { case restic.NodeTypeFile, restic.NodeTypeSymlink: diff --git a/internal/fs/node_windows_test.go b/internal/fs/node_windows_test.go index 1bb76b20455..f75df54d3c3 100644 --- a/internal/fs/node_windows_test.go +++ b/internal/fs/node_windows_test.go @@ -222,11 +222,11 @@ func restoreAndGetNode(t *testing.T, tempDir string, testNode *restic.Node, warn test.OK(t, errors.Wrapf(err, "Failed to restore metadata for: %s", testPath)) fs := &Local{} - fi, err := fs.Lstat(testPath) - test.OK(t, errors.Wrapf(err, "Could not Lstat for path: %s", testPath)) - - nodeFromFileInfo, err := fs.NodeFromFileInfo(testPath, fi, false) + meta, err := fs.OpenFile(testPath, O_NOFOLLOW, true) + test.OK(t, err) + nodeFromFileInfo, err := meta.ToNode(false) test.OK(t, errors.Wrapf(err, "Could not get NodeFromFileInfo for path: %s", testPath)) + test.OK(t, meta.Close()) return testPath, nodeFromFileInfo } diff --git a/internal/restic/tree_test.go b/internal/restic/tree_test.go index f1979f135de..07ca254f135 100644 --- a/internal/restic/tree_test.go +++ b/internal/restic/tree_test.go @@ -83,13 +83,17 @@ func TestNodeMarshal(t *testing.T) { } } -func TestNodeComparison(t *testing.T) { - fs := &fs.Local{} - fi, err := fs.Lstat("tree_test.go") +func nodeForFile(t *testing.T, name string) *restic.Node { + f, err := (&fs.Local{}).OpenFile(name, fs.O_NOFOLLOW, true) rtest.OK(t, err) - - node, err := fs.NodeFromFileInfo("tree_test.go", fi, false) + node, err := f.ToNode(false) rtest.OK(t, err) + rtest.OK(t, f.Close()) + return node +} + +func TestNodeComparison(t *testing.T) { + node := nodeForFile(t, "tree_test.go") n2 := *node rtest.Assert(t, node.Equals(n2), "nodes aren't equal") @@ -127,11 +131,7 @@ func TestTreeEqualSerialization(t *testing.T) { builder := restic.NewTreeJSONBuilder() for _, fn := range files[:i] { - fs := &fs.Local{} - fi, err := fs.Lstat(fn) - rtest.OK(t, err) - node, err := fs.NodeFromFileInfo(fn, fi, false) - rtest.OK(t, err) + node := nodeForFile(t, fn) rtest.OK(t, tree.Insert(node)) rtest.OK(t, builder.AddNode(node)) From 6084848e5a05666cabab43bd96d2419b879fd405 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 16 Nov 2024 15:38:40 +0100 Subject: [PATCH 06/10] fs: fix O_NOFOLLOW for metadata handles on Windows --- internal/fs/const_windows.go | 6 ++++-- internal/fs/file.go | 4 ++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/internal/fs/const_windows.go b/internal/fs/const_windows.go index 4c29e0b9d59..b2b1bab86b6 100644 --- a/internal/fs/const_windows.go +++ b/internal/fs/const_windows.go @@ -5,8 +5,10 @@ package fs // TODO honor flags when opening files -// O_NOFOLLOW is a noop on Windows. -const O_NOFOLLOW int = 0 +// O_NOFOLLOW is currently only interpreted by FS.OpenFile in metadataOnly mode and ignored by OpenFile. +// The value of the constant is invented and only for use within this fs package. It must not be used in other contexts. +// It must not conflict with the other O_* values from go/src/syscall/types_windows.go +const O_NOFOLLOW int = 0x40000000 // O_DIRECTORY is a noop on Windows. const O_DIRECTORY int = 0 diff --git a/internal/fs/file.go b/internal/fs/file.go index 81ee4bc7acd..57f1a996a07 100644 --- a/internal/fs/file.go +++ b/internal/fs/file.go @@ -3,6 +3,7 @@ package fs import ( "fmt" "os" + "runtime" ) // MkdirAll creates a directory named path, along with any necessary parents, @@ -47,6 +48,9 @@ func Lstat(name string) (os.FileInfo, error) { // methods on the returned File can be used for I/O. // If there is an error, it will be of type *PathError. func OpenFile(name string, flag int, perm os.FileMode) (*os.File, error) { + if runtime.GOOS == "windows" { + flag &^= O_NOFOLLOW + } return os.OpenFile(fixpath(name), flag, perm) } From 087f95a298b61723a52d85b28bf45eb4a3bf05ce Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 2 Nov 2024 22:45:48 +0100 Subject: [PATCH 07/10] fs: make generic and extended attrs independent of each other --- internal/fs/node.go | 7 ++---- internal/fs/node_aix.go | 4 ++-- internal/fs/node_netbsd.go | 4 ++-- internal/fs/node_openbsd.go | 4 ++-- internal/fs/node_windows.go | 46 ++++++++++++++++++++++--------------- internal/fs/node_xattr.go | 4 ++-- 6 files changed, 37 insertions(+), 32 deletions(-) diff --git a/internal/fs/node.go b/internal/fs/node.go index d3619432212..a5c821ff42a 100644 --- a/internal/fs/node.go +++ b/internal/fs/node.go @@ -22,11 +22,8 @@ func nodeFromFileInfo(path string, fi os.FileInfo, ignoreXattrListError bool) (* return node, err } - allowExtended, err := nodeFillGenericAttributes(node, path, &stat) - if allowExtended { - // Skip processing ExtendedAttributes if allowExtended is false. - err = errors.Join(err, nodeFillExtendedAttributes(node, path, ignoreXattrListError)) - } + err := nodeFillGenericAttributes(node, path, &stat) + err = errors.Join(err, nodeFillExtendedAttributes(node, path, ignoreXattrListError)) return node, err } diff --git a/internal/fs/node_aix.go b/internal/fs/node_aix.go index fd185724f3b..19c5a2efe61 100644 --- a/internal/fs/node_aix.go +++ b/internal/fs/node_aix.go @@ -21,6 +21,6 @@ func nodeRestoreGenericAttributes(node *restic.Node, _ string, warn func(msg str } // nodeFillGenericAttributes is a no-op on AIX. -func nodeFillGenericAttributes(_ *restic.Node, _ string, _ *ExtendedFileInfo) (allowExtended bool, err error) { - return true, nil +func nodeFillGenericAttributes(_ *restic.Node, _ string, _ *ExtendedFileInfo) error { + return nil } diff --git a/internal/fs/node_netbsd.go b/internal/fs/node_netbsd.go index d295bf57948..1bf00a136bc 100644 --- a/internal/fs/node_netbsd.go +++ b/internal/fs/node_netbsd.go @@ -18,6 +18,6 @@ func nodeRestoreGenericAttributes(node *restic.Node, _ string, warn func(msg str } // nodeFillGenericAttributes is a no-op on netbsd. -func nodeFillGenericAttributes(_ *restic.Node, _ string, _ *ExtendedFileInfo) (allowExtended bool, err error) { - return true, nil +func nodeFillGenericAttributes(_ *restic.Node, _ string, _ *ExtendedFileInfo) error { + return nil } diff --git a/internal/fs/node_openbsd.go b/internal/fs/node_openbsd.go index 712b144b46a..1041622af9b 100644 --- a/internal/fs/node_openbsd.go +++ b/internal/fs/node_openbsd.go @@ -18,6 +18,6 @@ func nodeRestoreGenericAttributes(node *restic.Node, _ string, warn func(msg str } // fillGenericAttributes is a no-op on openbsd. -func nodeFillGenericAttributes(_ *restic.Node, _ string, _ *ExtendedFileInfo) (allowExtended bool, err error) { - return true, nil +func nodeFillGenericAttributes(_ *restic.Node, _ string, _ *ExtendedFileInfo) error { + return nil } diff --git a/internal/fs/node_windows.go b/internal/fs/node_windows.go index 837d4642805..c0f8b08b09b 100644 --- a/internal/fs/node_windows.go +++ b/internal/fs/node_windows.go @@ -83,8 +83,28 @@ func nodeRestoreExtendedAttributes(node *restic.Node, path string) (err error) { return nil } -// fill extended attributes in the node. This also includes the Generic attributes for windows. +// fill extended attributes in the node +// It also checks if the volume supports extended attributes and stores the result in a map +// so that it does not have to be checked again for subsequent calls for paths in the same volume. func nodeFillExtendedAttributes(node *restic.Node, path string, _ bool) (err error) { + if strings.Contains(filepath.Base(path), ":") { + // Do not process for Alternate Data Streams in Windows + return nil + } + + // only capture xattrs for file/dir + if node.Type != restic.NodeTypeFile && node.Type != restic.NodeTypeDir { + return nil + } + + allowExtended, err := checkAndStoreEASupport(path) + if err != nil { + return err + } + if !allowExtended { + return nil + } + var fileHandle windows.Handle if fileHandle, err = openHandleForEA(node.Type, path, false); fileHandle == 0 { return nil @@ -316,40 +336,28 @@ func decryptFile(pathPointer *uint16) error { // nodeFillGenericAttributes fills in the generic attributes for windows like File Attributes, // Created time and Security Descriptors. -// It also checks if the volume supports extended attributes and stores the result in a map -// so that it does not have to be checked again for subsequent calls for paths in the same volume. -func nodeFillGenericAttributes(node *restic.Node, path string, stat *ExtendedFileInfo) (allowExtended bool, err error) { +func nodeFillGenericAttributes(node *restic.Node, path string, stat *ExtendedFileInfo) error { if strings.Contains(filepath.Base(path), ":") { // Do not process for Alternate Data Streams in Windows - // Also do not allow processing of extended attributes for ADS. - return false, nil + return nil } isVolume, err := isVolumePath(path) if err != nil { - return false, err + return err } if isVolume { // Do not process file attributes, created time and sd for windows root volume paths // Security descriptors are not supported for root volume paths. // Though file attributes and created time are supported for root volume paths, // we ignore them and we do not want to replace them during every restore. - allowExtended, err = checkAndStoreEASupport(path) - if err != nil { - return false, err - } - return allowExtended, err + return nil } var sd *[]byte if node.Type == restic.NodeTypeFile || node.Type == restic.NodeTypeDir { - // Check EA support and get security descriptor for file/dir only - allowExtended, err = checkAndStoreEASupport(path) - if err != nil { - return false, err - } if sd, err = getSecurityDescriptor(path); err != nil { - return allowExtended, err + return err } } @@ -361,7 +369,7 @@ func nodeFillGenericAttributes(node *restic.Node, path string, stat *ExtendedFil FileAttributes: &winFI.FileAttributes, SecurityDescriptor: sd, }) - return allowExtended, err + return err } // checkAndStoreEASupport checks if the volume of the path supports extended attributes and stores the result in a map diff --git a/internal/fs/node_xattr.go b/internal/fs/node_xattr.go index 1781452f700..aacc216dc86 100644 --- a/internal/fs/node_xattr.go +++ b/internal/fs/node_xattr.go @@ -71,8 +71,8 @@ func nodeRestoreGenericAttributes(node *restic.Node, _ string, warn func(msg str } // nodeFillGenericAttributes is a no-op. -func nodeFillGenericAttributes(_ *restic.Node, _ string, _ *ExtendedFileInfo) (allowExtended bool, err error) { - return true, nil +func nodeFillGenericAttributes(_ *restic.Node, _ string, _ *ExtendedFileInfo) error { + return nil } func nodeRestoreExtendedAttributes(node *restic.Node, path string) error { From d7f4b9db60da583b8d950539fc5cac6a9b22525b Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 3 Nov 2024 13:27:58 +0100 Subject: [PATCH 08/10] fs: deduplicate placeholders for generic and xattrs --- internal/fs/node_aix.go | 26 -------------------------- internal/fs/node_netbsd.go | 23 ----------------------- internal/fs/node_noxattr.go | 18 ++++++++++++++++++ internal/fs/node_openbsd.go | 23 ----------------------- internal/fs/node_unix.go | 12 ++++++++++++ internal/fs/node_xattr.go | 10 ---------- 6 files changed, 30 insertions(+), 82 deletions(-) delete mode 100644 internal/fs/node_aix.go delete mode 100644 internal/fs/node_netbsd.go create mode 100644 internal/fs/node_noxattr.go delete mode 100644 internal/fs/node_openbsd.go diff --git a/internal/fs/node_aix.go b/internal/fs/node_aix.go deleted file mode 100644 index 19c5a2efe61..00000000000 --- a/internal/fs/node_aix.go +++ /dev/null @@ -1,26 +0,0 @@ -//go:build aix -// +build aix - -package fs - -import "github.com/restic/restic/internal/restic" - -// nodeRestoreExtendedAttributes is a no-op on AIX. -func nodeRestoreExtendedAttributes(_ *restic.Node, _ string) error { - return nil -} - -// nodeFillExtendedAttributes is a no-op on AIX. -func nodeFillExtendedAttributes(_ *restic.Node, _ string, _ bool) error { - return nil -} - -// nodeRestoreGenericAttributes is no-op on AIX. -func nodeRestoreGenericAttributes(node *restic.Node, _ string, warn func(msg string)) error { - return restic.HandleAllUnknownGenericAttributesFound(node.GenericAttributes, warn) -} - -// nodeFillGenericAttributes is a no-op on AIX. -func nodeFillGenericAttributes(_ *restic.Node, _ string, _ *ExtendedFileInfo) error { - return nil -} diff --git a/internal/fs/node_netbsd.go b/internal/fs/node_netbsd.go deleted file mode 100644 index 1bf00a136bc..00000000000 --- a/internal/fs/node_netbsd.go +++ /dev/null @@ -1,23 +0,0 @@ -package fs - -import "github.com/restic/restic/internal/restic" - -// nodeRestoreExtendedAttributes is a no-op on netbsd. -func nodeRestoreExtendedAttributes(_ *restic.Node, _ string) error { - return nil -} - -// nodeFillExtendedAttributes is a no-op on netbsd. -func nodeFillExtendedAttributes(_ *restic.Node, _ string, _ bool) error { - return nil -} - -// nodeRestoreGenericAttributes is no-op on netbsd. -func nodeRestoreGenericAttributes(node *restic.Node, _ string, warn func(msg string)) error { - return restic.HandleAllUnknownGenericAttributesFound(node.GenericAttributes, warn) -} - -// nodeFillGenericAttributes is a no-op on netbsd. -func nodeFillGenericAttributes(_ *restic.Node, _ string, _ *ExtendedFileInfo) error { - return nil -} diff --git a/internal/fs/node_noxattr.go b/internal/fs/node_noxattr.go new file mode 100644 index 00000000000..27bc6913a6f --- /dev/null +++ b/internal/fs/node_noxattr.go @@ -0,0 +1,18 @@ +//go:build aix || netbsd || openbsd +// +build aix netbsd openbsd + +package fs + +import ( + "github.com/restic/restic/internal/restic" +) + +// nodeRestoreExtendedAttributes is a no-op +func nodeRestoreExtendedAttributes(_ *restic.Node, _ string) error { + return nil +} + +// nodeFillExtendedAttributes is a no-op +func nodeFillExtendedAttributes(_ *restic.Node, _ string, _ bool) error { + return nil +} diff --git a/internal/fs/node_openbsd.go b/internal/fs/node_openbsd.go deleted file mode 100644 index 1041622af9b..00000000000 --- a/internal/fs/node_openbsd.go +++ /dev/null @@ -1,23 +0,0 @@ -package fs - -import "github.com/restic/restic/internal/restic" - -// nodeRestoreExtendedAttributes is a no-op on openbsd. -func nodeRestoreExtendedAttributes(_ *restic.Node, _ string) error { - return nil -} - -// nodeFillExtendedAttributes is a no-op on openbsd. -func nodeFillExtendedAttributes(_ *restic.Node, _ string, _ bool) error { - return nil -} - -// nodeRestoreGenericAttributes is no-op on openbsd. -func nodeRestoreGenericAttributes(node *restic.Node, _ string, warn func(msg string)) error { - return restic.HandleAllUnknownGenericAttributesFound(node.GenericAttributes, warn) -} - -// fillGenericAttributes is a no-op on openbsd. -func nodeFillGenericAttributes(_ *restic.Node, _ string, _ *ExtendedFileInfo) error { - return nil -} diff --git a/internal/fs/node_unix.go b/internal/fs/node_unix.go index 5f08f362316..e88e5425104 100644 --- a/internal/fs/node_unix.go +++ b/internal/fs/node_unix.go @@ -5,8 +5,20 @@ package fs import ( "os" + + "github.com/restic/restic/internal/restic" ) func lchown(name string, uid, gid int) error { return os.Lchown(name, uid, gid) } + +// nodeRestoreGenericAttributes is no-op. +func nodeRestoreGenericAttributes(node *restic.Node, _ string, warn func(msg string)) error { + return restic.HandleAllUnknownGenericAttributesFound(node.GenericAttributes, warn) +} + +// nodeFillGenericAttributes is a no-op. +func nodeFillGenericAttributes(_ *restic.Node, _ string, _ *ExtendedFileInfo) error { + return nil +} diff --git a/internal/fs/node_xattr.go b/internal/fs/node_xattr.go index aacc216dc86..e1ddf9826e7 100644 --- a/internal/fs/node_xattr.go +++ b/internal/fs/node_xattr.go @@ -65,16 +65,6 @@ func handleXattrErr(err error) error { } } -// nodeRestoreGenericAttributes is no-op. -func nodeRestoreGenericAttributes(node *restic.Node, _ string, warn func(msg string)) error { - return restic.HandleAllUnknownGenericAttributesFound(node.GenericAttributes, warn) -} - -// nodeFillGenericAttributes is a no-op. -func nodeFillGenericAttributes(_ *restic.Node, _ string, _ *ExtendedFileInfo) error { - return nil -} - func nodeRestoreExtendedAttributes(node *restic.Node, path string) error { expectedAttrs := map[string]struct{}{} for _, attr := range node.ExtendedAttributes { From 6cb19e01905abcf27c06b57b1d81787fd8d7642a Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 15 Nov 2024 21:21:04 +0100 Subject: [PATCH 09/10] archiver: fix file type change test The test did not test the case that the type of a file changed unexpectedly. --- internal/archiver/archiver.go | 4 +- internal/archiver/archiver_test.go | 97 ++++++++++++++++++++---------- 2 files changed, 66 insertions(+), 35 deletions(-) diff --git a/internal/archiver/archiver.go b/internal/archiver/archiver.go index ae3edaf44c6..f4ff6f47b1e 100644 --- a/internal/archiver/archiver.go +++ b/internal/archiver/archiver.go @@ -369,7 +369,7 @@ func (arch *Archiver) dirToNodeAndEntries(snPath, dir string, meta fs.File) (nod return nil, nil, err } if node.Type != restic.NodeTypeDir { - return nil, nil, fmt.Errorf("directory %v changed type, refusing to archive", snPath) + return nil, nil, fmt.Errorf("directory %q changed type, refusing to archive", snPath) } names, err = meta.Readdirnames(-1) @@ -548,7 +548,7 @@ func (arch *Archiver) save(ctx context.Context, snPath, target string, previous // make sure it's still a file if !fi.Mode().IsRegular() { - err = errors.Errorf("file %v changed type, refusing to archive", target) + err = errors.Errorf("file %q changed type, refusing to archive", target) return filterError(err) } diff --git a/internal/archiver/archiver_test.go b/internal/archiver/archiver_test.go index 0b2957bc6d4..e698ba74106 100644 --- a/internal/archiver/archiver_test.go +++ b/internal/archiver/archiver_test.go @@ -2202,9 +2202,10 @@ func snapshot(t testing.TB, repo archiverRepo, fs fs.FS, parent *restic.Snapshot type overrideFS struct { fs.FS - overrideFI os.FileInfo - overrideNode *restic.Node - overrideErr error + overrideFI os.FileInfo + resetFIOnRead bool + overrideNode *restic.Node + overrideErr error } func (m *overrideFS) OpenFile(name string, flag int, metadataOnly bool) (fs.File, error) { @@ -2213,7 +2214,7 @@ func (m *overrideFS) OpenFile(name string, flag int, metadataOnly bool) (fs.File return f, err } - if filepath.Base(name) == "testfile" { + if filepath.Base(name) == "testfile" || filepath.Base(name) == "testdir" { return &overrideFile{f, m}, nil } return f, nil @@ -2225,7 +2226,18 @@ type overrideFile struct { } func (f overrideFile) Stat() (os.FileInfo, error) { + if f.ofs.overrideFI == nil { + return f.File.Stat() + } return f.ofs.overrideFI, nil + +} + +func (f overrideFile) MakeReadable() error { + if f.ofs.resetFIOnRead { + f.ofs.overrideFI = nil + } + return f.File.MakeReadable() } func (f overrideFile) ToNode(ignoreXattrListError bool) (*restic.Node, error) { @@ -2320,48 +2332,67 @@ func TestMetadataChanged(t *testing.T) { checker.TestCheckRepo(t, repo, false) } -func TestRacyFileSwap(t *testing.T) { +func TestRacyFileTypeSwap(t *testing.T) { files := TestDir{ "testfile": TestFile{ Content: "foo bar test file", }, + "testdir": TestDir{}, } - tempdir, repo := prepareTempdirRepoSrc(t, files) - - back := rtest.Chdir(t, tempdir) - defer back() + for _, dirError := range []bool{false, true} { + desc := "file changed type" + if dirError { + desc = "dir changed type" + } + t.Run(desc, func(t *testing.T) { + tempdir, repo := prepareTempdirRepoSrc(t, files) - // get metadata of current folder - fi := lstat(t, ".") - tempfile := filepath.Join(tempdir, "testfile") + back := rtest.Chdir(t, tempdir) + defer back() - statfs := &overrideFS{ - FS: fs.Local{}, - overrideFI: fi, - } + // get metadata of current folder + var fakeName, realName string + if dirError { + // lstat claims this is a directory, but it's actually a file + fakeName = "testdir" + realName = "testfile" + } else { + fakeName = "testfile" + realName = "testdir" + } + fakeFI := lstat(t, fakeName) + tempfile := filepath.Join(tempdir, realName) - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() + statfs := &overrideFS{ + FS: fs.Local{}, + overrideFI: fakeFI, + resetFIOnRead: true, + } - wg, ctx := errgroup.WithContext(ctx) - repo.StartPackUploader(ctx, wg) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() - arch := New(repo, fs.Track{FS: statfs}, Options{}) - arch.Error = func(item string, err error) error { - t.Logf("archiver error as expected for %v: %v", item, err) - return err - } - arch.runWorkers(ctx, wg) + wg, ctx := errgroup.WithContext(ctx) + repo.StartPackUploader(ctx, wg) - // fs.Track will panic if the file was not closed - _, excluded, err := arch.save(ctx, "/", tempfile, nil) - if err == nil { - t.Errorf("Save() should have failed") - } + arch := New(repo, fs.Track{FS: statfs}, Options{}) + arch.Error = func(item string, err error) error { + t.Logf("archiver error as expected for %v: %v", item, err) + return err + } + arch.runWorkers(ctx, wg) - if excluded { - t.Errorf("Save() excluded the node, that's unexpected") + // fs.Track will panic if the file was not closed + _, excluded, err := arch.save(ctx, "/", tempfile, nil) + rtest.Assert(t, err != nil && strings.Contains(err.Error(), "changed type, refusing to archive"), "save() returned wrong error: %v", err) + tpe := "file" + if dirError { + tpe = "directory" + } + rtest.Assert(t, strings.Contains(err.Error(), tpe+" "), "unexpected item type in error: %v", err) + rtest.Assert(t, !excluded, "Save() excluded the node, that's unexpected") + }) } } From b51bf0c0c44e63723a915ba995feeed2296a8ff0 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 15 Nov 2024 23:02:52 +0100 Subject: [PATCH 10/10] fs: test File implementation of Local FS --- internal/fs/fs_local_test.go | 222 ++++++++++++++++++++++++++++++ internal/fs/fs_local_unix_test.go | 40 ++++++ 2 files changed, 262 insertions(+) create mode 100644 internal/fs/fs_local_test.go create mode 100644 internal/fs/fs_local_unix_test.go diff --git a/internal/fs/fs_local_test.go b/internal/fs/fs_local_test.go new file mode 100644 index 00000000000..b1e85de0a7b --- /dev/null +++ b/internal/fs/fs_local_test.go @@ -0,0 +1,222 @@ +package fs + +import ( + "io" + "os" + "path/filepath" + "slices" + "testing" + + "github.com/restic/restic/internal/restic" + rtest "github.com/restic/restic/internal/test" +) + +type fsLocalMetadataTestcase struct { + name string + follow bool + setup func(t *testing.T, path string) + nodeType restic.NodeType +} + +func TestFSLocalMetadata(t *testing.T) { + for _, test := range []fsLocalMetadataTestcase{ + { + name: "file", + setup: func(t *testing.T, path string) { + rtest.OK(t, os.WriteFile(path, []byte("example"), 0o600)) + }, + nodeType: restic.NodeTypeFile, + }, + { + name: "directory", + setup: func(t *testing.T, path string) { + rtest.OK(t, os.Mkdir(path, 0o600)) + }, + nodeType: restic.NodeTypeDir, + }, + { + name: "symlink", + setup: func(t *testing.T, path string) { + rtest.OK(t, os.Symlink(path+"old", path)) + }, + nodeType: restic.NodeTypeSymlink, + }, + { + name: "symlink file", + follow: true, + setup: func(t *testing.T, path string) { + rtest.OK(t, os.WriteFile(path+"file", []byte("example"), 0o600)) + rtest.OK(t, os.Symlink(path+"file", path)) + }, + nodeType: restic.NodeTypeFile, + }, + } { + runFSLocalTestcase(t, test) + } +} + +func runFSLocalTestcase(t *testing.T, test fsLocalMetadataTestcase) { + t.Run(test.name, func(t *testing.T) { + tmp := t.TempDir() + path := filepath.Join(tmp, "item") + test.setup(t, path) + + testFs := &Local{} + flags := 0 + if !test.follow { + flags |= O_NOFOLLOW + } + f, err := testFs.OpenFile(path, flags, true) + rtest.OK(t, err) + checkMetadata(t, f, path, test.follow, test.nodeType) + rtest.OK(t, f.Close()) + }) + +} + +func checkMetadata(t *testing.T, f File, path string, follow bool, nodeType restic.NodeType) { + fi, err := f.Stat() + rtest.OK(t, err) + var fi2 os.FileInfo + if follow { + fi2, err = os.Stat(path) + } else { + fi2, err = os.Lstat(path) + } + rtest.OK(t, err) + assertFIEqual(t, fi2, fi) + + node, err := f.ToNode(false) + rtest.OK(t, err) + + // ModTime is likely unique per file, thus it provides a good indication that it is from the correct file + rtest.Equals(t, fi.ModTime(), node.ModTime, "node ModTime") + rtest.Equals(t, nodeType, node.Type, "node Type") +} + +func assertFIEqual(t *testing.T, want os.FileInfo, got os.FileInfo) { + t.Helper() + rtest.Equals(t, want.Name(), got.Name(), "Name") + rtest.Equals(t, want.IsDir(), got.IsDir(), "IsDir") + rtest.Equals(t, want.ModTime(), got.ModTime(), "ModTime") + rtest.Equals(t, want.Mode(), got.Mode(), "Mode") + rtest.Equals(t, want.Size(), got.Size(), "Size") +} + +func TestFSLocalRead(t *testing.T) { + testFSLocalRead(t, false) + testFSLocalRead(t, true) +} + +func testFSLocalRead(t *testing.T, makeReadable bool) { + tmp := t.TempDir() + path := filepath.Join(tmp, "item") + testdata := "example" + rtest.OK(t, os.WriteFile(path, []byte(testdata), 0o600)) + + f := openReadable(t, path, makeReadable) + checkMetadata(t, f, path, false, restic.NodeTypeFile) + + data, err := io.ReadAll(f) + rtest.OK(t, err) + rtest.Equals(t, testdata, string(data), "file content mismatch") + + rtest.OK(t, f.Close()) +} + +func openReadable(t *testing.T, path string, useMakeReadable bool) File { + testFs := &Local{} + f, err := testFs.OpenFile(path, O_NOFOLLOW, useMakeReadable) + rtest.OK(t, err) + if useMakeReadable { + // file was opened as metadataOnly. open for reading + rtest.OK(t, f.MakeReadable()) + } + return f +} + +func TestFSLocalReaddir(t *testing.T) { + testFSLocalReaddir(t, false) + testFSLocalReaddir(t, true) +} + +func testFSLocalReaddir(t *testing.T, makeReadable bool) { + tmp := t.TempDir() + path := filepath.Join(tmp, "item") + rtest.OK(t, os.Mkdir(path, 0o700)) + entries := []string{"testfile"} + rtest.OK(t, os.WriteFile(filepath.Join(path, entries[0]), []byte("example"), 0o600)) + + f := openReadable(t, path, makeReadable) + checkMetadata(t, f, path, false, restic.NodeTypeDir) + + names, err := f.Readdirnames(-1) + rtest.OK(t, err) + slices.Sort(names) + rtest.Equals(t, entries, names, "directory content mismatch") + + rtest.OK(t, f.Close()) +} + +func TestFSLocalReadableRace(t *testing.T) { + tmp := t.TempDir() + path := filepath.Join(tmp, "item") + testdata := "example" + rtest.OK(t, os.WriteFile(path, []byte(testdata), 0o600)) + + testFs := &Local{} + f, err := testFs.OpenFile(path, O_NOFOLLOW, true) + rtest.OK(t, err) + + pathNew := path + "new" + rtest.OK(t, os.Rename(path, pathNew)) + + err = f.MakeReadable() + if err == nil { + // a file handle based implementation should still work + checkMetadata(t, f, pathNew, false, restic.NodeTypeFile) + + data, err := io.ReadAll(f) + rtest.OK(t, err) + rtest.Equals(t, testdata, string(data), "file content mismatch") + } + + rtest.OK(t, f.Close()) +} + +func TestFSLocalTypeChange(t *testing.T) { + tmp := t.TempDir() + path := filepath.Join(tmp, "item") + testdata := "example" + rtest.OK(t, os.WriteFile(path, []byte(testdata), 0o600)) + + testFs := &Local{} + f, err := testFs.OpenFile(path, O_NOFOLLOW, true) + rtest.OK(t, err) + // cache metadata + _, err = f.Stat() + rtest.OK(t, err) + + pathNew := path + "new" + // rename instead of unlink to let the test also work on windows + rtest.OK(t, os.Rename(path, pathNew)) + + rtest.OK(t, os.Mkdir(path, 0o700)) + rtest.OK(t, f.MakeReadable()) + + fi, err := f.Stat() + rtest.OK(t, err) + if !fi.IsDir() { + // a file handle based implementation should still reference the file + checkMetadata(t, f, pathNew, false, restic.NodeTypeFile) + + data, err := io.ReadAll(f) + rtest.OK(t, err) + rtest.Equals(t, testdata, string(data), "file content mismatch") + } + // else: + // path-based implementation + // nothing to test here. stat returned the new file type + + rtest.OK(t, f.Close()) +} diff --git a/internal/fs/fs_local_unix_test.go b/internal/fs/fs_local_unix_test.go new file mode 100644 index 00000000000..5bcb5efd032 --- /dev/null +++ b/internal/fs/fs_local_unix_test.go @@ -0,0 +1,40 @@ +//go:build unix + +package fs + +import ( + "syscall" + "testing" + + "github.com/restic/restic/internal/restic" + rtest "github.com/restic/restic/internal/test" +) + +func TestFSLocalMetadataUnix(t *testing.T) { + for _, test := range []fsLocalMetadataTestcase{ + { + name: "socket", + setup: func(t *testing.T, path string) { + fd, err := syscall.Socket(syscall.AF_UNIX, syscall.SOCK_STREAM, 0) + rtest.OK(t, err) + defer func() { + _ = syscall.Close(fd) + }() + + addr := &syscall.SockaddrUnix{Name: path} + rtest.OK(t, syscall.Bind(fd, addr)) + }, + nodeType: restic.NodeTypeSocket, + }, + { + name: "fifo", + setup: func(t *testing.T, path string) { + rtest.OK(t, mkfifo(path, 0o600)) + }, + nodeType: restic.NodeTypeFifo, + }, + // device files can only be created as root + } { + runFSLocalTestcase(t, test) + } +}