[pulseaudio-discuss] [PATCH] echo-cancel: Avoid segfaults due to invalid master sink or source

[Tanu Kaskinen]

On Wed, 2017-05-03 at 22:19 +0200, Georg Chini wrote:
> On 03.05.2017 21:58, Tanu Kaskinen wrote:
> > On Tue, 2017-05-02 at 07:12 +0200, Georg Chini wrote:
> > > On 01.05.2017 22:10, Tanu Kaskinen wrote:
> > > > On Mon, 2017-04-24 at 19:33 +0200, Georg Chini wrote:
> > > > > There are several places in module-echo-cancel where a segfault is
> > > > > possible when the master sink or source is invalid.
> > > > 
> > > > I don't think the rewind, volume and mute callbacks are ever called
> > > > during stream moves, at least with the current code base. However,
> > > > adding the extra checks does no harm either, so I don't mind, but I
> > > > think the commit message should be clarified on this point.
> > > 
> > > I do not mention a move at all in the commit message, I just
> > > say there are several places where a segfault is possible.
> > 
> > The patch adds sink_input->sink and source_output->source checks, and
> > checking those pointers is only relevant during stream moves (or during
> > stream initialization before the routing has been decided, but the
> > callbacks aren't used that early). At all other times the sink and
> > source pointers are non-NULL.
> > 
> > > I think the rewind callback is called during a move, the
> > > PA_SINK_MESSAGE_START_MOVE calls request_rewind().
> > 
> > That happens before detaching the sink input, so the sink pointer is
> > not NULL.
> 
> This seems not correct. At least after fixing the first bug, the next
> crash occurred in pa_sink_invalidate_requested_latency() during
> PA_SINK_MESSAGE_START_MOVE. The request_rewind is just a few
> lines below.
> 
> There must be (at least for echo-cancel) a possible situation
> where the sink-input is moved from (null) to a real sink, otherwise
> the crash I fixed could not happen at all.

Sorry, I mixed the request rewind and process rewind callbacks. When
pa_sink_request_rewind() is called on the virtual sink, the request
gets propagated to the master sink, but in this case the filter sink is
not connected to the master sink.

I remember fighting with this case before... (looking up old stuff...)
Last time it was module-device-manager that was moving a stream from an
echo-cancel sink to the auto null sink during an alsa card profile
change. So the case was quite similar, just a different policy module
(device-manager instead of switch-on-connect).

The fix that was used last time is not really applicable here. Part of
the fix was to set the DONT_MOVE flags to module-echo-cancel's streams,
but those flags are only set when module-echo-cancel is autoloaded by
module-filter-apply, and apparently in this case module-echo-cancel is
manually loaded (otherwise this situation shouldn't happen).

We haven't so far supported moving streams that are connected to an
already-moving filter sink, and I don't think this patch is a good way
to add that support. The crash happens in some IO thread, but since the
filter sink is not attached to any master sink, it's not clear which IO
thread that is. Probably it's the old IO thread (the sink asyncmsgq
pointer gets updated in the moving callback), but my point is that if
it seems to work, it works by accident. After the filter sink has been
detached from its master, the old IO thread could be already destroyed,
so relying on it still running seems like a bad idea. If we want to
support moving streams while not connected to any IO thread, I think
the system design should be thought through properly.

I propose applying this patch (the commit message needs to be
rewritten): https://patchwork.freedesktop.org/patch/68741/

That patch was not applied, because it was not needed after all to
solve the bug last time, and because Arun didn't like it, but I think
it makes sense and it should fix this crash. It will prevent module-
switch-on-connect from moving the application stream to the auto null
sink, which is fine.

> > 
> > > Also the other checks are not pointless. In a situation where
> > > a virtual sink is the only remaining sink, you can still try to
> > > mute it or change the volume which will then crash PA.
> > 
> > A virtual sink can't exist without a master sink. The only time when a
> > virtual sink is not connected to a master sink is when the virtual sink
> > is being moved to a different master, and client commands (such as
> > volume and mute change commands) are not processed during moves.
> > 
> 
> Yes, I tested it, the modules get unloaded when no sinks are
> available. I could not find anything on the first glance that did
> it, so I assumed they would just "hang around".

FYI, pa_sink_unlink() calls pa_sink_input_kill() for every stream that
is connected to the sink. The kill callback of the filter sink will
then unload the module.

> Do you think I should remove the checks from the patch?

I think this patch shouldn't be applied at all, but in case the follow-
up discussion leads to a different conclusion, my preference is to
remove the volume/mute callback changes, although I don't feel very
strongly about that.

-- 
Tanu

https://www.patreon.com/tanuk