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

[Philippe Mathieu-Daudé]

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
>