Re: [Libguestfs] [nbdkit PATCH] 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: $ 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 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 fix is to gracefully handle an abrupt client disconnect, by not attempting to send on an already-closed socket. 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 parallel clients, it means any one client can trigger the SIGABRT by intentionally queueing multiple NBD_CMD_READ then disconnecting early, and thereby kill nbdkit and starve other clients. Whether it 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 so there is no privilege boundary escalation). Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2173054 Fixes: daef505e ("server: Give client EOF when we are done writing", v1.32.4) --- server/connections.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/server/connections.c b/server/connections.c index 4d776f2a..1b63eb73 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 @@ -400,7 +400,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;