-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 According to Eric Blake on 8/17/2009 5:14 PM: > Even though Bruno's pipe module is generally a better > interface than popen, I discovered yet another popen > bug in cygwin today[1] (I fixed a cygwin popen bug in > August 2006, only to have it regress in December of > that year when cgf rewrote the implementation to be > faster). So even though m4 no longer uses popen, I'm > at least going to write a popen replacement module, > and wondering if I should also add popen_safer in the > process. > > [1] http://cygwin.com/ml/cygwin/2009-08/msg00582.html
I went ahead and implemented popen_safer. There is still a bug in cygwin 1.5's popen that gnulib _could_ fix, but which I didn't want to invest the time into fixing because it is already fixed in cygwin 1.7 (namely, that cygwin 1.5 marks the fd of popen cloexec, unlike other platforms, and if the application undoes this, then subsequent popens leak the fd into children). - -- Don't work too hard, make some time for fun as well! Eric Blake [email protected] -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (Cygwin) Comment: Public key at home.comcast.net/~ericblake/eblake.gpg Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAkqMMQEACgkQ84KuGfSFAYB+rACg0DBpq9iXDZ+U5mZbgChCJz2q VZYAoMMDqqfbsBlJnK93x7VhElTVqW2N =AdK0 -----END PGP SIGNATURE-----
>From a8f637e3c49275e6789c05d67c1fbd1751e5610a Mon Sep 17 00:00:00 2001 From: Eric Blake <[email protected]> Date: Wed, 19 Aug 2009 07:15:54 -0600 Subject: [PATCH 1/3] 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 <[email protected]> --- 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 5259d2d..7a545c6 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,17 @@ +2009-08-19 Eric Blake <[email protected]> + + 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 <[email protected]> maint.mk: give full control over update-copyright exclusions diff --git a/MODULES.html.sh b/MODULES.html.sh index 298b655..cd23527 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 fc4842e..414e573 100644 --- a/doc/posix-functions/popen.texi +++ b/doc/posix-functions/popen.texi @@ -4,12 +4,21 @@ popen 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 0000000..a1f1d45 --- /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 <http://www.gnu.org/licenses/>. */ + +/* Written by Eric Blake <[email protected]>, 2009. */ + +#include <config.h> + +/* Get the original definition of popen. It might be defined as a macro. */ +#define __need_FILE +# include <stdio.h> +#undef __need_FILE + +static inline FILE * +orig_popen (const char *filename, const char *mode) +{ + return popen (filename, mode); +} + +/* Specification. */ +#include <stdio.h> + +#include <errno.h> +#include <fcntl.h> +#include <stdlib.h> +#include <unistd.h> + +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 0c33ed8..9dfb679 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 0000000..f774a9e --- /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 <stdio.h> +]], [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 fcbe68f..8c9aa8f 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 0000000..75e278d --- /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: +<stdio.h> + +License: +LGPL + +Maintainer: +Eric Blake diff --git a/modules/popen-tests b/modules/popen-tests new file mode 100644 index 0000000..ee7760e --- /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 cf88630..98c0aee 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 0000000..3d689e9 --- /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 <http://www.gnu.org/licenses/>. */ + +/* Written by Eric Blake <[email protected]>, 2009. */ + +#include <config.h> + +/* Specification. */ +#include <stdio.h> + +/* Helpers. */ +#include <stdlib.h> +#include <string.h> +#include <sys/wait.h> +#include <unistd.h> + +#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; +} -- 1.6.3.3.334.g916e1 >From fb8c836ade0d4ab8050c2c381795ce098b701115 Mon Sep 17 00:00:00 2001 From: Eric Blake <[email protected]> Date: Wed, 19 Aug 2009 09:54:54 -0600 Subject: [PATCH 2/3] tests: test some of the *-safer modules * modules/fopen-safer (Depends-on): Add fopen. * modules/fcntl-safer (Depends-on): Add fcntl. * modules/stdlib-safer (Depends-on): Add stdlib. (configure.ac): Set indicator. * modules/unistd-safer (configure.ac): Likewise. * modules/tmpfile-safer (configure.ac): Likewise. (Depends-on): Add tmpfile. * lib/stdio--.h (fopen, tmpfile): Don't override unless module is active. * tests/test-fopen.c (includes): Test safer versions when they are in use. * tests/test-open.c (includes): Likewise. Signed-off-by: Eric Blake <[email protected]> --- ChangeLog | 14 ++++++++++++++ lib/stdio--.h | 14 +++++++++----- modules/fcntl-safer | 1 + modules/fopen-safer | 1 + modules/stdlib-safer | 2 ++ modules/tmpfile-safer | 2 ++ modules/unistd-safer | 1 + tests/test-fopen.c | 6 +++++- tests/test-open.c | 4 ++++ 9 files changed, 39 insertions(+), 6 deletions(-) diff --git a/ChangeLog b/ChangeLog index 7a545c6..b5d16e6 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,19 @@ 2009-08-19 Eric Blake <[email protected]> + tests: test some of the *-safer modules + * modules/fopen-safer (Depends-on): Add fopen. + * modules/fcntl-safer (Depends-on): Add fcntl. + * modules/stdlib-safer (Depends-on): Add stdlib. + (configure.ac): Set indicator. + * modules/unistd-safer (configure.ac): Likewise. + * modules/tmpfile-safer (configure.ac): Likewise. + (Depends-on): Add tmpfile. + * lib/stdio--.h (fopen, tmpfile): Don't override unless module is + active. + * tests/test-fopen.c (includes): Test safer versions when they are + in use. + * tests/test-open.c (includes): Likewise. + popen: fix cygwin 1.5 bug when stdin closed * doc/posix-functions/popen.texi (popen): Document cygwin bugs. * modules/popen: New file. diff --git a/lib/stdio--.h b/lib/stdio--.h index 39fca29..eafbdb1 100644 --- a/lib/stdio--.h +++ b/lib/stdio--.h @@ -1,6 +1,6 @@ /* Like stdio.h, but redefine some names to avoid glitches. - Copyright (C) 2005, 2006 Free Software Foundation, Inc. + Copyright (C) 2005, 2006, 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 @@ -20,8 +20,12 @@ #include <stdio.h> #include "stdio-safer.h" -#undef fopen -#define fopen fopen_safer +#if GNULIB_FOPEN_SAFER +# undef fopen +# define fopen fopen_safer +#endif -#undef tmpfile -#define tmpfile tmpfile_safer +#if GNULIB_TMPFILE_SAFER +# undef tmpfile +# define tmpfile tmpfile_safer +#endif diff --git a/modules/fcntl-safer b/modules/fcntl-safer index 253b76b..b9da466 100644 --- a/modules/fcntl-safer +++ b/modules/fcntl-safer @@ -10,6 +10,7 @@ m4/fcntl-safer.m4 m4/mode_t.m4 Depends-on: +fcntl open unistd-safer diff --git a/modules/fopen-safer b/modules/fopen-safer index 087e045..6a68f0c 100644 --- a/modules/fopen-safer +++ b/modules/fopen-safer @@ -8,6 +8,7 @@ lib/fopen-safer.c m4/stdio-safer.m4 Depends-on: +fopen unistd-safer configure.ac: diff --git a/modules/stdlib-safer b/modules/stdlib-safer index 8f7cb3f..ddeb865 100644 --- a/modules/stdlib-safer +++ b/modules/stdlib-safer @@ -9,10 +9,12 @@ m4/stdlib-safer.m4 Depends-on: mkstemp +stdlib unistd-safer configure.ac: gl_STDLIB_SAFER +gl_MODULE_INDICATOR([stdlib-safer]) Makefile.am: diff --git a/modules/tmpfile-safer b/modules/tmpfile-safer index c819063..de61544 100644 --- a/modules/tmpfile-safer +++ b/modules/tmpfile-safer @@ -9,10 +9,12 @@ m4/stdio-safer.m4 Depends-on: binary-io +tmpfile unistd-safer configure.ac: gl_TMPFILE_SAFER +gl_MODULE_INDICATOR([tmpfile-safer]) Makefile.am: diff --git a/modules/unistd-safer b/modules/unistd-safer index 86e23ab..e2182fd 100644 --- a/modules/unistd-safer +++ b/modules/unistd-safer @@ -14,6 +14,7 @@ unistd configure.ac: gl_UNISTD_SAFER +gl_MODULE_INDICATOR([unistd-safer]) Makefile.am: diff --git a/tests/test-fopen.c b/tests/test-fopen.c index 337a389..b9e3694 100644 --- a/tests/test-fopen.c +++ b/tests/test-fopen.c @@ -1,5 +1,5 @@ /* Test of opening a file stream. - Copyright (C) 2007-2008 Free Software Foundation, Inc. + Copyright (C) 2007-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 @@ -21,6 +21,10 @@ #include <stdio.h> #include <stdlib.h> +#if GNULIB_FOPEN_SAFER +# include "stdio--.h" +#endif + #define ASSERT(expr) \ do \ { \ diff --git a/tests/test-open.c b/tests/test-open.c index f7bb543..c9f3641 100644 --- a/tests/test-open.c +++ b/tests/test-open.c @@ -23,6 +23,10 @@ #include <stdio.h> #include <stdlib.h> +#if GNULIB_FCNTL_SAFER +# include "fcntl--.h" +#endif + #define ASSERT(expr) \ do \ { \ -- 1.6.3.3.334.g916e1 >From ccb9c6798066958dd14c1005a7d5ffc1cbd6edae Mon Sep 17 00:00:00 2001 From: Eric Blake <[email protected]> Date: Wed, 19 Aug 2009 10:02:19 -0600 Subject: [PATCH 3/3] popen-safer: prevent popen from clobbering std descriptors * modules/popen-safer: New file. * lib/popen-safer.c: Likewise. * m4/stdio-safer.m4 (gl_POPEN_SAFER): New macro. * lib/stdio--.h (popen): Provide override. * lib/stdio-safer.h (popen_safer): Provide declaration. * tests/test-popen.c (includes): Partially test this. * modules/popen-safer-tests: New file, for more tests. * tests/test-popen-safer.c: Likewise. * MODULES.html.sh (file stream based Input/Output): Mention it. Signed-off-by: Eric Blake <[email protected]> --- ChangeLog | 11 ++++++ MODULES.html.sh | 1 + lib/popen-safer.c | 85 ++++++++++++++++++++++++++++++++++++++++++++ lib/stdio--.h | 5 +++ lib/stdio-safer.h | 3 +- m4/stdio-safer.m4 | 9 ++++- modules/popen-safer | 29 +++++++++++++++ modules/popen-safer-tests | 12 ++++++ tests/test-popen-safer.c | 86 +++++++++++++++++++++++++++++++++++++++++++++ tests/test-popen.c | 4 ++ 10 files changed, 242 insertions(+), 3 deletions(-) create mode 100644 lib/popen-safer.c create mode 100644 modules/popen-safer create mode 100644 modules/popen-safer-tests create mode 100644 tests/test-popen-safer.c diff --git a/ChangeLog b/ChangeLog index b5d16e6..2161c2b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,16 @@ 2009-08-19 Eric Blake <[email protected]> + popen-safer: prevent popen from clobbering std descriptors + * modules/popen-safer: New file. + * lib/popen-safer.c: Likewise. + * m4/stdio-safer.m4 (gl_POPEN_SAFER): New macro. + * lib/stdio--.h (popen): Provide override. + * lib/stdio-safer.h (popen_safer): Provide declaration. + * tests/test-popen.c (includes): Partially test this. + * modules/popen-safer-tests: New file, for more tests. + * tests/test-popen-safer.c: Likewise. + * MODULES.html.sh (file stream based Input/Output): Mention it. + tests: test some of the *-safer modules * modules/fopen-safer (Depends-on): Add fopen. * modules/fcntl-safer (Depends-on): Add fcntl. diff --git a/MODULES.html.sh b/MODULES.html.sh index cd23527..9d52e42 100755 --- a/MODULES.html.sh +++ b/MODULES.html.sh @@ -2538,6 +2538,7 @@ func_all_modules () func_module fwriting func_module getpass func_module getpass-gnu + func_module popen-safer func_module stdlib-safer func_module tmpfile-safer func_end_table diff --git a/lib/popen-safer.c b/lib/popen-safer.c new file mode 100644 index 0000000..2182a2c --- /dev/null +++ b/lib/popen-safer.c @@ -0,0 +1,85 @@ +/* Invoke popen, but avoid some glitches. + + 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 <http://www.gnu.org/licenses/>. */ + +/* Written by Eric Blake. */ + +#include <config.h> + +#include "stdio-safer.h" + +#include <errno.h> +#include <fcntl.h> +#include <unistd.h> + +#include "cloexec.h" + +#if (defined _WIN32 || defined __WIN32__) && ! defined __CYGWIN__ +# define O_CLOEXEC O_NOINHERIT +#elif !defined O_CLOEXEC +# define O_CLOEXEC 0 +#endif + +/* Like open (name, flags | O_CLOEXEC), although not necessarily + atomic. FLAGS must not include O_CREAT. */ + +static int +open_noinherit (char const *name, int flags) +{ + int fd = open (name, flags | O_CLOEXEC); + if (0 <= fd && !O_CLOEXEC && set_cloexec_flag (fd, true) != 0) + { + int saved_errno = errno; + close (fd); + fd = -1; + errno = saved_errno; + } + return fd; +} + +/* Like popen, but do not return stdin, stdout, or stderr. */ + +FILE * +popen_safer (char const *cmd, char const *mode) +{ + /* Unfortunately, we cannot use the fopen_safer approach of using + fdopen (dup_safer (fileno (popen (cmd, mode)))), because stdio + libraries maintain hidden state tying the original fd to the pid + to wait on when using pclose (this hidden state is also used to + avoid fd leaks in subsequent popen calls). So, we instead + guarantee that all standard streams are open prior to the popen + call (even though this puts more pressure on open fds), so that + the original fd created by popen is safe. */ + FILE *fp; + int fd = open_noinherit ("/dev/null", O_RDONLY); + if (0 <= fd && fd <= STDERR_FILENO) + { + /* Maximum recursion depth is 3. */ + int saved_errno; + fp = popen_safer (cmd, mode); + saved_errno = errno; + close (fd); + errno = saved_errno; + } + else + { + /* Either all fd's are tied up, or fd is safe and the real popen + will reuse it. */ + close (fd); + fp = popen (cmd, mode); + } + return fp; +} diff --git a/lib/stdio--.h b/lib/stdio--.h index eafbdb1..ed90dda 100644 --- a/lib/stdio--.h +++ b/lib/stdio--.h @@ -29,3 +29,8 @@ # undef tmpfile # define tmpfile tmpfile_safer #endif + +#if GNULIB_POPEN_SAFER +# undef popen +# define popen popen_safer +#endif diff --git a/lib/stdio-safer.h b/lib/stdio-safer.h index c881d5a..dde22f1 100644 --- a/lib/stdio-safer.h +++ b/lib/stdio-safer.h @@ -1,6 +1,6 @@ /* Invoke stdio functions, but avoid some glitches. - Copyright (C) 2001, 2003, 2006 Free Software Foundation, Inc. + Copyright (C) 2001, 2003, 2006, 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 @@ -20,4 +20,5 @@ #include <stdio.h> FILE *fopen_safer (char const *, char const *); +FILE *popen_safer (char const *, char const *); FILE *tmpfile_safer (void); diff --git a/m4/stdio-safer.m4 b/m4/stdio-safer.m4 index 3d71452..9f9d7cc 100644 --- a/m4/stdio-safer.m4 +++ b/m4/stdio-safer.m4 @@ -1,5 +1,5 @@ -#serial 10 -dnl Copyright (C) 2002, 2005-2007 Free Software Foundation, Inc. +#serial 11 +dnl Copyright (C) 2002, 2005-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, dnl with or without modifications, as long as this notice is preserved. @@ -9,6 +9,11 @@ AC_DEFUN([gl_FOPEN_SAFER], AC_LIBOBJ([fopen-safer]) ]) +AC_DEFUN([gl_POPEN_SAFER], +[ + AC_LIBOBJ([popen-safer]) +]) + AC_DEFUN([gl_TMPFILE_SAFER], [ AC_LIBOBJ([tmpfile-safer]) diff --git a/modules/popen-safer b/modules/popen-safer new file mode 100644 index 0000000..76bf4ee --- /dev/null +++ b/modules/popen-safer @@ -0,0 +1,29 @@ +Description: +popen function that avoids clobbering std{in,out,err}. + +Files: +lib/stdio--.h +lib/stdio-safer.h +lib/popen-safer.c +m4/stdio-safer.m4 + +Depends-on: +cloexec +open +popen +unistd-safer + +configure.ac: +gl_POPEN_SAFER +gl_MODULE_INDICATOR([popen-safer]) + +Makefile.am: + +Include: +"stdio-safer.h" + +License: +GPL + +Maintainer: +Eric Blake diff --git a/modules/popen-safer-tests b/modules/popen-safer-tests new file mode 100644 index 0000000..3dd67f2 --- /dev/null +++ b/modules/popen-safer-tests @@ -0,0 +1,12 @@ +Files: +tests/test-popen-safer.c + +Depends-on: +dup2 +sys_wait + +configure.ac: + +Makefile.am: +TESTS += test-popen-safer +check_PROGRAMS += test-popen-safer diff --git a/tests/test-popen-safer.c b/tests/test-popen-safer.c new file mode 100644 index 0000000..281dae9 --- /dev/null +++ b/tests/test-popen-safer.c @@ -0,0 +1,86 @@ +/* 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 <http://www.gnu.org/licenses/>. */ + +/* Written by Eric Blake <[email protected]>, 2009. */ + +#include <config.h> + +/* Specification. */ +#include "stdio--.h" + +/* Helpers. */ +#include <stdlib.h> +#include <sys/wait.h> +#include <unistd.h> + +/* This test intentionally closes stderr. So, we arrange to have fd 10 + (outside the range of interesting fd's during the test) set up to + duplicate the original stderr. */ + +#define BACKUP_STDERR_FILENO 10 +static FILE *myerr; + +#define ASSERT(expr) \ + do \ + { \ + if (!(expr)) \ + { \ + fprintf (myerr, "%s:%d: assertion failed\n", __FILE__, __LINE__); \ + fflush (myerr); \ + abort (); \ + } \ + } \ + while (0) + +int +main (int argc, char **argv) +{ + FILE *fp; + int status; + + /* We close fd 2 later, so save it in fd 10. */ + if (dup2 (STDERR_FILENO, BACKUP_STDERR_FILENO) != BACKUP_STDERR_FILENO + || (myerr = fdopen (BACKUP_STDERR_FILENO, "w")) == NULL) + return 2; + + ASSERT (fp = popen ("exit 0", "r")); + ASSERT (STDERR_FILENO < fileno (fp)); + status = pclose (fp); + ASSERT (WIFEXITED (status)); + ASSERT (!WEXITSTATUS (status)); + + ASSERT (fclose (stdin) == 0); + ASSERT (fp = popen ("exit 0", "r")); + ASSERT (STDERR_FILENO < fileno (fp)); + status = pclose (fp); + ASSERT (WIFEXITED (status)); + ASSERT (!WEXITSTATUS (status)); + + ASSERT (fclose (stdout) == 0); + ASSERT (fp = popen ("exit 0", "r")); + ASSERT (STDERR_FILENO < fileno (fp)); + status = pclose (fp); + ASSERT (WIFEXITED (status)); + ASSERT (!WEXITSTATUS (status)); + + ASSERT (fclose (stderr) == 0); + ASSERT (fp = popen ("exit 0", "r")); + ASSERT (STDERR_FILENO < fileno (fp)); + status = pclose (fp); + ASSERT (WIFEXITED (status)); + ASSERT (!WEXITSTATUS (status)); + return 0; +} diff --git a/tests/test-popen.c b/tests/test-popen.c index 3d689e9..4e43bd7 100644 --- a/tests/test-popen.c +++ b/tests/test-popen.c @@ -27,6 +27,10 @@ #include <sys/wait.h> #include <unistd.h> +#if GNULIB_POPEN_SAFER +# include "stdio--.h" +#endif + #define ASSERT(expr) \ do \ { \ -- 1.6.3.3.334.g916e1
