From 295ddb9e576df3f0c55d3a173ad684d4f8c2bf91 Mon Sep 17 00:00:00 2001 From: kitone Date: Fri, 28 Aug 2020 23:29:33 +0200 Subject: [PATCH 1/4] Add test case for inconsistent timestamps and permissions restoration Reproduce from https://github.com/restic/restic/issues/1212 --- internal/restorer/restorer_test.go | 128 +++++++++++++++++++++++++++-- 1 file changed, 122 insertions(+), 6 deletions(-) diff --git a/internal/restorer/restorer_test.go b/internal/restorer/restorer_test.go index 00c56bccd..d37cffb16 100644 --- a/internal/restorer/restorer_test.go +++ b/internal/restorer/restorer_test.go @@ -6,6 +6,7 @@ import ( "io/ioutil" "os" "path/filepath" + "runtime" "strings" "testing" "time" @@ -23,14 +24,17 @@ type Snapshot struct { } type File struct { - Data string - Links uint64 - Inode uint64 + Data string + Links uint64 + Inode uint64 + Mode os.FileMode + ModTime time.Time } type Dir struct { - Nodes map[string]Node - Mode os.FileMode + Nodes map[string]Node + Mode os.FileMode + ModTime time.Time } func saveFile(t testing.TB, repo restic.Repository, node File) restic.ID { @@ -66,9 +70,14 @@ func saveDir(t testing.TB, repo restic.Repository, nodes map[string]Node, inode if len(n.(File).Data) > 0 { fc = append(fc, saveFile(t, repo, node)) } + mode := node.Mode + if mode == 0 { + mode = 0644 + } tree.Insert(&restic.Node{ Type: "file", - Mode: 0644, + Mode: mode, + ModTime: node.ModTime, Name: name, UID: uint32(os.Getuid()), GID: uint32(os.Getgid()), @@ -88,6 +97,7 @@ func saveDir(t testing.TB, repo restic.Repository, nodes map[string]Node, inode tree.Insert(&restic.Node{ Type: "dir", Mode: mode, + ModTime: node.ModTime, Name: name, UID: uint32(os.Getuid()), GID: uint32(os.Getgid()), @@ -655,6 +665,7 @@ func TestRestorerTraverseTree(t *testing.T) { }, Visitor: checkVisitOrder([]TreeVisit{ {"visitNode", "/dir/otherfile"}, + {"leaveDir", "/dir"}, }), }, } @@ -688,3 +699,108 @@ func TestRestorerTraverseTree(t *testing.T) { }) } } + +func normalizeFileMode(mode os.FileMode) os.FileMode { + if runtime.GOOS == "windows" { + if mode.IsDir() { + return 0555 | os.ModeDir + } + return os.FileMode(0444) + } + return mode +} + +func checkConsistentInfo(t testing.TB, file string, fi os.FileInfo, modtime time.Time, mode os.FileMode) { + if fi.Mode() != mode { + t.Errorf("checking %q, Mode() returned wrong value, want 0%o, got 0%o", file, mode, fi.Mode()) + } + + if !fi.ModTime().Equal(modtime) { + t.Errorf("checking %s, ModTime() returned wrong value, want %v, got %v", file, modtime, fi.ModTime()) + } +} + +// test inspired from test case https://github.com/restic/restic/issues/1212 +func TestRestorerConsistentTimestampsAndPermissions(t *testing.T) { + timeForTest := time.Date(2019, time.January, 9, 1, 46, 40, 0, time.UTC) + + repo, cleanup := repository.TestRepository(t) + defer cleanup() + + _, id := saveSnapshot(t, repo, Snapshot{ + Nodes: map[string]Node{ + "dir": Dir{ + Mode: normalizeFileMode(0750 | os.ModeDir), + ModTime: timeForTest, + Nodes: map[string]Node{ + "file1": File{ + Mode: normalizeFileMode(os.FileMode(0700)), + ModTime: timeForTest, + Data: "content: file\n", + }, + "anotherfile": File{ + Data: "content: file\n", + }, + "subdir": Dir{ + Mode: normalizeFileMode(0700 | os.ModeDir), + ModTime: timeForTest, + Nodes: map[string]Node{ + "file2": File{ + Mode: normalizeFileMode(os.FileMode(0666)), + ModTime: timeForTest, + Links: 2, + Inode: 1, + }, + }, + }, + }, + }, + }, + }) + + res, err := NewRestorer(repo, id) + rtest.OK(t, err) + + res.SelectFilter = func(item string, dstpath string, node *restic.Node) (selectedForRestore bool, childMayBeSelected bool) { + switch filepath.ToSlash(item) { + case "/dir": + childMayBeSelected = true + case "/dir/file1": + selectedForRestore = true + childMayBeSelected = false + case "/dir/subdir": + selectedForRestore = true + childMayBeSelected = true + case "/dir/subdir/file2": + selectedForRestore = true + childMayBeSelected = false + } + return selectedForRestore, childMayBeSelected + } + + tempdir, cleanup := rtest.TempDir(t) + defer cleanup() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + err = res.RestoreTo(ctx, tempdir) + rtest.OK(t, err) + + var testPatterns = []struct { + path string + modtime time.Time + mode os.FileMode + }{ + {"dir", timeForTest, normalizeFileMode(0750 | os.ModeDir)}, + {filepath.Join("dir", "file1"), timeForTest, normalizeFileMode(os.FileMode(0700))}, + {filepath.Join("dir", "subdir"), timeForTest, normalizeFileMode(0700 | os.ModeDir)}, + {filepath.Join("dir", "subdir", "file2"), timeForTest, normalizeFileMode(os.FileMode(0666))}, + } + + for _, test := range testPatterns { + f, err := os.Stat(filepath.Join(tempdir, test.path)) + rtest.OK(t, err) + checkConsistentInfo(t, test.path, f, test.modtime, test.mode) + } +} From 9d7f616190c58fc569a6aed40943e64a088b97ea Mon Sep 17 00:00:00 2001 From: kitone Date: Sat, 29 Aug 2020 23:27:20 +0200 Subject: [PATCH 2/4] Improve restorer debug log information --- internal/restorer/restorer.go | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/internal/restorer/restorer.go b/internal/restorer/restorer.go index 06e590532..37b773e1b 100644 --- a/internal/restorer/restorer.go +++ b/internal/restorer/restorer.go @@ -90,7 +90,7 @@ func (res *Restorer) traverseTree(ctx context.Context, target, location string, } selectedForRestore, childMayBeSelected := res.SelectFilter(nodeLocation, nodeTarget, node) - debug.Log("SelectFilter returned %v %v", selectedForRestore, childMayBeSelected) + debug.Log("SelectFilter returned %v %v for %q", selectedForRestore, childMayBeSelected, nodeLocation) sanitizeError := func(err error) error { if err != nil { @@ -198,24 +198,23 @@ func (res *Restorer) RestoreTo(ctx context.Context, dst string) error { } } - restoreNodeMetadata := func(node *restic.Node, target, location string) error { - return res.restoreNodeMetadataTo(node, target, location) - } - noop := func(node *restic.Node, target, location string) error { return nil } - idx := restic.NewHardlinkIndex() filerestorer := newFileRestorer(dst, res.repo.Backend().Load, res.repo.Key(), res.repo.Index().Lookup) + debug.Log("first pass for %q", dst) + // first tree pass: create directories and collect all files to restore err = res.traverseTree(ctx, dst, string(filepath.Separator), *res.sn.Tree, treeVisitor{ enterDir: func(node *restic.Node, target, location string) error { + debug.Log("first pass, enterDir: mkdir %q, leaveDir should restore metadata", location) // create dir with default permissions // #leaveDir restores dir metadata after visiting all children return fs.MkdirAll(target, 0700) }, visitNode: func(node *restic.Node, target, location string) error { + debug.Log("first pass, visitNode: mkdir %q, leaveDir on second pass should restore metadata", location) // create parent dir with default permissions // second pass #leaveDir restores dir metadata after visiting/restoring all children err := fs.MkdirAll(filepath.Dir(target), 0700) @@ -242,7 +241,9 @@ func (res *Restorer) RestoreTo(ctx context.Context, dst string) error { return nil }, - leaveDir: noop, + leaveDir: func(node *restic.Node, target, location string) error { + return nil + }, }) if err != nil { return err @@ -253,10 +254,15 @@ func (res *Restorer) RestoreTo(ctx context.Context, dst string) error { return err } + debug.Log("second pass for %q", dst) + // second tree pass: restore special files and filesystem metadata return res.traverseTree(ctx, dst, string(filepath.Separator), *res.sn.Tree, treeVisitor{ - enterDir: noop, + enterDir: func(node *restic.Node, target, location string) error { + return nil + }, visitNode: func(node *restic.Node, target, location string) error { + debug.Log("second pass, visitNode: restore node %q", location) if node.Type != "file" { return res.restoreNodeTo(ctx, node, target, location) } @@ -275,7 +281,10 @@ func (res *Restorer) RestoreTo(ctx context.Context, dst string) error { return res.restoreNodeMetadataTo(node, target, location) }, - leaveDir: restoreNodeMetadata, + leaveDir: func(node *restic.Node, target, location string) error { + debug.Log("second pass, leaveDir restore metadata %q", location) + return res.restoreNodeMetadataTo(node, target, location) + }, }) } From 6099f81692be33f4c421dcc7243bf920bd159e94 Mon Sep 17 00:00:00 2001 From: kitone Date: Sun, 30 Aug 2020 00:08:38 +0200 Subject: [PATCH 3/4] Fix #1212 restore code produces inconsistent timestamps/permissions. Keep track of restored child status so parent and root directory not selected by filter will also restore metadata when traversing tree. --- internal/restorer/restorer.go | 46 ++++++++++++++++++++---------- internal/restorer/restorer_test.go | 2 +- 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/internal/restorer/restorer.go b/internal/restorer/restorer.go index 37b773e1b..48da0212e 100644 --- a/internal/restorer/restorer.go +++ b/internal/restorer/restorer.go @@ -49,12 +49,12 @@ type treeVisitor struct { // traverseTree traverses a tree from the repo and calls treeVisitor. // target is the path in the file system, location within the snapshot. -func (res *Restorer) traverseTree(ctx context.Context, target, location string, treeID restic.ID, visitor treeVisitor) error { +func (res *Restorer) traverseTree(ctx context.Context, target, location string, treeID restic.ID, visitor treeVisitor) (hasRestored bool, err error) { debug.Log("%v %v %v", target, location, treeID) tree, err := res.repo.LoadTree(ctx, treeID) if err != nil { debug.Log("error loading tree %v: %v", treeID, err) - return res.Error(location, err) + return hasRestored, res.Error(location, err) } for _, node := range tree.Nodes { @@ -66,7 +66,7 @@ func (res *Restorer) traverseTree(ctx context.Context, target, location string, debug.Log("node %q has invalid name %q", node.Name, nodeName) err := res.Error(location, errors.Errorf("invalid child node name %s", node.Name)) if err != nil { - return err + return hasRestored, err } continue } @@ -79,7 +79,7 @@ func (res *Restorer) traverseTree(ctx context.Context, target, location string, debug.Log("node %q has invalid target path %q", node.Name, nodeTarget) err := res.Error(nodeLocation, errors.New("node has invalid path")) if err != nil { - return err + return hasRestored, err } continue } @@ -92,6 +92,10 @@ func (res *Restorer) traverseTree(ctx context.Context, target, location string, selectedForRestore, childMayBeSelected := res.SelectFilter(nodeLocation, nodeTarget, node) debug.Log("SelectFilter returned %v %v for %q", selectedForRestore, childMayBeSelected, nodeLocation) + if selectedForRestore { + hasRestored = true + } + sanitizeError := func(err error) error { if err != nil { err = res.Error(nodeLocation, err) @@ -101,27 +105,38 @@ func (res *Restorer) traverseTree(ctx context.Context, target, location string, if node.Type == "dir" { if node.Subtree == nil { - return errors.Errorf("Dir without subtree in tree %v", treeID.Str()) + return hasRestored, errors.Errorf("Dir without subtree in tree %v", treeID.Str()) } if selectedForRestore { err = sanitizeError(visitor.enterDir(node, nodeTarget, nodeLocation)) if err != nil { - return err + return hasRestored, err } } + // keep track of restored child status + // so metadata of the current directory are restored on leaveDir + childHasRestored := false + if childMayBeSelected { - err = sanitizeError(res.traverseTree(ctx, nodeTarget, nodeLocation, *node.Subtree, visitor)) + childHasRestored, err = res.traverseTree(ctx, nodeTarget, nodeLocation, *node.Subtree, visitor) + err = sanitizeError(err) if err != nil { - return err + return hasRestored, err + } + // inform the parent directory to restore parent metadata on leaveDir if needed + if childHasRestored { + hasRestored = true } } - if selectedForRestore { + // metadata need to be restore when leaving the directory in both cases + // selected for restore or any child of any subtree have been restored + if selectedForRestore || childHasRestored { err = sanitizeError(visitor.leaveDir(node, nodeTarget, nodeLocation)) if err != nil { - return err + return hasRestored, err } } @@ -131,12 +146,12 @@ func (res *Restorer) traverseTree(ctx context.Context, target, location string, if selectedForRestore { err = sanitizeError(visitor.visitNode(node, nodeTarget, nodeLocation)) if err != nil { - return err + return hasRestored, err } } } - return nil + return hasRestored, nil } func (res *Restorer) restoreNodeTo(ctx context.Context, node *restic.Node, target, location string) error { @@ -205,7 +220,7 @@ func (res *Restorer) RestoreTo(ctx context.Context, dst string) error { debug.Log("first pass for %q", dst) // first tree pass: create directories and collect all files to restore - err = res.traverseTree(ctx, dst, string(filepath.Separator), *res.sn.Tree, treeVisitor{ + _, err = res.traverseTree(ctx, dst, string(filepath.Separator), *res.sn.Tree, treeVisitor{ enterDir: func(node *restic.Node, target, location string) error { debug.Log("first pass, enterDir: mkdir %q, leaveDir should restore metadata", location) // create dir with default permissions @@ -257,7 +272,7 @@ func (res *Restorer) RestoreTo(ctx context.Context, dst string) error { debug.Log("second pass for %q", dst) // second tree pass: restore special files and filesystem metadata - return res.traverseTree(ctx, dst, string(filepath.Separator), *res.sn.Tree, treeVisitor{ + _, err = res.traverseTree(ctx, dst, string(filepath.Separator), *res.sn.Tree, treeVisitor{ enterDir: func(node *restic.Node, target, location string) error { return nil }, @@ -286,6 +301,7 @@ func (res *Restorer) RestoreTo(ctx context.Context, dst string) error { return res.restoreNodeMetadataTo(node, target, location) }, }) + return err } // Snapshot returns the snapshot this restorer is configured to use. @@ -298,7 +314,7 @@ func (res *Restorer) VerifyFiles(ctx context.Context, dst string) (int, error) { // TODO multithreaded? count := 0 - err := res.traverseTree(ctx, dst, string(filepath.Separator), *res.sn.Tree, treeVisitor{ + _, err := res.traverseTree(ctx, dst, string(filepath.Separator), *res.sn.Tree, treeVisitor{ enterDir: func(node *restic.Node, target, location string) error { return nil }, visitNode: func(node *restic.Node, target, location string) error { if node.Type != "file" { diff --git a/internal/restorer/restorer_test.go b/internal/restorer/restorer_test.go index d37cffb16..661191410 100644 --- a/internal/restorer/restorer_test.go +++ b/internal/restorer/restorer_test.go @@ -692,7 +692,7 @@ func TestRestorerTraverseTree(t *testing.T) { // make sure we're creating a new subdir of the tempdir target := filepath.Join(tempdir, "target") - err = res.traverseTree(ctx, target, string(filepath.Separator), *sn.Tree, test.Visitor(t)) + _, err = res.traverseTree(ctx, target, string(filepath.Separator), *sn.Tree, test.Visitor(t)) if err != nil { t.Fatal(err) } From 052564007a723a3f4950ba75e6f5d9ae0d925c43 Mon Sep 17 00:00:00 2001 From: kitone Date: Fri, 9 Oct 2020 23:10:24 +0200 Subject: [PATCH 4/4] Add changelog --- changelog/unreleased/issue-1212 | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 changelog/unreleased/issue-1212 diff --git a/changelog/unreleased/issue-1212 b/changelog/unreleased/issue-1212 new file mode 100644 index 000000000..7a5a470d2 --- /dev/null +++ b/changelog/unreleased/issue-1212 @@ -0,0 +1,11 @@ +Bugfix: Restore timestamps and permissions on intermediate directories + +When using the `--include` option of the restore command, restic restored +timestamps and permissions only on directories selected by the include pattern. +Intermediate directories, which are necessary to restore files located in sub- +directories, were created with default permissions. We've fixed the restore +command to restore timestamps and permissions for these directories as well. + +https://github.com/restic/restic/issues/1212 +https://github.com/restic/restic/issues/1402 +https://github.com/restic/restic/pull/2906