Process: avoid performing operations on a different process with recycled pid by tmds · Pull Request #28404 · dotnet/corefx

tmds · 2018-03-23T11:39:59Z

With recent changes to the Process class, we know when our child processes have exited.
We can use this information to ensure a process instance doesn't start hitting a different process with a recycled pid.

Requires #27763

Example code:

            var processes = new Dictionary<int, Process>();
            for (int i = 0; i < 1_000_000; i++)
            {
                if (i % 100 == 0)
                {
                    Console.Write(".");
                }

                var process = new Process();
                process.StartInfo.FileName = "/usr/bin/sleep";
                process.StartInfo.Arguments = "5";
                process.EnableRaisingEvents = true;
                process.Start();

                Process recycled;
                if (processes.TryGetValue(process.Id, out recycled))
                {
                    Console.WriteLine("Killing recycled");
                    recycled.Kill();
                    recycled.WaitForExit();

                    Console.WriteLine("New process exited? " + process.HasExited);
                    return;
                }
                else
                {
                    processes.Add(process.Id, process);
                }

                process.Kill();
                process.WaitForExit();
            }

This code will throw when the Kill operation targets a recycled pid:

System.InvalidOperationException: Cannot process request because the process (12993) has exited.

CC @stephentoub @danmosemsft @wfurt

…cled pid

…freshing

tmds · 2018-03-23T12:45:45Z

unrelated CI fail in ClientAsyncAuthenticate_AllServerVsIndividualClientSupportedProtocols_Success(clientProtocol: Tls) on OSX.1012:

   at System.Net.Sockets.Socket.EndConnect(IAsyncResult asyncResult) in /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/System.Net.Sockets/src/System/Net/Sockets/Socket.cs:line 2430
   at System.Net.Sockets.TcpClient.EndConnect(IAsyncResult asyncResult) in /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/System.Net.Sockets/src/System/Net/Sockets/TCPClient.cs:line 334
   at System.Net.Sockets.TcpClient.<>c.<ConnectAsync>b__27_1(IAsyncResult asyncResult) in /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/System.Net.Sockets/src/System/Net/Sockets/TCPClient.cs:line 278
   at System.Threading.Tasks.TaskFactory`1.FromAsyncCoreLogic(IAsyncResult iar, Func`2 endFunction, Action`1 endAction, Task`1 promise, Boolean requiresSynchronization) in /Users/buildagent/agent/_work/482/s/src/mscorlib/src/System/Threading/Tasks/FutureFactory.cs:line 533
--- End of stack trace from previous location where exception was thrown ---
   at System.Net.Security.Tests.ClientAsyncAuthenticateTest.ClientAsyncSslHelper(EncryptionPolicy encryptionPolicy, SslProtocols clientSslProtocols, SslProtocols serverSslProtocols) in /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/System.Net.Security/tests/FunctionalTests/ClientAsyncAuthenticateTest.cs:line 156
   at System.Net.Security.Tests.ClientAsyncAuthenticateTest.ClientAsyncAuthenticate_AllServerVsIndividualClientSupportedProtocols_Success(SslProtocols clientProtocol) in /Users/dotnet-bot/j/workspace/dotnet_corefx/master/osx-TGroup_netcoreapp+CGroup_Debug+AGroup_x64+TestOuter_false_prtest/src/System.Net.Security/tests/FunctionalTests/ClientAsyncAuthenticateTest.cs:line 107
--- End of stack trace from previous location where exception was thrown ---

…tation

stephentoub · 2018-03-29T02:36:05Z

@dotnet-bot test this please

tmds · 2018-03-29T06:18:05Z

I want to update the test that is added in #27763 to cover this. I'll do that when that PR is merged.

stephentoub · 2018-03-31T01:42:55Z

