From 53baf2efa420cab6c4b1904c9a0c46a0c4ec80a1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 9 Nov 2021 00:11:38 +0100 Subject: [PATCH] sd-event: don't destroy inotify data structures from inotify event handler This fixes a bad memory access when we destroy an inotify source handler from the handler itself, and thus destroy the associated inotify_data structures. Fixes: #20177 --- src/libsystemd/sd-event/event-source.h | 5 ++++ src/libsystemd/sd-event/sd-event.c | 40 +++++++++++++++++++++++--- 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/src/libsystemd/sd-event/event-source.h b/src/libsystemd/sd-event/event-source.h index 08b409fef18..41845c0bb54 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 b06e00fae27..2a15e686d45 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( @@ -3556,13 +3578,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; }