Re: Help with create_pipe_bidi

[Eric Blake]

Eric Blake <ebb9 <at> byu.net> writes:

> but I think the simplest is just replacing the existing two calls 
> to pipe/fd_safer with the simpler one-line call to pipe_safer.  I'll commit a 
> patch later today, which also adds a pipe-test module using your test-pipe.c
> as a starting point (but avoiding the use of m4, which is not guaranteed to
> be available on all client machines of packages using the pipe module).

Like so:


From: Eric Blake <address@hidden>
Date: Fri, 17 Jul 2009 12:00:07 -0600
Subject: [PATCH] pipe: be robust in face of closed fds

* lib/pipe.c (create_pipe): Closed standard descriptors in parent
should cause child to misbehave.
* modules/pipe-tests: New module.
* tests/test-pipe.c: New file.
* tests/test-pipe.sh: New file.
Reported by Akim Demaille.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog          |   10 +++
 lib/pipe.c         |   10 ++--
 modules/pipe-tests |   14 +++++
 tests/test-pipe.c  |  160 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/test-pipe.sh |    8 +++
 5 files changed, 197 insertions(+), 5 deletions(-)
 create mode 100644 modules/pipe-tests
 create mode 100644 tests/test-pipe.c
 create mode 100755 tests/test-pipe.sh

diff --git a/ChangeLog b/ChangeLog
index f4ababb..81e6d83 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2009-07-17  Eric Blake  <address@hidden>
+
+       pipe: be robust in face of closed fds
+       * lib/pipe.c (create_pipe): Closed standard descriptors in parent
+       should cause child to misbehave.
+       * modules/pipe-tests: New module.
+       * tests/test-pipe.c: New file.
+       * tests/test-pipe.sh: New file.
+       Reported by Akim Demaille.
+
 2009-07-14  Bruno Haible  <address@hidden>

        * m4/wcwidth.m4 (gl_FUNC_WCWIDTH): Guess it works on glibc systems.
