From 09249ed1c629e412317779fc231d45fff4557cc0 Mon Sep 17 00:00:00 2001 From: junkerderprovinz Date: Fri, 9 Oct 2026 20:55:05 +0200 Subject: [PATCH] fix(scanner): skip symlinks back into the ancestors of a targeted folder A scan of a single folder started its branch at that folder, so a link like playlists/music -> .. walked the whole library again. Seed the branch with the resolved paths of the folders above the target. Also shortens the comments around the cycle check. Signed-off-by: junkerderprovinz --- scanner/walk_dir_tree.go | 52 +++++++++++++++------------------ scanner/walk_dir_tree_test.go | 54 ++++++++++++++++++++++------------- 2 files changed, 57 insertions(+), 49 deletions(-) diff --git a/scanner/walk_dir_tree.go b/scanner/walk_dir_tree.go index 10c0a4c9d..b93438a82 100644 --- a/scanner/walk_dir_tree.go +++ b/scanner/walk_dir_tree.go @@ -59,7 +59,7 @@ func walkDirTree(ctx context.Context, job *scanJob, targetFolders ...string) (<- } // Recursively walk this folder and all its children - err = walkFolder(ctx, job, newDirRef(job.fs, folderPath), checker, results, map[string]struct{}{}) + err = walkFolder(ctx, job, newDirRef(job.fs, folderPath), checker, results, ancestorsOf(job.fs, folderPath)) if utils.IsCtxDone(ctx) { return } @@ -73,10 +73,8 @@ func walkDirTree(ctx context.Context, job *scanJob, targetFolders ...string) (<- return results, nil } -// 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). +// dirRef is a folder to walk. realPath has all symlinks resolved and identifies the folder +// on disk, so a link back into one of its ancestors can be recognized as a cycle. type dirRef struct { path string realPath string @@ -87,10 +85,8 @@ func walkFolder(ctx context.Context, job *scanJob, dir dirRef, checker *IgnoreCh _ = checker.Push(ctx, dir.path) defer checker.Pop() // Pop patterns when leaving this folder - // 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 holds this folder and the ones above it. Only a link back into it is a cycle, two + // sibling links to the same folder are both walked. branch[dir.realPath] = struct{}{} defer delete(branch, dir.realPath) @@ -135,8 +131,6 @@ func loadDir(ctx context.Context, job *scanJob, dir dirRef, checker *IgnoreCheck folder = job.createFolderEntry(dirPath) folder.modTime = dirInfo.ModTime() - // 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) @@ -171,9 +165,6 @@ func loadDir(ctx context.Context, job *scanJob, dir dirRef, checker *IgnoreCheck } if isDir && isDirReadable(ctx, job.fs, 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) @@ -274,17 +265,26 @@ func isSymlinkedPath(fsys fs.FS, folderPath string) 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. +// newDirRef returns the reference for a folder a walk starts at, which may itself be 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. +// ancestorsOf returns the resolved paths of the folders above folderPath up to the library +// root, so walking only folderPath catches links back into them as a full walk would. +func ancestorsOf(fsys fs.FS, folderPath string) map[string]struct{} { + ancestors := map[string]struct{}{} + for p := path.Clean(folderPath); p != "." && p != "/"; { + p = path.Dir(p) + ancestors[newDirRef(fsys, p).realPath] = struct{}{} + } + return ancestors +} + +// newChildDirRef returns the reference for a subfolder of dir. Only symlinks need resolving, +// a real subfolder can never point back into its own ancestors. func newChildDirRef(fsys fs.FS, dir dirRef, entry fs.DirEntry) dirRef { child := dirRef{ path: path.Join(dir.path, entry.Name()), @@ -296,12 +296,8 @@ func newChildDirRef(fsys fs.FS, dir dirRef, entry fs.DirEntry) dirRef { 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. +// resolveDirPath returns dir's path with all symlinks resolved, or dir.realPath if it can't be +// resolved. Without OS-level resolution the chain is followed inside the FS from dir.realPath. func resolveDirPath(fsys fs.FS, dir dirRef) string { if resolver, ok := fsys.(storage.SymlinkResolverFS); ok { target, err := resolver.ResolveSymlink(dir.path) @@ -317,10 +313,8 @@ func resolveDirPath(fsys fs.FS, dir dirRef) string { 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. +// followSymlinkChain follows linkPath with fs.ReadLink and returns the last path reached and +// the hops taken. maxSymlinkHops hops means the chain most likely loops. func followSymlinkChain(fsys fs.FS, linkPath string) (string, int) { cur := linkPath hop := 0 diff --git a/scanner/walk_dir_tree_test.go b/scanner/walk_dir_tree_test.go index df5e62f1c..8dcfb7f0a 100644 --- a/scanner/walk_dir_tree_test.go +++ b/scanner/walk_dir_tree_test.go @@ -153,22 +153,19 @@ 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. + // See #5334 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. + // An undetected cycle walks forever, so each walk gets a deadline and a folder cap + // well above what any of these layouts contains. const walkTimeout = 3 * time.Second const maxFolders = 25 - walkFS := func(musicFS storage.MusicFS, libPath string) map[string]*folderEntry { + walkFS := func(musicFS storage.MusicFS, libPath string, targetFolders ...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) + results, err := walkDirTree(ctx, job, targetFolders...) Expect(err).ToNot(HaveOccurred()) folders := map[string]*folderEntry{} @@ -182,8 +179,8 @@ var _ = Describe("walk_dir_tree", func() { return folders } - walk := func(mapFS fstest.MapFS) map[string]*folderEntry { - return walkFS(&mockMusicFS{FS: mapFS}, "/music") + walk := func(mapFS fstest.MapFS, targetFolders ...string) map[string]*folderEntry { + return walkFS(&mockMusicFS{FS: mapFS}, "/music", targetFolders...) } BeforeEach(func() { @@ -244,8 +241,17 @@ var _ = Describe("walk_dir_tree", func() { 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. + It("skips a symlink back to the library root when walking only the folder holding it", func() { + folders := walk(fstest.MapFS{ + "artist/album/track1.mp3": {}, + "playlists/best.m3u": {}, + "playlists/music": {Mode: fs.ModeSymlink, Data: []byte("..")}, + }, "playlists") + + Expect(slices.Collect(maps.Keys(folders))).To(ConsistOf("playlists")) + }) + + // localFS resolves symlinks at the OS level instead of through fs.ReadLink Context("production local storage FS", func() { var musicFS storage.MusicFS var libRoot string @@ -253,12 +259,13 @@ var _ = Describe("walk_dir_tree", func() { BeforeEach(func() { tests.SkipOnWindows("symlink semantics") - // The layout reported in #5334: a subfolder linking back to the library root + // The layout reported in #5334: playlists/music links 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()) + Expect(os.MkdirAll(filepath.Join(libRoot, "artist", "album"), 0755)).To(Succeed()) + Expect(os.MkdirAll(filepath.Join(libRoot, "playlists"), 0755)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(libRoot, "artist", "album", "track1.mp3"), []byte("AUDIO"), 0600)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(libRoot, "playlists", "best.m3u"), []byte("#EXTM3U"), 0600)).To(Succeed()) + Expect(os.Symlink("..", filepath.Join(libRoot, "playlists", "music"))).To(Succeed()) u, err := storage.LocalPathToURL(libRoot) Expect(err).ToNot(HaveOccurred()) @@ -271,9 +278,16 @@ var _ = Describe("walk_dir_tree", func() { 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")) + Expect(slices.Collect(maps.Keys(folders))).To(ConsistOf(".", "artist", "artist/album", "playlists")) + Expect(folders["artist/album"].audioFiles).To(HaveKey("track1.mp3")) + Expect(folders["playlists"].playlistFiles).To(HaveKey("best.m3u")) + }) + + It("skips the same symlink when walking only the folder holding it", func() { + folders := walkFS(musicFS, libRoot, "playlists") + + Expect(slices.Collect(maps.Keys(folders))).To(ConsistOf("playlists")) + Expect(folders["playlists"].playlistFiles).To(HaveKey("best.m3u")) }) }) })