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

[Christian Schoenebeck]

On Dienstag, 18. August 2020 14:40:36 CEST Geoffrey McRae wrote:
> Due to a ridiculous commit in the Jack library, the client may have been
> freed already by a secondary audio device recovering its session.
> 
> https://github.com/jackaudio/jack2/issues/627
> 
> Until there is a proper fix for this we can not risk using the pointer
> at all if we have been notified of a shutdown as it may have been freed
> by the jack library, as such the close call is commented out to prevent
> a use after free segfault.
> 
> At this time, this will not cause a memory leak as the recovery routine
> will trigger the "cleanup" code in the jack library, however, if this is
> ever corrected in the jack library this will need to be revisited.
> 
> Signed-off-by: Geoffrey McRae <geoff@hostfission.com>
> ---
>  audio/jackaudio.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/audio/jackaudio.c b/audio/jackaudio.c
> index 72ed7c4929..e8faf1bb89 100644
> --- a/audio/jackaudio.c
> +++ b/audio/jackaudio.c
> @@ -563,7 +563,22 @@ static void qjack_client_fini(QJackClient *c)
>          /* fallthrough */
> 
>      case QJACK_STATE_SHUTDOWN:
> -        jack_client_close(c->client);
> +        /*
> +         * Due to a rediculous commit in the Jack library, the client may
> have +         * been freed already.

No need to be offending, and especially no need to insult Stéphane in QEMU 
code.

Could you please describe in more detail how you ran into this situation with 
your 2nd audio device?

> +         *
> +         * Until there is a proper fix for this we can not risk using the
> +         * pointer at all if we have been notified of a shutdown, as such
> the +         * below line is commented out to prevent a use after free
> segfault. +         * This will not cause a memory leak as the recovery
> routine will trigger +         * the "cleanup" code in the jack library.
> +         *
> +         *
> https://github.com/jackaudio/jack2/commit/171a3c4a0ddd18d2afae56f3af6291c8e
> 96ee3ac +         */
> +
> +        //jack_client_close(c->client);
> +        c->client = NULL;
> +
>          /* fallthrough */

Are you aware that there are two distinct variants of JACK? They are commonly 
referred to as JACK1 vs. JACK2 and despite their names, they are in fact 
completely separate implementations and there are people who prefer one over 
the other. Your change would affect JACK1 as well.

Best regards,
Christian Schoenebeck