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 d444b0bf00ca2ea220ecf2afb1fe829edb90d6c0..7de5a37289c65cad201a645a7ac9050cff8ebaf4 100644 (file)
--- 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 df13df6b50680b62ff254a016b12338be7cea6af..fabbdbb88c9f9770adb75b0d2630548f5d6a8214 100644 (file)
--- 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 c1b98c52d1da329ce4b9bcdb4a0963a88992a9b6..4e18dd540475f8453f785c6a486947430891c8c8 100644 (file)
--- 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);