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?
