Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Process.Start have a option to change handle inheritance #306

Open
pdelvo opened this issue Dec 19, 2014 · 54 comments

Comments

@pdelvo
Copy link

@pdelvo pdelvo commented Dec 19, 2014

Currently if you call Process.Start internally CreateProcess is called with bInheritHandles = true (hard coded). It would be great to make it possible to change this behavior, e.g. by adding a Property to ProcessStartInfo.

Currently there is no way I know of to change this other then reimplementing System.Diagnostics.Process.

Example

If you run this application twice without exiting the first notepad instance the second instance will not be able to open the tcp port, because notepad is still running. This can be a problem for server applications that are starting child processes themself and crash, or are killed by the user before the socket can be closed.

using System.Diagnostics;
using System.Net;
using System.Net.Sockets;

class Program
{
    static void Main()
    {
        TcpListener listener = new TcpListener(IPAddress.Any, 4567);
        listener.Start();

        Process.Start(new ProcessStartInfo("notepad.exe") { UseShellExecute = false });
        //Simulate application crash without freeing resources
    }
}

Design proposal

The easiest way to make this possible is to add a new Property to ProcessStartInfo and use this in the Call to CreateProcess

public sealed class ProcessStartInfo {
// ...
    public bool InheritHandles { get; set; } // defaults to true
// ...
}

Questions

  • Is there a very important reason why this was hardcoded like this in the first place?
@krwq

This comment has been minimized.

Copy link
Member

@krwq krwq commented Dec 22, 2014

I agree we should add this API. IMO we should also make TcpListener have option to change HandleInheritability and change the default behavior to not inherit handles if possible (at least when targeting newer version of the framework).

In most cases we should not change InheritHandles on Process and should rather do it whenever we create handles but we should still have an option to disable this.

@pdelvo

This comment has been minimized.

Copy link
Author

@pdelvo pdelvo commented Dec 23, 2014

I think adding options to TcpListener/TcpClient/Socket would be a good idea. Are there other places where handle inheritance can be a problem? I was not able to create a problem like the one mentioned above by exclusivly opening a file

@terrajobst

This comment has been minimized.

Copy link
Member

@terrajobst terrajobst commented Dec 24, 2014

This seems quite reasonable. As @krwq we should probably do a pass and look into similar classes that perform handle inheritability. This helps in understanding what API we could use.

@krwq

This comment has been minimized.

Copy link
Member

@krwq krwq commented Dec 29, 2014

@pdelvo, We were able to repro this with FileStream although that one creates non-inheritable handles by default so had to override that. I believe that every library should have non-inheritable handles by default but that might be hard to change at this point because of compat reasons

@ellismg

This comment has been minimized.

Copy link
Contributor

@ellismg ellismg commented Jan 9, 2015

One open question I have is what this would mean when we try to support this feature cross platform. What would it mean to say that handles are not inherited in Unix? Does "handles" here mean open file descriptors? If they are not to be inherited, does the framework have to find and close them in the child process? How does it interact with file descriptors marked FD_CLOEXEC?

@ellismg ellismg self-assigned this Jan 9, 2015
@stephentoub

This comment has been minimized.

Copy link
Member

@stephentoub stephentoub commented Jan 9, 2015

I expect there are going to be a fair number of features in System.Diagnostics.Process that result in PlatformNotSupportedException; this might be one of them, if e.g. explicitly closing all fds above 2 doesn't work out or is prohibitive for some reason.

@terrajobst

This comment has been minimized.

Copy link
Member

@terrajobst terrajobst commented Jan 15, 2015

We've reviewed this proposal and don't believe it's ready yet. Please take a look at the notes.

@Priya91

This comment has been minimized.

Copy link
Member

@Priya91 Priya91 commented Dec 7, 2016

@pdelvo @whoisj Would one of you be interested in working on the api proposal for this, responding to the last api review in @danmosemsft's link..

