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@redhat.com> --- 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