test-pipe: make a bit more robust.

* tests/test-pipe.c (myerr): Allow error messages regardless of
what we do to stderr.
(test_pipe): Rearrange to avoid deadlock.
(child_main): Try a larger read, to ensure we avoided deadlock.
* lib/pipe.c (create_pipe) [_WIN32]: Fix comment.
* lib/pipe.h (create_pipe_bidi): Document potential for deadlock
if misused.

Signed-off-by: Eric Blake <ebb9@byu.net>

diff --git a/ChangeLog b/ChangeLog
index 9c788528823a3b7cf496c6673da737c325e52e3e..ea5f5fe9aa287d6ab7eb5ad20d72be7ca4f811d8 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2009-07-20  Eric Blake  <ebb9@byu.net>
+
+       test-pipe: make a bit more robust.
+       * tests/test-pipe.c (myerr): Allow error messages regardless of
+       what we do to stderr.
+       (test_pipe): Rearrange to avoid deadlock.
+       (child_main): Try a larger read, to ensure we avoided deadlock.
+       * lib/pipe.c (create_pipe) [_WIN32]: Fix comment.
+       * lib/pipe.h (create_pipe_bidi): Document potential for deadlock
+       if misused.
+
 2009-07-19  Jim Meyering  <meyering@redhat.com>
 
        fts: avoid false-positive cycle-detection

diff --git a/lib/pipe.c b/lib/pipe.c
index 5d91590f391b391c969ccf397694cff0b1810a17..daf7f7ffeabd190e92ff4cd9f34d384c321a11ea 100644 (file)
--- a/lib/pipe.c
+++ b/lib/pipe.c
@@ -185,9 +185,7 @@ create_pipe (const char *progname,
                      && close (stdoutfd) >= 0)))))
     /* The child process doesn't inherit ifd[0], ifd[1], ofd[0], ofd[1],
        but it inherits all open()ed or dup2()ed file handles (which is what
-       we want in the case of STD*_FILENO) and also orig_stdin,
-       orig_stdout, orig_stderr (which is not explicitly wanted but
-       harmless).  */
+       we want in the case of STD*_FILENO).  */
     /* Use spawnvpe and pass the environment explicitly.  This is needed if
        the program has modified the environment using putenv() or [un]setenv().
        On Windows, programs have two environments, one in the "environment

diff --git a/lib/pipe.h b/lib/pipe.h
index 03f2c32c03b73aa246bcaa4831ca3b01ad7dd414..082f6875f763117f674cb46eaff23c044005958f 100644 (file)
--- a/lib/pipe.h
+++ b/lib/pipe.h
@@ -109,6 +109,18 @@ extern pid_t create_pipe_in (const char *progname,
  *
  * Note: When writing to a child process, it is useful to ignore the SIGPIPE
  * signal and the EPIPE error code.
+ *
+ * Note: The parent process must be careful to avoid deadlock.
+ * 1) If you write more than PIPE_MAX bytes or, more generally, if you write
+ *    more bytes than the subprocess can handle at once, the subprocess
+ *    may write its data and wait on you to read it, but you are currently
+ *    busy writing.
+ * 2) When you don't know ahead of time how many bytes the subprocess
+ *    will produce, the usual technique of calling read (fd, buf, BUFSIZ)
+ *    with a fixed BUFSIZ will, on Linux 2.2.17 and on BSD systems, cause
+ *    the read() call to block until *all* of the buffer has been filled.
+ *    But the subprocess cannot produce more data until you gave it more
+ *    input.  But you are currently busy reading from it.
  */
 extern pid_t create_pipe_bidi (const char *progname,
                               const char *prog_path, char **prog_argv,

diff --git a/tests/test-pipe.c b/tests/test-pipe.c
index 023f4034a3029f75d889a48e584ffd42bef6283a..e28fae70e56b0ec862f598d11325a703c76ce20f 100644 (file)
--- a/tests/test-pipe.c
+++ b/tests/test-pipe.c
@@ -34,15 +34,19 @@
 #include <string.h>
 
 /* Depending on arguments, this test intentionally closes stderr or
-   starts life with stderr closed.  So, the error messages might not
-   always print, but at least the exit status will be reliable.  */
+   starts life with stderr closed.  So, we arrange to have fd 10
+   (outside the range of interesting fd's during the test) set up to
+   duplicate the original stderr.  */
+
+static FILE *myerr;
+
 #define ASSERT(expr) \
   do                                                                         \
     {                                                                        \
       if (!(expr))                                                           \
         {                                                                    \
-          fprintf (stderr, "%s:%d: assertion failed\n", __FILE__, __LINE__); \
-          fflush (stderr);                                                   \
+          fprintf (myerr, "%s:%d: assertion failed\n", __FILE__, __LINE__);  \
+          fflush (myerr);                                                    \
           abort ();                                                          \
         }                                                                    \
     }                                                                        \
@@ -52,7 +56,7 @@
 static int
 child_main (int argc, char *argv[])
 {
-  char buffer[1];
+  char buffer[2] = { 's', 't' };
   int fd;
 
   ASSERT (argc == 3);
@@ -61,7 +65,7 @@ child_main (int argc, char *argv[])
      fd 2 should be closed iff the argument is 1.  Check that no other file
      descriptors leaked.  */
 
-  ASSERT (read (STDIN_FILENO, buffer, 1) == 1);
+  ASSERT (read (STDIN_FILENO, buffer, 2) == 1);
 
   buffer[0]++;
   ASSERT (write (STDOUT_FILENO, buffer, 1) == 1);
@@ -141,6 +145,7 @@ test_pipe (const char *argv0, bool stderr_closed)
 
   /* Push child's input.  */
   ASSERT (write (fd[1], buffer, 1) == 1);
+  ASSERT (close (fd[1]) == 0);
 
   /* Get child's output.  */
   ASSERT (read (fd[0], buffer, 2) == 1);
@@ -148,7 +153,6 @@ test_pipe (const char *argv0, bool stderr_closed)
   /* Wait for child.  */
   ASSERT (wait_subprocess (pid, argv0, true, false, true, true, NULL) == 0);
   ASSERT (close (fd[0]) == 0);
-  ASSERT (close (fd[1]) == 0);
 
   /* Check the result.  */
   ASSERT (buffer[0] == 'b');
@@ -215,9 +219,22 @@ parent_main (int argc, char *argv[])
 int
 main (int argc, char *argv[])
 {
-  ASSERT (argc >= 2);
+  if (argc < 2)
+    {
+      fprintf (stderr, "%s: need arguments\n", argv[0]);
+      return 2;
+    }
   if (strcmp (argv[1], "child") == 0)
-    return child_main (argc, argv);
-  else
-    return parent_main (argc, argv);
+    {
+      /* fd 2 might be closed, but fd 10 is the original stderr.  */
+      myerr = fdopen (10, "w");
+      if (!myerr)
+       return 2;
+      return child_main (argc, argv);
+    }
+  /* We might close fd 2 later, so save it in fd 10.  */
+  if (dup2 (STDERR_FILENO, 10) != 10
+      || (myerr = fdopen (10, "w")) == NULL)
+    return 2;
+  return parent_main (argc, argv);
 }