@Priya91 Priya91 added the up-for-grabs label Dec 7, 2016
@gistofj

This comment has been minimized.

Copy link

@gistofj gistofj commented Dec 7, 2016

@Priya91 yeah I could do something, but likely not until after VS 2017 ships (it is an all consuming effort). If @pdelvo wants to start on something, I'd be happy to collaborate as well.

What are the requirements for CoreFx API changes? Does the API need x-plat support, can there be Windows specific elements? Etc.

As a side note, I'd prefer to see a method added to System.Diagnostics.ProcessStartInfo like public void AddInhertiableHandles(IEnumerable<SafeHandle> handles), public void AddInhertiableHandles(IEnumerable<IntPtr> handles) than a misleading property like public bool InheritHandles { get; set; } because any time there's a redirection of pipes, inheritance needs to be enabled but consumers of the API do not likely mean to inherit every handle the parent process has.

@Priya91

This comment has been minimized.

Copy link
Member

@Priya91 Priya91 commented Dec 7, 2016

What are the requirements for CoreFx API changes? Does the API need x-plat support

We have the api addition process documented here. It also elaborates on the design principles. Yeah we do need x-plat support, as .NET Core supports Unix platforms as well.

can there be Windows specific elements? Etc.

Are you asking in terms of exposing an API only on Windows? We can't do that, although we have some windows specific APIs in .NET Core and throw PNSE on other platforms.

For now the focus should be more on the API design as you suggested in using a method over property etc, which will take shape once we understand the scenarios and requirements on all platforms.

@gistofj

This comment has been minimized.

Copy link

@gistofj gistofj commented Dec 8, 2016

@Priya91 👍 and thanks.

@danmosemsft

This comment has been minimized.

Copy link
Member

@danmosemsft danmosemsft commented Dec 27, 2016

@pdelvo do you want to update per the feedback and retry review? that would be welcome.

@karelz

This comment has been minimized.

Copy link
Member

@karelz karelz commented Jun 20, 2017

Next steps: We need to decide how to introduce Windows-only APIs. Ideally BCL is mostly platform agnostic. It is something we want in BCL.
Likely not good up-for-grabs candidate.

@jhudsoncedaron

This comment has been minimized.

Copy link

@jhudsoncedaron jhudsoncedaron commented Sep 19, 2018

@AArnott : The original "InheritHandles" was good enough, but the API review team rejected it. Let us answer their objections:

"There is no good way to disable inhering handles on Linux"

Oh yes there is. If inherit handles not set, close all handles > 2 in between fork() and exec().

"We should use System.IO.HandleInheritablility enum if we expose this property."

Then do so.

"How does this interact with how this places with redirects and UseShellExecute"

throw, as they already stated.

Whichever way you set the default for InheritHandles, somebody's got some code auditing to do. I find I don't care which way it is. I'm perfectly willing to track down all of my invocations and fix them. Trust me, it's a lot less trouble than trying to work around socket handles from HttpClient leaking into other processes.

@gistofj

This comment has been minimized.

Copy link

@gistofj gistofj commented Sep 19, 2018

@jhudsoncedaron the problem with InheritHandles is it must be true for IO redirection to work, so it is not a solution.

Instead, I believe we should focus on adding a white-list option. If the list is null ignore it, if the list exists then only take the handles in the list to the new process. Windows already has this functionality, and Linux does via closing all handles not in the list between fork() and exec().

@jhudsoncedaron

This comment has been minimized.

Copy link

@jhudsoncedaron jhudsoncedaron commented Sep 19, 2018

@gistofj : I projected that setting the managed "InheritHandles" to false/NotInherit would still permit redirection to work. There's no inherent reason why it wouldn't. Now if it just passed false to the native CreateProcess that would be a problem. The solution outlined in Raymond's article has to be used on Windows in any case.

If you would really rather provide a list of handles to inherit you can do so.

