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

[João Paulo Rechi Vita]

On Mon, Oct 14, 2013 at 5:24 AM, Tanu Kaskinen
<tanu.kaskinen at linux.intel.com> wrote:
> 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.
>

Yes, I wasn't proposing it as a fix, I was just wondering why we don't
use the pa_core pointer passed in the arguments.

-- 
João Paulo Rechi Vita
http://about.me/jprvita