* [PATCH] libxl: Do not call assert() in signal handlers
@ 2015-10-22 15:39 Ian Jackson
2015-10-22 16:22 ` Ian Campbell
0 siblings, 1 reply; 3+ messages in thread
From: Ian Jackson @ 2015-10-22 15:39 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Ian Jackson, Ian Campbell
assert is not async-signal-safe.
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).
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>
---
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;
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] libxl: Do not call assert() in signal handlers
2015-10-22 15:39 [PATCH] libxl: Do not call assert() in signal handlers Ian Jackson
@ 2015-10-22 16:22 ` Ian Campbell
2015-10-23 13:53 ` Ian Campbell
0 siblings, 1 reply; 3+ messages in thread
From: Ian Campbell @ 2015-10-22 16:22 UTC (permalink / raw)
To: Ian Jackson, xen-devel; +Cc: Andrew Cooper
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;
> }
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] libxl: Do not call assert() in signal handlers
2015-10-22 16:22 ` Ian Campbell
@ 2015-10-23 13:53 ` Ian Campbell
0 siblings, 0 replies; 3+ messages in thread
From: Ian Campbell @ 2015-10-23 13:53 UTC (permalink / raw)
To: Ian Jackson, xen-devel; +Cc: Andrew Cooper
On Thu, 2015-10-22 at 17:22 +0100, Ian Campbell wrote:
> 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 doe
> sn
> '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>
Applied.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-10-23 13:53 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-22 15:39 [PATCH] libxl: Do not call assert() in signal handlers Ian Jackson
2015-10-22 16:22 ` Ian Campbell
2015-10-23 13:53 ` Ian Campbell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.