From 0d4a49c025db5d012e2f78d75407d262ddd3c0b9 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Mon, 5 Mar 2018 17:17:47 +0100 Subject: [PATCH 1/6] ProcessWaitState: support recycled child pids --- .../Diagnostics/ProcessWaitState.Unix.cs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) 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 b2d689c3004e..f9ab4d64752d 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 @@ 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. /// - internal bool ReleaseRef() + internal void ReleaseRef() { ProcessWaitState pws; Dictionary 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; } /// From 4fff0f40d028242a6c1fe60e5a38889ed81e3d32 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 6 Mar 2018 10:19:44 +0100 Subject: [PATCH 2/6] Ensure ProcessWaitHandle uses same ProcessWaitState as Process --- .../src/System/Diagnostics/Process.Unix.cs | 15 +++++++---- .../src/System/Diagnostics/Process.cs | 3 +-- .../Diagnostics/ProcessWaitHandle.Unix.cs | 26 ++----------------- 3 files changed, 13 insertions(+), 31 deletions(-) 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 63db0a871fdd..60aa67ad02bf 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,11 @@ 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 + if (_waitStateHolder == null) + { + _waitStateHolder = new ProcessWaitState.Holder(_processId); + } } /// @@ -104,7 +108,7 @@ private void EnsureWatchingForExit() _watchingForExit = true; try { - _waitHandle = new ProcessWaitHandle(_processHandle); + _waitHandle = new ProcessWaitHandle(GetWaitState()); _registeredWaitHandle = ThreadPool.RegisterWaitForSingleObject(_waitHandle, new WaitOrTimerCallback(CompletionCallback), null, -1, true); } @@ -620,12 +624,13 @@ private static string GetNextArgument(string arguments, ref int i) /// Gets the wait state for this Process object. private ProcessWaitState GetWaitState() { - if (_waitStateHolder == null) + ProcessWaitState waitState = _waitStateHolder?._state; + if (waitState == null) { + // This will throw EnsureState(State.HaveId); - _waitStateHolder = new ProcessWaitState.Holder(_processId); } - return _waitStateHolder._state; + return waitState; } private static (uint userId, uint groupId) GetUserAndGroupIds(ProcessStartInfo startInfo) diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs index 1509b480032f..58cb90395c1b 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs @@ -117,10 +117,9 @@ private Process(string machineName, bool isRemoteMachine, int processId, Process _processInfo = processInfo; _machineName = machineName; _isRemoteMachine = isRemoteMachine; - _processId = processId; - _haveProcessId = true; _outputStreamReadMode = StreamReadMode.Undefined; _errorStreamReadMode = StreamReadMode.Undefined; + SetProcessId(processId); } public SafeProcessHandle SafeHandle 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 2f9270e1754f..8ce139ca5b59 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 { - /// - /// Holds a wait state object associated with this handle. - /// - 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 @@ 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); } } From af7513af391ad57f5ca8ac4ad22982ac1d5c1b35 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 6 Mar 2018 19:32:24 +0100 Subject: [PATCH 3/6] Fix race between Close and CompletionCallback --- .../src/FxCopBaseline.cs | 1 + .../src/System/Diagnostics/Process.Unix.cs | 2 +- .../src/System/Diagnostics/Process.Windows.cs | 2 +- .../src/System/Diagnostics/Process.cs | 16 +++++++++++++--- 4 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/System.Diagnostics.Process/src/FxCopBaseline.cs b/src/System.Diagnostics.Process/src/FxCopBaseline.cs index 2e370b51d3c9..e660ef168e86 100644 --- a/src/System.Diagnostics.Process/src/FxCopBaseline.cs +++ b/src/System.Diagnostics.Process/src/FxCopBaseline.cs @@ -7,3 +7,4 @@ [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)")] \ 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 60aa67ad02bf..539ca01bb419 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs @@ -110,7 +110,7 @@ private void EnsureWatchingForExit() { _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 1b70bc734741..c1fb4fc7d1d3 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 @@ 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 { diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs index 58cb90395c1b..34379f5e854e 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 @@ public event EventHandler Exited /// This is called from the threadpool when a process exits. /// /// - 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(); + } } /// From 861eae0834ea617d164a6a3778aefd907af16c69 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Mon, 12 Mar 2018 16:16:05 +0100 Subject: [PATCH 4/6] Revert GetWaitState --- .../src/System/Diagnostics/Process.Unix.cs | 12 ++++-------- .../src/System/Diagnostics/Process.cs | 3 ++- 2 files changed, 6 insertions(+), 9 deletions(-) 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 539ca01bb419..5692e471e2a2 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs @@ -85,10 +85,7 @@ 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"); // Initialize WaitStateHolder for non-child processes - if (_waitStateHolder == null) - { - _waitStateHolder = new ProcessWaitState.Holder(_processId); - } + GetWaitState(); } /// @@ -624,13 +621,12 @@ private static string GetNextArgument(string arguments, ref int i) /// Gets the wait state for this Process object. private ProcessWaitState GetWaitState() { - ProcessWaitState waitState = _waitStateHolder?._state; - if (waitState == null) + if (_waitStateHolder == null) { - // This will throw EnsureState(State.HaveId); + _waitStateHolder = new ProcessWaitState.Holder(_processId); } - return waitState; + return _waitStateHolder._state; } private static (uint userId, uint groupId) GetUserAndGroupIds(ProcessStartInfo startInfo) diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs index 34379f5e854e..44861734e24c 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs @@ -117,9 +117,10 @@ private Process(string machineName, bool isRemoteMachine, int processId, Process _processInfo = processInfo; _machineName = machineName; _isRemoteMachine = isRemoteMachine; + _processId = processId; + _haveProcessId = true; _outputStreamReadMode = StreamReadMode.Undefined; _errorStreamReadMode = StreamReadMode.Undefined; - SetProcessId(processId); } public SafeProcessHandle SafeHandle From b41ef1f066ba8dbda89f54a0e6162d8201041f8a Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 27 Mar 2018 09:38:16 +0200 Subject: [PATCH 5/6] Add test --- .../tests/ProcessTests.Unix.cs | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs index 617d1b7cf699..cab7f8e8734b 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs @@ -515,6 +515,39 @@ public void TestProcessWaitStateReferenceCount() } } + /// + /// Verifies a new Process instance can refer to a process with a recycled pid + /// for which there is still an existing Process instance. + /// + [ConditionalFact(typeof(TestEnvironment), nameof(TestEnvironment.IsStressModeEnabled))] + public void TestProcessRecycledPid() + { + const int LinuxPidMaxDefault = 32768; + var processes = new Dictionary(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; From 0de56c5d597cacb647cfc925755918f846e75c4e Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 3 Apr 2018 21:40:20 +0200 Subject: [PATCH 6/6] Handle race between Close and CompletionCallback --- src/System.Diagnostics.Process/src/FxCopBaseline.cs | 3 ++- .../src/System/Diagnostics/Process.cs | 9 ++++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/System.Diagnostics.Process/src/FxCopBaseline.cs b/src/System.Diagnostics.Process/src/FxCopBaseline.cs index e660ef168e86..1d3b6208e1bb 100644 --- a/src/System.Diagnostics.Process/src/FxCopBaseline.cs +++ b/src/System.Diagnostics.Process/src/FxCopBaseline.cs @@ -7,4 +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)")] \ No newline at end of file +[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.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs index 44861734e24c..addc1bd72a82 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs @@ -846,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