On Wed, Jul 27, 2022 at 05:30:59PM +0100, Richard W.M. Jones wrote: > qemu-nbd doesn't call gnutls_bye to cleanly shut down the connection > after we send NBD_CMD_DISC. When copying from a qemu-nbd server (or > any operation which calls nbd_shutdown) you will see errors like this: > > $ nbdcopy nbds://foo?tls-certificates=/var/tmp/pki null: > nbds://foo?tls-certificates=/var/tmp/pki: nbd_shutdown: gnutls_record_recv: The TLS connection was non-properly terminated. > > Relatedly you may also see: > > nbd_shutdown: gnutls_record_recv: Error in the pull function. > > This commit suppresses the error in the case where we know that we > have shut down writes (which happens after NBD_CMD_DISC has been sent > on the wire). > --- > interop/interop.c | 9 --------- > lib/crypto.c | 17 +++++++++++++++++ > lib/internal.h | 1 + > 3 files changed, 18 insertions(+), 9 deletions(-) > > +++ b/lib/crypto.c > @@ -189,6 +189,22 @@ tls_recv (struct nbd_handle *h, struct socket *sock, void *buf, size_t len) > errno = EAGAIN; > return -1; > } > + if (h->tls_shut_writes && > + (r == GNUTLS_E_PULL_ERROR || r == GNUTLS_E_PREMATURE_TERMINATION)) { > + /* qemu-nbd doesn't call gnutls_bye to cleanly shut down the > + * connection after we send NBD_CMD_DISC, instead it simply > + * closes the connection. On the client side we see > + * "gnutls_record_recv: The TLS connection was non-properly > + * terminated" or "gnutls_record_recv: Error in the pull > + * function.". > + * > + * If we see these errors after we shut down the write side > + * (h->tls_shut_writes), which happens after we have sent > + * NBD_CMD_DISC on the wire, downgrade them to a debug message. > + */ > + debug (h, "gnutls_record_recv: %s", gnutls_strerror (r)); > + return 0; /* EOF */ > + } Nice. These are still hard errors if we have not sent NBD_CMD_DISC (the connection disappearing while we are using it could be a MitM attacker), but once we know we are done talking, tolerating a server abruptly disappearing instead of gracefully leaving is desirable. Reviewed-by: Eric Blake <eblake(a)redhat.com&gt;

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. --- configure.ac | 15 +++++++++++++++ interop/interop.c | 10 ++++++++++ lib/crypto.c | 7 ++++++- 3 files changed, 31 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 86c3a08690..b5bae4f1b2 100644 --- a/configure.ac +++ b/configure.ac @@ -182,6 +182,21 @@ AS_IF([test "$GNUTLS_LIBS" != ""],[ gnutls_session_set_verify_cert \ gnutls_transport_is_ktls_enabled \ ]) + AC_MSG_CHECKING([if gnutls has GNUTLS_NO_SIGNAL])

+++ b/interop/interop.c @@ -84,6 +84,16 @@ main (int argc, char *argv[]) REQUIRES #endif + /* Ignore SIGPIPE. We only need this for GnuTLS < 3.4.2, since + * newer GnuTLS has the GNUTLS_NO_SIGNAL flag which adds + * MSG_NOSIGNAL to each write call.

+ */ +#if !HAVE_GNUTLS_NO_SIGNAL +#if TLS

