Re: Help with create_pipe_bidi

[Bruno Haible]

Eric Blake wrote:
> The test is still buggy on mingw; here's a followup for the test to at
> least let it compile, and I'm still investigating why the test fails on
> mingw (but it looks like the problem is in:
> 
> 156       orig_stdin = dup_noinherit (STDIN_FILENO);
> (gdb) n
> (null): _open_osfhandle failed: Bad file descriptor
> 
> when stdin is closed - dup_noinherit needs to silently ignore this failure).

There were a couple of more problems than this one.
  - The case of an INVALID_HANDLE_VALUE was not handled in dup_noinherit.
  - The new file descriptor created by dup_noinherit was not marked
    O_NOINHERIT.
  - The functions fd_safer and dup return file descriptors that are not marked
    O_NOINHERIT.
  - The file descriptors returned by dup_noinherit could clash with
    STDIN_FILENO, STDOUT_FILENO, or STDERR_FILENO.
  - The unit test expected that for an initially closed fd, close(fd) fails
    with error EBADF. Not so on native Windows platforms, where the file
    descriptor is actually initially open with an INVALID_HANDLE_VALUE handle.
    close(fd) succeeds in this case.

I've committed this. The test now passes on mingw (and of course also on
Linux and Cygwin).


2009-07-19  Bruno Haible  <address@hidden>

        Fix handling of closed stdin/stdout/stderr on mingw.
        * lib/w32spawn.h: Include unistd.h.
        (dup_noinherit): Return -1 if the old handle is invalid. Allocate new
        file descriptor with O_NOINHERIT flag.
        (fd_safer_noinherit): New function, based on fd-safer.c.
        (dup_safer_noinherit): New function, based on dup-safer.c.
        (undup_safer_noinherit): New function.
        * lib/execute.c (execute) [WIN32]: Use dup_safer_noinherit instead of
        dup_noinherit. Use undup_safer_noinherit instead of dup2 and close.
        * lib/pipe.c (create_pipe) [WIN32]: Likewise. Use fd_safer_noinherit
        instead of fd_safer.
        * tests/test-pipe.c: Include <windows.h>.
        (child_main) [WIN32]: Test the handle of STDERR_FILENO, not its close() 
result.

--- lib/execute.c.orig  2009-07-19 12:44:53.000000000 +0200
+++ lib/execute.c       2009-07-19 11:41:11.000000000 +0200
@@ -119,11 +119,11 @@
 
   /* Save standard file handles of parent process.  */
   if (null_stdin)
-    orig_stdin = dup_noinherit (STDIN_FILENO);
+    orig_stdin = dup_safer_noinherit (STDIN_FILENO);
   if (null_stdout)
-    orig_stdout = dup_noinherit (STDOUT_FILENO);
+    orig_stdout = dup_safer_noinherit (STDOUT_FILENO);
   if (null_stderr)
-    orig_stderr = dup_noinherit (STDERR_FILENO);
+    orig_stderr = dup_safer_noinherit (STDERR_FILENO);
   exitcode = -1;
 
   /* Create standard file handles of child process.  */
@@ -173,11 +173,11 @@
 
   /* Restore standard file handles of parent process.  */
   if (null_stderr)
-    dup2 (orig_stderr, STDERR_FILENO), close (orig_stderr);
+    undup_safer_noinherit (orig_stderr, STDERR_FILENO);
   if (null_stdout)
-    dup2 (orig_stdout, STDOUT_FILENO), close (orig_stdout);
+    undup_safer_noinherit (orig_stdout, STDOUT_FILENO);
   if (null_stdin)
-    dup2 (orig_stdin, STDIN_FILENO), close (orig_stdin);
+    undup_safer_noinherit (orig_stdin, STDIN_FILENO);
 
   if (termsigp != NULL)
     *termsigp = 0;
--- lib/pipe.c.orig     2009-07-19 12:44:53.000000000 +0200
+++ lib/pipe.c  2009-07-19 11:41:20.000000000 +0200
@@ -134,13 +134,13 @@
 
   if (pipe_stdout)
     if (_pipe (ifd, 4096, O_BINARY | O_NOINHERIT) < 0
-       || (ifd[0] = fd_safer (ifd[0])) < 0
-       || (ifd[1] = fd_safer (ifd[1])) < 0)
+       || (ifd[0] = fd_safer_noinherit (ifd[0])) < 0
+       || (ifd[1] = fd_safer_noinherit (ifd[1])) < 0)
       error (EXIT_FAILURE, errno, _("cannot create pipe"));
   if (pipe_stdin)
     if (_pipe (ofd, 4096, O_BINARY | O_NOINHERIT) < 0
-       || (ofd[0] = fd_safer (ofd[0])) < 0
-       || (ofd[1] = fd_safer (ofd[1])) < 0)
+       || (ofd[0] = fd_safer_noinherit (ofd[0])) < 0
+       || (ofd[1] = fd_safer_noinherit (ofd[1])) < 0)
       error (EXIT_FAILURE, errno, _("cannot create pipe"));
 /* Data flow diagram:
  *
@@ -153,11 +153,11 @@
 
   /* Save standard file handles of parent process.  */
   if (pipe_stdin || prog_stdin != NULL)
-    orig_stdin = dup_noinherit (STDIN_FILENO);
+    orig_stdin = dup_safer_noinherit (STDIN_FILENO);
   if (pipe_stdout || prog_stdout != NULL)
-    orig_stdout = dup_noinherit (STDOUT_FILENO);
+    orig_stdout = dup_safer_noinherit (STDOUT_FILENO);
   if (null_stderr)
-    orig_stderr = dup_noinherit (STDERR_FILENO);
+    orig_stderr = dup_safer_noinherit (STDERR_FILENO);
   child = -1;
 
   /* Create standard file handles of child process.  */
@@ -218,11 +218,11 @@
 
   /* Restore standard file handles of parent process.  */
   if (null_stderr)
-    dup2 (orig_stderr, STDERR_FILENO), close (orig_stderr);
+    undup_safer_noinherit (orig_stderr, STDERR_FILENO);
   if (pipe_stdout || prog_stdout != NULL)
-    dup2 (orig_stdout, STDOUT_FILENO), close (orig_stdout);
+    undup_safer_noinherit (orig_stdout, STDOUT_FILENO);
   if (pipe_stdin || prog_stdin != NULL)
-    dup2 (orig_stdin, STDIN_FILENO), close (orig_stdin);
+    undup_safer_noinherit (orig_stdin, STDIN_FILENO);
 
   if (pipe_stdin)
     close (ofd[0]);
