fix(scanner): skip symlinks that point back into the folder being scanned - #5334

A symlink pointing at a folder that is already part of the current branch of
the walk (e.g. `music/tracks/music -> ..`) made the scanner walk the same
folders again and again, re-adding the same files until the OS hit its
recursion or path limits.

Each folder now carries its resolved path, with symlinks followed, and the
walk keeps the resolved paths of the branch it is currently in. A child whose
resolved path is already in that branch is a cycle and is skipped. The set is
per branch rather than global, so two sibling folders linking to the same
target are both walked: that is the same folder reached twice, not a loop.

Resolution uses the storage's own resolver where the filesystem is backed by a
real one, and falls back to following the link chain with fs.ReadLink, which
keeps the comparison inside the FS-relative path space.

The tests give the walk a deadline and a folder cap, because an undetected
cycle does not fail, it runs forever. Verified both ways: green with the
detection, and failing on the deadline without it.

Fixes #5334

Signed-off-by: Junker der Provinz <133605895+junkerderprovinz@users.noreply.github.com>

scanner/walk_dir_tree.go

@@ -52,7 +52,7 @@ func walkDirTree(ctx context.Context, job *scanJob, targetFolders ...string) (<-
 			}
 
 			// Recursively walk this folder and all its children
-			err = walkFolder(ctx, job, folderPath, checker, results)
+			err = walkFolder(ctx, job, newDirRef(job.fs, folderPath), checker, results, map[string]struct{}{})
 			if utils.IsCtxDone(ctx) {
 				return
 			}
@@ -66,28 +66,44 @@ func walkDirTree(ctx context.Context, job *scanJob, targetFolders ...string) (<-
 	return results, nil
 }
 
