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 <ebb9@byu.net>

10 files changed, 242 insertions, 3 deletions

diff --git a/ChangeLog b/ChangeLog
index b5d16e6449..2161c2b0b4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,16 @@
 2009-08-19  Eric Blake  <ebb9@byu.net>
 
+	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 cd23527c95..9d52e42bfa 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 0000000000..2182a2c03c
--- /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 eafbdb127b..ed90dda5a5 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 c881d5a6a6..dde22f1045 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 3d71452ea9..9f9d7cc9a6 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 0000000000..76bf4ee552
--- /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 0000000000..3dd67f24be
--- /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 0000000000..281dae9a7a
--- /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 <ebb9@byu.net>, 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 3d689e9a92..4e43bd70ea 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									     \
     {									     \