bug#14803: Setting close-on-exec flag consistently

[Paul Eggert]

On 07/06/2013 04:31 AM, Eli Zaretskii wrote:
> The Windows build has its own implementations
> of 'pipe' and 'socket' (because it needs to make them behave like
> Posix APIs, and also because it needs to watch each subprocess and
> each socket for the benefit of 'select', 'waitpid', SIGCHLD, etc.).
> So the replacements from gnulib won't do.

The gnulib replacements do that sort of thing too.  Emacs
uses a hybrid approach, which uses gnulib for some things
and its own (pre-gnulib) solution for others, and it should
be possible to keep the hybrid working in this case.  In the
long run it may be better to merge the two approaches rather
than maintain a hybrid, but that issue doesn't need to be
addressed now.

> Socket handles _are_ inherited, but making them non-inheritable is not
> easy (some say impossible in the general case), and since no one
> complained till now, I'm willing to live with that until we hear some
> real problems.

That's the intent of this patch.  On systems like w32 that
lack SOCK_CLOEXEC, the code creates sockets just as it did
before (using plain 'socket' and 'accept' rather than
'socket' and 'accept4' with SOCK_CLOEXEC).  The only
difference is that the proposed code then invokes
fcntl (fd, F_SETFD, FD_CLOEXEC) to set the close-on-exec
flag; this is needed on POSIXish systems that don't have
SOCK_CLOEXEC.  From what you say, on w32 platforms it's OK
if this is a no-op.  w32.c's fcntl currently returns
SOCKET_ERROR for this fcntl call, which should be fine,
since Emacs doesn't check for an error.

> For mktemp/mkstemp in filelock.c, sys_open in w32.c already makes all
> handles not inheritable by default, so that problem doesn't exist,
> either.

This should be OK, since the w32 port doesn't define
HAVE_MKOSTEMP, so Emacs should use mkstemp or mktemp as it
did before.  Emacs will now invoke fcntl (fd, F_SETFD,
FD_CLOEXEC) on the resulting file descriptor but that should
be OK as described above.

> As for emacs_open, it needs to use O_NOINHERIT instead of O_CLOEXEC
> (or just use zero, since w32.c already adds the O_NOINHERIT flag in
> sys_open).

The proposed patch does this, as fcntl.h should #define
O_CLOEXEC to O_NOINHERIT on w32.

> So I think we should simply not compile lib/fcntl.c and lib/pipe2.c
> (including their dependency modules) on MS-Windows.

That should be OK.  w32.c will need minor tweaks to support
F_DUPFD_CLOEXEC and pipe2, something along the following lines.

=== modified file 'nt/ChangeLog'
--- nt/ChangeLog        2013-07-06 09:44:23 +0000
+++ nt/ChangeLog        2013-07-06 16:55:23 +0000
@@ -3,6 +3,8 @@
        Make file descriptors close-on-exec when possible (Bug#14803).
        * gnulib.mk: Remove empty gl_GNULIB_ENABLED_verify section;
        otherwise, gnulib-tool complains given close-on-exec changes.
+       * inc/ms-w32.h (pipe): Remove.
+       (pipe2): New macro.
 
 2013-06-25  Juanma Barranquero  <lekktu@gmail.com>
 

=== modified file 'nt/inc/ms-w32.h'
--- nt/inc/ms-w32.h     2013-05-15 17:06:26 +0000
+++ nt/inc/ms-w32.h     2013-07-06 16:55:23 +0000
@@ -211,7 +211,7 @@
 #define mktemp  sys_mktemp
 #undef open
 #define open    sys_open
-#define pipe    sys_pipe
+#define pipe2   sys_pipe2
 #undef read
 #define read    sys_read
 #define rename  sys_rename

=== modified file 'src/ChangeLog'
--- src/ChangeLog       2013-07-06 16:37:12 +0000
+++ src/ChangeLog       2013-07-06 17:12:42 +0000
@@ -33,6 +33,8 @@
        Make newly-created socket close-on-exec.
        * sysdep.c (emacs_open, emacs_fopen):
        Make new-created descriptor close-on-exec.
+       * w32.c (fcntl): Support F_DUPFD_CLOEXEC well enough for Emacs.
+       * w32.c, w32.h (sys_pipe2): Rename from 'pipe', with new flags arg.
 
 2013-07-06  Eli Zaretskii  <eliz@gnu.org>
 

=== modified file 'src/w32.c'
--- src/w32.c   2013-06-21 20:11:44 +0000
+++ src/w32.c   2013-07-06 17:12:42 +0000
@@ -6719,10 +6719,16 @@
 }
 
 /* Windows does not have an fcntl function.  Provide an implementation
-   solely for making sockets non-blocking.  */
+   good enough for Emacs.  */
 int
 fcntl (int s, int cmd, int options)
 {
+  /* In the w32 Emacs port, fcntl (fd, F_DUPFD_CLOEXEC, fd1) is always
+     invoked in a context where fd1 is closed and all descriptors less
+     than fd1 are open, so sys_dup is an adequate implementation.  */
+  if (cmd == F_DUPFD_CLOEXEC)
+    return sys_dup (s);
+
   if (winsock_lib == NULL)
     {
       errno = ENETDOWN;
@@ -6864,13 +6870,14 @@
   return rc;
 }
 
-/* Unix pipe() has only one arg */
 int
-sys_pipe (int * phandles)
+sys_pipe2 (int * phandles, int pipe2_flags)
 {
   int rc;
   unsigned flags;
 
+  eassert (pipe2_flags == O_CLOEXEC);
+
   /* make pipe handles non-inheritable; when we spawn a child, we
      replace the relevant handle with an inheritable one.  Also put
      pipes into binary mode; we will do text mode translation ourselves

=== modified file 'src/w32.h'
--- src/w32.h   2013-03-05 22:35:41 +0000
+++ src/w32.h   2013-07-06 16:55:23 +0000
@@ -188,7 +188,7 @@
 
 extern int fchmod (int, mode_t);
 extern int sys_rename_replace (char const *, char const *, BOOL);
-extern int sys_pipe (int *);
+extern int sys_pipe2 (int *, int);
 
 extern void set_process_dir (char *);
 extern int sys_spawnve (int, char *, char **, char **);