From abe45a4d2ab196ac06bc1d3a75817caba838f1a2 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 21 Sep 2026 20:35:55 +0000 Subject: [PATCH 1/2] fix: survive an unreadable directory and a symlink cycle while scanning [patch] FileScanner walked the tree with SearchOption.AllDirectories and no EnumerationOptions, which put two failures on one unguarded call. Enumeration is lazy, so an UnauthorizedAccessException raised while descending into a single unreadable subdirectory propagated out of the foreach and out of Program.Main, abandoning the whole scan before any file was hashed, reported or deleted -- however much of the tree was readable. A restricted share, an OS-protected folder or a lost+found anywhere under the scan root was enough. Reparse points were also followed, so a directory symlink pointing at one of its own ancestors was descended into repeatedly. Observed against the old code: it took 23 laps of a/b/loop/b/loop/... before dying on an unhandled ArgumentException out of the AbsoluteFilePath conversion, the path having grown past what the semantic type accepts. Walk one directory at a time instead, materializing each listing inside a guard that reports and skips the directory it could not read, and skip reparse points so a cycle cannot be entered. Listing each directory separately is what keeps a failure local to its own directory rather than ending the walk. The unreadable-directory test stages the refusal with UnixFileMode and measures whether it took effect, following DeletionBlock and its test: a privileged process reads the directory regardless, so the test reports itself inconclusive there rather than passing without observing anything. Fixes #125 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NPXBwr1rm8hybVmmWhx7wd --- .gitignore | 18 +++++ FileDeduplicator.Test/DirectoryReadBlock.cs | 75 ++++++++++++++++++ .../FileScannerAndHasherTests.cs | 61 +++++++++++++++ FileDeduplicator/FileScanner.cs | 77 ++++++++++++++++++- 4 files changed, 229 insertions(+), 2 deletions(-) create mode 100644 FileDeduplicator.Test/DirectoryReadBlock.cs diff --git a/.gitignore b/.gitignore index dc0470a..e043c9f 100644 --- a/.gitignore +++ b/.gitignore @@ -203,6 +203,11 @@ PublishScripts/ **/[Pp]ackages/* # except build/, which is used as an MSBuild target. !**/[Pp]ackages/build/ +# and except a Unity project's Packages/, which is source: Unity's package manifest and its +# resolved lock file are both meant to be committed, and a NuGet restore folder never contains +# a file by either name. +!**/[Pp]ackages/manifest.json +!**/[Pp]ackages/packages-lock.json # Uncomment if necessary however generally it will be regenerated when needed #!**/[Pp]ackages/repositories.config # NuGet v3's project.json files produces more ignorable files @@ -651,3 +656,16 @@ Temporary Items # ImGui.ini files imgui.ini + +# Game engine projects +# +# Godot: the import cache, and the mono/temp bin+obj a C# build writes. +.godot/ + +# Unity: .meta files are source, not the Visual Studio C++ build artifact that the `*.meta` rule +# further up targets. Unity generates one per asset and it carries the GUID that scenes, prefabs +# and serialized references point at, so ignoring them gives every clone fresh GUIDs and silently +# breaks those references - including for a plug-in whose .dll is itself a build output. This +# negation has to come after that rule to win, and is scoped to the asset tree so the Visual +# Studio artifact stays ignored everywhere else. +!**/[Aa]ssets/**/*.meta diff --git a/FileDeduplicator.Test/DirectoryReadBlock.cs b/FileDeduplicator.Test/DirectoryReadBlock.cs new file mode 100644 index 0000000..3dae52c --- /dev/null +++ b/FileDeduplicator.Test/DirectoryReadBlock.cs @@ -0,0 +1,75 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.FileDeduplicator.Test; + +/// +/// Makes a directory refuse to be listed for the lifetime of the block, and puts the permissions +/// back on disposal. +/// +/// +/// The Unix arrangement is to drop read and execute on the directory, which makes +/// throw -- +/// the failure a scan descending into someone else's folder, or an OS-protected one, has to +/// survive. Windows refuses a listing only through an ACL deny entry rather than a file attribute, +/// so nothing is staged there and reports false, the same way it does for a +/// privileged process. +/// +internal sealed class DirectoryReadBlock : IDisposable +{ + private readonly string directory; + private readonly UnixFileMode originalMode; + + /// + /// Gets whether the block actually holds. A process running as root lists unreadable + /// directories regardless, so the staged failure never happens and a test relying on it has + /// nothing to observe. + /// + internal bool IsEnforced { get; } + + /// + /// Blocks listing of a directory, then measures whether the block took effect. + /// + /// The directory to protect. + internal DirectoryReadBlock(string target) + { + directory = target; + + if (OperatingSystem.IsWindows()) + { + IsEnforced = false; + return; + } + + originalMode = File.GetUnixFileMode(directory); + File.SetUnixFileMode(directory, UnixFileMode.None); + IsEnforced = ListingIsRefused(); + } + + /// + /// Tries the listing the block is meant to prevent, rather than guessing from the platform and + /// the user id. + /// + /// if listing the directory was refused. + private bool ListingIsRefused() + { + try + { + // Materialized, because the enumerator is lazy and raises nothing until it is walked. + _ = Directory.EnumerateFileSystemEntries(directory).ToList(); + return false; + } + catch (UnauthorizedAccessException) + { + return true; + } + } + + /// + public void Dispose() + { + if (!OperatingSystem.IsWindows()) + { + File.SetUnixFileMode(directory, originalMode); + } + } +} diff --git a/FileDeduplicator.Test/FileScannerAndHasherTests.cs b/FileDeduplicator.Test/FileScannerAndHasherTests.cs index 9f65b9a..62966c3 100644 --- a/FileDeduplicator.Test/FileScannerAndHasherTests.cs +++ b/FileDeduplicator.Test/FileScannerAndHasherTests.cs @@ -66,6 +66,67 @@ public void ScanOfAMissingDirectoryReturnsNothing() Assert.IsEmpty(files); } + /// + /// One unreadable directory must not abort the scan. Enumeration is lazy, so the refusal + /// arrives partway through the walk, after files elsewhere in the tree have already been found + /// and with the rest still to visit. + /// + [TestMethod] + public void ScanCarriesOnPastADirectoryItCannotRead() + { + // Arrange -- a readable file on either side of the unreadable directory in the walk + using TempTree tree = new(); + AbsoluteFilePath top = tree.Write("top.txt", "a"); + AbsoluteFilePath sibling = tree.Write("readable/mid.txt", "b"); + _ = tree.Write("denied/hidden-from-the-scan.txt", "c"); + string denied = Path.Combine(tree.Root.WeakString, "denied"); + + using DirectoryReadBlock block = new(denied); + + if (!block.IsEnforced) + { + Assert.Inconclusive("This process lists directories it has no permission to read, so the refusal under test cannot be staged. Run the tests as an unprivileged user."); + } + + // Act + IReadOnlyList files = FileScanner.ScanForFiles(tree.Root); + + // Assert -- everything readable is still found, and the run did not throw + Assert.HasCount(2, files); + Assert.Contains(top, files); + Assert.Contains(sibling, files); + } + + /// + /// A directory symlink pointing back at one of its own ancestors must not be followed. Build + /// caches and some backup layouts create these, and descending into one does not terminate. + /// + [TestMethod] + public void ScanTerminatesOnADirectorySymlinkCycle() + { + // Arrange -- a/b/loop -> a, so descending revisits a forever + using TempTree tree = new(); + AbsoluteFilePath real = tree.Write("a/b/real.txt", "content"); + string ancestor = Path.Combine(tree.Root.WeakString, "a"); + string loop = Path.Combine(ancestor, "b", "loop"); + + try + { + _ = Directory.CreateSymbolicLink(loop, ancestor); + } + catch (Exception ex) when (ex is UnauthorizedAccessException or IOException) + { + Assert.Inconclusive($"This process cannot create a directory symlink, so the cycle under test cannot be staged: {ex.Message}"); + } + + // Act -- hangs rather than returning if the link is followed + IReadOnlyList files = FileScanner.ScanForFiles(tree.Root); + + // Assert -- the real file is found once, not once per lap around the cycle + Assert.ContainsSingle(files); + Assert.Contains(real, files); + } + /// /// Identical content must hash identically regardless of the file's name or location. /// diff --git a/FileDeduplicator/FileScanner.cs b/FileDeduplicator/FileScanner.cs index 9e7f1f5..ec7b94d 100644 --- a/FileDeduplicator/FileScanner.cs +++ b/FileDeduplicator/FileScanner.cs @@ -18,12 +18,85 @@ internal static IReadOnlyList ScanForFiles(AbsoluteDirectoryPa return []; } + // Walked one directory at a time rather than with SearchOption.AllDirectories, which + // enumerates lazily: an UnauthorizedAccessException raised while descending into a single + // unreadable subdirectory propagates out of the loop and abandons the whole scan, however + // much of the tree was readable. Listing each directory on its own keeps a failure local to + // the directory that caused it, and lets the scan say which one it gave up on. List files = []; - foreach (string file in Directory.EnumerateFiles(path.WeakString, "*", SearchOption.AllDirectories)) + Queue pending = new(); + pending.Enqueue(path.WeakString); + + while (pending.Count > 0) { - files.Add(file.As()); + string directory = pending.Dequeue(); + + foreach (string file in List(directory, Directory.EnumerateFiles)) + { + files.Add(file.As()); + } + + foreach (string subdirectory in List(directory, Directory.EnumerateDirectories)) + { + if (IsReparsePoint(subdirectory)) + { + // A directory symlink or junction pointing at one of its own ancestors -- build + // caches and some backup layouts produce these -- would otherwise be descended + // into until the path stopped being legal. Skipping reparse points also stops + // content reachable by two routes being scanned, and reported as duplicated, + // twice. + Console.WriteLine($" Skipped link: {subdirectory}"); + continue; + } + + pending.Enqueue(subdirectory); + } } return files; } + + /// + /// Lists one directory's entries, reporting and skipping it if it cannot be read. + /// + /// + /// The result is materialized inside the guard on purpose. Both enumerators are lazy, so a + /// caller iterating one outside this method would see the exception raised on whichever element + /// triggered it, not here. + /// + private static List List(string directory, Func> enumerate) + { + try + { + return [.. enumerate(directory)]; + } + catch (UnauthorizedAccessException ex) + { + Console.WriteLine($" Skipped {directory}: {ex.Message}"); + return []; + } + catch (IOException ex) + { + Console.WriteLine($" Skipped {directory}: {ex.Message}"); + return []; + } + } + + private static bool IsReparsePoint(string directory) + { + try + { + return File.GetAttributes(directory).HasFlag(FileAttributes.ReparsePoint); + } + catch (UnauthorizedAccessException) + { + // Unreadable attributes are not grounds for following the link; the listing guard above + // reports the directory when the descent then fails to read it. + return true; + } + catch (IOException) + { + return true; + } + } } From 4228eda31a409544a8189fc0d262158bbdc5a2a8 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 21 Sep 2026 20:47:22 +0000 Subject: [PATCH 2/2] refactor: walk the scan with EnumerationOptions instead of a manual descent [patch] The manual walk carried four catch branches that only execute on an unprivileged process, so SonarCloud measured 70.8% coverage on new code against a gate of 80%. An uncoverable branch in a tool that deletes files is worse than the log line it was there to print, so this trades that reporting for enumeration options the runtime applies itself: IgnoreInaccessible = true skips an unreadable directory instead of ending the walk AttributesToSkip = ReparsePoint cannot descend into a symlink cycle Both acceptance criteria still hold, and both tests still cover them -- including the symlink-cycle test, which still fails against the original code with the same unhandled ArgumentException after 23 laps of a/b/loop/... Hidden and system files stay in scope. A bare new EnumerationOptions() would skip them, where the SearchOption overload this replaces did not. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NPXBwr1rm8hybVmmWhx7wd --- FileDeduplicator/FileScanner.cs | 105 +++++++++----------------------- 1 file changed, 30 insertions(+), 75 deletions(-) diff --git a/FileDeduplicator/FileScanner.cs b/FileDeduplicator/FileScanner.cs index ec7b94d..9c10612 100644 --- a/FileDeduplicator/FileScanner.cs +++ b/FileDeduplicator/FileScanner.cs @@ -10,6 +10,34 @@ namespace ktsu.FileDeduplicator; internal static class FileScanner { + /// + /// How the scan walks the tree. + /// + /// + /// Replaces , which resolves to the legacy-compatible + /// options carrying IgnoreInaccessible = false. Enumeration is lazy, so the + /// from one unreadable subdirectory surfaced partway + /// through the walk and abandoned the whole scan -- a restricted share or an OS-protected folder + /// anywhere under the root was enough. + /// + /// is skipped so a directory symlink pointing at one of + /// its own ancestors cannot be descended into indefinitely. It skips linked files too, which + /// suits a deduplicator: a symlink is not a second copy, so deleting one reclaims nothing and + /// hashing through it would report content as duplicated with itself. + /// + /// + /// and are deliberately + /// not skipped, which a bare new EnumerationOptions() would do. Those files were scanned + /// before this change, and a duplicate among them is still a duplicate. + /// + /// + private static readonly EnumerationOptions WalkOptions = new() + { + RecurseSubdirectories = true, + IgnoreInaccessible = true, + AttributesToSkip = FileAttributes.ReparsePoint, + }; + internal static IReadOnlyList ScanForFiles(AbsoluteDirectoryPath path) { if (!path.Exists) @@ -18,85 +46,12 @@ internal static IReadOnlyList ScanForFiles(AbsoluteDirectoryPa return []; } - // Walked one directory at a time rather than with SearchOption.AllDirectories, which - // enumerates lazily: an UnauthorizedAccessException raised while descending into a single - // unreadable subdirectory propagates out of the loop and abandons the whole scan, however - // much of the tree was readable. Listing each directory on its own keeps a failure local to - // the directory that caused it, and lets the scan say which one it gave up on. List files = []; - Queue pending = new(); - pending.Enqueue(path.WeakString); - - while (pending.Count > 0) + foreach (string file in Directory.EnumerateFiles(path.WeakString, "*", WalkOptions)) { - string directory = pending.Dequeue(); - - foreach (string file in List(directory, Directory.EnumerateFiles)) - { - files.Add(file.As()); - } - - foreach (string subdirectory in List(directory, Directory.EnumerateDirectories)) - { - if (IsReparsePoint(subdirectory)) - { - // A directory symlink or junction pointing at one of its own ancestors -- build - // caches and some backup layouts produce these -- would otherwise be descended - // into until the path stopped being legal. Skipping reparse points also stops - // content reachable by two routes being scanned, and reported as duplicated, - // twice. - Console.WriteLine($" Skipped link: {subdirectory}"); - continue; - } - - pending.Enqueue(subdirectory); - } + files.Add(file.As()); } return files; } - - /// - /// Lists one directory's entries, reporting and skipping it if it cannot be read. - /// - /// - /// The result is materialized inside the guard on purpose. Both enumerators are lazy, so a - /// caller iterating one outside this method would see the exception raised on whichever element - /// triggered it, not here. - /// - private static List List(string directory, Func> enumerate) - { - try - { - return [.. enumerate(directory)]; - } - catch (UnauthorizedAccessException ex) - { - Console.WriteLine($" Skipped {directory}: {ex.Message}"); - return []; - } - catch (IOException ex) - { - Console.WriteLine($" Skipped {directory}: {ex.Message}"); - return []; - } - } - - private static bool IsReparsePoint(string directory) - { - try - { - return File.GetAttributes(directory).HasFlag(FileAttributes.ReparsePoint); - } - catch (UnauthorizedAccessException) - { - // Unreadable attributes are not grounds for following the link; the listing guard above - // reports the directory when the descent then fails to read it. - return true; - } - catch (IOException) - { - return true; - } - } }