Runtime: Make Process.Start have a option to change handle inheritance

[pdelvo]

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]

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]

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 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]

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

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?

[stephentoub]

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]

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

[danmosemsft]

Fixed link: https://github.com/dotnet/apireviews/tree/master/2015/01-14-misc#306-make-processstart-have-a-option-to-change-handle-inheritance

[Priya91]

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

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

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]

@Priya91 :+1: and thanks.

[danmosemsft]

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

[karelz]

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]

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

[ghost]

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]

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

According to this thread (https://github.com/brettwooldridge/NuProcess/issues/13#issuecomment-282071125), 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]

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

[madelson]

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

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

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]

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

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]

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]

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

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.

[GSPP]

Since a process often contains many components incl. 3rd party libraries it seems very unsafe to inherit handles by default. This could lead to random handles being inherited. The whole model of inheriting all inheritable handles, as pre-Vista OS'es did it, seems fundamentally unsafe.

Inheritable handles are mutable process-global state. Very dangerous in complex processes.

For that reason an opt-out option seems very appropriate to have. IMO, all new usages of the Process class should opt-out as a best practice.

[ayende]

This can lead to _really_ hard to figure out bugs, which was our case. Having an option to turn this off is very desriable.

[AArnott]

I think the most productive discussion here will not be one to persuade the .NET team to change the current behavior, as @stephentoub says.

Instead, let us direct our efforts at discussing how we can introduce an _option_ for more desirable behavior in the .NET API so that folks who spawn child processes can get the behavior they want.

For example, here are a few starter ideas:

1. an app.config setting
2. a new overload of Process.Start
3. a new property in ProcessStartInfo

[jhudsoncedaron]

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

[whoisj]

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

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

https://github.com/dotnet/corefx/pull/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]

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

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 https://github.com/dotnet/corefx/pull/32903 we also use them on Windows.

[jhudsoncedaron]

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

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:

https://github.com/dotnet/corefx/blob/27fc4ecc80878f52feeac1d6bb23345ab65f97f5/src/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Windows.cs#L215

[jhudsoncedaron]

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]

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]

@karelz this can be closed due to improvements in https://github.com/dotnet/corefx/pull/33097 and https://github.com/dotnet/corefx/pull/32903

[whoisj]

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

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

[whoisj]

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

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

[rhegner]

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]

Adding @jeremykuhne for his perspective

[krwq]

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

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

[GSPP]

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

[rhegner]

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]

Can you try using .NET Core 3.0 (which has https://github.com/dotnet/corefx/pull/32903)?

[rhegner]

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]

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.

[danmosemsft]

Going back to the original proposal, which was

c# public sealed class ProcessStartInfo { // ... public bool InheritHandles { get; set; } // defaults to true // ... }
If I understand this is easy to "implement" on Windows by passing bInheritHandles=false to CreateProcessW, but

1. this prevents inheriting the standard handles -- it would need to throw if one or more of these are not being redirected
2. sometimes one wants just certain handles inherited, on Windows this is done with PROC_THREAD_ATTRIBUTE_HANDLE_LIST, as @gistofj noted would need an extended API

As a side note, I'd prefer to see a method added to System.Diagnostics.ProcessStartInfo like public void AddInhertiableHandles(IEnumerable handles), public void AddInhertiableHandles(IEnumerable 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.

So the open questions are

1. Would this be useful without the ability to allow specific handles?
2. What would it do on Linux?

[tmds]

Would this be useful without the ability to allow specific handles?

