jackaudio / jack2

jack2 codebase
GNU General Public License v2.0
2.22k stars 376 forks source link

jack2 on darwin fails when client name exceeds 27 characters #474

Open hlolli opened 5 years ago

hlolli commented 5 years ago

There seems to be a difference to how long the client name can be from linux to darwin. I'm able to exceed client names with well over 32 characters on linux. But on OsX I get Cannot open 0iiiiiiiiiiiiiiiiiiiiiiiii12 client but deleting 1 character here (the number 2) it works just fine. To be more precise, I believe the limit is somewhere hard set to 32 with the clientName:inputPort (colon input-port). I compiled jack2 from develop branch last week. If it were possible, I'd love to see the clientName limit a bit higher 128 or preferably 256.

I believe the error is coming from here https://github.com/jackaudio/jack2/blob/develop/common/JackLibClient.cpp#L124-L127

falkTX commented 5 years ago

Most developers that use jack are not running osx, so these things take a while until they are noticed. Since you can see the issue directly, do you mind creating a PR with a fix please? Thanks

hlolli commented 5 years ago

I'll try few things, see if it gets fixed. Happy to contribute if I can :)

hlolli commented 5 years ago

Btw, I relize now that I did your Carla workshop in Berlin last summer :100:

hlolli commented 5 years ago

Before I PR something silly, I think I've located the error to the fact that 32+ characters actually cause the semaphores to crash on darwin: https://github.com/jackaudio/jack2/blob/98c882c5a545c3dd819655c9869f6928274df8a3/posix/JackPosixSemaphore.cpp#L158

I researched unnamed semaphores, which would make sense because the sem_t* fSemaphore is a unique pointer (don't see anywhere where a semaphore is opened from a different process, but I could be wrong). But didn't find any solution. Alternative would be just to substring the semaphore name, but it could be super dangerous if two processes open the same semaphore. How would you suggest to go about solving this, if solving this at all? @falkTX

sletz commented 5 years ago

Named semaphore are mandatory, since there are used between processes (JACK server and clients).

Le ven. 7 juin 2019 à 17:27, Hlöðver Sigurðsson notifications@github.com a écrit :

Before I PR something silly, I think I've located the error to the fact that 32+ characters actually cause the semaphores to crash on darwin: https://github.com/jackaudio/jack2/blob/98c882c5a545c3dd819655c9869f6928274df8a3/posix/JackPosixSemaphore.cpp#L158

I researched unnamed semaphores, which would make sense because the sem_t* fSemaphore is a unique pointer (don't see anywhere where a semaphore is opened from a different process, but I could be wrong). But didn't find any solution. Alternative would be just to substring the semaphore name, but it could be super dangerous if two processes open the same semaphore. How would you suggest to go about solving this, if solving this at all? @falkTX https://github.com/falkTX

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/jackaudio/jack2/issues/474?email_source=notifications&email_token=AAK5HV7UFE5ADWWHHRE6UHTPZJ47LA5CNFSM4HUHW5L2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXGFHJA#issuecomment-499930020, or mute the thread https://github.com/notifications/unsubscribe-auth/AAK5HV7QRL32BO6KNMAN5RDPZJ47LANCNFSM4HUHW5LQ .

hlolli commented 5 years ago

hash the string on darwin maybe?

kmatheussen commented 5 years ago

On Fri, Jun 7, 2019 at 5:33 PM Hlöðver Sigurðsson notifications@github.com wrote:

hash the string on darwin maybe?

Yeah, I just researched it. :-) And md5 seems to work fine for this: https://stackoverflow.com/questions/41603586/xcode-c-md5-hash

Is the reason for the crash that semaphore names are cut of at 32 characters, which generates name clashes?

hlolli commented 5 years ago

I'm not sure, I'm actually a linux user and on mac I don't have the gdb (I'm sure there's an alternative). I think the sem_open takes the whole string as it is, and just crashes (returns a (sem_t*)SEM_FAILED)

hlolli commented 5 years ago

given that I remove the sprintf statement, but let me look again, I've been reverting this file so many times...

kmatheussen commented 5 years ago

On Fri, Jun 7, 2019 at 5:39 PM Hlöðver Sigurðsson notifications@github.com wrote:

I'm not sure, I'm actually a linux user and on mac I don't have the gdb (I'm sure there's an alternative).

lldb maybe?

I think the sem_open takes the whole string as it is, and just crashes (returns a (sem_t*)SEM_FAILED)

But jack seems to truncate names down to 32 characters:

https://github.com/jackaudio/jack2/blob/98c882c5a545c3dd819655c9869f6928274df8a3/posix/JackPosixSemaphore.cpp#L43

given that I remove the sprintf statement, but let me look again, I've been reverting this file so many times...

Oh, so that is your file. Okay, that explains the confusion.

hlolli commented 5 years ago

sorry for that noise, I just deleted it again, the sprintf statement, and it successfully runs with a 100+ char jack_client name. I must have been out of coffee 2 days ago :)

