From daef505ea398a2251d56751788c65edc841be7b3 Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@redhat.com>
Date: Wed, 26 Oct 2022 17:18:00 -0500
Subject: [PATCH] server: Give client EOF when we are done writing

If we detect a fatal error but do not close the socket back to the
client, then we can create deadlock where we are stuck waiting to read
NBD_CMD_DISC from the client but where the client is stuck waiting to
read our reply.  Try harder to explicitly inform the client any time
that we have detected when our connection has degraded enough to
warrant that we won't do any more writing, even if we still hang on to
the socket for a bit longer for further reads.

State-wise, all clients will exit through one of two paths:

normal -> conn->close(SHUT_WR) -> conn->close(SHUT_RDWR)
normal -> conn->close(SHUT_RDWR)

That is, SHUT_WR is an optional state which exists to help avoid
deadlocks, but which does not cause an fd leak of sockin because we
will always reach SHUT_RDWR.

Message-Id: <20221026221802.207215-3-eblake@redhat.com>
(cherry picked from commit bf8653b34eba5d2de97af2a5a38e0875286f81a7)
---
 server/connections.c | 27 ++++++++++++++++++++-------
 server/crypto.c      | 22 +++++++++++++---------
 server/internal.h    |  2 +-
 3 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/server/connections.c b/server/connections.c
index 27177d709..1b6183dff 100644
--- a/server/connections.c
+++ b/server/connections.c
@@ -62,7 +62,7 @@ static int raw_send_socket (const void *buf, size_t len, int flags);
 #ifndef WIN32
 static int raw_send_other (const void *buf, size_t len, int flags);
 #endif
-static void raw_close (void);
+static void raw_close (int how);
 
 conn_status
 connection_get_status (void)
@@ -97,6 +97,8 @@ connection_set_status (conn_status value)
       if (write (conn->status_pipe[1], &c, 1) != 1 && errno != EAGAIN)
         debug ("failed to notify pipe-to-self: %m");
     }
+    if (conn->status >= STATUS_CLIENT_DONE && value < STATUS_CLIENT_DONE)
+      conn->close (SHUT_WR);
     conn->status = value;
   }
   if (conn->nworkers &&
@@ -348,7 +350,7 @@ free_connection (struct connection *conn)
   if (!conn)
     return;
 
-  conn->close ();
+  conn->close (SHUT_RDWR);
 
   /* Don't call the plugin again if quit has been set because the main
    * thread will be in the process of unloading it.  The plugin.unload
@@ -397,6 +399,7 @@ raw_send_socket (const void *vbuf, size_t len, int flags)
   ssize_t r;
   int f = 0;
 
+  assert (sock >= 0);
 #ifdef MSG_MORE
   if (flags & SEND_MORE)
     f |= MSG_MORE;
@@ -427,6 +430,7 @@ raw_send_other (const void *vbuf, size_t len, int flags)
   const char *buf = vbuf;
   ssize_t r;
 
+  assert (sock >= 0);
   while (len > 0) {
     r = write (sock, buf, len);
     if (r == -1) {
@@ -489,12 +493,21 @@ raw_recv (void *vbuf, size_t len)
  * close, so this function ignores errors.
  */
 static void
-raw_close (void)
+raw_close (int how)
 {
   GET_CONN;
 
-  if (conn->sockin >= 0)
-    closesocket (conn->sockin);
-  if (conn->sockout >= 0 && conn->sockin != conn->sockout)
-    closesocket (conn->sockout);
+  if (conn->sockout >= 0 && how == SHUT_WR) {
+    if (conn->sockin == conn->sockout)
+      shutdown (conn->sockout, how);
+    else
+      closesocket (conn->sockout);
+    conn->sockout = -1;
+  }
+  else {
+    if (conn->sockin >= 0)
+      closesocket (conn->sockin);
+    if (conn->sockout >= 0 && conn->sockin != conn->sockout)
+      closesocket (conn->sockout);
+  }
 }
diff --git a/server/crypto.c b/server/crypto.c
index 1f6050831..72486bf81 100644
--- a/server/crypto.c
+++ b/server/crypto.c
@@ -412,7 +412,7 @@ crypto_send (const void *vbuf, size_t len, int flags)
  * close, so this function ignores errors.
  */
 static void
-crypto_close (void)
+crypto_close (int how)
 {
   GET_CONN;
   gnutls_session_t session = conn->crypto_session;
@@ -420,17 +420,21 @@ crypto_close (void)
 
   assert (session != NULL);
 
-  gnutls_transport_get_int2 (session, &sockin, &sockout);
+  if (how == SHUT_WR)
+    gnutls_bye (session, GNUTLS_SHUT_WR);
+  else {
+    gnutls_transport_get_int2 (session, &sockin, &sockout);
 
-  gnutls_bye (session, GNUTLS_SHUT_RDWR);
+    gnutls_bye (session, GNUTLS_SHUT_RDWR);
 
-  if (sockin >= 0)
-    closesocket (sockin);
-  if (sockout >= 0 && sockin != sockout)
-    closesocket (sockout);
+    if (sockin >= 0)
+      closesocket (sockin);
+    if (sockout >= 0 && sockin != sockout)
+      closesocket (sockout);
 
-  gnutls_deinit (session);
-  conn->crypto_session = NULL;
+    gnutls_deinit (session);
+    conn->crypto_session = NULL;
+  }
 }
 
 #ifdef WIN32
diff --git a/server/internal.h b/server/internal.h
index fa6583640..69b4302c0 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -194,7 +194,7 @@ typedef int (*connection_recv_function) (void *buf, size_t len)
 typedef int (*connection_send_function) (const void *buf, size_t len,
                                          int flags)
   __attribute__((__nonnull__ (1)));
-typedef void (*connection_close_function) (void);
+typedef void (*connection_close_function) (int how);
 
 /* struct context stores data per connection and backend.  Primarily
  * this is the filter or plugin handle, but other state is also stored