On Tue, 2015-03-24 at 07:44 +0100, David Henningsson wrote: > > On 2015-03-23 18:18, Tanu Kaskinen wrote: > > On Mon, 2015-03-23 at 14:39 +0100, David Henningsson wrote: > >> While investigating bug 89672 it was found that pa_thread_mq_done > >> was called recursively. Regardless of whether the recursion should > >> be stopped by other means, it seems to make sense to make > >> pa_thread_mq_done more robust so that it can be called twice > >> (and even recursively) without harm. > >> > >> BugLink: https://bugs.freedesktop.org/show_bug.cgi?id=89672 > >> Signed-off-by: David Henningsson <david.henningsson at canonical.com> > >> --- > >> src/pulsecore/thread-mq.c | 32 +++++++++++++++++++++++--------- > >> 1 file changed, 23 insertions(+), 9 deletions(-) > > > > I would prefer to replace pa_thread_mq_init() and pa_thread_mq_done() > > with pa_thread_mq_new() and pa_thread_mq_free(), the difference of > > course being that _new() would return a newly allocated struct instead > > of modifying a caller-supplied struct. That would allow the bluetooth > > code to set the thread_mq to NULL when it's not used. > > In general, I don't mind that approach, but I wonder how well it would > work in this recursion case - it seems like such an attempt would double > free the thread_mq? Ah, right, I didn't read the patch carefully enough to realize that the pa_thread_mq destructor is particularly prone to recursion issues regardless of the choice between the "done" and "free" styles. So changing to the new/free style could replace the patch by Rakesh M K, but it doesn't really affect the validity of your patch. I think the extra pointer checks are fine in any case. A sidenote: I think it would be possible to get rid of this source of recursion altogether by passing false as the "run" parameter of pa_asyncmsgq_flush() and doing some other tweaks. I added this to the bottom of my bottomless todo list (feel free to implement it before I get around to it): "pa_asyncmsgq_flush() should never dispatch the queued messages. The "run" parameter is only set to true in pa_thread_mq_done(), and that function shouldn't set it to true either. pa_thread_mq_done() is called after the thread has already been torn down. At that point the queued messages should be irrelevant, so it should be ok to not dispatch them. Also, it should be possible to call pa_asyncmsgq_flush() while dispatching is ongoing (currently trying to do that will result in a crash). Flushing while dispatching should work so that the message being currently dispatched should be left alone, but all subsequent messages should be flushed. That would ensure that no messages get dispatched after pa_thread_mq_done() has been called (currently the mq will keep dispatching messages after pa_thread_mq_done() if pa_thread_mq_done() is called during the dispatch loop, because in that case pa_thread_mq_done() doesn't call pa_asyncmsgq_flush(), because with the current code doing so would cause a crash)." -- Tanu
