From 3d55491f388f6feb48c6707d34fda2772ab3f266 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Mon, 8 Jun 2026 08:20:34 +0000 Subject: [PATCH 1/3] Update default Microsoft Git version to v2.54.0.vfs.0.2 --- .github/workflows/build.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 1aaa7ba42..e517316ea 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -24,7 +24,7 @@ permissions: checks: read env: - GIT_VERSION: ${{ github.event.inputs.git_version || 'v2.53.0.vfs.0.7' }} + GIT_VERSION: ${{ github.event.inputs.git_version || 'v2.54.0.vfs.0.2' }} jobs: validate: From d98a2bb93fec4363bad6ff996b3ff92252391318 Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Wed, 10 Jun 2026 15:38:43 -0700 Subject: [PATCH 2/3] Service: escape JSON arg passed to child mount processes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GVFS.Service uses CreateProcessAsUser to spawn 'GVFS.exe mount --internal_use_only ' on each user logon. The JSON arg ({"ServiceName":null,"StartedByService":true,...}) was concatenated straight into the command line with no escaping. Windows CommandLineToArgvW strips the embedded " characters when re-parsing argv in the child, so the receiving process sees {ServiceName:null,StartedByService:true,...}. Newtonsoft.Json was permissive enough to round-trip that corrupted payload. After commit c83beab4 ("Migrate from Newtonsoft.Json to System.Text.Json"), System.Text.Json (strict — quoted property names required) throws JsonException, the verb hits ReportErrorAndExit *before* AddLogFileEventListener runs, and the child exits without ever creating a mount log file. The service then blocks for the full 60-second WaitUntilMounted timeout on each repo, fails automount, and reports only "the GVFS.Mount process is not responding." Fix: * Add CommandLineEscaping.EscapeArgument implementing the CommandLineToArgvW escaping rules (quote when needed, double backslashes that precede a quote terminator, escape literal " as \"). * Replace CurrentUser.RunAs(string, string) with TryRunAs(string, string[], out int processId) so callers supply argv values and the escaping happens centrally. Also surface the child PID so callers can track its liveness. * WindowsPlatform.StartBackgroundVFS4GProcess: route every arg through the new escaper (previously only space-containing args got naively quoted, embedded " was never escaped). * GVFSEnlistment.WaitUntilMounted: add an overload that takes a Func. When supplied, the wait loop polls the snapshot between short (500 ms) connect attempts and fails fast with the exit code if the child has already terminated, instead of blocking for the full pipe timeout. * GVFSMountProcess: pass the spawned child's PID into the new WaitUntilMounted overload so logon automount detects an early GVFS.exe exit within seconds and logs a useful diagnostic ("GVFS.Mount process (Id N) exited with code X before the named pipe was ready") instead of a 60-second silent timeout. Tests: * CommandLineEscapingTests covers the canonical edge cases (spaces, tabs, embedded quotes, trailing backslashes) and the exact InternalVerbParameters JSON shape that triggered the regression. A round-trip test feeds the escaped form back through CommandLineToArgvW and verifies the original string pops out unchanged. * WaitUntilMountedProcessTrackingTests verifies the wait short-circuits on an exit reported by the first snapshot poll, and also catches a late exit reported on a later poll, both well under the legacy 60-second timeout. Assisted-by: Claude Opus 4.7 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Common/CommandLineEscaping.cs | 105 +++++++++++++++ GVFS/GVFS.Common/GVFSEnlistment.cs | 126 ++++++++++++++++-- GVFS/GVFS.Platform.Windows/CurrentUser.cs | 52 +++++++- GVFS/GVFS.Platform.Windows/WindowsPlatform.cs | 2 +- GVFS/GVFS.Service/GVFSMountProcess.cs | 80 ++++++++++- .../Common/CommandLineEscapingTests.cs | 99 ++++++++++++++ .../WaitUntilMountedProcessTrackingTests.cs | 88 ++++++++++++ 7 files changed, 527 insertions(+), 25 deletions(-) create mode 100644 GVFS/GVFS.Common/CommandLineEscaping.cs create mode 100644 GVFS/GVFS.UnitTests/Common/CommandLineEscapingTests.cs create mode 100644 GVFS/GVFS.UnitTests/Common/WaitUntilMountedProcessTrackingTests.cs diff --git a/GVFS/GVFS.Common/CommandLineEscaping.cs b/GVFS/GVFS.Common/CommandLineEscaping.cs new file mode 100644 index 000000000..e902f0363 --- /dev/null +++ b/GVFS/GVFS.Common/CommandLineEscaping.cs @@ -0,0 +1,105 @@ +using System.Text; + +namespace GVFS.Common +{ + /// + /// Windows command-line argument escaping per the rules used by + /// CommandLineToArgvW and the Microsoft C runtime. + /// + /// + /// + /// When the service spawns a child process via CreateProcessAsUser, + /// or any code path builds a lpCommandLine string, the receiving + /// process re-parses that string into argv. Embedded + /// " characters that aren't escaped get stripped, which + /// silently corrupts JSON payloads (e.g. --internal_use_only) and + /// any other argument that contains quotes. System.Text.Json (unlike + /// Newtonsoft.Json) is strict about quoted property names, so the + /// corruption now manifests as a hard failure. + /// + /// + /// The escaping rules (see + /// ): + /// + /// + /// Arguments are separated by whitespace (space or tab). + /// A string surrounded by " is treated as one argument + /// even if it contains whitespace. + /// \" is interpreted as a literal ". + /// Backslashes are literal unless they immediately + /// precede a ": then 2n backslashes become n + /// backslashes followed by a quote terminator, and 2n+1 + /// backslashes become n backslashes followed by a literal + /// ". + /// + /// + public static class CommandLineEscaping + { + /// + /// Escapes a single argument for inclusion in a Windows command line + /// that will be parsed by CommandLineToArgvW (the default + /// parser used by the CRT and .NET). + /// + /// The raw argument value. + /// + /// The escaped argument, including surrounding quotes when needed. + /// Always quotes when the argument is empty or contains a space, + /// tab, double-quote, or is otherwise ambiguous to the parser. + /// + public static string EscapeArgument(string argument) + { + if (argument == null) + { + return "\"\""; + } + + if (argument.Length > 0 && argument.IndexOfAny(CharactersThatRequireQuoting) < 0) + { + return argument; + } + + StringBuilder builder = new StringBuilder(argument.Length + 2); + builder.Append('"'); + + int i = 0; + while (i < argument.Length) + { + int backslashes = 0; + while (i < argument.Length && argument[i] == '\\') + { + backslashes++; + i++; + } + + if (i == argument.Length) + { + // Backslashes at the end of the argument: double them so + // they don't escape the closing quote we're about to add. + builder.Append('\\', backslashes * 2); + break; + } + + if (argument[i] == '"') + { + // Backslashes preceding a literal quote: double them, then + // emit \" for the literal quote itself. + builder.Append('\\', backslashes * 2 + 1); + builder.Append('"'); + } + else + { + // Backslashes not followed by a quote: emit verbatim. + builder.Append('\\', backslashes); + builder.Append(argument[i]); + } + + i++; + } + + builder.Append('"'); + return builder.ToString(); + } + + private static readonly char[] CharactersThatRequireQuoting = new[] { ' ', '\t', '"' }; + } +} diff --git a/GVFS/GVFS.Common/GVFSEnlistment.cs b/GVFS/GVFS.Common/GVFSEnlistment.cs index 286b32037..457e3775b 100644 --- a/GVFS/GVFS.Common/GVFSEnlistment.cs +++ b/GVFS/GVFS.Common/GVFSEnlistment.cs @@ -219,22 +219,48 @@ public static bool WaitUntilMounted(ITracer tracer, string enlistmentRoot, bool } public static bool WaitUntilMounted(ITracer tracer, string pipeName, string enlistmentRoot, bool unattended, out string errorMessage) + { + return WaitUntilMounted(tracer, pipeName, enlistmentRoot, unattended, mountProcessStatus: null, out errorMessage); + } + + /// + /// Waits for the GVFS.Mount process to come up and signal readiness + /// over its named pipe. + /// + /// + /// Optional snapshot delegate. When provided, the wait loop polls it + /// during pipe-connection attempts so the caller can fail fast if the + /// mount process exits before the pipe is created — instead of + /// blocking on the full 60-second pipe timeout. Callers that don't + /// have a handle to the child process (e.g. clients connecting to + /// somebody else's mount) pass null. + /// + public static bool WaitUntilMounted( + ITracer tracer, + string pipeName, + string enlistmentRoot, + bool unattended, + Func mountProcessStatus, + out string errorMessage) { tracer.RelatedInfo($"{nameof(WaitUntilMounted)}: Creating NamedPipeClient for pipe '{pipeName}'"); + tracer.RelatedInfo($"{nameof(WaitUntilMounted)}: Connecting to '{pipeName}'"); errorMessage = null; - using (NamedPipeClient pipeClient = new NamedPipeClient(pipeName)) + int totalTimeoutMs = unattended ? 300000 : 60000; + NamedPipeClient pipeClient = TryConnectWithProcessTracking( + tracer, + pipeName, + totalTimeoutMs, + mountProcessStatus, + out errorMessage); + if (pipeClient == null) { - tracer.RelatedInfo($"{nameof(WaitUntilMounted)}: Connecting to '{pipeName}'"); - - int timeout = unattended ? 300000 : 60000; - if (!pipeClient.Connect(timeout)) - { - tracer.RelatedError($"{nameof(WaitUntilMounted)}: Failed to connect to '{pipeName}' after {timeout} ms"); - errorMessage = "Unable to mount because the GVFS.Mount process is not responding."; - return false; - } + return false; + } + using (pipeClient) + { tracer.RelatedInfo($"{nameof(WaitUntilMounted)}: Connected to '{pipeName}'"); while (true) @@ -280,6 +306,86 @@ public static bool WaitUntilMounted(ITracer tracer, string pipeName, string enli } } + private static NamedPipeClient TryConnectWithProcessTracking( + ITracer tracer, + string pipeName, + int totalTimeoutMs, + Func mountProcessStatus, + out string errorMessage) + { + errorMessage = null; + + // When no process snapshot is supplied, fall back to a single + // long-timeout connect to preserve previous behavior for callers + // that don't own the mount process (e.g. external pipe clients). + if (mountProcessStatus == null) + { + NamedPipeClient pipeClient = new NamedPipeClient(pipeName); + if (pipeClient.Connect(totalTimeoutMs)) + { + return pipeClient; + } + + pipeClient.Dispose(); + tracer.RelatedError($"{nameof(WaitUntilMounted)}: Failed to connect to '{pipeName}' after {totalTimeoutMs} ms"); + errorMessage = "Unable to mount because the GVFS.Mount process is not responding."; + return null; + } + + // With process tracking, retry with short connect attempts so we + // can detect early termination within seconds. + const int PerAttemptTimeoutMs = 500; + DateTime deadline = DateTime.UtcNow.AddMilliseconds(totalTimeoutMs); + while (true) + { + MountProcessSnapshot snapshot = mountProcessStatus(); + if (snapshot.HasExited) + { + errorMessage = string.Format( + "GVFS.Mount process (Id {0}) exited with code {1} before the named pipe was ready.", + snapshot.ProcessId, + snapshot.ExitCode); + tracer.RelatedError($"{nameof(WaitUntilMounted)}: {errorMessage}"); + return null; + } + + NamedPipeClient pipeClient = new NamedPipeClient(pipeName); + if (pipeClient.Connect(PerAttemptTimeoutMs)) + { + return pipeClient; + } + + pipeClient.Dispose(); + + if (DateTime.UtcNow >= deadline) + { + tracer.RelatedError($"{nameof(WaitUntilMounted)}: Failed to connect to '{pipeName}' after {totalTimeoutMs} ms (mount process Id {snapshot.ProcessId} still running)"); + errorMessage = "Unable to mount because the GVFS.Mount process is not responding."; + return null; + } + } + } + + /// + /// Snapshot of a child mount process's liveness, used by + /// + /// to short-circuit the pipe-wait when the child has crashed. + /// + public readonly struct MountProcessSnapshot + { + public MountProcessSnapshot(int processId, bool hasExited, int exitCode) + { + this.ProcessId = processId; + this.HasExited = hasExited; + this.ExitCode = exitCode; + } + + public int ProcessId { get; } + public bool HasExited { get; } + public int ExitCode { get; } + } + + public void SetGitVersion(string gitVersion) { this.SetOnce(gitVersion, ref this.gitVersion); diff --git a/GVFS/GVFS.Platform.Windows/CurrentUser.cs b/GVFS/GVFS.Platform.Windows/CurrentUser.cs index 1d1ff151f..30137f9bf 100644 --- a/GVFS/GVFS.Platform.Windows/CurrentUser.cs +++ b/GVFS/GVFS.Platform.Windows/CurrentUser.cs @@ -1,9 +1,11 @@ -using GVFS.Common.Tracing; +using GVFS.Common; +using GVFS.Common.Tracing; using System; using System.Collections.Generic; using System.ComponentModel; using System.Runtime.InteropServices; using System.Security.Principal; +using System.Text; namespace GVFS.Platform.Windows { @@ -108,12 +110,29 @@ private enum CreateProcessFlags : uint /// /// Launches a process for the current user. - /// This code will only work when running in a windows service running as LocalSystem - /// with the SE_TCB_NAME privilege. + /// This code will only work when running in a windows service running + /// as LocalSystem with the SE_TCB_NAME privilege. /// - /// True on successful process start - public bool RunAs(string processName, string args) + /// Full path to the executable to launch. + /// + /// Argument values exactly as the child process should see them in its + /// argv. Each value is escaped according to + /// CommandLineToArgvW rules so embedded quotes, spaces, and + /// backslashes round-trip safely. Passing pre-concatenated argument + /// strings here would re-introduce the quote-stripping bug that + /// silently corrupts the service's --internal_use_only JSON. + /// + /// + /// On success, the PID of the newly created process so the caller can + /// detect early termination (e.g. via + /// ) + /// instead of waiting the full pipe-connection timeout when the child + /// has already crashed. + /// + /// true if the process was successfully created. + public bool TryRunAs(string processName, string[] arguments, out int processId) { + processId = 0; IntPtr environment = IntPtr.Zero; IntPtr duplicate = IntPtr.Zero; if (this.token == IntPtr.Zero) @@ -121,6 +140,8 @@ public bool RunAs(string processName, string args) return false; } + string commandLine = BuildCommandLine(processName, arguments); + try { if (DuplicateTokenEx( @@ -140,7 +161,7 @@ public bool RunAs(string processName, string args) if (CreateProcessAsUser( duplicate, null, - string.Format("\"{0}\" {1}", processName, args), + commandLine, IntPtr.Zero, IntPtr.Zero, inheritHandles: false, @@ -152,7 +173,8 @@ public bool RunAs(string processName, string args) { try { - this.tracer.RelatedInfo("Started process '{0} {1}' with Id {2}", processName, args, procInfo.ProcessId); + processId = procInfo.ProcessId; + this.tracer.RelatedInfo("Started process '{0}' with Id {1}", commandLine, processId); } finally { @@ -193,6 +215,22 @@ public bool RunAs(string processName, string args) return false; } + private static string BuildCommandLine(string processName, string[] arguments) + { + StringBuilder builder = new StringBuilder(); + builder.Append(CommandLineEscaping.EscapeArgument(processName)); + if (arguments != null) + { + foreach (string argument in arguments) + { + builder.Append(' '); + builder.Append(CommandLineEscaping.EscapeArgument(argument)); + } + } + + return builder.ToString(); + } + /// /// Returns session IDs for sessions that have a logged-in user /// whose token can be queried via WTSQueryUserToken. diff --git a/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs b/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs index b8593f417..df8ccf873 100644 --- a/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs +++ b/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs @@ -142,7 +142,7 @@ public override Process StartBackgroundVFS4GProcess(ITracer tracer, string progr string programArguments = string.Empty; try { - programArguments = string.Join(" ", args.Select(arg => arg.Contains(' ') ? "\"" + arg + "\"" : arg)); + programArguments = string.Join(" ", args.Select(arg => CommandLineEscaping.EscapeArgument(arg))); ProcessStartInfo processInfo = new ProcessStartInfo(programName, programArguments); // UseShellExecute=true uses ShellExecuteEx which does NOT inherit diff --git a/GVFS/GVFS.Service/GVFSMountProcess.cs b/GVFS/GVFS.Service/GVFSMountProcess.cs index 4e65e6c62..74b77b30c 100644 --- a/GVFS/GVFS.Service/GVFSMountProcess.cs +++ b/GVFS/GVFS.Service/GVFSMountProcess.cs @@ -2,6 +2,8 @@ using GVFS.Common.Tracing; using GVFS.Platform.Windows; using GVFS.Service.Handlers; +using System; +using System.Diagnostics; namespace GVFS.Service { @@ -28,7 +30,8 @@ public bool MountRepository(string repoRoot, int sessionId) using (CurrentUser currentUser = new CurrentUser(this.tracer, sessionId)) { - if (!this.CallGVFSMount(repoRoot, currentUser)) + int mountProcessId; + if (!this.TryCallGVFSMount(repoRoot, currentUser, out mountProcessId)) { this.tracer.RelatedError($"{nameof(this.MountRepository)}: Unable to start the GVFS.exe process."); return false; @@ -53,22 +56,85 @@ public bool MountRepository(string repoRoot, int sessionId) } } - if (!GVFSEnlistment.WaitUntilMounted(this.tracer, pipeName, repoRoot, false, out errorMessage)) + // Track the spawned mount process so the wait short-circuits + // when it dies early — e.g. on argument-parsing failures that + // exit before any log file is created. Without this, the + // service would block for the full 60-second pipe timeout + // with no diagnostic beyond "not responding." + Process mountProcess = TryGetProcessById(this.tracer, mountProcessId); + Func snapshot = + mountProcess == null + ? (Func)null + : () => SnapshotMountProcess(mountProcess, mountProcessId); + + try { - this.tracer.RelatedError(errorMessage); - return false; + if (!GVFSEnlistment.WaitUntilMounted(this.tracer, pipeName, repoRoot, unattended: false, snapshot, out errorMessage)) + { + this.tracer.RelatedError(errorMessage); + return false; + } + } + finally + { + mountProcess?.Dispose(); } } return true; } - private bool CallGVFSMount(string repoRoot, CurrentUser currentUser) + private static Process TryGetProcessById(ITracer tracer, int processId) + { + try + { + return Process.GetProcessById(processId); + } + catch (ArgumentException) + { + // Process already exited between CreateProcessAsUser returning + // and us looking it up. Wait loop will catch this on first poll. + return null; + } + catch (InvalidOperationException e) + { + tracer.RelatedWarning($"{nameof(TryGetProcessById)}: Could not open handle to mount process Id {processId}: {e.Message}"); + return null; + } + } + + private static GVFSEnlistment.MountProcessSnapshot SnapshotMountProcess(Process mountProcess, int processId) + { + try + { + if (mountProcess.HasExited) + { + return new GVFSEnlistment.MountProcessSnapshot(processId, hasExited: true, exitCode: mountProcess.ExitCode); + } + } + catch (InvalidOperationException) + { + // No process associated — treat as exited so the caller fails fast. + return new GVFSEnlistment.MountProcessSnapshot(processId, hasExited: true, exitCode: -1); + } + + return new GVFSEnlistment.MountProcessSnapshot(processId, hasExited: false, exitCode: 0); + } + + private bool TryCallGVFSMount(string repoRoot, CurrentUser currentUser, out int processId) { InternalVerbParameters mountInternal = new InternalVerbParameters(startedByService: true); - return currentUser.RunAs( + return currentUser.TryRunAs( Configuration.Instance.GVFSLocation, - $"mount {repoRoot} --{GVFSConstants.VerbParameters.InternalUseOnly} {mountInternal.ToJson()}"); + new[] + { + "mount", + repoRoot, + "--" + GVFSConstants.VerbParameters.InternalUseOnly, + mountInternal.ToJson(), + }, + out processId); } } } + diff --git a/GVFS/GVFS.UnitTests/Common/CommandLineEscapingTests.cs b/GVFS/GVFS.UnitTests/Common/CommandLineEscapingTests.cs new file mode 100644 index 000000000..a34939963 --- /dev/null +++ b/GVFS/GVFS.UnitTests/Common/CommandLineEscapingTests.cs @@ -0,0 +1,99 @@ +using GVFS.Common; +using GVFS.Tests.Should; +using NUnit.Framework; +using System.Runtime.InteropServices; + +namespace GVFS.UnitTests.Common +{ + [TestFixture] + public class CommandLineEscapingTests + { + [TestCase("simple", "simple", Description = "No special characters: no quoting")] + [TestCase("with space", "\"with space\"")] + [TestCase("with\ttab", "\"with\ttab\"")] + [TestCase("with\"quote", "\"with\\\"quote\"")] + [TestCase("ends_with_backslash\\", "ends_with_backslash\\")] + [TestCase("path\\with\\backslashes", "path\\with\\backslashes")] + [TestCase("path with\\backslashes", "\"path with\\backslashes\"")] + [TestCase("trailing_backslash_with quote\\", "\"trailing_backslash_with quote\\\\\"")] + [TestCase("a\\\\b c", "\"a\\\\b c\"", Description = "Internal double-backslash not before quote: kept verbatim")] + [TestCase("a\\\\\"b", "\"a\\\\\\\\\\\"b\"", Description = "Two backslashes before quote: doubled to four plus escaped quote")] + [TestCase("", "\"\"", Description = "Empty argument: must be quoted so it isn't lost")] + public void EscapeArgument_ProducesExpectedString(string input, string expected) + { + CommandLineEscaping.EscapeArgument(input).ShouldEqual(expected); + } + + [TestCase] + public void EscapeArgument_NullInputIsEmptyQuotedString() + { + CommandLineEscaping.EscapeArgument(null).ShouldEqual("\"\""); + } + + [TestCase("simple")] + [TestCase("with space")] + [TestCase("with\"quote")] + [TestCase("ends_with_backslash\\")] + [TestCase("a\\\\\"b")] + [TestCase("path\\with\\backslashes")] + [TestCase("path with\\backslashes")] + [TestCase("trailing_backslash_with quote\\")] + [TestCase("{\"ServiceName\":null,\"StartedByService\":true,\"MaintenanceJob\":null,\"PackfileMaintenanceBatchSize\":null}", + Description = "The exact InternalVerbParameters.ToJson() shape that triggered the automount regression")] + [TestCase("")] + public void EscapeArgument_RoundTripsThroughCommandLineToArgvW(string input) + { + if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + { + Assert.Ignore("CommandLineToArgvW is a Windows-only API."); + } + + // Build a command line that begins with a fake program name so + // CommandLineToArgvW has a leading token to chew on, then the + // escaped argument we care about. + string commandLine = "stub.exe " + CommandLineEscaping.EscapeArgument(input); + + string[] argv = CommandLineToArgs(commandLine); + + argv.Length.ShouldEqual(2, "Expected exactly one argument after the program name"); + argv[1].ShouldEqual(input ?? string.Empty); + } + + private static string[] CommandLineToArgs(string commandLine) + { + int argc; + System.IntPtr argv = NativeMethods.CommandLineToArgvW(commandLine, out argc); + if (argv == System.IntPtr.Zero) + { + throw new System.ComponentModel.Win32Exception(); + } + + try + { + string[] args = new string[argc]; + for (int i = 0; i < argc; i++) + { + System.IntPtr p = Marshal.ReadIntPtr(argv, i * System.IntPtr.Size); + args[i] = Marshal.PtrToStringUni(p); + } + + return args; + } + finally + { + NativeMethods.LocalFree(argv); + } + } + + private static class NativeMethods + { + [DllImport("shell32.dll", CharSet = CharSet.Unicode, SetLastError = true)] + public static extern System.IntPtr CommandLineToArgvW( + [MarshalAs(UnmanagedType.LPWStr)] string lpCmdLine, + out int pNumArgs); + + [DllImport("kernel32.dll", SetLastError = true)] + public static extern System.IntPtr LocalFree(System.IntPtr hMem); + } + } +} diff --git a/GVFS/GVFS.UnitTests/Common/WaitUntilMountedProcessTrackingTests.cs b/GVFS/GVFS.UnitTests/Common/WaitUntilMountedProcessTrackingTests.cs new file mode 100644 index 000000000..cf04194eb --- /dev/null +++ b/GVFS/GVFS.UnitTests/Common/WaitUntilMountedProcessTrackingTests.cs @@ -0,0 +1,88 @@ +using GVFS.Common; +using GVFS.Tests.Should; +using GVFS.UnitTests.Mock.Common; +using NUnit.Framework; +using System; +using System.Threading; + +namespace GVFS.UnitTests.Common +{ + [TestFixture] + public class WaitUntilMountedProcessTrackingTests + { + [TestCase] + public void ReturnsImmediatelyWhenMountProcessSnapshotReportsExited() + { + const int FakePid = 13579; + const int FakeExitCode = 42; + int snapshotCallCount = 0; + Func snapshot = () => + { + Interlocked.Increment(ref snapshotCallCount); + return new GVFSEnlistment.MountProcessSnapshot(FakePid, hasExited: true, exitCode: FakeExitCode); + }; + + string errorMessage; + DateTime start = DateTime.UtcNow; + bool result = GVFSEnlistment.WaitUntilMounted( + new MockTracer(), + pipeName: "GVFS_no_such_pipe_for_test_" + Guid.NewGuid().ToString("N"), + enlistmentRoot: "C:\\fake\\root", + unattended: false, + mountProcessStatus: snapshot, + out errorMessage); + TimeSpan elapsed = DateTime.UtcNow - start; + + result.ShouldBeFalse(); + errorMessage.ShouldNotBeNull(); + errorMessage.ShouldContain(FakePid.ToString()); + errorMessage.ShouldContain(FakeExitCode.ToString()); + snapshotCallCount.ShouldBeAtLeast(1); + + // The legacy code path would have blocked for the full 60 second + // pipe timeout. With process tracking we should bail out in well + // under a second since the snapshot is checked before any + // connect attempt. + Assert.That(elapsed.TotalSeconds, Is.LessThan(5), "WaitUntilMounted should bail out quickly when the snapshot reports the mount process exited"); + } + + [TestCase] + public void DetectsLateProcessExitWhilePipeNeverAppears() + { + const int FakePid = 24680; + const int FakeExitCode = -1; + int snapshotCallCount = 0; + Func snapshot = () => + { + int count = Interlocked.Increment(ref snapshotCallCount); + + // Pretend the mount process is still running for the first + // couple of polls, then suddenly report it exited. + bool exited = count >= 2; + return new GVFSEnlistment.MountProcessSnapshot( + FakePid, + hasExited: exited, + exitCode: exited ? FakeExitCode : 0); + }; + + string errorMessage; + DateTime start = DateTime.UtcNow; + bool result = GVFSEnlistment.WaitUntilMounted( + new MockTracer(), + pipeName: "GVFS_no_such_pipe_for_test_" + Guid.NewGuid().ToString("N"), + enlistmentRoot: "C:\\fake\\root", + unattended: false, + mountProcessStatus: snapshot, + out errorMessage); + TimeSpan elapsed = DateTime.UtcNow - start; + + result.ShouldBeFalse(); + errorMessage.ShouldContain(FakePid.ToString()); + errorMessage.ShouldContain(FakeExitCode.ToString()); + + // Per-attempt connect timeout is 500ms; we expect to discover the + // exit on the second poll, well within a few seconds. + Assert.That(elapsed.TotalSeconds, Is.LessThan(10), "WaitUntilMounted should detect late process exit within a few connect retries"); + } + } +} From b7bac259c9da10f4f532ff3cd15f66f35fec7b7f Mon Sep 17 00:00:00 2001 From: Tyrie Vella Date: Wed, 10 Jun 2026 16:32:21 -0700 Subject: [PATCH 3/3] Service: hand mount Process handle to wait, not PID MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address code-review feedback on the prior commit. The wait loop used the spawned mount process's PID and re-opened it via Process.GetProcessById on every poll, which has two problems: 1. The lookup races with the very failure mode the fast-fail path is meant to catch. If the child exits between CreateProcessAsUser returning and GVFSMountProcess looking it up, GetProcessById throws ArgumentException, TryGetProcessById returns null, and WaitUntilMounted silently falls back to the legacy single 60-second blocking Connect — defeating the whole purpose. 2. Querying by PID is vulnerable to PID reuse: if Windows recycles the PID quickly enough, a snapshot poll could observe an unrelated process's exit state. Both problems disappear if we keep the process handle that CreateProcessAsUser already returned. The kernel keeps the process object alive for as long as we hold the handle, so liveness and exit-code queries remain valid even after the child has exited, and PID reuse is irrelevant because we never look up by PID again. Changes: * CurrentUser.TryRunAs returns a SafeProcessHandle (owned) instead of just a PID. Internally, the thread handle is still closed immediately; only the process handle is retained. * New ProcessHandleHelper exposes HasExited (non-blocking WaitForSingleObject(handle, 0)) and TryGetExitCode (GetExitCodeProcess) directly against the handle. This avoids the STILL_ACTIVE (259) ambiguity in GetExitCodeProcess alone — we only ask for the exit code once the wait says signaled. * GVFSMountProcess.MountRepository owns the SafeProcessHandle in a using block spanning the entire WaitUntilMounted call and uses the handle for snapshot queries. * Removed TryGetProcessById and the Process.GetProcessById / Process.HasExited path entirely — no PID lookup remains in the automount code path. Tests: * Build clean, 0 errors. * All 842 unit tests still pass (24 new + 818 existing). The existing WaitUntilMountedProcessTrackingTests don't need changes because they exercise the snapshot delegate, not the handle plumbing. Assisted-by: Claude Opus 4.7 Signed-off-by: Tyrie Vella --- GVFS/GVFS.Platform.Windows/CurrentUser.cs | 41 ++++--- .../ProcessHandleHelper.cs | 58 ++++++++++ GVFS/GVFS.Service/GVFSMountProcess.cs | 107 ++++++++---------- 3 files changed, 127 insertions(+), 79 deletions(-) create mode 100644 GVFS/GVFS.Platform.Windows/ProcessHandleHelper.cs diff --git a/GVFS/GVFS.Platform.Windows/CurrentUser.cs b/GVFS/GVFS.Platform.Windows/CurrentUser.cs index 30137f9bf..1b4fda012 100644 --- a/GVFS/GVFS.Platform.Windows/CurrentUser.cs +++ b/GVFS/GVFS.Platform.Windows/CurrentUser.cs @@ -1,5 +1,6 @@ using GVFS.Common; using GVFS.Common.Tracing; +using Microsoft.Win32.SafeHandles; using System; using System.Collections.Generic; using System.ComponentModel; @@ -122,16 +123,25 @@ private enum CreateProcessFlags : uint /// strings here would re-introduce the quote-stripping bug that /// silently corrupts the service's --internal_use_only JSON. /// + /// + /// On success, an owned handle to the newly created process. Callers + /// can use and + /// to query the + /// child's liveness without racing on PID lookup (the kernel keeps + /// the process object alive as long as we hold the handle, even + /// after the child exits, so the handle is always queryable and + /// never aliases a reused PID). Callers must + /// it when done — usually with using. + /// /// - /// On success, the PID of the newly created process so the caller can - /// detect early termination (e.g. via - /// ) - /// instead of waiting the full pipe-connection timeout when the child - /// has already crashed. + /// On success, the PID of the newly created process (for logging + /// and diagnostics only — use for + /// any liveness check). /// /// true if the process was successfully created. - public bool TryRunAs(string processName, string[] arguments, out int processId) + public bool TryRunAs(string processName, string[] arguments, out SafeProcessHandle processHandle, out int processId) { + processHandle = null; processId = 0; IntPtr environment = IntPtr.Zero; IntPtr duplicate = IntPtr.Zero; @@ -171,17 +181,14 @@ public bool TryRunAs(string processName, string[] arguments, out int processId) startupInfo: ref info, processInformation: out procInfo)) { - try - { - processId = procInfo.ProcessId; - this.tracer.RelatedInfo("Started process '{0}' with Id {1}", commandLine, processId); - } - finally - { - CloseHandle(procInfo.ProcessHandle); - CloseHandle(procInfo.ThreadHandle); - } - + // Always close the thread handle (we never use it). + // Wrap the process handle in a SafeProcessHandle that + // owns it; callers get a race-free liveness handle that + // remains queryable even after the child exits. + CloseHandle(procInfo.ThreadHandle); + processId = procInfo.ProcessId; + processHandle = new SafeProcessHandle(procInfo.ProcessHandle, ownsHandle: true); + this.tracer.RelatedInfo("Started process '{0}' with Id {1}", commandLine, processId); return true; } else diff --git a/GVFS/GVFS.Platform.Windows/ProcessHandleHelper.cs b/GVFS/GVFS.Platform.Windows/ProcessHandleHelper.cs new file mode 100644 index 000000000..d2b831864 --- /dev/null +++ b/GVFS/GVFS.Platform.Windows/ProcessHandleHelper.cs @@ -0,0 +1,58 @@ +using Microsoft.Win32.SafeHandles; +using System.Runtime.InteropServices; + +namespace GVFS.Platform.Windows +{ + /// + /// Race-free liveness checks against a Win32 process handle. + /// Unlike , + /// these helpers query an already-opened handle, so they cannot lose the + /// race where the target exits between the caller starting it and the + /// caller looking it up, and they cannot alias a reused PID. The kernel + /// keeps the process object alive for as long as the handle is held, + /// even after the child has exited and its PID has been reused. + /// + public static class ProcessHandleHelper + { + // From WinBase.h. WaitForSingleObject signals the process handle + // immediately when the process has exited; we pass a zero timeout + // so we never block. + private const uint WaitObject0 = 0x00000000; + private const uint WaitTimeout = 0x00000102; + + /// + /// Returns true if the process has exited. Uses a non-blocking + /// wait so it's safe to call from a polling loop. + /// + public static bool HasExited(SafeProcessHandle handle) + { + uint result = WaitForSingleObject(handle, 0); + return result == WaitObject0; + } + + /// + /// Reads the process's exit code. Only meaningful after + /// returns true. Returns false + /// if the underlying Win32 call fails. + /// + public static bool TryGetExitCode(SafeProcessHandle handle, out int exitCode) + { + uint code; + if (GetExitCodeProcess(handle, out code)) + { + exitCode = unchecked((int)code); + return true; + } + + exitCode = -1; + return false; + } + + [DllImport("kernel32.dll", SetLastError = true)] + private static extern uint WaitForSingleObject(SafeProcessHandle handle, uint timeoutMilliseconds); + + [DllImport("kernel32.dll", SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + private static extern bool GetExitCodeProcess(SafeProcessHandle handle, out uint exitCode); + } +} diff --git a/GVFS/GVFS.Service/GVFSMountProcess.cs b/GVFS/GVFS.Service/GVFSMountProcess.cs index 74b77b30c..08f4746aa 100644 --- a/GVFS/GVFS.Service/GVFSMountProcess.cs +++ b/GVFS/GVFS.Service/GVFSMountProcess.cs @@ -1,9 +1,9 @@ -using GVFS.Common; +using GVFS.Common; using GVFS.Common.Tracing; using GVFS.Platform.Windows; using GVFS.Service.Handlers; +using Microsoft.Win32.SafeHandles; using System; -using System.Diagnostics; namespace GVFS.Service { @@ -30,98 +30,79 @@ public bool MountRepository(string repoRoot, int sessionId) using (CurrentUser currentUser = new CurrentUser(this.tracer, sessionId)) { + SafeProcessHandle mountHandle; int mountProcessId; - if (!this.TryCallGVFSMount(repoRoot, currentUser, out mountProcessId)) + if (!this.TryCallGVFSMount(repoRoot, currentUser, out mountHandle, out mountProcessId)) { this.tracer.RelatedError($"{nameof(this.MountRepository)}: Unable to start the GVFS.exe process."); return false; } - string errorMessage; - string pipeName = GVFSPlatform.Instance.GetNamedPipeName(repoRoot); - string worktreeError; - GVFSEnlistment.WorktreeInfo wtInfo = GVFSEnlistment.TryGetWorktreeInfo(repoRoot, out worktreeError); - if (worktreeError != null) + // Always own the handle for the rest of this call so the + // kernel keeps the process object alive while we poll it. + using (mountHandle) { - this.tracer.RelatedError($"Failed to check worktree status for '{repoRoot}': {worktreeError}"); - return false; - } + string errorMessage; + string pipeName = GVFSPlatform.Instance.GetNamedPipeName(repoRoot); + string worktreeError; + GVFSEnlistment.WorktreeInfo wtInfo = GVFSEnlistment.TryGetWorktreeInfo(repoRoot, out worktreeError); + if (worktreeError != null) + { + this.tracer.RelatedError($"Failed to check worktree status for '{repoRoot}': {worktreeError}"); + return false; + } - if (wtInfo?.SharedGitDir != null) - { - string enlistmentRoot = wtInfo.GetEnlistmentRoot(); - if (enlistmentRoot != null) + if (wtInfo?.SharedGitDir != null) { - pipeName = GVFSPlatform.Instance.GetNamedPipeName(enlistmentRoot) + wtInfo.PipeSuffix; + string enlistmentRoot = wtInfo.GetEnlistmentRoot(); + if (enlistmentRoot != null) + { + pipeName = GVFSPlatform.Instance.GetNamedPipeName(enlistmentRoot) + wtInfo.PipeSuffix; + } } - } - // Track the spawned mount process so the wait short-circuits - // when it dies early — e.g. on argument-parsing failures that - // exit before any log file is created. Without this, the - // service would block for the full 60-second pipe timeout - // with no diagnostic beyond "not responding." - Process mountProcess = TryGetProcessById(this.tracer, mountProcessId); - Func snapshot = - mountProcess == null - ? (Func)null - : () => SnapshotMountProcess(mountProcess, mountProcessId); + // Track the spawned mount process so the wait short-circuits + // when it dies early — e.g. on argument-parsing failures that + // exit before any log file is created. Without this, the + // service would block for the full 60-second pipe timeout + // with no diagnostic beyond "not responding." + // + // We use the SafeProcessHandle returned by TryRunAs rather + // than Process.GetProcessById(pid) so we cannot race against + // the child exiting between CreateProcessAsUser and the + // lookup, and cannot alias a reused PID. + SafeProcessHandle handle = mountHandle; + Func snapshot = + () => SnapshotMountProcess(handle, mountProcessId); - try - { if (!GVFSEnlistment.WaitUntilMounted(this.tracer, pipeName, repoRoot, unattended: false, snapshot, out errorMessage)) { this.tracer.RelatedError(errorMessage); return false; } } - finally - { - mountProcess?.Dispose(); - } } return true; } - private static Process TryGetProcessById(ITracer tracer, int processId) + private static GVFSEnlistment.MountProcessSnapshot SnapshotMountProcess(SafeProcessHandle handle, int processId) { - try + if (!ProcessHandleHelper.HasExited(handle)) { - return Process.GetProcessById(processId); + return new GVFSEnlistment.MountProcessSnapshot(processId, hasExited: false, exitCode: 0); } - catch (ArgumentException) - { - // Process already exited between CreateProcessAsUser returning - // and us looking it up. Wait loop will catch this on first poll. - return null; - } - catch (InvalidOperationException e) - { - tracer.RelatedWarning($"{nameof(TryGetProcessById)}: Could not open handle to mount process Id {processId}: {e.Message}"); - return null; - } - } - private static GVFSEnlistment.MountProcessSnapshot SnapshotMountProcess(Process mountProcess, int processId) - { - try + int exitCode; + if (!ProcessHandleHelper.TryGetExitCode(handle, out exitCode)) { - if (mountProcess.HasExited) - { - return new GVFSEnlistment.MountProcessSnapshot(processId, hasExited: true, exitCode: mountProcess.ExitCode); - } - } - catch (InvalidOperationException) - { - // No process associated — treat as exited so the caller fails fast. - return new GVFSEnlistment.MountProcessSnapshot(processId, hasExited: true, exitCode: -1); + exitCode = -1; } - return new GVFSEnlistment.MountProcessSnapshot(processId, hasExited: false, exitCode: 0); + return new GVFSEnlistment.MountProcessSnapshot(processId, hasExited: true, exitCode: exitCode); } - private bool TryCallGVFSMount(string repoRoot, CurrentUser currentUser, out int processId) + private bool TryCallGVFSMount(string repoRoot, CurrentUser currentUser, out SafeProcessHandle processHandle, out int processId) { InternalVerbParameters mountInternal = new InternalVerbParameters(startedByService: true); return currentUser.TryRunAs( @@ -133,8 +114,10 @@ private bool TryCallGVFSMount(string repoRoot, CurrentUser currentUser, out int "--" + GVFSConstants.VerbParameters.InternalUseOnly, mountInternal.ToJson(), }, + out processHandle, out processId); } } } +