--- lib/w32spawn.h.orig 2009-07-19 12:44:53.000000000 +0200
+++ lib/w32spawn.h      2009-07-19 12:42:27.000000000 +0200
@@ -1,5 +1,5 @@
 /* Auxiliary functions for the creation of subprocesses.  Native Woe32 API.
-   Copyright (C) 2003, 2006-2009 Free Software Foundation, Inc.
+   Copyright (C) 2001, 2003, 2004-2009 Free Software Foundation, Inc.
    Written by Bruno Haible <address@hidden>, 2003.
 
    This program is free software: you can redistribute it and/or modify
@@ -24,11 +24,13 @@
 
 #include <stdbool.h>
 #include <string.h>
+#include <unistd.h>
 #include <errno.h>
 
 #include "xalloc.h"
 
-/* Duplicates a file handle, making the copy uninheritable.  */
+/* Duplicates a file handle, making the copy uninheritable.
+   Returns -1 for a file handle that is equivalent to closed.  */
 static int
 dup_noinherit (int fd)
 {
@@ -37,6 +39,12 @@
   HANDLE new_handle;
   int nfd;
 
+  if (old_handle == INVALID_HANDLE_VALUE)
+    /* fd is closed, or is open to no handle at all.
+       We cannot duplicate fd in this case, because _open_osfhandle fails for
+       an INVALID_HANDLE_VALUE argument.  */
+    return -1;
+
   if (!DuplicateHandle (curr_process,              /* SourceProcessHandle */
                        old_handle,                 /* SourceHandle */
                        curr_process,               /* TargetProcessHandle */
@@ -47,13 +55,61 @@
     error (EXIT_FAILURE, 0, _("DuplicateHandle failed with error code 0x%08x"),
           (unsigned int) GetLastError ());
 
-  nfd = _open_osfhandle ((long) new_handle, O_BINARY);
+  nfd = _open_osfhandle ((long) new_handle, O_BINARY | O_NOINHERIT);
   if (nfd < 0)
     error (EXIT_FAILURE, errno, _("_open_osfhandle failed"));
 
   return nfd;
 }
 
+/* Returns a file descriptor equivalent to FD, except that the resulting file
+   descriptor is none of STDIN_FILENO, STDOUT_FILENO, STDERR_FILENO.
+   FD must be open and non-inheritable.  The result will be non-inheritable as
+   well.
+   If FD < 0, FD itself is returned.  */
+static int
+fd_safer_noinherit (int fd)
+{
+  if (STDIN_FILENO <= fd && fd <= STDERR_FILENO)
+    {
+      /* The recursion depth is at most 3.  */
+      int nfd = fd_safer_noinherit (dup_noinherit (fd));
+      int saved_errno = errno;
+      close (fd);
+      errno = saved_errno;
+      return nfd;
+    }
+  return fd;
+}
+
+/* Duplicates a file handle, making the copy uninheritable and ensuring the
+   result is none of STDIN_FILENO, STDOUT_FILENO, STDERR_FILENO.
+   Returns -1 for a file handle that is equivalent to closed.  */
+static int
+dup_safer_noinherit (int fd)
+{
+  return fd_safer_noinherit (dup_noinherit (fd));
+}
+
+/* Undoes the effect of TEMPFD = dup_safer_noinherit (ORIGFD);  */
+static void
+undup_safer_noinherit (int tempfd, int origfd)
+{
+  if (tempfd >= 0)
+    {
+      if (dup2 (tempfd, origfd) < 0)
+        error (EXIT_FAILURE, errno, _("cannot restore fd %d: dup2 failed"),
+              origfd);
+      close (tempfd);
+    }
+  else
+    {
+      /* origfd was closed or open to no handle at all.  Set it to a closed
+        state.  This is (nearly) equivalent to the original state.  */
+      close (origfd);
+    }
+}
+
 /* Prepares an argument vector before calling spawn().
    Note that spawn() does not by itself call the command interpreter
      (getenv ("COMSPEC") != NULL ? getenv ("COMSPEC") :
--- tests/test-pipe.c.orig      2009-07-19 12:44:53.000000000 +0200
+++ tests/test-pipe.c   2009-07-19 12:29:41.000000000 +0200
@@ -20,6 +20,12 @@
 #include "pipe.h"
 #include "wait-process.h"
 
+#if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
+/* Get declarations of the Win32 API functions.  */
+# define WIN32_LEAN_AND_MEAN
+# include <windows.h>
+#endif
+
 #include <errno.h>
 #include <fcntl.h>
 #include <stdbool.h>
@@ -47,7 +53,6 @@
 child_main (int argc, char *argv[])
 {
   char buffer[1];
-  int i;
   int fd;
 
   ASSERT (argc == 3);
@@ -61,29 +66,46 @@
   buffer[0]++;
   ASSERT (write (STDOUT_FILENO, buffer, 1) == 1);
 
-  errno = 0;
-#ifdef F_GETFL
-  /* Try to keep stderr open for better diagnostics.  */
-  i = fcntl (STDERR_FILENO, F_GETFL);
-#else
-  /* But allow compilation on mingw.  You might need to disable this code for
-     debugging failures.  */
-  i = close (STDERR_FILENO);
-#endif
+#if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__
+  /* On Win32, the initial state of unassigned standard file descriptors is
+     that they are open but point to an INVALID_HANDLE_VALUE.  Thus
+     close (STDERR_FILENO) would always succeed.  */
   switch (atoi (argv[2]))
     {
     case 0:
-      /* Expect fd 2 was open.  */
-      ASSERT (i >= 0);
+      /* Expect fd 2 is open to a valid handle.  */
+      ASSERT ((HANDLE) _get_osfhandle (STDERR_FILENO) != INVALID_HANDLE_VALUE);
       break;
     case 1:
-      /* Expect fd 2 was closed.  */
-      ASSERT (i < 0);
-      ASSERT (errno == EBADF);
+      /* Expect fd 2 is pointing to INVALID_HANDLE_VALUE.  */
+      ASSERT ((HANDLE) _get_osfhandle (STDERR_FILENO) == INVALID_HANDLE_VALUE);
       break;
     default:
       ASSERT (false);
     }
+#elif defined F_GETFL
+  /* On Unix, the initial state of unassigned standard file descriptors is
+     that they are closed.  */
+  {
+    int ret;
+    errno = 0;
+    ret = fcntl (STDERR_FILENO, F_GETFL);
+    switch (atoi (argv[2]))
+      {
+      case 0:
+       /* Expect fd 2 is open.  */
+       ASSERT (ret >= 0);
+       break;
+      case 1:
+       /* Expect fd 2 is closed.  */
+       ASSERT (ret < 0);
+       ASSERT (errno == EBADF);
+       break;
+      default:
+       ASSERT (false);
+      }
+  }
+#endif
 
   for (fd = 3; fd < 7; fd++)
     {