From a8f637e3c49275e6789c05d67c1fbd1751e5610a Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 19 Aug 2009 07:15:54 -0600 Subject: [PATCH] popen: fix cygwin 1.5 bug when stdin closed * doc/posix-functions/popen.texi (popen): Document cygwin bugs. * modules/popen: New file. * modules/popen-tests: Likewise. * tests/test-popen.c: Likewise. * m4/popen.m4: Likewise. * lib/popen.c: Likewise. * lib/stdio.in.h (popen): New declaration. * m4/stdio_h.m4 (gl_STDIO_H_DEFAULTS): Add popen. * modules/stdio (Makefile.am): Likewise. * MODULES.html.sh (systems lacking POSIX:2008): Mention it. Signed-off-by: Eric Blake --- ChangeLog | 14 ++++++ MODULES.html.sh | 1 + doc/posix-functions/popen.texi | 11 ++++- lib/popen.c | 81 +++++++++++++++++++++++++++++++ lib/stdio.in.h | 14 ++++++ m4/popen.m4 | 34 +++++++++++++ m4/stdio_h.m4 | 4 +- modules/popen | 25 ++++++++++ modules/popen-tests | 12 +++++ modules/stdio | 2 + tests/test-popen.c | 106 +++++++++++++++++++++++++++++++++++++++++ 11 files changed, 302 insertions(+), 2 deletions(-) create mode 100644 lib/popen.c create mode 100644 m4/popen.m4 create mode 100644 modules/popen create mode 100644 modules/popen-tests create mode 100644 tests/test-popen.c diff --git a/ChangeLog b/ChangeLog index 5259d2d27..7a545c696 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,17 @@ +2009-08-19 Eric Blake + + popen: fix cygwin 1.5 bug when stdin closed + * doc/posix-functions/popen.texi (popen): Document cygwin bugs. + * modules/popen: New file. + * modules/popen-tests: Likewise. + * tests/test-popen.c: Likewise. + * m4/popen.m4: Likewise. + * lib/popen.c: Likewise. + * lib/stdio.in.h (popen): New declaration. + * m4/stdio_h.m4 (gl_STDIO_H_DEFAULTS): Add popen. + * modules/stdio (Makefile.am): Likewise. + * MODULES.html.sh (systems lacking POSIX:2008): Mention it. + 2009-08-17 Joel E. Denny maint.mk: give full control over update-copyright exclusions diff --git a/MODULES.html.sh b/MODULES.html.sh index 298b65565..cd23527c9 100755 --- a/MODULES.html.sh +++ b/MODULES.html.sh @@ -2290,6 +2290,7 @@ func_all_modules () func_module open func_module perror func_module poll + func_module popen func_module posix_spawn func_module posix_spawnattr_destroy func_module posix_spawnattr_getflags diff --git a/doc/posix-functions/popen.texi b/doc/posix-functions/popen.texi index fc4842e9d..414e573fa 100644 --- a/doc/posix-functions/popen.texi +++ b/doc/posix-functions/popen.texi @@ -4,12 +4,21 @@ POSIX specification: @url{http://www.opengroup.org/onlinepubs/9699919799/functions/popen.html} -Gnulib module: --- +Gnulib module: popen Portability problems fixed by Gnulib: @itemize +@item +Some platforms start the child with closed stdin or stdout if the +standard descriptors were closed in the parent: +Cygwin 1.5.x. @end itemize Portability problems not fixed by Gnulib: @itemize +@item +Some platforms mistakenly set the close-on-exec bit, then if it is +cleared by the application, the platform then leaks file descriptors +from earlier @code{popen} calls into subsequent @code{popen} children: +Cygwin 1.5.x. @end itemize diff --git a/lib/popen.c b/lib/popen.c new file mode 100644 index 000000000..a1f1d45bf --- /dev/null +++ b/lib/popen.c @@ -0,0 +1,81 @@ +/* Open a stream to a sub-process. + Copyright (C) 2009 Free Software Foundation, Inc. + + This program is free software: you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . */ + +/* Written by Eric Blake , 2009. */ + +#include + +/* Get the original definition of popen. It might be defined as a macro. */ +#define __need_FILE +# include +#undef __need_FILE + +static inline FILE * +orig_popen (const char *filename, const char *mode) +{ + return popen (filename, mode); +} + +/* Specification. */ +#include + +#include +#include +#include +#include + +FILE * +rpl_popen (const char *filename, const char *mode) +{ + /* The mingw popen works fine, and all other platforms have fcntl. + The bug of the child clobbering its own file descriptors if stdin + or stdout was closed in the parent can be worked around by + opening those two fds as close-on-exec to begin with. */ + /* Cygwin 1.5.x also has a bug where the popen fd is improperly + marked close-on-exec, and if the application undoes this, then + the fd leaks into subsequent popen calls. We could work around + this by maintaining a list of all fd's opened by popen, and + temporarily marking them cloexec around the real popen call, but + we would also have to override pclose, and the bookkeepping seems + extreme given that cygwin 1.7 no longer has the bug. */ + FILE *result; + int cloexec0 = fcntl (STDIN_FILENO, F_GETFD); + int cloexec1 = fcntl (STDOUT_FILENO, F_GETFD); + int saved_errno; + + if (cloexec0 < 0) + { + if (open ("/dev/null", O_RDONLY) != STDIN_FILENO + || fcntl (STDIN_FILENO, F_SETFD, + fcntl (STDIN_FILENO, F_GETFD) | FD_CLOEXEC) == -1) + abort (); + } + if (cloexec1 < 0) + { + if (open ("/dev/null", O_RDONLY) != STDOUT_FILENO + || fcntl (STDOUT_FILENO, F_SETFD, + fcntl (STDOUT_FILENO, F_GETFD) | FD_CLOEXEC) == -1) + abort (); + } + result = orig_popen (filename, mode); + saved_errno = errno; + if (cloexec0 < 0) + close (STDIN_FILENO); + if (cloexec1 < 0) + close (STDOUT_FILENO); + errno = saved_errno; + return result; +} diff --git a/lib/stdio.in.h b/lib/stdio.in.h index 0c33ed810..9dfb679d9 100644 --- a/lib/stdio.in.h +++ b/lib/stdio.in.h @@ -479,6 +479,20 @@ extern int puts (const char *string); extern size_t fwrite (const void *ptr, size_t s, size_t n, FILE *stream); #endif +#if @GNULIB_POPEN@ +# if @REPLACE_POPEN@ +# undef popen +# define popen rpl_popen +extern FILE *popen (const char *cmd, const char *mode); +# endif +#elif defined GNULIB_POSIXCHECK +# undef popen +# define popen(c,m) \ + (GL_LINK_WARNING ("popen is buggy on some platforms - " \ + "use gnulib module popen or pipe for more portability"), \ + popen (c, m)) +#endif + #if @GNULIB_GETDELIM@ # if !@HAVE_DECL_GETDELIM@ /* Read input, up to (and including) the next occurrence of DELIMITER, from diff --git a/m4/popen.m4 b/m4/popen.m4 new file mode 100644 index 000000000..f774a9efa --- /dev/null +++ b/m4/popen.m4 @@ -0,0 +1,34 @@ +# popen.m4 serial 1 +dnl Copyright (C) 2009 Free Software Foundation, Inc. +dnl This file is free software; the Free Software Foundation +dnl gives unlimited permission to copy and/or distribute it, +dnl with or without modifications, as long as this notice is preserved. + +AC_DEFUN([gl_FUNC_POPEN], +[ + AC_REQUIRE([gl_STDIO_H_DEFAULTS]) + AC_CACHE_CHECK([whether popen works with closed stdin], + [gl_cv_func_popen_works], + [ + AC_RUN_IFELSE([AC_LANG_PROGRAM([[#include +]], [FILE *child; + fclose (stdin); + fclose (stdout); + child = popen ("echo a", "r"); + return !(fgetc (child) == 'a' && pclose (child) == 0); +])], [gl_cv_func_popen_works=yes], [gl_cv_func_popen_works=no], + dnl For now, only cygwin 1.5 or older is known to be broken. + [gl_cv_func_popen_works='guessing yes']) + ]) + if test "$gl_cv_func_popen_works" = no; then + REPLACE_POPEN=1 + AC_LIBOBJ([popen]) + gl_PREREQ_POPEN + fi +]) + +# Prerequisites of lib/popen.c. +AC_DEFUN([gl_PREREQ_POPEN], +[ + AC_REQUIRE([AC_C_INLINE]) +]) diff --git a/m4/stdio_h.m4 b/m4/stdio_h.m4 index fcbe68f6b..8c9aa8f5b 100644 --- a/m4/stdio_h.m4 +++ b/m4/stdio_h.m4 @@ -1,4 +1,4 @@ -# stdio_h.m4 serial 16 +# stdio_h.m4 serial 17 dnl Copyright (C) 2007-2009 Free Software Foundation, Inc. dnl This file is free software; the Free Software Foundation dnl gives unlimited permission to copy and/or distribute it, @@ -73,6 +73,7 @@ AC_DEFUN([gl_STDIO_H_DEFAULTS], GNULIB_FPUTS=0; AC_SUBST([GNULIB_FPUTS]) GNULIB_PUTS=0; AC_SUBST([GNULIB_PUTS]) GNULIB_FWRITE=0; AC_SUBST([GNULIB_FWRITE]) + GNULIB_POPEN=0; AC_SUBST([GNULIB_POPEN]) GNULIB_GETDELIM=0; AC_SUBST([GNULIB_GETDELIM]) GNULIB_GETLINE=0; AC_SUBST([GNULIB_GETLINE]) GNULIB_PERROR=0; AC_SUBST([GNULIB_PERROR]) @@ -109,6 +110,7 @@ AC_DEFUN([gl_STDIO_H_DEFAULTS], REPLACE_FPURGE=0; AC_SUBST([REPLACE_FPURGE]) HAVE_DECL_FPURGE=1; AC_SUBST([HAVE_DECL_FPURGE]) REPLACE_FCLOSE=0; AC_SUBST([REPLACE_FCLOSE]) + REPLACE_POPEN=0; AC_SUBST([REPLACE_POPEN]) HAVE_DECL_GETDELIM=1; AC_SUBST([HAVE_DECL_GETDELIM]) HAVE_DECL_GETLINE=1; AC_SUBST([HAVE_DECL_GETLINE]) REPLACE_GETLINE=0; AC_SUBST([REPLACE_GETLINE]) diff --git a/modules/popen b/modules/popen new file mode 100644 index 000000000..75e278d8e --- /dev/null +++ b/modules/popen @@ -0,0 +1,25 @@ +Description: +popen() function: open a stream to a shell command. + +Files: +lib/popen.c +m4/popen.m4 + +Depends-on: +open +stdio + +configure.ac: +gl_FUNC_POPEN +gl_STDIO_MODULE_INDICATOR([popen]) + +Makefile.am: + +Include: + + +License: +LGPL + +Maintainer: +Eric Blake diff --git a/modules/popen-tests b/modules/popen-tests new file mode 100644 index 000000000..ee7760e3f --- /dev/null +++ b/modules/popen-tests @@ -0,0 +1,12 @@ +Files: +tests/test-popen.c + +Depends-on: +dup2 +sys_wait + +configure.ac: + +Makefile.am: +TESTS += test-popen +check_PROGRAMS += test-popen diff --git a/modules/stdio b/modules/stdio index cf886301b..98c0aeed7 100644 --- a/modules/stdio +++ b/modules/stdio @@ -58,6 +58,7 @@ stdio.h: stdio.in.h -e 's|@''GNULIB_FPUTS''@|$(GNULIB_FPUTS)|g' \ -e 's|@''GNULIB_PUTS''@|$(GNULIB_PUTS)|g' \ -e 's|@''GNULIB_FWRITE''@|$(GNULIB_FWRITE)|g' \ + -e 's|@''GNULIB_POPEN''@|$(GNULIB_POPEN)|g' \ -e 's|@''GNULIB_GETDELIM''@|$(GNULIB_GETDELIM)|g' \ -e 's|@''GNULIB_GETLINE''@|$(GNULIB_GETLINE)|g' \ -e 's|@''GNULIB_PERROR''@|$(GNULIB_PERROR)|g' \ @@ -91,6 +92,7 @@ stdio.h: stdio.in.h -e 's|@''REPLACE_FPURGE''@|$(REPLACE_FPURGE)|g' \ -e 's|@''HAVE_DECL_FPURGE''@|$(HAVE_DECL_FPURGE)|g' \ -e 's|@''REPLACE_FCLOSE''@|$(REPLACE_FCLOSE)|g' \ + -e 's|@''REPLACE_POPEN''@|$(REPLACE_POPEN)|g' \ -e 's|@''HAVE_DECL_GETDELIM''@|$(HAVE_DECL_GETDELIM)|g' \ -e 's|@''HAVE_DECL_GETLINE''@|$(HAVE_DECL_GETLINE)|g' \ -e 's|@''REPLACE_GETLINE''@|$(REPLACE_GETLINE)|g' \ diff --git a/tests/test-popen.c b/tests/test-popen.c new file mode 100644 index 000000000..3d689e9a9 --- /dev/null +++ b/tests/test-popen.c @@ -0,0 +1,106 @@ +/* Test of opening a subcommand stream. + Copyright (C) 2009 Free Software Foundation, Inc. + + This program is free software: you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see . */ + +/* Written by Eric Blake , 2009. */ + +#include + +/* Specification. */ +#include + +/* Helpers. */ +#include +#include +#include +#include + +#define ASSERT(expr) \ + do \ + { \ + if (!(expr)) \ + { \ + fprintf (stderr, "%s:%d: assertion failed\n", __FILE__, __LINE__); \ + fflush (stderr); \ + abort (); \ + } \ + } \ + while (0) + +int +main (int argc, char **argv) +{ + size_t len; + char *cmd; + int i; + + /* Children - use the pipe. */ + if (argc > 1) + { + if (*argv[1] == 'r') /* Parent is reading, so we write. */ + ASSERT (putchar ('c') == 'c'); + else /* Parent is writing, so we read. */ + ASSERT (getchar () == 'p'); + /* Test that parent can read non-zero status. */ + return 42; + } + + /* Parent - create read and write child, once under normal + circumstances and once with stdin and stdout closed. */ + len = strlen (argv[0]); + cmd = malloc (len + 3); /* Adding " r" and NUL. */ + ASSERT (cmd); + /* We count on argv[0] not containing any shell metacharacters. */ + strcpy (cmd, argv[0]); + cmd[len] = ' '; + cmd[len + 2] = '\0'; + for (i = 0; i < 2; i++) + { + FILE *child; + int status; + + if (i) + { + ASSERT (fclose (stdin) == 0); + ASSERT (fclose (stdout) == 0); + } + + cmd[len + 1] = 'r'; + ASSERT (child = popen (cmd, "r")); + ASSERT (fgetc (child) == 'c'); + status = pclose (child); + ASSERT (WIFEXITED (status)); + ASSERT (WEXITSTATUS (status) == 42); + if (i) + { + ASSERT (dup2 (STDIN_FILENO, STDIN_FILENO) == -1); + ASSERT (dup2 (STDOUT_FILENO, STDOUT_FILENO) == -1); + } + + cmd[len + 1] = 'w'; + ASSERT (child = popen (cmd, "w")); + ASSERT (fputc ('p', child) == 'p'); + status = pclose (child); + ASSERT (WIFEXITED (status)); + ASSERT (WEXITSTATUS (status) == 42); + if (i) + { + ASSERT (dup2 (STDIN_FILENO, STDIN_FILENO) == -1); + ASSERT (dup2 (STDOUT_FILENO, STDOUT_FILENO) == -1); + } + } + free (cmd); + return 0; +} -- 2.11.0