qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/1] audio/jack: fix use after free segfault
@ 2020-08-19  1:18 Geoffrey McRae
  2020-08-19  1:18 ` [PATCH v3 1/1] " Geoffrey McRae
  0 siblings, 1 reply; 6+ messages in thread
From: Geoffrey McRae @ 2020-08-19  1:18 UTC (permalink / raw)
  To: qemu-devel; +Cc: Geoffrey McRae, kraxel

Fixed accidental eof newline strip from `configure`

Geoffrey McRae (1):
  audio/jack: fix use after free segfault

 audio/jackaudio.c | 37 ++++++++++++++++++++++++++++++++++++-
 configure         |  4 +++-
 2 files changed, 39 insertions(+), 2 deletions(-)

-- 
2.20.1



^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v3 1/1] audio/jack: fix use after free segfault
  2020-08-19  1:18 [PATCH v3 0/1] audio/jack: fix use after free segfault Geoffrey McRae
@ 2020-08-19  1:18 ` Geoffrey McRae
  2020-08-19  3:32   ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 6+ messages in thread
From: Geoffrey McRae @ 2020-08-19  1:18 UTC (permalink / raw)
  To: qemu-devel; +Cc: Geoffrey McRae, kraxel

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         |  4 +++-
 2 files changed, 39 insertions(+), 2 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..43d2893fbb 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
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/1] audio/jack: fix use after free segfault
  2020-08-19  1:18 ` [PATCH v3 1/1] " Geoffrey McRae
@ 2020-08-19  3:32   ` Philippe Mathieu-Daudé
  2020-08-19  3:36     ` Geoffrey McRae
  0 siblings, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-08-19  3:32 UTC (permalink / raw)
  To: Geoffrey McRae, qemu-devel; +Cc: kraxel

Hi Geoffrey,

On 8/19/20 3:18 AM, Geoffrey McRae wrote:
> 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         |  4 +++-
>  2 files changed, 39 insertions(+), 2 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..43d2893fbb 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

Why not checking jack_get_version() using compile_prog here?

Thanks,

Phil.



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/1] audio/jack: fix use after free segfault
  2020-08-19  3:32   ` Philippe Mathieu-Daudé
@ 2020-08-19  3:36     ` Geoffrey McRae
  2020-08-19  4:46       ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 6+ messages in thread
From: Geoffrey McRae @ 2020-08-19  3:36 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé; +Cc: qemu-devel, kraxel



On 2020-08-19 13:32, Philippe Mathieu-Daudé wrote:
> Hi Geoffrey,
> 
> On 8/19/20 3:18 AM, Geoffrey McRae wrote:
>> 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         |  4 +++-
>>  2 files changed, 39 insertions(+), 2 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..43d2893fbb 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
> 
> Why not checking jack_get_version() using compile_prog here?
> 
> Thanks,
> 
> Phil.

Hi Phil,

Because the library can be swapped out after compile time as the 
versions are ABI compatible by design.

-Geoff


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/1] audio/jack: fix use after free segfault
  2020-08-19  3:36     ` Geoffrey McRae
@ 2020-08-19  4:46       ` Philippe Mathieu-Daudé
  2020-08-19  5:18         ` Geoffrey McRae
  0 siblings, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-08-19  4:46 UTC (permalink / raw)
  To: Geoffrey McRae; +Cc: qemu-devel, kraxel

On 8/19/20 5:36 AM, Geoffrey McRae wrote:
> 
> 
> On 2020-08-19 13:32, Philippe Mathieu-Daudé wrote:
>> Hi Geoffrey,
>>
>> On 8/19/20 3:18 AM, Geoffrey McRae wrote:
>>> 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         |  4 +++-
>>>  2 files changed, 39 insertions(+), 2 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..43d2893fbb 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
>>
>> Why not checking jack_get_version() using compile_prog here?
>>
>> Thanks,
>>
>> Phil.
> 
> Hi Phil,
> 
> Because the library can be swapped out after compile time as the
> versions are ABI compatible by design.

IIUC in the GH issue you linked you describe a problem from v1.9.1
to v1.9.14. I see jack_get_version() is already available in v1.9.1:
https://github.com/jackaudio/jack2/blob/1.9.1/common/jack/jack.h#L55

Why would someone link with jack2 then replace it with jack1 without
rebuilding? That sounds silly...

> 
> -Geoff
> 



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/1] audio/jack: fix use after free segfault
  2020-08-19  4:46       ` Philippe Mathieu-Daudé
@ 2020-08-19  5:18         ` Geoffrey McRae
  0 siblings, 0 replies; 6+ messages in thread
From: Geoffrey McRae @ 2020-08-19  5:18 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé; +Cc: qemu-devel, kraxel



On 2020-08-19 14:46, Philippe Mathieu-Daudé wrote:
> On 8/19/20 5:36 AM, Geoffrey McRae wrote:
>> 
>> 
>> On 2020-08-19 13:32, Philippe Mathieu-Daudé wrote:
>>> Hi Geoffrey,
>>> 
>>> On 8/19/20 3:18 AM, Geoffrey McRae wrote:
>>>> 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         |  4 +++-
>>>>  2 files changed, 39 insertions(+), 2 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..43d2893fbb 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
>>> 
>>> Why not checking jack_get_version() using compile_prog here?
>>> 
>>> Thanks,
>>> 
>>> Phil.
>> 
>> Hi Phil,
>> 
>> Because the library can be swapped out after compile time as the
>> versions are ABI compatible by design.
> 
> IIUC in the GH issue you linked you describe a problem from v1.9.1
> to v1.9.14. I see jack_get_version() is already available in v1.9.1:
> https://github.com/jackaudio/jack2/blob/1.9.1/common/jack/jack.h#L55

Do not confuse 1.9.1 with JACK1, this project has very strange 
versioning.

JACK1 uses the headers located here.
https://github.com/jackaudio/headers/blob/master/jack.h

> 
> Why would someone link with jack2 then replace it with jack1 without
> rebuilding? That sounds silly...

Distributions commonly let you select which jack library you wish to run 
with and allow them to be swapped around.

> 
>> 
>> -Geoff
>> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-08-19  5:19 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-19  1:18 [PATCH v3 0/1] audio/jack: fix use after free segfault Geoffrey McRae
2020-08-19  1:18 ` [PATCH v3 1/1] " Geoffrey McRae
2020-08-19  3:32   ` Philippe Mathieu-Daudé
2020-08-19  3:36     ` Geoffrey McRae
2020-08-19  4:46       ` Philippe Mathieu-Daudé
2020-08-19  5:18         ` Geoffrey McRae

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).