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.

diff --git a/TODO b/TODO
index 4274b37ee9ed6a4bf32852c76d482545f0fd1154..0ea410953cc150e6109537dc48c50214b8c8d0a7 100644 (file)
--- 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 b3964325a0607e7228451ad94e38e6a42606138e..6af52ecb3cdf45325d91e74d91c0a4801397dbc9 100644 (file)
--- 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 5317008a87139f539f5df14d2860b4ba6020476e..28ef6a3692bf2bb0657b51c6dadd454b54afbef2 100644 (file)
--- 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;
 }