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 newly created file descriptors non-inheritable #11399

Closed
garrison opened this issue May 22, 2015 · 7 comments
Closed

Make newly created file descriptors non-inheritable #11399

garrison opened this issue May 22, 2015 · 7 comments
Assignees
Labels
io Involving the I/O subsystem: libuv, read, write, etc. needs decision A decision on this change is needed
Milestone

Comments

@garrison
Copy link
Member

Currently, when a subprocess is executed in julia, the child process inherits all julia file descriptors that are currently open at the moment of exec (this is true at least on Linux). There are many good reasons to instead make file descriptors non-inheritable by default -- I will point toward Python's PEP on the subject, which says it all much better than I could. In particular, it notes that (in addition to Python) Perl, Go, and Ruby each make non-inheritable file descriptors the default.

As mentioned in #8295, libuv already opens all files with non-inheritable file descriptors. However, the Base.open function calls ios_file, which does not set the O_CLOEXEC/FD_CLOEXEC flag by default.

So here's a possible path forward:

  • Reaching consensus that this indeed ought to be changed
  • Identifying pieces of code that should change. ios_file is one; there are likely a few others, too.
  • Considering what form the problem takes on Windows (it may not exist there; I am not certain.)
  • Identifying existing code (if any) that may rely on inheritable file descriptors. (Currently, setting O_CLOEXEC in ios_file leads to all tests passing in Julia, so this is a good sign.)
  • Creating a way for Julia users to explicitly set file descriptors as being inheritable if they wish.
@garrison
Copy link
Member Author

I should link to the specific section of Python's PEP on the known issues with inheritable file descriptors. Note that there are both subtle bugs, as well as security issues, that can result from them.

@JeffBezanson JeffBezanson added the io Involving the I/O subsystem: libuv, read, write, etc. label May 28, 2015
@ScottPJones
Copy link
Contributor

👍 For whatever value my opinion has, I do believe this is important, for the security reasons alone...

@StefanKarpinski
Copy link
Member

It seems sensible to me to follow these other languages on this.

@StefanKarpinski StefanKarpinski added the needs decision A decision on this change is needed label Jun 10, 2015
@tkelman
Copy link
Contributor

tkelman commented Jun 10, 2015

Anyone know if node/io.js do this, or if they chose not to for some reason?

edit: needed to read more carefully, I'm guessing they do since this is going through ios_file rather than libuv

@garrison
Copy link
Member Author

@tkelman your suspicion is correct; see here and here.

@nalimilan
Copy link
Member

FWIW, this was also recently discussed by GLib developers: https://mail.gnome.org/archives/gtk-devel-list/2015-March/msg00038.html They decided that the only reliable solution is to close FDs before spawning new processes, as passing O_CLOEXEC to all calls to open never completely works: there are always a few incorrect calls, either in application or in third-party library code. But O_CLOEXEC is still considered a good idea.

@Keno Keno added this to the 0.4.0 milestone Jul 10, 2015
@JeffBezanson JeffBezanson self-assigned this Jul 10, 2015
@garrison
Copy link
Member Author

Cross referencing b3d6c25 here.

Also, I believe that when O_CLOEXEC does not work (or is not available), fcntl should be called with FD_CLOEXEC on the new file descriptor. This will no longer be atomic (so if a process is forked between the calls it should inherit the file descriptor) but is much better than nothing.

EDIT: quoting from the PEP linked above:

On Linux older than 2.6.23, O_CLOEXEC flag is simply ignored. So fcntl() must be called to check if the file descriptor is non-inheritable: O_CLOEXEC is not supported if the FD_CLOEXEC flag is missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
io Involving the I/O subsystem: libuv, read, write, etc. needs decision A decision on this change is needed
Projects
None yet
Development

No branches or pull requests

7 participants