Merge pull request #21270 from poettering/event-mem-corruption

sd-event: fix memory corruption

13 files changed, 199 insertions, 74 deletions

diff --git a/man/rules/meson.build b/man/rules/meson.build
index ad7cc41c98..7bf93fc16b 100644
--- a/man/rules/meson.build
+++ b/man/rules/meson.build
@@ -518,7 +518,9 @@ manpages = [
   ''],
  ['sd_event_add_inotify',
   '3',
-  ['sd_event_inotify_handler_t', 'sd_event_source_get_inotify_mask'],
+  ['sd_event_add_inotify_fd',
+   'sd_event_inotify_handler_t',
+   'sd_event_source_get_inotify_mask'],
   ''],
  ['sd_event_add_io',
   '3',
diff --git a/man/sd_event_add_inotify.xml b/man/sd_event_add_inotify.xml
index 1681143eb1..d632bf7282 100644
--- a/man/sd_event_add_inotify.xml
+++ b/man/sd_event_add_inotify.xml
@@ -17,6 +17,7 @@
 
   <refnamediv>
     <refname>sd_event_add_inotify</refname>
+    <refname>sd_event_add_inotify_fd</refname>
     <refname>sd_event_source_get_inotify_mask</refname>
     <refname>sd_event_inotify_handler_t</refname>
 
@@ -47,6 +48,16 @@
       </funcprototype>
 
       <funcprototype>
+        <funcdef>int <function>sd_event_add_inotify_fd</function></funcdef>
+        <paramdef>sd_event *<parameter>event</parameter></paramdef>
+        <paramdef>sd_event_source **<parameter>source</parameter></paramdef>
+        <paramdef>int <parameter>fd</parameter></paramdef>
+        <paramdef>uint32_t <parameter>mask</parameter></paramdef>
+        <paramdef>sd_event_inotify_handler_t <parameter>handler</parameter></paramdef>
+        <paramdef>void *<parameter>userdata</parameter></paramdef>
+      </funcprototype>
+
+      <funcprototype>
         <funcdef>int <function>sd_event_source_get_inotify_mask</function></funcdef>
         <paramdef>sd_event_source *<parameter>source</parameter></paramdef>
         <paramdef>uint32_t *<parameter>mask</parameter></paramdef>
@@ -71,6 +82,11 @@
     <citerefentry project='man-pages'><refentrytitle>inotify</refentrytitle><manvolnum>7</manvolnum></citerefentry> for
     further information.</para>
 
+    <para><function>sd_event_add_inotify_fd()</function> is identical to
+    <function>sd_event_add_inotify()</function>, except that it takes a file descriptor to an inode (possibly
+    an <constant>O_PATH</constant> one, but any other will do too) instead of a path in the file
+    system.</para>
+
     <para>If multiple event sources are installed for the same inode the backing inotify watch descriptor is
     automatically shared. The mask parameter may contain any flag defined by the inotify API, with the exception of
     <constant>IN_MASK_ADD</constant>.</para>
@@ -157,6 +173,19 @@
           <listitem><para>The passed event source is not an inotify process event source.</para></listitem>
         </varlistentry>
 
+        <varlistentry>
+          <term><constant>-EBADF</constant></term>
+
+          <listitem><para>The passed file descriptor is not valid.</para></listitem>
+        </varlistentry>
+
+        <varlistentry>
+          <term><constant>-ENOSYS</constant></term>
+
+          <listitem><para><function>sd_event_add_inotify_fd()</function> was called without
+          <filename>/proc/</filename> mounted.</para></listitem>
+        </varlistentry>
+
       </variablelist>
     </refsect2>
   </refsect1>
diff --git a/src/basic/inotify-util.c b/src/basic/inotify-util.c
index 848f8590fa..6da974dec0 100644
--- a/src/basic/inotify-util.c
+++ b/src/basic/inotify-util.c
@@ -2,14 +2,26 @@
 
 #include "fd-util.h"
 #include "inotify-util.h"
+#include "stat-util.h"
 
 int inotify_add_watch_fd(int fd, int what, uint32_t mask) {
-        int wd;
+        int wd, r;
 
         /* This is like inotify_add_watch(), except that the file to watch is not referenced by a path, but by an fd */
         wd = inotify_add_watch(fd, FORMAT_PROC_FD_PATH(what), mask);
-        if (wd < 0)
-                return -errno;
+        if (wd < 0) {
+                if (errno != ENOENT)
+                        return -errno;
+
+                /* Didn't work with ENOENT? If so, then either /proc/ isn't mounted, or the fd is bad */
+                r = proc_mounted();
+                if (r == 0)
+                        return -ENOSYS;
+                if (r > 0)
+                        return -EBADF;
+
+                return -ENOENT; /* OK, no clue, let's propagate the original error */
+        }
 
         return wd;
 }
diff --git a/src/journal/journald-stream.c b/src/journal/journald-stream.c
index cbff5036a4..26909840da 100644
--- a/src/journal/journald-stream.c
+++ b/src/journal/journald-stream.c
@@ -109,7 +109,6 @@ StdoutStream* stdout_stream_free(StdoutStream *s) {
                 return NULL;
 
         if (s->server) {
-
                 if (s->context)
                         client_context_release(s->server, s->context);
 
@@ -123,11 +122,7 @@ StdoutStream* stdout_stream_free(StdoutStream *s) {
                 (void) server_start_or_stop_idle_timer(s->server); /* Maybe we are idle now? */
         }
 
-        if (s->event_source) {
-                sd_event_source_set_enabled(s->event_source, SD_EVENT_OFF);
-                s->event_source = sd_event_source_unref(s->event_source);
-        }
-
+        sd_event_source_disable_unref(s->event_source);
         safe_close(s->fd);
         free(s->label);
         free(s->identifier);
diff --git a/src/libsystemd/libsystemd.sym b/src/libsystemd/libsystemd.sym
index 04db791f64..5e2fc9e231 100644
--- a/src/libsystemd/libsystemd.sym
+++ b/src/libsystemd/libsystemd.sym
@@ -766,4 +766,5 @@ global:
 LIBSYSTEMD_250 {
 global:
         sd_device_get_diskseq;
+        sd_event_add_inotify_fd;
 } LIBSYSTEMD_249;
diff --git a/src/libsystemd/sd-bus/sd-bus.c b/src/libsystemd/sd-bus/sd-bus.c
index 9ebf7b03c0..968b37e826 100644
--- a/src/libsystemd/sd-bus/sd-bus.c
+++ b/src/libsystemd/sd-bus/sd-bus.c
@@ -63,7 +63,6 @@
 
 static int bus_poll(sd_bus *bus, bool need_more, uint64_t timeout_usec);
 static void bus_detach_io_events(sd_bus *b);
-static void bus_detach_inotify_event(sd_bus *b);
 
 static thread_local sd_bus *default_system_bus = NULL;
 static thread_local sd_bus *default_user_bus = NULL;
@@ -140,7 +139,7 @@ void bus_close_io_fds(sd_bus *b) {
 void bus_close_inotify_fd(sd_bus *b) {
         assert(b);
 
-        bus_detach_inotify_event(b);
+        b->inotify_event_source = sd_event_source_disable_unref(b->inotify_event_source);
 
         b->inotify_fd = safe_close(b->inotify_fd);
         b->inotify_watches = mfree(b->inotify_watches);
@@ -3747,15 +3746,8 @@ int bus_attach_io_events(sd_bus *bus) {
 static void bus_detach_io_events(sd_bus *bus) {
         assert(bus);
 
-        if (bus->input_io_event_source) {
-                sd_event_source_set_enabled(bus->input_io_event_source, SD_EVENT_OFF);
-                bus->input_io_event_source = sd_event_source_unref(bus->input_io_event_source);
-        }
-
-        if (bus->output_io_event_source) {
-                sd_event_source_set_enabled(bus->output_io_event_source, SD_EVENT_OFF);
-                bus->output_io_event_source = sd_event_source_unref(bus->output_io_event_source);
-        }
+        bus->input_io_event_source = sd_event_source_disable_unref(bus->input_io_event_source);
+        bus->output_io_event_source = sd_event_source_disable_unref(bus->output_io_event_source);
 }
 
 int bus_attach_inotify_event(sd_bus *bus) {
@@ -3787,15 +3779,6 @@ int bus_attach_inotify_event(sd_bus *bus) {
         return 0;
 }
 
-static void bus_detach_inotify_event(sd_bus *bus) {
-        assert(bus);
-
-        if (bus->inotify_event_source) {
-                sd_event_source_set_enabled(bus->inotify_event_source, SD_EVENT_OFF);
-                bus->inotify_event_source = sd_event_source_unref(bus->inotify_event_source);
-        }
-}
-
 _public_ int sd_bus_attach_event(sd_bus *bus, sd_event *event, int priority) {
         int r;
 
@@ -3860,17 +3843,9 @@ _public_ int sd_bus_detach_event(sd_bus *bus) {
                 return 0;
 
         bus_detach_io_events(bus);
-        bus_detach_inotify_event(bus);
-
-        if (bus->time_event_source) {
-                sd_event_source_set_enabled(bus->time_event_source, SD_EVENT_OFF);
-                bus->time_event_source = sd_event_source_unref(bus->time_event_source);
-        }
-
-        if (bus->quit_event_source) {
-                sd_event_source_set_enabled(bus->quit_event_source, SD_EVENT_OFF);
-                bus->quit_event_source = sd_event_source_unref(bus->quit_event_source);
-        }
+        bus->inotify_event_source = sd_event_source_disable_unref(bus->inotify_event_source);
+        bus->time_event_source = sd_event_source_disable_unref(bus->time_event_source);
+        bus->quit_event_source = sd_event_source_disable_unref(bus->quit_event_source);
 
         bus->event = sd_event_unref(bus->event);
         return 1;
diff --git a/src/libsystemd/sd-event/event-source.h b/src/libsystemd/sd-event/event-source.h
index 08b409fef1..41845c0bb5 100644
--- a/src/libsystemd/sd-event/event-source.h
+++ b/src/libsystemd/sd-event/event-source.h
@@ -214,6 +214,11 @@ struct inotify_data {
          * the events locally if they can't be coalesced). */
         unsigned n_pending;
 
+        /* If this counter is non-zero, don't GC the inotify data object even if not used to watch any inode
+         * anymore. This is useful to pin the object for a bit longer, after the last event source needing it
+         * is gone. */
+        unsigned n_busy;
+
         /* A linked list of all inotify objects with data already read, that still need processing. We keep this list
          * to make it efficient to figure out what inotify objects to process data on next. */
         LIST_FIELDS(struct inotify_data, buffered);
diff --git a/src/libsystemd/sd-event/sd-event.c b/src/libsystemd/sd-event/sd-event.c
index b06e00fae2..0ca0248510 100644
--- a/src/libsystemd/sd-event/sd-event.c
+++ b/src/libsystemd/sd-event/sd-event.c
@@ -1820,6 +1820,29 @@ static void event_free_inode_data(
         free(d);
 }
 
+static void event_gc_inotify_data(
+                sd_event *e,
+                struct inotify_data *d) {
+
+        assert(e);
+
+        /* GCs the inotify data object if we don't need it anymore. That's the case if we don't want to watch
+         * any inode with it anymore, which in turn happens if no event source of this priority is interested
+         * in any inode any longer. That said, we maintain an extra busy counter: if non-zero we'll delay GC
+         * (under the expectation that the GC is called again once the counter is decremented). */
+
+        if (!d)
+                return;
+
+        if (!hashmap_isempty(d->inodes))
+                return;
+
+        if (d->n_busy > 0)
+                return;
+
+        event_free_inotify_data(e, d);
+}
+
 static void event_gc_inode_data(
                 sd_event *e,
                 struct inode_data *d) {
@@ -1837,8 +1860,7 @@ static void event_gc_inode_data(
         inotify_data = d->inotify_data;
         event_free_inode_data(e, d);
 
-        if (inotify_data && hashmap_isempty(inotify_data->inodes))
-                event_free_inotify_data(e, inotify_data);
+        event_gc_inotify_data(e, inotify_data);
 }
 
 static int event_make_inode_data(
@@ -1967,24 +1989,25 @@ static int inotify_exit_callback(sd_event_source *s, const struct inotify_event
         return sd_event_exit(sd_event_source_get_event(s), PTR_TO_INT(userdata));
 }
 
-_public_ int sd_event_add_inotify(
+static int event_add_inotify_fd_internal(
                 sd_event *e,
                 sd_event_source **ret,
-                const char *path,
+                int fd,
+                bool donate,
                 uint32_t mask,
                 sd_event_inotify_handler_t callback,
                 void *userdata) {
 
+        _cleanup_close_ int donated_fd = donate ? fd : -1;
+        _cleanup_(source_freep) sd_event_source *s = NULL;
         struct inotify_data *inotify_data = NULL;
         struct inode_data *inode_data = NULL;
-        _cleanup_close_ int fd = -1;
-        _cleanup_(source_freep) sd_event_source *s = NULL;
         struct stat st;
         int r;
 
         assert_return(e, -EINVAL);
         assert_return(e = event_resolve(e), -ENOPKG);
-        assert_return(path, -EINVAL);
+        assert_return(fd >= 0, -EBADF);
         assert_return(e->state != SD_EVENT_FINISHED, -ESTALE);
         assert_return(!event_pid_changed(e), -ECHILD);
 
@@ -1997,12 +2020,6 @@ _public_ int sd_event_add_inotify(
         if (mask & IN_MASK_ADD)
                 return -EINVAL;
 
-        fd = open(path, O_PATH|O_CLOEXEC|
-                  (mask & IN_ONLYDIR ? O_DIRECTORY : 0)|
-                  (mask & IN_DONT_FOLLOW ? O_NOFOLLOW : 0));
-        if (fd < 0)
-                return -errno;
-
         if (fstat(fd, &st) < 0)
                 return -errno;
 
@@ -2022,14 +2039,24 @@ _public_ int sd_event_add_inotify(
 
         r = event_make_inode_data(e, inotify_data, st.st_dev, st.st_ino, &inode_data);
         if (r < 0) {
-                event_free_inotify_data(e, inotify_data);
+                event_gc_inotify_data(e, inotify_data);
                 return r;
         }
 
         /* Keep the O_PATH fd around until the first iteration of the loop, so that we can still change the priority of
          * the event source, until then, for which we need the original inode. */
         if (inode_data->fd < 0) {
-                inode_data->fd = TAKE_FD(fd);
+                if (donated_fd >= 0)
+                        inode_data->fd = TAKE_FD(donated_fd);
+                else {
+                        inode_data->fd = fcntl(fd, F_DUPFD_CLOEXEC, 3);
+                        if (inode_data->fd < 0) {
+                                r = -errno;
+                                event_gc_inode_data(e, inode_data);
+                                return r;
+                        }
+                }
+
                 LIST_PREPEND(to_close, e->inode_data_to_close, inode_data);
         }
 
@@ -2042,8 +2069,6 @@ _public_ int sd_event_add_inotify(
         if (r < 0)
                 return r;
 
-        (void) sd_event_source_set_description(s, path);
-
         if (ret)
                 *ret = s;
         TAKE_PTR(s);
@@ -2051,6 +2076,48 @@ _public_ int sd_event_add_inotify(
         return 0;
 }
 
+_public_ int sd_event_add_inotify_fd(
+                sd_event *e,
+                sd_event_source **ret,
+                int fd,
+                uint32_t mask,
+                sd_event_inotify_handler_t callback,
+                void *userdata) {
+
+        return event_add_inotify_fd_internal(e, ret, fd, /* donate= */ false, mask, callback, userdata);
+}
+
+_public_ int sd_event_add_inotify(
+                sd_event *e,
+                sd_event_source **ret,
+                const char *path,
+                uint32_t mask,
+                sd_event_inotify_handler_t callback,
+                void *userdata) {
+
+        sd_event_source *s;
+        int fd, r;
+
+        assert_return(path, -EINVAL);
+
+        fd = open(path, O_PATH|O_CLOEXEC|
+                  (mask & IN_ONLYDIR ? O_DIRECTORY : 0)|
+                  (mask & IN_DONT_FOLLOW ? O_NOFOLLOW : 0));
+        if (fd < 0)
+                return -errno;
+
+        r = event_add_inotify_fd_internal(e, &s, fd, /* donate= */ true, mask, callback, userdata);
+        if (r < 0)
+                return r;
+
+        (void) sd_event_source_set_description(s, path);
+
+        if (ret)
+                *ret = s;
+
+        return r;
+}
+
 static sd_event_source* event_source_free(sd_event_source *s) {
         if (!s)
                 return NULL;
@@ -3556,13 +3623,23 @@ static int source_dispatch(sd_event_source *s) {
                 sz = offsetof(struct inotify_event, name) + d->buffer.ev.len;
                 assert(d->buffer_filled >= sz);
 
+                /* If the inotify callback destroys the event source then this likely means we don't need to
+                 * watch the inode anymore, and thus also won't need the inotify object anymore. But if we'd
+                 * free it immediately, then we couldn't drop the event from the inotify event queue without
+                 * memory corruption anymore, as below. Hence, let's not free it immediately, but mark it
+                 * "busy" with a counter (which will ensure it's not GC'ed away prematurely). Let's then
+                 * explicitly GC it after we are done dropping the inotify event from the buffer. */
+                d->n_busy++;
                 r = s->inotify.callback(s, &d->buffer.ev, s->userdata);
+                d->n_busy--;
 
-                /* When no event is pending anymore on this inotify object, then let's drop the event from the
-                 * buffer. */
+                /* When no event is pending anymore on this inotify object, then let's drop the event from
+                 * the inotify event queue buffer. */
                 if (d->n_pending == 0)
                         event_inotify_data_drop(e, d, sz);
 
+                /* Now we don't want to access 'd' anymore, it's OK to GC now. */
+                event_gc_inotify_data(e, d);
                 break;
         }
 
diff --git a/src/libsystemd/sd-event/test-event.c b/src/libsystemd/sd-event/test-event.c
index 406d10d92b..b637639f26 100644
--- a/src/libsystemd/sd-event/test-event.c
+++ b/src/libsystemd/sd-event/test-event.c
@@ -713,6 +713,40 @@ static void test_simple_timeout(void) {
         assert_se(t >= usec_add(f, some_time));
 }
 
+static int inotify_self_destroy_handler(sd_event_source *s, const struct inotify_event *ev, void *userdata) {
+        sd_event_source **p = userdata;
+
+        assert_se(ev);
+        assert_se(p);
+        assert_se(*p == s);
+
+        assert_se(FLAGS_SET(ev->mask, IN_ATTRIB));
+
+        assert_se(sd_event_exit(sd_event_source_get_event(s), 0) >= 0);
+
+        *p = sd_event_source_unref(*p); /* here's what we actually intend to test: we destroy the event
+                                         * source from inside the event source handler */
+        return 1;
+}
+
+static void test_inotify_self_destroy(void) {
+        _cleanup_(sd_event_source_unrefp) sd_event_source *s = NULL;
+        _cleanup_(sd_event_unrefp) sd_event *e = NULL;
+        char path[] = "/tmp/inotifyXXXXXX";
+        _cleanup_close_ int fd = -1;
+
+        /* Tests that destroying an inotify event source from its own handler is safe */
+
+        assert_se(sd_event_default(&e) >= 0);
+
+        fd = mkostemp_safe(path);
+        assert_se(fd >= 0);
+        assert_se(sd_event_add_inotify_fd(e, &s, fd, IN_ATTRIB, inotify_self_destroy_handler, &s) >= 0);
+        fd = safe_close(fd);
+        assert_se(unlink(path) >= 0); /* This will trigger IN_ATTRIB because link count goes to zero */
+        assert_se(sd_event_loop(e) >= 0);
+}
+
 int main(int argc, char *argv[]) {
         test_setup_logging(LOG_DEBUG);
 
@@ -731,5 +765,7 @@ int main(int argc, char *argv[]) {
 
         test_ratelimit();
 
+        test_inotify_self_destroy();
+
         return 0;
 }
diff --git a/src/libsystemd/sd-resolve/sd-resolve.c b/src/libsystemd/sd-resolve/sd-resolve.c
index c8af16a918..ad637bcf1d 100644
--- a/src/libsystemd/sd-resolve/sd-resolve.c
+++ b/src/libsystemd/sd-resolve/sd-resolve.c
@@ -1285,11 +1285,7 @@ _public_  int sd_resolve_detach_event(sd_resolve *resolve) {
         if (!resolve->event)
                 return 0;
 
-        if (resolve->event_source) {
-                sd_event_source_set_enabled(resolve->event_source, SD_EVENT_OFF);
-                resolve->event_source = sd_event_source_unref(resolve->event_source);
-        }
-
+        resolve->event_source = sd_event_source_disable_unref(resolve->event_source);
         resolve->event = sd_event_unref(resolve->event);
         return 1;
 }
diff --git a/src/login/logind-core.c b/src/login/logind-core.c
index 57d2d64e49..254a1a69fb 100644
--- a/src/login/logind-core.c
+++ b/src/login/logind-core.c
@@ -721,7 +721,9 @@ int manager_read_utmp(Manager *m) {
                 errno = 0;
                 u = getutxent();
                 if (!u) {
-                        if (errno != 0)
+                        if (errno == ENOENT)
+                                log_debug_errno(errno, _PATH_UTMPX " does not exist, ignoring.");
+                        else if (errno != 0)
                                 log_warning_errno(errno, "Failed to read " _PATH_UTMPX ", ignoring: %m");
                         return 0;
                 }
diff --git a/src/shared/varlink.c b/src/shared/varlink.c
index b31ca108fe..e0038dfd28 100644
--- a/src/shared/varlink.c
+++ b/src/shared/varlink.c
@@ -2365,14 +2365,8 @@ int varlink_server_detach_event(VarlinkServer *s) {
 
         assert_return(s, -EINVAL);
 
-        LIST_FOREACH(sockets, ss, s->sockets) {
-
-                if (!ss->event_source)
-                        continue;
-
-                (void) sd_event_source_set_enabled(ss->event_source, SD_EVENT_OFF);
-                ss->event_source = sd_event_source_unref(ss->event_source);
-        }
+        LIST_FOREACH(sockets, ss, s->sockets)
+                ss->event_source = sd_event_source_disable_unref(ss->event_source);
 
         sd_event_unref(s->event);
         return 0;
diff --git a/src/systemd/sd-event.h b/src/systemd/sd-event.h
index 2ae2a0da48..f4357aa593 100644
--- a/src/systemd/sd-event.h
+++ b/src/systemd/sd-event.h
@@ -93,6 +93,7 @@ int sd_event_add_signal(sd_event *e, sd_event_source **s, int sig, sd_event_sign
 int sd_event_add_child(sd_event *e, sd_event_source **s, pid_t pid, int options, sd_event_child_handler_t callback, void *userdata);
 int sd_event_add_child_pidfd(sd_event *e, sd_event_source **s, int pidfd, int options, sd_event_child_handler_t callback, void *userdata);
 int sd_event_add_inotify(sd_event *e, sd_event_source **s, const char *path, uint32_t mask, sd_event_inotify_handler_t callback, void *userdata);
+int sd_event_add_inotify_fd(sd_event *e, sd_event_source **s, int fd, uint32_t mask, sd_event_inotify_handler_t callback, void *userdata);
 int sd_event_add_defer(sd_event *e, sd_event_source **s, sd_event_handler_t callback, void *userdata);
 int sd_event_add_post(sd_event *e, sd_event_source **s, sd_event_handler_t callback, void *userdata);
 int sd_event_add_exit(sd_event *e, sd_event_source **s, sd_event_handler_t callback, void *userdata);