[v2] bluetooth: Fix crash in setup_stream()

Submitted by Frédéric Danis on June 18, 2019, 8:15 a.m.

Details

Message ID 20190618081515.7612-1-frederic.danis@collabora.com
State Accepted
Commit 661b13d50d5af5c14f80e8678aaf942fbc5628ae
Headers show
Series "bluetooth: Fix crash in setup_stream()" ( rev: 2 ) in PulseAudio

Not browsing as part of any series.

Commit Message

Frédéric Danis June 18, 2019, 8:15 a.m.
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(-)

Patch hide | download patch | download mbox

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;
     }
 

Comments

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.