From 0e0d0a794f7db0b0803401d82c4bbcf06fe54658 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kov=C3=A1cs=20B=C3=A1lint=20Hunor?= Date: Tue, 13 Oct 2026 00:42:32 +0300 Subject: [PATCH 1/2] Quarantine cross-device move sources before deletion Co-Authored-By: Codex Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016FAtbHHLQSiJiYcw2vnKiq --- .../LinuxFileOperationsHooks.cs | 6 + .../LinuxFileOperationsService.Move.cs | 70 ++++++++- .../Native/PosixFileOperationsNative.cs | 15 ++ .../Native/PosixNative.cs | 8 +- .../FileOperations/MoveQuarantineTests.cs | 145 ++++++++++++++++++ 5 files changed, 233 insertions(+), 11 deletions(-) create mode 100644 tests/Files.Platform.Tests/FileOperations/MoveQuarantineTests.cs diff --git a/src/Files.Platform.Linux/FileOperations/LinuxFileOperationsHooks.cs b/src/Files.Platform.Linux/FileOperations/LinuxFileOperationsHooks.cs index 7a79d1ab5c3d..2882b0551d5a 100644 --- a/src/Files.Platform.Linux/FileOperations/LinuxFileOperationsHooks.cs +++ b/src/Files.Platform.Linux/FileOperations/LinuxFileOperationsHooks.cs @@ -42,5 +42,11 @@ public sealed class LinuxFileOperationsHooks /// Invoked after an entry was classified and right before it is deleted or descended into. public Action? BeforeDeleteEntry { get; init; } + + /// Invoked after move source verification, before the final quarantine checks and rename. + public Action? AfterMoveSourceVerified { get; init; } + + /// Invoked after an atomic quarantine rename, before verification and deletion. + public Action? MoveSourceQuarantined { get; init; } } } diff --git a/src/Files.Platform.Linux/FileOperations/LinuxFileOperationsService.Move.cs b/src/Files.Platform.Linux/FileOperations/LinuxFileOperationsService.Move.cs index 1e272ae248b3..387fa0b96c87 100644 --- a/src/Files.Platform.Linux/FileOperations/LinuxFileOperationsService.Move.cs +++ b/src/Files.Platform.Linux/FileOperations/LinuxFileOperationsService.Move.cs @@ -69,6 +69,7 @@ private async Task MoveEntryAsync(string source, string destination, Di if (stat.IsSpecial) throw new FileOperationException(FileOperationErrorKind.UnsupportedFileType, "FIFOs, sockets and device files cannot be moved across file systems.", source); + using var moveSourceHandle = sourceKind == EntryKind.Symlink ? OpenMoveSource(sourceParent, name, source) : null; VerifyMoveSource(sourceParent, name, stat, source); var copy = sourceKind == EntryKind.Symlink ? CopyLink(source, destination, resolved.Replace, context, sourceParent, destinationParent) @@ -77,18 +78,13 @@ private async Task MoveEntryAsync(string source, string destination, Di { context.Hooks?.BeforeDeleteEntry?.Invoke(source); context.CancellationToken.ThrowIfCancellationRequested(); - if (!PosixNative.TryStat((int)input.SafeFileHandle.DangerousGetHandle(), out var current) - || !PosixNative.SameFileVersion(openedStat, current)) - throw new FileOperationException(FileOperationErrorKind.VerificationFailed, "The source changed while it was being moved.", source); - VerifyMoveSource(sourceParent, name, openedStat, source); - PosixNative.UnlinkAt(sourceParent.Descriptor, name, 0, source); + DeleteMoveSource(sourceParent, name, openedStat, source, (int)input.SafeFileHandle.DangerousGetHandle(), context); }, stat).ConfigureAwait(false); if (sourceKind == EntryKind.Symlink && copy.Status == FileOperationStatus.Succeeded) { context.Hooks?.BeforeDeleteEntry?.Invoke(source); context.CancellationToken.ThrowIfCancellationRequested(); - VerifyMoveSource(sourceParent, name, stat, source); - PosixNative.UnlinkAt(sourceParent.Descriptor, name, 0, source); + DeleteMoveSource(sourceParent, name, stat, source, (int)moveSourceHandle!.DangerousGetHandle(), context); context.PathChanged(source, destination, xattrStale: true); } return copy; @@ -132,12 +128,14 @@ private async Task MoveDirectoryContentsAsync(string source, string des if (result.Status == FileOperationStatus.Succeeded) { + if (!PosixNative.TryStat(sourceDirectory.Descriptor, out var removalStat)) + throw PosixNative.CreateException(5, source); context.Hooks?.BeforeDeleteEntry?.Invoke(source); context.CancellationToken.ThrowIfCancellationRequested(); VerifyMoveSource(sourceParent, Path.GetFileName(source), stat, source); try { - PosixNative.UnlinkAt(sourceParent.Descriptor, Path.GetFileName(source), PosixNative.AtRemoveDir, source); + DeleteMoveSource(sourceParent, Path.GetFileName(source), removalStat, source, sourceDirectory.Descriptor, context); context.PathChanged(source, created ? destination : null, xattrStale: created); } catch (IOException ex) when (ex.HResult == 39 && sourceDirectory.ListNames().Count > 0) @@ -150,6 +148,62 @@ private async Task MoveDirectoryContentsAsync(string source, string des return result; } + private static Microsoft.Win32.SafeHandles.SafeFileHandle OpenMoveSource(DirectoryHandle parent, string name, string source) + { + var fd = PosixNative.OpenAt(parent.Descriptor, name, PosixNative.ReadOnlyFlags | PosixNative.ONofollow | 0x200000, out var errno); // O_PATH + if (fd < 0) + throw PosixNative.CreateException(errno, source); + return new Microsoft.Win32.SafeHandles.SafeFileHandle(fd, true); + } + + private static void DeleteMoveSource(DirectoryHandle parent, string name, PosixStat expected, string source, int fd, FileOperationContext context) + { + void Changed() => throw new FileOperationException(FileOperationErrorKind.VerificationFailed, "The source changed while it was being moved.", source); + bool Matches(PosixStat current) => PosixNative.SameFileVersion(expected, current) && expected.LinkCount == current.LinkCount; + VerifyMoveSource(parent, name, expected, source); + context.Hooks?.AfterMoveSourceVerified?.Invoke(source); + if (!PosixNative.TryStat(fd, out var before) || !Matches(before) || !Matches(PosixNative.StatAt(parent.Descriptor, name, source))) + Changed(); + var quarantine = ".linuxfiles-move-" + Guid.NewGuid().ToString("N"); + while (!PosixNative.TryQuarantineRename(parent.Descriptor, name, quarantine, out var errno, context.Hooks)) + { + if (errno == 17) + { + quarantine = ".linuxfiles-move-" + Guid.NewGuid().ToString("N"); + continue; + } + if (errno is 22 or 38 or 95) // RENAME_NOREPLACE unsupported; preserve legacy verification. + { + VerifyMoveSource(parent, name, expected, source); + if (!PosixNative.TryStat(fd, out var fallback) || !Matches(fallback)) + Changed(); + PosixNative.UnlinkAt(parent.Descriptor, name, expected.IsDirectory ? PosixNative.AtRemoveDir : 0, source); + return; + } + throw PosixNative.CreateException(errno, source); + } + try + { + context.Hooks?.MoveSourceQuarantined?.Invoke(source, Path.Combine(Path.GetDirectoryName(source)!, quarantine)); + var entry = PosixNative.StatAt(parent.Descriptor, quarantine, source); + // Linux rename itself updates ctime: check the recorded ctime before rename and require + // the held fd and quarantined entry to agree afterward. A writer with an already open fd + // (or another hardlink) can still write after the final stat; POSIX cannot close that window, + // nor distinguish a ctime-only change between the pre-rename stat and rename's own update. + var renamedExpected = expected with { ChangedSeconds = entry.ChangedSeconds, ChangedNanoseconds = entry.ChangedNanoseconds }; + if (!PosixNative.SameFileVersion(renamedExpected, entry) || expected.LinkCount != entry.LinkCount + || !PosixNative.TryStat(fd, out var held) || !PosixNative.SameFileVersion(entry, held) || entry.LinkCount != held.LinkCount) + Changed(); + PosixNative.UnlinkAt(parent.Descriptor, quarantine, expected.IsDirectory ? PosixNative.AtRemoveDir : 0, source); + } + catch + { + // Never overwrite a newly created source name. If restoration fails, retain quarantine. + PosixNative.TryQuarantineRename(parent.Descriptor, quarantine, name, out _, context.Hooks); + throw; + } + } + private static void VerifyMoveSource(DirectoryHandle parent, string name, PosixStat expected, string source) { var current = PosixNative.StatAt(parent.Descriptor, name, source); diff --git a/src/Files.Platform.Linux/Native/PosixFileOperationsNative.cs b/src/Files.Platform.Linux/Native/PosixFileOperationsNative.cs index b190d10b8538..f1036122c51a 100644 --- a/src/Files.Platform.Linux/Native/PosixFileOperationsNative.cs +++ b/src/Files.Platform.Linux/Native/PosixFileOperationsNative.cs @@ -33,6 +33,21 @@ public static PosixStat StatAt(int dirfd, string name, string displayPath) return stat; } + // Strict atomic rename: quarantine must never use the compatibility check/rename or link/unlink paths. + public static bool TryQuarantineRename(int dirfd, string source, string destination, out int errno, LinuxFileOperationsHooks? hooks) + { + try + { + errno = hooks?.RenameError?.Invoke(source, destination, false) + ?? (renameat2(dirfd, source, dirfd, destination, 1) == 0 ? 0 : Marshal.GetLastPInvokeError()); + } + catch (EntryPointNotFoundException) + { + errno = 38; + } + return errno == 0; + } + public static bool TryRenameAt(int sourceFd, string source, int destinationFd, string destination, bool replace, out int errno, LinuxFileOperationsHooks? hooks = null) { var cache = hooks?.UnsupportedRenameMounts ?? s_noRename2; diff --git a/src/Files.Platform.Linux/Native/PosixNative.cs b/src/Files.Platform.Linux/Native/PosixNative.cs index 37819abb5425..216a8f94345b 100644 --- a/src/Files.Platform.Linux/Native/PosixNative.cs +++ b/src/Files.Platform.Linux/Native/PosixNative.cs @@ -15,7 +15,7 @@ namespace Files.Platform.Linux.Native /// Mount-related statx fields: attribute bits with their support mask, the mount id (when reported) and the device. public readonly record struct MountInfo(ulong Attributes, ulong AttributesMask, bool MountIdValid, ulong MountId, uint DevMajor, uint DevMinor); - internal readonly record struct PosixStat(uint Mode, ulong Size, uint OwnerUserId, long ModifiedSeconds, uint ModifiedNanoseconds, ulong Inode = 0, uint DevMajor = 0, uint DevMinor = 0, ulong? MountId = null, long ChangedSeconds = 0, uint ChangedNanoseconds = 0) + internal readonly record struct PosixStat(uint Mode, ulong Size, uint OwnerUserId, long ModifiedSeconds, uint ModifiedNanoseconds, ulong Inode = 0, uint DevMajor = 0, uint DevMinor = 0, ulong? MountId = null, long ChangedSeconds = 0, uint ChangedNanoseconds = 0, uint LinkCount = 0) { public uint FileType => Mode & 0xF000; @@ -45,6 +45,7 @@ internal static unsafe partial class PosixNative private const uint StatxType = 0x1; private const uint StatxMode = 0x2; + private const uint StatxNlink = 0x4; private const uint StatxUid = 0x8; private const uint StatxMtime = 0x40; private const uint StatxCtime = 0x80; @@ -89,7 +90,7 @@ public static bool TryStat(int dirfd, string path, int flags, out PosixStat stat try { - const uint mask = StatxType | StatxMode | StatxUid | StatxMtime | StatxCtime | StatxSize | StatxIno | StatxMountId; + const uint mask = StatxType | StatxMode | StatxUid | StatxMtime | StatxCtime | StatxSize | StatxIno | StatxMountId | StatxNlink; if (!TryStatx(dirfd, path, flags, mask, out var buffer, out errno)) return false; @@ -106,7 +107,8 @@ public static bool TryStat(int dirfd, string path, int flags, out PosixStat stat (returned & StatxIno) != 0 ? BitConverter.ToUInt64(buffer, 32) : 0, BitConverter.ToUInt32(buffer, 136), BitConverter.ToUInt32(buffer, 140), (returned & StatxMountId) != 0 ? BitConverter.ToUInt64(buffer, 144) : null, (returned & StatxCtime) != 0 ? BitConverter.ToInt64(buffer, 96) : 0, - (returned & StatxCtime) != 0 ? BitConverter.ToUInt32(buffer, 104) : 0); + (returned & StatxCtime) != 0 ? BitConverter.ToUInt32(buffer, 104) : 0, + (returned & StatxNlink) != 0 ? BitConverter.ToUInt32(buffer, 16) : 0); return true; } catch (Exception ex) when (ex is DllNotFoundException or EntryPointNotFoundException) diff --git a/tests/Files.Platform.Tests/FileOperations/MoveQuarantineTests.cs b/tests/Files.Platform.Tests/FileOperations/MoveQuarantineTests.cs new file mode 100644 index 000000000000..c6f0afa4f2dd --- /dev/null +++ b/tests/Files.Platform.Tests/FileOperations/MoveQuarantineTests.cs @@ -0,0 +1,145 @@ +// Copyright (c) Files Community +// Licensed under the MIT License. + +using System.IO; +using System.Runtime.Versioning; +using System.Threading.Tasks; +using Files.Platform.Abstractions.FileOperations; +using Files.Platform.Linux.FileOperations; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +namespace Files.Platform.Tests.FileOperations +{ + [TestClass] + [SupportedOSPlatform("linux")] + public sealed class MoveQuarantineTests : FileOperationsTestBase + { + [TestMethod] + [DataRow(false)] + [DataRow(true)] + public async Task ChangedAfterVerification_PreservesSource(bool directory) + { + var source = directory ? Path.Combine(Src, "tree") : Write(Path.Combine(Src, "a"), "OLD"); + if (directory) + Write(Path.Combine(source, "child"), "OLD"); + var invoked = false; + var service = new LinuxFileOperationsService((_, _) => false, new LinuxFileOperationsHooks + { + AfterMoveSourceVerified = path => + { + if (path != source) + return; + invoked = true; + Write(directory ? Path.Combine(source, "new") : source, "CHANGED"); + }, + }); + var result = await service.MoveAsync([source], Dst); + Assert.IsTrue(invoked); + Assert.AreEqual(FileOperationErrorKind.VerificationFailed, result[0].ErrorKind); + Assert.AreEqual("CHANGED", File.ReadAllText(directory ? Path.Combine(source, "new") : source)); + } + + [TestMethod] + public async Task ReplacedBySymlinkDuringRename_RestoresReplacement() + { + var source = Write(Path.Combine(Src, "a"), "OLD"); + var precious = Write(Path.Combine(Root, "precious"), "KEEP"); + var swapped = false; + var service = new LinuxFileOperationsService((_, _) => false, new LinuxFileOperationsHooks + { + RenameError = (from, to, _) => + { + if (from == "a" && to.StartsWith(".linuxfiles-move-") && !swapped) + { + swapped = true; + File.Move(source, source + ".saved"); + File.CreateSymbolicLink(source, precious); + } + return null; + }, + }); + var result = await service.MoveAsync([source], Dst); + Assert.IsTrue(swapped); + Assert.AreEqual(FileOperationErrorKind.VerificationFailed, result[0].ErrorKind); + Assert.AreEqual(precious, new FileInfo(source).LinkTarget); + Assert.AreEqual("KEEP", File.ReadAllText(precious)); + Assert.AreEqual("OLD", File.ReadAllText(source + ".saved")); + Assert.IsEmpty(Directory.GetFileSystemEntries(Src, ".linuxfiles-move-*")); + } + + [TestMethod] + [DataRow(false, false)] + [DataRow(true, false)] + [DataRow(false, true)] + public async Task QuarantineMismatch_RestoresOrRetainsWithoutOverwriting(bool directory, bool occupied) + { + var source = directory ? Path.Combine(Src, "tree") : Write(Path.Combine(Src, "a"), "OLD"); + if (directory) + Write(Path.Combine(source, "child"), "OLD"); + string? quarantined = null; + var service = new LinuxFileOperationsService((_, _) => false, new LinuxFileOperationsHooks + { + MoveSourceQuarantined = (path, hidden) => + { + if (path != source) + return; + quarantined = hidden; + Write(directory ? Path.Combine(hidden, "new") : hidden, "CHANGED"); + if (occupied) + Write(source, "KEEP"); + }, + }); + var result = await service.MoveAsync([source], Dst); + Assert.IsNotNull(quarantined); + Assert.AreEqual(FileOperationErrorKind.VerificationFailed, result[0].ErrorKind); + var retained = occupied ? quarantined : source; + Assert.AreEqual("CHANGED", File.ReadAllText(directory ? Path.Combine(retained, "new") : retained)); + if (occupied) + Assert.AreEqual("KEEP", File.ReadAllText(source)); + else + Assert.IsEmpty(Directory.GetFileSystemEntries(Src, ".linuxfiles-move-*")); + } + + [TestMethod] + public async Task EditedBetweenFinalStatAndRename_RestoresChangedSource() + { + var source = Write(Path.Combine(Src, "a"), "OLD"); + var edited = false; + var service = new LinuxFileOperationsService((_, _) => false, new LinuxFileOperationsHooks + { + RenameError = (from, to, _) => + { + if (from == "a" && to.StartsWith(".linuxfiles-move-")) + { + edited = true; + Write(source, "CHANGED AFTER STAT"); + } + return null; + }, + }); + var result = await service.MoveAsync([source], Dst); + Assert.IsTrue(edited); + Assert.AreEqual(FileOperationErrorKind.VerificationFailed, result[0].ErrorKind); + Assert.AreEqual("CHANGED AFTER STAT", File.ReadAllText(source)); + Assert.AreEqual("OLD", File.ReadAllText(Path.Combine(Dst, "a"))); + Assert.IsEmpty(Directory.GetFileSystemEntries(Src, ".linuxfiles-move-*")); + } + + [TestMethod] + [DataRow(22)] + [DataRow(38)] + [DataRow(95)] + public async Task UnsupportedRename_UsesVerifiedFallback(int errno) + { + var source = Write(Path.Combine(Src, "a"), "OLD"); + var service = new LinuxFileOperationsService((_, _) => false, new LinuxFileOperationsHooks + { + RenameError = (_, to, _) => to.StartsWith(".linuxfiles-move-") ? errno : null, + }); + var result = await service.MoveAsync([source], Dst); + Assert.IsTrue(result[0].Succeeded, result[0].ErrorMessage); + Assert.IsFalse(File.Exists(source)); + Assert.AreEqual("OLD", File.ReadAllText(Path.Combine(Dst, "a"))); + } + } +} From 2054e382565a5bfcb8ca6bf7237c70dcbcf53304 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kov=C3=A1cs=20B=C3=A1lint=20Hunor?= Date: Tue, 13 Oct 2026 00:55:18 +0300 Subject: [PATCH 2/2] Harden quarantine checks before deleting moved sources Co-Authored-By: Codex Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016FAtbHHLQSiJiYcw2vnKiq --- .../investigations/security-review-2026-10.md | 4 + .../LinuxFileOperationsHooks.cs | 3 + .../LinuxFileOperationsService.Move.cs | 34 +++++-- .../Native/PosixFileOperationsNative.cs | 6 +- .../FileOperations/MoveQuarantineTests.cs | 88 +++++++++++++++++++ 5 files changed, 124 insertions(+), 11 deletions(-) diff --git a/docs/linux-port/investigations/security-review-2026-10.md b/docs/linux-port/investigations/security-review-2026-10.md index 02e5ff16468b..907e543c37ae 100644 --- a/docs/linux-port/investigations/security-review-2026-10.md +++ b/docs/linux-port/investigations/security-review-2026-10.md @@ -46,6 +46,10 @@ Copy verification checks only output length and the source's current size. `Veri **Suggested fix:** Carry the opened source's identity, size, nanosecond mtime and ctime through publication and recheck the pinned inode and source name immediately before unlink. Verify copied content against the source and preserve the original on detected changes. Use the helper's content/metadata checks as a reference while acknowledging the unavoidable final check/unlink race; the present gap accepts changes made before the final check. +**Remediation and threat model:** Cross-device deletion protects against accidental concurrent modification, such as an application saving during a move: data differing from the copied version must be retained. Before each quarantine rename, fstat of the held descriptor must match the copied identity, size, mtime and ctime; writes bump ctime, which a writer cannot restore by setting mtime. Only after that check passes may post-rename verification accept rename's ctime update, while still requiring size, mtime, device/inode and link count to match. Immediately before unlink, recheck the held version and lstat the quarantine name against its device/inode. Unsupported atomic rename uses the same final held-fd/version and original-name identity checks. Verification failure retains or restores the source without overwriting a newly created original name. Regular-file tag re-keying runs only after successful deletion. + +A hostile process running as the same user and racing the quarantine name is outside this threat model. POSIX offers no unlink-by-inode; a writer using an open descriptor can still write in the final stat/rename or stat/unlink window. These checks reject observed changes, but cannot guarantee exclusion of writes during those unavoidable syscall gaps. + ### R5 — P2: Direct thumbnail cache hits bypass image validation **Location:** `src/Files.Platform.Linux/Thumbnails/LinuxThumbnailService.cs:179`, `:239`, `:249`, `:259`; contrast external output validation at `:481`. diff --git a/src/Files.Platform.Linux/FileOperations/LinuxFileOperationsHooks.cs b/src/Files.Platform.Linux/FileOperations/LinuxFileOperationsHooks.cs index 2882b0551d5a..37859607cebb 100644 --- a/src/Files.Platform.Linux/FileOperations/LinuxFileOperationsHooks.cs +++ b/src/Files.Platform.Linux/FileOperations/LinuxFileOperationsHooks.cs @@ -48,5 +48,8 @@ public sealed class LinuxFileOperationsHooks /// Invoked after an atomic quarantine rename, before verification and deletion. public Action? MoveSourceQuarantined { get; init; } + + /// Invoked before the final held-fd and entry checks immediately preceding move source unlink. + public Action? BeforeMoveSourceUnlink { get; init; } } } diff --git a/src/Files.Platform.Linux/FileOperations/LinuxFileOperationsService.Move.cs b/src/Files.Platform.Linux/FileOperations/LinuxFileOperationsService.Move.cs index 387fa0b96c87..8546a90586df 100644 --- a/src/Files.Platform.Linux/FileOperations/LinuxFileOperationsService.Move.cs +++ b/src/Files.Platform.Linux/FileOperations/LinuxFileOperationsService.Move.cs @@ -79,6 +79,7 @@ private async Task MoveEntryAsync(string source, string destination, Di context.Hooks?.BeforeDeleteEntry?.Invoke(source); context.CancellationToken.ThrowIfCancellationRequested(); DeleteMoveSource(sourceParent, name, openedStat, source, (int)input.SafeFileHandle.DangerousGetHandle(), context); + context.PathChanged(source, destination, xattrStale: true); }, stat).ConfigureAwait(false); if (sourceKind == EntryKind.Symlink && copy.Status == FileOperationStatus.Succeeded) { @@ -164,18 +165,30 @@ private static void DeleteMoveSource(DirectoryHandle parent, string name, PosixS context.Hooks?.AfterMoveSourceVerified?.Invoke(source); if (!PosixNative.TryStat(fd, out var before) || !Matches(before) || !Matches(PosixNative.StatAt(parent.Descriptor, name, source))) Changed(); + void VerifyBeforeRename() + { + if (!PosixNative.TryStat(fd, out var current) || !Matches(current) + || !Matches(PosixNative.StatAt(parent.Descriptor, name, source))) + Changed(); + } + // Protect accidental concurrent saves: never knowingly delete a version absent from the copy. + // Writes change ctime even when mtime is restored. Only a verified pre-rename version may + // accept rename's ctime update. Hostile same-user quarantine-name races are out of scope: + // POSIX has no unlink-by-inode, so final stat/rename and stat/unlink windows remain. var quarantine = ".linuxfiles-move-" + Guid.NewGuid().ToString("N"); - while (!PosixNative.TryQuarantineRename(parent.Descriptor, name, quarantine, out var errno, context.Hooks)) + while (!PosixNative.TryQuarantineRename(parent.Descriptor, name, quarantine, out var errno, context.Hooks, VerifyBeforeRename)) { if (errno == 17) { quarantine = ".linuxfiles-move-" + Guid.NewGuid().ToString("N"); continue; } - if (errno is 22 or 38 or 95) // RENAME_NOREPLACE unsupported; preserve legacy verification. + if (errno is 22 or 38 or 95) // RENAME_NOREPLACE unsupported; recheck the held version and name. { - VerifyMoveSource(parent, name, expected, source); - if (!PosixNative.TryStat(fd, out var fallback) || !Matches(fallback)) + context.Hooks?.BeforeMoveSourceUnlink?.Invoke(source, source); + var fallbackEntry = PosixNative.StatAt(parent.Descriptor, name, source); + if (!PosixNative.TryStat(fd, out var fallback) || !Matches(fallback) + || !Matches(fallbackEntry) || !PosixNative.SameEntry(fallback, fallbackEntry)) Changed(); PosixNative.UnlinkAt(parent.Descriptor, name, expected.IsDirectory ? PosixNative.AtRemoveDir : 0, source); return; @@ -186,13 +199,16 @@ private static void DeleteMoveSource(DirectoryHandle parent, string name, PosixS { context.Hooks?.MoveSourceQuarantined?.Invoke(source, Path.Combine(Path.GetDirectoryName(source)!, quarantine)); var entry = PosixNative.StatAt(parent.Descriptor, quarantine, source); - // Linux rename itself updates ctime: check the recorded ctime before rename and require - // the held fd and quarantined entry to agree afterward. A writer with an already open fd - // (or another hardlink) can still write after the final stat; POSIX cannot close that window, - // nor distinguish a ctime-only change between the pre-rename stat and rename's own update. + // The pre-rename fd check passed; only rename's ctime update is permitted. var renamedExpected = expected with { ChangedSeconds = entry.ChangedSeconds, ChangedNanoseconds = entry.ChangedNanoseconds }; + var held = default(PosixStat); if (!PosixNative.SameFileVersion(renamedExpected, entry) || expected.LinkCount != entry.LinkCount - || !PosixNative.TryStat(fd, out var held) || !PosixNative.SameFileVersion(entry, held) || entry.LinkCount != held.LinkCount) + || !PosixNative.TryStat(fd, out held) || !PosixNative.SameFileVersion(entry, held) || entry.LinkCount != held.LinkCount) + Changed(); + context.Hooks?.BeforeMoveSourceUnlink?.Invoke(source, Path.Combine(Path.GetDirectoryName(source)!, quarantine)); + if (!PosixNative.TryStat(fd, out var finalHeld) || !PosixNative.SameFileVersion(held, finalHeld) + || held.LinkCount != finalHeld.LinkCount + || !PosixNative.SameFileVersion(finalHeld, PosixNative.StatAt(parent.Descriptor, quarantine, source))) Changed(); PosixNative.UnlinkAt(parent.Descriptor, quarantine, expected.IsDirectory ? PosixNative.AtRemoveDir : 0, source); } diff --git a/src/Files.Platform.Linux/Native/PosixFileOperationsNative.cs b/src/Files.Platform.Linux/Native/PosixFileOperationsNative.cs index f1036122c51a..c6e7d5bf06c7 100644 --- a/src/Files.Platform.Linux/Native/PosixFileOperationsNative.cs +++ b/src/Files.Platform.Linux/Native/PosixFileOperationsNative.cs @@ -34,11 +34,13 @@ public static PosixStat StatAt(int dirfd, string name, string displayPath) } // Strict atomic rename: quarantine must never use the compatibility check/rename or link/unlink paths. - public static bool TryQuarantineRename(int dirfd, string source, string destination, out int errno, LinuxFileOperationsHooks? hooks) + public static bool TryQuarantineRename(int dirfd, string source, string destination, out int errno, LinuxFileOperationsHooks? hooks, Action? beforeRename = null) { try { - errno = hooks?.RenameError?.Invoke(source, destination, false) + var injected = hooks?.RenameError?.Invoke(source, destination, false); + beforeRename?.Invoke(); + errno = injected ?? (renameat2(dirfd, source, dirfd, destination, 1) == 0 ? 0 : Marshal.GetLastPInvokeError()); } catch (EntryPointNotFoundException) diff --git a/tests/Files.Platform.Tests/FileOperations/MoveQuarantineTests.cs b/tests/Files.Platform.Tests/FileOperations/MoveQuarantineTests.cs index c6f0afa4f2dd..ce50a4dd73e0 100644 --- a/tests/Files.Platform.Tests/FileOperations/MoveQuarantineTests.cs +++ b/tests/Files.Platform.Tests/FileOperations/MoveQuarantineTests.cs @@ -14,6 +14,33 @@ namespace Files.Platform.Tests.FileOperations [SupportedOSPlatform("linux")] public sealed class MoveQuarantineTests : FileOperationsTestBase { + [TestMethod] + [DataRow(false)] + [DataRow(true)] + public async Task SuccessfulFileDeletion_ReportsPathChangeAfterUnlink(bool child) + { + var source = Write(Path.Combine(Src, child ? "tree/a" : "a"), "OLD"); + var destination = Path.Combine(Dst, child ? "tree/a" : "a"); + var reported = false; + var result = await new LinuxFileOperationsService((_, _) => false).MoveAsync( + [child ? Path.GetDirectoryName(source)! : source], Dst, new FileOperationOptions + { + PathChanged = (from, to, recursive, stale) => + { + if (from != source) + return; + reported = true; + Assert.AreEqual(destination, to); + Assert.IsFalse(recursive); + Assert.IsTrue(stale); + Assert.IsFalse(File.Exists(source)); + Assert.IsEmpty(Directory.GetFileSystemEntries(Path.GetDirectoryName(source)!, ".linuxfiles-move-*")); + }, + }); + Assert.IsTrue(result[0].Succeeded, result[0].ErrorMessage); + Assert.IsTrue(reported); + } + [TestMethod] [DataRow(false)] [DataRow(true)] @@ -125,6 +152,67 @@ public async Task EditedBetweenFinalStatAndRename_RestoresChangedSource() Assert.IsEmpty(Directory.GetFileSystemEntries(Src, ".linuxfiles-move-*")); } + [TestMethod] + public async Task EqualLengthEditWithRestoredMtimeBeforeRename_PreservesSource() + { + var source = Write(Path.Combine(Src, "a"), "OLD"); + var mtime = File.GetLastWriteTimeUtc(source); + var edited = false; + var service = new LinuxFileOperationsService((_, _) => false, new LinuxFileOperationsHooks + { + RenameError = (from, to, _) => + { + if (from == "a" && to.StartsWith(".linuxfiles-move-")) + { + edited = true; + Write(source, "NEW"); + File.SetLastWriteTimeUtc(source, mtime); + } + return null; + }, + }); + var result = await service.MoveAsync([source], Dst); + Assert.IsTrue(edited); + Assert.AreEqual(FileOperationErrorKind.VerificationFailed, result[0].ErrorKind); + Assert.AreEqual("NEW", File.ReadAllText(source)); + Assert.AreEqual("OLD", File.ReadAllText(Path.Combine(Dst, "a"))); + Assert.IsEmpty(Directory.GetFileSystemEntries(Src, ".linuxfiles-move-*")); + } + + [TestMethod] + [DataRow(0, false)] + [DataRow(0, true)] + [DataRow(22, false)] + [DataRow(38, false)] + [DataRow(95, false)] + [DataRow(22, true)] + public async Task ChangedImmediatelyBeforeUnlink_PreservesNewData(int errno, bool replacement) + { + var source = Write(Path.Combine(Src, "a"), "OLD"); + var invoked = false; + var service = new LinuxFileOperationsService((_, _) => false, new LinuxFileOperationsHooks + { + RenameError = (_, to, _) => errno != 0 && to.StartsWith(".linuxfiles-move-") ? errno : null, + BeforeMoveSourceUnlink = (_, entry) => + { + invoked = true; + var mtime = File.GetLastWriteTimeUtc(entry); + if (replacement) + File.Move(entry, source + ".saved"); + Write(entry, "NEW"); + File.SetLastWriteTimeUtc(entry, mtime); + }, + }); + var result = await service.MoveAsync([source], Dst); + Assert.IsTrue(invoked); + Assert.AreEqual(FileOperationErrorKind.VerificationFailed, result[0].ErrorKind); + Assert.AreEqual("NEW", File.ReadAllText(source)); + Assert.AreEqual("OLD", File.ReadAllText(Path.Combine(Dst, "a"))); + if (replacement) + Assert.AreEqual("OLD", File.ReadAllText(source + ".saved")); + Assert.IsEmpty(Directory.GetFileSystemEntries(Src, ".linuxfiles-move-*")); + } + [TestMethod] [DataRow(22)] [DataRow(38)]