Eric Blake
2022-Oct-26 22:18 UTC
[Libguestfs] [nbdkit PATCH 2/4] 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.
---
server/internal.h | 2 +-
server/connections.c | 27 ++++++++++++++++++++-------
server/crypto.c | 22 +++++++++++++---------
3 files changed, 34 insertions(+), 17 deletions(-)
diff --git a/server/internal.h b/server/internal.h
index fa658364..69b4302c 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
diff --git a/server/connections.c b/server/connections.c
index 27177d70..1b6183df 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 1f605083..72486bf8 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
--
2.37.3
Richard W.M. Jones
2022-Oct-27 08:49 UTC
[Libguestfs] [nbdkit PATCH 2/4] server: Give client EOF when we are done writing
On Wed, Oct 26, 2022 at 05:18:00PM -0500, Eric Blake wrote:> - 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;Don't we leak conn->sockin, if how == SHUT_WR && conn->sockin == conn->sockout? Rich.> + } > + 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 1f605083..72486bf8 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 > -- > 2.37.3 > > _______________________________________________ > Libguestfs mailing list > Libguestfs at redhat.com > https://listman.redhat.com/mailman/listinfo/libguestfs-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top