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

[Laszlo Ersek]

On 07/28/22 15:01, Richard W.M. Jones wrote:
> 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.

Ouch, I completely lost track that this was *libnbd*. :/

You are right of course!

Acked-by: Laszlo Ersek <lersek at redhat.com>


> 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.
>