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 #13943

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

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

pdelvo opened this issue Dec 19, 2014 · 91 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

namespace System.Diagnostics
{
    public partial class ProcessStartInfo
    {
        public bool InheritHandles
        {
            get;  // defaults to true
            [MinimumOSPlatform("windows7.0")]
            set;
        }
    }
}

Questions

  • Is there a very important reason why this was hardcoded like this in the first place?
@krwq
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
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
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
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
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
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
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
Copy link
Contributor

@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..

@whoisj
Copy link

@whoisj whoisj 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
Copy link
Contributor

@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.

@whoisj
Copy link

@whoisj whoisj commented Dec 8, 2016

@Priya91 👍 and thanks.

@danmoseley
Copy link
Member

@danmoseley danmoseley commented Dec 27, 2016

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

@karelz
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.

@wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Feb 1, 2018

@agocke had some interest in this issue with https://github.com/dotnet/corefx/issues/26711, anybody know what is the latest?

@ghost
Copy link

@ghost ghost commented Mar 12, 2018

In node.js they use the concept from Win32 API DETACHED_PROCESS and CREATE_NEW_PROCESS_GROUP https://github.com/nodejs/node/blob/3a19122/deps/uv/src/win/process.c#L1089

See process creation flags: https://msdn.microsoft.com/en-us/library/windows/desktop/ms684863(v=vs.85).aspx

Then in Unix, they also use detached process from the options and then call setsid() to detach the process. https://github.com/nodejs/node/blob/3a19122/deps/uv/src/unix/process.c#L289

Maybe instead of calling it InheritHandles, use the term Detach?

cc @wfurt

@whoisj
Copy link

@whoisj whoisj commented Mar 13, 2018

@kasper3 that's fine for NodeJs which is launch processes to behave like threads. Often, developers want to share the console's standard in/out/err pipes with the child. Additionally, neither DETACHED_PROCESS nor CREATE_NEW_PROCESS_GROUP prevent handle inheritance (to the best of my knowledge).

@karelz why not just have the Process.StartInfo take an IEnumerable<SafeHandle> (or similar) which then causes the handle exclusion logic used by each platform to whittle down the inherited handles to only the set specified by the enumerable (and maybe include standard handles as necessary).

@madelson
Copy link
Contributor

@madelson madelson commented Jun 1, 2018

