diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index a2778fb5a1..32706260ff 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.55.0.vfs.0.8' }} + GIT_VERSION: ${{ github.event.inputs.git_version || 'v2.55.0.vfs.0.10' }} jobs: validate: diff --git a/.github/workflows/functional-tests.yaml b/.github/workflows/functional-tests.yaml index 7274f97656..110e3c14b8 100644 --- a/.github/workflows/functional-tests.yaml +++ b/.github/workflows/functional-tests.yaml @@ -209,7 +209,7 @@ jobs: - name: Upload failure diagnostics (mount dumps + logs) if: always() && steps.skip.outputs.result != 'true' - uses: actions/upload-artifact@v7 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 continue-on-error: true with: name: ${{ env.ARTIFACT_PREFIX }}FailureDiagnostics_${{ env.FT_MATRIX_NAME }} diff --git a/AGENTS.md b/AGENTS.md index 1a8a42a47e..ccb7412a4d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -67,19 +67,27 @@ an installer. `PublishAot=false` skips ilc (~3–4 min saved); `GVFS.Payload` only assembles the payload directory — it does **not** build or publish the projects it copies from (see the warning below). -> **Prerequisite: the native C++ projects must already be built.** They are -> `.vcxproj` (see [Native C++ projects](#native-c-projects-need-msbuild-not-dotnet-build) +> **Prerequisite: the native C++ projects and all managed payload projects must +> already have built outputs.** The native projects are `.vcxproj` (see +> [Native C++ projects](#native-c-projects-need-msbuild-not-dotnet-build) > below) and `dotnet publish` will not build them for you. If you have not > already done a `Build.bat` once in this enlistment, build the native -> projects via VS MSBuild first (or run `Build.bat` once to populate `out\`, -> then iterate with the commands below). After that they are incremental and -> only rebuild when their own sources change. +> projects via VS MSBuild and publish the managed payload projects below +> (or run `Build.bat` once to populate `out\`, then iterate with the commands +> below). After that they are incremental and only rebuild when their own +> sources change. ```powershell dotnet publish src\GVFS\GVFS.FunctionalTests\GVFS.FunctionalTests.csproj ` -c Debug /p:PublishAot=false dotnet publish src\GVFS\GVFS\GVFS.csproj ` -c Debug /p:PublishAot=false +dotnet publish src\GVFS\GVFS.Hooks\GVFS.Hooks.csproj ` + -c Debug /p:PublishAot=false +dotnet publish src\GVFS\GVFS.Mount\GVFS.Mount.csproj ` + -c Debug /p:PublishAot=false +dotnet publish src\GVFS\GVFS.Service\GVFS.Service.csproj ` + -c Debug /p:PublishAot=false dotnet publish src\GVFS\GVFS.Payload\GVFS.Payload.csproj ` -c Debug /p:PublishAot=false /p:SkipCreateInstaller=true @@ -113,11 +121,19 @@ hook binaries are copied straight from the vcxproj output. > your edit, publish the project that owns the changed code and re-run the > payload publish. The same applies to `GVFS.Mount`, `GVFS.Hooks`, and > `GVFS.Service` when you change those. +> +> Publish all four managed payload projects at least once in a new enlistment. `RunFunctionalTests-Dev.ps1` runs functional tests against the build output without requiring admin or a system-wide install. It launches the test -service as a console process. Each invocation gets a unique service name -and data dir, so concurrent runs from different worktrees don't collide. +service as a console process. Each invocation gets a unique service name and +service-data directory. Some fixtures still use shared machine paths or drive +mappings, so do not assume that separate test processes can run concurrently. + +The script sets dev-mode variables. `Settings.Default.Initialize` then resolves +`gvfs.exe` and `GVFS.Service.exe` from `out\GVFS.Payload\...`, not +`C:\Program Files\VFS for Git\`. This path does not require a GVFS +installation. > **Pass `-Configuration` and `-Arch` by name.** The script's first two > positional parameters are `Configuration` and `Arch`, so a bare @@ -166,12 +182,14 @@ participate in the C# inner-loop paths above. ```powershell # ✅ Correct -& "out\GVFS.UnitTests\bin\...\GVFS.UnitTests.exe" --test "GVFS.UnitTests.Common.WorktreeInfoTests" -src\scripts\RunFunctionalTests-Dev.ps1 -Configuration Debug -Arch x64 --test=GVFS.FunctionalTests.Tests.GVFSVerbTests.UnknownVerb +& "out\GVFS.UnitTests\bin\...\GVFS.UnitTests.exe" --test "GVFS.UnitTests.Common.WorktreeInfoTests" +src\scripts\RunFunctionalTests-Dev.ps1 -Configuration Debug -Arch x64 ` + --test=GVFS.FunctionalTests.Tests.GVFSVerbTests.UnknownVerb # ❌ Wrong — silently runs the entire suite -& "out\GVFS.UnitTests\bin\...\GVFS.UnitTests.exe" --where "class =~ Worktree" -src\scripts\RunFunctionalTests-Dev.ps1 -Configuration Debug -Arch x64 --where "cat == Smoke" +& "out\GVFS.UnitTests\bin\...\GVFS.UnitTests.exe" --where "class =~ Worktree" +src\scripts\RunFunctionalTests-Dev.ps1 -Configuration Debug -Arch x64 ` + --where "cat == Smoke" ``` For unit tests, `--where` is merely annoying (the whole suite runs in @@ -256,6 +274,43 @@ Default new gates to `false` and gate the **runtime entry point** into a feature, not its build, so the code still compiles and ships (and keeps getting exercised) while its behavior stays off by default. +## libgit2 P/Invoke string marshalling (UTF-8, not ANSI) + +The libgit2 bindings live in `GVFS.Common/Git/LibGit2Repo.cs` under the +`Native` class. **libgit2 treats every string it receives and returns as +UTF-8** — paths, revspecs, config keys and values, and error messages. + +The .NET default for a bare `[DllImport]` string parameter is +`CharSet.Ansi`, which encodes through the Windows ANSI code page and +**silently corrupts non-ASCII input** (a repo path under a non-English user +name, a non-ASCII branch name, etc.) — an unmappable character becomes `?`, +so `git_repository_open` and friends resolve the wrong path or fail. There is +no `[module: DefaultCharSet]` override in `GVFS.Common`, so the ANSI default +applies unless each declaration opts out. + +When you add a libgit2 P/Invoke: + +- Annotate **every** `string` parameter, `out string` parameter, string return + value, and string struct field with + `[MarshalAs(UnmanagedType.LPUTF8Str)]`. +- Do **not** use `CharSet.Unicode` — that marshals UTF-16, which libgit2 does + not accept (it is the same bug in the other direction). +- For a function that returns a **borrowed** pointer owned by libgit2 (e.g. + `git_config_get_string`), marshal it as `IntPtr` and copy with + `Marshal.PtrToStringUTF8(...)`; do not marshal it as `out string`, or the + interop marshaller frees libgit2's heap memory with the wrong allocator. + +Because the mismatch only manifests through the native call, cover new +string-carrying paths with a real-libgit2 test that uses a non-ASCII input +(see `GVFS.FunctionalTests/Tests/LibGit2NonAsciiPathTests.cs`), not a +mock-based unit test. + +## Public repository hygiene + +Do not include Microsoft-internal work item IDs, ADO URLs, incident IDs, or +internal service names in repository content or pull requests. Describe the +public problem and fix without requiring internal access. + ## Coding standards See [CONTRIBUTING.md](CONTRIBUTING.md) for StyleCop rules, error-handling 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 e4a80cd5b2..e2deedd9a4 100644 --- a/GVFS/GVFS.Common/Git/GitProcess.cs +++ b/GVFS/GVFS.Common/Git/GitProcess.cs @@ -321,17 +321,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.InvokeGitAgainstDotGitFolderOrOutsideEnlistment( "credential fill", stdin => stdin.Write("protocol=cert\npath=" + certificatePath + "\nusername=\n\n"), - usePreCommandHook: false); + parseStdOutLine: null, + 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, @@ -348,7 +358,8 @@ public virtual bool TryGetCertificatePassword( EventMetadata metadata = new EventMetadata { { "Success", success }, - { "CertificatePath", certificatePath } + { "CertificatePath", certificatePath }, + { "hasVisibleConsoleWindow", hasVisibleConsoleWindow } }; if (!success) @@ -375,16 +386,28 @@ 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.InvokeGitAgainstDotGitFolderOrOutsideEnlistment( GenerateCredentialVerbCommand("fill"), stdin => stdin.Write($"url={repoUrl}\n\n"), out bool usedDotGitFolder, + parseStdOutLine: null, usePreCommandHook: false, - timeoutMs: timeoutMs); + timeoutMs: timeoutMs, + mayRequireAuth: true, + hasVisibleConsoleWindow: hasVisibleConsoleWindow); if (gitCredentialOutput.ExitCodeIsFailure) { EventMetadata errorData = new EventMetadata(); + errorData.Add("hasVisibleConsoleWindow", hasVisibleConsoleWindow); // Records whether repo-local configuration (and therefore a // repo-local credential.helper) was visible to git. @@ -410,6 +433,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="); @@ -1063,7 +1094,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 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; @@ -1080,7 +1111,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; @@ -1148,7 +1188,9 @@ protected virtual Result InvokeGitImpl( int timeoutMs, string gitObjectsDirectory = null, bool usePreCommandHook = true, - Action parseStdOutToken = null) + Action parseStdOutToken = null, + bool mayRequireAuth = false, + bool? hasVisibleConsoleWindow = null) { if (failedToSetEncoding && writeStdIn != null) { @@ -1168,7 +1210,7 @@ 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)) + using (this.executingProcess = 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 @@ -1456,6 +1498,20 @@ internal static void ReadStdOutTokens(StreamReader reader, Action parseS } } + 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; @@ -1491,7 +1547,9 @@ private Result InvokeGitOutsideEnlistment( Action writeStdIn, Action parseStdOutLine, int timeout = -1, - bool usePreCommandHook = true) + bool usePreCommandHook = true, + bool mayRequireAuth = false, + bool? hasVisibleConsoleWindow = null) { return this.InvokeGitImpl( command, @@ -1501,7 +1559,9 @@ private Result InvokeGitOutsideEnlistment( writeStdIn: writeStdIn, parseStdOutLine: parseStdOutLine, timeoutMs: timeout, - usePreCommandHook: usePreCommandHook); + usePreCommandHook: usePreCommandHook, + mayRequireAuth: mayRequireAuth, + hasVisibleConsoleWindow: hasVisibleConsoleWindow); } /// @@ -1526,7 +1586,9 @@ private Result InvokeGitAgainstDotGitFolderOrOutsideEnlistment( Action writeStdIn, Action parseStdOutLine = null, bool usePreCommandHook = true, - int timeoutMs = -1) + int timeoutMs = -1, + bool mayRequireAuth = false, + bool? hasVisibleConsoleWindow = null) { return this.InvokeGitAgainstDotGitFolderOrOutsideEnlistment( command, @@ -1534,7 +1596,9 @@ private Result InvokeGitAgainstDotGitFolderOrOutsideEnlistment( out bool _, parseStdOutLine, usePreCommandHook, - timeoutMs); + timeoutMs, + mayRequireAuth, + hasVisibleConsoleWindow); } /// @@ -1550,7 +1614,9 @@ private Result InvokeGitAgainstDotGitFolderOrOutsideEnlistment( out bool usedDotGitFolder, Action parseStdOutLine = null, bool usePreCommandHook = true, - int timeoutMs = -1) + int timeoutMs = -1, + bool mayRequireAuth = false, + bool? hasVisibleConsoleWindow = null) { // Evaluate once so the reported route always matches the route taken. usedDotGitFolder = this.DotGitRootExists(); @@ -1562,7 +1628,9 @@ private Result InvokeGitAgainstDotGitFolderOrOutsideEnlistment( writeStdIn, parseStdOutLine, usePreCommandHook: usePreCommandHook, - timeoutMs: timeoutMs); + timeoutMs: timeoutMs, + mayRequireAuth: mayRequireAuth, + hasVisibleConsoleWindow: hasVisibleConsoleWindow); } return this.InvokeGitOutsideEnlistment( @@ -1570,7 +1638,9 @@ private Result InvokeGitAgainstDotGitFolderOrOutsideEnlistment( writeStdIn, parseStdOutLine, timeout: timeoutMs, - usePreCommandHook: usePreCommandHook); + usePreCommandHook: usePreCommandHook, + mayRequireAuth: mayRequireAuth, + hasVisibleConsoleWindow: hasVisibleConsoleWindow); } /// @@ -1624,7 +1694,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 @@ -1638,7 +1710,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.Common/Git/LibGit2Repo.cs b/GVFS/GVFS.Common/Git/LibGit2Repo.cs index b21eb83b9e..6667dd2c82 100644 --- a/GVFS/GVFS.Common/Git/LibGit2Repo.cs +++ b/GVFS/GVFS.Common/Git/LibGit2Repo.cs @@ -7,6 +7,24 @@ namespace GVFS.Common.Git { + /// + /// In-process wrapper over libgit2 (git2.dll) for local Git object, tree, blob, and + /// config lookups without spawning git.exe. + /// + /// + /// libgit2 marshalling contract: libgit2 treats EVERY string it receives and returns as + /// UTF-8 (paths, revspecs, config keys and values, error messages). The default marshalling + /// for a bare [DllImport] string parameter is , which encodes + /// through the Windows ANSI code page and silently corrupts (or drops, as '?') any non-ASCII + /// byte — for example a repository path under a non-English Windows user name. When adding a + /// new libgit2 P/Invoke to , annotate every string parameter, + /// out string parameter, string return value, and string struct field with + /// [MarshalAs(UnmanagedType.LPUTF8Str)]. Do NOT use — that + /// marshals UTF-16, which libgit2 does not accept. For a libgit2 function that returns a borrowed + /// pointer owned by libgit2 (for example git_config_get_string), marshal it as + /// and copy with so the + /// interop marshaller does not free libgit2's heap memory. + /// public class LibGit2Repo : IDisposable { private bool disposedValue = false; @@ -572,6 +590,12 @@ protected bool CheckSafeDirectoryConfigForCaseSensitivityIssue(ITracer tracer, s return configuredMatchingDirectory != null && TryOpenRepo(configuredMatchingDirectory, out repoHandle) == Native.ResultCode.Success; } + /// + /// Raw libgit2 P/Invoke declarations. See the marshalling contract on + /// : every string parameter/return/field passed to or from + /// libgit2 MUST be [MarshalAs(UnmanagedType.LPUTF8Str)] (never the ANSI default, + /// never ). + /// public static class Native { public enum ResultCode : int @@ -602,7 +626,7 @@ public static GitOid IntPtrToGitOid(IntPtr oidPtr) public static extern int Shutdown(); [DllImport(Git2NativeLibName, EntryPoint = "git_revparse_single")] - public static extern ResultCode RevParseSingle(out IntPtr objectHandle, IntPtr repoHandle, string oid); + public static extern ResultCode RevParseSingle(out IntPtr objectHandle, IntPtr repoHandle, [MarshalAs(UnmanagedType.LPUTF8Str)] string oid); public static string GetLastError() { @@ -612,7 +636,11 @@ public static string GetLastError() return "Operation was successful"; } - return Marshal.PtrToStructure(ptr).Message; + // git_error.message is a borrowed UTF-8 char* owned by libgit2. Keep it as an + // IntPtr and copy it with Marshal.PtrToStringUTF8 so the interop marshaller never + // frees libgit2's heap memory (same pattern as GitConfigEntry). + GitError error = Marshal.PtrToStructure(ptr); + return error.Message == IntPtr.Zero ? null : Marshal.PtrToStringUTF8(error.Message); } [DllImport(Git2NativeLibName, EntryPoint = "git_error_last")] @@ -621,8 +649,7 @@ public static string GetLastError() [StructLayout(LayoutKind.Sequential)] private struct GitError { - [MarshalAs(UnmanagedType.LPStr)] - public string Message; + public IntPtr Message; public int Klass; } @@ -630,7 +657,7 @@ private struct GitError public static class Repo { [DllImport(Git2NativeLibName, EntryPoint = "git_repository_open")] - public static extern ResultCode Open(out IntPtr repoHandle, string path); + public static extern ResultCode Open(out IntPtr repoHandle, [MarshalAs(UnmanagedType.LPUTF8Str)] string path); [DllImport(Git2NativeLibName, EntryPoint = "git_repository_free")] public static extern void Free(IntPtr repoHandle); @@ -649,7 +676,7 @@ public static class Odb public static extern void Free(IntPtr odbHandle); [DllImport(Git2NativeLibName, EntryPoint = "git_oid_fromstr")] - public static extern ResultCode OidFromStr(out GitOid oid, string str); + public static extern ResultCode OidFromStr(out GitOid oid, [MarshalAs(UnmanagedType.LPUTF8Str)] string str); } public static class Config @@ -664,13 +691,13 @@ public static class Config public static extern ResultCode Snapshot(out IntPtr snapshotConfigHandle, IntPtr configHandle); [DllImport(Git2NativeLibName, EntryPoint = "git_config_get_string")] - public static extern ResultCode GetString(out IntPtr value, IntPtr configHandle, string name); + public static extern ResultCode GetString(out IntPtr value, IntPtr configHandle, [MarshalAs(UnmanagedType.LPUTF8Str)] string name); [DllImport(Git2NativeLibName, EntryPoint = "git_config_get_multivar_foreach")] public static extern ResultCode GetMultivarForeach( IntPtr configHandle, - string name, - string regex, + [MarshalAs(UnmanagedType.LPUTF8Str)] string name, + [MarshalAs(UnmanagedType.LPUTF8Str)] string regex, GitConfigMultivarCallback callback, IntPtr payload); @@ -719,7 +746,7 @@ private static string MarshalUtf8String(IntPtr ptr) } [DllImport(Git2NativeLibName, EntryPoint = "git_config_get_bool")] - public static extern ResultCode GetBool(out bool value, IntPtr configHandle, string name); + public static extern ResultCode GetBool(out bool value, IntPtr configHandle, [MarshalAs(UnmanagedType.LPUTF8Str)] string name); [DllImport(Git2NativeLibName, EntryPoint = "git_config_free")] public static extern void Free(IntPtr configHandle); diff --git a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CorruptionReproTests.cs b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CorruptionReproTests.cs index 9e7eec0bb6..4dade3f56c 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CorruptionReproTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/CorruptionReproTests.cs @@ -12,7 +12,10 @@ namespace GVFS.FunctionalTests.Tests.GitCommands { /// - /// This class is used to reproduce corruption scenarios in the GVFS virtual projection. + /// Reproduces reported scenarios that corrupted the GVFS virtual projection or + /// gave results that did not match a normal git repo. Each test is a black-box + /// regression test: it runs the command sequence that exposed the problem and + /// compares the results with the control repo. /// [Category(Categories.GitCommands)] [TestFixtureSource(typeof(GitRepoTests), nameof(GitRepoTests.ValidateWorkingTree))] @@ -76,6 +79,32 @@ public void ReproCherryPickRestoreCorruption() this.FilesShouldMatchCheckoutOfSourceBranch(); } + /// + /// Regression test for a sequence of commands that a user ran and that + /// showed incorrect behavior: after "git blame" and "git reset --mixed", + /// GVFS did not report a changed file as modified. The control repo did. + /// + /// This is a black-box test. It does not assert why the behavior was wrong. + /// It only runs the same commands and compares the results with the + /// control repo. ResetMixedTests.ResetMixedClearsSkipWorktreeOnHydratedPlaceholder + /// asserts the conditions that cause the problem. + /// + /// The fix requires microsoft/git v2.55.0.vfs.0.3 or later. + /// + /// On the FunctionalTests/20201014 branch, Readme.md is the only file that + /// differs between HEAD and HEAD~1. + /// + [TestCase] + public void ReproResetMixedSkipWorktree() + { + this.ValidateGitCommand("blame Readme.md"); + this.ValidateGitCommand("checkout -b tests/functional/ReproResetMixedSkipWorktree"); + + // Both repos must report Readme.md as modified. + this.ValidateGitCommand("reset HEAD~1"); + this.ValidateGitCommand("ls-files -v Readme.md"); + } + /// /// Reproduction of a reported issue: /// Restoring a file after its parent directory was deleted fails with diff --git a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/ResetMixedTests.cs b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/ResetMixedTests.cs index 0f7740d901..af77ee6e46 100644 --- a/GVFS/GVFS.FunctionalTests/Tests/GitCommands/ResetMixedTests.cs +++ b/GVFS/GVFS.FunctionalTests/Tests/GitCommands/ResetMixedTests.cs @@ -1,6 +1,9 @@ using GVFS.FunctionalTests.Properties; using GVFS.FunctionalTests.Should; +using GVFS.FunctionalTests.Tools; +using GVFS.Tests.Should; using NUnit.Framework; +using System.IO; namespace GVFS.FunctionalTests.Tests.GitCommands { @@ -30,6 +33,52 @@ public void ResetMixedAfterPrefetch() this.FilesShouldMatchCheckoutOfTargetBranch(); } + /// + /// A mixed reset must clear skip-worktree on a hydrated placeholder whose + /// index entry the reset changes. A hydrated placeholder is on disk but is + /// not in ModifiedPaths, so it still has skip-worktree. The reset does not + /// update the working tree, so the file on disk no longer matches the index. + /// If skip-worktree stays set, git does not compare the file to the index, + /// and reset and status do not report it as modified. + /// + /// Placeholders that are not on disk do not have this problem, because git + /// writes them to disk and clears skip-worktree. For that reason, this test + /// asserts each precondition before the reset. + /// + /// The git side of this behavior requires microsoft/git v2.55.0.vfs.0.3 or later. + /// + [TestCase] + public void ResetMixedClearsSkipWorktreeOnHydratedPlaceholder() + { + string filePath = Path.Combine("Test_ConflictTests", "ModifiedFiles", "ChangeInTarget.txt"); + string gitPath = filePath.Replace(Path.DirectorySeparatorChar, TestConstants.GitPathSeparator); + + // Create local branches for both commits in both repos. + this.ValidateGitCommand("checkout " + GitRepoTests.ConflictSourceBranch); + this.ValidateGitCommand("checkout " + GitRepoTests.ConflictTargetBranch); + + // Precondition: the reset changes the index entry for the file. + GitProcess.InvokeProcess( + this.ControlGitRepo.RootPath, + $"diff --quiet {GitRepoTests.ConflictSourceBranch} {GitRepoTests.ConflictTargetBranch} -- {gitPath}") + .ExitCode.ShouldEqual(1, $"{gitPath} must differ between the reset source and target"); + + // Precondition: the file is a hydrated placeholder. A read hydrates it + // but does not add it to ModifiedPaths, so skip-worktree stays set. + this.Enlistment.GetVirtualPathTo(filePath).ShouldBeAFile(this.FileSystem).WithContents(); + GVFSHelpers.ModifiedPathsShouldNotContain(this.Enlistment, this.FileSystem, gitPath); + this.SkipWorktreeFlagShouldBe(gitPath, expectedFlag: 'S'); + + // The reset output and status must report the file as modified, as in the control repo. + this.ValidateGitCommand("reset --mixed " + GitRepoTests.ConflictSourceBranch); + + // After the reset, skip-worktree is cleared, and GVFS adds the file to + // ModifiedPaths so that later git commands also compare it to the index. + this.SkipWorktreeFlagShouldBe(gitPath, expectedFlag: 'H'); + GVFSHelpers.ModifiedPathsShouldContain(this.Enlistment, this.FileSystem, gitPath); + this.FileContentsShouldMatch(filePath); + } + [TestCase] public void ResetMixedAndCheckoutNewBranch() { @@ -108,5 +157,13 @@ protected override void CreateEnlistment() this.ControlGitRepo.Fetch(GitRepoTests.ConflictTargetBranch); this.ControlGitRepo.Fetch(GitRepoTests.ConflictSourceBranch); } + + private void SkipWorktreeFlagShouldBe(string gitPath, char expectedFlag) + { + // "ls-files -v" prefixes each entry with a tag: "S" means skip-worktree is set, "H" means it is not. + ProcessResult result = GitProcess.InvokeProcess(this.Enlistment.RepoRoot, "ls-files -v -- " + gitPath); + result.ExitCode.ShouldEqual(0, result.Errors); + result.Output.Trim().ShouldEqual($"{expectedFlag} {gitPath}"); + } } } diff --git a/GVFS/GVFS.FunctionalTests/Tests/LibGit2NonAsciiPathTests.cs b/GVFS/GVFS.FunctionalTests/Tests/LibGit2NonAsciiPathTests.cs new file mode 100644 index 0000000000..d641ac6770 --- /dev/null +++ b/GVFS/GVFS.FunctionalTests/Tests/LibGit2NonAsciiPathTests.cs @@ -0,0 +1,107 @@ +using GVFS.Common.Git; +using GVFS.Common.Tracing; +using GVFS.FunctionalTests.Tools; +using GVFS.Tests.Should; +using NUnit.Framework; +using System.IO; +using System.Runtime.InteropServices; +using System.Text; +using GitProcess = GVFS.FunctionalTests.Tools.GitProcess; + +namespace GVFS.FunctionalTests.Tests +{ + /// + /// Exercises the real libgit2 (git2.dll) open/read path in against a + /// repository whose path and config value contain non-ASCII characters. This is a regression + /// guard for the string-marshalling bug where the libgit2 P/Invoke declarations used the + /// implicit default instead of UTF-8: + /// on a non-UTF-8 Windows code page the ANSI marshaller drops an unmappable character to '?', + /// so git_repository_open resolves the wrong path and the constructor throws. No + /// mock-based unit test can catch this because it only manifests through the native P/Invoke. + /// + [TestFixture] + public class LibGit2NonAsciiPathTests + { + // "café_日本語_☃" — Latin-1 accent, CJK, and a symbol outside every common Windows ANSI + // code page. Built from \u escapes so the reproduction does not depend on this source + // file's on-disk encoding. + private const string NonAsciiToken = "caf\u00e9_\u65e5\u672c\u8a9e_\u2603"; + + private const string StringConfigKey = "gvfs.functionaltests-nonascii"; + private const string NonAsciiConfigValue = "caf\u00e9-\u65e5\u672c\u8a9e-\u2603"; + + private string repoRoot; + + [OneTimeSetUp] + public void CreateRepo() + { + // The ANSI default marshalling only corrupts non-ASCII characters when the active + // Windows ANSI code page cannot represent them. If the process ANSI code page is + // UTF-8 (CP65001, the "Beta: Use Unicode UTF-8" setting), ANSI marshalling already + // produces correct UTF-8, so the bug cannot reproduce and this guard is inconclusive. + // Log the active code page so a reader of the test results can tell whether this guard + // actually ran (e.g. CP1252 on the standard CI runners) or was skipped as inconclusive. + const uint CP_UTF8 = 65001; + uint activeCodePage = GetACP(); + TestContext.Progress.WriteLine($"{nameof(LibGit2NonAsciiPathTests)}: active ANSI code page = {activeCodePage}"); + if (activeCodePage == CP_UTF8) + { + Assert.Ignore( + $"Active ANSI code page is UTF-8 (CP{CP_UTF8}); ANSI marshalling already produces UTF-8, so the " + + "ANSI-vs-UTF-8 marshalling bug cannot reproduce and this regression guard is inconclusive here. " + + "Run on a non-UTF-8 code page (e.g. CP1252, the standard CI runner default) to exercise it."); + } + + this.repoRoot = Path.Combine( + Path.GetTempPath(), + "GVFS.LibGit2NonAsciiPathTests_" + NonAsciiToken + "_" + Path.GetRandomFileName()); + Directory.CreateDirectory(this.repoRoot); + + GitProcess.Invoke(this.repoRoot, "init"); + GitProcess.Invoke(this.repoRoot, "config user.name \"Functional Test User\""); + GitProcess.Invoke(this.repoRoot, "config user.email \"functional@test.com\""); + + // Write the non-ASCII config value straight to .git/config as UTF-8 (no BOM) so the + // value round-tripped through libgit2 is not confounded by how git.exe would encode a + // command-line argument. + string configPath = Path.Combine(this.repoRoot, ".git", "config"); + File.AppendAllText( + configPath, + $"[gvfs]\n\tfunctionaltests-nonascii = {NonAsciiConfigValue}\n", + new UTF8Encoding(encoderShouldEmitUTF8Identifier: false)); + } + + [OneTimeTearDown] + public void DeleteRepo() + { + if (this.repoRoot != null) + { + RepositoryHelpers.DeleteTestDirectory(this.repoRoot); + } + } + + [TestCase] + public void OpensRepositoryAtNonAsciiPath() + { + // Before the LPUTF8Str marshalling fix, git_repository_open received an ANSI-mangled + // path, failed, and the constructor threw InvalidDataException. + using (LibGit2Repo repo = new LibGit2Repo(NullTracer.Instance, this.repoRoot)) + { + // Reading a known config value proves the RepoHandle is real, not just non-null. + repo.GetConfigString("user.email").ShouldEqual("functional@test.com"); + } + } + + [TestCase] + public void GetConfigStringReturnsNonAsciiValue() + { + using (LibGit2Repo repo = new LibGit2Repo(NullTracer.Instance, this.repoRoot)) + { + repo.GetConfigString(StringConfigKey).ShouldEqual(NonAsciiConfigValue); + } + } + + [DllImport("kernel32.dll")] + private static extern uint GetACP(); + } +} diff --git a/GVFS/GVFS.FunctionalTests/Tools/GitHelpers.cs b/GVFS/GVFS.FunctionalTests/Tools/GitHelpers.cs index 8bdc74f17a..48418452d2 100644 --- a/GVFS/GVFS.FunctionalTests/Tools/GitHelpers.cs +++ b/GVFS/GVFS.FunctionalTests/Tools/GitHelpers.cs @@ -130,6 +130,11 @@ private static string FilterMessages( return input; } + /// + /// Runs a git command in the control repo and in the GVFS repo, and asserts + /// that the output and errors match. If the command is not "status", this + /// method then runs "status" in both repos and compares that output too. + /// public static void ValidateGitCommand( GVFSFunctionalTestEnlistment enlistment, ControlGitRepo controlGitRepo, 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/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 593ea05aa5..a1a5485453 100644 --- a/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs +++ b/GVFS/GVFS.UnitTests/Mock/Git/MockGitProcess.cs @@ -27,6 +27,14 @@ public MockGitProcess(string gitBinPath, string workingDirectoryRoot) } public List CommandsRun { get; private set; } + + /// + /// 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; private set; } public bool ShouldFail { get; set; } public Dictionary StoredCredentials { get; private set; } public Dictionary> CredentialApprovals { get; private set; } @@ -38,6 +46,23 @@ public MockGitProcess(string gitBinPath, string workingDirectoryRoot) /// public List DotGitDirectoriesUsed { get; private set; } + /// + /// 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); @@ -94,10 +119,13 @@ protected override Result InvokeGitImpl( int timeoutMs, string gitObjectsDirectory = null, bool usePrecommandHook = true, - Action parseStdOutToken = null) + Action parseStdOutToken = null, + bool mayRequireAuth = false, + bool? hasVisibleConsoleWindow = null) { this.CommandsRun.Add(command); this.DotGitDirectoriesUsed.Add(dotGitDirectory); + this.InvocationsRun.Add(new GitInvocation(command, mayRequireAuth, hasVisibleConsoleWindow)); if (this.ShouldFail) { @@ -156,11 +184,26 @@ private void Initialize() { this.CommandsRun = new List(); this.DotGitDirectoriesUsed = new List(); + this.InvocationsRun = new List(); this.StoredCredentials = new Dictionary(StringComparer.OrdinalIgnoreCase); this.CredentialApprovals = new Dictionary>(); this.CredentialRejections = new Dictionary>(); } + 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) diff --git a/GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs b/GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs index 660e9a45cb..856b0c6f1b 100644 --- a/GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs +++ b/GVFS/GVFS.UnitTests/Virtualization/FileSystemCallbacksTests.cs @@ -123,6 +123,7 @@ public void GetMetadataForHeartBeatDoesNotChangeEventLevelWhenNoPlaceholderHaveB metadata.ShouldContain("ModifiedPathsCount", 1); metadata.ShouldContain("FilePlaceholderCount", 0); metadata.ShouldContain(nameof(RepoMetadata.Instance.EnlistmentId), RepoMetadata.Instance.EnlistmentId); + metadata.ShouldContain("IsSendingAnonymousRequests", true); } mockPlaceholderDb.VerifyAll(); @@ -163,7 +164,7 @@ public void GetMetadataForHeartBeatDoesSetsEventLevelToInformationalWhenPlacehol eventLevel.ShouldEqual(EventLevel.Informational); // "ModifiedPathsCount" should be 1 because ".gitattributes" is always present - metadata.Count.ShouldEqual(8); + metadata.Count.ShouldEqual(9); metadata.ContainsKey("FilePlaceholderCreation").ShouldBeTrue(); metadata.TryGetValue("FilePlaceholderCreation", out object fileNestedMetadata); GVFSJsonOptions.Serialize(fileNestedMetadata).ShouldContain("\"ProcessName1\":\"GVFS.UnitTests.exe\""); @@ -173,6 +174,7 @@ public void GetMetadataForHeartBeatDoesSetsEventLevelToInformationalWhenPlacehol metadata.ShouldContain("FolderPlaceholderCount", 0); metadata.ShouldContain(nameof(RepoMetadata.Instance.EnlistmentId), RepoMetadata.Instance.EnlistmentId); metadata.ContainsKey("PhysicalDiskInfo").ShouldBeTrue(); + metadata.ShouldContain("IsSendingAnonymousRequests", true); // Create more placeholders fileSystemCallbacks.OnPlaceholderFileCreated("test.txt", "2222233333444445555566666777778888899999", "GVFS.UnitTests.exe2"); @@ -186,7 +188,7 @@ public void GetMetadataForHeartBeatDoesSetsEventLevelToInformationalWhenPlacehol eventLevel = writeToLogFile2 ? EventLevel.Informational : EventLevel.Verbose; eventLevel.ShouldEqual(EventLevel.Informational); - metadata.Count.ShouldEqual(8); + metadata.Count.ShouldEqual(9); // Only processes that have created placeholders since the last heartbeat should be named metadata.ContainsKey("FilePlaceholderCreation").ShouldBeTrue(); @@ -206,6 +208,7 @@ public void GetMetadataForHeartBeatDoesSetsEventLevelToInformationalWhenPlacehol metadata.ShouldContain("FolderPlaceholderCount", 1); metadata.ShouldContain(nameof(RepoMetadata.Instance.EnlistmentId), RepoMetadata.Instance.EnlistmentId); metadata.ContainsKey("PhysicalDiskInfo").ShouldBeTrue(); + metadata.ShouldContain("IsSendingAnonymousRequests", true); } mockPlaceholderDb.VerifyAll(); diff --git a/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs b/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs index eaccc0c98c..b3fc2fdd27 100644 --- a/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs +++ b/GVFS/GVFS.Virtualization/FileSystemCallbacks.cs @@ -310,6 +310,12 @@ public EventMetadata GetAndResetHeartBeatMetadata(out bool logToFile) this.context.Enlistment.WorkingDirectoryBackingRoot, sizeStatsOnly: true)); + // True/false means "HttpRequestor is/isn't attaching an Authorization + // header to outgoing requests" (see HttpRequestor.cs), so a mount stuck + // reporting true for long stretches against a server that requires auth + // is the signature of the anonymous-auth-latch bug this field detects. + metadata.Add("IsSendingAnonymousRequests", this.context.Enlistment.Authentication.IsAnonymous); + return metadata; } diff --git a/scripts/Build.bat b/scripts/Build.bat index 592020f594..2cb426056f 100644 --- a/scripts/Build.bat +++ b/scripts/Build.bat @@ -142,6 +142,13 @@ IF NOT DEFINED VCVARS_BAT ( ECHO INFO: Initializing VC++ env for %ARCH% via "%VCVARS_BAT%" CALL "%VCVARS_BAT%" %ARCH% || GOTO ERROR +REM cl.exe can transiently fail with C3859 "Failed to create virtual memory +REM for PCH" under memory pressure (observed on the arm64 runners, where PCH +REM creation can collide with other concurrent processes for address space). +REM It is not caused by anything in this repo's code and reliably succeeds on +REM a retry, so retry each native project build a few times before failing. +SET NATIVE_BUILD_RETRIES=3 + FOR %%P IN ( "%VFS_SRCDIR%\GVFS\GitHooksLoader\GitHooksLoader.vcxproj" "%VFS_SRCDIR%\GVFS\GVFS.NativeTests\GVFS.NativeTests.vcxproj" @@ -149,14 +156,7 @@ FOR %%P IN ( "%VFS_SRCDIR%\GVFS\GVFS.ReadObjectHook\GVFS.ReadObjectHook.vcxproj" "%VFS_SRCDIR%\GVFS\GVFS.VirtualFileSystemHook\GVFS.VirtualFileSystemHook.vcxproj" ) DO ( - ECHO Building %%~nP... - "%MSBUILD_EXEC%" %%P ^ - /t:Build ^ - /v:%VERBOSITY% ^ - /p:Configuration=%CONFIGURATION% ^ - /p:Platform=%NATIVE_PLATFORM% ^ - /p:VfsArch=%ARCH% ^ - /p:SolutionDir="%VFS_SRCDIR%\\" || GOTO ERROR + CALL :BuildNativeProject %%P || GOTO ERROR ) REM vcvarsall.bat sets Platform= in the environment. MSBuild picks that @@ -201,6 +201,46 @@ FOR %%P IN ( GOTO :EOF +REM Builds a single native vcxproj, retrying on failure up to +REM %NATIVE_BUILD_RETRIES% times, but ONLY when the failure is the known +REM transient cl.exe PCH virtual-memory error (C3859). Any other failure +REM (a real compile error, a bad project change, etc.) fails immediately +REM on the first attempt so it isn't masked or slowed down by retries. +:BuildNativeProject +SET "NATIVE_PROJ=%~1" +SET NATIVE_ATTEMPT=1 +:BuildNativeProject_Retry +SET "NATIVE_LOG=%TEMP%\gvfs_native_build_%~n1_%RANDOM%.log" +ECHO Building %~n1 (attempt %NATIVE_ATTEMPT% of %NATIVE_BUILD_RETRIES%)... +"%MSBUILD_EXEC%" "%NATIVE_PROJ%" ^ + /t:Build ^ + /v:%VERBOSITY% ^ + /p:Configuration=%CONFIGURATION% ^ + /p:Platform=%NATIVE_PLATFORM% ^ + /p:VfsArch=%ARCH% ^ + /p:SolutionDir="%VFS_SRCDIR%\\" > "%NATIVE_LOG%" 2>&1 +SET NATIVE_BUILD_RESULT=%ERRORLEVEL% +TYPE "%NATIVE_LOG%" +IF %NATIVE_BUILD_RESULT% EQU 0 ( + DEL "%NATIVE_LOG%" >NUL 2>&1 + EXIT /B 0 +) +FINDSTR /C:"C3859" "%NATIVE_LOG%" >NUL +SET NATIVE_IS_TRANSIENT=%ERRORLEVEL% +DEL "%NATIVE_LOG%" >NUL 2>&1 +IF NOT %NATIVE_IS_TRANSIENT% EQU 0 ( + ECHO ERROR: Build of %~n1 failed with a non-transient error; not retrying. + EXIT /B 1 +) +IF %NATIVE_ATTEMPT% GEQ %NATIVE_BUILD_RETRIES% ( + ECHO ERROR: Build of %~n1 failed after %NATIVE_BUILD_RETRIES% attempts, still with C3859. + EXIT /B 1 +) +ECHO WARNING: Build of %~n1 failed on attempt %NATIVE_ATTEMPT% with the transient PCH virtual-memory error ^(C3859^). Retrying in 10 seconds... +TIMEOUT /T 10 /NOBREAK >NUL +SET /A NATIVE_ATTEMPT+=1 +GOTO BuildNativeProject_Retry + :USAGE ECHO usage: %~n0%~x0 [^] [^] [^] [^] ECHO.