macOS: Reset Mach exception ports in PTY child pre_exec#8835
macOS: Reset Mach exception ports in PTY child pre_exec#8835smitbarmase wants to merge 1 commit intoalacritty:masterfrom
pre_exec#8835Conversation
66912d1 to
b97b156
Compare
Follow-up to #47420 and #44193. #47420 fixed child-exit status handling via upstream alacritty change (alacritty/alacritty#8825). However, stable/preview/nightly builds still reproduced hangs that dev builds did not. The root cause matches #44193: spawned children inherit crash-handler Mach exception ports and hang. We already reset exception ports in `util::command::new_smol_command` and `util::set_pre_exec_to_start_new_session`, but PTY child spawning in `alacritty_terminal` was not covered. This PR updates `alacritty_terminal` to include PTY `pre_exec` exception-port reset: zed-industries/alacritty@9d9640d Upstream: - alacritty/alacritty#8835 Release Notes: - Fixed terminal tasks hanging on macOS when a spawned process is killed by a signal.
chrisduerr
left a comment
There was a problem hiding this comment.
What an atrocious way to do crash handling.
| } | ||
|
|
||
| #[cfg(target_os = "macos")] | ||
| reset_exception_ports(); |
There was a problem hiding this comment.
You're right to point this out. It made me question this as well. As far as I've researched, I don't think it's async-signal-safe. However, we're doing exactly this in Zed, and it works for us. Now that I think about it, maybe we should also eliminate this and find a workaround that doesn't reset the exception port in pre_exec. I'll close this for now, but we'll continue using the patched version until we find a workaround.
There was a problem hiding this comment.
Yeah I'm happy to review anything that tries to work around this, but unfortunately "we're doing exactly this in Zed, and it works for us" is kind of a poor approach since async signal safety is inherently unreliable at causing issues due to being racy. Alacritty admittedly did the exact same thing for a long time but just recently someone actually tried to improve things and I'd hate to undo their work just to go back to the "it works fine 99.9% of the time" mentality.
| behavior: exception_behavior_t, | ||
| new_flavor: thread_state_flavor_t, | ||
| ) -> kern_return_t; | ||
| } |
There was a problem hiding this comment.
I feel like this belongs in a different module.
There was a problem hiding this comment.
The module isn't too big, so it might be fine either way, but I suspect this to be unsafe anyway.
On macOS, task exception ports are inherited across
fork. If a parent process installs exception ports (e.g., for crash reporting), PTY child processes inherit them.This adds a macOS-only reset in the PTY child
pre_exechook so that child processes start with exception ports restored to system defaults (MACH_PORT_NULL).See also: