Libguestfs - Jul 2022 - [PATCH libnbd v2 0/3] Use GNUTLS_NO_SIGNAL if available

V1 was here:

https://listman.redhat.com/archives/libguestfs/2022-July/029545.html

The original second patch is now upstream.

As well as updating 1/3 with the feedback, I added two new patches.

This has been compile tested on FreeBSD and doesn't seem to break
anything, but testing if it correctly suppresses SIGPIPE on FreeBSD is
tricky.

Rich.

libnbd has long used MSG_NOSIGNAL to avoid receiving SIGPIPE if we
accidentally write on a closed socket, which is a nice alternative to
using a SIGPIPE signal handler.  However with TLS connections, gnutls
did not use this flag and so programs using libnbd + TLS would receive
SIGPIPE in some situations, notably if the server closed the
connection abruptly while we were trying to write something.

GnuTLS 3.4.2 introduces GNUTLS_NO_SIGNAL which does the same thing.
Use this flag if available.

RHEL 7 has an older gnutls which lacks this flag.  To avoid qemu-nbd
interop tests failing (rarely, but more often with a forthcoming
change to TLS shutdown behaviour), register a SIGPIPE signal handler
in the test if the flag is missing.
---
 interop/interop.c | 8 ++++++++
 lib/crypto.c      | 7 ++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/interop/interop.c b/interop/interop.c
index f3437d7dea..bd5dc2e196 100644
--- a/interop/interop.c
+++ b/interop/interop.c
@@ -84,6 +84,14 @@ main (int argc, char *argv[])
   REQUIRES
 #endif
 
+  /* Ignore SIGPIPE.  We only need this for GnuTLS that lacks the
+   * GNUTLS_NO_SIGNAL flag, either because it predates GnuTLS 3.4.2 or
+   * because the OS lacks MSG_NOSIGNAL support.
+   */
+#if TLS && !defined(HAVE_GNUTLS_NO_SIGNAL)
+  signal (SIGPIPE, SIG_IGN);
+#endif
+
   /* Create a large sparse temporary file. */
 #ifdef NEEDS_TMPFILE
   int fd = mkstemp (TMPFILE);
diff --git a/lib/crypto.c b/lib/crypto.c
index 9d6332c6c9..7ff83c2314 100644
--- a/lib/crypto.c
+++ b/lib/crypto.c
@@ -606,8 +606,13 @@ nbd_internal_crypto_create_session (struct nbd_handle *h,
   gnutls_session_t session;
   gnutls_psk_client_credentials_t pskcreds = NULL;
   gnutls_certificate_credentials_t xcreds = NULL;
+  gnutls_init_flags_t init_flags;
 
-  err = gnutls_init (&session, GNUTLS_CLIENT|GNUTLS_NONBLOCK);
+  init_flags = GNUTLS_CLIENT | GNUTLS_NONBLOCK;
+#ifdef GNUTLS_NO_SIGNAL
+  init_flags |= GNUTLS_NO_SIGNAL;
+#endif
+  err = gnutls_init (&session, init_flags);
   if (err < 0) {
     set_error (errno, "gnutls_init: %s", gnutls_strerror (err));
     return NULL;
-- 
2.37.0.rc2

If the platform supports it, set the SO_NOSIGPIPE socket option on
sockets that we create in nbd_connect_* functions (except
nbd_connect_socket where the caller gets to choose).

Link: https://www.doof.me.uk/2020/09/23/sigpipe-and-how-to-ignore-it/
---
 generator/states-connect.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/generator/states-connect.c b/generator/states-connect.c
index 8de12183d6..8a286e6630 100644
--- a/generator/states-connect.c
+++ b/generator/states-connect.c
@@ -45,6 +45,21 @@ disable_nagle (int sock)
   setsockopt (sock, IPPROTO_TCP, TCP_NODELAY, &flag, sizeof flag);
 }
 
+/* Disable SIGPIPE on FreeBSD & MacOS.
+ *
+ * Does nothing on other platforms, but if those platforms have
+ * MSG_NOSIGNAL then we will set that when writing.  (FreeBSD has both.)
+ */
+static void
+disable_sigpipe (int sock)
+{
+#ifdef SO_NOSIGPIPE
+  const int flag = 1;
+
+  setsockopt (sock, SOL_SOCKET, SO_NOSIGPIPE, &flag, sizeof flag);
+#endif
+}
+
 STATE_MACHINE {
  CONNECT.START:
   sa_family_t family;
@@ -65,6 +80,7 @@ STATE_MACHINE {
   }
 
   disable_nagle (fd);
+  disable_sigpipe (fd);
 
   r = connect (fd, (struct sockaddr *) &h->connaddr, h->connaddrlen);
   if (r == 0 || (r == -1 && errno == EINPROGRESS))
@@ -177,6 +193,7 @@ STATE_MACHINE {
   }
 
   disable_nagle (fd);
+  disable_sigpipe (fd);
 
   if (connect (fd, h->rp->ai_addr, h->rp->ai_addrlen) == -1) {
     if (errno != EINPROGRESS) {
-- 
2.37.0.rc2

Document that libnbd doesn't set signal handlers, use of MSG_NOSIGNAL
or SO_NOSIGPIPE, and optional registration of a global SIGPIPE
handler.
---
 docs/libnbd.pod | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/docs/libnbd.pod b/docs/libnbd.pod
index 7cb2a48473..dd880c3bff 100644
--- a/docs/libnbd.pod
+++ b/docs/libnbd.pod
@@ -940,6 +940,18 @@ should return C<1> on all control paths, even when
handling errors
 (note that with automatic retirement, assigning into C<error> is
 pointless as there is no later API to see that value).
 
+=head1 SIGNALS
+
+Libnbd does not install signal handlers.  It attempts to disable
+C<SIGPIPE> when writing to the NBD socket using the C<MSG_NOSIGNAL>
+flag of L<send(2)>, or the C<SO_NOSIGPIPE> socket option, on
platforms
+that support those.
+
+On some old Linux or newer non-Linux platforms the main program may
+wish to register a signal handler to ignore SIGPIPE:
+
+ signal (SIGPIPE, SIG_IGN);
+
 =head1 COMPILING YOUR PROGRAM
 
 On most systems, C programs that use libnbd can be compiled like this:
-- 
2.37.0.rc2

On Thu, Jul 28, 2022 at 03:19:07PM +0100, Richard W.M. Jones
wrote:
> V1 was here:
> 
> https://listman.redhat.com/archives/libguestfs/2022-July/029545.html
> 
> The original second patch is now upstream.
> 
> As well as updating 1/3 with the feedback, I added two new patches.
> 
> This has been compile tested on FreeBSD and doesn't seem to break
> anything, but testing if it correctly suppresses SIGPIPE on FreeBSD is
> tricky.
ACK series

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org