diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.FreeBSD.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.FreeBSD.cs index e25217e82114..91a8646d8046 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.FreeBSD.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.FreeBSD.cs @@ -14,7 +14,7 @@ internal DateTime StartTimeCore { get { - EnsureState(State.HaveId); + EnsureState(State.HaveNonExitedId); Interop.Process.proc_stats stat = Interop.Process.GetThreadInfo(_processId, 0); return new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc).AddSeconds(stat.startTime); @@ -30,7 +30,7 @@ public TimeSpan TotalProcessorTime { get { - EnsureState(State.HaveId); + EnsureState(State.HaveNonExitedId); Interop.Process.proc_stats stat = Interop.Process.GetThreadInfo(_processId, 0); return Process.TicksToTimeSpan(stat.userTime + stat.systemTime); } @@ -44,7 +44,7 @@ public TimeSpan UserProcessorTime { get { - EnsureState(State.HaveId); + EnsureState(State.HaveNonExitedId); Interop.Process.proc_stats stat = Interop.Process.GetThreadInfo(_processId, 0); return Process.TicksToTimeSpan(stat.userTime); @@ -56,7 +56,7 @@ public TimeSpan PrivilegedProcessorTime { get { - EnsureState(State.HaveId); + EnsureState(State.HaveNonExitedId); Interop.Process.proc_stats stat = Interop.Process.GetThreadInfo(_processId, 0); return Process.TicksToTimeSpan(stat.systemTime); diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Linux.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Linux.cs index 829ea2e9f068..718e1e076592 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Linux.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Linux.cs @@ -145,6 +145,11 @@ partial void EnsureHandleCountPopulated() { if (_processInfo.HandleCount <= 0 && _haveProcessId) { + // Don't get information for a PID that exited and has possibly been recycled. + if (GetWaitState().GetExited(out _, refresh: false)) + { + return; + } string path = Interop.procfs.GetFileDescriptorDirectoryPathForProcess(_processId); if (Directory.Exists(path)) { @@ -166,7 +171,7 @@ private unsafe IntPtr ProcessorAffinityCore { get { - EnsureState(State.HaveId); + EnsureState(State.HaveNonExitedId); IntPtr set; if (Interop.Sys.SchedGetAffinity(_processId, out set) != 0) @@ -178,7 +183,7 @@ private unsafe IntPtr ProcessorAffinityCore } set { - EnsureState(State.HaveId); + EnsureState(State.HaveNonExitedId); if (Interop.Sys.SchedSetAffinity(_processId, ref value) != 0) { @@ -247,7 +252,7 @@ internal static string GetExePath(int processId = -1) /// Reads the stats information for this process from the procfs file system. private Interop.procfs.ParsedStat GetStat() { - EnsureState(State.HaveId); + EnsureState(State.HaveNonExitedId); Interop.procfs.ParsedStat stat; if (!Interop.procfs.TryReadStatFile(_processId, out stat, new ReusableTextReader())) { diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.OSX.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.OSX.cs index 750b8ebe2f4a..12023995e1e0 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.OSX.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.OSX.cs @@ -14,7 +14,7 @@ public TimeSpan PrivilegedProcessorTime { get { - EnsureState(State.HaveId); + EnsureState(State.HaveNonExitedId); Interop.libproc.rusage_info_v3 info = Interop.libproc.proc_pid_rusage(_processId); return new TimeSpan(Convert.ToInt64(info.ri_system_time)); } @@ -41,7 +41,7 @@ internal DateTime StartTimeCore // mach_timebase_info() which give us the factor. Then we multiply the factor by the absolute time and the divide // the result by 10^9 to convert it from nanoseconds to seconds. - EnsureState(State.HaveId); + EnsureState(State.HaveNonExitedId); uint numer, denom; Interop.Sys.GetTimebaseInfo(out numer, out denom); @@ -74,7 +74,7 @@ public TimeSpan TotalProcessorTime { get { - EnsureState(State.HaveId); + EnsureState(State.HaveNonExitedId); Interop.libproc.rusage_info_v3 info = Interop.libproc.proc_pid_rusage(_processId); return new TimeSpan(Convert.ToInt64(info.ri_system_time + info.ri_user_time)); } @@ -88,7 +88,7 @@ public TimeSpan UserProcessorTime { get { - EnsureState(State.HaveId); + EnsureState(State.HaveNonExitedId); Interop.libproc.rusage_info_v3 info = Interop.libproc.proc_pid_rusage(_processId); return new TimeSpan(Convert.ToInt64(info.ri_user_time)); } 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 6705a4412939..4bfc348e4485 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs @@ -56,7 +56,7 @@ public static Process Start(string fileName, string arguments, string userName, /// Stops the associated process immediately. public void Kill() { - EnsureState(State.HaveId); + EnsureState(State.HaveNonExitedId); if (Interop.Sys.Kill(_processId, Interop.Sys.Signals.SIGKILL) != 0) { throw new Win32Exception(); // same exception as on Windows @@ -158,7 +158,7 @@ public ProcessModule MainModule private void UpdateHasExited() { int? exitCode; - _exited = GetWaitState().GetExited(out exitCode); + _exited = GetWaitState().GetExited(out exitCode, refresh: true); if (_exited && exitCode != null) { _exitCode = exitCode.Value; @@ -191,7 +191,7 @@ private ProcessPriorityClass PriorityClassCore // and the other values above and below are simply distributed evenly. get { - EnsureState(State.HaveId); + EnsureState(State.HaveNonExitedId); int pri = 0; int errno = Interop.Sys.GetPriority(Interop.Sys.PriorityWhich.PRIO_PROCESS, _processId, out pri); @@ -211,6 +211,8 @@ private ProcessPriorityClass PriorityClassCore } set { + EnsureState(State.HaveNonExitedId); + int pri = 0; // Normal switch (value) { @@ -238,6 +240,20 @@ private static int GetCurrentProcessId() return Interop.Sys.GetPid(); } + partial void ThrowIfExited(bool refresh) + { + // Don't allocate a ProcessWaitState.Holder unless we're refreshing. + if (_waitStateHolder == null && !refresh) + { + return; + } + + if (GetWaitState().GetExited(out _, refresh)) + { + throw new InvalidOperationException(SR.Format(SR.ProcessHasExited, _processId.ToString())); + } + } + /// /// Gets a short-term handle to the process, with the given access. If a handle exists, /// then it is reused. If the process has exited, it throws an exception. @@ -246,14 +262,12 @@ private SafeProcessHandle GetProcessHandle() { if (_haveProcessHandle) { - if (GetWaitState().HasExited) - { - throw new InvalidOperationException(SR.Format(SR.ProcessHasExited, _processId.ToString(CultureInfo.CurrentCulture))); - } + ThrowIfExited(refresh: true); + return _processHandle; } - EnsureState(State.HaveId | State.IsLocal); + EnsureState(State.HaveNonExitedId | State.IsLocal); return new SafeProcessHandle(_processId); } diff --git a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs index 71fdfc59acdb..bd6b87397993 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs @@ -302,7 +302,7 @@ public ProcessModuleCollection Modules { if (_modules == null) { - EnsureState(State.HaveId | State.IsLocal); + EnsureState(State.HaveNonExitedId | State.IsLocal); _modules = ProcessManager.GetModules(_processId); } return _modules; @@ -907,6 +907,10 @@ public void Close() } } + // Checks if the process hasn't exited on Unix systems. + // This is used to detect recycled child PIDs. + partial void ThrowIfExited(bool refresh); + /// /// Helper method for checking preconditions when accessing properties. /// @@ -931,6 +935,10 @@ private void EnsureState(State state) throw new InvalidOperationException(SR.ProcessIdRequired); } } + if ((state & State.HaveNonExitedId) == State.HaveNonExitedId) + { + ThrowIfExited(refresh: false); + } } if ((state & State.IsLocal) != (State)0 && _isRemoteMachine) @@ -942,7 +950,10 @@ private void EnsureState(State state) { if (_processInfo == null) { - if ((state & State.HaveId) == (State)0) EnsureState(State.HaveId); + if ((state & State.HaveNonExitedId) != State.HaveNonExitedId) + { + EnsureState(State.HaveNonExitedId); + } _processInfo = ProcessManager.GetProcessInfo(_processId, _machineName); if (_processInfo == null) { @@ -1514,6 +1525,7 @@ private enum State { HaveId = 0x1, IsLocal = 0x2, + HaveNonExitedId = HaveId | 0x4, HaveProcessInfo = 0x8, Exited = 0x10, Associated = 0x20, 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 f9ab4d64752d..1ed83b847f5e 100644 --- a/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs +++ b/src/System.Diagnostics.Process/src/System/Diagnostics/ProcessWaitState.Unix.cs @@ -12,15 +12,14 @@ namespace System.Diagnostics // Overview // -------- // We have a few constraints we're working under here: - // - waitpid is used on Unix to get the exit status (including exit code) of a child process, but the first call - // to it after the child has completed will reap the child removing the chance of subsequent calls getting status. + // - waitid is used on Unix to get the exit status (including exit code) of a child process, but once a child + // process is reaped, it is no longer possible to get the status. // - The Process design allows for multiple independent Process objects to be handed out, and each of those // objects may be used concurrently with each other, even if they refer to the same underlying process. // Same with ProcessWaitHandle objects. This is based on the Windows design where anyone with a handle to the // process can retrieve completion information about that process. - // - There is no good Unix equivalent to a process handle nor to being able to asynchronously be notified - // of a process' exit (without more intrusive mechanisms like ptrace), which means such support - // needs to be layered on top of waitpid. + // - There is no good Unix equivalent to asynchronously be notified of a non-child process' exit, which means such + // support needs to be layered on top of kill. // // As a result, we have the following scheme: // - We maintain a static/shared table that maps process ID to ProcessWaitState objects. @@ -37,17 +36,10 @@ namespace System.Diagnostics // the wait state object uses its own lock to protect the per-process state. This includes // caching exit / exit code / exit time information so that a Process object for a process that's already // had waitpid called for it can get at its exit information. - // - // A negative ramification of this is that if a process exits, but there are outstanding wait handles - // handed out (and rooted, so they can't be GC'd), and then a new process is created and the pid is recycled, - // new calls to get that process's wait state will get the old process's wait state. However, pid recycling - // will be a more general issue, since pids are the only identifier we have to a process, so if a Process - // object is created for a particular pid, then that process goes away and a new one comes in with the same pid, - // our Process object will silently switch to referring to the new pid. Unix systems typically have a simple - // policy for pid recycling, which is that they start at a low value, increment up to a system maximum (e.g. - // 32768), and then wrap around and start reusing value that aren't currently in use. On Linux, - // proc/sys/kernel/pid_max defines the max pid value. Given the conditions that would be required for this - // to happen, it's possible but unlikely. + // - When we detect a recycled pid, we remove that ProcessWaitState from the table and replace it with a new one + // that represents the new process. For child processes we know a pid is recycled when we see the pid of a new + // child is already in the table. For non-child processes, we assume that a pid may be recycled as soon as + // we've observed it has exited. /// Exit information and waiting capabilities for a process. internal sealed class ProcessWaitState : IDisposable @@ -126,12 +118,32 @@ internal static ProcessWaitState AddRef(int processId, bool isNewChild) { lock (s_processWaitStates) { + DateTime exitTime = default; // We are referencing an existing process. // This may be a child process, so we check s_childProcessWaitStates too. - if (!s_childProcessWaitStates.TryGetValue(processId, out pws) && - !s_processWaitStates.TryGetValue(processId, out pws)) + if (s_childProcessWaitStates.TryGetValue(processId, out pws)) { - pws = new ProcessWaitState(processId, isChild: false); + // child process + } + else if (s_processWaitStates.TryGetValue(processId, out pws)) + { + // This is best effort for dealing with recycled pids for non-child processes. + // As long as we haven't observed process exit, it's safe to share the ProcessWaitState. + // Once we've observed the exit, we'll create a new ProcessWaitState just in case + // this may be a recycled pid. + // If it wasn't, that ProcessWaitState will observe too that the process has exited. + // We pass the ExitTime so it can be the same, but we'll clear it when we see there + // is a live process with that pid. + if (pws.GetExited(out _, refresh: false)) + { + s_processWaitStates.Remove(processId); + exitTime = pws.ExitTime; + pws = null; + } + } + if (pws == null) + { + pws = new ProcessWaitState(processId, isChild: false, exitTime); s_processWaitStates.Add(processId, pws); } pws._outstandingRefCount++; @@ -204,11 +216,12 @@ internal void ReleaseRef() /// Initialize the wait state object. /// The associated process' ID. - private ProcessWaitState(int processId, bool isChild) + private ProcessWaitState(int processId, bool isChild, DateTime exitTime = default) { Debug.Assert(processId >= 0); _processId = processId; _isChild = isChild; + _exitTime = exitTime; } /// Releases managed resources used by the ProcessWaitState. @@ -232,7 +245,10 @@ private void SetExited() Debug.Assert(Monitor.IsEntered(_gate)); _exited = true; - _exitTime = DateTime.Now; + if (_exitTime == default) + { + _exitTime = DateTime.Now; + } _exitedEvent?.Set(); } @@ -285,11 +301,11 @@ internal bool HasExited get { int? ignored; - return GetExited(out ignored); + return GetExited(out ignored, refresh: true); } } - internal bool GetExited(out int? exitCode) + internal bool GetExited(out int? exitCode, bool refresh) { lock (_gate) { @@ -308,9 +324,12 @@ internal bool GetExited(out int? exitCode) return false; } - // We don't know if we've exited, but no one else is currently - // checking, so check. - CheckForNonChildExit(); + if (refresh) + { + // We don't know if we've exited, but no one else is currently + // checking, so check. + CheckForNonChildExit(); + } // We now have an up-to-date snapshot for whether we've exited, // and if we have, what the exit code is (if we were able to find out). @@ -324,6 +343,7 @@ private void CheckForNonChildExit() Debug.Assert(Monitor.IsEntered(_gate)); if (!_isChild) { + bool exited; // We won't be able to get an exit code, but we'll at least be able to determine if the process is // still running. int killResult = Interop.Sys.Kill(_processId, Interop.Sys.Signals.None); // None means don't send a signal @@ -332,7 +352,7 @@ private void CheckForNonChildExit() // Process is still running. This could also be a defunct process that has completed // its work but still has an entry in the processes table due to its parent not yet // having waited on it to clean it up. - return; + exited = false; } else // error from kill { @@ -340,18 +360,27 @@ private void CheckForNonChildExit() if (errno == Interop.Error.ESRCH) { // Couldn't find the process; assume it's exited - SetExited(); - return; + exited = true; } else if (errno == Interop.Error.EPERM) { // Don't have permissions to the process; assume it's alive - return; + exited = false; + } + else + { + Debug.Fail("Unexpected errno value from kill"); + exited = true; } - else Debug.Fail("Unexpected errno value from kill"); } - - SetExited(); + if (exited) + { + SetExited(); + } + else + { + _exitTime = default; + } } } diff --git a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs index fcea38b05c86..5e81697e7329 100644 --- a/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs +++ b/src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs @@ -590,8 +590,9 @@ public async Task TestProcessWaitStateReferenceCount() } /// - /// Verifies a new Process instance can refer to a process with a recycled pid - /// for which there is still an existing Process instance. + /// Verifies a new Process instance can refer to a process with a recycled pid for which + /// there is still an existing Process instance. Operations on the existing instance will + /// throw since that process has exited. /// [ConditionalFact(typeof(TestEnvironment), nameof(TestEnvironment.IsStressModeEnabled))] public void TestProcessRecycledPid() @@ -604,7 +605,12 @@ public void TestProcessRecycledPid() var process = CreateProcessLong(); process.Start(); - foundRecycled = processes.ContainsKey(process.Id); + Process recycled; + foundRecycled = processes.TryGetValue(process.Id, out recycled); + if (foundRecycled) + { + Assert.Throws(() => recycled.Kill()); + } process.Kill(); process.WaitForExit();