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

[João Paulo Rechi Vita]

On Mon, Oct 14, 2013 at 5:19 AM, Tanu Kaskinen
<tanu.kaskinen at linux.intel.com> wrote:
> On Wed, 2013-10-09 at 12:26 -0300, João Paulo Rechi Vita 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)
>> >
>> > It doesn't crash always, because m->core is a random address that may or
>> > may not be possible to dereference (it was 0 when I debugged this).
>> >
>> > When this line doesn't crash, I get an assertion about pa_core.shared
>> > not being empty when pa_core is freed. I hope that's another symptom of
>> > this same bug (I looked at the code and the management of pa_core.shared
>> > seemed correct).
>> >
>>
>> I don't get the assertion either. I'm testing with the current master,
>> which has this patch included.
>>
>> Anyway, I guess one solution would be to keep track of
>> module-bluez[45]-discover using the module index instead of the module
>> pointer (assuming indexes are not reused in a same pulseaudio run).
>> This way the caller would pass the pa_core pointer in the call instead
>> of relying on the module pointer. What do you think?
>
> I don't know what you mean by "the caller would pass the pa_core pointer
> in the call instead of relying on the module pointer". I understand your

I meant that pa_module_unload_by_index() receives a pa_core pointer
through its arguments (I haven't paid attention that
pa_module_unload() also does), so the code doesn't dereferences the
pa_module pointer in order to get the core pointer (which is the line
that causes the crash on your setup).

> proposal so that you'd use pa_module_unload_by_index() instead of
> pa_module_unload(). That should work, and I'm fine with that solution.
>

Ok, I'm changing this code to use pa_module_unload_by_index() then.

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