Re: [PATCH] libxl: Do not call assert() in signal handlers

[Ian Campbell <ian.campbell@citrix.com>]

On Thu, 2015-10-22 at 16:39 +0100, Ian Jackson wrote:
> assert is not async-signal-safe.

I don't doubt you, but I'm curious regarding a reference.

http://pubs.opengroup.org/onlinepubs/9699919799/functions/assert.html doesn
't appear to be it, unless it is too subtle for me.

> In practice the effect of calling assert there is that if the
> assertion fails we might get a secondary crash, or other undesirable
> behaviour from stdio (which is how assert usually reports failures).

Or maybe it's just transitive through the (usual) use of stdio as part of
assert()?

> Mention in a comment in libxl__self_pipe_wakeup that it has to be
> async-signal-safe.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
>  tools/libxl/libxl_event.c       |    3 ++-
>  tools/libxl/libxl_fork.c        |    2 +-
>  tools/libxl/libxl_save_helper.c |    7 +++++--
>  3 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/libxl/libxl_event.c b/tools/libxl/libxl_event.c
> index 7d549ad..0df6d6c 100644
> --- a/tools/libxl/libxl_event.c
> +++ b/tools/libxl/libxl_event.c
> @@ -1564,6 +1564,7 @@ int libxl__pipe_nonblock(libxl_ctx *ctx, int
> fds[2])
>  
>  int libxl__self_pipe_wakeup(int fd)
>  {
> +    /* Called from signal handlers, so needs to be async-signal-safe */
>      static const char buf[1] = "";
>  
>      for (;;) {
> @@ -1572,7 +1573,7 @@ int libxl__self_pipe_wakeup(int fd)
>          assert(r==-1);
>          if (errno == EINTR) continue;
>          if (errno == EWOULDBLOCK) return 0;
> -        assert(errno);
> +        if (!errno) abort();
>          return errno;
>      }
>  }
> diff --git a/tools/libxl/libxl_fork.c b/tools/libxl/libxl_fork.c
> index 024c1e2..eea3d5d 100644
> --- a/tools/libxl/libxl_fork.c
> +++ b/tools/libxl/libxl_fork.c
> @@ -239,7 +239,7 @@ static void sigchld_handler(int signo)
>  
>      LIBXL_LIST_FOREACH(notify, &sigchld_users, sigchld_users_entry) {
>          int e = libxl__self_pipe_wakeup(notify->sigchld_selfpipe[1]);
> -        assert(!e); /* errors are probably EBADF, very bad */
> +        if (e) abort(); /* errors are probably EBADF, very bad */
>      }
>  
>      r = pthread_mutex_unlock(&sigchld_defer_mutex);
> diff --git a/tools/libxl/libxl_save_helper.c
> b/tools/libxl/libxl_save_helper.c
> index 57ae978..39038f9 100644
> --- a/tools/libxl/libxl_save_helper.c
> +++ b/tools/libxl/libxl_save_helper.c
> @@ -148,8 +148,11 @@ static void save_signal_handler(int num)
>      int esave = errno;
>  
>      int r = dup2(unwriteable_fd, io_fd);
> -    assert(r == io_fd); /* if not we can't write an xtl message because we
> -                         * might end up interleaving on our control stream */
> +    if (r != io_fd)
> +        /* we can't write an xtl message because we might end up
> +         * interleaving on our control stream; we can't use stdio
> +         * because it's not async-signal-safe */
> +        abort();
>  
>      errno = esave;
>  }