I'll PR that deletion

hlolli commented 5 years ago

maybe keep the js_ append?

hlolli commented 5 years ago

give me 15 mins to confim, it looks like I was right, just didn't see the error, looking at jack_lsp the 100 char jack_client_name didn't work

On Fri, 7 Jun 2019 at 17:42, Kjetil Matheussen notifications@github.com wrote:

On Fri, Jun 7, 2019 at 5:39 PM Hlöðver Sigurðsson < notifications@github.com> wrote:

I'm not sure, I'm actually a linux user and on mac I don't have the gdb (I'm sure there's an alternative).

lldb maybe?

I think the sem_open takes the whole string as it is, and just crashes (returns a (sem_t*)SEM_FAILED)

But jack seems to truncate names down to 32 characters:

https://github.com/jackaudio/jack2/blob/98c882c5a545c3dd819655c9869f6928274df8a3/posix/JackPosixSemaphore.cpp#L43

given that I remove the sprintf statement, but let me look again, I've been reverting this file so many times...

Oh, so that is your file. Okay, that explains the confusion.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jackaudio/jack2/issues/474?email_source=notifications&email_token=ABOLDARHWETB4JYPWNXFWRTPZJ6WZA5CNFSM4HUHW5L2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXGGT7A#issuecomment-499935740, or mute the thread https://github.com/notifications/unsubscribe-auth/ABOLDAUXYRPO6ZBQGK5ADJDPZJ6WZANCNFSM4HUHW5LQ .

hlolli commented 5 years ago

So the reason I got confused, was because I exceeded the 64 char limit. With this deletion I can create 32-64 char client_name, but the audio engine I was testing this on, Csound, was stopping me because of this api funciton https://github.com/jackaudio/jack2/blob/8521fbdbe1d17c5a4cffc1c16ffddd591511a4cf/common/JackAPI.cpp#L1475-L1478 so it makes total sense, and I'll PR this deletion.

On Fri, 7 Jun 2019 at 17:53, Hlöðver Sigurðsson hlolli@gmail.com wrote:

give me 15 mins to confim, it looks like I was right, just didn't see the error, looking at jack_lsp the 100 char jack_client_name didn't work

On Fri, 7 Jun 2019 at 17:42, Kjetil Matheussen notifications@github.com wrote:

On Fri, Jun 7, 2019 at 5:39 PM Hlöðver Sigurðsson < notifications@github.com> wrote:

I'm not sure, I'm actually a linux user and on mac I don't have the gdb (I'm sure there's an alternative).

lldb maybe?

I think the sem_open takes the whole string as it is, and just crashes (returns a (sem_t*)SEM_FAILED)

But jack seems to truncate names down to 32 characters:

https://github.com/jackaudio/jack2/blob/98c882c5a545c3dd819655c9869f6928274df8a3/posix/JackPosixSemaphore.cpp#L43

given that I remove the sprintf statement, but let me look again, I've been reverting this file so many times...

Oh, so that is your file. Okay, that explains the confusion.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jackaudio/jack2/issues/474?email_source=notifications&email_token=ABOLDARHWETB4JYPWNXFWRTPZJ6WZA5CNFSM4HUHW5L2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXGGT7A#issuecomment-499935740, or mute the thread https://github.com/notifications/unsubscribe-auth/ABOLDAUXYRPO6ZBQGK5ADJDPZJ6WZANCNFSM4HUHW5LQ .

falkTX commented 5 years ago

might be worth investigating if using the client uuid for the semaphore name is an option or not. it is just an integer that gets incremented for every new client, so it will be small in size (string-wise)

