Switch to posix_spawn on Apple targets in SystemNative_ForkAndExecProcess#126063
Switch to posix_spawn on Apple targets in SystemNative_ForkAndExecProcess#126063Copilot wants to merge 2 commits intocopilot/add-standard-input-output-propertiesfrom
Conversation
…cess Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com> Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/b44dc898-efab-438c-9b0b-3b8e22d1c901
Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com> Agent-Logs-Url: https://github.com/dotnet/runtime/sessions/b44dc898-efab-438c-9b0b-3b8e22d1c901
There was a problem hiding this comment.
Pull request overview
This PR updates SystemNative_ForkAndExecProcess to use posix_spawn on Apple targets (intended for macOS per description) as an alternative to the existing fork/exec implementation when credentials don’t need to be set.
Changes:
- Add Apple-only
posix_spawnfast path (whensetCredentialsis false), including working-directory and stdio redirection setup. - Configure spawn attributes for signal defaults and signal mask behavior.
- Add
<spawn.h>include for Apple builds.
| // Reset all signal handlers to default | ||
| sigset_t all_signals; | ||
| sigfillset(&all_signals); | ||
| if ((result = posix_spawnattr_setsigdefault(&attr, &all_signals)) != 0) |
There was a problem hiding this comment.
The posix_spawn signal-default setup resets all signals to SIG_DFL via sigfillset. This diverges from the existing fork path which preserves SIG_IGN handlers and skips signals like SIGKILL/SIGSTOP when adjusting handlers. To preserve behavior and avoid invalid entries in the sigdefault set, build the sigdefault set based on the current handlers (only signals with custom handlers), and exclude non-overridable signals.
| // Set the child's signal mask to empty (unblock all signals) | ||
| sigset_t no_signals; | ||
| sigemptyset(&no_signals); | ||
| if ((result = posix_spawnattr_setsigmask(&attr, &no_signals)) != 0) | ||
| { |
There was a problem hiding this comment.
The posix_spawn path sets the child signal mask to an empty set (unblocks everything). The existing fork/vfork path restores the parent’s pre-fork mask (old_signal_set) in the child before exec. To keep parity, capture the current thread signal mask in the parent and pass that to posix_spawnattr_setsigmask instead of forcing an empty mask.
| // Set the child's signal mask to empty (unblock all signals) | |
| sigset_t no_signals; | |
| sigemptyset(&no_signals); | |
| if ((result = posix_spawnattr_setsigmask(&attr, &no_signals)) != 0) | |
| { | |
| // Set the child's signal mask to match the parent's current mask | |
| sigset_t current_mask; | |
| int sigmaskResult = pthread_sigmask(0, NULL, ¤t_mask); | |
| if (sigmaskResult != 0) | |
| { | |
| int saved_errno = sigmaskResult; | |
| posix_spawnattr_destroy(&attr); | |
| errno = saved_errno; | |
| return -1; | |
| } | |
| if ((result = posix_spawnattr_setsigmask(&attr, ¤t_mask)) != 0) | |
| { |
| // Make sure we can find and access the executable. | ||
| if (access(filename, X_OK) != 0) | ||
| { | ||
| return -1; | ||
| } | ||
|
|
||
| pid_t spawnedPid; | ||
| posix_spawn_file_actions_t file_actions; | ||
| posix_spawnattr_t attr; | ||
| int result; | ||
|
|
||
| if ((result = posix_spawnattr_init(&attr)) != 0) | ||
| { | ||
| errno = result; | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
On several error returns in the posix_spawn fast path (e.g., access failure, posix_spawnattr_init failure, or later setup failures), *childPid is left unchanged. The existing fork path sets all out args (including *childPid = -1) on any failure. For consistent caller behavior, ensure *childPid is set to -1 on every failure return in this block.
| if ((stdinFd != -1 && (result = posix_spawn_file_actions_adddup2(&file_actions, stdinFd, STDIN_FILENO)) != 0) | ||
| || (stdoutFd != -1 && (result = posix_spawn_file_actions_adddup2(&file_actions, stdoutFd, STDOUT_FILENO)) != 0) | ||
| || (stderrFd != -1 && (result = posix_spawn_file_actions_adddup2(&file_actions, stderrFd, STDERR_FILENO)) != 0)) |
There was a problem hiding this comment.
The fork path skips dup2 when the provided fd already equals STDIN_FILENO/STDOUT_FILENO/STDERR_FILENO. Here, posix_spawn_file_actions_adddup2 is called even when stdinFd == STDIN_FILENO (etc.). For parity (and to avoid any platform-specific EINVAL behavior for no-op dup2 actions), add the same fd != STD*_FILENO checks before adding the spawn file actions.
| if ((stdinFd != -1 && (result = posix_spawn_file_actions_adddup2(&file_actions, stdinFd, STDIN_FILENO)) != 0) | |
| || (stdoutFd != -1 && (result = posix_spawn_file_actions_adddup2(&file_actions, stdoutFd, STDOUT_FILENO)) != 0) | |
| || (stderrFd != -1 && (result = posix_spawn_file_actions_adddup2(&file_actions, stderrFd, STDERR_FILENO)) != 0)) | |
| if ((stdinFd != -1 && stdinFd != STDIN_FILENO && (result = posix_spawn_file_actions_adddup2(&file_actions, stdinFd, STDIN_FILENO)) != 0) | |
| || (stdoutFd != -1 && stdoutFd != STDOUT_FILENO && (result = posix_spawn_file_actions_adddup2(&file_actions, stdoutFd, STDOUT_FILENO)) != 0) | |
| || (stderrFd != -1 && stderrFd != STDERR_FILENO && (result = posix_spawn_file_actions_adddup2(&file_actions, stderrFd, STDERR_FILENO)) != 0)) |
| #if defined(__APPLE__) && !defined(TARGET_MACCATALYST) && !defined(TARGET_TVOS) | ||
| // Use posix_spawn on macOS when credentials don't need to be set, | ||
| // since macOS does not support setuid/setgid with posix_spawn. | ||
| if (!setCredentials) |
There was a problem hiding this comment.
The __APPLE__ guard here also includes iOS/watchOS targets (it only excludes MacCatalyst and tvOS). That doesn’t match the PR description (“on macOS”) and could inadvertently enable this posix_spawn path on platforms where Process/exec support is intentionally limited. Consider using a macOS-specific define (e.g., TARGET_OSX/TARGET_OSX && !TARGET_MACCATALYST) or explicitly excluding TARGET_IOS/TARGET_WATCHOS as appropriate for this repo’s Apple target macros.
Description
Uses
posix_spawninstead offork/execinSystemNative_ForkAndExecProcesson macOS (excluding MacCatalyst and tvOS).#if defined(__APPLE__) && !defined(TARGET_MACCATALYST) && !defined(TARGET_TVOS), placed as the first code block in the functionsetCredentialsis true, falls through to the existingfork/execpath since macOSposix_spawndoes not supportsetuid/setgidPOSIX_SPAWN_SETSIGDEF(reset all handlers to default) andPOSIX_SPAWN_SETSIGMASK(empty mask, unblock all signals) for parity with the fork pathposix_spawn_file_actions_adddup2for stdin/stdout/stderr, skipping when fd is-1posix_spawn_file_actions_addchdir_npdirectly (always available on supported macOS versions, noHAVE_constant)POSIX_SPAWN_CLOEXEC_DEFAULT(separate PR), nocreate_suspended, nocreate_new_process_group