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

Disable socket inheritance on Windows and macOS #32903

Merged
merged 3 commits into from
Oct 20, 2018

Conversation

stephentoub
Copy link
Member

We already disabled it on Linux. Doing the same for Windows and macOS.

@davidsh, @geoffkizer, @tmds, can you think of any real scenarios that'll break from doing this? We don't currently have a Socket ctor that takes a handle, so someone would need to be inheriting the handle, passing the handle IntPtr into the child process in some way (e.g. cmdline arg), and then using something like FileStream to read/write it as an arbitrary handle. You can do that, but it seems really rare.

I wrote a test for this as well, but ended up deleting it, as it could easily have false positives, since the child process could easily have another handle with the same value as the one in the parent process, especially on Unix.

Fixes https://github.com/dotnet/corefx/issues/32902

We already disabled it on Linux.  Doing the same for Windows and macOS.
@davidsh
Copy link
Contributor

davidsh commented Oct 19, 2018

@CIPop - any feedback on this Sockets change?

@davidsh
Copy link
Contributor

davidsh commented Oct 19, 2018

@Tratcher

@tmds
Copy link
Member

tmds commented Oct 19, 2018

You can do that, but it seems really rare.

Agreed, it would be super rare.

You could add a test for this similar to this one: aspnet/KestrelHttpServer#2944.
For the test, you could exclude mac. Or if you nest it in a RemoteInvoke, you can avoid other children inheriting the socket due to the non-atomic CLOEXEC set.

On Linux we use the accept4 call to set CLOEXEC for accepted connections. I don't know how this is on Mac/Windows. Maybe this is copied from the listen socket, or maybe you need to also change the call/flags.

@stephentoub
Copy link
Member Author

On Linux we use the accept4 call to set CLOEXEC for accepted connections.

Ah, right, I'll take care of those.

You could add a test for this similar to..

Of course. Thanks.

@stephentoub
Copy link
Member Author

On Linux we use the accept4 call to set CLOEXEC for accepted connections. I don't know how this is on Mac/Windows. Maybe this is copied from the listen socket, or maybe you need to also change the call/flags.

Windows is inheriting it from the listening socket, so nothing further was needed there beyond my initial changes.

And we actually already handled the accept case with fcntl and CLOEXEC on systems without SOCK_CLOEXEC, so nothing further was needed there, either.

So I've just augmented the PR with a test.

@tmds
Copy link
Member

tmds commented Oct 19, 2018

Thanks for checking. The test LGTM. CI still has some issue with it:

2018-10-19 20:04:52,720: INFO: proc(55): run_and_log_output: Output: Unhandled Exception: Xunit.Sdk.TrueException: A test System.Net.Sockets.Tests, Version=4.2.1.0, Culture=neutral, PublicKeyToken=9d77cc7ad39b68eb!System.Net.Sockets.Tests.CreateSocket+<>c.<CtorAndAccept_SocketNotKeptAliveViaInheritance>b__11_0 forgot to Dispose() the result of RemoteInvoke()

@stephentoub
Copy link
Member Author

CI still has some issue with it:

Oops. The problem with a last minute tweak before git push. Fixed.

@stephentoub stephentoub merged commit 6ccd555 into dotnet:master Oct 20, 2018
@stephentoub stephentoub deleted the socketinherit branch October 20, 2018 00:24
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
* Disable socket inheritance on Windows and macOS

We already disabled it on Linux.  Doing the same for Windows and macOS.

* Add test for socket non-inheritance

* Disable new test on netfx


Commit migrated from dotnet/corefx@6ccd555
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants