Merge pull request #4668 from MichaelEischer/backup-xattr-parent-enoperm

backup: Ignore xattr.list permission error for parent directories

changelog/unreleased/issue-3600

@@ -0,0 +1,11 @@
+Bugfix: `backup` works if xattrs above the backup target cannot be read
+
+When backup targets are specified using absolute paths, then `backup` also
+includes information about the parent folders of the backup targets in the
+snapshot. If the extended attributes for some of these folders could not be
+read due to missing permissions, this caused the backup to fail. This has been
+fixed.
+
+https://github.com/restic/restic/issues/3600
+https://github.com/restic/restic/pull/4668
+https://forum.restic.net/t/parent-directories-above-the-snapshot-source-path-fatal-error-permission-denied/7216

internal/archiver/archiver.go

@@ -237,8 +237,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) (*restic.Node, error) {
-	node, err := restic.NodeFromFileInfo(filename, fi)
+func (arch *Archiver) nodeFromFileInfo(snPath, filename string, fi os.FileInfo, ignoreXattrListError bool) (*restic.Node, error) {
+	node, err := restic.NodeFromFileInfo(filename, fi, ignoreXattrListError)
 	if !arch.WithAtime {
 		node.AccessTime = node.ModTime
 	}
@@ -289,7 +289,7 @@ func (arch *Archiver) wrapLoadTreeError(id restic.ID, err error) error {
 func (arch *Archiver) saveDir(ctx context.Context, snPath string, dir string, fi os.FileInfo, previous *restic.Tree, complete CompleteFunc) (d FutureNode, err error) {
 	debug.Log("%v %v", snPath, dir)
 
-	treeNode, err := arch.nodeFromFileInfo(snPath, dir, fi)
+	treeNode, err := arch.nodeFromFileInfo(snPath, dir, fi, false)
 	if err != nil {
 		return FutureNode{}, err
 	}
@@ -444,7 +444,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)
+				node, err := arch.nodeFromFileInfo(snPath, target, fi, false)
 				if err != nil {
 					return FutureNode{}, false, err
 				}
@@ -540,7 +540,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)
+		node, err := arch.nodeFromFileInfo(snPath, target, fi, false)
 		if err != nil {
 			return FutureNode{}, false, err
 		}
@@ -623,7 +623,9 @@ func (arch *Archiver) saveTree(ctx context.Context, snPath string, atree *Tree,
 		}
 
 		debug.Log("%v, dir node data loaded from %v", snPath, atree.FileInfoPath)
-		node, err = arch.nodeFromFileInfo(snPath, atree.FileInfoPath, fi)
+		// in some cases reading xattrs for directories above the backup target is not allowed
+		// thus ignore errors for such folders.
+		node, err = arch.nodeFromFileInfo(snPath, atree.FileInfoPath, fi, true)
 		if err != nil {
 			return FutureNode{}, 0, err
 		}

internal/archiver/archiver_test.go

@@ -556,7 +556,7 @@ func rename(t testing.TB, oldname, newname string) {
 }
 
 func nodeFromFI(t testing.TB, filename string, fi os.FileInfo) *restic.Node {
-	node, err := restic.NodeFromFileInfo(filename, fi)
+	node, err := restic.NodeFromFileInfo(filename, fi, false)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -2230,7 +2230,7 @@ func TestMetadataChanged(t *testing.T) {
 
 	// get metadata
 	fi := lstat(t, "testfile")
-	want, err := restic.NodeFromFileInfo("testfile", fi)
+	want, err := restic.NodeFromFileInfo("testfile", fi, false)
 	if err != nil {
 		t.Fatal(err)
 	}

internal/archiver/archiver_unix_test.go

@@ -48,7 +48,7 @@ func wrapFileInfo(fi os.FileInfo) os.FileInfo {
 
 func statAndSnapshot(t *testing.T, repo restic.Repository, name string) (*restic.Node, *restic.Node) {
 	fi := lstat(t, name)
-	want, err := restic.NodeFromFileInfo(name, fi)
+	want, err := restic.NodeFromFileInfo(name, fi, false)
 	rtest.OK(t, err)
 
 	_, node := snapshot(t, repo, fs.Local{}, nil, name)

internal/archiver/file_saver.go

@@ -29,7 +29,7 @@ type FileSaver struct {
 
 	CompleteBlob func(bytes uint64)
 
-	NodeFromFileInfo func(snPath, filename string, fi os.FileInfo) (*restic.Node, error)
+	NodeFromFileInfo func(snPath, filename string, fi os.FileInfo, ignoreXattrListError bool) (*restic.Node, error)
 }
 
 // NewFileSaver returns a new file saver. A worker pool with fileWorkers is
@@ -156,7 +156,7 @@ func (s *FileSaver) saveFile(ctx context.Context, chnker *chunker.Chunker, snPat
 
 	debug.Log("%v", snPath)
 
-	node, err := s.NodeFromFileInfo(snPath, f.Name(), fi)
+	node, err := s.NodeFromFileInfo(snPath, f.Name(), fi, false)
 	if err != nil {
 		_ = f.Close()
 		completeError(err)

internal/archiver/file_saver_test.go

@@ -49,8 +49,8 @@ func startFileSaver(ctx context.Context, t testing.TB) (*FileSaver, context.Cont
 	}
 
 	s := NewFileSaver(ctx, wg, saveBlob, pol, workers, workers)
-	s.NodeFromFileInfo = func(snPath, filename string, fi os.FileInfo) (*restic.Node, error) {
-		return restic.NodeFromFileInfo(filename, fi)
+	s.NodeFromFileInfo = func(snPath, filename string, fi os.FileInfo, ignoreXattrListError bool) (*restic.Node, error) {
+		return restic.NodeFromFileInfo(filename, fi, ignoreXattrListError)
 	}
 
 	return s, ctx, wg

internal/restic/node.go

@@ -134,7 +134,7 @@ func (node Node) String() string {
 
 // NodeFromFileInfo returns a new node from the given path and FileInfo. It
 // returns the first error that is encountered, together with a node.
-func NodeFromFileInfo(path string, fi os.FileInfo) (*Node, error) {
+func NodeFromFileInfo(path string, fi os.FileInfo, ignoreXattrListError bool) (*Node, error) {
 	mask := os.ModePerm | os.ModeType | os.ModeSetuid | os.ModeSetgid | os.ModeSticky
 	node := &Node{
 		Path:    path,
@@ -148,7 +148,7 @@ func NodeFromFileInfo(path string, fi os.FileInfo) (*Node, error) {
 		node.Size = uint64(fi.Size())
 	}
 
-	err := node.fillExtra(path, fi)
+	err := node.fillExtra(path, fi, ignoreXattrListError)
 	return node, err
 }
 
@@ -675,7 +675,7 @@ func lookupGroup(gid uint32) string {
 	return group
 }
 
-func (node *Node) fillExtra(path string, fi os.FileInfo) error {
+func (node *Node) fillExtra(path string, fi os.FileInfo, ignoreXattrListError bool) error {
 	stat, ok := toStatT(fi.Sys())
 	if !ok {
 		// fill minimal info with current values for uid, gid
@@ -719,7 +719,7 @@ func (node *Node) fillExtra(path string, fi os.FileInfo) error {
 	allowExtended, err := node.fillGenericAttributes(path, fi, stat)
 	if allowExtended {
 		// Skip processing ExtendedAttributes if allowExtended is false.
-		errEx := node.fillExtendedAttributes(path)
+		errEx := node.fillExtendedAttributes(path, ignoreXattrListError)
 		if err == nil {
 			err = errEx
 		} else {
@@ -729,10 +729,13 @@ func (node *Node) fillExtra(path string, fi os.FileInfo) error {
 	return err
 }
 
-func (node *Node) fillExtendedAttributes(path string) error {
+func (node *Node) fillExtendedAttributes(path string, ignoreListError bool) error {
 	xattrs, err := Listxattr(path)
 	debug.Log("fillExtendedAttributes(%v) %v %v", path, xattrs, err)
 	if err != nil {
+		if ignoreListError && IsListxattrPermissionError(err) {
+			return nil
+		}
 		return err
 	}
 

internal/restic/node_aix.go

@@ -33,6 +33,10 @@ func Listxattr(path string) ([]string, error) {
 	return nil, nil
 }
 
+func IsListxattrPermissionError(_ error) bool {
+	return false
+}
+
 // Setxattr is a no-op on AIX.
 func Setxattr(path, name string, data []byte) error {
 	return nil

internal/restic/node_netbsd.go

@@ -23,6 +23,10 @@ func Listxattr(path string) ([]string, error) {
 	return nil, nil
 }
 
+func IsListxattrPermissionError(_ error) bool {
+	return false
+}
+
 // Setxattr is a no-op on netbsd.
 func Setxattr(path, name string, data []byte) error {
 	return nil

internal/restic/node_openbsd.go

@@ -23,6 +23,10 @@ func Listxattr(path string) ([]string, error) {
 	return nil, nil
 }
 
+func IsListxattrPermissionError(_ error) bool {
+	return false
+}
+
 // Setxattr is a no-op on openbsd.
 func Setxattr(path, name string, data []byte) error {
 	return nil

internal/restic/node_test.go

@@ -11,6 +11,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
 	"github.com/restic/restic/internal/test"
 	rtest "github.com/restic/restic/internal/test"
 )
@@ -31,7 +32,7 @@ func BenchmarkNodeFillUser(t *testing.B) {
 	t.ResetTimer()
 
 	for i := 0; i < t.N; i++ {
-		_, err := NodeFromFileInfo(path, fi)
+		_, err := NodeFromFileInfo(path, fi, false)
 		rtest.OK(t, err)
 	}
 
@@ -55,7 +56,7 @@ func BenchmarkNodeFromFileInfo(t *testing.B) {
 	t.ResetTimer()
 
 	for i := 0; i < t.N; i++ {
-		_, err := NodeFromFileInfo(path, fi)
+		_, err := NodeFromFileInfo(path, fi, false)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -227,8 +228,11 @@ func TestNodeRestoreAt(t *testing.T) {
 			fi, err := os.Lstat(nodePath)
 			rtest.OK(t, err)
 
-			n2, err := NodeFromFileInfo(nodePath, fi)
+			n2, err := NodeFromFileInfo(nodePath, fi, false)
 			rtest.OK(t, err)
+			n3, err := NodeFromFileInfo(nodePath, fi, true)
+			rtest.OK(t, err)
+			rtest.Assert(t, n2.Equals(*n3), "unexpected node info mismatch %v", cmp.Diff(n2, n3))
 
 			rtest.Assert(t, test.Name == n2.Name,
 				"%v: name doesn't match (%v != %v)", test.Type, test.Name, n2.Name)

internal/restic/node_unix_test.go

@@ -128,7 +128,7 @@ func TestNodeFromFileInfo(t *testing.T) {
 				return
 			}
 
-			node, err := NodeFromFileInfo(test.filename, fi)
+			node, err := NodeFromFileInfo(test.filename, fi, false)
 			if err != nil {
 				t.Fatal(err)
 			}

internal/restic/node_windows.go

@@ -78,6 +78,10 @@ func Listxattr(path string) ([]string, error) {
 	return nil, nil
 }
 
+func IsListxattrPermissionError(_ error) bool {
+	return false
+}
+
 // Setxattr associates name and data together as an attribute of path.
 func Setxattr(path, name string, data []byte) error {
 	return nil

internal/restic/node_windows_test.go

@@ -165,7 +165,7 @@ func restoreAndGetNode(t *testing.T, tempDir string, testNode Node, warningExpec
 	fi, err := os.Lstat(testPath)
 	test.OK(t, errors.Wrapf(err, "Could not Lstat for path: %s", testPath))
 
-	nodeFromFileInfo, err := NodeFromFileInfo(testPath, fi)
+	nodeFromFileInfo, err := NodeFromFileInfo(testPath, fi, false)
 	test.OK(t, errors.Wrapf(err, "Could not get NodeFromFileInfo for path: %s", testPath))
 
 	return testPath, nodeFromFileInfo

internal/restic/node_xattr.go

@@ -25,6 +25,14 @@ func Listxattr(path string) ([]string, error) {
 	return l, handleXattrErr(err)
 }
 
+func IsListxattrPermissionError(err error) bool {
+	var xerr *xattr.Error
+	if errors.As(err, &xerr) {
+		return xerr.Op == "xattr.list" && errors.Is(xerr.Err, os.ErrPermission)
+	}
+	return false
+}
+
 // Setxattr associates name and data together as an attribute of path.
 func Setxattr(path, name string, data []byte) error {
 	return handleXattrErr(xattr.LSet(path, name, data))

internal/restic/node_xattr_test.go

@@ -0,0 +1,28 @@
+//go:build darwin || freebsd || linux || solaris
+// +build darwin freebsd linux solaris
+
+package restic
+
+import (
+	"os"
+	"testing"
+
+	"github.com/pkg/xattr"
+	rtest "github.com/restic/restic/internal/test"
+)
+
+func TestIsListxattrPermissionError(t *testing.T) {
+	xerr := &xattr.Error{
+		Op:   "xattr.list",
+		Name: "test",
+		Err:  os.ErrPermission,
+	}
+	err := handleXattrErr(xerr)
+	rtest.Assert(t, err != nil, "missing error")
+	rtest.Assert(t, IsListxattrPermissionError(err), "expected IsListxattrPermissionError to return true for %v", err)
+
+	xerr.Err = os.ErrNotExist
+	err = handleXattrErr(xerr)
+	rtest.Assert(t, err != nil, "missing error")
+	rtest.Assert(t, !IsListxattrPermissionError(err), "expected IsListxattrPermissionError to return false for %v", err)
+}

internal/restic/tree_test.go

@@ -86,7 +86,7 @@ func TestNodeComparison(t *testing.T) {
 	fi, err := os.Lstat("tree_test.go")
 	rtest.OK(t, err)
 
-	node, err := restic.NodeFromFileInfo("tree_test.go", fi)
+	node, err := restic.NodeFromFileInfo("tree_test.go", fi, false)
 	rtest.OK(t, err)
 
 	n2 := *node
@@ -127,7 +127,7 @@ func TestTreeEqualSerialization(t *testing.T) {
 		for _, fn := range files[:i] {
 			fi, err := os.Lstat(fn)
 			rtest.OK(t, err)
-			node, err := restic.NodeFromFileInfo(fn, fi)
+			node, err := restic.NodeFromFileInfo(fn, fi, false)
 			rtest.OK(t, err)
 
 			rtest.OK(t, tree.Insert(node))