7:22 a.m.
On Wed, Jul 27, 2022 at 05:30:58PM +0100, 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
Especially since we are embedded in a larger program, where we may not
always have say on whether the rest of the program wants a SIGPIPE
handler installed.
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])
Do we really need this? gnutls is one of the nicer programs which
does both an enum and #define wrappers around the enum:
/usr/include/gnutls/gnutls.h:505: GNUTLS_NO_SIGNAL = (1<<6),
/usr/include/gnutls/gnutls.h:533:#define GNUTLS_NO_SIGNAL (1<<6)
which means that we can merely use #ifdef GNUTLS_NO_SIGNAL rather than
having to define HAVE_GNUTLS_NO_SIGNAL as a wrapper ourselves,
provided that we only care about the feature during .c files. (If we
cared during Makefile.am, to skip compilation of an entire test, then
it would make more sense to add this snippet to configure.ac)
+++ 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.
It sounds like MSG_NOSIGNAL does not exist on all platforms, so even
with GnuTLS >= 3.4.2 we may still be in the scenario where we install
this signal handler. The comment could be made slightly more
accurate, along the lines of:
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 !HAVE_GNUTLS_NO_SIGNAL
+#if TLS
Could simplify to:
#if TLS && !defined(GNUTLS_NO_SIGNAL)
+ 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
+ );
I'm not a fan of mid-expression #if. If the expression is itself a
macro call, the preprocessor may have issues. I would have written:
unsigned int flags = GNUTLS_CLIENT | GNUTLS_NONBLOCK;
#ifdef GNUTLS_NO_SIGNAL
flags |= GNUTLS_NO_SIGNAL;
#endif
err = gnutls_init (&session, flags);
However, despite my comments, I agree with the principle of the patch
- we cannot assume the larger application will install a SIGPIPE
handler (except in the limited cases like our testsuite or nbdcopy
where we ARE the larger application), so telling GnuTLS to avoid it on
our behalf when possible is wise.
We may also want to add documentation to warn users (such as on BSD)
that larger applications that do not install SIGPIPE handlers, but
which lack MSG_NOSIGNAL, are at risk of subtle failures when the
server disconnects abruptly.
I also found this link, which suggests we may also want to try
setsockopt (SO_NOSIGPIPE) where that is available (MacOS), and that we
are out of luck on Solaris:
https://github.com/lpeterse/haskell-socket/issues/8
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org