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

[Geoffrey McRae]

On 2020-08-19 04:11, Christian Schoenebeck wrote:
> 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.

Fair enough, I was not intending to offend Stéphane, and I apologize for 
this.
I will revise this patch.

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

Sure. Run a Windows guest with two audio devices, let it boot up, then 
restart
the jack service to trigger the recovery routine, then attempt to use 
the 2nd
(non-primary) audio device. Ie, go to windows audio settings to test the
microphone of the second audio device.

When windows try to use the 2nd audio device it goes through the 
recovery
routine triggering this fault.

> > +         *
> > +         * 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.

I am aware and since these libraries are interchangeable I had assumed 
that
JACK1 will have the same fault. If not I suppose we need to detect which 
is in
use and change this code appropriately.

> Best regards,
> Christian Schoenebeck