Fix handling of closed stdin/stdout/stderr on mingw.

5 files changed, 126 insertions, 34 deletions

diff --git a/ChangeLog b/ChangeLog
index 2009441af2..44d91e6ea8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,19 @@
 2009-07-19  Bruno Haible  <bruno@clisp.org>
 
+	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.
+
 	* tests/test-pipe.c (child_main, parent_main): New functions, extracted
 	from main.
 	(test_pipe): Pass an extra argument for disambiguation.
diff --git a/lib/execute.c b/lib/execute.c
index a6911d97b3..5127884c70 100644
--- a/lib/execute.c
+++ b/lib/execute.c
@@ -119,11 +119,11 @@ execute (const char *progname,
 
   /* 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 @@ execute (const char *progname,
 
   /* 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;
diff --git a/lib/pipe.c b/lib/pipe.c
index 64fa6a2473..c63321c257 100644
--- a/lib/pipe.c
+++ b/lib/pipe.c
@@ -134,13 +134,13 @@ create_pipe (const char *progname,
 
   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 @@ create_pipe (const char *progname,
 
   /* 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 @@ create_pipe (const char *progname,
 
   /* 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]);
diff --git a/lib/w32spawn.h b/lib/w32spawn.h
index ee286dd8cc..375d0cb5b8 100644
--- a/lib/w32spawn.h
+++ b/lib/w32spawn.h
@@ -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 <bruno@clisp.org>, 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 @@ dup_noinherit (int fd)
   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 @@ dup_noinherit (int fd)
     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") :
diff --git a/tests/test-pipe.c b/tests/test-pipe.c
index c5425094d4..023f4034a3 100644
--- a/tests/test-pipe.c
+++ b/tests/test-pipe.c
@@ -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 @@ static int
 child_main (int argc, char *argv[])
 {
   char buffer[1];
-  int i;
   int fd;
 
   ASSERT (argc == 3);
@@ -61,29 +66,46 @@ child_main (int argc, char *argv[])
   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++)
     {