[pulseaudio-discuss] [PATCH v2] module: Fix unsafe iteration

[Tanu Kaskinen]

This fixes an issue when requesting module unload for
module-bluetooth-discover. When unloading the module, it also unloads
module-bluez4-discover and/or module-bluez5-discover, and that
invalidated the state variable that was used for iterating through the
modules idxset.

The pa_module.unload_requested flag could now otherwise be removed,
but it's still being (ab)used in the bluetooth modules.
---

Changes in v2:
 * Remove modules from pa_core.modules_pending_unload in
   pa_module_unload() instead of defer_cb(). This ensures that
   modules_pending_unload gets emptied when shutting down.
 * Align pa_core_new() better with the member ordering of pa_core.


 src/pulsecore/core.c   | 6 +++++-
 src/pulsecore/core.h   | 1 +
 src/pulsecore/module.c | 9 +++++----
 3 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/src/pulsecore/core.c b/src/pulsecore/core.c
index e461963..c6fa8d7 100644
--- a/src/pulsecore/core.c
+++ b/src/pulsecore/core.c
@@ -117,7 +117,7 @@ pa_core* pa_core_new(pa_mainloop_api *m, bool shared, size_t shm_size) {
     c->deferred_volume_extra_delay_usec = 0;
 
     c->module_defer_unload_event = NULL;
-    c->scache_auto_unload_event = NULL;
+    c->modules_pending_unload = pa_hashmap_new(NULL, NULL);
 
     c->subscription_defer_event = NULL;
     PA_LLIST_HEAD_INIT(pa_subscription, c->subscriptions);
@@ -133,6 +133,7 @@ pa_core* pa_core_new(pa_mainloop_api *m, bool shared, size_t shm_size) {
         pa_mempool_set_is_remote_writable(c->rw_mempool, true);
 
     c->exit_event = NULL;
+    c->scache_auto_unload_event = NULL;
 
     c->exit_idle_time = -1;
     c->scache_idle_time = 20;
@@ -204,6 +205,9 @@ static void core_free(pa_object *o) {
     pa_assert(pa_hashmap_isempty(c->shared));
     pa_hashmap_free(c->shared);
 
+    pa_assert(pa_hashmap_isempty(c->modules_pending_unload));
+    pa_hashmap_free(c->modules_pending_unload);
+
     pa_subscription_free_all(c);
 
     if (c->exit_event)
diff --git a/src/pulsecore/core.h b/src/pulsecore/core.h
index 1f9df73..b0d1211 100644
--- a/src/pulsecore/core.h
+++ b/src/pulsecore/core.h
@@ -164,6 +164,7 @@ struct pa_core {
     int deferred_volume_extra_delay_usec;
 
     pa_defer_event *module_defer_unload_event;
+    pa_hashmap *modules_pending_unload; /* pa_module -> pa_module (hashmap-as-a-set) */
 
     pa_defer_event *subscription_defer_event;
     PA_LLIST_HEAD(pa_subscription, subscriptions);
diff --git a/src/pulsecore/module.c b/src/pulsecore/module.c
index bee8a20..4974034 100644
--- a/src/pulsecore/module.c
+++ b/src/pulsecore/module.c
@@ -247,6 +247,8 @@ void pa_module_unload(pa_core *c, pa_module *m, bool force) {
     if (m->core->disallow_module_loading && !force)
         return;
 
+    pa_hashmap_remove(c->modules_pending_unload, m);
+
     if (!(m = pa_idxset_remove_by_data(c->modules, m, NULL)))
         return;
 
@@ -303,16 +305,14 @@ void pa_module_unload_all(pa_core *c) {
 }
 
 static void defer_cb(pa_mainloop_api*api, pa_defer_event *e, void *userdata) {
-    void *state = NULL;
     pa_core *c = PA_CORE(userdata);
     pa_module *m;
 
     pa_core_assert_ref(c);
     api->defer_enable(e, 0);
 
-    while ((m = pa_idxset_iterate(c->modules, &state, NULL)))
-        if (m->unload_requested)
-            pa_module_unload(c, m, true);
+    while ((m = pa_hashmap_first(c->modules_pending_unload)))
+        pa_module_unload(c, m, true);
 }
 
 void pa_module_unload_request(pa_module *m, bool force) {
@@ -322,6 +322,7 @@ void pa_module_unload_request(pa_module *m, bool force) {
         return;
 
     m->unload_requested = true;
+    pa_hashmap_put(m->core->modules_pending_unload, m, m);
 
     if (!m->core->module_defer_unload_event)
         m->core->module_defer_unload_event = m->core->mainloop->defer_new(m->core->mainloop, defer_cb, m->core);
-- 
1.9.3