-func walkFolder(ctx context.Context, job *scanJob, currentFolder string, checker *IgnoreChecker, results chan<- *folderEntry) error {
+// dirRef is a folder to be walked: its path in the library's filesystem and its resolved
+// path, with all symlinks followed. The resolved path is the folder's identity: two paths
+// that resolve to the same one are the same folder on disk, which is how symlink cycles
+// are detected while walking (see #5334).
+type dirRef struct {
+	path     string
+	realPath string
+}
+
+func walkFolder(ctx context.Context, job *scanJob, dir dirRef, checker *IgnoreChecker, results chan<- *folderEntry, branch map[string]struct{}) error {
 	// Push patterns for this folder onto the stack
-	_ = checker.Push(ctx, currentFolder)
+	_ = checker.Push(ctx, dir.path)
 	defer checker.Pop() // Pop patterns when leaving this folder
 
-	folder, children, err := loadDir(ctx, job, currentFolder, checker)
+	// Keep track of the folders in the current branch, so any symlink pointing back into
+	// one of them is recognized as a cycle and skipped. Removed again on the way out, so
+	// two sibling folders linking to the same target are both walked - that is not a
+	// cycle, just the same folder reached twice.
+	branch[dir.realPath] = struct{}{}
+	defer delete(branch, dir.realPath)
+
+	folder, children, err := loadDir(ctx, job, dir, checker, branch)
 	if err != nil {
-		log.Warn(ctx, "Scanner: Error loading dir. Skipping", "path", currentFolder, err)
+		log.Warn(ctx, "Scanner: Error loading dir. Skipping", "path", dir.path, err)
 		return nil
 	}
 	for _, c := range children {
-		err := walkFolder(ctx, job, c, checker, results)
+		err := walkFolder(ctx, job, c, checker, results, branch)
 		if err != nil {
 			return err
 		}
 	}
 
-	dir := path.Clean(currentFolder)
-	log.Trace(ctx, "Scanner: Found directory", " path", dir, "audioFiles", maps.Keys(folder.audioFiles),
+	cleanPath := path.Clean(dir.path)
+	log.Trace(ctx, "Scanner: Found directory", " path", cleanPath, "audioFiles", maps.Keys(folder.audioFiles),
 		"images", maps.Keys(folder.imageFiles), "playlists", len(folder.playlistFiles), "imagesUpdatedAt", folder.imagesUpdatedAt,
 		"updTime", folder.updTime, "modTime", folder.modTime, "numChildren", len(children))
-	folder.path = dir
+	folder.path = cleanPath
 	folder.elapsed.Start()
 
 	select {
@@ -98,7 +114,8 @@ func walkFolder(ctx context.Context, job *scanJob, currentFolder string, checker
 	}
 }
 
-func loadDir(ctx context.Context, job *scanJob, dirPath string, checker *IgnoreChecker) (folder *folderEntry, children []string, err error) {
+func loadDir(ctx context.Context, job *scanJob, dir dirRef, checker *IgnoreChecker, branch map[string]struct{}) (folder *folderEntry, children []dirRef, err error) {
+	dirPath := dir.path
 	// Check if directory exists before creating the folder entry
 	// This is important to avoid removing the folder from lastUpdates if it doesn't exist
 	dirInfo, err := fs.Stat(job.fs, dirPath)
@@ -111,20 +128,22 @@ func loadDir(ctx context.Context, job *scanJob, dirPath string, checker *IgnoreC
 	folder = job.createFolderEntry(dirPath)
 	folder.modTime = dirInfo.ModTime()
 
-	dir, err := job.fs.Open(dirPath)
+	// Named dirFile rather than dir: dir is now the folder reference this function
+	// received, and shadowing it here would silently walk the wrong path.
+	dirFile, err := job.fs.Open(dirPath)
 	if err != nil {
 		log.Warn(ctx, "Scanner: Error in Opening directory", "path", dirPath, err)
 		return folder, children, err
 	}
-	defer dir.Close()
-	dirFile, ok := dir.(fs.ReadDirFile)
+	defer dirFile.Close()
+	readDirFile, ok := dirFile.(fs.ReadDirFile)
 	if !ok {
 		log.Error(ctx, "Not a directory", "path", dirPath)
 		return folder, children, err
 	}
 
-	entries := fullReadDir(ctx, dirFile)
-	children = make([]string, 0, len(entries))
+	entries := fullReadDir(ctx, readDirFile)
+	children = make([]dirRef, 0, len(entries))
 	for _, entry := range entries {
 		entryPath := path.Join(dirPath, entry.Name())
 		if checker.ShouldIgnore(ctx, entryPath) {
@@ -144,7 +163,16 @@ func loadDir(ctx context.Context, job *scanJob, dirPath string, checker *IgnoreC
 			continue
 		}
 		if isDir && isDirReadable(ctx, job.fs, entryPath) {
-			children = append(children, entryPath)
+			child := newChildDirRef(job.fs, dir, entry)
+			// A symlink whose target is already in the current branch points back into a
+			// folder being walked right now: following it would walk the same folders
+			// forever. Everything else is walked normally.
+			if _, isCycle := branch[child.realPath]; isCycle {
+				log.Debug(ctx, "Scanner: Skipping symlink pointing back into a folder being scanned",
+					"path", entryPath, "target", child.realPath)
+				continue
+			}
+			children = append(children, child)
 			folder.numSubFolders++
 		} else {
 			fileInfo, err := entry.Info()
@@ -227,6 +255,72 @@ func isDirOrSymlinkToDir(fsys fs.FS, baseDir string, dirEnt fs.DirEntry) (bool,
 
 const maxSymlinkHops = 40
 
+// newDirRef returns the reference for the folder a walk starts at, resolving it in case
+// the folder itself is (or is reached through) a symlink.
+func newDirRef(fsys fs.FS, dirPath string) dirRef {
+	dir := dirRef{path: dirPath, realPath: path.Clean(dirPath)}
+	dir.realPath = resolveDirPath(fsys, dir)
+	return dir
+}
+
+// newChildDirRef returns the reference for a subfolder of dir. Only symlinks need to be
+// resolved: a real subfolder is always a new folder, it can never point back into one of
+// its own ancestors.
+func newChildDirRef(fsys fs.FS, dir dirRef, entry fs.DirEntry) dirRef {
+	child := dirRef{
+		path:     path.Join(dir.path, entry.Name()),
+		realPath: path.Join(dir.realPath, entry.Name()),
+	}
+	if entry.Type()&fs.ModeSymlink != 0 {
+		child.realPath = resolveDirPath(fsys, child)
+	}
+	return child
+}
+
+// resolveDirPath returns the path of the folder dir points to, with all symlinks
+// resolved. Storages backed by a real filesystem resolve the whole path at the OS level.
+// For any other filesystem the symlink chain is followed with fs.ReadLink, starting from
+// the parent's already resolved path, so the walk keeps comparing paths in the same
+// (FS-relative) space. If the path cannot be resolved it is returned as is, and the
+// folder is walked as any other.
+func resolveDirPath(fsys fs.FS, dir dirRef) string {
+	if resolver, ok := fsys.(storage.SymlinkResolverFS); ok {
+		target, err := resolver.ResolveSymlink(dir.path)
+		if err != nil {
+			return dir.realPath
+		}
+		return filepath.ToSlash(target)
+	}
+	target, hops := followSymlinkChain(fsys, dir.realPath)
+	if hops >= maxSymlinkHops {
+		return dir.realPath
+	}
+	return target
+}
+
+// followSymlinkChain follows the symlink chain starting at linkPath using fs.ReadLink,
+// and returns the last path it could reach, plus the number of hops it took to get there.
+// Zero hops means linkPath is not a symlink this filesystem can read; maxSymlinkHops means
+// the chain is too long to be resolved and is most likely a loop.
+func followSymlinkChain(fsys fs.FS, linkPath string) (string, int) {
+	cur := linkPath
+	hop := 0
+	for ; hop < maxSymlinkHops; hop++ {
+		target, err := fs.ReadLink(fsys, cur)
+		if err != nil {
+			break
+		}
+		if path.IsAbs(target) {
+			// Absolute targets are not valid fs.FS paths, so the next ReadLink fails and
+			// resolution stops here, leaving cur as the final target.
+			cur = target
+		} else {
+			cur = path.Join(path.Dir(cur), target)
+		}
+	}
+	return cur, hop
+}
+
 // resolveEntryName returns the name to classify the entry by, and whether to
 // consider it at all. Symlinks are resolved to their final target so the caller
 // classifies by the target's extension, not the link's name. Returns ok=false

scanner/walk_dir_tree_test.go

@@ -4,9 +4,12 @@ import (
 	"context"
 	"fmt"
 	"io/fs"
+	"maps"
 	"os"
 	"path/filepath"
+	"slices"
 	"testing/fstest"
+	"time"
 
 	"github.com/navidrome/navidrome/conf"
 	"github.com/navidrome/navidrome/conf/configtest"
@@ -150,6 +153,131 @@ var _ = Describe("walk_dir_tree", func() {
 			)
 		})
 
+		// Regression tests for #5334: a symlink pointing back into a folder that is already
+		// being walked made the scanner walk the same folders over and over, re-adding the
+		// same files until the OS hit its recursion/path limits.
+		Context("with symlink cycles", func() {
+			// A cycle that is not detected makes the walk run forever, so it gets a deadline
+			// and a hard cap on the number of folders. All layouts below are small enough to
+			// be walked instantly, and none of them has more folders than the cap.
+			const walkTimeout = 3 * time.Second
+			const maxFolders = 25
+
+			walkFS := func(musicFS storage.MusicFS, libPath string) map[string]*folderEntry {
+				job := &scanJob{fs: musicFS, lib: model.Library{Path: libPath}}
+				ctx, cancel := context.WithTimeout(GinkgoT().Context(), walkTimeout)
+				defer cancel()
+
+				results, err := walkDirTree(ctx, job)
+				Expect(err).ToNot(HaveOccurred())
+
+				folders := map[string]*folderEntry{}
+				for folder := range results {
+					folders[folder.path] = folder
+					if len(folders) >= maxFolders {
+						cancel()
+					}
+				}
+				Expect(ctx.Err()).ToNot(Equal(context.DeadlineExceeded), "the walk never finished: symlink cycle not detected")
+				return folders
+			}
+
+			walk := func(mapFS fstest.MapFS) map[string]*folderEntry {
+				return walkFS(&mockMusicFS{FS: mapFS}, "/music")
+			}
+
+			BeforeEach(func() {
+				DeferCleanup(configtest.SetupConfig())
+				conf.Server.Scanner.FollowSymlinks = true
+			})
+
+			It("skips a symlink pointing to the parent folder", func() {
+				folders := walk(fstest.MapFS{
+					"music/track1.mp3":        {},
+					"music/tracks/track2.mp3": {},
+					"music/tracks/music":      {Mode: fs.ModeSymlink, Data: []byte("..")},
+				})
+
+				Expect(slices.Collect(maps.Keys(folders))).To(ConsistOf(".", "music", "music/tracks"))
+				Expect(folders["music/tracks"].audioFiles).To(HaveLen(1))
+			})
+
+			It("skips a symlink pointing to its own folder", func() {
+				folders := walk(fstest.MapFS{
+					"music/track1.mp3": {},
+					"music/music":      {Mode: fs.ModeSymlink, Data: []byte(".")},
+				})
+
+				Expect(slices.Collect(maps.Keys(folders))).To(ConsistOf(".", "music"))
+				Expect(folders["music"].audioFiles).To(HaveLen(1))
+			})
+
+			It("skips a symlink closing a cycle between two folders", func() {
+				folders := walk(fstest.MapFS{
+					"lib/a/track1.mp3": {},
+					"lib/a/toB":        {Mode: fs.ModeSymlink, Data: []byte("../b")},
+					"lib/b/track2.mp3": {},
+					"lib/b/toA":        {Mode: fs.ModeSymlink, Data: []byte("../a")},
+				})
+
+				Expect(slices.Collect(maps.Keys(folders))).To(ConsistOf(".", "lib", "lib/a", "lib/a/toB", "lib/b", "lib/b/toA"))
+			})
+
+			It("still follows a symlink to a folder outside the current branch", func() {
+				folders := walk(fstest.MapFS{
+					"lib/real/track1.mp3": {},
+					"lib/alias":           {Mode: fs.ModeSymlink, Data: []byte("real")},
+				})
+
+				Expect(slices.Collect(maps.Keys(folders))).To(ConsistOf(".", "lib", "lib/real", "lib/alias"))
+				Expect(folders["lib/alias"].audioFiles).To(HaveKey("track1.mp3"))
+			})
+
+			It("does not follow the cycle when symlinks are disabled", func() {
+				conf.Server.Scanner.FollowSymlinks = false
+				folders := walk(fstest.MapFS{
+					"music/track1.mp3":        {},
+					"music/tracks/track2.mp3": {},
+					"music/tracks/music":      {Mode: fs.ModeSymlink, Data: []byte("..")},
+				})
+
+				Expect(slices.Collect(maps.Keys(folders))).To(ConsistOf(".", "music", "music/tracks"))
+			})
+
+			// The production localFS resolves symlinks at the OS level, a different code path
+			// than the in-memory filesystem used by the specs above.
+			Context("production local storage FS", func() {
+				var musicFS storage.MusicFS
+				var libRoot string
+
+				BeforeEach(func() {
+					tests.SkipOnWindows("symlink semantics")
+
+					// The layout reported in #5334: a subfolder linking back to the library root
+					libRoot = filepath.Join(GinkgoT().TempDir(), "music")
+					Expect(os.MkdirAll(filepath.Join(libRoot, "tracks"), 0755)).To(Succeed())
+					Expect(os.WriteFile(filepath.Join(libRoot, "track1.mp3"), []byte("AUDIO"), 0600)).To(Succeed())
+					Expect(os.WriteFile(filepath.Join(libRoot, "tracks", "track2.mp3"), []byte("AUDIO"), 0600)).To(Succeed())
+					Expect(os.Symlink("..", filepath.Join(libRoot, "tracks", "music"))).To(Succeed())
+
+					u, err := storage.LocalPathToURL(libRoot)
+					Expect(err).ToNot(HaveOccurred())
+					s, err := storage.For(u.String())
+					Expect(err).ToNot(HaveOccurred())
+					musicFS, err = s.FS()
+					Expect(err).ToNot(HaveOccurred())
+				})
+
+				It("skips a symlink pointing back into the library", func() {
+					folders := walkFS(musicFS, libRoot)
+
+					Expect(slices.Collect(maps.Keys(folders))).To(ConsistOf(".", "tracks"))
+					Expect(folders["."].audioFiles).To(HaveKey("track1.mp3"))
+					Expect(folders["tracks"].audioFiles).To(HaveKey("track2.mp3"))
+				})
+			})
+		})
+
 		Context("with target folders", func() {
 			BeforeEach(func() {
 				DeferCleanup(configtest.SetupConfig())