Merge pull request #1989 from tyrielv/tyrielv/fix-orphan-lock-pid-race

Fix PID-recycling race in orphan-lock detection

Author: tyrielv

GVFS/GVFS.Common/GVFSLock.cs

@@ -41,6 +41,20 @@ public bool TryAcquireLockForExternalRequestor(
 
             existingExternalHolder = null;
 
+            // Capture the requestor's process start time so we can later distinguish the
+            // genuine holder from an unrelated process that happens to be reusing the same
+            // PID after the holder exits. If we cannot read the start time (e.g. permission
+            // failure on OpenProcess for a different-integrity caller) we still accept the
+            // lock and fall back to the legacy PID-only orphan check; record the fallback in
+            // telemetry so we can spot if it becomes common.
+            long? requestorStartTime = GVFSPlatform.Instance.TryGetActiveProcessStartTime(requestor.PID, out long startTime)
+                ? startTime
+                : (long?)null;
+            if (requestorStartTime == null)
+            {
+                metadata.Add("StartTimeUnavailable", true);
+            }
+
             try
             {
                 lock (this.acquisitionLock)
@@ -65,7 +79,7 @@ public bool TryAcquireLockForExternalRequestor(
                     metadata.Add("Result", "Accepted");
                     eventLevel = EventLevel.Informational;
 
-                    this.currentLockHolder.AcquireForExternalRequestor(requestor);
+                    this.currentLockHolder.AcquireForExternalRequestor(requestor, requestorStartTime);
                     this.Stats = new ActiveGitCommandStats();
 
                     return true;
@@ -190,12 +204,14 @@ private bool IsLockAvailable(bool checkExternalHolderOnly, out NamedPipeMessages
                 }
 
                 bool externalHolderTerminatedWithoutReleasingLock;
+                string terminationReason;
                 existingExternalHolder = this.currentLockHolder.GetExternalHolder(
-                    out externalHolderTerminatedWithoutReleasingLock);
+                    out externalHolderTerminatedWithoutReleasingLock,
+                    out terminationReason);
 
                 if (externalHolderTerminatedWithoutReleasingLock)
                 {
-                    this.ReleaseLockForTerminatedProcess(existingExternalHolder.PID);
+                    this.ReleaseLockForTerminatedProcess(existingExternalHolder.PID, terminationReason);
                     this.tracer.SetGitCommandSessionId(string.Empty);
                     existingExternalHolder = null;
                 }
@@ -204,11 +220,11 @@ private bool IsLockAvailable(bool checkExternalHolderOnly, out NamedPipeMessages
             }
         }
 
-        private bool ReleaseExternalLock(int pid, string eventName)
+        private bool ReleaseExternalLock(int pid, string eventName, EventMetadata extraMetadata = null)
         {
             lock (this.acquisitionLock)
             {
-                EventMetadata metadata = new EventMetadata();
+                EventMetadata metadata = extraMetadata ?? new EventMetadata();
 
                 try
                 {
@@ -251,9 +267,11 @@ private bool ReleaseExternalLock(int pid, string eventName)
             }
         }
 
-        private void ReleaseLockForTerminatedProcess(int pid)
+        private void ReleaseLockForTerminatedProcess(int pid, string terminationReason)
         {
-            this.ReleaseExternalLock(pid, "ExternalLockHolderExited");
+            EventMetadata metadata = new EventMetadata();
+            metadata.Add("ExternalHolderTerminationReason", terminationReason ?? "Unknown");
+            this.ReleaseExternalLock(pid, "ExternalLockHolderExited", metadata);
         }
 
         // The lock release event is a convenient place to record stats about things that happened while a git command was running,
@@ -383,6 +401,7 @@ public void AddStatsToTelemetry(EventMetadata metadata)
         private class LockHolder
         {
             private NamedPipeMessages.LockData externalLockHolder;
+            private long? externalLockHolderStartTime;
 
             public bool IsFree
             {
@@ -404,7 +423,7 @@ public void AcquireForGVFS()
                 this.IsGVFS = true;
             }
 
-            public void AcquireForExternalRequestor(NamedPipeMessages.LockData externalLockHolder)
+            public void AcquireForExternalRequestor(NamedPipeMessages.LockData externalLockHolder, long? startTime)
             {
                 if (this.IsGVFS ||
                     this.externalLockHolder != null)
@@ -413,27 +432,59 @@ public void AcquireForExternalRequestor(NamedPipeMessages.LockData externalLockH
                 }
 
                 this.externalLockHolder = externalLockHolder;
+                this.externalLockHolderStartTime = startTime;
             }
 
             public void Release()
             {
                 this.IsGVFS = false;
                 this.externalLockHolder = null;
+                this.externalLockHolderStartTime = null;
             }
 
             public NamedPipeMessages.LockData GetExternalHolder()
             {
                 return this.externalLockHolder;
             }
 
-            public NamedPipeMessages.LockData GetExternalHolder(out bool externalHolderTerminatedWithoutReleasingLock)
+            public NamedPipeMessages.LockData GetExternalHolder(out bool externalHolderTerminatedWithoutReleasingLock, out string terminationReason)
             {
                 externalHolderTerminatedWithoutReleasingLock = false;
+                terminationReason = null;
 
                 if (this.externalLockHolder != null)
                 {
                     int pid = this.externalLockHolder.PID;
-                    externalHolderTerminatedWithoutReleasingLock = !GVFSPlatform.Instance.IsProcessActive(pid);
+
+                    if (this.externalLockHolderStartTime is long capturedStartTime)
+                    {
+                        // Identity check: confirm the same process still owns this PID by comparing
+                        // the OS-supplied process start time we captured at acquisition with the
+                        // current one. A mismatch means the original holder exited and Windows
+                        // recycled the PID to a different process (the bug this code fixes).
+                        if (!GVFSPlatform.Instance.TryGetActiveProcessStartTime(pid, out long currentStartTime))
+                        {
+                            externalHolderTerminatedWithoutReleasingLock = true;
+                            terminationReason = "ProcessNotActive";
+                        }
+                        else if (currentStartTime != capturedStartTime)
+                        {
+                            externalHolderTerminatedWithoutReleasingLock = true;
+                            terminationReason = "PidRecycled";
+                        }
+                    }
+                    else
+                    {
+                        // Fallback for the rare case where we could not capture a start time at
+                        // acquisition time (e.g. cross-integrity OpenProcess denial). Use the
+                        // legacy PID-only liveness check, which is vulnerable to PID recycling
+                        // but matches pre-fix behavior.
+                        if (!GVFSPlatform.Instance.IsProcessActive(pid))
+                        {
+                            externalHolderTerminatedWithoutReleasingLock = true;
+                            terminationReason = "ProcessNotActive";
+                        }
+                    }
                 }
 
                 return this.externalLockHolder;

GVFS/GVFS.Common/GVFSPlatform.cs

@@ -66,6 +66,19 @@ public static void Register(GVFSPlatform platform)
         public abstract void PrepareProcessToRunInBackground();
 
         public abstract bool IsProcessActive(int processId);
+
+        /// <summary>
+        /// Returns true and writes an opaque, OS-supplied process-identity token (e.g. process
+        /// creation time on Windows) when the process with the given PID is currently active.
+        /// The token has no meaning beyond identity comparison: two calls for the same underlying
+        /// process yield equal tokens, and a call after the OS has recycled the PID to a different
+        /// process yields a different token. Returns false (and startTime = 0) if the process
+        /// is no longer running, or if it could not be identified for any reason (e.g. permission
+        /// failure). Callers must treat a false return as "no identity information available" and
+        /// fall back to <see cref="IsProcessActive(int)"/> if they still need a liveness check.
+        /// </summary>
+        public abstract bool TryGetActiveProcessStartTime(int processId, out long startTime);
+
         public abstract void IsServiceInstalledAndRunning(string name, out bool installed, out bool running);
         public abstract string GetNamedPipeName(string enlistmentRoot);
         public abstract string GetGVFSServiceNamedPipeName(string serviceName);

GVFS/GVFS.Common/NativeMethods.Shared.cs

@@ -158,6 +158,18 @@ public static extern SafeFileHandle OpenProcess(
         [return: MarshalAs(UnmanagedType.Bool)]
         public static extern bool GetExitCodeProcess(SafeFileHandle hProcess, out uint lpExitCode);
 
+        // GetProcessTimes writes four FILETIME values (each two DWORDs / 8 bytes).
+        // We marshal each as `out long` because we only ever compare the raw 64-bit value
+        // (creation time) for identity purposes; we never decode it as a date.
+        [DllImport("kernel32.dll", SetLastError = true)]
+        [return: MarshalAs(UnmanagedType.Bool)]
+        public static extern bool GetProcessTimes(
+            SafeFileHandle hProcess,
+            out long lpCreationTime,
+            out long lpExitTime,
+            out long lpKernelTime,
+            out long lpUserTime);
+
         [DllImport("kernel32.dll", SetLastError = true, CharSet = CharSet.Unicode)]
         private static extern SafeFileHandle CreateFile(
             [In] string lpFileName,

GVFS/GVFS.Platform.Windows/WindowsPlatform.Shared.cs

@@ -70,6 +70,44 @@ public static bool IsProcessActiveImplementation(int processId, bool tryGetProce
             }
         }
 
+        /// <summary>
+        /// Returns true if a process with the given PID is currently active AND writes its
+        /// creation timestamp (raw FILETIME, 100-ns ticks since 1601) to <paramref name="startTime"/>.
+        /// The returned value is intended for identity comparison only -- two calls for the
+        /// same underlying process (no PID reuse) always yield equal values; if the OS recycles
+        /// a PID to a new process the value will differ. Returns false (and startTime = 0)
+        /// if the process is gone, has terminated (but its kernel object lingers due to an
+        /// outstanding handle elsewhere), or cannot be opened for QueryLimitedInformation.
+        /// </summary>
+        public static bool TryGetActiveProcessStartTimeImplementation(int processId, out long startTime)
+        {
+            startTime = 0;
+
+            using (SafeFileHandle process = NativeMethods.OpenProcess(NativeMethods.ProcessAccessFlags.QueryLimitedInformation, false, processId))
+            {
+                if (process.IsInvalid)
+                {
+                    return false;
+                }
+
+                // GetProcessTimes succeeds for terminated processes whose kernel object still
+                // exists (e.g., an outstanding handle elsewhere). Confirm the process is still
+                // running before trusting the creation time as an identity marker.
+                if (!NativeMethods.GetExitCodeProcess(process, out uint exitCode) || exitCode != StillActive)
+                {
+                    return false;
+                }
+
+                if (!NativeMethods.GetProcessTimes(process, out long creationTime, out _, out _, out _))
+                {
+                    return false;
+                }
+
+                startTime = creationTime;
+                return true;
+            }
+        }
+
         public static string GetNamedPipeNameImplementation(string enlistmentRoot)
         {
             return "GVFS_" + enlistmentRoot.ToUpper().Replace(':', '_');

GVFS/GVFS.Platform.Windows/WindowsPlatform.cs

@@ -205,6 +205,11 @@ public override bool IsProcessActive(int processId)
             return WindowsPlatform.IsProcessActiveImplementation(processId, tryGetProcessById: true);
         }
 
+        public override bool TryGetActiveProcessStartTime(int processId, out long startTime)
+        {
+            return WindowsPlatform.TryGetActiveProcessStartTimeImplementation(processId, out startTime);
+        }
+
         public override void IsServiceInstalledAndRunning(string name, out bool installed, out bool running)
         {
             ServiceController service = ServiceController.GetServices().FirstOrDefault(s => s.ServiceName.Equals(name, StringComparison.Ordinal));

GVFS/GVFS.UnitTests/Common/GVFSLockTests.cs

@@ -182,6 +182,84 @@ public void TryAcquireLockForExternalRequestor_WhenExternalHolderTerminated()
             mockTracer.VerifyAll();
         }
 
+        /// <summary>
+        /// Regression test for OrphanedGVFSLockIsCleanedUp flake: simulates the original lock holder
+        /// exiting and the OS recycling its PID to a different, unrelated process before the orphan
+        /// check fires. Identity is preserved via process start time, so the orphan must be detected
+        /// even though the (now-different) process at the holder's PID still appears active.
+        /// </summary>
+        [TestCase]
+        public void TryAcquireLockForExternalRequestor_WhenHolderPidRecycled()
+        {
+            Mock<ITracer> mockTracer = new Mock<ITracer>(MockBehavior.Strict);
+            mockTracer.Setup(x => x.RelatedEvent(EventLevel.Informational, "TryAcquireLockExternal", It.IsAny<EventMetadata>()));
+            mockTracer.Setup(x => x.RelatedEvent(EventLevel.Informational, "ExternalLockHolderExited", It.IsAny<EventMetadata>(), Keywords.Telemetry));
+            mockTracer.Setup(x => x.SetGitCommandSessionId(string.Empty));
+            MockPlatform mockPlatform = (MockPlatform)GVFSPlatform.Instance;
+
+            // Acquire as DefaultLockData.PID with an explicit start time so the new identity-check
+            // path (not the legacy fallback) runs.
+            mockPlatform.ActiveProcesses.Add(DefaultLockData.PID);
+            mockPlatform.ProcessStartTimes[DefaultLockData.PID] = 1000;
+            GVFSLock gvfsLock = new GVFSLock(mockTracer.Object);
+            NamedPipeMessages.LockData existingExternalHolder;
+            gvfsLock.TryAcquireLockForExternalRequestor(DefaultLockData, out existingExternalHolder).ShouldBeTrue();
+            existingExternalHolder.ShouldBeNull();
+            this.ValidateLockHeld(gvfsLock, DefaultLockData);
+
+            // Simulate PID recycling: the PID is still reported active (a different process now owns it),
+            // but the start time differs from the one we captured at acquisition.
+            mockPlatform.ProcessStartTimes[DefaultLockData.PID] = 2000;
+
+            NamedPipeMessages.LockData newLockData = new NamedPipeMessages.LockData(4321, false, false, "git new", "456");
+            mockPlatform.ActiveProcesses.Add(newLockData.PID);
+            mockPlatform.ProcessStartTimes[newLockData.PID] = 3000;
+            gvfsLock.TryAcquireLockForExternalRequestor(newLockData, out existingExternalHolder).ShouldBeTrue();
+            existingExternalHolder.ShouldBeNull();
+            this.ValidateLockHeld(gvfsLock, newLockData);
+
+            mockPlatform.ActiveProcesses.Remove(DefaultLockData.PID);
+            mockPlatform.ActiveProcesses.Remove(newLockData.PID);
+            mockPlatform.ProcessStartTimes.Remove(DefaultLockData.PID);
+            mockPlatform.ProcessStartTimes.Remove(newLockData.PID);
+            mockTracer.VerifyAll();
+        }
+
+        /// <summary>
+        /// Inverse of the PID-recycle test: when start time still matches, the original holder
+        /// must still be considered the active holder and a new acquisition must be denied.
+        /// Guards against an over-eager orphan detector that would release any lock on every check.
+        /// </summary>
+        [TestCase]
+        public void TryAcquireLockForExternalRequestor_WhenHolderStartTimeMatches()
+        {
+            Mock<ITracer> mockTracer = new Mock<ITracer>(MockBehavior.Strict);
+            mockTracer.Setup(x => x.RelatedEvent(EventLevel.Informational, "TryAcquireLockExternal", It.IsAny<EventMetadata>()));
+            mockTracer.Setup(x => x.RelatedEvent(EventLevel.Verbose, "TryAcquireLockExternal", It.IsAny<EventMetadata>()));
+            MockPlatform mockPlatform = (MockPlatform)GVFSPlatform.Instance;
+
+            mockPlatform.ActiveProcesses.Add(DefaultLockData.PID);
+            mockPlatform.ProcessStartTimes[DefaultLockData.PID] = 1000;
+            GVFSLock gvfsLock = new GVFSLock(mockTracer.Object);
+            NamedPipeMessages.LockData existingExternalHolder;
+            gvfsLock.TryAcquireLockForExternalRequestor(DefaultLockData, out existingExternalHolder).ShouldBeTrue();
+            this.ValidateLockHeld(gvfsLock, DefaultLockData);
+
+            // Start time is unchanged -- holder is genuinely still alive; new acquire must be denied.
+            NamedPipeMessages.LockData newLockData = new NamedPipeMessages.LockData(4321, false, false, "git new", "456");
+            mockPlatform.ActiveProcesses.Add(newLockData.PID);
+            mockPlatform.ProcessStartTimes[newLockData.PID] = 3000;
+            gvfsLock.TryAcquireLockForExternalRequestor(newLockData, out existingExternalHolder).ShouldBeFalse();
+            this.ValidateExistingExternalHolder(DefaultLockData, existingExternalHolder);
+            this.ValidateLockHeld(gvfsLock, DefaultLockData);
+
+            mockPlatform.ActiveProcesses.Remove(DefaultLockData.PID);
+            mockPlatform.ActiveProcesses.Remove(newLockData.PID);
+            mockPlatform.ProcessStartTimes.Remove(DefaultLockData.PID);
+            mockPlatform.ProcessStartTimes.Remove(newLockData.PID);
+            mockTracer.VerifyAll();
+        }
+
         private GVFSLock AcquireDefaultLock(MockPlatform mockPlatform, ITracer mockTracer)
         {
             GVFSLock gvfsLock = new GVFSLock(mockTracer);

GVFS/GVFS.UnitTests/Mock/Common/MockPlatform.cs

@@ -45,6 +45,14 @@ public override bool SupportsSystemInstallLog
 
         public HashSet<int> ActiveProcesses { get; } = new HashSet<int>();
 
+        /// <summary>
+        /// Optional per-PID start-time overrides used by tests that exercise PID-reuse
+        /// detection. When a PID is in <see cref="ActiveProcesses"/> but not in this
+        /// dictionary, <see cref="TryGetActiveProcessStartTime"/> returns the PID itself
+        /// as a synthetic non-zero start time.
+        /// </summary>
+        public Dictionary<int, long> ProcessStartTimes { get; } = new Dictionary<int, long>();
+
         public override void ConfigureVisualStudio(string gitBinPath, ITracer tracer)
         {
             throw new NotSupportedException();
@@ -138,6 +146,21 @@ public override bool IsProcessActive(int processId)
             return this.ActiveProcesses.Contains(processId);
         }
 
+        public override bool TryGetActiveProcessStartTime(int processId, out long startTime)
+        {
+            if (this.ActiveProcesses.Contains(processId))
+            {
+                // If the test populated an explicit start time use that, otherwise fall back
+                // to a deterministic non-zero value so test scenarios that don't care about
+                // PID identity (the existing majority of unit tests) keep working.
+                startTime = this.ProcessStartTimes.TryGetValue(processId, out long stored) ? stored : processId;
+                return true;
+            }
+
+            startTime = 0;
+            return false;
+        }
+
         public override void IsServiceInstalledAndRunning(string name, out bool installed, out bool running)
         {
             throw new NotSupportedException();