Re: [Libguestfs] [PATCH libnbd 1/2] lib/crypto: Use GNUTLS_NO_SIGNAL if available

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.