Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

macOS: Pass JackMachSemaphore send right via mach_msg IPC #788

Merged
merged 3 commits into from
Aug 14, 2021

Conversation

p--b
Copy link
Contributor

@p--b p--b commented Aug 8, 2021

Previously, JackMachSemaphore would communicate the send right for the
semaphore object from the server to a client via a named service
registered via bootstrap_register. However, to do this, it would
register the semaphore's port as the service port directly.

In theory this ought to be fine, however in practice, macOS launchd,
which provides the bootstrap_register interface, does not correctly
detect when such a port becomes dead, and incorrectly believes that the
service that it provides is forever alive, even past the end of the
jackd process' (and therefore the semaphore's) existence. This seems
to be specific to semaphore ports, as launchd is expecting a
standard IPC port, owned by the task, not the kernel. This prevents
jackd from later registering another service with the same name, as
launchd rejects the registration as conflicting with an active service.

To get around this, jackd previously added a counter to the end of the
named service registrations, allowing old services to remain in the
system until the end of the session. To prevent things getting out of
hand, this was capped at 98 service registrations for a given semaphore
name. This led to #784, in which running a client for the 99th time
resulted in the semaphore creation failing and the client failing to
connect.

As launchd outlives multiple runs of jackd, this situation persisted
across restarts of jackd, requiring a restart of the user's session
(i.e. a reboot) to fix.

An initial attempt at fixing this (see #785) tried passing the port
rights directly via shared memory, however mach is too clever for us and
foils that plan by having port names be looked up in a per-task table
(sensible when you think about it).

In this commit, we use mach IPC messages to transfer the send right for
the semaphore from the server to the client. By registering a standard IPC
port with the bootstrap server, the service registrations are correctly torn
down when the ports are destroyed.

It works something like this:

  • Server creates IPC port and registers it globally via bootstrap_register
  • Server listens on IPC port for messages
  • Client looks up IPC port via bootstrap_look_up
  • Client sends it a message
  • Server replies with a message containing a send right to the
    semaphore's port
  • Client is then free to use the semaphore port as before.

This resolves #784.

@p--b
Copy link
Contributor Author

p--b commented Aug 8, 2021

@falkTX as promised - PTAL

Previously, JackMachSemaphore would communicate the send right for the
semaphore object from the server to a client via a named service
registered via `bootstrap_register`. However, to do this, it would
register the semaphore's port as the service port directly.

In theory this ought to be fine, however in practice, macOS `launchd`,
which provides the `bootstrap_register` interface, does not correctly
detect when such a port becomes dead, and incorrectly believes that the
service that it provides is forever alive, even past the end of the
`jackd` process' (and therefore the semaphore's) existence. This seems
to be *specific* to semaphore ports, as `launchd` is expecting a
standard IPC port, owned by the task, not the kernel. This prevents
`jackd` from later registering another service with the same name, as
`launchd` rejects the registration as conflicting with an active service.

To get around this, `jackd` previously added a counter to the end of the
named service registrations, allowing old services to remain in the
system until the end of the session. To prevent things getting out of
hand, this was capped at 98 service registrations for a given semaphore
name. This led to jackaudio#784, in which running a client for the 99th time
resulted in the semaphore creation failing and the client failing to
connect.

As `launchd` outlives multiple runs of `jackd`, this situation persisted
across restarts of `jackd`, requiring a restart of the user's session
(i.e. a reboot) to fix.

