If nanosleep fails (typically with EINTR because we handled a signal), we end up not sleeping long enough. In practice, this patch seldom makes a difference: directing signals at the nbdkit process tends to get serviced by the thread running poll(), and when that happens, the thread blocked in nanosleep() continues uninterrupted. As a result, nbdkit stalls in exiting because it is waiting for the filter plugin to quit servicing commands. But if you get lucky (or if you direct the kill to the specific thread stuck in nanosleep rather than the process as a whole), then this patch causes us to report early failure with EINTR mapped to EIO over the wire, at which point, we are back to practice: the only signals we handle trigger nbdkit to shutdown, so we may not even have a chance to get our unusual error reported over the wire. In theory, we could resume nanosleep where it left off by passing a non-null second argument, but that only matters if we have a signal handler whose action doesn't intend to request nbdkit to shut down soon. A later patch will add a utility function so that the server can do a better job, both for preventing short sleeps by resuming on EINTR (assuming the server ever adds support for a non-fatal signal, whether that be support for SIGHUP re-reading configuration or support for SIGUSR1/SIGINFO in providing server statistics so far), and for terminating sleeps with a better error than EINTR the moment we know a particular client is shutting down (whether because a signal is terminating all of nbdkit, or we got NBD_CMD_DISC or detected EOF on this particular connection). At that point, we'll only have to tweak the delay function to use that utility function in place of nanosleep. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- filters/rate/rate.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/filters/rate/rate.c b/filters/rate/rate.c index 30e093ee..f8dda0b0 100644 --- a/filters/rate/rate.c +++ b/filters/rate/rate.c @@ -228,8 +228,9 @@ maybe_adjust (const char *file, struct bucket *bucket, pthread_mutex_t *lock) old_rate, new_rate); } -static inline void -maybe_sleep (struct bucket *bucket, pthread_mutex_t *lock, uint32_t count) +static inline int +maybe_sleep (struct bucket *bucket, pthread_mutex_t *lock, uint32_t count, + int *err) { struct timespec ts; uint64_t bits; @@ -248,8 +249,13 @@ maybe_sleep (struct bucket *bucket, pthread_mutex_t *lock, uint32_t count) } if (bits > 0) - nanosleep (&ts, NULL); + if (nanosleep (&ts, NULL) == -1) { + nbdkit_error ("nanosleep: %m"); + *err = errno; + return -1; + } } + return 0; } /* Read data. */ @@ -261,9 +267,11 @@ rate_pread (struct nbdkit_next_ops *next_ops, void *nxdata, struct rate_handle *h = handle; maybe_adjust (rate_file, &read_bucket, &read_bucket_lock); - maybe_sleep (&read_bucket, &read_bucket_lock, count); + if (maybe_sleep (&read_bucket, &read_bucket_lock, count, err)) + return -1; maybe_adjust (connection_rate_file, &h->read_bucket, &h->read_bucket_lock); - maybe_sleep (&h->read_bucket, &h->read_bucket_lock, count); + if (maybe_sleep (&h->read_bucket, &h->read_bucket_lock, count, err)) + return -1; return next_ops->pread (nxdata, buf, count, offset, flags, err); } @@ -278,9 +286,11 @@ rate_pwrite (struct nbdkit_next_ops *next_ops, void *nxdata, struct rate_handle *h = handle; maybe_adjust (rate_file, &write_bucket, &write_bucket_lock); - maybe_sleep (&write_bucket, &write_bucket_lock, count); + if (maybe_sleep (&write_bucket, &write_bucket_lock, count, err)) + return -1; maybe_adjust (connection_rate_file, &h->write_bucket, &h->write_bucket_lock); - maybe_sleep (&h->write_bucket, &h->write_bucket_lock, count); + if (maybe_sleep (&h->write_bucket, &h->write_bucket_lock, count, err)) + return -1; return next_ops->pwrite (nxdata, buf, count, offset, flags, err); } -- 2.20.1

