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

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

[Geoffrey McRae]

The client may have been freed already by a secondary audio device
recovering its session as JACK2 has some cleanup code to work around
broken clients, which doesn't account for well behaved clients.

https://github.com/jackaudio/jack2/issues/627

As JACK1 and JACK2 are interchangeable and JACK2 has "cleanup" routine
that JACK1 does not have, we need to determine which version is in use
at runtime. Unfortunatly there is no way to determine which is in use
other then to look for symbols that are missing in JACK1, which in this
case is `jack_get_version`.

An issue has been raised over this, but to be compatible with older
versions we must use this method to determine which library is in use.
If at some time the jack developers implement `jack_get_version` in
JACK1, this code will need to be revisited.

At worst the workaround will be enabled and this will introduce a small
memory leak if the jack server is restarted. This however is better then
the alternative which would be a use after free segfault.

Signed-off-by: Geoffrey McRae <geoff@hostfission.com>
---
 audio/jackaudio.c | 37 ++++++++++++++++++++++++++++++++++++-
 configure         |  5 +++--
 2 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/audio/jackaudio.c b/audio/jackaudio.c
index 72ed7c4929..d1685999c3 100644
--- a/audio/jackaudio.c
+++ b/audio/jackaudio.c
@@ -31,6 +31,7 @@
 #define AUDIO_CAP "jack"
 #include "audio_int.h"
 
+#include <dlfcn.h>
 #include <jack/jack.h>
 #include <jack/thread.h>
 
@@ -84,6 +85,7 @@ typedef struct QJackIn {
 }
 QJackIn;
 
+static int QJackWorkaroundCloseBug;
 static int qjack_client_init(QJackClient *c);
 static void qjack_client_connect_ports(QJackClient *c);
 static void qjack_client_fini(QJackClient *c);
@@ -563,7 +565,10 @@ static void qjack_client_fini(QJackClient *c)
         /* fallthrough */
 
     case QJACK_STATE_SHUTDOWN:
-        jack_client_close(c->client);
+        if (!QJackWorkaroundCloseBug) {
+            jack_client_close(c->client);
+        }
+        c->client = NULL;
         /* fallthrough */
 
     case QJACK_STATE_DISCONNECTED:
@@ -662,6 +667,36 @@ static void qjack_info(const char *msg)
 
 static void register_audio_jack(void)
 {
+    void *handle;
+
+    /*
+     * As JACK1 and JACK2 are interchangeable and JACK2 has "cleanup" routine
+     * that JACK1 does not have, we need to determine which version is in use at
+     * runtime. Unfortunatly there is no way to determine which is in use other
+     * then to look for symbols that are missing in JACK1, which in this case is
+     * `jack_get_version`. An issue has been raised over this, but to be
+     * compatible with older versions we must use this method to determine which
+     * library is in use. If at some time the jack developers implement
+     * `jack_get_version` in JACK1, this code will need to be revisited.
+     *
+     * At worst the workaround will be enabled and we will introduce a small
+     * memory leak if the jack server is restarted. This is better then the
+     * alternative which would be a use after free segfault.
+     */
+
+    handle = dlopen("libjack.so", RTLD_LAZY | RTLD_NOLOAD);
+    if (!handle) {
+        dolog("unable to open libjack.so to determine version\n");
+        dolog("assuming JACK2 and enabling the close bug workaround\n");
+        QJackWorkaroundCloseBug = 1;
+    } else {
+        if (dlsym(handle, "jack_get_version")) {
+            dolog("JACK2 detected, enabling close bug workaround\n");
+            QJackWorkaroundCloseBug = 1;
+        }
+        dlclose(handle);
+    }
+
     audio_driver_register(&jack_driver);
     jack_set_thread_creator(qjack_thread_creator);
     jack_set_error_function(qjack_error);
diff --git a/configure b/configure
index 2acc4d1465..e65ec5c5e3 100755
--- a/configure
+++ b/configure
@@ -3754,7 +3754,8 @@ for drv in $audio_drv_list; do
 
     jack | try-jack)
     if $pkg_config jack --exists; then