An initial attempt at fixing this (see jackaudio#785) tried passing the port
rights directly via shared memory, however mach is too clever for us and
foils that plan by having port names be looked up in a per-task table
(sensible when you think about it).

In this commit, we use mach IPC messages to transfer the send right for
the semaphore from the server to the client. By registering a standard
IPC port with the bootstrap server, the service registrations are
correctly torn down when the ports are destroyed.

It works something like this:

* Server creates IPC port and registers it globally via `bootstrap_register`
* Server listens on IPC port for messages
* Client looks up IPC port via `bootstrap_look_up`
* Client sends it a message
* Server replies with a message containing a send right to the
semaphore's port
* Client is then free to use the semaphore port as before.

This resolves jackaudio#784.
@p--b p--b force-pushed the semaphore-message branch from 4cf582a to 7f66a3d Compare August 9, 2021 00:05
@falkTX
Copy link
Member

falkTX commented Aug 9, 2021

Overall looks good, thanks for the extra code comments!
One thing though, there is no cleanup on error at all in the JackMachSemaphore class.
If bootstrap process goes ok but thread fails to start, we need to cleanup the bootstrap stuff. Otherwise after 1 single error in the system jack becomes unusable for certain clients.

@p--b
Copy link
Contributor Author

p--b commented Aug 9, 2021

Ah, so if Allocate() returns false then Destroy() won't be called? I see what you mean - will add

@falkTX
Copy link
Member

falkTX commented Aug 9, 2021

Ah, so if Allocate() returns false then Destroy() won't be called?

yes. like you dont call free() on a failed malloc.
thanks

@p--b
Copy link
Contributor Author

p--b commented Aug 9, 2021

This should be better, PTAL

@p--b
Copy link
Contributor Author

p--b commented Aug 10, 2021

Looks like that CI failure might be spurious?

@falkTX
Copy link
Member

falkTX commented Aug 10, 2021

yes, for some reason xvfb-run randomly fails to start sometimes

@sletz
Copy link
Member

sletz commented Aug 11, 2021

Nice to see this old Mach based code properly reworked. @p--b thanks for that, since having a precise knowledge of those exotic Mach APIs is quite rare ((-;

@p--b
Copy link
Contributor Author

p--b commented Aug 11, 2021

@sletz I wouldn't assume I have too much precise knowledge... I just really enjoy reading old Apple opensource listings ;)

@falkTX
Copy link
Member

falkTX commented Aug 14, 2021

Gave this a try, all seems to be working as far as I can tell.
Merging, thanks a lot!

@falkTX falkTX merged commit 1ab3445 into jackaudio:develop Aug 14, 2021
@vrslev
Copy link

vrslev commented Jan 12, 2022

@p--b @falkTX When this fix will be included in stable release?

@falkTX
Copy link
Member

falkTX commented Jan 12, 2022

in 3 days

@vrslev
Copy link

vrslev commented Jan 12, 2022

@falkTX Thanks!

falkTX pushed a commit that referenced this pull request Jan 15, 2022
* macOS: Pass JackMachSemaphore send right via mach_msg IPC

Previously, JackMachSemaphore would communicate the send right for the
semaphore object from the server to a client via a named service
registered via `bootstrap_register`. However, to do this, it would
register the semaphore's port as the service port directly.

In theory this ought to be fine, however in practice, macOS `launchd`,
which provides the `bootstrap_register` interface, does not correctly
detect when such a port becomes dead, and incorrectly believes that the
service that it provides is forever alive, even past the end of the
`jackd` process' (and therefore the semaphore's) existence. This seems
to be *specific* to semaphore ports, as `launchd` is expecting a
standard IPC port, owned by the task, not the kernel. This prevents
`jackd` from later registering another service with the same name, as
`launchd` rejects the registration as conflicting with an active service.

To get around this, `jackd` previously added a counter to the end of the
named service registrations, allowing old services to remain in the
system until the end of the session. To prevent things getting out of
hand, this was capped at 98 service registrations for a given semaphore
name. This led to #784, in which running a client for the 99th time
resulted in the semaphore creation failing and the client failing to
connect.

As `launchd` outlives multiple runs of `jackd`, this situation persisted
across restarts of `jackd`, requiring a restart of the user's session
(i.e. a reboot) to fix.

An initial attempt at fixing this (see #785) tried passing the port
rights directly via shared memory, however mach is too clever for us and
foils that plan by having port names be looked up in a per-task table
(sensible when you think about it).

In this commit, we use mach IPC messages to transfer the send right for
the semaphore from the server to the client. By registering a standard
IPC port with the bootstrap server, the service registrations are
correctly torn down when the ports are destroyed.

It works something like this:

* Server creates IPC port and registers it globally via `bootstrap_register`
* Server listens on IPC port for messages
* Client looks up IPC port via `bootstrap_look_up`
* Client sends it a message
* Server replies with a message containing a send right to the
semaphore's port
* Client is then free to use the semaphore port as before.

This resolves #784.

* Improve error handling

* Add myself to Authors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

macOS: clients can only connect 98 times per boot
4 participants