Testing interactive passwords is difficult (we'd have to spawn a pty), and left for another day. But reading passwords from a file or as a direct argument is easy. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- server/test-utils.c | 79 +++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 76 insertions(+), 3 deletions(-) diff --git a/server/test-utils.c b/server/test-utils.c index d6bdf465..264b1793 100644 --- a/server/test-utils.c +++ b/server/test-utils.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 @@ -35,6 +35,8 @@ #include <stdio.h> #include <inttypes.h> #include <stdbool.h> +#include <string.h> +#include <unistd.h> #include "internal.h" @@ -137,11 +139,82 @@ test_nbdkit_parse_size (void) return pass; } +static bool +test_nbdkit_read_password (void) +{ + bool pass = true; + char template[] = "+/tmp/nbdkit_testpw_XXXXXX"; + char *pw = template; + int fd; + + /* Test expected failure - no such file */ + error_flagged = false; + if (nbdkit_read_password ("+/nosuch", &pw) != -1) { + fprintf (stderr, "Failed to diagnose failed password file\n"); + pass = false; + } + else if (pw != NULL) { + fprintf (stderr, "Failed to set password to NULL on failure\n"); + pass = false; + } + else if (!error_flagged) { + fprintf (stderr, "Wrong error message handling\n"); + pass = false; + } + error_flagged = false; + + /* Test direct password */ + if (nbdkit_read_password ("abc", &pw) != 0) { + fprintf (stderr, "Failed to reuse direct password\n"); + pass = false; + } + else if (strcmp (pw, "abc") != 0) { + fprintf (stderr, "Wrong direct password, expected 'abc' got '%s'\n", pw); + pass = false; + } + free (pw); + pw = NULL; + + /* Test reading password from file */ + fd = mkstemp (&template[1]); + if (fd < 0) { + perror ("mkstemp"); + pass = false; + } + else if (write (fd, "abc\n", 4) != 4) { + fprintf (stderr, "Failed to write to file %s\n", &template[1]); + pass = false; + } + else if (nbdkit_read_password (template, &pw) != 0) { + fprintf (stderr, "Failed to read password from file %s\n", &template[1]); + pass = false; + } + else if (strcmp (pw, "abc") != 0) { + fprintf (stderr, "Wrong file password, expected 'abc' got '%s'\n", pw); + pass = false; + } + free (pw); + + if (fd >= 0) { + close (fd); + unlink (&template[1]); + } + + if (error_flagged) { + fprintf (stderr, "Wrong error message handling\n"); + pass = false; + } + + /* XXX Testing reading from stdin would require setting up a pty */ + return pass; +} + int main (int argc, char *argv[]) { bool pass = true; pass &= test_nbdkit_parse_size (); - /* XXX add tests for nbdkit_absolute_path, nbdkit_read_password */ - return pass ? 0 : 1; + pass &= test_nbdkit_read_password (); + /* XXX add tests for nbdkit_absolute_path */ + return pass ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.20.1

On 7/31/19 4:31 PM, Eric Blake wrote: I plan to also squash in this: (not in the original 25206df2 in 1.1.13, but instead added by copy-and-paste in 1.1.20): diff --git i/plugins/split/split.c w/plugins/split/split.c index 1b8e69a2..68682b29 100644 --- i/plugins/split/split.c +++ w/plugins/split/split.c @@ -45,10 +45,6 @@ #include <nbdkit-plugin.h> -#ifndef O_CLOEXEC -#define O_CLOEXEC 0 -#endif - /* The files. */ static char **filenames = NULL; static size_t nr_files = 0; -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org

The sh plugin makes it easy to test whether we leak fds into a plugin that fork()s as part of handling a client request: ./nbdkit -U - sh - --run 'qemu-io -r -f raw -c "r 0 1" $nbd' <<\EOF case $1 in get_size) echo 1m;; pread) ls -l /proc/$$/fd >/dev/tty dd iflag=count_bytes count=$3 if=/dev/zero || exit 1 ;; *) exit 2 ;; esac EOF Pre-patch, this demonstrates that we are leaking internal pipe and socket fds (the listing should only include fds 0, 1, 2, and whatever sh has open on the temporary script file). [Another patch will deal with leaks in several filters, also observable in this manner.] What's more, we need CLOEXEC to be set atomically for anything that can happen in parallel with plugin processing (in our case, during accept_connection); otherwise, there is a race window: client1 client2 fd1 = accept pread fd2 = accept fork fcntl(F_SETFD, FD_CLOEXEC) where the fd2 socket for the second client leaks into the fork for handling the first client's pread callback. Use of accept4() is not yet in POSIX, but has been documented [1] for several years now, and many implementations already support it (RHEL 6 and FreeBSD 10, for example). We can worry about a racy fallback for outliers if we hit compilation failure. It is less important whether quit.c use pipe2 or falls back to pipe/fcntl, as that code is never run in parallel with plugin code, but as long as we are assuming modern interfaces, fewer lines of code is nice. [1] http://austingroupbugs.net/view.php?id=411 For nbdkit_read_password, it is harder to test whether fopen("re") is supported on all platforms: we won't get a compilation failure. But if our unit tests start failing on platforms that have not caught up to POSIX (my first guess? Haiku, given that the previous patch shows that it also lacks mkostemp), then we can either fall back to fdopen(open(O_CLOEXEC)) or just simply document that nbdkit_read_password is only safe during .config when there are no other threads running in parallel to worry about a window where the fd can be leaked. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- server/connections.c | 1 + server/quit.c | 5 +++-- server/sockets.c | 7 ++++--- server/utils.c | 4 ++-- 4 files changed, 10 insertions(+), 7 deletions(-) diff --git a/server/connections.c b/server/connections.c index f56590c2..b1a15f23 100644 --- a/server/connections.c +++ b/server/connections.c @@ -39,6 +39,7 @@ #include <string.h> #include <unistd.h> #include <sys/socket.h> +#include <fcntl.h> #include "internal.h" diff --git a/server/quit.c b/server/quit.c index c2ac11ef..537c4e47 100644 --- a/server/quit.c +++ b/server/quit.c @@ -37,6 +37,7 @@ #include <stdarg.h> #include <string.h> #include <unistd.h> +#include <fcntl.h> #include "internal.h" @@ -54,8 +55,8 @@ set_up_quit_pipe (void) { int fds[2]; - if (pipe (fds) < 0) { - perror ("pipe"); + if (pipe2 (fds, O_CLOEXEC) < 0) { + perror ("pipe2"); exit (EXIT_FAILURE); } quit_fd = fds[0]; diff --git a/server/sockets.c b/server/sockets.c index b25405cb..587763cf 100644 --- a/server/sockets.c +++ b/server/sockets.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2013-2018 Red Hat Inc. + * Copyright (C) 2013-2019 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -286,8 +286,9 @@ accept_connection (int listen_sock) thread_data->instance_num = instance_num++; thread_data->addrlen = sizeof thread_data->addr; again: - thread_data->sock = accept (listen_sock, - &thread_data->addr, &thread_data->addrlen); + thread_data->sock = accept4 (listen_sock, + &thread_data->addr, &thread_data->addrlen, + SOCK_CLOEXEC); if (thread_data->sock == -1) { if (errno == EINTR || errno == EAGAIN) goto again; diff --git a/server/utils.c b/server/utils.c index 5a2d471a..eabef200 100644 --- a/server/utils.c +++ b/server/utils.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2013-2018 Red Hat Inc. + * Copyright (C) 2013-2019 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -237,7 +237,7 @@ nbdkit_read_password (const char *value, char **password) /* Read password from a file. */ else if (value[0] == '+') { - fp = fopen (&value[1], "r"); + fp = fopen (&value[1], "re"); if (fp == NULL) { nbdkit_error ("open %s: %m", &value[1]); return -1; -- 2.20.1

