Eric Blake
2023-Feb-24 22:59 UTC
[Libguestfs] [nbdkit PATCH v2] server: Don't assert on send if client hangs up early
libnbd's copy/copy-nbd-error.sh was triggering an assertion failure in
nbdkit (this is the command that triggered the failure, at least with
nbdcopy 1.14.2; although future nbdcopy may be fixed independently to
more gracefully exit):
$ nbdcopy -- [ nbdkit --exit-with-parent -v --filter=error pattern 5M
error-pread-rate=0.5 ] null:
...
nbdkit: pattern.2: debug: error-inject: pread count=262144 offset=4718592
nbdkit: pattern.2: debug: pattern: pread count=262144 offset=4718592
nbdkit: pattern.1: debug: error-inject: pread count=262144 offset=4456448
nbdkit: pattern.1: error: injecting EIO error into pread
nbdkit: pattern.1: debug: sending error reply: Input/output error
nbdkit: pattern.3: debug: error-inject: pread count=262144 offset=4980736
nbdkit: pattern.3: debug: pattern: pread count=262144 offset=4980736
nbdkit: pattern.4: error: write data: NBD_CMD_READ: Broken pipe
nbdkit: pattern.4: debug: exiting worker thread pattern.4
nbdkit: connections.c:402: raw_send_socket: Assertion `sock >= 0' failed.
When there are multiple queued requests, and the client hangs up
abruptly as soon as the first error response is seen (rather than
waiting until all other queued responses are flushed then sending
NBD_CMD_DISC), another thread that has a queued response ready to send
sees EPIPE (the client is no longer reading) and closes the socket,
then the third thread triggers the assertion failure because it was
not expecting to attempt a send to a closed socket. Thus, my claim
that sock would always be non-negative after collecting data from the
plugin was wrong. The bug can only happen for a server with
NBDKIT_THREAD_MODEL_PARALLEL, as that is the only time a single client
can have more than one worker thread; and even with parallel service,
it is possible to avoid the bug by using 'nbdkit --threads=1' (at the
expense of performance, because one client can no longer benefit from
interleaved requests).
For the nbdcopy example, --exit-with-parent means it's just an
annoyance (nbdcopy has already exited, and no further client will be
connecting); but for a longer-running nbdkit server accepting multiple
clients (particularly likely when advertising MULTI-CONN, although
that is not a prerequisite), it means any one client can easily
trigger the SIGABRT by intentionally queueing multiple NBD_CMD_READ
then disconnecting early, and thereby kill nbdkit and starve other
clients. Note that SIGABRT is the most likely outcome; however, there
is also a small window for something else: if client 1's first thread
triggers conn->close(SHUT_WR) while holding read_lock, there is a
window betwen shutdown(conn->sockout) and conn->sockout=-1 where the
client's second thread holding write_lock can copy a stale
non-negative value of sock=conn->sockout. If during this window a 2nd
client connects, an fd allocated as part of the second client can end
up with the same value as the stale 'sock' in the first client, where
we send data to a completely unrelated socket rather than triggering
the assertion failure.
Whether this bug rises to the level of CVE depends on whether you
consider one client being able to starve others a privilege escalation
(if you are not using TLS, there are other ways for a bad client to
starve peers; if you are using TLS, then the starved client has the
same credentials as the client that caused the SIGABRT or data
corruption so there is no privilege boundary escalation).
Partially reverts 26fd5e42; protocol_recv_request_send_reply() must
once again have a return status, because it is essential that when the
client has multiple worker threads, conn->sockout not be modified
except under the write_lock, and altering it is easier at a central
point where we do not risk deadlock with read_lock or status_lock.
Add some testsuite coverage that does not depend on current nbdcopy's
abrubt disconnect behavior. The TLS case happens to pass even without
this patch, but the plaintext version demonstrates the same core dump
that triggered my efforts on this patch.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2173054
Fixes: daef505e ("server: Give client EOF when we are done writing",
v1.32.4)
---
tests/Makefile.am | 6 +++
server/internal.h | 19 +++++---
server/connections.c | 27 ++++++++---
server/protocol.c | 85 +++++++++++++++-------------------
server/public.c | 7 ++-
server/test-public.c | 4 +-
tests/test-client-death-tls.sh | 81 ++++++++++++++++++++++++++++++++
tests/test-client-death.sh | 60 ++++++++++++++++++++++++
8 files changed, 224 insertions(+), 65 deletions(-)
create mode 100755 tests/test-client-death-tls.sh
create mode 100755 tests/test-client-death.sh
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 15832138..d4717383 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -256,6 +256,8 @@ TESTS += \
test-swap.sh \
test-disconnect.sh \
test-disconnect-tls.sh \
+ test-client-death.sh \
+ test-client-death-tls.sh \
test-shutdown.sh \
test-nbdkit-backend-debug.sh \
test-read-password.sh \
@@ -299,6 +301,8 @@ EXTRA_DIST += \
test-read-password-plugin.c \
test-disconnect.sh \
test-disconnect-tls.sh \
+ test-client-death.sh \
+ test-client-death-tls.sh \
test-shutdown.sh \
test-single-from-file.sh \
test-single-sh.sh \
@@ -406,6 +410,8 @@ test_disconnect_plugin_la_LDFLAGS = \
$(NULL)
test_disconnect_plugin_la_LIBADD = $(IMPORT_LIBRARY_ON_WINDOWS)
+test-client-death-tls.sh: keys.psk
+
# 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/server/internal.h b/server/internal.h
index 77fdc21d..af5d8677 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2013-2022 Red Hat Inc.
+ * Copyright (C) 2013-2023 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -244,10 +244,13 @@ typedef enum {
} conn_status;
struct connection {
- pthread_mutex_t request_lock;
- pthread_mutex_t read_lock;
- pthread_mutex_t write_lock;
- pthread_mutex_t status_lock;
+ /* Listed in precedence order: do not grab earlier locks in this list
+ * while holding a later lock.
+ */
+ pthread_mutex_t request_lock; /* Forces serialization of requests */
+ pthread_mutex_t read_lock; /* Read entire client payload off wire */
+ pthread_mutex_t write_lock; /* Protect sockout, write response to wire */
+ pthread_mutex_t status_lock; /* Track current status of client */
conn_status status;
int status_pipe[2]; /* track status changes via poll when nworkers > 1 */
@@ -269,14 +272,16 @@ struct connection {
const char *exportname;
int sockin, sockout;
+ /* If nworkers > 1, only call this while read_lock is held */
connection_recv_function recv;
+ /* If nworkers > 1, only call these while write_lock is held */
connection_send_function send;
connection_close_function close;
};
extern void handle_single_connection (int sockin, int sockout);
extern conn_status connection_get_status (void);
-extern void connection_set_status (conn_status value);
+extern bool connection_set_status (conn_status value);
/* protocol-handshake.c */
extern int protocol_handshake (void);
@@ -291,7 +296,7 @@ extern int protocol_handshake_oldstyle (void);
extern int protocol_handshake_newstyle (void);
/* protocol.c */
-extern void protocol_recv_request_send_reply (void);
+extern bool protocol_recv_request_send_reply (void);
/* The context ID of base:allocation. As far as I can tell it doesn't
* matter what this is as long as nbdkit always returns the same
diff --git a/server/connections.c b/server/connections.c
index 4d776f2a..6d9f6a96 100644
--- a/server/connections.c
+++ b/server/connections.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2013-2022 Red Hat Inc.
+ * Copyright (C) 2013-2023 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -80,11 +80,14 @@ connection_get_status (void)
return r;
}
-/* Update the status if the new value is lower than the existing value. */
-void
+/* Update the status if the new value is lower than the existing value.
+ * Return true if the caller should shutdown.
+ */
+bool
connection_set_status (conn_status value)
{
GET_CONN;
+ bool ret = false;
if (conn->nworkers &&
pthread_mutex_lock (&conn->status_lock))
@@ -99,12 +102,13 @@ connection_set_status (conn_status value)
debug ("failed to notify pipe-to-self: %m");
}
if (conn->status >= STATUS_CLIENT_DONE && value <
STATUS_CLIENT_DONE)
- conn->close (SHUT_WR);
+ ret = true;
conn->status = value;
}
if (conn->nworkers &&
pthread_mutex_unlock (&conn->status_lock))
abort ();
+ return ret;
}
struct worker_data {
@@ -126,7 +130,10 @@ connection_worker (void *data)
free (worker);
while (!quit && connection_get_status () > STATUS_CLIENT_DONE)
- protocol_recv_request_send_reply ();
+ if (protocol_recv_request_send_reply ()) {
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&conn->write_lock);
+ conn->close (SHUT_WR);
+ }
debug ("exiting worker thread %s", threadlocal_get_name ());
free (name);
return NULL;
@@ -178,7 +185,8 @@ handle_single_connection (int sockin, int sockout)
/* No need for a separate thread. */
debug ("handshake complete, processing requests serially");
while (!quit && connection_get_status () > STATUS_CLIENT_DONE)
- protocol_recv_request_send_reply ();
+ if (protocol_recv_request_send_reply ())
+ conn->close (SHUT_WR);
}
else {
/* Create thread pool to process requests. */
@@ -400,7 +408,10 @@ raw_send_socket (const void *vbuf, size_t len, int flags)
ssize_t r;
int f = 0;
- assert (sock >= 0);
+ if (sock < 0) {
+ errno = EBADF;
+ return -1;
+ }
#ifdef MSG_MORE
if (flags & SEND_MORE)
f |= MSG_MORE;
@@ -492,6 +503,8 @@ raw_recv (void *vbuf, size_t len)
/* There's no place in the NBD protocol to send back errors from
* close, so this function ignores errors.
+ *
+ * If @how == SHUT_WR and conn->nworkers > 1, the caller holds
write_lock.
*/
static void
raw_close (int how)
diff --git a/server/protocol.c b/server/protocol.c
index cc1e4ed8..58f865ef 100644
--- a/server/protocol.c
+++ b/server/protocol.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2013-2022 Red Hat Inc.
+ * Copyright (C) 2013-2023 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -362,7 +362,7 @@ nbd_errno (int error, uint16_t flags)
}
}
-static void
+static bool
send_simple_reply (uint64_t handle, uint16_t cmd, uint16_t flags,
const char *buf, uint32_t count,
uint32_t error)
@@ -380,8 +380,7 @@ send_simple_reply (uint64_t handle, uint16_t cmd, uint16_t
flags,
r = conn->send (&reply, sizeof reply, f);
if (r == -1) {
nbdkit_error ("write reply: %s: %m", name_of_nbd_cmd (cmd));
- connection_set_status (STATUS_DEAD);
- return;
+ return connection_set_status (STATUS_DEAD);
}
/* Send the read data buffer. */
@@ -389,12 +388,13 @@ send_simple_reply (uint64_t handle, uint16_t cmd, uint16_t
flags,
r = conn->send (buf, count, 0);
if (r == -1) {
nbdkit_error ("write data: %s: %m", name_of_nbd_cmd (cmd));
- connection_set_status (STATUS_DEAD);
+ return connection_set_status (STATUS_DEAD);
}
}
+ return false;
}
-static void
+static bool
send_structured_reply_read (uint64_t handle, uint16_t cmd,
const char *buf, uint32_t count, uint64_t offset)
{
@@ -420,8 +420,7 @@ send_structured_reply_read (uint64_t handle, uint16_t cmd,
r = conn->send (&reply, sizeof reply, SEND_MORE);
if (r == -1) {
nbdkit_error ("write reply: %s: %m", name_of_nbd_cmd (cmd));
- connection_set_status (STATUS_DEAD);
- return;
+ return connection_set_status (STATUS_DEAD);
}
/* Send the offset + read data buffer. */
@@ -429,15 +428,15 @@ send_structured_reply_read (uint64_t handle, uint16_t cmd,
r = conn->send (&offset_data, sizeof offset_data, SEND_MORE);
if (r == -1) {
nbdkit_error ("write data: %s: %m", name_of_nbd_cmd (cmd));
- connection_set_status (STATUS_DEAD);
- return;
+ return connection_set_status (STATUS_DEAD);
}
r = conn->send (buf, count, 0);
if (r == -1) {
nbdkit_error ("write data: %s: %m", name_of_nbd_cmd (cmd));
- connection_set_status (STATUS_DEAD);
+ return connection_set_status (STATUS_DEAD);
}
+ return false;
}
/* Convert a list of extents into NBD_REPLY_TYPE_BLOCK_STATUS blocks.
@@ -522,7 +521,7 @@ extents_to_block_descriptors (struct nbdkit_extents
*extents,
return blocks;
}
-static void
+static bool
send_structured_reply_block_status (uint64_t handle,
uint16_t cmd, uint16_t flags,
uint32_t count, uint64_t offset,
@@ -542,10 +541,8 @@ send_structured_reply_block_status (uint64_t handle,
blocks = extents_to_block_descriptors (extents, flags, count, offset,
&nr_blocks);
- if (blocks == NULL) {
- connection_set_status (STATUS_DEAD);
- return;
- }
+ if (blocks == NULL)
+ return connection_set_status (STATUS_DEAD);
reply.magic = htobe32 (NBD_STRUCTURED_REPLY_MAGIC);
reply.handle = handle;
@@ -557,8 +554,7 @@ send_structured_reply_block_status (uint64_t handle,
r = conn->send (&reply, sizeof reply, SEND_MORE);
if (r == -1) {
nbdkit_error ("write reply: %s: %m", name_of_nbd_cmd (cmd));
- connection_set_status (STATUS_DEAD);
- return;
+ return connection_set_status (STATUS_DEAD);
}
/* Send the base:allocation context ID. */
@@ -566,8 +562,7 @@ send_structured_reply_block_status (uint64_t handle,
r = conn->send (&context_id, sizeof context_id, SEND_MORE);
if (r == -1) {
nbdkit_error ("write reply: %s: %m", name_of_nbd_cmd (cmd));
- connection_set_status (STATUS_DEAD);
- return;
+ return connection_set_status (STATUS_DEAD);
}
/* Send each block descriptor. */
@@ -576,12 +571,13 @@ send_structured_reply_block_status (uint64_t handle,
i == nr_blocks - 1 ? 0 : SEND_MORE);
if (r == -1) {
nbdkit_error ("write reply: %s: %m", name_of_nbd_cmd (cmd));
- connection_set_status (STATUS_DEAD);
+ return connection_set_status (STATUS_DEAD);
}
}
+ return false;
}
-static void
+static bool
send_structured_reply_error (uint64_t handle, uint16_t cmd, uint16_t flags,
uint32_t error)
{
@@ -600,8 +596,7 @@ send_structured_reply_error (uint64_t handle, uint16_t cmd,
uint16_t flags,
r = conn->send (&reply, sizeof reply, SEND_MORE);
if (r == -1) {
nbdkit_error ("write error reply: %m");
- connection_set_status (STATUS_DEAD);
- return;
+ return connection_set_status (STATUS_DEAD);
}
/* Send the error. */
@@ -610,12 +605,14 @@ send_structured_reply_error (uint64_t handle, uint16_t
cmd, uint16_t flags,
r = conn->send (&error_data, sizeof error_data, 0);
if (r == -1) {
nbdkit_error ("write data: %s: %m", name_of_nbd_cmd (cmd));
- connection_set_status (STATUS_DEAD);
+ return connection_set_status (STATUS_DEAD);
}
/* No human readable error message at the moment. */
+ return false;
}
-void
+/* Do a recv/send sequence. Return true if the caller should shutdown. */
+bool
protocol_recv_request_send_reply (void)
{
GET_CONN;
@@ -634,24 +631,21 @@ protocol_recv_request_send_reply (void)
r = conn->recv (&request, sizeof request);
cs = connection_get_status ();
if (cs <= STATUS_CLIENT_DONE)
- return;
+ return false;
if (r == -1) {
nbdkit_error ("read request: %m");
- connection_set_status (STATUS_DEAD);
- return;
+ return connection_set_status (STATUS_DEAD);
}
if (r == 0) {
debug ("client closed input socket, closing connection");
- connection_set_status (STATUS_CLIENT_DONE); /* disconnect */
- return;
+ return connection_set_status (STATUS_CLIENT_DONE); /* disconnect */
}
magic = be32toh (request.magic);
if (magic != NBD_REQUEST_MAGIC) {
nbdkit_error ("invalid request: 'magic' field is incorrect
(0x%x)",
magic);
- connection_set_status (STATUS_DEAD);
- return;
+ return connection_set_status (STATUS_DEAD);
}
flags = be16toh (request.flags);
@@ -662,16 +656,14 @@ protocol_recv_request_send_reply (void)
if (cmd == NBD_CMD_DISC) {
debug ("client sent %s, closing connection", name_of_nbd_cmd
(cmd));
- connection_set_status (STATUS_CLIENT_DONE); /* disconnect */
- return;
+ return connection_set_status (STATUS_CLIENT_DONE); /* disconnect */
}
/* Validate the request. */
if (!validate_request (cmd, flags, offset, count, &error)) {
if (cmd == NBD_CMD_WRITE &&
skip_over_write_buffer (conn->sockin, count) < 0) {
- connection_set_status (STATUS_DEAD);
- return;
+ return connection_set_status (STATUS_DEAD);
}
goto send_reply;
}
@@ -685,8 +677,7 @@ protocol_recv_request_send_reply (void)
error = ENOMEM;
if (cmd == NBD_CMD_WRITE &&
skip_over_write_buffer (conn->sockin, count) < 0) {
- connection_set_status (STATUS_DEAD);
- return;
+ return connection_set_status (STATUS_DEAD);
}
goto send_reply;
}
@@ -711,8 +702,7 @@ protocol_recv_request_send_reply (void)
}
if (r == -1) {
nbdkit_error ("read data: %s: %m", name_of_nbd_cmd (cmd));
- connection_set_status (STATUS_DEAD);
- return;
+ return connection_set_status (STATUS_DEAD);
}
}
}
@@ -731,7 +721,7 @@ protocol_recv_request_send_reply (void)
/* Send the reply packet. */
send_reply:
if (connection_get_status () < STATUS_CLIENT_DONE)
- return;
+ return false;
if (error != 0) {
/* Since we're about to send only the limited NBD_E* errno to the
@@ -752,14 +742,15 @@ protocol_recv_request_send_reply (void)
(cmd == NBD_CMD_READ || cmd == NBD_CMD_BLOCK_STATUS)) {
if (!error) {
if (cmd == NBD_CMD_READ)
- send_structured_reply_read (request.handle, cmd, buf, count, offset);
+ return send_structured_reply_read (request.handle, cmd, buf, count,
+ offset);
else /* NBD_CMD_BLOCK_STATUS */
- send_structured_reply_block_status (request.handle, cmd, flags,
- count, offset, extents);
+ return send_structured_reply_block_status (request.handle, cmd, flags,
+ count, offset, extents);
}
else
- send_structured_reply_error (request.handle, cmd, flags, error);
+ return send_structured_reply_error (request.handle, cmd, flags, error);
}
else
- send_simple_reply (request.handle, cmd, flags, buf, count, error);
+ return send_simple_reply (request.handle, cmd, flags, buf, count, error);
}
diff --git a/server/public.c b/server/public.c
index c2f67451..74ac18a1 100644
--- a/server/public.c
+++ b/server/public.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2013-2022 Red Hat Inc.
+ * Copyright (C) 2013-2023 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -1107,5 +1107,8 @@ nbdkit_disconnect (int force)
debug ("no connection in this thread, ignoring disconnect
request");
return;
}
- connection_set_status (force ? STATUS_DEAD : STATUS_SHUTDOWN);
+ if (connection_set_status (force ? STATUS_DEAD : STATUS_SHUTDOWN)) {
+ ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&conn->write_lock);
+ conn->close (SHUT_WR);
+ }
}
diff --git a/server/test-public.c b/server/test-public.c
index e1c48cba..e0249fc7 100644
--- a/server/test-public.c
+++ b/server/test-public.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2018-2022 Red Hat Inc.
+ * Copyright (C) 2018-2023 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -96,7 +96,7 @@ connection_get_status (void)
abort ();
}
-void
+bool
connection_set_status (conn_status v)
{
abort ();
diff --git a/tests/test-client-death-tls.sh b/tests/test-client-death-tls.sh
new file mode 100755
index 00000000..49b3ec18
--- /dev/null
+++ b/tests/test-client-death-tls.sh
@@ -0,0 +1,81 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2019-2023 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 -x
+
+requires nbdsh -c 'exit(not h.supports_tls())'
+
+
+# Does the nbdkit binary support TLS?
+if ! nbdkit --dump-config | grep -sq tls=yes; then
+ echo "$0: nbdkit built without TLS support"
+ exit 77
+fi
+
+# Did we create the PSK keys file?
+# Probably 'certtool' is missing.
+if [ ! -s keys.psk ]; then
+ echo "$0: PSK keys file was not created by the test harness"
+ exit 77
+fi
+
+sock=$(mktemp -u /tmp/nbdkit-test-sock.XXXXXX)
+files="client-death-tls.pid $sock"
+cleanup_fn rm -f $files
+
+# Start long-running nbdkit
+start_nbdkit -P client-death-tls.pid --tls require --tls-psk=keys.psk \
+ -U $sock memory 2M
+
+pid=`cat client-death-tls.pid`
+
+# We can't use 'nbdsh -u "$uri" because of
nbd_set_uri_allow_local_file.
+# Run a client that abandons several in-flight requests, each large enough
+# that we should see EPIPE on one handler while other handlers are still
+# waiting to send their response.
+nbdsh -c '
+h.set_tls(nbd.TLS_REQUIRE)
+h.set_tls_psk_file("keys.psk")
+h.set_tls_username("qemu")
+h.connect_unix("'"$sock"'")
+
+buf = nbd.Buffer(2*1024*1024)
+c1 = h.aio_pread(buf, 0)
+c2 = h.aio_pread(buf, 0)
+c3 = h.aio_pread(buf, 0)
+c4 = h.aio_pread(buf, 0)
+'
+
+# nbdkit should still be running
+sleep 1
+kill -s 0 $pid
diff --git a/tests/test-client-death.sh b/tests/test-client-death.sh
new file mode 100755
index 00000000..d7f57ca9
--- /dev/null
+++ b/tests/test-client-death.sh
@@ -0,0 +1,60 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2019-2023 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 -x
+
+requires_nbdsh_uri
+
+sock=$(mktemp -u /tmp/nbdkit-test-sock.XXXXXX)
+files="client-death.pid $sock"
+cleanup_fn rm -f $files
+
+# Start long-running nbdkit
+start_nbdkit -P client-death.pid -U $sock memory 2M
+
+pid=`cat client-death.pid`
+
+# Run a client that abandons several in-flight requests, each large enough
+# that we should see EPIPE on one handler while other handlers are still
+# waiting to send their response.
+nbdsh -u "nbd+unix:///?socket=$sock" -c '
+buf = nbd.Buffer(2*1024*1024)
+c1 = h.aio_pread(buf, 0)
+c2 = h.aio_pread(buf, 0)
+c3 = h.aio_pread(buf, 0)
+c4 = h.aio_pread(buf, 0)
+'
+
+# nbdkit should still be running
+sleep 1
+kill -s 0 $pid
--
2.39.2
Laszlo Ersek
2023-Feb-26 09:16 UTC
[Libguestfs] [nbdkit PATCH v2] server: Don't assert on send if client hangs up early
On 2/24/23 23:59, Eric Blake wrote:> @@ -752,14 +742,15 @@ protocol_recv_request_send_reply (void) > (cmd == NBD_CMD_READ || cmd == NBD_CMD_BLOCK_STATUS)) { > if (!error) { > if (cmd == NBD_CMD_READ) > - send_structured_reply_read (request.handle, cmd, buf, count, offset); > + return send_structured_reply_read (request.handle, cmd, buf, count, > + offset); > else /* NBD_CMD_BLOCK_STATUS */ > - send_structured_reply_block_status (request.handle, cmd, flags, > - count, offset, extents); > + return send_structured_reply_block_status (request.handle, cmd, flags, > + count, offset, extents); > } > else > - send_structured_reply_error (request.handle, cmd, flags, error); > + return send_structured_reply_error (request.handle, cmd, flags, error); > } > else > - send_simple_reply (request.handle, cmd, flags, buf, count, error); > + return send_simple_reply (request.handle, cmd, flags, buf, count, error); > }I think this would look better: if (!conn->structured_replies || (cmd != NBD_CMD_READ && cmd != NBD_CMD_BLOCK_STATUS)) return send_simple_reply (request.handle, cmd, flags, buf, count, error); if (error) { return send_structured_reply_error (request.handle, cmd, flags, error); if (cmd == NBD_CMD_READ) return send_structured_reply_read (request.handle, cmd, buf, count, offset); /* NBD_CMD_BLOCK_STATUS */ return send_structured_reply_block_status (request.handle, cmd, flags, count, offset, extents); Either way: Reviewed-by: Laszlo Ersek <lersek at redhat.com>