[pulseaudio-discuss] [PATCH] sink-input, source-output: don't allow moving of streams connected to moving filter devices

[Tanu Kaskinen]

On Mon, 2015-12-21 at 06:55 +0530, Arun Raghavan wrote:
> On 20 December 2015 at 15:48, Tanu Kaskinen <tanuk at iki.fi> wrote:
> > This fixes a crash that was observed in the following scenario:
> > 
> > A phone applications creates playback and recording streams with
> > filter.want=echo-cancel. An echo-cancel sink gets created. The alsa
> > card profile has enabled a 4.0 sink, and the user changes the profile
> > to have a stereo sink instead. A complex sequence of events happens:
> > the echo-cancel sink input gets detached from the alsa sink, the 4.0
> > sink gets removed, which triggers loading of a null sink by
> > module-always-sink, and that in turn triggers rerouting in
> > module-device-manager, which decides to move the phone stream to the
> > null sink. Moving a sink input involves sending a message to the IO
> > thread of the old sink, but in this case the old sink is the
> > echo-cancel sink, which was detached from the now-dead alsa sink.
> > Therefore, the echo-cancel sink doesn't have an IO thread associated
> > with it, and as can be expected, sending a message to a non-existing
> > thread results in a crash.
> > 
> > The crash can be avoided by disallowing moving streams that are
> > connected to filter devices that themselves are being moved. The
> > profile switch still doesn't work smoothly, though, because the
> > echo-cancel streams don't support moving, so when the old alsa sink
> > goes away, module-echo-cancel gets unloaded, and since the
> > echo-cancel sink input got detached before that, the phone sink input
> > isn't movable either and gets killed. But that's a separate issue.
> > 
> > BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=93443
> > ---
> >  src/pulsecore/sink-input.c    | 26 ++++++++++++++++++++++++++
> >  src/pulsecore/source-output.c | 27 +++++++++++++++++++++++++++
> >  2 files changed, 53 insertions(+)
> > 
> > diff --git a/src/pulsecore/sink-input.c b/src/pulsecore/sink-input.c
> > index 539ae17..0e01893 100644
> > --- a/src/pulsecore/sink-input.c
> > +++ b/src/pulsecore/sink-input.c
> > @@ -1528,6 +1528,22 @@ static bool find_filter_sink_input(pa_sink_input *target, pa_sink *s) {
> >      return false;
> >  }
> > 
> > +static bool is_filter_sink_moving(pa_sink_input *i) {
> > +    pa_sink *sink = i->sink;
> > +
> > +    if (!sink)
> > +        return false;
> > +
> > +    while (sink->input_to_master) {
> > +        sink = sink->input_to_master->sink;
> > +
> > +        if (!sink)
> > +            return true;
> > +    }
> > +
> > +    return false;
> > +}
> > +
> >  /* Called from main context */
> >  bool pa_sink_input_may_move_to(pa_sink_input *i, pa_sink *dest) {
> >      pa_sink_input_assert_ref(i);
> > @@ -1547,6 +1563,16 @@ bool pa_sink_input_may_move_to(pa_sink_input *i, pa_sink *dest) {
> >          return false;
> >      }
> > 
> > +    /* If this sink input is connected to a filter sink that itself is moving,
> > +     * then don't allow the move. Moving requires sending a message to the IO
> > +     * thread of the old sink, and if the old sink is a filter sink that is
> > +     * moving, there's no IO thread associated to the old sink. */
> > +    if (is_filter_sink_moving(i)) {
> > +        pa_log_debug("Can't move input from filter sink %s, because the filter sink itself is currently moving.",
> > +                     i->sink->name);
> > +        return false;
> > +    }
> > +
> >      if (pa_idxset_size(dest->inputs) >= PA_MAX_INPUTS_PER_SINK) {
> >          pa_log_warn("Failed to move sink input: too many inputs per sink.");
> >          return false;
> > diff --git a/src/pulsecore/source-output.c b/src/pulsecore/source-output.c
> > index 9000972..24e9df4 100644
> > --- a/src/pulsecore/source-output.c
> > +++ b/src/pulsecore/source-output.c
> > @@ -1184,6 +1184,22 @@ static bool find_filter_source_output(pa_source_output *target, pa_source *s) {
> >      return false;
> >  }
> > 
> > +static bool is_filter_source_moving(pa_source_output *o) {
> > +    pa_source *source = o->source;
> > +
> > +    if (!source)
> > +        return false;
> > +
> > +    while (source->output_from_master) {
> > +        source = source->output_from_master->source;
> > +
> > +        if (!source)
> > +            return true;
> > +    }
> > +
> > +    return false;
> > +}
> > +
> >  /* Called from main context */
> >  bool pa_source_output_may_move_to(pa_source_output *o, pa_source *dest) {
> >      pa_source_output_assert_ref(o);
> > @@ -1202,6 +1218,17 @@ bool pa_source_output_may_move_to(pa_source_output *o, pa_source *dest) {
> >          return false;
> >      }
> > 
> > +    /* If this source output is connected to a filter source that itself is
> > +     * moving, then don't allow the move. Moving requires sending a message to
> > +     * the IO thread of the old source, and if the old source is a filter
> > +     * source that is moving, there's no IO thread associated to the old
> > +     * source. */
> > +    if (is_filter_source_moving(o)) {
> > +        pa_log_debug("Can't move output from filter source %s, because the filter source itself is currently moving.",
> > +                     o->source->name);
> > +        return false;
> > +    }
> > +
> >      if (pa_idxset_size(dest->outputs) >= PA_MAX_OUTPUTS_PER_SOURCE) {
> >          pa_log_warn("Failed to move source output: too many outputs per source.");
> >          return false;
> > --
> 
> I don't think we should push this out -- it effectively makes using
> filters unsupported, imo.

Not any less unsupported than what it already is.

> We've hit a similar problem in the past,
> which I applied a band-aid around as well [1]. I'm going to try to dig
> in a bit more and find some alternatives.
> 
> Cheers,
> Arun
> 
> [1] https://bugs.freedesktop.org/show_bug.cgi?id=90416

Ah yes, looks like the same bug. There was actually a reference to
90416 in 93443, but I didn't see the connection when I first started to
look into this.

I think instead of postponing the unloading of the module like is done
in the previous band-aid, we could unload it earlier. Using the
DONT_MOVE flags on the streams should cause unloading already when
starting to move the streams when the original alsa sink is about to go
away. That should trigger rescuing the application stream before the
echo-cancel stream is detached from the alsa sink. I think that would
solve both reported crashes.

That said, my patch makes sense anyway, even if it's not needed in this
case, because we still won't be able to move streams that are connected
to a filter device that is moving.

-- 
Tanu