4:31 p.m.
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>
---
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