[pulseaudio-discuss] [PATCH] bluetooth: Fix crash on disconnection

[Tanu Kaskinen]

On Mon, 2012-05-14 at 16:21 +0200, Frédéric Danis wrote:
> When a Bluetooth headset is connected only to HFP profile (not connected
> to A2DP) and host streams to it, a crash occurs if host disconnects.
> 
> When HFP disconnects, audio thread will fail on POLLHUP then generate
> a message to set PA profile to Off before ending.
> If this message is managed before PA unload bluetooth device module,
> all works fine.
> But, if this message is managed during module unload, this finish by
> re-entrance in release code (stop_thread) and a crash.
> 
> This fix prevents to send the profile change when module is unloading.

So the problem is that pa_thread_mq_done() dispatches all pending
messages that have been sent to the main thread. If one of those
messages is BLUETOOTH_MESSAGE_SET_PROFILE, things blow up.

> diff --git a/src/modules/bluetooth/module-bluetooth-device.c b/src/modules/bluetooth/module-bluetooth-device.c
> index b7945ed..97e5944 100644
> --- a/src/modules/bluetooth/module-bluetooth-device.c
> +++ b/src/modules/bluetooth/module-bluetooth-device.c
> @@ -1750,7 +1750,8 @@ static void thread_func(void *userdata) {
>  fail:
>      /* If this was no regular exit from the loop we have to continue processing messages until we receive PA_MESSAGE_SHUTDOWN */
>      pa_log_debug("IO thread failed");
> -    pa_asyncmsgq_post(pa_thread_mq_get()->outq, PA_MSGOBJECT(u->msg), BLUETOOTH_MESSAGE_SET_PROFILE, "off", 0, NULL, NULL);
> +    if (!u->module->unload_requested)
> +        pa_asyncmsgq_post(pa_thread_mq_get()->outq, PA_MSGOBJECT(u->msg), BLUETOOTH_MESSAGE_SET_PROFILE, "off", 0, NULL, NULL);
>      pa_asyncmsgq_wait_for(u->thread_mq.inq, PA_MESSAGE_SHUTDOWN);
>  
>  finish:

The IO thread shouldn't check the state of the main thread. Here,
unload_requested may be FALSE when it's checked, but it may change to
TRUE before the message is processed.

I think the correct fix would be to change the
BLUETOOTH_MESSAGE_SET_PROFILE handler so that it checks whether setting
the profile makes sense. As a cosmetic improvement, I'd also rename the
message to BLUETOOTH_MESSAGE_IO_THREAD_FAILED. I think it's not really
the IO thread's job to tell the main thread what to do - the control
relationship goes to the other direction. The IO thread should only tell
the main thread about what's happening, and the main thread can then
decide what's the best thing to do - should the profile be changed to
"off" or should the message be just ignored.

-- 
Tanu