From 2fa480592d4f4334881361c5558f563e5ea4c9c3 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 22 Apr 2024 05:22:24 +0900 Subject: [PATCH] sd-event: fix fd leak when fd is owned by IO event source When an IO event source owns relevant fd, replacing with a new fd leaks the previously assigned fd. === sd_event_add_io(event, &s, fd, ...); sd_event_source_set_io_fd_own(s, true); sd_event_source_set_io_fd(s, new_fd); <-- The previous fd is not closed. sd_event_source_unref(s); <-- new_fd is closed as expected. === Without the change, valgrind reports the leak: ==998589== ==998589== FILE DESCRIPTORS: 4 open (3 std) at exit. ==998589== Open file descriptor 4: ==998589== at 0x4F119AB: pipe2 (in /usr/lib64/libc.so.6) ==998589== by 0x408830: test_sd_event_source_set_io_fd (test-event.c:862) ==998589== by 0x403302: run_test_table (tests.h:171) ==998589== by 0x408E31: main (test-event.c:935) ==998589== ==998589== ==998589== HEAP SUMMARY: ==998589== in use at exit: 0 bytes in 0 blocks ==998589== total heap usage: 33,305 allocs, 33,305 frees, 1,283,581 bytes allocated ==998589== ==998589== All heap blocks were freed -- no leaks are possible ==998589== ==998589== For lists of detected and suppressed errors, rerun with: -s ==998589== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0) --- man/sd_event_add_io.xml | 24 ++++++++++++++---------- src/libsystemd/sd-event/sd-event.c | 17 ++++++++--------- src/libsystemd/sd-event/test-event.c | 18 ++++++++++++++++++ 3 files changed, 40 insertions(+), 19 deletions(-) diff --git a/man/sd_event_add_io.xml b/man/sd_event_add_io.xml index 3935141fc05..09a5b11dfff 100644 --- a/man/sd_event_add_io.xml +++ b/man/sd_event_add_io.xml @@ -216,16 +216,20 @@ source object and returns the non-negative file descriptor or a negative error number on error (see below). - sd_event_source_set_io_fd() - changes the UNIX file descriptor of an I/O event source created - previously with sd_event_add_io(). It takes - the event source object and the new file descriptor. - - sd_event_source_set_io_fd_own() controls whether the file descriptor of the event source - shall be closed automatically when the event source is freed, i.e. whether it shall be considered 'owned' by the - event source object. By default it is not closed automatically, and the application has to do this on its own. The - b parameter is a boolean parameter: if zero, the file descriptor is not closed automatically - when the event source is freed, otherwise it is closed. + sd_event_source_set_io_fd() changes the UNIX file descriptor of an I/O event + source created previously with sd_event_add_io(). It takes the event source object + and the new file descriptor. If the event source takes the ownership of the previous file descriptor, + that is, sd_event_source_set_io_fd_own() was called for the event source with a + non-zero value, then the previous file descriptor will be closed and the event source will also take the + ownership of the new file descriptor on success. + + sd_event_source_set_io_fd_own() controls whether the file descriptor of the + event source shall be closed automatically when the event source is freed (or when the file descriptor + assigned to the event source is replaced by sd_event_source_set_io_fd()), i.e. + whether it shall be considered 'owned' by the event source object. By default it is not closed + automatically, and the application has to do this on its own. The b parameter is a + boolean parameter: if zero, the file descriptor is not closed automatically when the event source is + freed, otherwise it is closed. sd_event_source_get_io_fd_own() may be used to query the current setting of the file descriptor ownership boolean flag as set with sd_event_source_set_io_fd_own(). It returns diff --git a/src/libsystemd/sd-event/sd-event.c b/src/libsystemd/sd-event/sd-event.c index 39c60297f06..bd5bd81ac43 100644 --- a/src/libsystemd/sd-event/sd-event.c +++ b/src/libsystemd/sd-event/sd-event.c @@ -2647,7 +2647,7 @@ _public_ int sd_event_source_get_io_fd(sd_event_source *s) { } _public_ int sd_event_source_set_io_fd(sd_event_source *s, int fd) { - int r; + int saved_fd, r; assert_return(s, -EINVAL); assert_return(fd >= 0, -EBADF); @@ -2657,16 +2657,12 @@ _public_ int sd_event_source_set_io_fd(sd_event_source *s, int fd) { if (s->io.fd == fd) return 0; - if (event_source_is_offline(s)) { - s->io.fd = fd; - s->io.registered = false; - } else { - int saved_fd; + saved_fd = s->io.fd; + s->io.fd = fd; - saved_fd = s->io.fd; - assert(s->io.registered); + assert(event_source_is_offline(s) == !s->io.registered); - s->io.fd = fd; + if (s->io.registered) { s->io.registered = false; r = source_io_register(s, s->enabled, s->io.events); @@ -2679,6 +2675,9 @@ _public_ int sd_event_source_set_io_fd(sd_event_source *s, int fd) { (void) epoll_ctl(s->event->epoll_fd, EPOLL_CTL_DEL, saved_fd, NULL); } + if (s->io.owned) + safe_close(saved_fd); + return 0; } diff --git a/src/libsystemd/sd-event/test-event.c b/src/libsystemd/sd-event/test-event.c index 991aa925a42..c6179359922 100644 --- a/src/libsystemd/sd-event/test-event.c +++ b/src/libsystemd/sd-event/test-event.c @@ -843,6 +843,24 @@ TEST(fork) { assert_se(r >= 0); } +TEST(sd_event_source_set_io_fd) { + _cleanup_(sd_event_source_unrefp) sd_event_source *s = NULL; + _cleanup_(sd_event_unrefp) sd_event *e = NULL; + _cleanup_close_pair_ int pfd_a[2] = EBADF_PAIR, pfd_b[2] = EBADF_PAIR; + + assert_se(sd_event_default(&e) >= 0); + + assert_se(pipe2(pfd_a, O_CLOEXEC) >= 0); + assert_se(pipe2(pfd_b, O_CLOEXEC) >= 0); + + assert_se(sd_event_add_io(e, &s, pfd_a[0], EPOLLIN, NULL, INT_TO_PTR(-ENOANO)) >= 0); + assert_se(sd_event_source_set_io_fd_own(s, true) >= 0); + TAKE_FD(pfd_a[0]); + + assert_se(sd_event_source_set_io_fd(s, pfd_b[0]) >= 0); + TAKE_FD(pfd_b[0]); +} + static int hup_callback(sd_event_source *s, int fd, uint32_t revents, void *userdata) { unsigned *c = userdata; -- 2.39.5