[Libguestfs] [nbdkit PATCH 2/2] server: Sanitize stdin/out before running plugin code
Eric Blake
eblake at redhat.com
Sat Apr 4 22:02:39 UTC 2020
As shown in the previous patch, plugins may choose to use stdin or
stdout during .config. But from .get_ready onwards, well-written
plugins shouldn't be needing any further use of stdin/out. We already
swapped stdin/out to /dev/null while daemonizing, but did not do do
during -f or --run, which leads to some surprising inconsistency when
trying to debug a plugin that works in the foreground but fails when
daemonized. What's more, while the task executed by --run should use
the original stdin/out (and we even have tests that would fail without
this), we don't want the plugin to interfere with whatever --run is
doing. So it's time to move the swap over to /dev/null into a central
location, right before .get_ready.
Signed-off-by: Eric Blake <eblake at redhat.com>
---
server/internal.h | 2 ++
server/background.c | 12 ++++--------
server/captive.c | 10 ++++++++--
server/connections.c | 12 ------------
server/main.c | 33 ++++++++++++++++++++++++++++++++-
tests/test-layers-plugin.c | 12 +++++++++++-
tests/test-layers.c | 4 +++-
7 files changed, 60 insertions(+), 25 deletions(-)
diff --git a/server/internal.h b/server/internal.h
index 67eb6a32..79e1906c 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -132,6 +132,8 @@ extern bool tls_verify_peer;
extern char *unixsocket;
extern const char *user, *group;
extern bool verbose;
+extern int orig_in;
+extern int orig_out;
/* Linked list of backends. Each backend struct is followed by either
* a filter or plugin struct. "top" points to the first one. They
diff --git a/server/background.c b/server/background.c
index 531ba470..8388d9c8 100644
--- a/server/background.c
+++ b/server/background.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2019 Red Hat Inc.
+ * Copyright (C) 2019-2020 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -70,13 +70,9 @@ fork_into_background (void)
chdir ("/");
#pragma GCC diagnostic pop
- /* Close stdin/stdout and redirect to /dev/null. */
- close (0);
- close (1);
- open ("/dev/null", O_RDONLY);
- open ("/dev/null", O_WRONLY);
-
- /* If not verbose, set stderr to the same as stdout as well. */
+ /* By this point, stdin/out have been redirected to /dev/null.
+ * If not verbose, set stderr to the same as stdout as well.
+ */
if (!verbose)
dup2 (1, 2);
diff --git a/server/captive.c b/server/captive.c
index 0a243d2b..1ff10192 100644
--- a/server/captive.c
+++ b/server/captive.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2019 Red Hat Inc.
+ * Copyright (C) 2019-2020 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -151,7 +151,13 @@ run_command (void)
}
if (pid > 0) { /* Parent process is the run command. */
- r = system (cmd);
+ /* Restore original stdin/out */
+ if (dup2 (orig_in, STDIN_FILENO) == -1 ||
+ dup2 (orig_out, STDOUT_FILENO) == -1) {
+ r = -1;
+ }
+ else
+ r = system (cmd);
if (r == -1) {
nbdkit_error ("failure to execute external command: %m");
r = EXIT_FAILURE;
diff --git a/server/connections.c b/server/connections.c
index c54c71c1..c7b55ca1 100644
--- a/server/connections.c
+++ b/server/connections.c
@@ -335,18 +335,6 @@ free_connection (struct connection *conn)
return;
conn->close ();
- if (listen_stdin) {
- int fd;
-
- /* Restore something to stdin/out so the rest of our code can
- * continue to assume that all new fds will be above stderr.
- * Swap directions to get EBADF on improper use of stdin/out.
- */
- fd = open ("/dev/null", O_WRONLY | O_CLOEXEC);
- assert (fd == 0);
- fd = open ("/dev/null", O_RDONLY | O_CLOEXEC);
- assert (fd == 1);
- }
/* Don't call the plugin again if quit has been set because the main
* thread will be in the process of unloading it. The plugin.unload
diff --git a/server/main.c b/server/main.c
index 748122fc..596dd306 100644
--- a/server/main.c
+++ b/server/main.c
@@ -101,6 +101,8 @@ const char *user, *group; /* -u & -g */
bool verbose; /* -v */
bool vsock; /* --vsock */
unsigned int socket_activation /* $LISTEN_FDS and $LISTEN_PID set */;
+int orig_in = -1; /* dup'd stdin during -s/--run */
+int orig_out = -1; /* dup'd stdout during -s/--run */
/* The linked list of zero or more filters, and one plugin. */
struct backend *top;
@@ -694,6 +696,35 @@ main (int argc, char *argv[])
top->config_complete (top);
+ /* Sanitize stdin/stdout to /dev/null, after saving the originals
+ * when needed. We are still single-threaded at this point, and
+ * already checked that stdin/out were open, so we don't have to
+ * worry about other threads accidentally grabbing our intended fds,
+ * or races on FD_CLOEXEC.
+ */
+ if (listen_stdin || run) {
+#ifndef F_DUPFD_CLOEXEC
+#define F_DUPFD_CLOEXEC F_DUPFD
+#endif
+ orig_in = fcntl (STDIN_FILENO, F_DUPFD_CLOEXEC, STDERR_FILENO + 1);
+ orig_out = fcntl (STDOUT_FILENO, F_DUPFD_CLOEXEC, STDERR_FILENO + 1);
+#if F_DUPFD == F_DUPFD_CLOEXEC
+ orig_in = set_cloexec (orig_in);
+ orig_out = set_cloexec (orig_out);
+#endif
+ if (orig_in == -1 || orig_out == -1) {
+ perror ("fcntl");
+ exit (EXIT_FAILURE);
+ }
+ }
+ close (STDIN_FILENO);
+ close (STDOUT_FILENO);
+ if (open ("/dev/null", O_RDONLY) != STDIN_FILENO ||
+ open ("/dev/null", O_WRONLY) != STDOUT_FILENO) {
+ perror ("open");
+ exit (EXIT_FAILURE);
+ }
+
/* Select the correct thread model based on config. */
lock_init_thread_model ();
@@ -918,7 +949,7 @@ start_serving (void)
change_user ();
write_pidfile ();
threadlocal_new_server_thread ();
- handle_single_connection (0, 1);
+ handle_single_connection (orig_in, orig_out);
return;
}
diff --git a/tests/test-layers-plugin.c b/tests/test-layers-plugin.c
index 8858bede..5a6b3020 100644
--- a/tests/test-layers-plugin.c
+++ b/tests/test-layers-plugin.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2018-2019 Red Hat Inc.
+ * Copyright (C) 2018-2020 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -36,6 +36,7 @@
#include <stdlib.h>
#include <stdint.h>
#include <string.h>
+#include <unistd.h>
#define NBDKIT_API_VERSION 2
#include <nbdkit-plugin.h>
@@ -75,7 +76,16 @@ test_layers_plugin_config_complete (void)
static int
test_layers_plugin_get_ready (void)
{
+ char c = 'X';
DEBUG_FUNCTION;
+
+ /* Test that stdin/stdout have been sanitized */
+ if (write (STDOUT_FILENO, &c, 1) != 1 ||
+ read (STDIN_FILENO, &c, 1) != 0) {
+ nbdkit_error ("unusual stdio behavior");
+ return -1;
+ }
+
return 0;
}
diff --git a/tests/test-layers.c b/tests/test-layers.c
index 33ae5a75..5bdce54e 100644
--- a/tests/test-layers.c
+++ b/tests/test-layers.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2018-2019 Red Hat Inc.
+ * Copyright (C) 2018-2020 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -125,6 +125,8 @@ main (int argc, char *argv[])
if (pid == 0) { /* Child. */
dup2 (sfd[1], 0);
dup2 (sfd[1], 1);
+ close (sfd[0]);
+ close (sfd[1]);
close (pfd[0]);
dup2 (pfd[1], 2);
close (pfd[1]);
--
2.26.0.rc2
More information about the Libguestfs
mailing list