event: when unreffing an event source from its own handler, detach fd from epoll

The pattern of unreffing an IO event source and then closing its fd is
frequently seen in even source callbacks. Previously this likely
resultet in us removing the fd from the epoll after it was closed which
is problematic, since while we were dispatching we always kept an extra
reference to event source objects because we might still need it later.

3 files changed, 53 insertions, 12 deletions

diff --git a/TODO b/TODO
index 4274b37ee9..0ea410953c 100644
--- a/TODO
+++ b/TODO
@@ -134,8 +134,7 @@ Features:
   - fix sd-event hookup when we connect to multiple servers one after the other
 
 * sd-event
-  - allow multiple signal handlers per signal
-  - when dispatching an event source then _unref() on it should remove it from the epoll
+  - allow multiple signal handlers per signal?
 
 * in the final killing spree, detect processes from the root directory, and
   complain loudly if they have argv[0][0] == '@' set.
diff --git a/src/libsystemd-bus/sd-event.c b/src/libsystemd-bus/sd-event.c
index b3964325a0..6af52ecb3c 100644
--- a/src/libsystemd-bus/sd-event.c
+++ b/src/libsystemd-bus/sd-event.c
@@ -58,6 +58,7 @@ struct sd_event_source {
         EventSourceType type:4;
         int enabled:3;
         bool pending:1;
+        bool dispatching:1;
 
         int priority;
         unsigned pending_index;
@@ -993,8 +994,21 @@ _public_ sd_event_source* sd_event_source_unref(sd_event_source *s) {
         assert(s->n_ref >= 1);
         s->n_ref--;
 
-        if (s->n_ref <= 0)
-                source_free(s);
+        if (s->n_ref <= 0) {
+                /* Here's a special hack: when we are called from a
+                 * dispatch handler we won't free the event source
+                 * immediately, but we will detach the fd from the
+                 * epoll. This way it is safe for the caller to unref
+                 * the event source and immediately close the fd, but
+                 * we still retain a valid event source object after
+                 * the callback. */
+
+                if (s->dispatching) {
+                        if (s->type == SOURCE_IO)
+                                source_io_unregister(s);
+                } else
+                        source_free(s);
+        }
 
         return NULL;
 }
@@ -1689,7 +1703,7 @@ static int source_dispatch(sd_event_source *s) {
                         return r;
         }
 
-        sd_event_source_ref(s);
+        s->dispatching = true;
 
         switch (s->type) {
 
@@ -1734,12 +1748,16 @@ static int source_dispatch(sd_event_source *s) {
                 break;
         }
 
-        if (r < 0) {
+        s->dispatching = false;
+
+        if (r < 0)
                 log_debug("Event source %p returned error, disabling: %s", s, strerror(-r));
+
+        if (s->n_ref == 0)
+                source_free(s);
+        else if (r < 0)
                 sd_event_source_set_enabled(s, SD_EVENT_OFF);
-        }
 
-        sd_event_source_unref(s);
         return 1;
 }
 
@@ -1761,10 +1779,18 @@ static int event_prepare(sd_event *e) {
                         return r;
 
                 assert(s->prepare);
+
+                s->dispatching = true;
                 r = s->prepare(s, s->userdata);
+                s->dispatching = false;
+
                 if (r < 0)
-                        return r;
+                        log_debug("Prepare callback of event source %p returned error, disabling: %s", s, strerror(-r));
 
+                if (s->n_ref == 0)
+                        source_free(s);
+                else if (r < 0)
+                        sd_event_source_set_enabled(s, SD_EVENT_OFF);
         }
 
         return 0;
diff --git a/src/libsystemd-bus/test-event.c b/src/libsystemd-bus/test-event.c
index 5317008a87..28ef6a3692 100644
--- a/src/libsystemd-bus/test-event.c
+++ b/src/libsystemd-bus/test-event.c
@@ -28,9 +28,15 @@ static int prepare_handler(sd_event_source *s, void *userdata) {
         return 1;
 }
 
-static bool got_a, got_b, got_c;
+static bool got_a, got_b, got_c, got_unref;
 static unsigned got_d;
 
+static int unref_handler(sd_event_source *s, int fd, uint32_t revents, void *userdata) {
+        sd_event_source_unref(s);
+        got_unref = true;
+        return 0;
+}
+
 static int io_handler(sd_event_source *s, int fd, uint32_t revents, void *userdata) {
 
         log_info("got IO on %c", PTR_TO_INT(userdata));
@@ -155,18 +161,26 @@ static int exit_handler(sd_event_source *s, void *userdata) {
 
 int main(int argc, char *argv[]) {
         sd_event *e = NULL;
-        sd_event_source *w = NULL, *x = NULL, *y = NULL, *z = NULL, *q = NULL;
+        sd_event_source *w = NULL, *x = NULL, *y = NULL, *z = NULL, *q = NULL, *t = NULL;
         static const char ch = 'x';
-        int a[2] = { -1, -1 }, b[2] = { -1, -1}, d[2] = { -1, -1};
+        int a[2] = { -1, -1 }, b[2] = { -1, -1}, d[2] = { -1, -1}, k[2] = { -1, -1 };
 
         assert_se(pipe(a) >= 0);
         assert_se(pipe(b) >= 0);
         assert_se(pipe(d) >= 0);
+        assert_se(pipe(k) >= 0);
 
         assert_se(sd_event_default(&e) >= 0);
 
         assert_se(sd_event_set_watchdog(e, true) >= 0);
 
+        /* Test whether we cleanly can destroy an io event source from its own handler */
+        got_unref = false;
+        assert_se(sd_event_add_io(e, k[0], EPOLLIN, unref_handler, NULL, &t) >= 0);
+        assert_se(write(k[1], &ch, 1) == 1);
+        assert_se(sd_event_run(e, (uint64_t) -1) >= 1);
+        assert_se(got_unref);
+
         got_a = false, got_b = false, got_c = false, got_d = 0;
 
         /* Add a oneshot handler, trigger it, re-enable it, and trigger
@@ -227,6 +241,8 @@ int main(int argc, char *argv[]) {
 
         close_pipe(a);
         close_pipe(b);
+        close_pipe(d);
+        close_pipe(k);
 
         return 0;
 }