From a1ccba770ff8235d893e3df1c4876b6f25aae0a2 Mon Sep 17 00:00:00 2001 From: Bruno Haible Date: Sun, 19 Jul 2009 12:45:28 +0200 Subject: [PATCH] Fix handling of closed stdin/stdout/stderr on mingw. --- ChangeLog | 14 +++++++++++++ lib/execute.c | 12 +++++------ lib/pipe.c | 20 +++++++++--------- lib/w32spawn.h | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++--- tests/test-pipe.c | 52 ++++++++++++++++++++++++++++++++-------------- 5 files changed, 126 insertions(+), 34 deletions(-) diff --git a/ChangeLog b/ChangeLog index 2009441af..44d91e6ea 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,19 @@ 2009-07-19 Bruno Haible + 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 . + (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 a6911d97b..5127884c7 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 64fa6a247..c63321c25 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 ee286dd8c..375d0cb5b 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 , 2003. This program is free software: you can redistribute it and/or modify @@ -24,11 +24,13 @@ #include #include +#include #include #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 c5425094d..023f4034a 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 +#endif + #include #include #include @@ -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++) { -- 2.11.0