From deb58d481fc8dc345e463002c7087a08a4f1d5f0 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 20 Jul 2009 06:41:01 -0600 Subject: [PATCH] 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 --- ChangeLog | 11 +++++++++++ lib/pipe.c | 4 +--- lib/pipe.h | 12 ++++++++++++ tests/test-pipe.c | 39 ++++++++++++++++++++++++++++----------- 4 files changed, 52 insertions(+), 14 deletions(-) diff --git a/ChangeLog b/ChangeLog index 9c7885288..ea5f5fe9a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,14 @@ +2009-07-20 Eric Blake + + 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 fts: avoid false-positive cycle-detection diff --git a/lib/pipe.c b/lib/pipe.c index 5d91590f3..daf7f7ffe 100644 --- 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 03f2c32c0..082f6875f 100644 --- 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 023f4034a..e28fae70e 100644 --- a/tests/test-pipe.c +++ b/tests/test-pipe.c @@ -34,15 +34,19 @@ #include /* 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); } -- 2.11.0