Merge 59d30d1ecf into caa2f8a0c0

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())