On Mon, Apr 13, 2020 at 07:28:59PM -0500, Eric Blake wrote: [...] This patch is fine and can be pushed if you want, but I've got some small comments. I wonder if we want to say "returns 1" rather than true, to give ourselves wiggle room in future in case we suddenly decided that we needed this to return an error indication? On the other hand, maybe errors can never happen in any conceivable situation. I think this could be clearer written the other way around: if (fd < STDERR_FILENO && !nbdkit_stdio_safe ()) { but then thinking about this more, why isn't it this? if (fd == STDIN_FILENO && !nbdkit_stdio_safe ()) { Anyway, these are minor points, ACK. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org

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. Note that if a plugin consumes some, but not all, input during .config, then we want --run to pick up with reading the rest of the input. For that to happen, we need to flush any buffered read-ahead data from stdin. POSIX says fflush(NULL) is supposed to be sufficient for the task, but in practice, glibc still has a bug that requires an additional fflush(stdin). Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- server/internal.h | 2 ++ server/background.c | 14 +++++--------- server/captive.c | 10 ++++++++-- server/connections.c | 12 ------------ server/main.c | 38 +++++++++++++++++++++++++++++++++++++- 5 files changed, 52 insertions(+), 24 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..72ab1ef6 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,15 +70,11 @@ 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); + dup2 (STDOUT_FILENO, STDERR_FILENO); forked_into_background = true; debug ("forked into background (new pid = %d)", getpid ()); 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 054e60b9..bbb416c3 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,40 @@ 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. POSIX says that 'fflush(NULL)' is + * supposed to reset the underlying offset of seekable stdin, but + * glibc is buggy and requires an explicit fflush(stdin) as + * well. https://sourceware.org/bugzilla/show_bug.cgi?id=12799 + */ + fflush (stdin); + fflush (NULL); + 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 +954,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; } -- 2.26.0

