www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Phobos: Posix hands down open files to sub processes.

reply Marco Leise <Marco.Leise gmx.de> writes:
A common bug on Posix seems to be to forget to close file
descriptors not meant to stay open in child processes as this
search reveals:

http://www.google.de/search?q=%22set+FD_CLOEXEC%22+OR+%22FD_CLOEXEC+not+set%22+bug

Unlike on Windows where this is opt-in, it is opt-out and you
have to use FD_CLOEXEC, which closes the file descriptor
across an "execve" (used to execute another program).

My proposal is here to add this flag to all file descriptors
Phobos opens (files, sockets, streams) and make it a
convention for Phobos development on Posix. The assumption is
that this is the saner default nowadays, kudos to Windows here.

This issue came up in the std.process thread, where hacks
around it for the sake of OS independent behavior are
discussed.

Would this be feasible and work ?

-- 
Marco
Mar 10 2013
parent reply Marco Leise <Marco.Leise gmx.de> writes:
I've taken a look around at how other cross-platform languages
go about this...

OpenJDK:


 All file descriptors that are opened in the JVM and not
 specifically destined for a subprocess should have the
 close-on-exec flag set.  If we don't set it, then careless 3rd
 party native code might fork and exec without closing all
 appropriate file descriptors (e.g. as we do in closeDescriptors in
 UNIXProcess.c), and this in turn might:

 - cause end-of-file to fail to be detected on some file
   descriptors, resulting in mysterious hangs, or

 - might cause an fopen in the subprocess to fail on a system
   suffering from bug 1085341.

 (Yes, the default setting of the close-on-exec flag is a Unix
 design flaw)


Python:


 def _mkstemp_inner(dir, pre, suf, flags):
     """Code common to mkstemp, TemporaryFile, and NamedTemporaryFile."""
=20
     names =3D _get_candidate_names()
=20
     for seq in range(TMP_MAX):
         name =3D next(names)
         file =3D _os.path.join(dir, pre + name + suf)
         try:
             fd =3D _os.open(file, flags, 0o600)
             _set_cloexec(fd)
             return (fd, _os.path.abspath(file))
         except FileExistsError:

=20
     raise FileExistsError("No usable temporary file name found")


Ruby:


 Ruby sets close-on-exec flags of all file descriptors by default since
 Ruby 2.0.0. So you don=E2=80=99t need to set by yourself. Also, unsetting=


 a

 close-on-exec flag can cause file descriptor leak if another thread
 use fork() and exec() (via system() method for example).
 If you really needs file descriptor inheritance to child process,
 use spawn()=E2=80=98s argument such as fd=3D>fd.


Haskell:

I found some discussion here in 2009: http://therning.org/magnus/archives/7=
27
I think they just rely on the C library at the moment for their System.IO,
which in turn doesn't set FD_CLOEXEC. But I'm really bad at reading Haskell=
 :p

Rust:

Uses C stdlib.

-----------------------------------------------------

=46rom this small set of languages it looks like those that have their
own IO implementation and don't rely on the C lib, mostly set
FD_CLOEXEC by default.

Despite the increased maintenance cost I think we should adapt that
behavoir in D as well.

--=20
Marco
Mar 10 2013
parent reply "Vladimir Panteleev" <vladimir thecybershadow.net> writes:
On Monday, 11 March 2013 at 05:24:49 UTC, Marco Leise wrote:

 Python:


 def _mkstemp_inner(dir, pre, suf, flags):
     """Code common to mkstemp, TemporaryFile, and 
 NamedTemporaryFile."""




So it only sets the flag on temporary files?


 I think they just rely on the C library at the moment for their 
 System.IO,
 which in turn doesn't set FD_CLOEXEC.



 Rust:

 Uses C stdlib.


Is there any discussion on why libc doesn't do it, and what do 
APIs that wrap the C API do?


 Despite the increased maintenance cost I think we should adapt 
 that
 behavoir in D as well.


I don't think there would be a maintenance cost to speak of. 
Wouldn't it be a one-line addition to a few places?
Mar 10 2013
parent Marco Leise <Marco.Leise gmx.de> writes:
Am Mon, 11 Mar 2013 06:32:42 +0100
schrieb "Vladimir Panteleev" <vladimir thecybershadow.net>:


 On Monday, 11 March 2013 at 05:24:49 UTC, Marco Leise wrote:

 Python:


 def _mkstemp_inner(dir, pre, suf, flags):
     """Code common to mkstemp, TemporaryFile, and 
 NamedTemporaryFile."""




 
 So it only sets the flag on temporary files?


It's the only place where I could quickly identify it with a
grep. Even for Python 3.4 this is still an open issue it seems,
like this bug report shows: http://bugs.python.org/issue12107
It also discusses pros and cons a bit.


 I think they just rely on the C library at the moment for their 
 System.IO,
 which in turn doesn't set FD_CLOEXEC.


 

 Rust:

 Uses C stdlib.


 
 Is there any discussion on why libc doesn't do it, and what do 
 APIs that wrap the C API do?


I haven't researched any of that so far. All I know is that
glibc added a new flag with 2.7 for that:


 e (since glibc 2.7)
     Open the file with the O_CLOEXEC flag.  See open(2) for more
     information.  This flag is ignored for fdopen().



 Despite the increased maintenance cost I think we should adapt 
 that behavoir in D as well.


 
 I don't think there would be a maintenance cost to speak of. 
 Wouldn't it be a one-line addition to a few places?


More or less. You have to query the flags, add FD_CLOEXEC and
set them again. Error handling wouldn't be bad either. Its
enough to write a helper function for that.

-- 
Marco
Mar 10 2013