Created attachment 114468 [details]
PulseAudio Logs

Created attachment 114469 [details] [review]
This patch avoids the crash issue explained in this bug

This bug looks like a timing issue where POLLHUP has happened during rendering.

This patch may not be the proper fix, but the crash is actually avoided with the attached changes.

In addition to the issue explained by Mr. Sagar, please find the root cause for the issue.

//When remote headset is unpaired or disconnected, blueZ will call "ClearConfiguration" Dbus method. Which will update the transport state to disconnected.

16:19:35.027+0900   375   375 I PULSEAUDIO: [pulseaudio] bluez5-util.c: dbus: path=/MediaEndpoint/A2DPSource, interface=org.bluez.MediaEndpoint1, member=ClearConfiguration
16:19:35.027+0900   375   375 I PULSEAUDIO: [pulseaudio] bluez5-util.c: Clearing transport /org/bluez/hci0/dev_00_18_09_24_68_25/fd0 profile a2dp_sink

//Due to which the STOP thread will be handled.
transport_state_changed_cb() -> pa_card_set_profile() -> set_profile_cb() -> stop_thread().

//When Stop thread handling is in progress, rendering thread will post the "BLUETOOTH_MESSAGE_IO_THREAD_FAILED" due to audio packet pa_write() fail.
thread_func() -> a2dp_process_render() -> goto fail; -> pa_asyncmsgq_post(with IO thread fail).
16:19:35.032+0900   375   620 E PULSEAUDIO: [bluetooth] module-bluez5-device.c: Failed to write data to socket: Broken pipe
16:19:35.032+0900   375   620 I PULSEAUDIO: [bluetooth] module-bluez5-device.c: IO thread failed

//Since processing of message is done inside the stop_thread() which internally calls the Stop_thread() one more time.
stop_thread() -> pa_thread_mq_done() -> pa_asyncmsgq_flush() -> pa_asyncmsgq_dispatch() -> device_process_msg() for  BLUETOOTH_MESSAGE_IO_THREAD_FAILED -> pa_card_set_profile() -> set_profile_cb() -> stop_thread().

Created attachment 114486 [details]
Proposed Patch to Fix the PA crash due to stop_thread() recursive.

Proposed Patch to avoid the PA crash due to stop_thread() recursive.

When remote headset is unpaired or disconnected, blueZ
will call "ClearConfiguration" Dbus method. Which will update
the transport state to disconnected. Due to which the STOP thread
will be handled. When Stop thread handling is in progress, rendering
thread will post the "BLUETOOTH_MESSAGE_IO_THREAD_FAILED" due to
audio packet pa_write() fail. Since processing of message is done
inside the stop_thread() which internally calls the Stop_thread()
one more time. Further access pa msgQue memory which is already
freed will leads to the pulseAudio crash.
This patch with avoid the recursive call of stop thread.

Hi,

In general, stopping recursion is good and as such I don't mind the patch by Rakesh M K, but I think we should also make pa_thread_mq_done save against recursion.

http://lists.freedesktop.org/archives/pulseaudio-discuss/2015-March/023450.html

Created attachment 114580 [details] [review]
Fix: Prevent calling pa_rtpoll_free() for a NULL rtpoll

Hello David,

We encountered a crash issue while verifying your below change:
http://lists.freedesktop.org/archives/pulseaudio-discuss/2015-March/023450.html

We have attached a patch that addressed the crash issue observed. Please share your opinion on the same.

Thanks,
Sagar N

Comment on attachment 114580 [details] [review]
Fix: Prevent calling pa_rtpoll_free() for a NULL rtpoll

Review of attachment 114580 [details] [review]:
-----------------------------------------------------------------

Sorry for the slow reply. Are you 100% certain that messages dispatched by pa_thread_mq_done will not need the rtpoll? If not, perhaps the following is better:

    if (u->rtpoll)
        pa_thread_mq_done(&u->thread_mq); 
    /* pa_thread_mq_done might recurse into stop_thread again, hence this extra "if (u->rtpoll)" check */
    if (u->rpoll) {
        pa_rtpoll_free(u->rtpoll);
        u->rtpoll = NULL;
    }

Do we have any other cases where we expect pa_thread_mq_done() to be called recursively?

It seems to me that it is stop_thread() that should be safe against multiple calls (maybe tear down the thread first, and if it's already been torn down, just return).

Created attachment 114666 [details] [review]
PATCH v1: Prevent calling pa_rtpoll_free() for a NULL rtpoll

Hi David,

We agree with your comment. Its safer to have an extra check.

Regards,
Sagar Nm

The rtpoll object is the event loop of the IO thread. No other thread has any business accessing it (except when creating and freeing it). When pa_thread_mq_done() is called, the IO thread has already been torn down, so there's no risk of needing the rtpoll object inside pa_thread_mq_done().

After some discussion, two patches have now been committed; "Fix: Prevent calling pa_rtpoll_free() for a NULL rtpoll" and "thread-mq: Make pa_thread_mq_done more robust".

These two patches should resolve the bug, if I understand things correctly. Please reopen the bug if this is not correct. Thanks!