-        jack_libs=$($pkg_config jack --libs)
+        # dl is needed to check at runtime if jack1 or jack2 is in use
+        jack_libs="$($pkg_config jack --libs) -ldl"
         if test "$drv" = "try-jack"; then
             audio_drv_list=$(echo "$audio_drv_list" | sed -e 's/try-jack/jack/')
         fi
@@ -8644,4 +8645,4 @@ printf " '%s'" "$0" "$@" >>config.status
 echo ' "$@"' >>config.status
 chmod +x config.status
 
-rm -r "$TMPDIR1"
+rm -r "$TMPDIR1"
\ No newline at end of file
-- 
2.20.1

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

[Gerd Hoffmann]

  Hi,

> As JACK1 and JACK2 are interchangeable and JACK2 has "cleanup" routine
> that JACK1 does not have, we need to determine which version is in use
> at runtime. Unfortunatly there is no way to determine which is in use
> other then to look for symbols that are missing in JACK1, which in this
> case is `jack_get_version`.

No.  That'll quickly becomes a maintainance nightmare.

How about moving the qjack_client_fini() call to qjack_shutdown()?  Or,
if that isn't an option due to qjack_shutdown being called from a signal
handler, schedule a bottom half calling qjack_client_fini()?

take care,
  Gerd

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

[Geoffrey McRae]

On 2020-08-19 15:04, Gerd Hoffmann wrote:
> Hi,
> 
>> As JACK1 and JACK2 are interchangeable and JACK2 has "cleanup" routine
>> that JACK1 does not have, we need to determine which version is in use
>> at runtime. Unfortunatly there is no way to determine which is in use
>> other then to look for symbols that are missing in JACK1, which in 
>> this
>> case is `jack_get_version`.
> 
> No.  That'll quickly becomes a maintainance nightmare.
> 
> How about moving the qjack_client_fini() call to qjack_shutdown()?  Or,
> if that isn't an option due to qjack_shutdown being called from a 
> signal
> handler, schedule a bottom half calling qjack_client_fini()?

You are correct, you can not perform such actions in the callback.

> schedule a bottom half calling qjack_client_fini()

Does QEMU have such a mechanism for doing this?

> 
> take care,
>   Gerd

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

[Geoffrey McRae]

On 2020-08-19 15:28, Geoffrey McRae wrote:
> On 2020-08-19 15:04, Gerd Hoffmann wrote:
>> Hi,
>> 
>>> As JACK1 and JACK2 are interchangeable and JACK2 has "cleanup" 
>>> routine
>>> that JACK1 does not have, we need to determine which version is in 
>>> use
>>> at runtime. Unfortunatly there is no way to determine which is in use
>>> other then to look for symbols that are missing in JACK1, which in 
>>> this
>>> case is `jack_get_version`.
>> 
>> No.  That'll quickly becomes a maintainance nightmare.
>> 
>> How about moving the qjack_client_fini() call to qjack_shutdown()?  
>> Or,
>> if that isn't an option due to qjack_shutdown being called from a 
>> signal
>> handler, schedule a bottom half calling qjack_client_fini()?
> 
> You are correct, you can not perform such actions in the callback.
> 
>> schedule a bottom half calling qjack_client_fini()
> 
> Does QEMU have such a mechanism for doing this?

There could also be a possible race here if `jack_client_connect` is 
called before the scheduled shutdown takes place.

> 
>> 
>> take care,
>>   Gerd

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

[Gerd Hoffmann]

  Hi,

> > schedule a bottom half calling qjack_client_fini()
> 
> Does QEMU have such a mechanism for doing this?

Yes, look for QEMUBH in include/qemu/main-loop.h

HTH,
  Gerd

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

[Gerd Hoffmann]

  Hi,

> > > schedule a bottom half calling qjack_client_fini()
> > 
> > Does QEMU have such a mechanism for doing this?
> 
> There could also be a possible race here if `jack_client_connect` is called
> before the scheduled shutdown takes place.

You can cancel a scheduled bottom half, and checking ->state on connect
should tell you whenever this is needed or not.  I think you can even
just cancel unconditionally (when not scheduled the cancel is a nop).

HTH,
  Gerd