+ signal (SIGPIPE, SIG_IGN); +#endif +#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 ffc2b4ab5f..ffba4bda9b 100644 --- a/lib/crypto.c +++ b/lib/crypto.c @@ -590,7 +590,12 @@ nbd_internal_crypto_create_session (struct nbd_handle *h, gnutls_psk_client_credentials_t pskcreds = NULL; gnutls_certificate_credentials_t xcreds = NULL; - err = gnutls_init (&session, GNUTLS_CLIENT|GNUTLS_NONBLOCK); + err = gnutls_init (&session, + GNUTLS_CLIENT | GNUTLS_NONBLOCK +#if HAVE_GNUTLS_NO_SIGNAL + | GNUTLS_NO_SIGNAL +#endif + );

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. --- configure.ac | 15 +++++++++++++++ interop/interop.c | 10 ++++++++++ lib/crypto.c | 7 ++++++- 3 files changed, 31 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 86c3a08690..b5bae4f1b2 100644 --- a/configure.ac +++ b/configure.ac @@ -182,6 +182,21 @@ AS_IF([test "$GNUTLS_LIBS" != ""],[ gnutls_session_set_verify_cert \ gnutls_transport_is_ktls_enabled \ ]) + AC_MSG_CHECKING([if gnutls has GNUTLS_NO_SIGNAL]) + AC_COMPILE_IFELSE( + [AC_LANG_PROGRAM([ + #include <gnutls/gnutls.h> + gnutls_session_t session; + ], [ + gnutls_init(&session, GNUTLS_CLIENT|GNUTLS_NO_SIGNAL); + ]) + ], [ + AC_MSG_RESULT([yes]) + AC_DEFINE([HAVE_GNUTLS_NO_SIGNAL], [1], + [GNUTLS_NO_SIGNAL found at compile time]) + ], [ + AC_MSG_RESULT([no]) + ]) LIBS="$old_LIBS" ]) diff --git a/interop/interop.c b/interop/interop.c index b41f3ca887..036545bd82 100644 --- a/interop/interop.c +++ b/interop/interop.c @@ -84,6 +84,16 @@ main (int argc, char *argv[]) REQUIRES #endif + /* Ignore SIGPIPE. We only need this for GnuTLS < 3.4.2, since + * newer GnuTLS has the GNUTLS_NO_SIGNAL flag which adds + * MSG_NOSIGNAL to each write call. + */ +#if !HAVE_GNUTLS_NO_SIGNAL +#if TLS + signal (SIGPIPE, SIG_IGN); +#endif +#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 ffc2b4ab5f..ffba4bda9b 100644 --- a/lib/crypto.c +++ b/lib/crypto.c @@ -590,7 +590,12 @@ nbd_internal_crypto_create_session (struct nbd_handle *h, gnutls_psk_client_credentials_t pskcreds = NULL; gnutls_certificate_credentials_t xcreds = NULL; - err = gnutls_init (&session, GNUTLS_CLIENT|GNUTLS_NONBLOCK); + err = gnutls_init (&session, + GNUTLS_CLIENT | GNUTLS_NONBLOCK +#if HAVE_GNUTLS_NO_SIGNAL + | GNUTLS_NO_SIGNAL +#endif + ); if (err < 0) { set_error (errno, "gnutls_init: %s", gnutls_strerror (err)); return NULL;

On Thu, Jul 28, 2022 at 02:57:49PM +0200, Laszlo Ersek wrote: > On 07/27/22 18:30, Richard W.M. Jones wrote: >> 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. >> --- >> configure.ac | 15 +++++++++++++++ >> interop/interop.c | 10 ++++++++++ >> lib/crypto.c | 7 ++++++- >> 3 files changed, 31 insertions(+), 1 deletion(-) >> >> diff --git a/configure.ac b/configure.ac >> index 86c3a08690..b5bae4f1b2 100644 >> --- a/configure.ac >> +++ b/configure.ac >> @@ -182,6 +182,21 @@ AS_IF([test "$GNUTLS_LIBS" != ""],[ >> gnutls_session_set_verify_cert \ >> gnutls_transport_is_ktls_enabled \ >> ]) >> + AC_MSG_CHECKING([if gnutls has GNUTLS_NO_SIGNAL]) >> + AC_COMPILE_IFELSE( >> + [AC_LANG_PROGRAM([ >> + #include <gnutls/gnutls.h> >> + gnutls_session_t session; >> + ], [ >> + gnutls_init(&session, GNUTLS_CLIENT|GNUTLS_NO_SIGNAL); >> + ]) >> + ], [ >> + AC_MSG_RESULT([yes]) >> + AC_DEFINE([HAVE_GNUTLS_NO_SIGNAL], [1], >> + [GNUTLS_NO_SIGNAL found at compile time]) >> + ], [ >> + AC_MSG_RESULT([no]) >> + ]) >> LIBS="$old_LIBS" >> ]) >> >> diff --git a/interop/interop.c b/interop/interop.c >> index b41f3ca887..036545bd82 100644 >> --- a/interop/interop.c >> +++ b/interop/interop.c >> @@ -84,6 +84,16 @@ main (int argc, char *argv[]) >> REQUIRES >> #endif >> >> + /* Ignore SIGPIPE. We only need this for GnuTLS < 3.4.2, since >> + * newer GnuTLS has the GNUTLS_NO_SIGNAL flag which adds >> + * MSG_NOSIGNAL to each write call. >> + */ >> +#if !HAVE_GNUTLS_NO_SIGNAL >> +#if TLS >> + signal (SIGPIPE, SIG_IGN); >> +#endif >> +#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 ffc2b4ab5f..ffba4bda9b 100644 >> --- a/lib/crypto.c >> +++ b/lib/crypto.c >> @@ -590,7 +590,12 @@ nbd_internal_crypto_create_session (struct nbd_handle *h, >> gnutls_psk_client_credentials_t pskcreds = NULL; >> gnutls_certificate_credentials_t xcreds = NULL; >> >> - err = gnutls_init (&session, GNUTLS_CLIENT|GNUTLS_NONBLOCK); >> + err = gnutls_init (&session, >> + GNUTLS_CLIENT | GNUTLS_NONBLOCK >> +#if HAVE_GNUTLS_NO_SIGNAL >> + | GNUTLS_NO_SIGNAL >> +#endif >> + ); >> if (err < 0) { >> set_error (errno, "gnutls_init: %s", gnutls_strerror (err)); >> return NULL; >> > > I've never seen a single example of SIGPIPE being useful to a program > that checks for, and properly handles, write() / send() / sendto() / > sendmsg() failures. Agreed! > Even if we want to die with a SIGPIPE actually (so that the parent see > it), we can recognize errno == EPIPE, then reset the disposition of > SIGPIPE to SIG_DFLT in a controlled manner, and re-raise the signal. > (But this is totally obscure; in most cases, a parent process is > satisfied if the child exits with status 1 or so in case the child sees > an EPIPE.) > > So, why not set the disposition of SIGPIPE to SIG_IGN indiscriminately? Basically because changing signal handlers of a program from a library is not cool.

There are related issues with programs linked to multiple libraries that might all have their own idea of signal handling, and also with thread safety since signal handlers are program-global state. Rich.