+
+            if (GetWaitState().GetExited(out _, refresh))
+            {
+                throw new InvalidOperationException(SR.Format(SR.ProcessHasExited, _processId.ToString(CultureInfo.CurrentCulture)));


Is the CultureInfo.CurrentCulture needed?

I lifted this exception from GetProcessHandle. I don't know if it is needed.

We can remove it. It won't affect the default formatting.

stephentoub · 2018-03-31T01:44:41Z

        {
            HaveId = 0x1,
            IsLocal = 0x2,
+            HaveNonExitedId = HaveId | 0x4,


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

stephentoub · 2018-03-31T01:50:59Z

+                        }
+                        if (pws == null)
+                        {
+                            pws = new ProcessWaitState(processId, isChild: false, exitTime);


Can you help me understand this better? It's not clear to me from this code nor from the comment why a different ProcessWaitState object helps with anything. What's the key difference between the pws we nulled out and this one we're creating?

The key difference is the new pws isn't in the exited state.
If we have a non-child Process with a recycled PID and we'd reused the exited pws, then the Process is unusable because HaveNonExitedId throws.

danmoseley · 2018-04-02T21:14:48Z

I suppose it is unlikely this would help fix https://github.com/dotnet/corefx/issues/28668

tmds · 2018-04-11T11:59:20Z

@dotnet-bot test this please

danmoseley · 2018-04-11T12:05:28Z

@tmds @stephentoub given all the churn to pid management in the last month and we are about to enter ask -mode for 2.1 changes, do we need some focused stress testing to reduce risk here?

stephentoub · 2018-04-11T12:08:15Z

Yes. I'm not comfortable with the level of churn and the associated risks on stability. I'm concerned we've been fixing some niche issues that could cause problems for some, but only in rare conditions, and in exchange potentially causing problems for main-stream cases. It's possible of course everything is fine, but...

danmoseley · 2018-04-11T12:11:54Z

@stephentoub what do you suggest? Defer changes (which) or test more first? Perhaps take this change then do focused testing, based on that choose if and whether it is necessary to revert any part for 2.1?

Testing would have to cover distro /Mac matrix.

stephentoub · 2018-04-11T12:13:23Z

I'd suggest testing more first. I'm not convinced this PR is necessary for 2.1.

danmoseley · 2018-04-11T12:29:43Z

@ViktorHofer could you please look at this and related pid-tracking changes (@stephentoub can list them perhaps) and devise and execute a plan for stress tests? We can use CI/official runs to cover the matrix.

stephentoub · 2018-04-11T12:37:22Z

The most recent two shown here: https://github.com/dotnet/corefx/commits/master/src/System.Diagnostics.Process/src?author=tmds. They overhaul how process waiting works.

tmds · 2018-04-11T12:46:39Z

These are the related PRs:

ensure children are reaped: Use SIGCHLD to trigger Process waitpid check #26291
support a new child to have the same pid as an existing Process instance: Handle recycled child PIDs #27763

The first PR is the most important one. It is also the most risky/churny.
The second (and this PR) are about recycled pids. I think they solve issues that may occur in practice, but with a low probability. So I agree, this isn't must-have for 2.1.

edit: #26291 is in preview2 so it will get some external 'testing' too.
edit2: #27763 included some fixes. These were afaik not fixing issues introduced by #26291, rather the test added in that PR revealed some existing issues.

ViktorHofer · 2018-04-23T19:34:35Z

@ViktorHofer could you please look at this and related pid-tracking changes (@stephentoub can list them perhaps) and devise and execute a plan for stress tests? We can use CI/official runs to cover the matrix.

sorry, I missed that one. does this still apply?

danmoseley · 2018-04-23T20:43:20Z

@ViktorHofer it's less urgent but yes, I would look at the changes above by @tmds, the tests he performed locally and/or added, and recommend whether we should add more test coverage/stress tests.

stephentoub · 2018-05-23T03:04:08Z

@dotnet-bot test this please

stephentoub · 2018-05-24T14:33:56Z

+                return;
+            }
+
+            if (GetWaitState().GetExited(out _, refresh))


Maybe combine the above into:

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

?

danmoseley · 2018-05-29T04:45:03Z

@tmds seems I can merge, unless you want to update per suggestion above?

tmds · 2018-05-29T06:26:11Z

The combined check would be:

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

I've left it separate and added a comment what we're checking for.

stephentoub · 2018-05-29T11:04:57Z

The combined check would be:

It would? Doesn't the == need to be != ? Otherwise I'm confused.

tmds · 2018-05-29T11:11:43Z

It would? Doesn't the == need to be != ? Otherwise I'm confused.

GetWaitState assigns _waitStateHolder when it is null. When refresh is false we don't assign _waitStateHolder since it would just say we haven't exited.

stephentoub · 2018-05-29T11:33:52Z

Ok, but how does that work with the current check? Currently you return if _waitStateHolder == null && !refresh. So not returning is !(_waitStateHolder == null && !refresh) which is _waitStateHolder != null || refresh.

tmds · 2018-05-29T12:00:26Z

Correct. The combined check is:

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

I think you had suggested

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

but that would have been different too.

I had added a comment about what we're checking to make it more clear. Do you prefer putting things in a single if statement? with the comment?

stephentoub · 2018-05-29T12:09:20Z

The combined check is

Yes

I think you had suggested

Yes, we both had it wrong. I fixed mine after the fact.

Do you prefer putting things in a single if statement?

Either is fine. But I wanted to make sure what's there is what you intended since the combined version you shared initially didn't match the behavior.

…ycled pid (dotnet/corefx#28404) * Process: avoid performing operations on a different process with recycled pid * ThrowIfExited: don't create ProcessWaitState(Holder) if we are not refreshing * Handle non-child pid recycling * ProcessWaitState: update overview comment to reflect current implementation * Update TestProcessRecycledPid * Add comment to ThrowIfExited * Remove CultureInfo.CurrentCulture from _processId.ToString Commit migrated from dotnet/corefx@4e27e0a

tmds added 2 commits March 23, 2018 12:32

Process: avoid performing operations on a different process with recy…

738d1c0

…cled pid

ThrowIfExited: don't create ProcessWaitState(Holder) if we are not re…

94cf9a9

…freshing

tmds added 2 commits March 23, 2018 15:10

Handle non-child pid recycling

badc2f3

ProcessWaitState: update overview comment to reflect current implemen…

909b124

…tation

tmds mentioned this pull request Mar 23, 2018

Handle recycled child PIDs #27763

Merged

karelz added this to the 2.1.0 milestone Mar 27, 2018

karelz added the area-System.Diagnostics.Process label Mar 28, 2018

karelz assigned tmds, joperezr and wtgodbe Mar 28, 2018

karelz removed this from the 2.1.0 milestone Mar 28, 2018

stephentoub reviewed Mar 31, 2018

View reviewed changes

danmoseley added this to the 2.1.0 milestone Apr 2, 2018

tmds added 2 commits April 5, 2018 13:02

Merge remote-tracking branch 'upstream/master' into check_exited

836e3cf

Update TestProcessRecycledPid

48dbcec

stephentoub removed this from the 2.1.0 milestone Apr 14, 2018

stephentoub added this to the 2.2.0 milestone Apr 14, 2018

stephentoub closed this May 17, 2018

stephentoub reopened this May 17, 2018

stephentoub reviewed May 24, 2018

View reviewed changes

stephentoub approved these changes May 24, 2018

View reviewed changes

Add comment to ThrowIfExited

ea97178

Remove CultureInfo.CurrentCulture from _processId.ToString

dab279d

stephentoub merged commit 4e27e0a into dotnet:master May 29, 2018

Conversation

tmds commented Mar 23, 2018

Uh oh!

tmds commented Mar 23, 2018

Uh oh!

stephentoub commented Mar 29, 2018

Uh oh!

tmds commented Mar 29, 2018

Uh oh!

stephentoub Mar 31, 2018

Choose a reason for hiding this comment

Uh oh!

tmds Apr 3, 2018

Choose a reason for hiding this comment

Uh oh!

stephentoub May 24, 2018

Choose a reason for hiding this comment

Uh oh!

stephentoub Mar 31, 2018

Choose a reason for hiding this comment

Uh oh!

stephentoub Mar 31, 2018

Choose a reason for hiding this comment

Uh oh!

tmds Apr 3, 2018

Choose a reason for hiding this comment

Uh oh!

danmoseley commented Apr 2, 2018

Uh oh!

tmds commented Apr 11, 2018

Uh oh!

danmoseley commented Apr 11, 2018

Uh oh!

stephentoub commented Apr 11, 2018

Uh oh!

danmoseley commented Apr 11, 2018

Uh oh!

stephentoub commented Apr 11, 2018

Uh oh!

danmoseley commented Apr 11, 2018

Uh oh!

stephentoub commented Apr 11, 2018

Uh oh!

tmds commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ViktorHofer commented Apr 23, 2018

Uh oh!

danmoseley commented Apr 23, 2018

Uh oh!

stephentoub commented May 23, 2018

Uh oh!

stephentoub May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmoseley commented May 29, 2018

Uh oh!

tmds commented May 29, 2018

Uh oh!

stephentoub commented May 29, 2018

Uh oh!

tmds commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephentoub commented May 29, 2018

Uh oh!

tmds commented May 29, 2018

Uh oh!

stephentoub commented May 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tmds commented Apr 11, 2018 •

edited

Loading

stephentoub May 24, 2018 •

edited

Loading

tmds commented May 29, 2018 •

edited

Loading