[pulseaudio-discuss] [PATCH] thread-mq: Make pa_thread_mq_done more robust

[Tanu Kaskinen]

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