Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Commit 8b66ba0

Browse files
tmdsstephentoub
tmds
authored and
stephentoub
committed
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
1 parent b3ba359 commit 8b66ba0

File tree

7 files changed

+75
-39
lines changed

7 files changed

+75
-39
lines changed

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,3 +7,5 @@
77
[assembly: SuppressMessage("Microsoft.Reliability", "CA2002:DoNotLockOnObjectsWithWeakIdentity", Scope = "member", Target = "System.Diagnostics.Process.#EnsureWatchingForExit()")]
88
[assembly: SuppressMessage("Microsoft.Reliability", "CA2002:DoNotLockOnObjectsWithWeakIdentity", Scope = "member", Target = "System.Diagnostics.Process.#RaiseOnExited()")]
99
[assembly: SuppressMessage("Microsoft.Reliability", "CA2002:DoNotLockOnObjectsWithWeakIdentity", Scope = "member", Target = "System.Diagnostics.Process.#StopWatchingForExit()")]
10+
[assembly: SuppressMessage("Microsoft.Reliability", "CA2002:DoNotLockOnObjectsWithWeakIdentity", Scope = "member", Target = "System.Diagnostics.Process.#CompletionCallback(System.Object,System.Boolean)")]
11+
[assembly: SuppressMessage("Microsoft.Reliability", "CA2002:DoNotLockOnObjectsWithWeakIdentity", Scope = "member", Target = "System.Diagnostics.Process.Close()")]

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,8 @@ partial void ConfigureAfterProcessIdSet()
8484
{
8585
// Make sure that we configure the wait state holder for this process object, which we can only do once we have a process ID.
8686
Debug.Assert(_haveProcessId, $"{nameof(ConfigureAfterProcessIdSet)} should only be called once a process ID is set");
87-
GetWaitState(); // lazily initializes the wait state
87+
// Initialize WaitStateHolder for non-child processes
88+
GetWaitState();
8889
}
8990