Enhance the testsuite to ensure we don't regress with recent changes to stdin/out handling. This adds: - test-single-sh.sh: prove that 'nbdkit -s sh script' is viable - test-stdio.sh: create plugin that checks stdin/out match /dev/null, then run it with -s, --run, -f Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- tests/Makefile.am | 23 ++++++ tests/test-single-sh.sh | 78 +++++++++++++++++ tests/test-stdio.sh | 95 +++++++++++++++++++++ tests/test-stdio-plugin.c | 170 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 366 insertions(+) create mode 100755 tests/test-single-sh.sh create mode 100755 tests/test-stdio.sh create mode 100644 tests/test-stdio-plugin.c diff --git a/tests/Makefile.am b/tests/Makefile.am index 2aa95890..63d47fc9 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -189,6 +189,7 @@ EXTRA_DIST = \ test-retry-reopen-fail.sh \ test-retry-zero-flags.sh \ test-ssh.sh \ + test-stdio.sh \ test-swap.sh \ test.tcl \ test-shebang-perl.sh \ @@ -200,6 +201,7 @@ EXTRA_DIST = \ test-sh-extents.sh \ test-single.sh \ test-single-from-file.sh \ + test-single-sh.sh \ test-split-extents.sh \ test-start.sh \ test-random-sock.sh \ @@ -263,6 +265,8 @@ TESTS += \ test-start.sh \ test-single.sh \ test-single-from-file.sh \ + test-single-sh.sh \ + test-stdio.sh \ test-captive.sh \ test-random-sock.sh \ test-tls.sh \ @@ -289,6 +293,25 @@ test_socket_activation_CPPFLAGS = \ $(NULL) test_socket_activation_CFLAGS = $(WARNINGS_CFLAGS) +# check_LTLIBRARIES won't build a shared library (see automake manual). +# So we have to do this and add a dependency. +noinst_LTLIBRARIES += \ + test-stdio-plugin.la \ + $(NULL) +test-stdio.sh: test-stdio-plugin.la + +test_stdio_plugin_la_SOURCES = \ + test-stdio-plugin.c \ + $(top_srcdir)/include/nbdkit-plugin.h \ + $(NULL) +test_stdio_plugin_la_CPPFLAGS = -I$(top_srcdir)/include +test_stdio_plugin_la_CFLAGS = $(WARNINGS_CFLAGS) +# For use of the -rpath option, see: +# https://lists.gnu.org/archive/html/libtool/2007-07/msg00067.html +test_stdio_plugin_la_LDFLAGS = \ + -module -avoid-version -shared $(SHARED_LDFLAGS) -rpath /nowhere \ + $(NULL) + # check_LTLIBRARIES won't build a shared library (see automake manual). # So we have to do this and add a dependency. noinst_LTLIBRARIES += \ diff --git a/tests/test-single-sh.sh b/tests/test-single-sh.sh new file mode 100755 index 00000000..a1586868 --- /dev/null +++ b/tests/test-single-sh.sh @@ -0,0 +1,78 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright (C) 2020 Red Hat Inc. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +source ./functions.sh +set -e +set -x + +requires nbdsh --version + +files="single-sh.script single-sh.log" +rm -f $files + +cleanup_fn rm -f $files + +fail=0 +# Inline scripts are incompatible with -s +if nbdkit -s sh - >/dev/null <<EOF +echo "oops: should not have run '$@'" >>single-sh.log +EOF +then + echo "$0: failed to diagnose -s vs. 'sh -'" + fail=1 +fi +if test -f single-sh.log; then + echo "$0: script unexpectedly ran" + cat single-sh.log + fail=1 +fi + +cat >single-sh.script <<\EOF +case $1 in + get_size) echo 1m ;; + pread) dd if=/dev/zero count=$3 iflag=count_bytes ;; + *) exit 2 ;; +esac +EOF +chmod +x single-sh.script + +# The sh plugin sets up pipes to handle stdin/out per each run of the +# script, but this is not incompatible with using -s for the client +nbdsh -c ' +h.connect_command (["nbdkit", "-s", "sh", "single-sh.script"]) +assert h.get_size() == 1024 * 1024 +buf1 = h.pread (512, 0) +buf2 = bytearray (512) +assert buf1 == buf2 +' + +exit $fail diff --git a/tests/test-stdio.sh b/tests/test-stdio.sh new file mode 100755 index 00000000..43913d66 --- /dev/null +++ b/tests/test-stdio.sh @@ -0,0 +1,95 @@ +#!/usr/bin/env bash +# nbdkit +# 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 +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +source ./functions.sh +set -xe + +requires nbdsh -c 'exit (not h.supports_uri ())' + +plugin=.libs/test-stdio-plugin.so +requires test -f $plugin + +sock1=`mktemp -u` +sock2=`mktemp -u` +files="test-stdio.in test-stdio.out test-stdio.err + test-stdio.pid1 test-stdio.pid2 $sock1 $sock2" +rm -f $files +cleanup_fn rm -f $files + +# Using a seekable file lets us prove that if the plugin consumes less +# than the full input, the next process sees the rest +cat >test-stdio.in <<EOF +string1 +string2 +EOF + +# .dump_plugin using stdout is normal; using stdin is odd, but viable +{ nbdkit --dump-plugin $plugin; printf 'rest='; cat +} < test-stdio.in > test-stdio.out +grep "input=string1" test-stdio.out +grep "rest=string2" test-stdio.out + +# Test with --run. +nbdkit -v $plugin one=1 --run 'printf cmd=; cat' \ + < test-stdio.in > test-stdio.out +cat test-stdio.out +grep "one=string1" test-stdio.out +grep "cmd=string2" test-stdio.out + +# Test with -f; we have to repeat body of start_nbdkit ourselves +echo "string" | nbdkit -P test-stdio.pid1 -v --filter=exitlast \ + -f -U $sock1 $plugin two=2 | tee test-stdio.out & pid=$! +for i in {1..60}; do + if test -s test-stdio.pid1; then + break + fi + sleep 1 +done +if ! test -s test-stdio.pid1; then + echo "$0: PID file $pidfile was not created" + exit 1 +fi +nbdsh -u "nbd+unix:///?socket=$sock1" -c 'buf = h.pread (512, 0)' +wait $pid +grep "two=string" test-stdio.out + +# Test as daemon +echo "string" | start_nbdkit -P test-stdio.pid2 --filter=exitlast \ + -U $sock2 $plugin three=3 | tee test-stdio.out +nbdsh -u "nbd+unix:///?socket=$sock2" -c 'buf = h.pread (512, 0)' +grep "three=string" test-stdio.out + +# Test with -s; here, the plugin produces no output. +nbdsh -c ' +h.connect_command (["nbdkit", "-v", "-s", "'$plugin'", "four=4"]) +buf = h.pread (512, 0) +' diff --git a/tests/test-stdio-plugin.c b/tests/test-stdio-plugin.c new file mode 100644 index 00000000..6af1ba90 --- /dev/null +++ b/tests/test-stdio-plugin.c @@ -0,0 +1,170 @@ +/* nbdkit + * Copyright (C) 2013-2020 Red Hat Inc. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * * Neither the name of Red Hat nor the names of its contributors may be + * used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, + * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A + * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF + * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT + * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +#include <config.h> + +#include <assert.h> +#include <fcntl.h> +#include <stdbool.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <sys/stat.h> +#include <unistd.h> + +#define NBDKIT_API_VERSION 2 + +#include <nbdkit-plugin.h> + +static const char *msg = "input"; + +/* Check whether stdin/out match /dev/null */ +static bool +stdio_check (void) +{ + static int dn = -1; + struct stat st1, st2; + + if (dn == -1) { + dn = open ("/dev/null", O_RDONLY); + assert (dn > STDERR_FILENO); + } + if (fstat (dn, &st1) == -1) + assert (false); + + if (fstat (STDIN_FILENO, &st2) == -1) + assert (false); + if (st1.st_dev != st2.st_dev || st1.st_ino != st2.st_ino) + return false; + + if (fstat (STDOUT_FILENO, &st2) == -1) + assert (false); + if (st1.st_dev != st2.st_dev || st1.st_ino != st2.st_ino) + return false; + + return true; +} + +static void +stdio_dump_plugin (void) +{ + char *buf = NULL; + size_t len = 0; + bool check = stdio_check (); + + assert (check == false); + + /* Reading from stdin during .dump_plugin is unusual, but not forbidden */ + if (getline (&buf, &len, stdin) == -1) + assert (false); + /* The point of .dump_plugin is to extend details sent to stdout */ + printf ("%s=%s\n", msg, buf); + free (buf); +} + +static int +stdio_config (const char *key, const char *value) +{ + bool check = stdio_check (); + assert (check == false); + msg = key; + return 0; +} + +static int +stdio_config_complete (void) +{ + bool check = stdio_check (); + assert (check == false); + if (nbdkit_stdio_safe ()) { + char *buf = NULL; + size_t len = 0; + + /* Reading from stdin during .config_complete is safe except under -s */ + if (getline (&buf, &len, stdin) == -1) + assert (false); + /* Output during .config_complete is unusual, but not forbidden */ + printf ("%s=%s\n", msg, buf); + free (buf); + } + return 0; +} + +static int +stdio_get_ready (void) +{ + bool check = stdio_check (); + assert (check == true); + return 0; +} + +static void * +stdio_open (int readonly) +{ + bool check = stdio_check (); + assert (check == true); + return NBDKIT_HANDLE_NOT_NEEDED; +} + +static int64_t +stdio_get_size (void *handle) +{ + bool check = stdio_check (); + assert (check == true); + return 1024*1024; +} + +#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL + +static int +stdio_pread (void *handle, void *buf, uint32_t count, uint64_t offset, + uint32_t flags) +{ + bool check = stdio_check (); + assert (check == true); + memset (buf, 0, count); + return 0; +} + +static struct nbdkit_plugin plugin = { + .name = "stdio", + .version = PACKAGE_VERSION, + .dump_plugin = stdio_dump_plugin, + .config = stdio_config, + .config_complete = stdio_config_complete, + .get_ready = stdio_get_ready, + .open = stdio_open, + .get_size = stdio_get_size, + .pread = stdio_pread, +}; + +NBDKIT_REGISTER_PLUGIN(plugin) -- 2.26.0