[pulseaudio-discuss] [PATCH] bluetooth: Fix crash in setup_stream()

[Tanu Kaskinen]

On Mon, 2019-06-17 at 11:49 +0200, Frédéric Danis wrote:
> setup_stream() crashes when calling set_nonblock() with an invalid
> stream_fd.
> 
> This crash is due to a race condition.
> The audio call starts normally, and a 1st call to setup_stream() is
> completed properly.
> But, if the call is "quickly" hung up, the stream_fd is in error (POLLHUP)
> before other modules (loopback, …) have completed their initialization.
> This error triggers a call to teardown_stream() which set stream_fd to -1.
> After that, the other modules continue their initialization before the IO
> thread is stopped, triggering a 2nd call to setup_stream().

I think this description is a bit inaccurate or unclear. Based on the
log you showed in irc, it seems that this is what happens:

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.

The fix is probably good, but the commit message and the comments need
some changes.

> ---
>  src/modules/bluetooth/module-bluez5-device.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/src/modules/bluetooth/module-bluez5-device.c b/src/modules/bluetooth/module-bluez5-device.c
> index 56c96054d..d0e3c7a09 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,13 @@ 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 as been invalidated, this can
> +             * occurs in case of POLLHUP before other modules have finished
> +             * their initialization */

The initialization of other modules is not relevant (except maybe due
to the delay that causes which makes the race condition trigger more
easily). I think this would be a better comment:

            /* 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 +1015,13 @@ 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 as been invalidated, this can
> +             * occurs in case of POLLHUP before other modules have finished
> +             * their initialization */
> +            if (u->stream_fd < 0)
> +                pa_log_debug("Skip sink stream setup while closing");
> +            else
> +                setup_stream(u);
>              return 0;
>      }
>  
-- 
Tanu

https://www.patreon.com/tanuk
https://liberapay.com/tanuk