[Libguestfs] [nbdkit PATCH 2/2] server: Sanitize stdin/out before running plugin code
Richard W.M. Jones
rjones at redhat.com
Tue Apr 7 08:06:23 UTC 2020
On Sat, Apr 04, 2020 at 05:02:39PM -0500, Eric Blake wrote:
> 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;
> }
All the way down to here, we're all fine, ACK.
Can the part below be a separate test rather than overloading the
existing (layers) test? I wonder if it could be done fairly easily
using a test with sh or eval plugin.
It would also be nice to have tests of -s and --run and that their
stdin/stdout behaviour is still correct: for example that the --run
subscript is connected to the same stdin/stdout/stderr as the parent
process (although obviously that's a bunch more work).
> 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]);
Thanks,
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine. Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/
More information about the Libguestfs
mailing list