Repository: kopia/kopia
Pull request #4413 (2025-02-15 07:07:30)

fix(general): prevent infinite loop while resolving ignore file symlinks
Other cleanups:
- simplify error messages
- remove spurious error wrapping

Ref:
- #2037
- #4190

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/kopia/kopia/pull/4413?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) Report
Attention: Patch coverage is `81.81818%` with `2 lines` in your changes missing coverage. Please review.
> Project coverage is 76.33%. Comparing base [(`cb455c6`)](https://app.codecov.io/gh/kopia/kopia/commit/cb455c699a04e54cb47e25a676d7d65063ce883e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) to head [(`5a39740`)](https://app.codecov.io/gh/kopia/kopia/commit/5a397404d31734198f2e2b299c2f9ee470735a70?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia).
> Report is 421 commits behind head on master.

| [Files with missing lines](https://app.codecov.io/gh/kopia/kopia/pull/4413?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) | Patch % | Lines |
|---|---|---|
| [fs/ignorefs/ignorefs.go](https://app.codecov.io/gh/kopia/kopia/pull/4413?src=pr&el=tree&filepath=fs%2Fignorefs%2Fignorefs.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia#diff-ZnMvaWdub3JlZnMvaWdub3JlZnMuZ28=) | 83.33% | [1 Missing :warning: ](https://app.codecov.io/gh/kopia/kopia/pull/4413?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) |
| [fs/localfs/local\_fs.go](https://app.codecov.io/gh/kopia/kopia/pull/4413?src=pr&el=tree&filepath=fs%2Flocalfs%2Flocal_fs.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia#diff-ZnMvbG9jYWxmcy9sb2NhbF9mcy5nbw==) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/kopia/kopia/pull/4413?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #4413      +/-   ##
==========================================
+ Coverage   75.86%   76.33%   +0.46%     
==========================================
  Files         470      527      +57     
  Lines       37301    39933    +2632     
==========================================
+ Hits        28299    30482    +2183     
- Misses       7071     7445     +374     
- Partials     1931     2006      +75     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/kopia/kopia/pull/4413?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia).


Patch:
diff --git a/fs/entry.go b/fs/entry.go
index f208166a..74f9cdc9 100644
--- a/fs/entry.go
+++ b/fs/entry.go
@@ -72,19 +72,15 @@ type Directory interface {
 func IterateEntries(ctx context.Context, dir Directory, cb func(context.Context, Entry) error) error {
 	iter, err := dir.Iterate(ctx)
 	if err != nil {
-		return errors.Wrapf(err, "in fs.IterateEntries, creating iterator for directory %s", dir.Name())
+		return errors.Wrapf(err, "cannot iterate directory '%q'", dir.Name())
 	}
 
 	defer iter.Close()
 
 	cur, err := iter.Next(ctx)
-	if err != nil {
-		err = errors.Wrapf(err, "in fs.IterateEntries, on first iteration")
-	}
-
 	for cur != nil {
 		if err2 := cb(ctx, cur); err2 != nil {
-			return errors.Wrapf(err2, "in fs.IterateEntries, while calling callback on file %s", cur.Name())
+			return errors.Wrapf(err2, "callback failed on '%q'", cur.Name())
 		}
 
 		cur, err = iter.Next(ctx)
@@ -136,7 +132,7 @@ func GetAllEntries(ctx context.Context, d Directory) ([]Entry, error) {
 	defer iter.Close()
 
 	cur, err := iter.Next(ctx)
-	for cur != nil {
+	for err == nil && cur != nil {
 		entries = append(entries, cur)
 		cur, err = iter.Next(ctx)
 	}
diff --git a/fs/ignorefs/ignorefs.go b/fs/ignorefs/ignorefs.go
index 6c727791..9fe7aa1d 100644
--- a/fs/ignorefs/ignorefs.go
+++ b/fs/ignorefs/ignorefs.go
@@ -20,6 +20,7 @@
 var (
 	log                = logging.Module("ignorefs")
 	errSymlinkNotAFile = errors.New("Symlink does not link to a file")
+	errTooManySymlinks = errors.New("too many levels of symbolic links")
 )
 
 // IgnoreCallback is a function called by ignorefs to report whenever a file or directory is being ignored while listing its parent.
@@ -188,12 +189,12 @@ func (d *ignoreDirectory) Iterate(ctx context.Context) (fs.DirectoryIterator, er
 
 	thisContext, err := d.buildContext(ctx)
 	if err != nil {
-		return nil, errors.Wrapf(err, "in ignoreDirectory.Iterate, when building context")
+		return nil, err
 	}
 
 	inner, err := d.Directory.Iterate(ctx)
 	if err != nil {
-		return nil, errors.Wrapf(err, "in ignoreDirectory.Iterate, when creating iterator")
+		return nil, errors.Wrap(err, "cannot create iterator")
 	}
 
 	it := ignoreDirIteratorPool.Get().(*ignoreDirIterator) //nolint:forcetypeassert
@@ -275,7 +276,9 @@ func (d *ignoreDirectory) Child(ctx context.Context, name string) (fs.Entry, err
 }
 
 func resolveSymlink(ctx context.Context, entry fs.Symlink) (fs.File, error) {
-	for {
+	const maxSymlinkFollow = 30
+
+	for range maxSymlinkFollow {
 		target, err := entry.Resolve(ctx)
 		if err != nil {
 			link, _ := entry.Readlink(ctx)
@@ -292,6 +295,8 @@ func resolveSymlink(ctx context.Context, entry fs.Symlink) (fs.File, error) {
 			return nil, errors.Wrapf(errSymlinkNotAFile, "%s does not eventually link to a file", entry.Name())
 		}
 	}
+
+	return nil, errors.Wrapf(errTooManySymlinks, "cannot resolve '%q'", entry.Name())
 }
 
 func (d *ignoreDirectory) buildContext(ctx context.Context) (*ignoreContext, error) {
diff --git a/fs/localfs/local_fs.go b/fs/localfs/local_fs.go
index b21ec972..f3caa11c 100644
--- a/fs/localfs/local_fs.go
+++ b/fs/localfs/local_fs.go
@@ -120,12 +120,10 @@ func (fsl *filesystemSymlink) Readlink(ctx context.Context) (string, error) {
 func (fsl *filesystemSymlink) Resolve(ctx context.Context) (fs.Entry, error) {
 	target, err := filepath.EvalSymlinks(fsl.fullPath())
 	if err != nil {
-		return nil, errors.Wrapf(err, "while reading symlink %s", fsl.fullPath())
+		return nil, errors.Wrapf(err, "cannot resolve symlink for '%q'", fsl.fullPath())
 	}
 
-	entry, err := NewEntry(target)
-
-	return entry, err
+	return NewEntry(target)
 }
 
 func (e *filesystemErrorEntry) ErrorInfo() error {


Test patch:
diff --git a/fs/ignorefs/resolve_test.go b/fs/ignorefs/resolve_test.go
new file mode 100644
index 00000000..5e4d852b
--- /dev/null
+++ b/fs/ignorefs/resolve_test.go
@@ -0,0 +1,31 @@
+package ignorefs
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/kopia/kopia/fs"
+	"github.com/kopia/kopia/internal/mockfs"
+	"github.com/kopia/kopia/internal/testlogging"
+)
+
+func TestNoInfiniteResolveLink(t *testing.T) {
+	root := mockfs.NewDirectory()
+
+	root.AddSymlink("a", "./b", 0)
+	root.AddSymlink("b", "./c", 0)
+	root.AddSymlink("c", "./a", 0)
+
+	ctx := testlogging.Context(t)
+	e, err := root.Child(ctx, "b")
+	require.NoError(t, err)
+
+	s, ok := e.(fs.Symlink)
+	require.True(t, ok)
+
+	f, err := resolveSymlink(ctx, s)
+
+	require.ErrorIs(t, err, errTooManySymlinks)
+	require.Nil(t, f)
+}