From 4807d2d068ae9fc08b87121fc0a574394f8acc5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Sat, 4 Oct 2014 22:57:43 -0400 Subject: [PATCH] sd-event: be more careful when enabling/disabling signals When a child event is disabled (in order to be freed) and there is no SIGCHLD signal event, sd_event_source_set_enabled will disable SIGCHLD even if there are other child events. Also remove some unneeded signalfd updates. https://bugs.freedesktop.org/show_bug.cgi?id=84659 Based-on-a-patch-by: Hristo Venev --- src/libsystemd/sd-event/sd-event.c | 86 ++++++++++++++++++++++++++++---------- 1 file changed, 63 insertions(+), 23 deletions(-) diff --git a/src/libsystemd/sd-event/sd-event.c b/src/libsystemd/sd-event/sd-event.c index 4c67ee87e1..c5f062b3e0 100644 --- a/src/libsystemd/sd-event/sd-event.c +++ b/src/libsystemd/sd-event/sd-event.c @@ -590,6 +590,14 @@ static struct clock_data* event_get_clock_data(sd_event *e, EventSourceType t) { } } +static bool need_signal(sd_event *e, int signal) { + return (e->signal_sources && e->signal_sources[signal] && + e->signal_sources[signal]->enabled != SD_EVENT_OFF) + || + (signal == SIGCHLD && + e->n_enabled_child_sources > 0); +} + static void source_disconnect(sd_event_source *s) { sd_event *event; @@ -626,11 +634,13 @@ static void source_disconnect(sd_event_source *s) { case SOURCE_SIGNAL: if (s->signal.sig > 0) { - if (s->signal.sig != SIGCHLD || s->event->n_enabled_child_sources == 0) - assert_se(sigdelset(&s->event->sigset, s->signal.sig) == 0); - if (s->event->signal_sources) s->event->signal_sources[s->signal.sig] = NULL; + + /* If the signal was on and now it is off... */ + if (s->enabled != SD_EVENT_OFF && !need_signal(s->event, s->signal.sig)) { + assert_se(sigdelset(&s->event->sigset, s->signal.sig) == 0); + } } break; @@ -640,10 +650,12 @@ static void source_disconnect(sd_event_source *s) { if (s->enabled != SD_EVENT_OFF) { assert(s->event->n_enabled_child_sources > 0); s->event->n_enabled_child_sources--; - } - if (!s->event->signal_sources || !s->event->signal_sources[SIGCHLD]) - assert_se(sigdelset(&s->event->sigset, SIGCHLD) == 0); + /* We know the signal was on, if it is off now... */ + if (!need_signal(s->event, SIGCHLD)) { + assert_se(sigdelset(&s->event->sigset, SIGCHLD) == 0); + } + } hashmap_remove(s->event->child_sources, INT_TO_PTR(s->child.pid)); } @@ -963,6 +975,7 @@ _public_ int sd_event_add_signal( sd_event_source *s; sigset_t ss; int r; + bool previous; assert_return(e, -EINVAL); assert_return(sig > 0, -EINVAL); @@ -987,6 +1000,8 @@ _public_ int sd_event_add_signal( } else if (e->signal_sources[sig]) return -EBUSY; + previous = need_signal(e, sig); + s = source_new(e, !ret, SOURCE_SIGNAL); if (!s) return -ENOMEM; @@ -997,9 +1012,10 @@ _public_ int sd_event_add_signal( s->enabled = SD_EVENT_ON; e->signal_sources[sig] = s; - assert_se(sigaddset(&e->sigset, sig) == 0); - if (sig != SIGCHLD || e->n_enabled_child_sources == 0) { + if (!previous) { + assert_se(sigaddset(&e->sigset, sig) == 0); + r = event_update_signal_fd(e); if (r < 0) { source_free(s); @@ -1023,6 +1039,7 @@ _public_ int sd_event_add_child( sd_event_source *s; int r; + bool previous; assert_return(e, -EINVAL); assert_return(pid > 1, -EINVAL); @@ -1039,6 +1056,8 @@ _public_ int sd_event_add_child( if (hashmap_contains(e->child_sources, INT_TO_PTR(pid))) return -EBUSY; + previous = need_signal(e, SIGCHLD); + s = source_new(e, !ret, SOURCE_CHILD); if (!s) return -ENOMEM; @@ -1057,9 +1076,9 @@ _public_ int sd_event_add_child( e->n_enabled_child_sources ++; - assert_se(sigaddset(&e->sigset, SIGCHLD) == 0); + if (!previous) { + assert_se(sigaddset(&e->sigset, SIGCHLD) == 0); - if (!e->signal_sources || !e->signal_sources[SIGCHLD]) { r = event_update_signal_fd(e); if (r < 0) { source_free(s); @@ -1437,23 +1456,32 @@ _public_ int sd_event_source_set_enabled(sd_event_source *s, int m) { } case SOURCE_SIGNAL: + assert(need_signal(s->event, s->signal.sig)); + s->enabled = m; - if (s->signal.sig != SIGCHLD || s->event->n_enabled_child_sources == 0) { + + if (!need_signal(s->event, s->signal.sig)) { assert_se(sigdelset(&s->event->sigset, s->signal.sig) == 0); - event_update_signal_fd(s->event); + + (void) event_update_signal_fd(s->event); + /* If disabling failed, we might get a spurious event, + * but otherwise nothing bad should happen. */ } break; case SOURCE_CHILD: + assert(need_signal(s->event, SIGCHLD)); + s->enabled = m; assert(s->event->n_enabled_child_sources > 0); s->event->n_enabled_child_sources--; - if (!s->event->signal_sources || !s->event->signal_sources[SIGCHLD]) { + if (!need_signal(s->event, SIGCHLD)) { assert_se(sigdelset(&s->event->sigset, SIGCHLD) == 0); - event_update_signal_fd(s->event); + + (void) event_update_signal_fd(s->event); } break; @@ -1501,22 +1529,34 @@ _public_ int sd_event_source_set_enabled(sd_event_source *s, int m) { } case SOURCE_SIGNAL: - s->enabled = m; - - if (s->signal.sig != SIGCHLD || s->event->n_enabled_child_sources == 0) { + /* Check status before enabling. */ + if (!need_signal(s->event, s->signal.sig)) { assert_se(sigaddset(&s->event->sigset, s->signal.sig) == 0); - event_update_signal_fd(s->event); + + r = event_update_signal_fd(s->event); + if (r < 0) { + s->enabled = SD_EVENT_OFF; + return r; + } } + + s->enabled = m; break; case SOURCE_CHILD: + /* Check status before enabling. */ if (s->enabled == SD_EVENT_OFF) { - s->event->n_enabled_child_sources++; - - if (!s->event->signal_sources || !s->event->signal_sources[SIGCHLD]) { - assert_se(sigaddset(&s->event->sigset, SIGCHLD) == 0); - event_update_signal_fd(s->event); + if (!need_signal(s->event, SIGCHLD)) { + assert_se(sigaddset(&s->event->sigset, s->signal.sig) == 0); + + r = event_update_signal_fd(s->event); + if (r < 0) { + s->enabled = SD_EVENT_OFF; + return r; + } } + + s->event->n_enabled_child_sources++; } s->enabled = m;