Yes. It can avoid that child processes keep handles open which they aren't using.
And improve perf when launching child processes on Windows (https://github.com/dotnet/corefx/issues/42697).

What would it do on Linux?

Close open file descriptors after fork (except for ProcessStartInfo Redirects).

[stephentoub]

Close open file descriptors after fork (except for ProcessStartInfo Redirects).

Meaning:
C# for (int i = 3; i <= short.MaxValue; i++) close(i);
?

[tmds]

After reading through some threads (ref https://sourceware.org/bugzilla/show_bug.cgi?id=10353), there is no nice way to do this on Linux.
Issues mentioned:

• performance looping over a large nr of fds
• risk of closing file descriptors that shouldn't be closed

BSDs have a dedicated closefrom syscall.
Some frameworks implement a poor man's closefrom that closes fds from /proc/self/fd or loop till _SC_OPEN_MAX.

I think real-world issues mentioned in this thread were fixed in .NET Core by using CLOEXEC and Windows equivalent.
Except for the Windows perf issue: https://github.com/dotnet/corefx/issues/42697.

[jhudsoncedaron]

It's probable any remaining issues can be reliably solved by exposing an API to take the "create process" lock. (Note that all implementations have such a lock, it's just not the same one for all of them.) Plausible API surface:

// owns must be initialized to false before calling
TryTakeProcessLock(ref bool owns);
TakeProcessLock(ref bool owns);
ReleaseProcessLock(ref bool owns); // does nothing if owns is false

Note this is like the Monitor.Lock functions. The implementation isn't exactly trivial because Process.Create needs to know about the lock being taken by these APIs on its own thread vs another thread.

This only does anything useful for people who intend to create their own native handles.

Don't like this idea, feel free to ignore it.

[danmosemsft]

I think real-world issues mentioned in this thread were fixed in .NET Core by using CLOEXEC and Windows equivalent.

For handles we create, right? Anyone can create their own. The example @gistofj mentioned is spawning processes from VS, a large app with plenty of native code owned by other components, creating lots of inheritable handles. Presumably that scenario exists on Unix as well -- it would be unfortunate to offer a new API that blocks inheritance on Windows but not on Unix if the problem can exist on both.

[madelson]

Would this be useful without the ability to allow specific handles?

I think it would still be useful, especially if any redirected standard IO handles were automatically included in the opt-in list used to create the process.

If there is an exposed API for specifying a list of handles to be inherited, then it should still automatically append any redirected standard IO handles to the list under the hood (this doesn't deprive the caller of any control since they must already opt into redirection explicitly anyway).

[madelson]

@danmosemsft what is the next step here? Here is a potential updated proposal:

API

public sealed class ProcessStartInfo {
    ...
    public SafeHandle[] InheritedHandles { get; set; }
    ...
}

Behavior

InheritedHandles defaults to null (for backwards compat). If InheritedHandles is set to a non-null value, then only the handles specified will be inherited by the child process.

Any standard IO handles that get created during process creation will always be inherited (these are already opt-in and it is pointless to create them without inheritance).

Therefore, the most common usage of this property would be to disable non-standard-IO handle inheritance by setting it to Array.Empty<SafeHandle>(). Arguably, this would be the "right" thing to do for almost all scenarios, so it may be worth considering making Array.Empty<SafeHandle>() the default value (perhaps behind a switch of some sort).

Implementation

If InheritedHandles is null, the current behavior is maintained.

On Windows, this can be implemented straightforwardly with functionality in CreateProcess. Likely we should throw if any of the provided InheritedHandles are specifically flagged as not being inheritable.

On Unix, there are two options:

• It seems like Java's approach is to have the rest of the framework always open handles with FD_CLOEXEC. If this is the case for .NET, then after fork but before exec we simply have to remove this flag from any standard IO handles or any specified InheritedHandles. One downside of this approach is that there could be an open non-FD_CLOEXEC handle created by native code, but arguably this is not something we should be trying to control for.
• We can loop over all file descriptors after fork but before exec and close them if they are not one of the redirected standard IO descriptors or one of the specified InheritedHandles. This loop can be over a fixed integer range, over the entries in /proc/self/fd if it is available, or using one of these methods for the range of descriptors below the max inherited value and then using closefrom after that if closefrom is available.

[jhudsoncedaron]

over the entries in /proc/self/fd if it is available

So I'm going to add some information here. Yes, you can write this code. Unfortunately you can't use opendir() because opendir() calls malloc() and you can't all malloc() in a fork() child in a multi-threaded process. If another thread was in malloc() at the time, you get a deadlock. The way to allocate memory not on the stack is mmap().

This means you need to write platform specific code for all platforms. It's not even hard. I've done it before.

[tmds]

ne downside of this approach is that there could be an open non-FD_CLOEXEC handle created by native code, but arguably this is not something we should be trying to control for.

I'd prefer to accept that Linux doesn't support the concept of not inheriting handles well. And by design requires multi-threaded applications to create CLOEXEC handles.

public SafeHandle[] InheritedHandles { get; set; }

This should be some IntPtr -> SafeHandle Dictionary type. You want to control the handle values that are seen by the child.

If InheritedHandles is null, the current behavior is maintained.

And if it is set, we can use this on Windows for bInheritHandles=false to eliminate taking the lock.

Likely we should throw if any of the provided InheritedHandles are specifically flagged as not being inheritable.

This gives issue for Linux CLOEXEC handles that you try to inherit.

InheritedHandles will allow a superset of what is proposed in https://github.com/dotnet/corefx/issues/35685.

[madelson]

This should be some IntPtr -> SafeHandle Dictionary type. You want to control the handle values that are seen by the child.

@tmds can you explain? SafeHandle holds an IntPtr internally which you can retrieve with DangerousGetHandle(). My understanding is that modern .NET code should use SafeHandle over IntPtr to make it easier to ensure that handles are GC'd at the right time.

[tmds]

@tmds can you explain? SafeHandle holds an IntPtr internally which you can retrieve with DangerousGetHandle(). My understanding is that modern .NET code should use SafeHandle over IntPtr to make it easier to ensure that handles are GC'd at the right time.

These IntPtr are the values of filedescriptors the program is operating on. For example, stderr is file descriptor 2. If you ad 2>/dev/null to a commandline, you're telling it to open /dev/null and make that file descriptor 2 of the program that is started.
Between fork and execve, dup2/dup3 are used to map the SafeHandle.IntPtrs to the fds the application is expecting.

[jhudsoncedaron]

@madelson If I were actually trying to use this API for explicitly controlling extra inherited handles, it's most likely I only have them in IntPtr, not SafeHandle in the first place, because they came from native API requests.

[madelson]

@jhudsoncedaron I may be mistaken, but doesn't SafeHandle work with p/invoke?

[jhudsoncedaron]

@madelson: Try declaring two functions with identical signatures except for return two different derived classes of SafeHandle because the handle ownership rule in effect depends on the call locus.

SafeHandle is great for higher-level abstractions. SafeHandle is great for libraries. SafeHandle is a problem for a streight-shot set of calls to set up and use a native handle in one function ten times (that is, ten functions, each with one native handle local scoped to the function.)

[whoisj]

Native API more often than not return integer or Boolean values to signal success/failure, with output values generally being written to provided arguments. Think C#'s out parameters. In which case, it is easy to provide both IntPtr and SafeHandle support.

```C#
public static extern int NativeFunction(IntPtr input, out IntPtr handle_out);

public static extern int NativeFunction(SafeHandle input, out SafeHandle handle_out);
```

[danmosemsft]

cc @JeremyKuhne @carlossanlop (for IO)
@stephentoub @jkotas how would you feel about the proposal above? Disregarding the exact API shape which can be figured out. For Windows, the value seems clear and also the implementation. The sticking point is that it would probably have to throw PNSE for Linux, unless someone comes up with a new idea here. So we would have a Windows-only API.

[jkotas]

I would be fine with adding an API for this.

The sticking point is that it would probably have to throw PNSE for Linux

It can be no-op on platforms where this cannot be controlled. The API would basically say "I do not depend on inheritance of random handles in the process, except for the ones that I have explicitly listed here. The implementation may, but is not required to, take advantage of this fact for better performance.".

[jkotas]

Related: https://www.microsoft.com/en-us/research/publication/a-fork-in-the-road/

[danmosemsft]

It can be no-op on platforms where this cannot be controlled. The API would basically say "I do not depend on inheritance of random handles in the process, except for the ones that I have explicitly listed here. The implementation may, but is not required to, take advantage of this fact for better performance.".

My only thought here is, would that make it hard to implement later? One might say "may, but is not required to" in the docs, but that doesn't stop code assuming that it doesn't, and getting surprised later. Whereas throwing would make it possible to implement later.

(Sounds like possibly spawn and its file_actions might offer a way forward on Linux someday ? - but I am completely out of my area of expertise. @tmds?)

[jhudsoncedaron]

@danmosemsft : I've been able to do it on Linux since it was first proposed. If I had to do it on Mac I'd have revert to the close 32k handles loop because I don't have a Mac to verify the /proc route. Unfortunately, implementing handle inheritance control as inherit all is deadlock territory.

You can't use the spawn family because you want to support changing userid and current directory.

[jkotas]

There are two different purposes discussed here:

1. Better create process performance / scalability: An API that opts-in better create process performance can be no-op where the underlying platform does not support. It is what I had in my mind in my response above.

2. Functionality - guaranteeing that all random handles are closed: An API for this would have to throw when the underlying platform does not allow us to implement this. It is unclear what this API would do when it is possible to implement, but the implementation is slow (ie via the 32k handles loop).

We may want to look at these as two APIs.

doesn't stop code assuming that it doesn't

The code out there makes invalid assumptions like these all the time. We do not maintain bug-for-bug compatibility between major .NET Core versions to keep code with invalid assumptions like these working.

[jhudsoncedaron]

@jkotas : I can write a fast solution for a Mac if you can test it.

[Tyrrrz]

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]

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 https://github.com/dotnet/runtime/pull/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 (https://github.com/dotnet/core-setup/pull/9010).

[Tyrrrz]

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

[Joe4evr]

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.

[danmosemsft]

@terrajobst is that consistent with your TFM plan?

[danmosemsft]

Ping @terrajobst

[jkotas]

This should be ready to review. We have a plan for Windows-specific APIs now, we have just approved other similar Windows specific properties (https://github.com/dotnet/runtime/issues/28114).

[terrajobst]

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

C# namespace System.Diagnostics { public partial class ProcessStartInfo { public bool InheritHandles { get; [MinimumOSPlatform("windows7.0")] set; } } }

[jhudsoncedaron]

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]

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.