diff --git a/lib/pipe.c b/lib/pipe.c
index f8e7dee..64fa6a2 100644
--- a/lib/pipe.c
+++ b/lib/pipe.c
@@ -134,10 +134,12 @@ create_pipe (const char *progname,

   if (pipe_stdout)
     if (_pipe (ifd, 4096, O_BINARY | O_NOINHERIT) < 0
-       || (ifd[0] = fd_safer (ifd[0])) < 0)
+       || (ifd[0] = fd_safer (ifd[0])) < 0
+       || (ifd[1] = fd_safer (ifd[1])) < 0)
       error (EXIT_FAILURE, errno, _("cannot create pipe"));
   if (pipe_stdin)
     if (_pipe (ofd, 4096, O_BINARY | O_NOINHERIT) < 0
+       || (ofd[0] = fd_safer (ofd[0])) < 0
        || (ofd[1] = fd_safer (ofd[1])) < 0)
       error (EXIT_FAILURE, errno, _("cannot create pipe"));
 /* Data flow diagram:
@@ -258,12 +260,10 @@ create_pipe (const char *progname,
   pid_t child;

   if (pipe_stdout)
-    if (pipe (ifd) < 0
-       || (ifd[0] = fd_safer (ifd[0])) < 0)
+    if (pipe_safer (ifd) < 0)
       error (EXIT_FAILURE, errno, _("cannot create pipe"));
   if (pipe_stdin)
-    if (pipe (ofd) < 0
-       || (ofd[1] = fd_safer (ofd[1])) < 0)
+    if (pipe_safer (ofd) < 0)
       error (EXIT_FAILURE, errno, _("cannot create pipe"));
 /* Data flow diagram:
  *
diff --git a/modules/pipe-tests b/modules/pipe-tests
new file mode 100644
index 0000000..1371192
--- /dev/null
+++ b/modules/pipe-tests
@@ -0,0 +1,14 @@
+Files:
+tests/test-pipe.sh
+tests/test-pipe.c
+
+Depends-on:
+progname
+
+configure.ac:
+
+Makefile.am:
+TESTS += test-pipe.sh
+TESTS_ENVIRONMENT += EXEEXT='@EXEEXT@'
+check_PROGRAMS += test-pipe
+test_closein_LDADD = $(LDADD) @LIBINTL@
diff --git a/tests/test-pipe.c b/tests/test-pipe.c
new file mode 100644
index 0000000..3018186
--- /dev/null
+++ b/tests/test-pipe.c
@@ -0,0 +1,160 @@
+/* Test of create_pipe_bidi/wait_subprocess.
+   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, 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, write to the Free Software Foundation,
+   Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.  */
+
+#include <config.h>
+
+#include "pipe.h"
+#include "wait-process.h"
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#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.  */
+#define ASSERT(expr) \
+  do                                                                         \
+    {                                                                        \
+      if (!(expr))                                                           \
+        {                                                                    \
+          fprintf (stderr, "%s:%d: assertion failed\n", __FILE__, __LINE__); \
+          fflush (stderr);                                                   \
+          abort ();                                                          \
+        }                                                                    \
+    }                                                                        \
+  while (0)
+
+/* Create a bi-directional pipe to a test child, and validate that the
+   child program returns the expected output.  The child is the same
+   program as the parent ARGV0, but with different arguments.
+   STDERR_CLOSED is true if we have already closed fd 2.  */
+static void
+test_pipe (const char *argv0, bool stderr_closed)
+{
+  int fd[2];
+  const char *argv[3];
+  pid_t pid;
+  char buffer[2] = { 'a', 'a' };
+
+  /* Set up child.  */
+  argv[0] = argv0;
+  argv[1] = stderr_closed ? "9" : "8";
+  argv[2] = NULL;
+  pid = create_pipe_bidi(argv0, argv0, (char **) argv,
+                         false, true, true, fd);
+  ASSERT (0 <= pid);
+  ASSERT (STDERR_FILENO < fd[0]);
+  ASSERT (STDERR_FILENO < fd[1]);
+
+  /* Push child's input.  */
+  ASSERT (write (fd[1], buffer, 1) == 1);
+
+  /* Get child's output.  */
+  ASSERT (read (fd[0], buffer, 2) == 1);
+
+  /* Wait for child.  */
+  ASSERT (wait_subprocess (pid, argv0, true, false, false, true, NULL) == 0);
+  ASSERT (close (fd[0]) == 0);
+  ASSERT (close (fd[1]) == 0);
+
+  /* Check the result.  */
+  ASSERT (buffer[0] == 'b');
+  ASSERT (buffer[1] == 'a');
+}
+
+int
+main (int argc, const char *argv[])
+{
+  int i;
+  int test;
+  ASSERT (argc == 2);
+  test = atoi (argv[1]);
+  switch (test)
+    {
+      /* Driver cases.  Selectively close various standard fds, to
+         ensure the child process is not impacted by this.  */
+    case 0:
+      break;
+    case 1:
+      close (0);
+      break;
+    case 2:
+      close (1);
+      break;
+    case 3:
+      close (0);
+      close (1);
+      break;
+    case 4:
+      close (2);
+      break;
+    case 5:
+      close (0);
+      close (2);
+      break;
+    case 6:
+      close (1);
+      close (2);
+      break;
+    case 7:
+      close (0);
+      close (1);
+      close (2);
+      break;
+
+      /* Slave cases.  Read one byte from fd 0, and write its value
+         plus one to fd 1.  fd 2 should be closed iff the argument is
+         9.  Check that no other fd's leaked.  */
+    case 8:
+    case 9:
+      {
+        char buffer[1];
+        ASSERT (read (STDIN_FILENO, buffer, 1) == 1);
+        buffer[0]++;
+        ASSERT (write (STDOUT_FILENO, buffer, 1) == 1);
+        errno = 0;
+        i = fcntl (STDERR_FILENO, F_GETFL);
+        if (test == 8)
+          ASSERT (0 <= i);
+        else
+          {
+            ASSERT (i < 0);
+            ASSERT (errno == EBADF);
+          }
+        for (i = 3; i < 7; i++)
+          {
+            errno = 0;
+            ASSERT (close (i) == -1);
+            ASSERT (errno == EBADF);
+          }
+        return 0;
+      }
+    default:
+      ASSERT (0);
+    }
+  /* All remaining code is for the driver.  Plug any leaks inherited
+     from outside world before starting, so that child has a clean
+     slate (at least for the fds that we might be manipulating).  */
+  for (i = 3; i < 7; i++)
+    close (i);
+  test_pipe (argv[0], 3 < test);
+  return 0;
+}
diff --git a/tests/test-pipe.sh b/tests/test-pipe.sh
new file mode 100755
index 0000000..323c90f
--- /dev/null
+++ b/tests/test-pipe.sh
@@ -0,0 +1,8 @@
+#!/bin/sh
+
+st=0
+for i in 0 1 2 3 4 5 6 7 ; do
+  ./test-pipe${EXEEXT} $i \
+    || { echo test-pipe.sh: iteration $i failed >&2; st=1; }
+done
+exit $st
-- 
1.6.3.2