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? -- Jo?o Paulo Rechi Vita http://about.me/jprvita