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 <[email protected]> --- 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 _______________________________________________ Libguestfs mailing list [email protected] https://www.redhat.com/mailman/listinfo/libguestfs
