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

[David Henningsson]

It looks like this could cause a problem in case somebody (ab)uses the 
API like:

pa_module_unload_request(m, true);
pa_module_unload(c, m, true);

...later on, the deferred event triggers, and modules_pending_unload now 
has dangling pointers.

I'm not sure this is ever encountered in the current code base, but now 
we're no longer resilient to this type of (ab)use.

On 2014-12-05 14:56, Tanu Kaskinen wrote:
> 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.
> ---
>   src/pulsecore/core.c   | 5 +++++
>   src/pulsecore/core.h   | 1 +
>   src/pulsecore/module.c | 7 +++----
>   3 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/src/pulsecore/core.c b/src/pulsecore/core.c
> index e461963..258ee2a 100644
> --- a/src/pulsecore/core.c
> +++ b/src/pulsecore/core.c
> @@ -116,6 +116,8 @@ pa_core* pa_core_new(pa_mainloop_api *m, bool shared, size_t shm_size) {
>       c->deferred_volume_safety_margin_usec = 8000;
>       c->deferred_volume_extra_delay_usec = 0;
>
> +    c->modules_pending_unload = pa_hashmap_new(NULL, NULL);
> +
>       c->module_defer_unload_event = NULL;
>       c->scache_auto_unload_event = NULL;
>
> @@ -204,6 +206,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..ce87f84 100644
> --- a/src/pulsecore/module.c
> +++ b/src/pulsecore/module.c
> @@ -303,16 +303,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_steal_first(c->modules_pending_unload)))
> +        pa_module_unload(c, m, true);
>   }
>
>   void pa_module_unload_request(pa_module *m, bool force) {
> @@ -322,6 +320,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);
>

-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic