Repository navigation
Protect GitProcess.executingProcess with a single lock protocol - #2132
Draft
Tyrie Vella (tyrielv) wants to merge 3 commits into
Draft
Tyrie Vella (tyrielv) wants to merge 3 commits into
Tyrie Vella (tyrielv) wants to merge 3 commits into
Conversation
Background GVFS processes such as GVFS.Mount.exe run with a hidden console window. When git launches the Git Credential Manager from such a process, the credential manager parents its interactive sign-in prompt to the hidden console. The prompt then has no foreground right and sinks behind other windows, so the user never sees it. Launch git detached from the console (CreateNoWindow=true) for commands that may require authentication when the current process has no visible console window. With no console, git starts the credential manager detached, so the credential manager parents its prompt to a topmost stub window instead of the hidden console. A process with a visible console window (e.g. gvfs.exe run from a classic console) keeps the current behavior, so the credential manager parents the prompt to that window. Detection keys on the console window, not an arbitrary GUI window. A process whose console is hidden, or a non-displayed pseudo-console such as a foreground gvfs.exe run from Windows Terminal, is treated as having no visible window and also detaches git. For the Git Credential Manager this is equivalent or better, because the detached prompt is topmost instead of owned by a hidden host window. Add GVFSPlatform.CurrentProcessHasVisibleConsoleWindow. The Windows implementation checks GetAncestor(GetConsoleWindow(), GA_ROOTOWNER) and IsWindowVisible. Add a mayRequireAuth option to GitProcess.GetGitProcess, which detaches git from a hidden console only when set. Only credential fill can trigger an interactive prompt, so only the url and certificate fills set mayRequireAuth. Credential approve and reject store or erase an already-obtained credential and never prompt, so they keep the default. Every other git command GVFS runs is local and keeps the default, so the per-child conhost cost stays avoided for every non-authenticating call. Log hasVisibleConsoleWindow from TryGetCredential so a credential fill that does not surface a prompt stays diagnosable from telemetry. Guard the native console probe so a binding failure keeps the previous behavior instead of aborting the fill. Add a certificate-fill test for the mayRequireAuth wiring. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
…og it on failure Address review feedback on the credential-prompt console-detach change. - Probe CurrentProcessHasVisibleConsoleWindow once per credential-fill call and thread the value through InvokeGitAgainstDotGitFolder, InvokeGitImpl, and GetGitProcess so the logged value and the CreateNoWindow decision are the same probe. GetGitProcess treats a supplied value as authoritative and only probes when none is passed. - Log hasVisibleConsoleWindow on the credential-fill failure path (including the timeout a hidden, unanswered prompt produces), not only on success. The success-path metadata and the certificate-fill success and failure paths log the same value. - Remove the misnamed intermediate local in GetGitProcess; compute CreateNoWindow directly so the && short-circuits the probe when auth cannot be required. - Restore the original pre-command-hook comment wording. - MockPlatform gains ThrowOnConsoleProbe to simulate a native probe that cannot run; add tests for the safe fallback, the caller-supplied authoritative value, and console-visibility telemetry on the URL and certificate credential-fill failure paths. The telemetry tests assert the logged value, not just the key. - MockGitProcess records each git invocation (command, mayRequireAuth, and hasVisibleConsoleWindow) instead of keeping only the last value per command, so the wiring assertions cannot be fooled if a command runs twice with different flags. The failure-telemetry tests also assert the probed console-visibility value threads down to the git invocation. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
InvokeGitImpl wrote the shared executingProcess field outside any lock (at the using assignment and the finally cleanup), while TryKillRunningProcess reads it under processLock and only Start() was wrapped in processLock. The inconsistent protocol let a concurrent invocation on the same instance clobber the field, so a kill could target a not-yet-started or orphaned process, and the using of one invocation could dispose the process another still referenced. Publish and clear the field only under processLock, and only while the invocation holds executionLock. Keep the Process in a local so the using block disposes exactly the instance this invocation created. Add a deterministic regression test that runs two InvokeGitImpl calls on one instance and asserts TryKillRunningProcess observes the running process. Assisted-by: Claude Opus 4.8 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem and Context
GitProcess.InvokeGitImplandTryKillRunningProcessshare the instance fieldexecutingProcess, but they do not agree on how it is locked:usingassignment that creates the process, and at thefinallythat sets it back tonull.TryKillRunningProcessunderprocessLock.Start()call sits insideprocessLock.This inconsistent protocol has two consequences:
InvokeGitImplcalls on the sameGitProcessinstance clobber each other'sexecutingProcess. A later call's lockless assignment (or the= nullcleanup) can hide or orphan the other call's process, soTryKillRunningProcesskills the wrong process or a not-yet-started one. Theusingof one call can also dispose theProcessthat the other call still references.TryKillRunningProcesshas a visibility and ordering hole: the kill reads underprocessLock, but the write is lockless, so the kill can observe a stale ornullfield.This race is pre-existing and not introduced by any recent change. Credential fill is effectively serialized today, so it is latent, but general git invocations are not guaranteed to be serialized per instance.
Changes
executingProcessfield obey one consistent protocol: it is published and cleared only underprocessLock, and only while the invocation holdsexecutionLock. The field is assigned just beforeStart()insideprocessLock, and cleared in afinallyunderprocessLockbeforeexecutionLockis released, so a queued invocation cannot clobber it.Processin a local variable for theusingblock, so disposal always targets exactly the instance this invocation created, never one another invocation shares.GetGitProcessvirtualto provide a seam the unit test uses to substitute a controllable process.ConcurrentInvocation_DoesNotClobberExecutingProcessField) that runs twoInvokeGitImplcalls on one instance, orders them so the second queues onexecutionLock, and asserts thatTryKillRunningProcessobserves the running process rather than the queued one. The test fails against the old lockless protocol and passes with the fix.Stacking
This change is stacked on #2127, which also edits
GitProcess.cs(including theInvokeGitImplregion). It must merge after #2127. Until #2127 merges, this PR's diff includes #2127's commits; once #2127 merges tomaster, the diff reduces to only the race fix. After that, the branch will be rebased ontomaster.