Handle recycled child PIDs (#27763)

* ProcessWaitState: support recycled child pids

* Ensure ProcessWaitHandle uses same ProcessWaitState as Process

* Fix race between Close and CompletionCallback

* Revert GetWaitState

* Add test

* Handle race between Close and CompletionCallback

7 files changed, 75 insertions, 39 deletions

diff --git a/src/System.Diagnostics.Process/src/FxCopBaseline.cs b/src/System.Diagnostics.Process/src/FxCopBaseline.cs
index 2e370b51d3..1d3b6208e1 100644
--- a/src/System.Diagnostics.Process/src/FxCopBaseline.cs
+++ b/src/System.Diagnostics.Process/src/FxCopBaseline.cs
@@ -7,3 +7,5 @@ using System.Diagnostics.CodeAnalysis;
 [assembly: SuppressMessage("Microsoft.Reliability", "CA2002:DoNotLockOnObjectsWithWeakIdentity", Scope = "member", Target = "System.Diagnostics.Process.#EnsureWatchingForExit()")]
 [assembly: SuppressMessage("Microsoft.Reliability", "CA2002:DoNotLockOnObjectsWithWeakIdentity", Scope = "member", Target = "System.Diagnostics.Process.#RaiseOnExited()")]
 [assembly: SuppressMessage("Microsoft.Reliability", "CA2002:DoNotLockOnObjectsWithWeakIdentity", Scope = "member", Target = "System.Diagnostics.Process.#StopWatchingForExit()")]
+[assembly: SuppressMessage("Microsoft.Reliability", "CA2002:DoNotLockOnObjectsWithWeakIdentity", Scope = "member", Target = "System.Diagnostics.Process.#CompletionCallback(System.Object,System.Boolean)")]
+[assembly: SuppressMessage("Microsoft.Reliability", "CA2002:DoNotLockOnObjectsWithWeakIdentity", Scope = "member", Target = "System.Diagnostics.Process.Close()")]
\ No newline at end of file
diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs
index 82800ac9ac..6705a44129 100644
--- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs
+++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs
@@ -84,7 +84,8 @@ namespace System.Diagnostics
         {
             // Make sure that we configure the wait state holder for this process object, which we can only do once we have a process ID.
             Debug.Assert(_haveProcessId, $"{nameof(ConfigureAfterProcessIdSet)} should only be called once a process ID is set");
-            GetWaitState(); // lazily initializes the wait state
+            // Initialize WaitStateHolder for non-child processes
+            GetWaitState();
         }
 
         /// <devdoc>
@@ -104,9 +105,9 @@ namespace System.Diagnostics
                         _watchingForExit = true;
                         try
                         {
-                            _waitHandle = new ProcessWaitHandle(_processHandle);
+                            _waitHandle = new ProcessWaitHandle(GetWaitState());
                             _registeredWaitHandle = ThreadPool.RegisterWaitForSingleObject(_waitHandle,
-                                new WaitOrTimerCallback(CompletionCallback), null, -1, true);
+                                new WaitOrTimerCallback(CompletionCallback), _waitHandle, -1, true);
                         }
                         catch
                         {
diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs
index d3bc24b578..e9cf343d28 100644
--- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs
+++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs
@@ -126,7 +126,7 @@ namespace System.Diagnostics
                         {
                             _waitHandle = new Interop.Kernel32.ProcessWaitHandle(_processHandle);
                             _registeredWaitHandle = ThreadPool.RegisterWaitForSingleObject(_waitHandle,
-                                new WaitOrTimerCallback(CompletionCallback), null, -1, true);
+                                new WaitOrTimerCallback(CompletionCallback), _waitHandle, -1, true);
                         }
                         catch
                         {
diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs
index 2f647de698..71fdfc59ac 100644
--- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs
+++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs
@@ -782,10 +782,20 @@ namespace System.Diagnostics
         ///     This is called from the threadpool when a process exits.
         /// </devdoc>
         /// <internalonly/>
-        private void CompletionCallback(object context, bool wasSignaled)
+        private void CompletionCallback(object waitHandleContext, bool wasSignaled)
         {
-            StopWatchingForExit();
-            RaiseOnExited();
+            Debug.Assert(waitHandleContext != null, "Process.CompletionCallback called with no waitHandleContext");
+            lock (this)
+            {
+                // Check the exited event that we get from the threadpool
+                // matches the event we are waiting for.
+                if (waitHandleContext != _waitHandle)
+                {
+                    return;
+                }
+                StopWatchingForExit();
+                RaiseOnExited();
+            }
         }
 
         /// <internalonly/>
@@ -835,7 +845,14 @@ namespace System.Diagnostics
             {
                 if (_haveProcessHandle)
                 {
-                    StopWatchingForExit();
+                    // We need to lock to ensure we don't run concurrently with CompletionCallback.
+                    // Without this lock we could reset _raisedOnExited which causes CompletionCallback to
+                    // raise the Exited event a second time for the same process.
+                    lock (this)
+                    {
+                        // This sets _waitHandle to null which causes CompletionCallback to not emit events.
+                        StopWatchingForExit();
+                    }
 #if FEATURE_TRACESWITCH
                     Debug.WriteLineIf(_processTracing.TraceVerbose, "Process - CloseHandle(process) in Close()");
 #endif
diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitHandle.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitHandle.Unix.cs
index 2f9270e175..8ce139ca5b 100644
--- a/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitHandle.Unix.cs
+++ b/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitHandle.Unix.cs
@@ -9,26 +9,12 @@ namespace System.Diagnostics
 {
     internal sealed class ProcessWaitHandle : WaitHandle
     {
-        /// <summary>
-        /// Holds a wait state object associated with this handle.
-        /// </summary>
-        private ProcessWaitState.Holder _waitStateHolder;
-
-        internal ProcessWaitHandle(SafeProcessHandle processHandle)
+        internal ProcessWaitHandle(ProcessWaitState processWaitState)
         {
-            // Get the process ID from the process handle.  The handle is just a facade that wraps
-            // the process ID, and closing the handle won't affect the process or its ID at all.
-            // So we can grab it, and it's not "dangerous".
-            int processId = (int)processHandle.DangerousGetHandle();
-
-            // Create a wait state holder for this process ID.  This gives us access to the shared
-            // wait state associated with this process.
-            _waitStateHolder = new ProcessWaitState.Holder(processId);
-
             // Get the wait state's event, and use that event's safe wait handle
             // in place of ours.  This will let code register for completion notifications
             // on this ProcessWaitHandle and be notified when the wait state's handle completes.
-            ManualResetEvent mre = _waitStateHolder._state.EnsureExitedEvent();
+            ManualResetEvent mre = processWaitState.EnsureExitedEvent();
             this.SetSafeWaitHandle(mre.GetSafeWaitHandle());
         }
 
@@ -36,14 +22,6 @@ namespace System.Diagnostics
         {
             // ProcessWaitState will dispose the handle
             this.SafeWaitHandle = null;
-            if (explicitDisposing)
-            {
-                if (_waitStateHolder != null)
-                {
-                    _waitStateHolder.Dispose();
-                    _waitStateHolder = null;
-                }
-            }
             base.Dispose(explicitDisposing);
         }
     }
diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs
index b2d689c300..f9ab4d6475 100644
--- a/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs
+++ b/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs
@@ -114,6 +114,9 @@ namespace System.Diagnostics
                 ProcessWaitState pws;
                 if (isNewChild)
                 {
+                    // When the PID is recycled for a new child, we remove the old child.
+                    s_childProcessWaitStates.Remove(processId);
+
                     pws = new ProcessWaitState(processId, isChild: true);
                     s_childProcessWaitStates.Add(processId, pws);
                     pws._outstandingRefCount++; // For Holder
@@ -142,22 +145,25 @@ namespace System.Diagnostics
         /// Decrements the ref count on the wait state object, and if it's the last one,
         /// removes it from the table.
         /// </summary>
-        internal bool ReleaseRef()
+        internal void ReleaseRef()
         {
             ProcessWaitState pws;
             Dictionary<int, ProcessWaitState> waitStates = _isChild ? s_childProcessWaitStates : s_processWaitStates;
-            bool removed = false;
             lock (waitStates)
             {
                 bool foundState = waitStates.TryGetValue(_processId, out pws);
                 Debug.Assert(foundState);
                 if (foundState)
                 {
-                    --pws._outstandingRefCount;
-                    if (pws._outstandingRefCount == 0)
+                    --_outstandingRefCount;
+                    if (_outstandingRefCount == 0)
                     {
-                        waitStates.Remove(_processId);
-                        removed = true;
+                        // The dictionary may contain a different ProcessWaitState if the pid was recycled.
+                        if (pws == this)
+                        {
+                            waitStates.Remove(_processId);
+                        }
+                        pws = this;
                     }
                     else
                     {
@@ -166,7 +172,6 @@ namespace System.Diagnostics
                 }
             }
             pws?.Dispose();
-            return removed;
         }
 
         /// <summary>
diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs
index 479e593601..fcea38b05c 100644
--- a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs
+++ b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs
@@ -589,6 +589,39 @@ namespace System.Diagnostics.Tests
             }
         }
 
+        /// <summary>
+        /// Verifies a new Process instance can refer to a process with a recycled pid
+        /// for which there is still an existing Process instance.
+        /// </summary>
+        [ConditionalFact(typeof(TestEnvironment), nameof(TestEnvironment.IsStressModeEnabled))]
+        public void TestProcessRecycledPid()
+        {
+            const int LinuxPidMaxDefault = 32768;
+            var processes = new Dictionary<int, Process>(LinuxPidMaxDefault);
+            bool foundRecycled = false;
+            for (int i = 0; i < int.MaxValue; i++)
+            {
+                var process = CreateProcessLong();
+                process.Start();
+
+                foundRecycled = processes.ContainsKey(process.Id);
+
+                process.Kill();
+                process.WaitForExit();
+
+                if (foundRecycled)
+                {
+                    break;
+                }
+                else
+                {
+                    processes.Add(process.Id, process);
+                }
+            }
+
+            Assert.True(foundRecycled);
+        }
+
         private static IDictionary GetWaitStateDictionary(bool childDictionary)
         {
             Assembly assembly = typeof(Process).Assembly;