Re: [PATCH v8 1/1] audio/jack: fix use after free segfault

[Christian Schoenebeck]

On Samstag, 22. August 2020 02:16:23 CEST Geoffrey McRae wrote:
> On 2020-08-22 03:47, Paolo Bonzini wrote:
> > On 21/08/20 19:34, Christian Schoenebeck wrote:
> >>>  static void qjack_fini_out(HWVoiceOut *hw)
> >>>  {
> >>>  
> >>>      QJackOut *jo = (QJackOut *)hw;
> >>>      qjack_client_fini(&jo->c);
> >>> 
> >>> +
> >>> +    qemu_bh_delete(jo->c.shutdown_bh);
> >> 
> >> Paolo wrapped that qemu_bh_delete() call inside the lock as well. So I
> >> guess
> >> it makes a difference for the BH API?
> > 
> > It is not a problem as long as qjack_client_fini is idempotent.
> 
> `qjack_client_fini` is indeed idempotent

Right.

> >>> +    qemu_mutex_destroy(&jo->c.shutdown_lock);
> >>> 
> >>>  }
> >> 
> >> Hmmm, is this qemu_mutex_destroy() safe at this point?
> > 
> > Perhaps make the mutex global and not destroy it at all.
> 
> It's safe at this point as `qjack_fini_out` is only called at device
> destruction, and `qjack_client_fini` ensures that JACK is shut down
> which prevents jack from trying to call the shutdown event handler.

You mean because jack_client_close() is synchronized. That prevents JACK from 
firing the callback after jack_client_close() returns, that's correct.

But as qemu_bh_delete() is async, you do not have a guarantee that a 
previously scheduled BH shutdown handler is no longer running. So it might 
still hold the lock when you attempt to destroy the mutex.

On doubt I would do like Paolo suggested by making the mutex global and not 
destroying it at all.

Best regards,
Christian Schoenebeck