From 4807d2d068ae9fc08b87121fc0a574394f8acc5b Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>

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 <mustrumr97@gmail.com>

---

 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;