The rate filter is potentially opening fds in one thread while another thread is processing a fork() in the plugin. Although the file is not open for long, we MUST atomically use CLOEXEC to avoid fd leaks. This one is a bit harder to observe using only the sh plugin, because the window is small; you'll have better success at catching the leak by using gdb or recompiling code to insert strategic sleeps. Although fopen("re") is not required by POSIX yet, it has been documented for several years [1] and many platforms already support it. Failure to support it will not be seen at compile time, and it's hard to state whether our unit tests will catch it; thus, I decided to also add a comment to another use of fopen("re") that is much more likely to show if we hit a problematic system. [1] http://austingroupbugs.net/view.php?id=411 Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- filters/rate/rate.c | 2 +- server/utils.c | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/filters/rate/rate.c b/filters/rate/rate.c index f8dda0b0..9476a27f 100644 --- a/filters/rate/rate.c +++ b/filters/rate/rate.c @@ -204,7 +204,7 @@ maybe_adjust (const char *file, struct bucket *bucket, pthread_mutex_t *lock) if (!file) return; - fp = fopen (file, "r"); + fp = fopen (file, "re"); if (fp == NULL) return; /* this is not an error */ diff --git a/server/utils.c b/server/utils.c index eabef200..d7e202ec 100644 --- a/server/utils.c +++ b/server/utils.c @@ -236,6 +236,10 @@ nbdkit_read_password (const char *value, char **password) } /* Read password from a file. */ + /* Note: If the unit test for this fails because fopen("re") is + * unsupported by a given system, you must also fix the rate filter + * to use a similar workaround. + */ else if (value[0] == '+') { fp = fopen (&value[1], "re"); if (fp == NULL) { -- 2.20.1