Service: Fix automount regression - escape JSON arg passed to child mount processes

GVFS/GVFS.Common/CommandLineEscaping.cs

@@ -0,0 +1,105 @@
+using System.Text;
+
+namespace GVFS.Common
+{
+    /// <summary>
+    /// Windows command-line argument escaping per the rules used by
+    /// <c>CommandLineToArgvW</c> and the Microsoft C runtime.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// When the service spawns a child process via <c>CreateProcessAsUser</c>,
+    /// or any code path builds a <c>lpCommandLine</c> string, the receiving
+    /// process re-parses that string into <c>argv</c>.  Embedded
+    /// <c>"</c> characters that aren't escaped get <em>stripped</em>, which
+    /// silently corrupts JSON payloads (e.g. <c>--internal_use_only</c>) 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.
+    /// </para>
+    /// <para>
+    /// The escaping rules (see
+    /// <see href="https://learn.microsoft.com/cpp/cpp/main-function-command-line-args#parsing-c-command-line-arguments"/>):
+    /// </para>
+    /// <list type="bullet">
+    ///   <item>Arguments are separated by whitespace (space or tab).</item>
+    ///   <item>A string surrounded by <c>"</c> is treated as one argument
+    ///         even if it contains whitespace.</item>
+    ///   <item><c>\"</c> is interpreted as a literal <c>"</c>.</item>
+    ///   <item>Backslashes are literal <em>unless</em> they immediately
+    ///         precede a <c>"</c>: then <c>2n</c> backslashes become <c>n</c>
+    ///         backslashes followed by a quote terminator, and <c>2n+1</c>
+    ///         backslashes become <c>n</c> backslashes followed by a literal
+    ///         <c>"</c>.</item>
+    /// </list>
+    /// </remarks>
+    public static class CommandLineEscaping
+    {
+        /// <summary>
+        /// Escapes a single argument for inclusion in a Windows command line
+        /// that will be parsed by <c>CommandLineToArgvW</c> (the default
+        /// parser used by the CRT and .NET).
+        /// </summary>
+        /// <param name="argument">The raw argument value.</param>
+        /// <returns>
+        /// 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.
+        /// </returns>
+        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', '"' };
+    }
+}

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);
+        }
+
+        /// <summary>
+        /// Waits for the GVFS.Mount process to come up and signal readiness
+        /// over its named pipe.
+        /// </summary>
+        /// <param name="mountProcessStatus">
+        /// 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 <c>null</c>.
+        /// </param>
+        public static bool WaitUntilMounted(
+            ITracer tracer,
+            string pipeName,
+            string enlistmentRoot,
+            bool unattended,
+            Func<MountProcessSnapshot> 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<MountProcessSnapshot> 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;
+                }
+            }
+        }
+
+        /// <summary>
+        /// Snapshot of a child mount process's liveness, used by
+        /// <see cref="WaitUntilMounted(ITracer, string, string, bool, Func{MountProcessSnapshot}, out string)"/>
+        /// to short-circuit the pipe-wait when the child has crashed.
+        /// </summary>
+        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);

GVFS/GVFS.Platform.Windows/CurrentUser.cs

@@ -1,9 +1,12 @@
-using GVFS.Common.Tracing;
+using GVFS.Common;
+using GVFS.Common.Tracing;
+using Microsoft.Win32.SafeHandles;
 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,19 +111,47 @@ private enum CreateProcessFlags : uint
 
         /// <summary>
         /// 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.
         /// </summary>
-        /// <returns>True on successful process start</returns>
-        public bool RunAs(string processName, string args)
+        /// <param name="processName">Full path to the executable to launch.</param>
+        /// <param name="arguments">
+        /// Argument values exactly as the child process should see them in its
+        /// <c>argv</c>. Each value is escaped according to
+        /// <c>CommandLineToArgvW</c> 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 <c>--internal_use_only</c> JSON.
+        /// </param>
+        /// <param name="processHandle">
+        /// On success, an owned handle to the newly created process. Callers
+        /// can use <see cref="ProcessHandleHelper.HasExited"/> and
+        /// <see cref="ProcessHandleHelper.TryGetExitCode"/> 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 <see cref="SafeProcessHandle.Dispose"/>
+        /// it when done — usually with <c>using</c>.
+        /// </param>
+        /// <param name="processId">
+        /// On success, the PID of the newly created process (for logging
+        /// and diagnostics only — use <paramref name="processHandle"/> for
+        /// any liveness check).
+        /// </param>
+        /// <returns><c>true</c> if the process was successfully created.</returns>
+        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;
             if (this.token == IntPtr.Zero)
             {
                 return false;
             }
 
+            string commandLine = BuildCommandLine(processName, arguments);
+
             try
             {
                 if (DuplicateTokenEx(
@@ -140,7 +171,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,
@@ -150,16 +181,14 @@ public bool RunAs(string processName, string args)
                             startupInfo: ref info,
                             processInformation: out procInfo))
                         {
-                            try
-                            {
-                                this.tracer.RelatedInfo("Started process '{0} {1}' with Id {2}", processName, args, procInfo.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
@@ -193,6 +222,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();
+        }
+
         /// <summary>
         /// Returns session IDs for sessions that have a logged-in user
         /// whose token can be queried via <c>WTSQueryUserToken</c>.