popen: fix cygwin 1.5 bug when stdin closed
authorEric Blake <ebb9@byu.net>
Wed, 19 Aug 2009 13:15:54 +0000 (07:15 -0600)
committerEric Blake <ebb9@byu.net>
Wed, 19 Aug 2009 13:42:30 +0000 (07:42 -0600)
* 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 <ebb9@byu.net>
ChangeLog
MODULES.html.sh
doc/posix-functions/popen.texi
lib/popen.c [new file with mode: 0644]
lib/stdio.in.h
m4/popen.m4 [new file with mode: 0644]
m4/stdio_h.m4
modules/popen [new file with mode: 0644]
modules/popen-tests [new file with mode: 0644]
modules/stdio
tests/test-popen.c [new file with mode: 0644]

index 5259d2d..7a545c6 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,17 @@
+2009-08-19  Eric Blake  <ebb9@byu.net>
+
+       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  <jdenny@clemson.edu>
 
        maint.mk: give full control over update-copyright exclusions
index 298b655..cd23527 100755 (executable)
@@ -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
index fc4842e..414e573 100644 (file)
@@ -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 (file)
index 0000000..a1f1d45
--- /dev/null
@@ -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 <ebb9@byu.net>, 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;
+}
index 0c33ed8..9dfb679 100644 (file)
@@ -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 (file)
index 0000000..f774a9e
--- /dev/null
@@ -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])
+])
index fcbe68f..8c9aa8f 100644 (file)
@@ -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 (file)
index 0000000..75e278d
--- /dev/null
@@ -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 (file)
index 0000000..ee7760e
--- /dev/null
@@ -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
index cf88630..98c0aee 100644 (file)
@@ -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 (file)
index 0000000..3d689e9
--- /dev/null
@@ -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 <ebb9@byu.net>, 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;
+}