On Tue, 2019-06-18 at 10:15 +0200, Frédéric Danis wrote: > setup_stream() crashes when calling set_nonblock() with an invalid > stream_fd. > > On a new call, the ofono backend gets notified of a new connection. > The ofono backend sets the transport state to playing, and that triggers > a profile change, which sets up the stream for the first time. > Then module-bluetooth-policy sets up the loopbacks. The loopbacks get > fully initialized before the crash. > > After module-bluetooth-policy has done its things, the execution > continues in the transport state change hook. The next hook user is > module-bluez5-device, whose handle_transport_state_change() function > gets called. It will then set up the stream again even though it's > already set up. I'm not sure if that's a some kind of a bug. > setup_stream() can handle the case where it's unnecessarily called, > though, so this second setup is not a big problem. > > The crash happens, because the connection died due to POLLHUP in the IO > thread before the second setup_stream() call. > --- > Changes in v2: > * Update comments and commit message > > src/modules/bluetooth/module-bluez5-device.c | 26 ++++++++++++++++++-- > 1 file changed, 24 insertions(+), 2 deletions(-) > > diff --git a/src/modules/bluetooth/module-bluez5-device.c > b/src/modules/bluetooth/module-bluez5-device.c > index 56c96054d..91c678223 100644 > --- a/src/modules/bluetooth/module-bluez5-device.c > +++ b/src/modules/bluetooth/module-bluez5-device.c > @@ -753,6 +753,8 @@ static void setup_stream(struct userdata *u) { > struct pollfd *pollfd; > int one; > > + pa_assert(u->stream_fd >= 0); > + > /* return if stream is already set up */ > if (u->stream_setup_done) > return; > @@ -829,7 +831,17 @@ static int source_process_msg(pa_msgobject *o, int code, > void *data, int64_t off > } > > case PA_SOURCE_MESSAGE_SETUP_STREAM: > - setup_stream(u); > + /* Skip stream setup if stream_fd has been invalidated. > + This can occur if the stream has already been set up and > + then immediately received POLLHUP. If the stream has > + already been set up earlier, then this setup_stream() > + call is redundant anyway, but currently the code > + is such that this kind of unnecessary setup_stream() > + calls can happen. */ > + if (u->stream_fd < 0) > + pa_log_debug("Skip source stream setup while closing"); > + else > + setup_stream(u); > return 0; > > } > @@ -1007,7 +1019,17 @@ static int sink_process_msg(pa_msgobject *o, int code, > void *data, int64_t offse > } > > case PA_SINK_MESSAGE_SETUP_STREAM: > - setup_stream(u); > + /* Skip stream setup if stream_fd has been invalidated. > + This can occur if the stream has already been set up and > + then immediately received POLLHUP. If the stream has > + already been set up earlier, then this setup_stream() > + call is redundant anyway, but currently the code > + is such that this kind of unnecessary setup_stream() > + calls can happen. */ > + if (u->stream_fd < 0) > + pa_log_debug("Skip sink stream setup while closing"); > + else > + setup_stream(u); > return 0; > } >
Thanks! Applied. -- Tanu https://www.patreon.com/tanuk https://liberapay.com/tanuk _______________________________________________ pulseaudio-discuss mailing list pulseaudio-discuss@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss