[Libguestfs] [nbdkit PATCH v2 14/17] sh: Use pipe2 with CLOEXEC when possible
Technically, as long as our thread model is SERIALIZE_ALL_REQUESTS, we
don't have to be very careful about atomic CLOEXEC on any of the pipes
we create for communication with the child. However, the next patch
wants to promote sh plugin to parallel when possible, which requires
the use of pipe2 to avoid fd leaks. Also, add an assert to ensure
that we avoid dup2(n, n) (which would fail to clear CLOEXEC) and
close(0-2) (otherwise, the logic for which fds to dup and close
becomes a lot more tedious).
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
plugins/sh/call.c | 34 +++++++++++++++++++++++++++++++++-
plugins/sh/sh.c | 5 ++++-
2 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/plugins/sh/call.c b/plugins/sh/call.c
index 9b8b48e2..bb80f642 100644
--- a/plugins/sh/call.c
+++ b/plugins/sh/call.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2018 Red Hat Inc.
+ * Copyright (C) 2018-2019 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -32,6 +32,8 @@
#include <config.h>
+#include <assert.h>
+#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <inttypes.h>
@@ -94,6 +96,27 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to stdin */
*rbuflen = *ebuflen = 0;
rbufalloc = ebufalloc = 0;
+#ifdef HAVE_PIPE2
+ if (pipe2 (in_fd, O_CLOEXEC) == -1) {
+ nbdkit_error ("%s: pipe2: %m", script);
+ goto error;
+ }
+ if (pipe2 (out_fd, O_CLOEXEC) == -1) {
+ nbdkit_error ("%s: pipe2: %m", script);
+ goto error;
+ }
+ if (pipe2 (err_fd, O_CLOEXEC) == -1) {
+ nbdkit_error ("%s: pipe2: %m", script);
+ goto error;
+ }
+#else
+ /* Without pipe2, we leave .fork_safe at 0 which forces
+ * NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS, this in turn ensures
+ * no other thread will be trying to fork, and thus we can skip
+ * worrying about CLOEXEC races. Adding full parallel support on
+ * this system would require a loop after fork to close all
+ * unexpected fds.
+ */
if (pipe (in_fd) == -1) {
nbdkit_error ("%s: pipe: %m", script);
goto error;
@@ -106,6 +129,15 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to stdin */
nbdkit_error ("%s: pipe: %m", script);
goto error;
}
+#endif
+
+ /* Ensure that stdin/out/err of the current process were not empty
+ * before we started creating pipes (otherwise, the close and dup2
+ * calls below become more complex to juggle fds around correctly).
+ */
+ assert (in_fd[0] > STDERR_FILENO && in_fd[1] > STDERR_FILENO &&
+ out_fd[0] > STDERR_FILENO && out_fd[1] > STDERR_FILENO
&&
+ err_fd[0] > STDERR_FILENO && err_fd[1] > STDERR_FILENO);
pid = fork ();
if (pid == -1) {
diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c
index 669008e9..79c9a4ac 100644
--- a/plugins/sh/sh.c
+++ b/plugins/sh/sh.c
@@ -263,6 +263,7 @@ sh_config_complete (void)
}
}
+/* See also .fork_safe and the comments in call.c:call3() */
#define THREAD_MODEL NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS
static int
@@ -960,7 +961,9 @@ static struct nbdkit_plugin plugin = {
.cache = sh_cache,
.errno_is_preserved = 1,
- .fork_safe = 0, /* use of fork() is not yet safe from fd leaks */
+#ifdef HAVE_PIPE2
+ .fork_safe = 1, /* use of fork() is only safe with atomic CLOEXEC */
+#endif
};
NBDKIT_REGISTER_PLUGIN(plugin)
--
2.20.1