diff --git a/docs/adr/0006-settings-file-protocol.md b/docs/adr/0006-settings-file-protocol.md index 085d3af..d996d2f 100644 --- a/docs/adr/0006-settings-file-protocol.md +++ b/docs/adr/0006-settings-file-protocol.md @@ -52,7 +52,7 @@ written. An empty file reads as defaults at once. | Empty file | Defaults | Replaced after the wait; nothing kept aside | | Valid settings | The stored values | Replaced | | Some invalid properties | Valid properties kept, invalid ones as defaults | Replaced; a copy of the original is kept aside | -| Not settings at all | Defaults | Replaced; a copy of the original is kept aside | +| Not settings at all | Defaults | Replaced; a copy of the original is kept aside, unless it is over 16 MiB | | Unreadable or being written | Defaults | Refused; nothing is written | A trailing comma, a comment and a quoted number are valid: they are what a hand edit @@ -69,27 +69,41 @@ old file and allow deletion go on reading it. `File.Replace` is not used because is two renames with the file missing between them, and `File.Move` with overwrite is not used because it refuses while any handle is open on the destination. +The rename is given the path in the form Windows takes as written, so a data folder +whose path is longer than 260 characters, or whose name is not well-formed UTF-16, +behaves like any other. + On a volume that does not implement that rename, the store closes its own read handle and falls back to `File.Move` with overwrite. If the file did not exist when it was read, the temporary file is renamed into place without overwriting. A failed update leaves the stored file as it was and removes its temporary file. -**Kept-aside copies.** Before invalid content is replaced it is copied to -`settings.json.corrupt`, or to the first free of `settings.json.corrupt-2` to -`settings.json.corrupt-9`; the last name is overwritten once all exist. The copy is a -courtesy for recovering a hand edit: if it cannot be made the fault is reported and -the save proceeds, because refusing would block every later change too. A save that -then fails removes the copy it made. Nothing reads the copies back. They contain -whatever the file contained, including recent-document paths, so clearing recent -files deletes them. +**Kept-aside copies.** Before invalid content is replaced it is copied to a temporary +file. Once the save has succeeded the copy is renamed to `settings.json.corrupt`, or +to the first free of `settings.json.corrupt-2` to `settings.json.corrupt-9`; the last +name is overwritten once all exist. A save that fails removes its temporary copy and +leaves every earlier copy as it was, and the invalid file is still in place. The copy +is a courtesy for recovering a hand edit, with these limits: + +- a file over 16 MiB is replaced without a copy, since it was never this + application's settings and copying it would stall the window; +- a copy that cannot be given its name is removed and reported, and the save stands, + because refusing would block every later change too; +- a process killed between the save and the rename of the copy loses that copy. + +Nothing reads the copies back. They contain whatever the file contained, including +recent-document paths, so clearing recent files deletes them. **Abandoned temporary files.** A process killed between its write and its rename -leaves a temporary file. The next update, holding the lock, removes files that match -this store's own name pattern and no others. +leaves a temporary file, and possibly a temporary copy. The next update, holding the +lock, removes files that match this store's own name pattern and no others. **Waiting.** Only a sharing violation is waited out: at most 20 attempts, 50 ms apart, for the lock, the read and the rename. Access denied is treated as permanent -and reported at once, so a read-only profile does not stall a window. +and reported at once, so a read-only profile does not stall a window. The fallback +move is the exception: it answers access denied while any handle is open on the +file, a reader's included, so there access denied is waited out as well, and a +read-only file costs the full wait before the save is refused. **Reporting.** Faults are reported by kind with the exception type and HRESULT. No path and no operating-system message text reaches the log. diff --git a/docs/user-guide.md b/docs/user-guide.md index 9db21e5..7e47870 100644 --- a/docs/user-guide.md +++ b/docs/user-guide.md @@ -74,7 +74,7 @@ Documents are rendered locally. Authored scripts and remote images are blocked; - **The viewer is not a default option:** use Setup rather than a portable copy, then reopen Windows Default Apps. - **The app does not start:** confirm Microsoft Edge WebView2 Runtime is installed and inspect the activity logs if available. - **A setting could not be saved:** another program is using `settings.json`, or the file or folder is read-only. Nothing is overwritten while the file cannot be read; try the change again. -- **Settings were edited by hand:** a trailing comma, a comment or a quoted number is accepted; the app rewrites the file in its own format on the next save, so comments are not kept. A setting whose value is not valid is ignored and the other settings are kept. When that happens, the next save first copies the file as it was to `settings.json.corrupt` (then `.corrupt-2` up to `.corrupt-9`). Those copies contain everything the file contained; **File → Recent files → Clear recent files** deletes them. +- **Settings were edited by hand:** a trailing comma, a comment or a quoted number is accepted; the app rewrites the file in its own format on the next save, so comments are not kept. A syntax error such as a missing comma or quote makes the whole file unreadable: every setting starts from its default, and the next save keeps a copy of your file as described below. A setting whose value is not valid is ignored and the other settings are kept. When either happens, the next save keeps a copy of the file as it was, named `settings.json.corrupt` (then `.corrupt-2` up to `.corrupt-9`). Those copies contain everything the file contained; **File → Recent files → Clear recent files** deletes them. For help, use [Discussions](https://github.com/KofTwentyTwo/MarkdownViewr.Windows/discussions). Report reproducible problems through [Issues](https://github.com/KofTwentyTwo/MarkdownViewr.Windows/issues/new/choose), including **Help → About** version, Windows version, and relevant sanitized logs. Report vulnerabilities privately using [SECURITY.md](../SECURITY.md). diff --git a/src/MarkdownViewr.Windows.Core/AppSettingsStore.cs b/src/MarkdownViewr.Windows.Core/AppSettingsStore.cs index 85d87ad..c09b20b 100644 --- a/src/MarkdownViewr.Windows.Core/AppSettingsStore.cs +++ b/src/MarkdownViewr.Windows.Core/AppSettingsStore.cs @@ -106,8 +106,12 @@ public sealed class AppSettingsStore AppSettings edited = change(current); edited.Sanitize(); RemoveAbandonedTemporaryFiles(); - string? aside = read is SettingsRead.Corrupt or SettingsRead.Repaired ? KeepAside(stored!) : null; - Replace(JsonSerializer.SerializeToUtf8Bytes(edited, AppSettingsContext.Default.AppSettings), stored, aside); + string? copy = read is SettingsRead.Corrupt or SettingsRead.Repaired ? CopyAside(stored!) : null; + Replace(JsonSerializer.SerializeToUtf8Bytes(edited, AppSettingsContext.Default.AppSettings), stored, copy); + if(copy is not null) + { + KeepAside(copy); + } return edited; } catch(Exception exception) when(exception is IOException or UnauthorizedAccessException) @@ -340,12 +344,35 @@ public sealed class AppSettingsStore /// - /// Copies an invalid file to the first free kept-aside name before it is replaced, or over the - /// last name once all are taken, and returns the copy's path. The copy is a courtesy: failing - /// to make it is reported and does not stop the save, since refusing would block every later - /// change as well; null is returned then. + /// Copies an invalid file to a temporary name before it is replaced and returns that name. A + /// file past the size bound is not this application's settings and is not worth a copy: that + /// is reported and null returned. A copy left behind by a save that never finished is removed + /// with the other abandoned temporary files. /// - private string? KeepAside(FileStream stored) + private string? CopyAside(FileStream stored) + { + if(stored.Length > MaximumBytes) + { + _report(SettingsFault.NotKeptAside, null); + return null; + } + string temporary = TemporaryName(); + using var copy = new FileStream(temporary, FileMode.CreateNew, FileAccess.Write, FileShare.None); + stored.Position = 0; + stored.CopyTo(copy); + copy.Flush(flushToDisk: true); + return temporary; + } + + + + /// + /// Gives the copy of a replaced invalid file its lasting name: the first free kept-aside name, + /// or the last one once all are taken. This happens after the save, so a save that fails + /// never disturbs an earlier copy. The copy is a courtesy: one that cannot be placed is + /// removed and reported, and the save stands. + /// + private void KeepAside(string copy) { int slot = 1; while(slot < MaximumKeptAside && (File.Exists(KeptAsideName(slot)) || Directory.Exists(KeptAsideName(slot)))) @@ -354,16 +381,12 @@ public sealed class AppSettingsStore } try { - using var copy = new FileStream(KeptAsideName(slot), FileMode.Create, FileAccess.Write, FileShare.None); - stored.Position = 0; - stored.CopyTo(copy); - copy.Flush(flushToDisk: true); - return KeptAsideName(slot); + File.Move(copy, KeptAsideName(slot), overwrite: true); } catch(Exception exception) when(exception is IOException or UnauthorizedAccessException) { + TryDelete(copy); _report(SettingsFault.NotKeptAside, exception); - return null; } } @@ -374,14 +397,19 @@ public sealed class AppSettingsStore + /// A name beside the settings file that only this store uses, so an abandoned one can be recognized and removed. + private string TemporaryName() => Path + "." + Guid.NewGuid().ToString("N") + ".tmp"; + + + /// /// Writes beside the file and flushes to disk, then renames over it in one step. A failure - /// leaves the stored file as it was, and neither the temporary file nor the copy just kept - /// aside behind. + /// leaves the stored file as it was, and neither the temporary file nor the copy of the + /// invalid original behind. /// - private void Replace(byte[] content, FileStream? stored, string? aside) + private void Replace(byte[] content, FileStream? stored, string? copy) { - string temporary = Path + "." + Guid.NewGuid().ToString("N") + ".tmp"; + string temporary = TemporaryName(); bool replaced = false; try { @@ -401,9 +429,10 @@ public sealed class AppSettingsStore Retry(() => renamed = _renameOver(temporary, Path)); if(!renamed) { - // This volume cannot rename over an open file, so the file is closed first. + // This volume cannot rename over an open file, so the file is closed first. This + // move is refused while any handle is open on the file, a reader's included. stored.Dispose(); - Retry(() => File.Move(temporary, Path, overwrite: true)); + Retry(() => File.Move(temporary, Path, overwrite: true), IsBlockedByAHandle); } } replaced = true; @@ -413,9 +442,9 @@ public sealed class AppSettingsStore if(!replaced) { TryDelete(temporary); - if(aside is not null) + if(copy is not null) { - TryDelete(aside); + TryDelete(copy); } } } @@ -439,9 +468,14 @@ public sealed class AppSettingsStore - /// Repeats an operation only while another handle is in the way, a bounded number of times; the last failure is thrown. - private void Retry(Action operation) + /// + /// Repeats an operation only while another handle is in the way, a bounded number of times; + /// the last failure is thrown. says which failures mean that: + /// a sharing violation unless the operation reports an open handle differently. + /// + private void Retry(Action operation, Func? inTheWay = null) { + inTheWay ??= IsTransient; for(int attempt = 1; ; attempt++) { try @@ -449,7 +483,7 @@ public sealed class AppSettingsStore operation(); return; } - catch(IOException exception) when(attempt < MaximumAttempts && IsTransient(exception)) + catch(Exception exception) when(attempt < MaximumAttempts && inTheWay(exception)) { _wait(RetryDelay); } @@ -459,7 +493,16 @@ public sealed class AppSettingsStore /// A sharing violation: another handle on the file that is expected to go away. Access denied is not transient. - private static bool IsTransient(IOException exception) => exception.HResult == SharingViolation; + private static bool IsTransient(Exception exception) => exception is IOException && exception.HResult == SharingViolation; + + + + /// + /// What a move over an existing file fails with while another handle is open on it. Unlike + /// an open or a rename, that move answers access denied, which therefore has to be waited out + /// there even though a read-only file gives the same answer. + /// + private static bool IsBlockedByAHandle(Exception exception) => exception is UnauthorizedAccessException || IsTransient(exception); diff --git a/src/MarkdownViewr.Windows.Core/FileSwap.cs b/src/MarkdownViewr.Windows.Core/FileSwap.cs index 6e33b8c..f2c9eae 100644 --- a/src/MarkdownViewr.Windows.Core/FileSwap.cs +++ b/src/MarkdownViewr.Windows.Core/FileSwap.cs @@ -5,7 +5,6 @@ using System.ComponentModel; using System.Runtime.InteropServices; -using System.Text; using Microsoft.Win32.SafeHandles; @@ -40,13 +39,14 @@ internal static partial class FileSwap /// public static bool TryRenameOver(string source, string destination) { - using SafeFileHandle handle = CreateFile(source, DeleteAccess, ShareAll, 0, OpenExisting, 0, 0); + using SafeFileHandle handle = CreateFile(Verbatim(source), DeleteAccess, ShareAll, 0, OpenExisting, 0, 0); if(handle.IsInvalid) { throw Failure(Marshal.GetLastPInvokeError()); } // FILE_RENAME_INFO: flags, root directory handle, name length in bytes, then the name itself. - byte[] name = Encoding.Unicode.GetBytes(destination); + // The name is copied as stored: a file name may hold text that is not well-formed UTF-16. + byte[] name = MemoryMarshal.AsBytes(Verbatim(destination).AsSpan()).ToArray(); int lengthOffset = 2 * IntPtr.Size; int nameOffset = lengthOffset + sizeof(int); byte[] rename = new byte[nameOffset + name.Length + sizeof(char)]; @@ -63,6 +63,22 @@ internal static partial class FileSwap + /// + /// The form of a full path that Windows takes as written, without the 260-character limit the + /// plain form has in a process that has not opted in to long paths. The managed file calls add + /// it themselves; a direct Windows call has to. + /// + internal static string Verbatim(string fullPath) + { + if(fullPath.StartsWith(@"\\?\", StringComparison.Ordinal) || fullPath.StartsWith(@"\\.\", StringComparison.Ordinal)) + { + return fullPath; + } + return fullPath.StartsWith(@"\\", StringComparison.Ordinal) ? string.Concat(@"\\?\UNC\", fullPath.AsSpan(2)) : @"\\?\" + fullPath; + } + + + /// The errors a volume answers with when it does not implement this kind of rename: invalid function, not supported, invalid parameter. internal static bool IsUnsupported(int error) => error is 1 or 50 or 87; diff --git a/tests/MarkdownViewr.Windows.Core.Tests/AppSettingsStoreTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/AppSettingsStoreTests.cs index ebcfcc5..05b1e48 100644 --- a/tests/MarkdownViewr.Windows.Core.Tests/AppSettingsStoreTests.cs +++ b/tests/MarkdownViewr.Windows.Core.Tests/AppSettingsStoreTests.cs @@ -850,6 +850,170 @@ public class AppSettingsStoreTests + /// A failed save over invalid content leaves every earlier kept-aside copy as it was, the last one included. + [Fact] + public void Update_InvalidContentAndFailedSaveWithEveryNameUsed_KeepsTheEarlierCopies() + { + using var temp = new TempDirectory(); + File.WriteAllText(temp.File("settings.json.corrupt"), "kept 1"); + for(int slot = 2; slot <= AppSettingsStore.MaximumKeptAside; slot++) + { + string number = slot.ToString(System.Globalization.CultureInfo.InvariantCulture); + File.WriteAllText(temp.File("settings.json.corrupt-" + number), "kept " + number); + } + File.WriteAllText(temp.File("settings.json"), "newest invalid"); + string[] before = FileNames(temp.Path); + var faults = new List(); + var store = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault), _ => { }); + using(new FileStream(temp.File("settings.json"), FileMode.Open, FileAccess.Read, FileShare.Read)) + { + Assert.Null(store.Update(settings => settings)); + } + Assert.Equal([SettingsFault.Corrupt, SettingsFault.WriteFailed], faults); + Assert.Equal("kept 9", File.ReadAllText(temp.File("settings.json.corrupt-9"))); + Assert.Equal("kept 1", File.ReadAllText(temp.File("settings.json.corrupt"))); + Assert.Equal("newest invalid", File.ReadAllText(temp.File("settings.json"))); + Assert.Equal([.. before, "settings.json.lock"], FileNames(temp.Path)); + } + + + + /// A file past the size bound is replaced without being copied: copying it would stall the window and fill the disk with something that was never settings. + [Fact] + public void Update_OversizedFile_IsReplacedWithoutACopy() + { + using var temp = new TempDirectory(); + using(var oversized = new FileStream(temp.File("settings.json"), FileMode.CreateNew)) + { + oversized.SetLength(AppSettingsStore.MaximumBytes + 1); + } + var faults = new List<(SettingsFault Fault, Exception? Exception)>(); + var store = new AppSettingsStore(temp.File("settings.json"), (fault, exception) => faults.Add((fault, exception))); + Assert.NotNull(store.Update(settings => + { + settings.Theme = "Dark"; + return settings; + })); + Assert.Equal([SettingsFault.Corrupt, SettingsFault.NotKeptAside], faults.Select(entry => entry.Fault)); + Assert.Null(faults[1].Exception); + Assert.Equal("Dark", store.Load().Theme); + Assert.Equal(["settings.json", "settings.json.lock"], FileNames(temp.Path)); + } + + + + /// + /// On a volume that cannot rename over an open file, the move that replaces it is refused with + /// access denied while a reader has the file open. That is waited out like any other handle, + /// each pause being the store's retry delay. + /// + [Fact] + public void Update_FallbackMoveBlockedByAReader_WaitsForIt() + { + using var temp = new TempDirectory(); + Seed(temp); + FileStream? reader = new(temp.File("settings.json"), FileMode.Open, FileAccess.Read, FileShare.Read | FileShare.Delete); + var delays = new List(); + var faults = new List(); + var store = new AppSettingsStore(temp.File("settings.json"), (fault, _) => faults.Add(fault), delay => + { + delays.Add(delay); + if(delays.Count == 2) + { + reader.Dispose(); + } + }, (_, _) => false); + Assert.NotNull(store.Update(settings => + { + settings.Theme = "Light"; + return settings; + })); + Assert.Equal([AppSettingsStore.RetryDelay, AppSettingsStore.RetryDelay], delays); + Assert.Empty(faults); + Assert.Equal("Light", store.Load().Theme); + Assert.Equal(["settings.json", "settings.json.lock"], FileNames(temp.Path)); + } + + + + /// The same move on a read-only file is refused for good: the store gives up after its bounded wait and leaves the file and the folder as they were. + [Fact] + public void Update_FallbackMoveOnAReadOnlyFile_GivesUpAfterTheWait() + { + using var temp = new TempDirectory(); + Seed(temp); + byte[] before = File.ReadAllBytes(temp.File("settings.json")); + File.SetAttributes(temp.File("settings.json"), FileAttributes.ReadOnly); + try + { + int waits = 0; + var faults = new List<(SettingsFault Fault, Exception? Exception)>(); + var store = new AppSettingsStore(temp.File("settings.json"), (fault, exception) => faults.Add((fault, exception)), _ => waits++, (_, _) => false); + Assert.Null(store.Update(settings => settings)); + Assert.Equal(AppSettingsStore.MaximumAttempts - 1, waits); + Assert.Equal(SettingsFault.WriteFailed, Assert.Single(faults).Fault); + Assert.IsType(faults[0].Exception); + Assert.Equal(before, File.ReadAllBytes(temp.File("settings.json"))); + Assert.Equal(["settings.json", "settings.json.lock"], FileNames(temp.Path)); + } + finally + { + File.SetAttributes(temp.File("settings.json"), FileAttributes.Normal); + } + } + + + + /// + /// The data folder can be anywhere the person points it. A path past the classic 260-character + /// limit, and a folder whose name is not well-formed UTF-16 (which the file system allows), + /// are saved to again and again like any other. + /// + [Theory] + [InlineData(true)] + [InlineData(false)] + public void Update_UnusualFolderPath_SavesRepeatedly(bool longPath) + { + using var temp = new TempDirectory(); + string folder = longPath + ? Path.Combine(temp.Path, new string('a', 120), new string('b', 120), new string('c', 60)) + : Path.Combine(temp.Path, "profile \uD800 name"); + var faults = new List(); + var store = new AppSettingsStore(Path.Combine(folder, "settings.json"), (fault, _) => faults.Add(fault)); + Assert.Equal(longPath, store.Path.Length > 260); + foreach(string theme in new[] { "Dark", "Light", "Dark" }) + { + Assert.NotNull(store.Update(settings => + { + settings.Theme = theme; + return settings; + })); + Assert.Equal(theme, store.Load().Theme); + } + Assert.Empty(faults); + Assert.Equal(["settings.json", "settings.json.lock"], FileNames(folder)); + } + + + + /// Without a wait supplied, the store really pauses between attempts instead of spinning through them. + [Fact] + public void Update_LockHeld_DefaultWaitPausesBetweenAttempts() + { + using var temp = new TempDirectory(); + Seed(temp); + var store = new AppSettingsStore(temp.File("settings.json")); + using(new FileStream(temp.File("settings.json.lock"), FileMode.Open, FileAccess.ReadWrite, FileShare.None)) + { + long started = System.Diagnostics.Stopwatch.GetTimestamp(); + Assert.Null(store.Update(settings => settings)); + TimeSpan elapsed = System.Diagnostics.Stopwatch.GetElapsedTime(started); + Assert.True(elapsed >= AppSettingsStore.RetryDelay * (AppSettingsStore.MaximumAttempts - 1) / 2, "The store waited only " + elapsed + "."); + } + } + + + /// A change that throws leaves the file, the lock and the folder as they were; the next save works. [Fact] public void Update_ChangeThrows_LeavesTheFileAndReleasesTheLock() diff --git a/tests/MarkdownViewr.Windows.Core.Tests/FileSwapTests.cs b/tests/MarkdownViewr.Windows.Core.Tests/FileSwapTests.cs index 2b5c013..4868c21 100644 --- a/tests/MarkdownViewr.Windows.Core.Tests/FileSwapTests.cs +++ b/tests/MarkdownViewr.Windows.Core.Tests/FileSwapTests.cs @@ -9,6 +9,20 @@ namespace MarkdownViewr.Windows.Core.Tests; /// Renaming one real file over another, with other real handles open on the destination. public sealed class FileSwapTests { + /// A full path is given the form Windows takes as written: a drive path, a network path, and paths that already have it. + [Theory] + [InlineData(@"C:\data\settings.json", @"\\?\C:\data\settings.json")] + [InlineData(@"\\server\share\settings.json", @"\\?\UNC\server\share\settings.json")] + [InlineData(@"\\?\C:\data\settings.json", @"\\?\C:\data\settings.json")] + [InlineData(@"\\?\UNC\server\share\settings.json", @"\\?\UNC\server\share\settings.json")] + [InlineData(@"\\.\C:\data\settings.json", @"\\.\C:\data\settings.json")] + public void Verbatim_FullPath_IsPrefixedOnce(string path, string expected) + { + Assert.Equal(expected, FileSwap.Verbatim(path)); + } + + + /// The rename replaces the destination and removes the source name. [Fact] public void TryRenameOver_ExistingDestination_ReplacesIt()