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

Set (or provide option to set) FD_CLOEXEC flag on opened sockets #2252

Closed
rcombs opened this issue Jan 19, 2018 · 6 comments

Comments

@rcombs
Copy link
Contributor

commented Jan 19, 2018

I did this

Open a HTTPS connection with libcurl

I expected the following

The resulting socket should have the FD_CLOEXEC flag set to avoid being inherited by child processes, or there should be a flag to make that happen.

curl/libcurl version

Currently libcurl 7.30.0; appears to be unchanged in master

operating system

Tested on Linux; applies to any POSIX.

I would expect this to entail OR-ing SOCK_CLOEXEC into the socktype when calling socket() in Curl_socket on platforms supporting that flag (e.g. Linux and FreeBSD 10+), and calling fcntl(*sockfd, F_SETFD, FD_CLOEXEC) otherwise (which is racy in a multithreaded environment, but better than nothing, and the common behavior).

@bagder

This comment has been minimized.

Copy link
Member

commented Jan 19, 2018

Isn't CURLOPT_SOCKOPTFUNCTION giving you exactly that ability? Or are you suggesting that we should rather provide an option for the specific purpose of toggling FD_CLOEXEC ?

@rcombs

This comment has been minimized.

Copy link
Contributor Author

commented Jan 19, 2018

Yes, that would work, and if you think that's the best way to go, I'll use that (though I think in any case that should be recommended in documentation). I do think this should be built-in, though, since it's needed in basically any application that spawns child processes.
(I've thought a bit about whether this should be the default or not; I can't think of a case where you'd fork(), execve(), and then want to still have FDs opened by curl remain open in the child… but maybe you have some idea?)

@bagder

This comment has been minimized.

Copy link
Member

commented Jan 19, 2018

My only concern about doing this by default is the risk that there are two users out there who rely on the bit not being set. But I agree with you that they ought to be a small minority. If we set the bit by default before this callback is called, such users could still have the option to unset the bit using this callback...

@rcombs

This comment has been minimized.

Copy link
Contributor Author

commented Jan 19, 2018

Keep in mind that it can't be set before the call on Darwin or older FreeBSD or Linux (not sure what the supported minimum of any platform is here), so there still needs to be a post-connect() fcntl() there. That could be done only in the non-callback path, though, with the docs on the callback updated to indicate that if you use it, you have to set CLOEXEC yourself if you want that.

@bagder

This comment has been minimized.

Copy link
Member

commented Apr 29, 2018

Unfortunately this hasn't caught anyone's interest so far. Will add this to docs/TODO and close this issue for now.

@bagder bagder closed this in 0cbfff9 Apr 29, 2018

@karlito-go

This comment has been minimized.

Copy link

commented Jul 11, 2018

It's unfortunate that curl isn't doing this by default for all fds (it's safe practice). However, it is not necessary in threaded apps if you instead close extraneous fds after fork but before exec. E.g. like python's subprocess(close_fds=True) does. That may be somewhat more cpu expensive though, depending on how you iterate the fds.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 9, 2018

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