bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: O_CLOEXEC support


From: Eric Blake
Subject: Re: O_CLOEXEC support
Date: Thu, 20 Aug 2009 17:42:40 -0600
User-agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.22) Gecko/20090605 Thunderbird/2.0.0.22 Mnenhy/0.7.6.666

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Paolo Bonzini on 8/20/2009 2:12 AM:
>> open - Required by POSIX 2008 - We can't support atomic O_CLOEXEC if the
>> OS doesn't obey POSIX 2008, but we can at least fake it.  But not all
>> programs will be using O_CLOEXEC, so it seems like it is best to add a
>> new
>> module open-cloexec
> 
> I'd rather keep the open module alone.  However, there's an interesting
> thing that goes to our advantage.  You cannot detect O_CLOEXEC at
> build-time, otherwise binary compatibility with older kernels goes
> south: I did this for GNU Smalltalk and two people reported that they
> needed to reboot to get GNU Smalltalk to run (because they had new
> kernel-headers and old kernel).  So, if O_CLOEXEC is requested, you must
> _unconditionally_ use lib/open.c.

Interesting.  What happens if you pass the bit value of O_CLOEXEC to an
older kernel that doesn't understand it - does the open fail with ENOSYS,
or does it cause a kernel panic (requiring a reboot for recovery)?  I hope
it's the former, in which case I agree that my popen-safer.c wrapper
function dup_noinherit should be taught to handle ENOSYS failure, along
these lines (and then move this into whatever wrapper we use in lib/open.c
when making O_CLOEXEC support more generic).  In other words, do I need to
install something like this?

diff --git a/lib/popen-safer.c b/lib/popen-safer.c
index 2182a2c..27c62a4 100644
- --- a/lib/popen-safer.c
+++ b/lib/popen-safer.c
@@ -23,6 +23,7 @@

 #include <errno.h>
 #include <fcntl.h>
+#include <stdbool.h>
 #include <unistd.h>

 #include "cloexec.h"
@@ -39,8 +40,14 @@
 static int
 open_noinherit (char const *name, int flags)
 {
+  bool needs_cloexec = !O_CLOEXEC;
   int fd = open (name, flags | O_CLOEXEC);
- -  if (0 <= fd && !O_CLOEXEC && set_cloexec_flag (fd, true) != 0)
+  if (fd < 0 && !needs_cloexec && errno == ENOSYS)
+    {
+      needs_cloexec = true;
+      fd = open (name, flags);
+    }
+  if (0 <= fd && needs_cloexec && set_cloexec_flag (fd, true) != 0)
     {
       int saved_errno = errno;
       close (fd);

> 
> So, the open-cloexec module can be used simply as a guard and it would
> just do "gl_REPLACE_OPEN".  At this point, just call it "cloexec".

We already have a cloexec module, which provides the utility wrapper
function set_cloexec_flag.  But I could agree to making this module cover
all the magic that turns on cloexec support across the rest of gnulib.

>> Any other fd creation functions I'm
>> forgetting?
> 
> socket(2).  You can again set up the cloexec module to do
> gl_REPLACE_SOCKET unconditionally.

And socketpair.  Thanks for catching that (I missed it because it uses
SOCK_CLOEXEC, rather than FD_CLOEXEC or O_CLOEXEC).

I also thought of creat, but that is fixed by using open, so nothing to
worry about there.

Also, openat() will need the same treatment as open().

>> Also, on mingw, I know of no way to open
>> the first free fd after a given point (the only arbitrary opening
>> point is
>> via dup2, but that clobbers any existing fd), so we'd either have to loop
>> from the target until we find a free fd, or temporarily tie up all free
>> fds lower than the target (neither of which sounds thread-safe, but then
>> again I didn't promise atomic operation).
> 
> I think the usual way to do it is the same as dupfd in lib/dup2.c, only
> with an == replaced by >=:
> 
> int
> dup_until (int fd, int target)
> {
>   int duped = dup (fd);
>   if (duped < 0 || duped >= target)
>     return duped;
>   else
>     {
>       int result = dup_until (fd, target);
>       int save_errno = errno;
>       close (duped);
>       errno = save_errno;
>       return result;
>     }
> }
> 
> This is much more thread-safe than the alternative

Yes, but unlike dup2.c where we are emulating fcntl(n, F_DUPFD, 3), you
are now introducing an arbitrarily large target, such that your recursion
could now risk overflowing the stack.  We'd probably have to rewrite it to
track dup allocation attempts using a heap struct if target is larger than
some minimum (or, is mingw subject to a compile-time maximum of open fds
where we can just return EMFILE up front?).

> For popen you couldn't do this at all, since pclose won't behave
> correctly for files opened with fdopen, so you wouldn't have atomicity.

Good point - O_CLOEXEC is POSIX, but 'e' is not; so when we fake 'e', we
can make the fopen version atomic (on some platforms) but not the popen
version.  But anyone using gnulib to get 'e' access already has to deal
with the fact that even for fopen, we don't promise atomicity.  Rather,
this is just a limitation that we can do a better job with fopen than we
can for popen, depending on the underlying platform.

>  OTOH gnulib provides twenty-odd ways to open pipes, so let's figure out
> for which of these *defaulting* to close-on-exec would be a good idea,
> and let's flip the default there once the basic cloexec support is there.

Or else add (yet another) parameter to create_pipe and variants to allow
the caller to make that selection.

- --
Don't work too hard, make some time for fun as well!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkqN33AACgkQ84KuGfSFAYCO+QCfW5CJLMIAAuwXwodZlVlDIDGs
jFgAn3PMNshxFsqzGLBgeKSfOfSfaI1K
=fF6l
-----END PGP SIGNATURE-----




reply via email to

[Prev in Thread] Current Thread [Next in Thread]