Handle recycled child PIDs

src/System.Diagnostics.Process/src/FxCopBaseline.cs

@@ -7,3 +7,5 @@
 [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()")]

src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs

@@ -84,7 +84,8 @@ partial void ConfigureAfterProcessIdSet()
         {
             // 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 @@ private void EnsureWatchingForExit()
                         _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
                         {

src/System.Diagnostics.Process/src/System/Diagnostics/Process.Windows.cs

@@ -126,7 +126,7 @@ private void EnsureWatchingForExit()
                         {
                             _waitHandle = new Interop.Kernel32.ProcessWaitHandle(_processHandle);
                             _registeredWaitHandle = ThreadPool.RegisterWaitForSingleObject(_waitHandle,
-                                new WaitOrTimerCallback(CompletionCallback), null, -1, true);
+                                new WaitOrTimerCallback(CompletionCallback), _waitHandle, -1, true);
                         }
                         catch
                         {

src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs

@@ -783,10 +783,20 @@ public event EventHandler Exited
         ///     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/>
@@ -836,7 +846,14 @@ public void Close()
             {
                 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

src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitHandle.Unix.cs

@@ -9,41 +9,19 @@ 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());
         }
 
         protected override void Dispose(bool explicitDisposing)
         {
             // ProcessWaitState will dispose the handle
             this.SafeWaitHandle = null;
-            if (explicitDisposing)
-            {
-                if (_waitStateHolder != null)
-                {
-                    _waitStateHolder.Dispose();
-                    _waitStateHolder = null;
-                }
-            }
             base.Dispose(explicitDisposing);
         }
     }

src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs

@@ -114,6 +114,9 @@ internal static ProcessWaitState AddRef(int processId, bool isNewChild)
                 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 @@ internal static ProcessWaitState AddRef(int processId, bool isNewChild)
         /// 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 @@ internal bool ReleaseRef()
                 }
             }
             pws?.Dispose();
-            return removed;
         }
 
         /// <summary>

src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs

@@ -515,6 +515,39 @@ public void TestProcessWaitStateReferenceCount()
             }
         }
 
+        /// <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;