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

Process: avoid performing operations on a different process with recycled pid #28404

Merged
stephentoub merged 8 commits into from
May 29, 2018
Merged

Process: avoid performing operations on a different process with recycled pid #28404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
738d1c0
Process: avoid performing operations on a different process with recy…
tmds Mar 23, 2018
94cf9a9
ThrowIfExited: don't create ProcessWaitState(Holder) if we are not re…
tmds Mar 23, 2018
badc2f3
Handle non-child pid recycling
tmds Mar 23, 2018
909b124
ProcessWaitState: update overview comment to reflect current implemen…
tmds Mar 23, 2018
836e3cf
Merge remote-tracking branch 'upstream/master' into check_exited
tmds Apr 5, 2018
48dbcec
Update TestProcessRecycledPid
tmds Apr 5, 2018
ea97178
Add comment to ThrowIfExited
tmds May 29, 2018
dab279d
Remove CultureInfo.CurrentCulture from _processId.ToString
tmds May 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/System.Diagnostics.Process/src/System/Diagnostics/Process.FreeBSD.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
}
Expand All @@ -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);
Expand All @@ -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);
Expand Down
11 changes: 8 additions & 3 deletions src/System.Diagnostics.Process/src/System/Diagnostics/Process.Linux.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
{
Expand All @@ -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)
Expand All @@ -178,7 +183,7 @@ private unsafe IntPtr ProcessorAffinityCore
}
set
{
EnsureState(State.HaveId);
EnsureState(State.HaveNonExitedId);

if (Interop.Sys.SchedSetAffinity(_processId, ref value) != 0)
{
Expand Down Expand Up @@ -247,7 +252,7 @@ internal static string GetExePath(int processId = -1)
/// <summary>Reads the stats information for this process from the procfs file system.</summary>
private Interop.procfs.ParsedStat GetStat()
{
EnsureState(State.HaveId);
EnsureState(State.HaveNonExitedId);
Interop.procfs.ParsedStat stat;
if (!Interop.procfs.TryReadStatFile(_processId, out stat, new ReusableTextReader()))
{
Expand Down
8 changes: 4 additions & 4 deletions src/System.Diagnostics.Process/src/System/Diagnostics/Process.OSX.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand All @@ -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);
Expand Down Expand Up @@ -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));
}
Expand All @@ -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));
}
Expand Down
30 changes: 22 additions & 8 deletions src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public static Process Start(string fileName, string arguments, string userName,
/// <summary>Stops the associated process immediately.</summary>
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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -211,6 +211,8 @@ private ProcessPriorityClass PriorityClassCore
}
set
{
EnsureState(State.HaveNonExitedId);

int pri = 0; // Normal
switch (value)
{
Expand Down Expand Up @@ -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))
Copy link
Copy Markdown
Member

@stephentoub stephentoub May 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe combine the above into:

if ((_waitStateHolder != null || refresh) && GetWaitState().GetExited(out _, refresh))

?

{
throw new InvalidOperationException(SR.Format(SR.ProcessHasExited, _processId.ToString()));
}
}

/// <summary>
/// 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.
Expand All @@ -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);
}

Expand Down
16 changes: 14 additions & 2 deletions src/System.Diagnostics.Process/src/System/Diagnostics/Process.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);

/// <devdoc>
/// Helper method for checking preconditions when accessing properties.
/// </devdoc>
Expand All @@ -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)
Expand All @@ -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)
{
Expand Down Expand Up @@ -1514,6 +1525,7 @@ private enum State
{
HaveId = 0x1,
IsLocal = 0x2,
HaveNonExitedId = HaveId | 0x4,
Copy link
Copy Markdown
Member

@stephentoub stephentoub Mar 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was nice of Nt to leave a gap here :)

HaveProcessInfo = 0x8,
Exited = 0x10,
Associated = 0x20,
Expand Down
Loading