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

[Philippe Mathieu-Daudé]

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.