Re: [Libguestfs] [nbdkit PATCH] server: Don't assert on send if client hangs up early

On Fri, Feb 24, 2023 at 06:41:49AM +0100, Laszlo Ersek wrote: Good question. raw_send_socket() is never invoked directly, but rather used by the function pointer conn->send() (that's how we switch between raw_send_socket() and crypto_send() when enabling TLS). More importantly, note how the function starts: static int raw_send_socket (const void *vbuf, size_t len, int flags) { GET_CONN; int sock = conn->sockout; and looking at the definition of GET_CONN, conn is a non-NULL pointer in thread-local storage. You are indeed correct that once we close our socket, another client can be connecting in parallel in a different thread and open its own fd on the number we had previously been storing in conn->sockout. So now we have to check if all assignments/usages of conn->sockout are thread-safe. conn->sockout is directly referenced during protocol-handshake-newstyle.c calling crypto_negotiate_tls() - but at that point, we know the socket is still open. All other references to conn->sockout are in connections.c. new_connection() assigns it as part of creating the socket - at that point, conn is thread-local, and we haven't utilized the fd, so it is not invalid. raw_close() (reached by conn->close()) assigns -1 to conn->sockout after closing it on SHUT_WR, but leaves the variable unchanged after closing it on SHUT_RDWR. So next we check the two callers: conn->close(SHUT_RDWR) is only called from free_connection(), which is only reached after handle_single_connection() has joined all worker threads for conn, so there is no other thread that can attempt to read the value that we left stale. Meanwhile, conn->close(SHUT_WR) is only called from connection_set_status(), which is guarded by the mutex conn->status_lock if more than one thread can be using the connection. We'll use that knowledge below. The only other access of conn->sockout is when raw_send_socket() reads its value to learn where to attempt to send, and here, we have to widen our search to observe that conn->send() is reached in multiple places; however, anything related to handshaking is done while conn is still single-threaded (handle_single_connection() calls protocol_handshake() before spawning worker threads), so the only time we can have more than one thread using conn->send() simultaneously is during protocol_recv_request_send_reply(). All such uses of conn->send() from that function are guarded by the mutex conn->write_lock. But the question you asked is whether there can ever be a race where: thread 1 thread 2 thread 3 protocol_recv_request_send_reply() protocol_recv_request_send_reply() - read from socket - call into plugin - connection_get_status(): - grab conn->status_lock - observe live socket (aka conn->sockout still valid) - release status_lock .. - grab conn->read_lock .. - conn->recv() detects failure .. - call connection_set_status(STATUS_DEAD) .. - grab conn->status_lock .. - call conn->close(SHUT_WR) .. - call closesocket() > window where another - call send_*() to send reply .. > client can connect, - grab conn->write_lock .. > thereby claiming the - call conn->send() .. > fd still present in - sock=conn->sockout .. > thread 1's conn->sockout .. - assign conn->sockout=-1 - write to sock Eww. It looks like your hunch was right. I don't see adequate protection to prevent thread 1 from attempting to use a stale conn->sockout value, and it IS possible that another client connecting at the same time can have re-allocated that fd. multi-conn is precisely multiple clients for a single nbdkit server! But yes, it is also possible for a single server to serve multiple non-related clients even without multiconn (perhaps even different data to parallel clients, such as the file plugin in dir mode where the export name requested by the client determines which file is being served over the connection). No, it is spot on. And it means I need to do a v2 of this patch, which properly protects conn->sockout from stale reuse by a second thread. I don't know if ref-counting is easiest, or just ensuring that the assignment to conn->sockout always has a happens-before relationship to another thread utilizing conn->sockout. conn->read_lock and conn->write_lock are meant to be held for potentially long periods of time (the duration of time for which one direction of the socket is in exclusive use by one half of a message, which may require several network packets). But conn->status_lock is intended to be held as short as possible. Right now, it only protects reads and writes of conn->status, but extending it to also cover reads and writes of conn->sockin/sockout may be sufficient. (It may be possible to use atomics instead of a mutex, except that I'm not sure what code base to copy from - nbdkit's license means I can't copy from Paolo's nice atomics framework in qemu). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org