diff --git a/scanner/walk_dir_tree.go b/scanner/walk_dir_tree.go index 1864a6a44..6d5d8ac03 100644 --- a/scanner/walk_dir_tree.go +++ b/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 diff --git a/scanner/walk_dir_tree_test.go b/scanner/walk_dir_tree_test.go index 43939e5c2..1cb48aaca 100644 --- a/scanner/walk_dir_tree_test.go +++ b/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())