bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: popen bugs, popen_safer


From: Eric Blake
Subject: Re: popen bugs, popen_safer
Date: Wed, 19 Aug 2009 11:06:09 -0600
User-agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.22) Gecko/20090605 Thunderbird/2.0.0.22 Mnenhy/0.7.6.666

-----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             address@hidden
-----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 <address@hidden>
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 <address@hidden>
---
 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  <address@hidden>
+
+       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  <address@hidden>

        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
address@hidden
+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
address@hidden
+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 <address@hidden>, 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 address@hidden@
 /* 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 <address@hidden>, 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 <address@hidden>
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 <address@hidden>
---
 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  <address@hidden>

+       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 <address@hidden>
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 <address@hidden>
---
 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  <address@hidden>

+       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 <address@hidden>, 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


reply via email to

[Prev in Thread] Current Thread [Next in Thread]