From: Luca Boccassi Date: Thu, 2 Jul 2026 19:55:53 +0000 (+0100) Subject: udev: clear event back-pointer when freeing a worker X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=7eb7d46f9750d3baf9eaa9f3edb64d5c00fc25cf;p=thirdparty%2Fsystemd.git udev: clear event back-pointer when freeing a worker worker_free() did not clear worker->event->worker, leaving an event that was still attached to a worker pointing at freed memory once the worker was gone. manager_free() frees the workers (hashmap_free()) before it walks manager->events to process them, and event_free() then writes event->worker->event = NULL into the already-freed worker. Clear the back-pointer in worker_free(), mirroring event_free() which already clears the worker's pointer. The link used to be torn down by the event_free(worker->event) call in worker_free(), which was dropped when events became reference counted. The Event and Worker definitions move to udev-manager.h (next to Manager) so the unit test can construct and free a bound worker/event pair. Follow-up for cb16d47f30cc0d848ca6868c39b844810520d713 --- diff --git a/src/udev/test-udev-manager.c b/src/udev/test-udev-manager.c index d444b0bf00c..7de5a37289c 100644 --- a/src/udev/test-udev-manager.c +++ b/src/udev/test-udev-manager.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ +#include "alloc-util.h" #include "tests.h" #include "udev-manager.h" @@ -16,4 +17,22 @@ TEST(devpath_conflict) { "/devices/pci0000:00/0000:00:1c.4/0000:3c:00.0/nvme/nvme0/nvme0n1/nvme0n1p1")); } +TEST(worker_free_detaches_event) { + /* worker_free() must clear the back-pointer of the event it is processing, otherwise the event is + * left referencing the freed worker. This happens e.g. on manager_free(), which frees the workers + * before the events still referencing them. */ + + Worker *worker = ASSERT_PTR(new0(Worker, 1)); + worker->pidref = PIDREF_NULL; + + _cleanup_free_ Event *event = ASSERT_PTR(new0(Event, 1)); + + worker->event = event; + event->worker = worker; + + worker_free(worker); + + ASSERT_NULL(event->worker); +} + DEFINE_TEST_MAIN(LOG_DEBUG); diff --git a/src/udev/udev-manager.c b/src/udev/udev-manager.c index df13df6b506..fabbdbb88c9 100644 --- a/src/udev/udev-manager.c +++ b/src/udev/udev-manager.c @@ -50,63 +50,6 @@ #define EVENT_REQUEUE_INTERVAL_USEC (200 * USEC_PER_MSEC) #define EVENT_REQUEUE_TIMEOUT_USEC (3 * USEC_PER_MINUTE) -typedef enum EventState { - EVENT_UNDEF, - EVENT_QUEUED, - EVENT_RUNNING, - EVENT_LOCKED, - EVENT_PROCESSED, -} EventState; - -typedef struct Event { - /* All events that have not been processed (state != EVENT_PROCESSED) are referenced by the Manager. - * Additionally, an event may be referenced by events blocked by this event. See event_find_blocker(). */ - unsigned n_ref; - - Manager *manager; - Worker *worker; - EventState state; - - sd_device *dev; - - sd_device_action_t action; - uint64_t seqnum; - const char *id; - const char *devpath; - const char *devpath_old; - const char *devnode; - - /* Used when the device is locked by another program. */ - usec_t requeue_next_usec; - usec_t requeue_timeout_usec; - unsigned locked_event_prioq_index; - char *whole_disk; - LIST_FIELDS(Event, same_disk); - - /* The last blocker for this event. This event must not be processed before the blocker is processed. */ - Event *blocker; - - LIST_FIELDS(Event, event); -} Event; - -typedef enum WorkerState { - WORKER_UNDEF, - WORKER_RUNNING, - WORKER_IDLE, - WORKER_KILLED, -} WorkerState; - -typedef struct Worker { - Manager *manager; - PidRef pidref; - sd_event_source *child_event_source; - sd_event_source *timeout_warning_event_source; - sd_event_source *timeout_kill_event_source; - union sockaddr_union address; - WorkerState state; - Event *event; -} Worker; - static void event_unset_whole_disk(Event *event) { Manager *manager = ASSERT_PTR(ASSERT_PTR(event)->manager); @@ -171,13 +114,18 @@ static Event* event_enter_processed(Event *event) { DEFINE_TRIVIAL_CLEANUP_FUNC(Event*, event_enter_processed); -static Worker* worker_free(Worker *worker) { +Worker* worker_free(Worker *worker) { if (!worker) return NULL; if (worker->manager) hashmap_remove(worker->manager->workers, &worker->pidref); + /* If an event is still attached, clear its back-pointer so it doesn't dangle (mirrors event_free(), + * which clears the worker's pointer). */ + if (worker->event) + worker->event->worker = NULL; + sd_event_source_disable_unref(worker->child_event_source); sd_event_source_unref(worker->timeout_warning_event_source); sd_event_source_unref(worker->timeout_kill_event_source); diff --git a/src/udev/udev-manager.h b/src/udev/udev-manager.h index c1b98c52d1d..4e18dd54047 100644 --- a/src/udev/udev-manager.h +++ b/src/udev/udev-manager.h @@ -1,9 +1,12 @@ /* SPDX-License-Identifier: GPL-2.0-or-later */ #pragma once +#include "sd-device.h" #include "sd-event.h" #include "list.h" +#include "pidref.h" +#include "socket-util.h" #include "udev-config.h" #include "udev-forward.h" @@ -76,10 +79,69 @@ typedef struct Manager { bool exit; } Manager; +typedef enum EventState { + EVENT_UNDEF, + EVENT_QUEUED, + EVENT_RUNNING, + EVENT_LOCKED, + EVENT_PROCESSED, +} EventState; + +typedef struct Event { + /* All events that have not been processed (state != EVENT_PROCESSED) are referenced by the Manager. + * Additionally, an event may be referenced by events blocked by this event. See event_find_blocker(). */ + unsigned n_ref; + + Manager *manager; + Worker *worker; + EventState state; + + sd_device *dev; + + sd_device_action_t action; + uint64_t seqnum; + const char *id; + const char *devpath; + const char *devpath_old; + const char *devnode; + + /* Used when the device is locked by another program. */ + usec_t requeue_next_usec; + usec_t requeue_timeout_usec; + unsigned locked_event_prioq_index; + char *whole_disk; + LIST_FIELDS(Event, same_disk); + + /* The last blocker for this event. This event must not be processed before the blocker is processed. */ + Event *blocker; + + LIST_FIELDS(Event, event); +} Event; + +typedef enum WorkerState { + WORKER_UNDEF, + WORKER_RUNNING, + WORKER_IDLE, + WORKER_KILLED, +} WorkerState; + +typedef struct Worker { + Manager *manager; + PidRef pidref; + sd_event_source *child_event_source; + sd_event_source *timeout_warning_event_source; + sd_event_source *timeout_kill_event_source; + union sockaddr_union address; + WorkerState state; + Event *event; +} Worker; + Manager* manager_new(void); Manager* manager_free(Manager *manager); DEFINE_TRIVIAL_CLEANUP_FUNC(Manager*, manager_free); +Worker* worker_free(Worker *worker); + int manager_main(Manager *manager); void manager_reload(Manager *manager, bool force); void manager_revert(Manager *manager);