falkTX commented 5 years ago

also found this https://stackoverflow.com/questions/5288671/sem-open-sets-enametoolong-for-less-than-64-characters-names-on-mac-os-x-10-6-6

hlolli commented 5 years ago

interesting, I'm not getting this error, I wonder if either SEM_NAME_LEN has been changed or changed to something like sizeof(int). Going to research, hope I'm not ruining users on a recent macosx, I'm on a new Mojave.

On Fri, 7 Jun 2019 at 18:24, Filipe Coelho notifications@github.com wrote:

also found this https://stackoverflow.com/questions/5288671/sem-open-sets-enametoolong-for-less-than-64-characters-names-on-mac-os-x-10-6-6

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jackaudio/jack2/issues/474?email_source=notifications&email_token=ABOLDAUUPWLCF7QAGSCPKILPZKDUNA5CNFSM4HUHW5L2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXGKFFQ#issuecomment-499950230, or mute the thread https://github.com/notifications/unsubscribe-auth/ABOLDAUA7FKNPDDPFDFS2M3PZKDUNANCNFSM4HUHW5LQ .

hlolli commented 5 years ago

anyway, I can't seem to find any history of this on mac. On the safe side, keeping the 32 char limit makes sense. If this limit was fixed in Mac 10.6.7 or something (like 8 years ago), In that case I wouldn't see a specific reason to be so backwards compatible. You decide, I'm happy to help/test a string->hash way of naming the semaphores.

On Fri, 7 Jun 2019 at 18:36, Hlöðver Sigurðsson hlolli@gmail.com wrote:

interesting, I'm not getting this error, I wonder if either SEM_NAME_LEN has been changed or changed to something like sizeof(int). Going to research, hope I'm not ruining users on a recent macosx, I'm on a new Mojave.

On Fri, 7 Jun 2019 at 18:24, Filipe Coelho notifications@github.com wrote:

also found this https://stackoverflow.com/questions/5288671/sem-open-sets-enametoolong-for-less-than-64-characters-names-on-mac-os-x-10-6-6

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jackaudio/jack2/issues/474?email_source=notifications&email_token=ABOLDAUUPWLCF7QAGSCPKILPZKDUNA5CNFSM4HUHW5L2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXGKFFQ#issuecomment-499950230, or mute the thread https://github.com/notifications/unsubscribe-auth/ABOLDAUA7FKNPDDPFDFS2M3PZKDUNANCNFSM4HUHW5LQ .

falkTX commented 4 years ago

this is fixed now on 1.9.15/16, please give it a try. the semaphore is based on the client name, but not restricted by it. at least in theory

HaHeho commented 4 years ago

The new limitation seems to be 63 characters. Can somebody else confirm?

With 64 and above, my Python implementation still crashes in the same ungracious manner as before. But at least there is also a clear error message.

"0123456789012345678901234567890123456789012345678901234567890123" is too long to be used as a JACK client name.
Please use 63 characters or less
JackShmReadWritePtr1::~JackShmReadWritePtr1 - Init not done for -1, skipping unlock
Traceback (most recent call last):
  ...
jack.JackOpenError: Error initializing "0123456789012345678901234567890123456789012345678901234567890123": <jack.Status 0x21: failure, server_error>

In that way, I still have to catch and truncate extended names myself. Is that reasonable?

If this confirmed, I assume we could also modify the Python API to do the truncation, so any downstream implementation does not have to care?!

falkTX commented 4 years ago

Is JACK crashing or your application? (due to expecting to open a client and not dealing with the failure)

You can use jack_client_name_size before creating the client, see https://jackaudio.org/api/group__ClientFunctions.html#gab8b16ee616207532d0585d04a0bd1d60

I did not check this, but it should be returning 63 on macOS (otherwise it is a bug)

HaHeho commented 4 years ago

Is JACK crashing or your application? (due to expecting to open a client and not dealing with the failure)

Just the application.

You can use jack_client_name_size before creating the client, see https://jackaudio.org/api/group__ClientFunctions.html#gab8b16ee616207532d0585d04a0bd1d60

I did not check this, but it should be returning 63 on macOS (otherwise it is a bug)

Good to know. I will check on how to access the API function in order to truncate names during startup.

