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
