diff --git a/GVFS/GVFS.Common/GVFSPlatform.cs b/GVFS/GVFS.Common/GVFSPlatform.cs index 5b232628aa..964b8d6acd 100644 --- a/GVFS/GVFS.Common/GVFSPlatform.cs +++ b/GVFS/GVFS.Common/GVFSPlatform.cs @@ -65,6 +65,20 @@ public static void Register(GVFSPlatform platform) /// public abstract void PrepareProcessToRunInBackground(); + /// + /// Returns true if the current process owns a visible console window that a child process + /// (e.g. git launching the credential manager) can use as the owner for an interactive prompt. + /// + /// + /// This inspects the current process's console window and its root owner, not arbitrary GUI + /// windows. Background processes such as GVFS.Mount run with a hidden console, so this returns + /// false for them; foreground processes launched from a visible console return true. Credential + /// helpers launched by a process with no visible console window should be started detached from + /// the console, so the credential manager parents its prompt to a topmost stub window instead + /// of the hidden console. + /// + public abstract bool CurrentProcessHasVisibleConsoleWindow(); + public abstract bool IsProcessActive(int processId); /// diff --git a/GVFS/GVFS.Common/Git/GitProcess.cs b/GVFS/GVFS.Common/Git/GitProcess.cs index 43a40ced02..51997a1dea 100644 --- a/GVFS/GVFS.Common/Git/GitProcess.cs +++ b/GVFS/GVFS.Common/Git/GitProcess.cs @@ -273,18 +273,27 @@ public virtual bool TryGetCertificatePassword( using (ITracer activity = tracer.StartActivity("TryGetCertificatePassword", EventLevel.Informational)) { + // Record whether the current process has a visible console window, using the same value + // GetGitProcess uses to decide whether to detach git from a hidden console. Capturing it + // once here keeps the logged value authoritative and diagnosable on both the success and + // failure paths when a credential prompt fails to appear. + bool hasVisibleConsoleWindow = CurrentProcessHasVisibleConsoleWindowSafe(); + // See GetFromConfig for why pre-command hook is disabled // for bootstrap-time git operations. Result gitCredentialOutput = this.InvokeGitAgainstDotGitFolder( "credential fill", stdin => stdin.Write("protocol=cert\npath=" + certificatePath + "\nusername=\n\n"), parseStdOutLine: null, - usePreCommandHook: false); + usePreCommandHook: false, + mayRequireAuth: true, + hasVisibleConsoleWindow: hasVisibleConsoleWindow); if (gitCredentialOutput.ExitCodeIsFailure) { EventMetadata errorData = new EventMetadata(); errorData.Add("CertificatePath", certificatePath); + errorData.Add("hasVisibleConsoleWindow", hasVisibleConsoleWindow); tracer.RelatedWarning( errorData, "Git could not get credentials: " + gitCredentialOutput.Errors, @@ -301,7 +310,8 @@ public virtual bool TryGetCertificatePassword( EventMetadata metadata = new EventMetadata { { "Success", success }, - { "CertificatePath", certificatePath } + { "CertificatePath", certificatePath }, + { "hasVisibleConsoleWindow", hasVisibleConsoleWindow } }; if (!success) @@ -328,6 +338,12 @@ public virtual bool TryGetCredential( using (ITracer activity = tracer.StartActivity(nameof(this.TryGetCredential), EventLevel.Informational)) { + // Record whether the current process has a visible console window, using the same value + // GetGitProcess uses to decide whether to detach git from a hidden console. Capturing it + // once here keeps the logged value authoritative and diagnosable on both the success and + // failure paths (including the timeout that a hidden, unanswered prompt produces). + bool hasVisibleConsoleWindow = CurrentProcessHasVisibleConsoleWindowSafe(); + // See GetFromConfig for why pre-command hook is disabled // for bootstrap-time git operations. Result gitCredentialOutput = this.InvokeGitAgainstDotGitFolder( @@ -335,11 +351,14 @@ public virtual bool TryGetCredential( stdin => stdin.Write($"url={repoUrl}\n\n"), parseStdOutLine: null, usePreCommandHook: false, - timeoutMs: timeoutMs); + timeoutMs: timeoutMs, + mayRequireAuth: true, + hasVisibleConsoleWindow: hasVisibleConsoleWindow); if (gitCredentialOutput.ExitCodeIsFailure) { EventMetadata errorData = new EventMetadata(); + errorData.Add("hasVisibleConsoleWindow", hasVisibleConsoleWindow); if (gitCredentialOutput.Errors.StartsWith("Operation timed out")) { @@ -361,6 +380,14 @@ public virtual bool TryGetCredential( return false; } + // Record whether the current process had a visible console window. GetGitProcess uses + // this to decide whether to detach git from a hidden console so the credential prompt is + // not left behind other windows; logging it keeps that decision diagnosable from telemetry + // when a prompt fails to appear. + EventMetadata consoleMetadata = new EventMetadata(); + consoleMetadata.Add("hasVisibleConsoleWindow", hasVisibleConsoleWindow); + activity.RelatedInfo(consoleMetadata, "Credential fill console window visibility"); + username = ParseValue(gitCredentialOutput.Output, "username="); password = ParseValue(gitCredentialOutput.Output, "password="); @@ -902,7 +929,7 @@ public Result MultiPackIndexRepack(string gitObjectDirectory, string batchSize) return this.InvokeGitAgainstDotGitFolder($"-c pack.threads=1 -c repack.packKeptObjects=true multi-pack-index repack --object-dir=\"{gitObjectDirectory}\" --batch-size={batchSize} --no-progress"); } - public Process GetGitProcess(string command, string workingDirectory, string dotGitDirectory, bool useReadObjectHook, string gitObjectsDirectory, bool usePreCommandHook) + public virtual Process GetGitProcess(string command, string workingDirectory, string dotGitDirectory, bool useReadObjectHook, string gitObjectsDirectory, bool usePreCommandHook, bool mayRequireAuth = false, bool? hasVisibleConsoleWindow = null) { ProcessStartInfo processInfo = new ProcessStartInfo(this.gitBinPath); processInfo.WorkingDirectory = workingDirectory; @@ -919,7 +946,16 @@ public Process GetGitProcess(string command, string workingDirectory, string dot // instead), CreateNoWindow must be set to true for that case — otherwise // the non-redirected stream inherits the parent's console handle, which // may be absent when running as a service, causing lost output. - processInfo.CreateNoWindow = false; + // + // When a command may require authentication, git can launch the credential + // manager. Detaching from a hidden console (CreateNoWindow=true) makes that + // prompt appear on top instead of behind other windows. See + // GVFSPlatform.CurrentProcessHasVisibleConsoleWindow for the full rationale. + // A caller that already probed the console visibility passes the result in + // hasVisibleConsoleWindow so the logged and acted-on value stay identical; + // otherwise probe here. The && short-circuits the probe when auth cannot + // be required. + processInfo.CreateNoWindow = mayRequireAuth && !(hasVisibleConsoleWindow ?? CurrentProcessHasVisibleConsoleWindowSafe()); processInfo.StandardOutputEncoding = UTF8NoBOM; processInfo.StandardErrorEncoding = UTF8NoBOM; @@ -986,7 +1022,9 @@ protected virtual Result InvokeGitImpl( Action parseStdOutLine, int timeoutMs, string gitObjectsDirectory = null, - bool usePreCommandHook = true) + bool usePreCommandHook = true, + bool mayRequireAuth = false, + bool? hasVisibleConsoleWindow = null) { if (failedToSetEncoding && writeStdIn != null) { @@ -998,7 +1036,14 @@ protected virtual Result InvokeGitImpl( // From https://msdn.microsoft.com/en-us/library/system.diagnostics.process.standardoutput.aspx // To avoid deadlocks, use asynchronous read operations on at least one of the streams. // Do not perform a synchronous read to the end of both redirected streams. - using (this.executingProcess = this.GetGitProcess(command, workingDirectory, dotGitDirectory, useReadObjectHook, gitObjectsDirectory: gitObjectsDirectory, usePreCommandHook: usePreCommandHook)) + // + // Keep the Process in a local so the using block disposes exactly the instance this + // invocation created. The shared this.executingProcess field (read by + // TryKillRunningProcess) is published and cleared only under processLock, and only while + // this invocation holds executionLock. That single lock protocol prevents a concurrent + // invocation on the same instance from clobbering the field and prevents TryKillRunningProcess + // from observing a stale, not-yet-started, or already-disposed process. + using (Process process = this.GetGitProcess(command, workingDirectory, dotGitDirectory, useReadObjectHook, gitObjectsDirectory: gitObjectsDirectory, usePreCommandHook: usePreCommandHook, mayRequireAuth: mayRequireAuth, hasVisibleConsoleWindow: hasVisibleConsoleWindow)) { // Bound how much stdout/stderr we buffer so a pathologically noisy git command // cannot grow these buffers without limit until GVFS.Mount hits an @@ -1008,14 +1053,14 @@ protected virtual Result InvokeGitImpl( BoundedGitOutputBuffer output = new BoundedGitOutputBuffer(MaxCapturedStdOutChars); BoundedGitOutputBuffer errors = new BoundedGitOutputBuffer(MaxCapturedStdErrChars); - this.executingProcess.ErrorDataReceived += (sender, args) => + process.ErrorDataReceived += (sender, args) => { if (args.Data != null) { errors.AppendLine(args.Data); } }; - this.executingProcess.OutputDataReceived += (sender, args) => + process.OutputDataReceived += (sender, args) => { if (args.Data != null) { @@ -1032,53 +1077,60 @@ protected virtual Result InvokeGitImpl( lock (this.executionLock) { - lock (this.processLock) + try { - if (this.stopping) - { - return new Result(string.Empty, nameof(GitProcess) + " is stopping", Result.GenericFailureCode); - } - - this.executingProcess.Start(); - - if (this.LowerPriority) + lock (this.processLock) { - try + if (this.stopping) { - this.executingProcess.PriorityClass = ProcessPriorityClass.BelowNormal; + return new Result(string.Empty, nameof(GitProcess) + " is stopping", Result.GenericFailureCode); } - catch (InvalidOperationException) + + this.executingProcess = process; + process.Start(); + + if (this.LowerPriority) { - // This is thrown if the process completes before we can set its priority. + try + { + process.PriorityClass = ProcessPriorityClass.BelowNormal; + } + catch (InvalidOperationException) + { + // This is thrown if the process completes before we can set its priority. + } } } - } - writeStdIn?.Invoke(this.executingProcess.StandardInput); - this.executingProcess.StandardInput.Close(); + writeStdIn?.Invoke(process.StandardInput); + process.StandardInput.Close(); - this.executingProcess.BeginOutputReadLine(); - this.executingProcess.BeginErrorReadLine(); + process.BeginOutputReadLine(); + process.BeginErrorReadLine(); - if (!this.executingProcess.WaitForExit(timeoutMs)) - { - this.executingProcess.Kill(); + if (!process.WaitForExit(timeoutMs)) + { + process.Kill(); + + return new Result(output.ToString(), "Operation timed out: " + errors.ToString(), Result.GenericFailureCode, output.Truncated, errors.Truncated); + } - return new Result(output.ToString(), "Operation timed out: " + errors.ToString(), Result.GenericFailureCode, output.Truncated, errors.Truncated); + return new Result(output.ToString(), errors.ToString(), process.ExitCode, output.Truncated, errors.Truncated); + } + finally + { + lock (this.processLock) + { + this.executingProcess = null; + } } } - - return new Result(output.ToString(), errors.ToString(), this.executingProcess.ExitCode, output.Truncated, errors.Truncated); } } catch (Win32Exception e) { return new Result(string.Empty, e.Message, Result.GenericFailureCode); } - finally - { - this.executingProcess = null; - } } private static string GenerateCredentialVerbCommand(string verb) @@ -1086,6 +1138,20 @@ private static string GenerateCredentialVerbCommand(string verb) return $"-c {GitConfigSetting.CredentialUseHttpPath}=true credential {verb}"; } + private static bool CurrentProcessHasVisibleConsoleWindowSafe() + { + try + { + return GVFSPlatform.Instance.CurrentProcessHasVisibleConsoleWindow(); + } + catch (Exception ex) when (ex is DllNotFoundException || ex is EntryPointNotFoundException) + { + // If the native console probe cannot run, assume a visible console so callers keep + // the previous behavior (CreateNoWindow=false) rather than aborting the command. + return true; + } + } + private static string ParseValue(string contents, string prefix) { int startIndex = contents.IndexOf(prefix) + prefix.Length; @@ -1166,7 +1232,9 @@ private Result InvokeGitAgainstDotGitFolder( Action parseStdOutLine, bool usePreCommandHook = true, string gitObjectsDirectory = null, - int timeoutMs = -1) + int timeoutMs = -1, + bool mayRequireAuth = false, + bool? hasVisibleConsoleWindow = null) { // This git command should not need/use the working directory of the repo. // Run git.exe in Environment.SystemDirectory to ensure the git.exe process @@ -1180,7 +1248,9 @@ private Result InvokeGitAgainstDotGitFolder( parseStdOutLine: parseStdOutLine, timeoutMs: timeoutMs, gitObjectsDirectory: gitObjectsDirectory, - usePreCommandHook: usePreCommandHook); + usePreCommandHook: usePreCommandHook, + mayRequireAuth: mayRequireAuth, + hasVisibleConsoleWindow: hasVisibleConsoleWindow); } public class Result diff --git a/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs b/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs index 91da42f767..a27c980834 100644 --- a/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs +++ b/GVFS/GVFS.Platform.Windows/WindowsPlatform.cs @@ -11,6 +11,7 @@ using System.IO; using System.IO.Pipes; using System.Linq; +using System.Runtime.InteropServices; using System.Security.AccessControl; using System.Security.Principal; using System.ServiceProcess; @@ -28,6 +29,9 @@ public partial class WindowsPlatform : GVFSPlatform private const string ProfileImagePathRegistryValue = "ProfileImagePath"; private const string AdminProtectionProfilePrefix = "ADMIN_"; + // GetAncestor flag GA_ROOTOWNER (3): walk the owner chain to the root owned window. + private const uint GA_ROOTOWNER = 3; + public WindowsPlatform() : base(underConstruction: new UnderConstructionFlags()) { } @@ -224,6 +228,22 @@ public override void PrepareProcessToRunInBackground() // No additional work required } + public override bool CurrentProcessHasVisibleConsoleWindow() + { + IntPtr consoleWindow = GetConsoleWindow(); + if (consoleWindow == IntPtr.Zero) + { + // The process has no console at all (e.g. a GUI process), so there is no console + // window to own an interactive prompt. + return false; + } + + // GVFS.Mount runs with a hidden console, so its console window exists but its root + // owner is not visible. Only treat the window as usable when its root owner is visible. + IntPtr rootOwner = GetAncestor(consoleWindow, GA_ROOTOWNER); + return rootOwner != IntPtr.Zero && IsWindowVisible(rootOwner); + } + public override NamedPipeServerStream CreatePipeByName(string pipeName) { PipeSecurity security = new PipeSecurity(); @@ -458,6 +478,16 @@ private static object GetValueFromRegistry(RegistryHive registryHive, string key return value; } + [DllImport("kernel32.dll")] + private static extern IntPtr GetConsoleWindow(); + + [DllImport("user32.dll")] + private static extern IntPtr GetAncestor(IntPtr hwnd, uint gaFlags); + + [DllImport("user32.dll")] + [return: MarshalAs(UnmanagedType.Bool)] + private static extern bool IsWindowVisible(IntPtr hWnd); + public class WindowsPlatformConstants : GVFSPlatformConstants { public override string ExecutableExtension diff --git a/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs b/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs index 25aa675b7e..682a80537f 100644 --- a/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs +++ b/GVFS/GVFS.UnitTests/Git/GitAuthenticationTests.cs @@ -1,7 +1,9 @@ using System; +using System.Diagnostics; using System.Linq; using System.Threading; using System.Threading.Tasks; +using GVFS.Common; using GVFS.Common.Git; using GVFS.Tests; using GVFS.Tests.Should; @@ -342,6 +344,201 @@ public void TryGetCredentialsWaitsForBackgroundInitializationThenSucceeds() authString.ShouldNotBeNull("A credential string should be returned"); } + [TestCase] + public void CredentialFillIsFlaggedAsMayRequireAuth() + { + // Only credential fill can trigger an interactive sign-in from the credential manager, so + // only it is flagged mayRequireAuth=true. GetGitProcess uses that flag to detach git from a + // hidden console so the prompt is not left behind other windows. + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + dut.TryInitializeAndRequireAuth(tracer, out _); + + dut.TryGetCredentials(tracer, out _, out string error).ShouldEqual(true, "Failed to get credential: " + error); + + gitProcess.MayRequireAuthFor($"{AzureDevOpsUseHttpPathString} credential fill") + .ShouldEqual(true, "Credential fill should be flagged as possibly requiring auth"); + + // Non-credential git commands never prompt, so they must not be flagged. + gitProcess.MayRequireAuthFor("config --get-urlmatch http mock://repoUrl") + .ShouldEqual(false, "Non-credential git commands should not be flagged as requiring auth"); + } + + [TestCase] + public void CredentialApproveIsNotFlaggedAsMayRequireAuth() + { + // Credential approve stores an already-obtained credential and never prompts. + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + dut.TryInitializeAndRequireAuth(tracer, out _); + + dut.TryGetCredentials(tracer, out string authString, out string error).ShouldEqual(true, "Failed to get credential: " + error); + dut.ApproveCredentials(tracer, authString); + + gitProcess.MayRequireAuthFor($"{AzureDevOpsUseHttpPathString} credential approve") + .ShouldEqual(false, "Credential approve should not be flagged as requiring auth"); + } + + [TestCase] + public void CredentialRejectIsNotFlaggedAsMayRequireAuth() + { + // Credential reject erases a stored credential and never prompts. + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = this.GetGitProcess(); + GitAuthentication dut = new GitAuthentication(gitProcess, "mock://repoUrl"); + dut.TryInitializeAndRequireAuth(tracer, out _); + + dut.TryGetCredentials(tracer, out string authString, out string error).ShouldEqual(true, "Failed to get credential: " + error); + dut.RejectCredentials(tracer, authString); + + gitProcess.MayRequireAuthFor($"{AzureDevOpsUseHttpPathString} credential reject") + .ShouldEqual(false, "Credential reject should not be flagged as requiring auth"); + } + + [TestCase] + public void CertificateCredentialFillIsFlaggedAsMayRequireAuth() + { + // The certificate credential fill can also trigger an interactive sign-in, so it is flagged + // mayRequireAuth=true like the URL fill. It is covered directly here because the standard + // credential flow does not exercise the certificate path. + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = new MockGitProcess(); + gitProcess.SetExpectedCommandResult( + "credential fill", + () => new GitProcess.Result("password=certpassword\r\n", string.Empty, GitProcess.Result.SuccessCode)); + + gitProcess.TryGetCertificatePassword(tracer, CertificatePath, out string password, out string error) + .ShouldEqual(true, "Failed to get certificate password: " + error); + + gitProcess.MayRequireAuthFor("credential fill") + .ShouldEqual(true, "Certificate credential fill should be flagged as possibly requiring auth"); + } + + [TestCase] + public void GetGitProcessDetachesFromConsoleOnlyWhenAuthMayBeRequiredAndNoVisibleWindow() + { + MockPlatform platform = (MockPlatform)GVFSPlatform.Instance; + bool originalHasVisibleWindow = platform.HasVisibleWindow; + try + { + GitProcess gitProcess = new GitProcess("git.exe", workingDirectoryRoot: null); + + // No visible console + command may require auth => detach so the prompt is topmost. + platform.HasVisibleWindow = false; + using (Process process = gitProcess.GetGitProcess("credential fill", Environment.SystemDirectory, dotGitDirectory: null, useReadObjectHook: false, gitObjectsDirectory: null, usePreCommandHook: false, mayRequireAuth: true)) + { + process.StartInfo.CreateNoWindow.ShouldEqual(true, "Should detach when auth may be required and there is no visible console window"); + } + + // Visible console + command may require auth => keep the console so the prompt parents to it. + platform.HasVisibleWindow = true; + using (Process process = gitProcess.GetGitProcess("credential fill", Environment.SystemDirectory, dotGitDirectory: null, useReadObjectHook: false, gitObjectsDirectory: null, usePreCommandHook: false, mayRequireAuth: true)) + { + process.StartInfo.CreateNoWindow.ShouldEqual(false, "Should keep the console when it is visible"); + } + + // Command cannot require auth => never detach, regardless of console visibility. + platform.HasVisibleWindow = false; + using (Process process = gitProcess.GetGitProcess("rev-parse HEAD", Environment.SystemDirectory, dotGitDirectory: null, useReadObjectHook: false, gitObjectsDirectory: null, usePreCommandHook: true, mayRequireAuth: false)) + { + process.StartInfo.CreateNoWindow.ShouldEqual(false, "Should never detach when auth cannot be required"); + } + + // Native console probe cannot run => safe fallback assumes a visible console, so do not detach. + platform.HasVisibleWindow = false; + platform.ThrowOnConsoleProbe = true; + using (Process process = gitProcess.GetGitProcess("credential fill", Environment.SystemDirectory, dotGitDirectory: null, useReadObjectHook: false, gitObjectsDirectory: null, usePreCommandHook: false, mayRequireAuth: true)) + { + process.StartInfo.CreateNoWindow.ShouldEqual(false, "Should not detach when the console probe cannot run"); + } + + // A caller-supplied console-visibility value is authoritative and bypasses the platform probe + // (the probe would throw if consulted here). + platform.ThrowOnConsoleProbe = true; + platform.HasVisibleWindow = true; + using (Process process = gitProcess.GetGitProcess("credential fill", Environment.SystemDirectory, dotGitDirectory: null, useReadObjectHook: false, gitObjectsDirectory: null, usePreCommandHook: false, mayRequireAuth: true, hasVisibleConsoleWindow: false)) + { + process.StartInfo.CreateNoWindow.ShouldEqual(true, "A caller-supplied false should force detach without probing the platform"); + } + } + finally + { + platform.HasVisibleWindow = originalHasVisibleWindow; + platform.ThrowOnConsoleProbe = false; + } + } + + [TestCase] + public void TryGetCredentialLogsConsoleVisibilityWhenCredentialFillFails() + { + // The console-visibility telemetry must be recorded on the failure path too, because the + // hidden-prompt timeout that this diagnostic exists to explain is itself a failure result. + MockPlatform platform = (MockPlatform)GVFSPlatform.Instance; + bool originalHasVisibleWindow = platform.HasVisibleWindow; + try + { + platform.HasVisibleWindow = false; + + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = new MockGitProcess(); + gitProcess.SetExpectedCommandResult( + $"{AzureDevOpsUseHttpPathString} credential fill", + () => new GitProcess.Result(string.Empty, "Operation timed out waiting for a response", GitProcess.Result.GenericFailureCode)); + + gitProcess.TryGetCredential(tracer, "mock://repoUrl", out string username, out string password, out string error, timeoutMs: 5000) + .ShouldEqual(false, "Credential fill should report failure when git times out"); + + // Assert the logged value, not just the key: the diagnostic is only useful if it records + // the actual console-visibility state (false here) that explains the hidden-prompt timeout. + tracer.RelatedWarningEvents.ShouldContain(e => e.Contains("\"hasVisibleConsoleWindow\":false")); + + // The probed value must thread all the way down to the git invocation that GetGitProcess uses. + gitProcess.InvocationsRun + .Single(invocation => invocation.Command == $"{AzureDevOpsUseHttpPathString} credential fill") + .HasVisibleConsoleWindow.ShouldEqual(false, "The probed console-visibility value must thread down to the git invocation"); + } + finally + { + platform.HasVisibleWindow = originalHasVisibleWindow; + } + } + + [TestCase] + public void TryGetCertificatePasswordLogsConsoleVisibilityWhenCredentialFillFails() + { + // The certificate credential fill stamps the same console-visibility telemetry as the URL fill. + // It is covered separately because the standard credential flow does not exercise the certificate + // path, and the value must be present on the failure path where a hidden prompt times out. + MockPlatform platform = (MockPlatform)GVFSPlatform.Instance; + bool originalHasVisibleWindow = platform.HasVisibleWindow; + try + { + platform.HasVisibleWindow = false; + + MockTracer tracer = new MockTracer(); + MockGitProcess gitProcess = new MockGitProcess(); + gitProcess.SetExpectedCommandResult( + "credential fill", + () => new GitProcess.Result(string.Empty, "Operation timed out waiting for a response", GitProcess.Result.GenericFailureCode)); + + gitProcess.TryGetCertificatePassword(tracer, CertificatePath, out string password, out string error) + .ShouldEqual(false, "Certificate password fill should report failure when git times out"); + + tracer.RelatedWarningEvents.ShouldContain(e => e.Contains("\"hasVisibleConsoleWindow\":false")); + + // The probed value must thread all the way down to the git invocation that GetGitProcess uses. + gitProcess.InvocationsRun + .Single(invocation => invocation.Command == "credential fill") + .HasVisibleConsoleWindow.ShouldEqual(false, "The probed console-visibility value must thread down to the git invocation"); + } + finally + { + platform.HasVisibleWindow = originalHasVisibleWindow; + } + } + private MockGitProcess GetGitProcess() { MockGitProcess gitProcess = new MockGitProcess(); diff --git a/GVFS/GVFS.UnitTests/Git/GitProcessTests.cs b/GVFS/GVFS.UnitTests/Git/GitProcessTests.cs index 8182f8dfb5..5edab24a51 100644 --- a/GVFS/GVFS.UnitTests/Git/GitProcessTests.cs +++ b/GVFS/GVFS.UnitTests/Git/GitProcessTests.cs @@ -2,7 +2,9 @@ using GVFS.Tests.Should; using GVFS.UnitTests.Mock.Common; using NUnit.Framework; +using System; using System.Diagnostics; +using System.Threading; namespace GVFS.UnitTests.Git { @@ -102,6 +104,97 @@ public void TryKillRunningProcess_NeverRan() error.ShouldBeNull(); } + [TestCase] + [Description("Regression: a concurrent invocation on the same GitProcess must not clobber the " + + "executingProcess field that TryKillRunningProcess reads. The field is published and cleared " + + "only under processLock while the invocation holds executionLock.")] + public void ConcurrentInvocation_DoesNotClobberExecutingProcessField() + { + // Two InvokeGitImpl calls race on one instance. Invocation A starts its process and holds + // executionLock. Invocation B queues behind A on executionLock. Before the fix, B published its + // own (not-yet-started) process into the shared executingProcess field outside any lock, so a + // concurrent TryKillRunningProcess read B's process instead of the running one (A). The fix makes + // the field obey a single processLock protocol, so the field still holds A while B waits. + FieldProtocolGitProcess gitProcess = new FieldProtocolGitProcess(); + + Thread threadA = new Thread(() => gitProcess.Invoke()) { IsBackground = true }; + Thread threadB = new Thread(() => gitProcess.Invoke()) { IsBackground = true }; + + try + { + // Start A and wait until its real process is running (its field value is published). + threadA.Start(); + SpinWait.SpinUntil( + () => IsStarted(gitProcess.FirstProcess), + TimeSpan.FromSeconds(20)).ShouldBeTrue("Invocation A never started its process"); + + // Start B. It blocks inside GetGitProcess until released, confirming A is running first. + threadB.Start(); + gitProcess.SecondCallReached.Wait(TimeSpan.FromSeconds(20)) + .ShouldBeTrue("Invocation B never reached GetGitProcess"); + gitProcess.ReleaseSecondCall(); + + // Wait until B has queued on executionLock. By then the buggy lockless field write has run; + // the fixed code has left the field untouched because B does not hold executionLock. + SpinWait.SpinUntil( + () => threadB.ThreadState.HasFlag(System.Threading.ThreadState.WaitSleepJoin), + TimeSpan.FromSeconds(20)).ShouldBeTrue("Invocation B never queued on the execution lock"); + Thread.Sleep(100); + + // Only A's process is running. With the fix the field holds A, so TryKillRunningProcess + // reports A's process name. Without the fix the field holds B's not-yet-started process, and + // reading its ProcessName throws here. + bool killed = gitProcess.TryKillRunningProcess(out string processName, out int _, out string _); + + killed.ShouldBeTrue(); + processName.ShouldNotBeNull(); + processName.ToUpperInvariant().Contains("PING") + .ShouldBeTrue("Expected the running process (A), but the field held: " + processName); + } + finally + { + KillQuietly(gitProcess.FirstProcess); + threadA.Join(TimeSpan.FromSeconds(10)); + threadB.Join(TimeSpan.FromSeconds(10)); + KillQuietly(gitProcess.SecondProcess); + } + } + + private static bool IsStarted(Process process) + { + if (process == null) + { + return false; + } + + try + { + return process.Id != 0; + } + catch (InvalidOperationException) + { + // Thrown while the process has not been started yet. + return false; + } + } + + private static void KillQuietly(Process process) + { + if (process == null) + { + return; + } + + try + { + process.Kill(); + } + catch (Exception) + { + // The process was never started, already exited, or is already disposed. + } + } + [TestCase] public void ResultHasNoErrors() { @@ -402,5 +495,77 @@ public void QuoteGitPath_RoundTripThroughProcess() } } } + + /// + /// Test harness that drives the real but replaces the git + /// child process with a controllable, long-running OS process so the executingProcess field protocol + /// can be exercised deterministically. The first invocation gets a running process; the second blocks + /// inside until released so the test can order the two invocations. + /// + private sealed class FieldProtocolGitProcess : GitProcess + { + private readonly ManualResetEventSlim secondCallGate = new ManualResetEventSlim(false); + private int callCount; + + public FieldProtocolGitProcess() + : base("git", "mock:\\root") + { + } + + public Process FirstProcess { get; private set; } + + public Process SecondProcess { get; private set; } + + public ManualResetEventSlim SecondCallReached { get; } = new ManualResetEventSlim(false); + + public void ReleaseSecondCall() + { + this.secondCallGate.Set(); + } + + public override Process GetGitProcess(string command, string workingDirectory, string dotGitDirectory, bool useReadObjectHook, string gitObjectsDirectory, bool usePreCommandHook, bool mayRequireAuth = false, bool? hasVisibleConsoleWindow = null) + { + int call = Interlocked.Increment(ref this.callCount); + if (call == 1) + { + this.FirstProcess = CreateSleeperProcess(); + return this.FirstProcess; + } + + this.SecondProcess = CreateSleeperProcess(); + this.SecondCallReached.Set(); + this.secondCallGate.Wait(); + return this.SecondProcess; + } + + public Result Invoke() + { + return this.InvokeGitImpl( + "status", + "mock:\\root", + dotGitDirectory: null, + useReadObjectHook: false, + writeStdIn: null, + parseStdOutLine: null, + timeoutMs: 60000); + } + + private static Process CreateSleeperProcess() + { + // A benign, always-available Windows process that runs long enough for the test to observe + // it while it is alive. Its streams are redirected because InvokeGitImpl closes stdin and + // begins async reads on stdout/stderr. + ProcessStartInfo startInfo = new ProcessStartInfo("ping.exe", "-n 30 127.0.0.1") + { + UseShellExecute = false, + RedirectStandardInput = true, + RedirectStandardOutput = true, + RedirectStandardError = true, + CreateNoWindow = true, + }; + + return new Process { StartInfo = startInfo }; + } + } } } diff --git a/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs b/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs index 3ee7425de0..a5c92b0c9f 100644 --- a/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs +++ b/GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs @@ -21,6 +21,18 @@ public class MockPlatform : GVFSPlatform public string MockCurrentUser { get; set; } + /// + /// Controls the return value of . Defaults to true + /// so tests keep the foreground behavior unless they opt into the background-process case. + /// + public bool HasVisibleWindow { get; set; } = true; + + /// + /// When true, throws to simulate a native + /// console probe that cannot run (missing entry point), so tests can cover the safe fallback. + /// + public bool ThrowOnConsoleProbe { get; set; } + public override IKernelDriver KernelDriver => throw new NotSupportedException(); public override IGitInstallation GitInstallation { get; } = new MockGitInstallation(); @@ -200,6 +212,16 @@ public override void PrepareProcessToRunInBackground() throw new NotSupportedException(); } + public override bool CurrentProcessHasVisibleConsoleWindow() + { + if (this.ThrowOnConsoleProbe) + { + throw new EntryPointNotFoundException(); + } + + return this.HasVisibleWindow; + } + public override bool IsGitStatusCacheSupported() { return true; diff --git a/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs b/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs index c9095cc2c8..cf786a9835 100644 --- a/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs +++ b/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs @@ -18,17 +18,43 @@ public MockGitProcess() : base(new MockGVFSEnlistment()) { this.CommandsRun = new List(); + this.InvocationsRun = new List(); this.StoredCredentials = new Dictionary(StringComparer.OrdinalIgnoreCase); this.CredentialApprovals = new Dictionary>(); this.CredentialRejections = new Dictionary>(); } public List CommandsRun { get; } + + /// + /// Records every call into in order, with the mayRequireAuth and + /// hasVisibleConsoleWindow values passed to it. Recording each invocation (rather than keeping only + /// the last value per command) keeps the wiring assertions honest if a command is ever run more than + /// once with different flags. + /// + public List InvocationsRun { get; } public bool ShouldFail { get; set; } public Dictionary StoredCredentials { get; } public Dictionary> CredentialApprovals { get; } public Dictionary> CredentialRejections { get; } + /// + /// Returns the mayRequireAuth value the given command was invoked with, asserting the command ran and + /// that every invocation of it agreed on the value. + /// + public bool MayRequireAuthFor(string command) + { + List matches = this.InvocationsRun + .Where(invocation => string.Equals(invocation.Command, command, StringComparison.Ordinal)) + .ToList(); + + matches.Count.ShouldBeAtLeast(1, "Command was never run: " + command); + matches.Select(invocation => invocation.MayRequireAuth).Distinct().Count() + .ShouldEqual(1, "Command ran with conflicting mayRequireAuth values: " + command); + + return matches[0].MayRequireAuth; + } + public void SetExpectedCommandResult(string command, Func result, bool matchPrefix = false) { CommandInfo commandInfo = new CommandInfo(command, result, matchPrefix); @@ -84,9 +110,12 @@ protected override Result InvokeGitImpl( Action parseStdOutLine, int timeoutMs, string gitObjectsDirectory = null, - bool usePrecommandHook = true) + bool usePrecommandHook = true, + bool mayRequireAuth = false, + bool? hasVisibleConsoleWindow = null) { this.CommandsRun.Add(command); + this.InvocationsRun.Add(new GitInvocation(command, mayRequireAuth, hasVisibleConsoleWindow)); if (this.ShouldFail) { @@ -125,6 +154,20 @@ protected override Result InvokeGitImpl( return result; } + public class GitInvocation + { + public GitInvocation(string command, bool mayRequireAuth, bool? hasVisibleConsoleWindow) + { + this.Command = command; + this.MayRequireAuth = mayRequireAuth; + this.HasVisibleConsoleWindow = hasVisibleConsoleWindow; + } + + public string Command { get; } + public bool MayRequireAuth { get; } + public bool? HasVisibleConsoleWindow { get; } + } + public class Credential { public Credential(string username, string password)