This is one of the few things that is really hard to fix by P/Invoke. Hint: you cannot allocate memory between fork() and exec() on Mac.

@tmds

This comment has been minimized.

Copy link
Member

@tmds tmds commented Oct 25, 2018

#32903 disables socket inheritance on Windows and Mac. That fixes the original issue reported here.
Are there other handles being inherited that cause trouble? Or can this issue be closed?

@jhudsoncedaron

This comment has been minimized.

Copy link

@jhudsoncedaron jhudsoncedaron commented Oct 25, 2018

@tmds : There are more issues.

  1. Process.Start() races with itself in two threads, occasionally causing child processes to inherit handles intended for other child processes, which can cause deadlocks.

  2. new Socket() can create a socket that is initially inheritable. While there is an API to change it and the framework uses it, this also always races with Process.Start() so it's worse. Hint: HttpClient. SmtpClient. The list goes on and on.

I actually discovered this from 1 above the first time.

Tell you what. You settle the API surface, and I can do an implementation for both Linux and Windows. Somebody else will have to do the Mac as I haven't got access to one.

@tmds

This comment has been minimized.

Copy link
Member

@tmds tmds commented Oct 25, 2018

Process.Start() races with itself in two threads, occasionally causing child processes to inherit handles intended for other child processes, which can cause deadlocks.

This is very vague.

new Socket() can create a socket that is initially inheritable. While there is an API to change it and the framework uses it, this also always races with Process.Start() so it's worse. Hint: HttpClient. SmtpClient. The list goes on and on.

On Linux and Windows there are APIs that create the socket non inheritable. We already used those on Linux, and with #32903 we also use them on Windows.

@jhudsoncedaron

This comment has been minimized.

Copy link

@jhudsoncedaron jhudsoncedaron commented Oct 25, 2018

@tmds: The handle that is standard output for one process gets inherited onto another process as well because they both hit the native call at the same time. The deadlock happens if ReadToEnd() is used. Also, one processes's stdin can be inherited by another process as well. In that case the deadlock happens if the operation mode is feed data to stdin until end as the other child holds the handle open.

Here is a place where a socket is initially created with inheritable handle and then changed to not inheritable. https://github.com/libuv/libuv/blob/27ba66281199bdcade823677af8dedc161152fb6/src/win/tcp.c#L420

@tmds

This comment has been minimized.

Copy link
Member

@tmds tmds commented Oct 25, 2018

The handle that is standard output for one process gets inherited onto another process as well because they both hit the native call at the same time. The deadlock happens if ReadToEnd() is used. Also, one processes's stdin can be inherited by another process as well. In that case the deadlock happens if the operation mode is feed data to stdin until end as the other child holds the handle open.

You can use ProcessStartInfo.RedirectStandard{Input,Output,Error} to avoid leaking stdin/out/err into the children.

Here is a place where a socket is initially created with inheritable handle and then changed to not inheritable.

In corefx it happens on creation:

InnerSafeCloseSocket result = Interop.Winsock.WSASocketW(addressFamily, socketType, protocolType, IntPtr.Zero, 0, Interop.Winsock.SocketConstructorFlags.WSA_FLAG_OVERLAPPED | Interop.Winsock.SocketConstructorFlags.WSA_FLAG_NO_HANDLE_INHERIT);

@jhudsoncedaron

This comment has been minimized.

Copy link

@jhudsoncedaron jhudsoncedaron commented Oct 25, 2018

The race condition happens when using one of ProcessStartInfo.RedirectStandard{Input,Output,Error} on more than one Process.Start entered simultaneously from different processes.

I cited that particular call in libuv because libuv is a dependency of net core somehow. I see it get shipped with dotnet package -r

@tmds

This comment has been minimized.

Copy link
Member

@tmds tmds commented Oct 26, 2018

The race condition happens when using one of ProcessStartInfo.RedirectStandard{Input,Output,Error} on more than one Process.Start entered simultaneously from different processes.

So, the issue is that the end of the pipe that is used by the process to read/write to the child gets cloned into another child?
This shouldn't be a problem on Linux. The pipe doesn't get inherited.
On Windows, if there is an issue, probably it can be fixed. Please create a separate issue.

I cited that particular call in libuv because libuv is a dependency of net core somehow. I see it get shipped with dotnet package -r

libuv is used by Kestrel. From Kestrel 2.1 onwards, it is not the default, corefx Sockets are used instead.
The fix for Windows will be in .NET Core 3.0. Kestrel 2.2 will have a local fix for the socket used by the webserver.

@tmds

This comment has been minimized.

Copy link
Member

@tmds tmds commented Nov 7, 2018

@karelz this can be closed due to improvements in #33097 and #32903

@gistofj

This comment has been minimized.

Copy link

@gistofj gistofj commented Nov 15, 2018

@tmds, I don't know that either of those issues will resolve the problem entirely. They sure will make it less prevalent though. I ran into this issue (head first at 90 mph) during the VS 2017 bring-up. At the time I was developing a library to wrap git.exe in a way that Visual Studio could consume it via Team Explorer.

I was constantly running into git.exe processes that would never exit, they'd just hand there until all of VS was torn down. After a lot of spelunking, the root cause was that VS is a multi-tenant application: there are dozens, if not hundreds of tiny services all running along side each other in blissful ignorance of each other. In my case, msbuild.exe was being launched and then parked awaiting its next build task. Too often is would inherit all of the handles in the VS process, including our local handles to Git's standard input, output, and error pipes.

The net effect was that we'd close our handle to the standard input pipe (the signal to Git that it's OK to exit) but because the third process would have a duplicate of the handle, the pipe would net get closed and Git would dutifully away input.

The solve was not tricks with mutexes or anything like that. The answer was ThreadProcAttributeList and its related functions. I highly suggest you take a close look at how these work for Windows. Please remember that Windows doesn't care on iota about AppDomains or any trick the CLR can bring - this is a kernel level thing happening, and it is old, and unlikely to be changed in the near future (and never to be changed on an older version of Windows).

I projected that setting the managed "InheritHandles" to false/NotInherit would still permit redirection to work. There's no inherent reason why it wouldn't.

@jhudsoncedaron, I wish you were correct but empirical evidence shows that it does not work. Even if I pass handles to CreateProcess for the resulting process to use, if I do not also set bInherit to true, it does not work. That is the entire point and reasoning behind ThreadProcAttributeList.

You can disagree with me about this, but I spent time with the teams responsible for this part of Windows, looking at the source code; and this is was the best answer we could find.

As for Linux, I have no idea how to resolve that issue. I am very far from being a Linux SME (subject matter expert).

@jhudsoncedaron

This comment has been minimized.

Copy link

@jhudsoncedaron jhudsoncedaron commented Nov 15, 2018

@gistofj : The reality is I was focusing on 100% .NET applications not mixed applications. Others must carry the bw-compat vs. reliability debate from here. I have posted how to fix it in Linux.

@gistofj

This comment has been minimized.

Copy link

@gistofj gistofj commented Nov 15, 2018

@jhudsoncedaron I do not see how that's a fix. It is a reasonable workaround if you control the entire situation, but that's rarely the case in real world scenarios. The proper fix is for the runtime to correctly utilize the functionality provided by the operating system(s).

@jhudsoncedaron

This comment has been minimized.

Copy link

@jhudsoncedaron jhudsoncedaron commented Nov 15, 2018

@gistofj : When I said I have posted how to fix it in Linux I wasn't kidding. I posted how to get the equivalent of ThreadProcAttributeList for file handles on Linux earlier in this thread.

@stephentoub stephentoub modified the milestones: Future, 5.0 Jul 18, 2019
MaximLipnin pushed a commit to MaximLipnin/corefx that referenced this issue Jul 31, 2019
…net#306)

