[Libguestfs] [nbdkit PATCH v2 07/17] build: Audit for use of pipe2

[Eric Blake]

Haiku unfortunately lacks pipe2, so we have to plan for a fallback at
each site that uses it.  This also makes a good time to audit all
existing users of pipe, to see if they should be using pipe2.  The
tests fork() but don't fail because of fd leaks; and the nbd plugin
doesn't fork() but was merely using pipe2 for convenience over
multiple fcntl calls.  However, the server's quit_fd definitely needs
to be marked CLOEXEC (it's easy to use the sh plugin to show that we
are currently leaking it to children), although doing so can occur
without worrying about atomicity since it is called before threads
begin.  Finally, it's also worth updating our set_cloexec helper
function to document that we still prefer atomicity where possible.

Signed-off-by: Eric Blake <eblake at redhat.com>
---
 configure.ac           |  1 +
 common/utils/utils.c   |  4 ++--
 plugins/nbd/nbd.c      | 31 +++++++++++++++++++++++++++++++
 server/quit.c          | 18 ++++++++++++++++++
 tests/test-layers.c    |  4 +++-
 tests/test-streaming.c |  5 +++--
 6 files changed, 58 insertions(+), 5 deletions(-)

diff --git a/configure.ac b/configure.ac
index c6bb1b10..cabec5c8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -196,6 +196,7 @@ AC_CHECK_FUNCS([\
 	fdatasync \
 	get_current_dir_name \
 	mkostemp \
+	pipe2 \
 	posix_fadvise])

 dnl Check whether printf("%m") works
diff --git a/common/utils/utils.c b/common/utils/utils.c
index 7b63b4d4..9ac3443b 100644
--- a/common/utils/utils.c
+++ b/common/utils/utils.c
@@ -140,13 +140,13 @@ exit_status_to_nbd_error (int status, const char *cmd)
  */
 int
 set_cloexec (int fd) {
-#if defined SOCK_CLOEXEC && defined HAVE_MKOSTEMP
+#if defined SOCK_CLOEXEC && defined HAVE_MKOSTEMP && defined HAVE_PIPE2
   nbdkit_error ("prefer creating fds with CLOEXEC atomically set");
   close (fd);
   errno = EBADF;
   return -1;
 #else
-# if defined SOCK_CLOEXEC || defined HAVE_MKOSTEMP
+# if defined SOCK_CLOEXEC || defined HAVE_MKOSTEMP || defined HAVE_PIPE2
 # error "Unexpected: your system has incomplete atomic CLOEXEC support"
 # endif
   int f;
diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c
index e8bc7798..95d910e7 100644
--- a/plugins/nbd/nbd.c
+++ b/plugins/nbd/nbd.c
@@ -431,11 +431,42 @@ nbdplug_open_handle (int readonly)
     nbdkit_error ("malloc: %m");
     return NULL;
   }
+#ifdef HAVE_PIPE2
   if (pipe2 (h->fds, O_NONBLOCK)) {
+    nbdkit_error ("pipe2: %m");
+    free (h);
+    return NULL;
+  }
+#else
+  /* This plugin doesn't fork, so we don't care about CLOEXEC. Our use
+   * of pipe2 is merely for convenience.
+   */
+  if (pipe (h->fds)) {
     nbdkit_error ("pipe: %m");
     free (h);
     return NULL;
   }
+  else {
+    int f;
+
+    f = fcntl (h->fds[0], F_GETFL);
+    if (f == -1 || fcntl (h->fds[0], F_SETFL, f | O_NONBLOCK) == -1) {
+      nbdkit_error ("fcntl: %m");
+      close (h->fds[0]);
+      close (h->fds[1]);
+      free (h);
+      return NULL;
+    }
+    f = fcntl (h->fds[1], F_GETFL);
+    if (f == -1 || fcntl (h->fds[1], F_SETFL, f | O_NONBLOCK) == -1) {
+      nbdkit_error ("fcntl: %m");
+      close (h->fds[0]);
+      close (h->fds[1]);
+      free (h);
+      return NULL;
+    }
+  }
+#endif

  retry:
   h->fd = -1;
diff --git a/server/quit.c b/server/quit.c
index c2ac11ef..ffd3fecb 100644
--- a/server/quit.c
+++ b/server/quit.c
@@ -32,6 +32,7 @@

 #include <config.h>

+#include <fcntl.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdarg.h>
@@ -39,6 +40,7 @@
 #include <unistd.h>

 #include "internal.h"
+#include "utils.h"

 /* Detection of request to exit via signal.  Most places in the code
  * can just poll quit at opportune moments, while sockets.c needs a
@@ -54,10 +56,26 @@ set_up_quit_pipe (void)
 {
   int fds[2];

+#ifdef HAVE_PIPE2
+  if (pipe2 (fds, O_CLOEXEC) < 0) {
+    perror ("pipe2");
+    exit (EXIT_FAILURE);
+  }
+#else
+  /* This is called early enough that no other thread will be
+   * fork()ing while we create this; but we must set CLOEXEC so that
+   * the fds don't leak into children.
+   */
   if (pipe (fds) < 0) {
     perror ("pipe");
     exit (EXIT_FAILURE);
   }
+  if (set_cloexec (fds[0]) == -1 ||
+      set_cloexec (fds[1]) == -1) {
+    perror ("fcntl");
+    exit (EXIT_FAILURE);
+  }
+#endif
   quit_fd = fds[0];
   write_quit_fd = fds[1];
 }
diff --git a/tests/test-layers.c b/tests/test-layers.c
index a820ba57..6617cd73 100644
--- a/tests/test-layers.c
+++ b/tests/test-layers.c
@@ -101,7 +101,9 @@ main (int argc, char *argv[])
   exit (77);
 #endif

-  /* Socket for communicating with nbdkit. */
+  /* Socket for communicating with nbdkit. The test doesn't care about
+   * fd leaks, so we don't bother with CLOEXEC.
+   */
   if (socketpair (AF_LOCAL, SOCK_STREAM, 0, sfd) == -1) {
     perror ("socketpair");
     exit (EXIT_FAILURE);
diff --git a/tests/test-streaming.c b/tests/test-streaming.c
index 4aaac9cd..e8c52ee8 100644
--- a/tests/test-streaming.c
+++ b/tests/test-streaming.c
@@ -70,8 +70,9 @@ main (int argc, char *argv[])
                          NULL) == -1)
     exit (EXIT_FAILURE);

-  /* Fork to run a second process which reads from streaming.fifo
-   * and checks that the content is correct.
+  /* Fork to run a second process which reads from streaming.fifo and
+   * checks that the content is correct.  The test doesn't care about
+   * fd leaks, so we don't bother with CLOEXEC.
    */
   if (pipe (pipefd) == -1) {
     perror ("pipe");
-- 
2.20.1