9091
/// <devdoc>
@@ -104,9 +105,9 @@ private void EnsureWatchingForExit()
104105
_watchingForExit = true;
105106
try
106107
{
107-
_waitHandle = new ProcessWaitHandle(_processHandle);
108+
_waitHandle = new ProcessWaitHandle(GetWaitState());
108109
_registeredWaitHandle = ThreadPool.RegisterWaitForSingleObject(_waitHandle,
109-
new WaitOrTimerCallback(CompletionCallback), null, -1, true);
110+
new WaitOrTimerCallback(CompletionCallback), _waitHandle, -1, true);
110111
}
111112
catch
112113
{

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ private void EnsureWatchingForExit()
126126
{
127127
_waitHandle = new Interop.Kernel32.ProcessWaitHandle(_processHandle);
128128
_registeredWaitHandle = ThreadPool.RegisterWaitForSingleObject(_waitHandle,
129-
new WaitOrTimerCallback(CompletionCallback), null, -1, true);
129+
new WaitOrTimerCallback(CompletionCallback), _waitHandle, -1, true);
130130
}
131131
catch
132132
{

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

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -782,10 +782,20 @@ public event EventHandler Exited
782782
/// This is called from the threadpool when a process exits.
783783
/// </devdoc>
784784
/// <internalonly/>
785-
private void CompletionCallback(object context, bool wasSignaled)
785+
private void CompletionCallback(object waitHandleContext, bool wasSignaled)
786786
{
787-
StopWatchingForExit();
788-
RaiseOnExited();
787+
Debug.Assert(waitHandleContext != null, "Process.CompletionCallback called with no waitHandleContext");
788+
lock (this)
789+
{
790+
// Check the exited event that we get from the threadpool
791+
// matches the event we are waiting for.
792+
if (waitHandleContext != _waitHandle)
793+
{
794+
return;
795+
}
796+
StopWatchingForExit();
797+
RaiseOnExited();
798+
}
789799
}
790800

791801
/// <internalonly/>
@@ -835,7 +845,14 @@ public void Close()
835845
{
836846
if (_haveProcessHandle)
837847
{
838-
StopWatchingForExit();
848+
// We need to lock to ensure we don't run concurrently with CompletionCallback.
849+
// Without this lock we could reset _raisedOnExited which causes CompletionCallback to
850+
// raise the Exited event a second time for the same process.
851+
lock (this)
852+
{
853+
// This sets _waitHandle to null which causes CompletionCallback to not emit events.
854+
StopWatchingForExit();
855+
}
839856
#if FEATURE_TRACESWITCH
840857
Debug.WriteLineIf(_processTracing.TraceVerbose, "Process - CloseHandle(process) in Close()");
841858
#endif

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

Lines changed: 2 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -9,41 +9,19 @@ namespace System.Diagnostics
99
{
1010
internal sealed class ProcessWaitHandle : WaitHandle
1111
{
12-
/// <summary>
13-
/// Holds a wait state object associated with this handle.
14-
/// </summary>
15-
private ProcessWaitState.Holder _waitStateHolder;
16-
17-
internal ProcessWaitHandle(SafeProcessHandle processHandle)
12+
internal ProcessWaitHandle(ProcessWaitState processWaitState)
1813
{
19-
// Get the process ID from the process handle. The handle is just a facade that wraps
20-
// the process ID, and closing the handle won't affect the process or its ID at all.
21-
// So we can grab it, and it's not "dangerous".
22-
int processId = (int)processHandle.DangerousGetHandle();
23-
24-
// Create a wait state holder for this process ID. This gives us access to the shared
25-
// wait state associated with this process.
26-
_waitStateHolder = new ProcessWaitState.Holder(processId);
27-
2814
// Get the wait state's event, and use that event's safe wait handle
2915
// in place of ours. This will let code register for completion notifications
3016
// on this ProcessWaitHandle and be notified when the wait state's handle completes.
31-
ManualResetEvent mre = _waitStateHolder._state.EnsureExitedEvent();
17+
ManualResetEvent mre = processWaitState.EnsureExitedEvent();
3218
this.SetSafeWaitHandle(mre.GetSafeWaitHandle());
3319
}
3420

3521
protected override void Dispose(bool explicitDisposing)
3622
{
3723
// ProcessWaitState will dispose the handle
3824
this.SafeWaitHandle = null;
39-
if (explicitDisposing)
40-
{
41-
if (_waitStateHolder != null)
42-
{
43-
_waitStateHolder.Dispose();
44-
_waitStateHolder = null;
45-
}
46-
}
4725
base.Dispose(explicitDisposing);
4826
}
4927
}

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

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,9 @@ internal static ProcessWaitState AddRef(int processId, bool isNewChild)
114114
ProcessWaitState pws;
115115
if (isNewChild)
116116
{
117+
// When the PID is recycled for a new child, we remove the old child.
118+
s_childProcessWaitStates.Remove(processId);
119+
117120
pws = new ProcessWaitState(processId, isChild: true);
118121
s_childProcessWaitStates.Add(processId, pws);
119122
pws._outstandingRefCount++; // For Holder
@@ -142,22 +145,25 @@ internal static ProcessWaitState AddRef(int processId, bool isNewChild)
142145
/// Decrements the ref count on the wait state object, and if it's the last one,
143146
/// removes it from the table.
144147
/// </summary>
145-
internal bool ReleaseRef()
148+
internal void ReleaseRef()
146149
{
147150
ProcessWaitState pws;
148151
Dictionary<int, ProcessWaitState> waitStates = _isChild ? s_childProcessWaitStates : s_processWaitStates;
149-
bool removed = false;
150152
lock (waitStates)
151153
{
152154
bool foundState = waitStates.TryGetValue(_processId, out pws);
153155
Debug.Assert(foundState);
154156
if (foundState)
155157
{
156-
--pws._outstandingRefCount;
157-
if (pws._outstandingRefCount == 0)
158+
--_outstandingRefCount;
159+
if (_outstandingRefCount == 0)
158160
{
159-
waitStates.Remove(_processId);
160-
removed = true;
161+
// The dictionary may contain a different ProcessWaitState if the pid was recycled.
162+
if (pws == this)
163+
{
164+
waitStates.Remove(_processId);
165+
}
166+
pws = this;
161167
}
162168
else
163169
{
@@ -166,7 +172,6 @@ internal bool ReleaseRef()
166172
}
167173
}
168174
pws?.Dispose();
169-
return removed;
170175
}
171176

172177
/// <summary>

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -589,6 +589,39 @@ public async Task TestProcessWaitStateReferenceCount()
589589
}
590590
}
591591

592+
/// <summary>
593+
/// Verifies a new Process instance can refer to a process with a recycled pid
594+
/// for which there is still an existing Process instance.
595+
/// </summary>
596+
[ConditionalFact(typeof(TestEnvironment), nameof(TestEnvironment.IsStressModeEnabled))]
597+
public void TestProcessRecycledPid()
598+
{
599+
const int LinuxPidMaxDefault = 32768;
600+
var processes = new Dictionary<int, Process>(LinuxPidMaxDefault);
601+
bool foundRecycled = false;
602+
for (int i = 0; i < int.MaxValue; i++)
603+
{
604+
var process = CreateProcessLong();
605+
process.Start();
606+
607+
foundRecycled = processes.ContainsKey(process.Id);
608+
609+
process.Kill();
610+
process.WaitForExit();
611+
612+
if (foundRecycled)
613+
{
614+
break;
615+
}
616+
else
617+
{
618+
processes.Add(process.Id, process);
619+
}
620+
}
621+
622+
Assert.True(foundRecycled);
623+
}
624+
592625
private static IDictionary GetWaitStateDictionary(bool childDictionary)
593626
{
594627
Assembly assembly = typeof(Process).Assembly;

0 commit comments

Comments
 (0)