[pulseaudio-discuss] [PATCH v5 39/39] bluetooth: Revive module-bluetooth-discover

[Tanu Kaskinen]

On Wed, 2013-10-09 at 14:41 -0300, João Paulo Rechi Vita wrote:
> On Wed, Oct 9, 2013 at 12:26 PM, João Paulo Rechi Vita
> <jprvita at gmail.com> wrote:
> > On Tue, Oct 8, 2013 at 12:15 PM, Tanu Kaskinen
> > <tanu.kaskinen at linux.intel.com> wrote:
> >> On Tue, 2013-10-08 at 11:19 -0300, João Paulo Rechi Vita wrote:
> >>> On Sun, Sep 29, 2013 at 1:39 PM, Tanu Kaskinen
> >>> <tanu.kaskinen at linux.intel.com> wrote:
> >>> > On Tue, 2013-09-24 at 19:45 -0300, jprvita at gmail.com wrote:
> >>> >> +void pa__done(pa_module* m) {
> >>> >> +    struct userdata *u;
> >>> >> +
> >>> >> +    pa_assert(m);
> >>> >> +
> >>> >> +    if (!(u = m->userdata))
> >>> >> +        return;
> >>> >> +
> >>> >> +    if (u->bluez5_module)
> >>> >> +        pa_module_unload(m->core, u->bluez5_module, true);
> >>> >> +
> >>> >> +    if (u->bluez4_module)
> >>> >> +        pa_module_unload(m->core, u->bluez4_module, true);
> >>> >
> >>> > This crashes when shutting down the daemon, because when the daemon
> >>> > unloads all modules, module-bluez*-discover gets unloaded before
> >>> > module-bluetooth-discover, so the y->bluez5_module and u->bluez4_module
> >>> > pointers become stale. I see two ways of fixing this: add a hook that is
> >>> > fired when modules are unloaded and use that hook in
> >>> > module-bluetooth-discover to drop the reference to the unloaded module,
> >>> > or unload module-bluetooth-discover immediately after loading
> >>> > module-bluez5-discover and module-bluez4-discover. The second solution
> >>> > is of course much simpler, but I proposed that already earlier, and you
> >>> > didn't like that.
> >>> >
> >>>
> >>> It doesn't crash (and that's what I'm experiencing here) because
> >>> pa_module_unload() will look for that module pointer in its internal
> >>> hash of modules before trying to unload it. I agree we are left with a
> >>> stale pointer, but as long as we don't dereference it, we should be
> >>> fine.
> >>
> >> pa_module_unload() dereferences the pointer already before
> >>
> >>     if (!(m = pa_idxset_remove_by_data(c->modules, m, NULL)))
> >>         return;
> >>
> >> which I think you are referring to as the safeguard. The line that
> >> crashes is this:
> >>
> >>     if (m->core->disallow_module_loading && !force)
> >>
> 
> Hum, another thing came to my mind: pa_module_unload() also receives
> the pa_core pointer through its arguments, why not use it in the check
> for disallow_module_loading? Like this:
> 
> if (c->disallow_module_loading && !force)

That still doesn't fix the problem properly. The idxset in this call

    pa_idxset_remove_by_data(c->modules, m, NULL)

may (in theory) contain another module with the same pointer as the
stale pointer m. This is highly unlikely, but anyway, passing stale
pointers around is horribly ugly. Never do that.

-- 
Tanu