This allows us to use the test with the managed linker enabled in Xamarin.iOS
since it no longer strips out the MaxValue/MinValue fields.

See mono/mono#14877

Port of upstream corefx PR dotnet#39095
@rhegner

This comment has been minimized.

Copy link

@rhegner rhegner commented Aug 2, 2019

We are experiencing this problem in production now in an application which launches a separate updater process which installs a new version and which then launches the new version of our product.

So even if we had an option to change handle inheritance in Process.Start, that would not solve the problem for all the instances of the old version which are already running on customers premises.

I'm therefore looking for a workaround which can be implemented purely in the child process.

Is there a way to close all the inherited handles at the beginning of a child process? Note that our ASP.NET Core application can run on any platform (primarily Windows and Linux).

This is kind of urgent, as we need to ship a new version of our product to customers soon....

@danmosemsft

This comment has been minimized.

Copy link
Member

@danmosemsft danmosemsft commented Aug 2, 2019

Adding @JeremyKuhne for his perspective

@krwq

This comment has been minimized.

Copy link
Member

@krwq krwq commented Aug 2, 2019

@rhegner one thing to try out is to find way to enumerate all process handles on startup and then inspect/close as needed. Perhaps some of those will work for you:
https://stackoverflow.com/questions/3019066/get-all-window-handles-for-a-process
https://stackoverflow.com/questions/733384/how-to-enumerate-process-handles

@tmds

This comment has been minimized.

Copy link
Member

@tmds tmds commented Aug 3, 2019

Afaik there should be no more issues on Linux.
For Windows, it would be good to figure out what handles are being leaked.

@GSPP

This comment has been minimized.

Copy link

@GSPP GSPP commented Aug 4, 2019

@rhegner Maybe the child process can re-launch itself with the correct handle inheritance settings.

@rhegner

This comment has been minimized.

Copy link

@rhegner rhegner commented Aug 5, 2019

Thank you all for your answers so far!
For troubleshooting I created a minimal sample program to reproduce the problem. You can find it here:
https://github.com/rhegner/HandleInheritanceTest

You can launch it like this (where 5000 is the port):

dotnet HandleInheritanceTest.dll Parent 5000 NoFix

It will start a webhost, and then start a child process which waits until the parent process has exited, and then also tries to start a webhost on the same port. It fails with

System.IO.IOException: Failed to bind to address http://[::]:5000: address already in use.

I tried to work around the problem by closing all inherited sockets when at the beginning of the child process using NtQuerySystemInformation and NtQueryObject. This is very ugly code (platform specific, potential issues with 32bit vs. 64bit, "undocumented" APIs, NtQueryObject hangs under certain condition). I did manage to enumerate all the handles of the child process and get their type and name information. But browsing through that list, nothing looks like it could be a socket handle. So I don't know which handles to close.

You can try this attempted workaround by starting my example program in a different mode like this:

dotnet HandleInheritanceTest.dll Parent 5000 CloseInheritedSockets

To summarize: I'm still lost here - any help would be appreciated!!

PS: In the mean time, next thing I'll try is re-writing Process.Start so it doesn't leak handles to the new process.

@tmds

This comment has been minimized.

Copy link
Member

@tmds tmds commented Aug 6, 2019

Can you try using .NET Core 3.0 (which has #32903)?

@rhegner

This comment has been minimized.

Copy link

@rhegner rhegner commented Aug 6, 2019

Yes chaning my little example project to .NET Core 3.0 solves the problem. That's good news.
However, for our real product re-targeting to a preview version is not an option, so I still need a workaround for the upcomig version of our software...

@danmosemsft

This comment has been minimized.

Copy link
Member

@danmosemsft danmosemsft commented Aug 6, 2019

However, for our real product re-targeting to a preview version is not an option,

If it helps, 3.0 will have its stable release next month.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.