From: Lennart Poettering Date: Fri, 13 Dec 2013 03:03:30 +0000 (+0100) Subject: event: when unreffing an event source from its own handler, detach fd from epoll X-Git-Tag: v209~952 X-Git-Url: http://www.chiark.greenend.org.uk/ucgi/~ianmdlvl/git?p=elogind.git;a=commitdiff_plain;h=12179984a38fe74581333fbcdc11c822d81f505f;hp=7b77ed8cf36e8eca6017791626044b61ae2d68e7 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 4274b37ee..0ea410953 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 b3964325a..6af52ecb3 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 5317008a8..28ef6a369 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; }