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

[Georg Chini]

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.

>
>> 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".

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