h1z1 commented 4 years ago

Can you clarify if exceeding this limit is meant to be an error or is jackd supposed to truncate it internally?

It is a hard fail in JackInternalClient and JackLibClient for example.

ffmpeg appears to assume otherwise as it blindly uses the current source url as a name by default

    /* Register as a JACK client, using the context url as client name. */
    self->client = jack_client_open(context->url, JackNullOption, &status);
    if (!self->client) {
        av_log(context, AV_LOG_ERROR, "Unable to register as a JACK client\n");
        return AVERROR(EIO);
    }

Though it has a fallback, cubeb which is the backend for audio in Firefox also assumes that:

  const char * jack_client_name = "cubeb";
  if (context_name)
    jack_client_name = context_name;

  ctx->jack_client = api_jack_client_open(jack_client_name,
                                          JackNoStartServer,
                                          NULL);
falkTX commented 4 years ago

This is meant to be a hard fail yes. The client name must not be greater than the value returned by jack_client_name_size()

Documentation for jack_client_open says:

client_name: of at most jack_client_name_size() characters

applications need to be aware of that. carla for example takes care of this in multi-client mode by always generating a name that is very likely to be unique, within the limits of the client name size. see https://github.com/falkTX/Carla/blob/master/source/backend/engine/CarlaEngine.cpp#L1048

h1z1 commented 4 years ago

Based on the crude searching I did yesterday, Carla is a rarity. Most applications do not. The assume jack will truncate it.

falkTX commented 4 years ago

In some cases yes, but that is a bad assumption to make. Linux and macOS should be able to cope with this, but Windows might be the last one needing some attention.

Even then, if application does not take the client name size into consideration, it likely will also not take into account that the name assigned to it by jack might be different from what was requested (this is why an open flag exists for asking a unique name)

h1z1 commented 4 years ago

From what I can find they don't though.. not even what I'd consider the the three major media players in Linux do

App         | Lib       | jack_client_open  | jack_client_name_size | file
--------------------------------------------------------------------------

audacity    | Portaudio | yes               | yes
lib-src/portaudio-v19/src/hostapi/jack/pa_jack.cjack_client_open
========================================================
PaJack_SetClientName->jack_client_name_size

Carla   | own   | yes | Yes
============================

Patchage | own  | yes | NO
============================

freqtweak  | own | yes |  NO
============================
   const char *server_name = NULL;

baudline | own | yes | no
============================

jackd-jamin | own | yes | NO
============================
  - has specific warn that jackd might change the name
  char *client_name = NULL;
  client = jack_client_open(client_name, JackNullOption, &status);

alsa jackplugin | own | yes | NO
============================
  - has warning
  char jack_client_name[32];

libsdl | own | yes | NO
============================
 - hard coded to "SDL"
 - warning re unimplemented name api

 client = JACK_jack_client_open("SDL", JackNoStartServer, &status, NULL);

OBS | own | yes | NO
============================
 char *device;

ffmpeg | libavdevice | yes | NO
==================================
  libavdevice/jack.c: self->client = jack_client_open(context->url, JackNullOption, &status);

mpv-native | own | yes | NO
============================
char *client_name;
  p->client = jack_client_open(p->opts->client_name, open_options, NULL);

mpv-libavdevice | libavdevice | yes | NO
=========================================

VLC | own | yes | NO
========================================================
    p_sys->p_jack_client = jack_client_open( psz_name,
                 JackNullOption | JackNoStartServer,
                 NULL );

GStreamer | own | yes | NO
========================================================
    src_client = jack_client_open ("src_client", JackNoStartServer, &status);
falkTX commented 4 years ago

not all of those need to care though. if the default client name is just "mpv" or something small, they will not run into issues. the problem is only when it allows user input without checking further.

h1z1 commented 4 years ago

Indeed and it's mpv's case I noticed it. You can specify the name with --jack-name and --jack-port. Firefox is able to cause some serious instability in both jackd and attached clients too.

Couple other weird things about this, can you confirm the intent behind JackUseExactName given the above?

 @param client_name of at most jack_client_name_size() characters.
 * The name scope is local to each server.  Unless forbidden by the
 * @ref JackUseExactName option, the server will modify this name to create a unique variant, if needed.

I can't find any reference to jack_client_name_size being used in the example applications either.