[pulseaudio-discuss] [PATCH] module-loopback: Reset callbacks in teardown

[Tanu Kaskinen]

On Wed, 2012-04-04 at 08:10 +0200, David Henningsson wrote:
> On 04/04/2012 07:20 AM, Tanu Kaskinen wrote:
> > On Tue, 2012-04-03 at 19:03 +0200, David Henningsson wrote:
> >> Make sure we can't be called into by remaining references to
> >> sink-inputs and source-outputs after we have teared down, as
> >> that will likely lead to segfaults or assertion failures.
> >>
> >> Signed-off-by: David Henningsson<david.henningsson at canonical.com>
> >> ---
> >>
> >> This is a continuation of the "loopback: Fix crash if max request change at
> >> unload" thread. Not sure if this makes sense, it probably does, but I have not
> >> verified how the core will react when all of these are NULL, so won't push without
> >> additional comments/acks.
> >>
> >> Frederic, it would be nice to know if this improves your situation as well.
> >
> > pa_sink_input_unlink() already resets the callbacks.
> 
> Ok, that's good. However, looking at it a bit closer, the most important 
> one is not reset in the reset_callbacks:
> 
> u->sink_input->parent.process_msg = NULL;

pa_asyncmsgq_dispatch() would segfault.

> or, perhaps even better:
> 
> u->sink_input->parent.process_msg = pa_sink_input_process_msg;

This might be a viable fix. pa_sink_input_process_msg() will return
-PA_ERR_NOTIMPLEMENTED, but that probably doesn't cause any trouble.
asyncmsgq_read_cb() in thread-mq.c doesn't seem to do anything drastic
on failures.

> Perhaps it would make sense to put the above into teardown()?
> 
> > Did my suggestion
> > of a one-line fix get any consideration?
> > http://lists.freedesktop.org/archives/pulseaudio-discuss/2012-March/013091.html
> 
> Hmm, I don't understand how your suggestion could make a difference - at 
> least now with the patch I pushed, adjust_time is set to zero at the 
> same time as the sink is unlinked.

To me the problem seems to be that the process_msg callback is called
after unloading module-loopback. At that point userdata has been freed,
so it's not safe to read u->adjust_time. The pa_sink_input object,
however, is still alive (but unlinked), because the message queue holds
a reference to it.

Checking whether the input is unlinked in process_msg should get rid of
this problem. But so should your suggestion of changing the process_msg
function to pa_sink_input_process_msg. Which fix would be better? (I
would prefer my fix, but that may be only due to bias.)

-- 
Tanu