According to this thread (brettwooldridge/NuProcess#13 (comment)), it sounds like Java is able to solve this issue in it's Process implementation:

The process implementation in Java uses JNI to first fork the JVM process, then the child JVM process performs a close of all file descriptors except for the pipes to the parent, and then executes the user-specified process, which replaces the child process in-situ (never to return) but inherits the file descriptors (now only the pipes to the parent).

Probably .NET could do something similar?

@whoisj
Copy link

@whoisj whoisj commented Jun 1, 2018

@madelson on Linux? sure. On Windows? not so much.

@madelson
Copy link
Contributor

@madelson madelson commented Jun 1, 2018

@whoisj The OP calls out that today we call CreateProcess with bInheritHandles = true (hard coded). So on Windows I assumed the solution would be to just pass false for this parameter. From the discussion I thought the main issue was a concern that this behavior couldn't easily be replicated across OS's (e. g. @Priya91's comment about this API being Windows only).

@whoisj
Copy link

@whoisj whoisj commented Jun 1, 2018

@madelson passing false for bInheritHandles doesn't work if the caller needs stream redirection.

Instead, I would think (for Windows) we'd want to focus on InitializeProcThreadAttributeList.

@jhudsoncedaron
Copy link

@jhudsoncedaron jhudsoncedaron commented Sep 18, 2018

I know exactly why this bug exists. The Process.Start() code was written originally to run on Windows 9x where you literally couldn't fix this. The API for actually fixing this on Windows was added on Windows Vista, but nobody ever revisited this code. And when the port to Linux was done it was done directly, emulating the bug, probably without thinking about it.

Anybody using blind inherit handles from any thread other than the main thread is bugged and doesn't know it yet.

@karelz
Copy link
Member

@karelz karelz commented Sep 18, 2018

@jhudsoncedaron do you have specific changes in mind?
.NET Core 2.1+ supports only Win7+: https://github.com/dotnet/core/blob/master/release-notes/2.1/2.1-supported-os.md#windows so using Vista API should be reasonable.

@jhudsoncedaron
Copy link

@jhudsoncedaron jhudsoncedaron commented Sep 18, 2018

Yup!

How to fix (Windows): https://blogs.msdn.microsoft.com/oldnewthing/20111216-00/?p=8873
More problems caused by this: https://blogs.msdn.microsoft.com/oldnewthing/20131018-00/?p=2893

To fix on Unix, the child process needs to enumerate over /proc/self/fd and close any integers > 2 except for the handle being used to enumerate the directory itself.

@stephentoub
Copy link
Member

@stephentoub stephentoub commented Sep 18, 2018

This is not a "bug", which suggests you could just change the implementation... doing that would break an absolute ton of code. If we want to add an option to the API (and work through any associated issues), that's fine, but we must not change the existing behavior of the existing API. And we absolutely thought about the behavior when implementing this for Unix.

@jhudsoncedaron
Copy link

@jhudsoncedaron jhudsoncedaron commented Sep 18, 2018

@stethentoub: If you create two child process from two separate threads without a lock on the unix platform, your code can hang. I'm not certain but I think the build server code in dotnet is affected. It's difficult to come up with a case on the Windows platform where someone could be depending on random handles being inherited because only socket handles are inherited.

@stephentoub
Copy link
Member

@stephentoub stephentoub commented Sep 18, 2018

It's difficult to come up with a case on the Windows platform where someone could be depending on random handles being inherited

e.g. anonymous pipes

If you create two child process from two separate threads without a lock on the unix platform, your code can hang.

Code can hang for lots of reasons. My point is, the implementation of the API is not buggy; it's doing exactly what it's defined to do. You may not like the definition, but that's different than saying there's a bug in the implementation.

@Tyrrrz
Copy link
Contributor

@Tyrrrz Tyrrrz commented May 9, 2020

This issue is causing problems in a project I'm maintaining when combined with self-contained single-file deployments. It appears that when a self-contained single-file app runs, it claims a file handle on its exe and then unpacks the contents to a temp directory, without freeing the handle right away. Because of that, child processes inherit the handle and can't obtain write access for the original exe file (which my project needs to overwrite it with a different file as part of an auto-update).

@jkotas
Copy link
Member

@jkotas jkotas commented May 9, 2020

It appears that when a self-contained single-file app runs, it claims a file handle on its exe and then unpacks the contents to a temp directory, without freeing the handle right away.

A bug like this was fixed by #2272 . Do you see the problem with this fix? You can try nightly .NET 5 build from https://github.com/dotnet/installer . Also, the fix is scheduled to be included in .NET 3.1.4 update (dotnet/core-setup#9010).

@Tyrrrz
Copy link
Contributor

@Tyrrrz Tyrrrz commented May 9, 2020

@jkotas that seems to be exactly the fix for my problem, thanks!

@Joe4evr
Copy link

@Joe4evr Joe4evr commented May 9, 2020

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.

Now that the plan for .NET 5 (and beyond) is to have platform-specific APIs shipped under a TFM suffix, is this not solved? When the developer doesn't target net5.0-windows, they won't get light-up on the new property, and if they want to multi-target, they should #ifdef around the line where they set it.

@danmoseley
Copy link
Member

@danmoseley danmoseley commented May 9, 2020

@terrajobst is that consistent with your TFM plan?

@danmoseley
Copy link
Member

@danmoseley danmoseley commented May 15, 2020

@jkotas
Copy link
Member

@jkotas jkotas commented Jul 23, 2020

This should be ready to review. We have a plan for Windows-specific APIs now, we have just approved other similar Windows specific properties (#28114).

@terrajobst
Copy link
Member

@terrajobst terrajobst commented Jul 24, 2020

Video

  • It shouldn't default to true, it should return the actual behavior of the underlying platform.
  • Marking the setter as platform specific makes sense, assuming we can't (or don't want to) implement the necessary gymnastics.
  • @adamsitnik @eiriktsarpalis please check what the possible behavior/desirable behavior for Linux/Unix/macOS is
namespace System.Diagnostics
{
    public partial class ProcessStartInfo
    {
        public bool InheritHandles
        {
            get;
            [MinimumOSPlatform("windows7.0")]
            set;
        }
    }
}

@jhudsoncedaron
Copy link

@jhudsoncedaron jhudsoncedaron commented Jul 24, 2020

In the UNIX environment, if you don't lift a finger the behavior will be true for outside-provided handles and false for handles opened by the framework itself. Pretty much everybody's depending on the fact that framework-opened handles aren't inherited.

@jkotas
Copy link
Member

@jkotas jkotas commented Jul 24, 2020

it should return the actual behavior of the underlying platform.

The default behavior of both Windows and Unix is to inherit handles that are marked as inheritable.

In the UNIX environment, if you don't lift a finger the behavior will be true for outside-provided handles and false for handles opened by the framework itself

Right, the difference between Windows and Unix is that handles in ordinary code are typically opened as non-inheritable on Windows and inheritable on Unix.

@tmds
Copy link
Member

@tmds tmds commented Sep 15, 2021

there is no nice way to do this on Linux.

In the meanwhile Linux 5.9 (nov 2020) introduced close_range.

The sensible default would be to not inherit handles (beside stdin/out/err) since that is what most applications need, and that avoids issues due to inherited handles. However, that would be a breaking change.

@tmds
Copy link
Member

@tmds tmds commented Sep 24, 2021

This issue has been on my mind since I looked into #59079.

I'd like there to be more control to allow passing handles, without unintentionally leaking others.

Something like:

    public partial class ProcessStartInfo
    {
        public SafeHandle?[]? Handles { get; set; }
    }

When set, this controls the handles passed to the child.

The assigned array must have a length of at least 3 to explicitly indicate the handles for stdin, stdout, stderr.

The item at index i becomes fd i in the child.

All file descriptors at Handles.Length and higher are closed (if the platform supports it).

When an item is null it is passed as is to to the child.

The proposed InheritHandles can remain as a shortcut to setting Handles = new[] { null, null, null } (when InheritHandles = false).

It would be doable to implement this on Linux/Unix.
Can this API be implemented on Windows?

@jkotas
Copy link
Member

@jkotas jkotas commented Sep 24, 2021

Can this API be implemented on Windows?

I do not think this is implementable on Windows. Windows only lets you specify the subset of inheritable handles to inherit for given process (look for PROC_THREAD_ATTRIBUTE_HANDLE_LIST).

@tmds
Copy link
Member

@tmds tmds commented Sep 27, 2021

Thank you for the pointer @jkotas.

I took a look at the docs. From what I read, on Windows we can't control what file descriptor the handle becomes in the child process. We can drop that requirement.

The rest should be doable by setting hStdInput, hStdOutput, hStdError based on the first three items in Handles, and passing remaining handles via PROC_THREAD_ATTRIBUTE_HANDLE_LIST.

The approved InheritHandles is meaningful on its own.

I'll write up a separate issue about the Handles use-case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet