From a394a5029feffd0c44aaefca96c624d18705149e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Julio=20L=C3=B3pez?=
 <1953782+julio-lopez@users.noreply.github.com>
Date: Fri, 14 Feb 2025 23:38:32 -0800
Subject: [PATCH] fix(general): prevent infinite loop while resolving ignore
 file symlinks (#4413)

* test symlink infinite loop
* remove spurious error wrap
* cleanup error messages
* remove unneeded intermediate vars
* check error in loop in fs.GetAllEntries
  While cur is expected to be `nil` when there's an error, this
  makes the check explicit.

Ref:
- #2037
- #4190
---
 fs/entry.go                 | 10 +++-------
 fs/ignorefs/ignorefs.go     | 11 ++++++++---
 fs/ignorefs/resolve_test.go | 31 +++++++++++++++++++++++++++++++
 fs/localfs/local_fs.go      |  6 ++----
 4 files changed, 44 insertions(+), 14 deletions(-)
 create mode 100644 fs/ignorefs/resolve_test.go

diff --git a/fs/entry.go b/fs/entry.go
index f208166a7..74f9cdc9c 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 6c7277913..9fe7aa1d7 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/ignorefs/resolve_test.go b/fs/ignorefs/resolve_test.go
new file mode 100644
index 000000000..5e4d852b8
--- /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)
+}
diff --git a/fs/localfs/local_fs.go b/fs/localfs/local_fs.go
index b21ec972c..f3caa11c0 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 {