posix spawn, take 2 by vtjnash · Pull Request #3257 · libuv/libuv

vtjnash · 2021-07-26T18:30:20Z

This is a commit series to re-enable the usage of posix_spawn on macOS.

CI: https://ci.nodejs.org/job/libuv-test-commit-osx-cmake/359/
CI+node: https://ci.nodejs.org/job/libuv-in-node/191/

vtjnash · 2021-07-27T17:41:07Z

darn, of course we would now run into what looks like another pre-existing nodejs/kernel bug more frequently now 😂

deepak1556 · 2021-08-02T22:24:28Z

@vtjnash there is also an issue with posix_spawnp code path when used with posix_spawn_file_actions_addchdir_np which turns out to be a bug in the implementation. Originally caught in microsoft/vscode#118267 (comment). I am using pthread_chdir_np as a workaround.

diff --git a/src/unix/process.c b/src/unix/process.c
index 0fb9fda1..ef290d0e 100644
--- a/src/unix/process.c
+++ b/src/unix/process.c
@@ -366,6 +366,28 @@ static uv_once_t posix_spawn_init_once = UV_ONCE_INIT;
 static uv__posix_spawn_fncs_t posix_spawn_fncs;
 static int posix_spawn_can_use_setsid;
 
+static int uv__change_current_thread_directory(const char* path) {
+  // Changes the current thread's directory to a path or directory file
+  // descriptor. libpthread only exposes a syscall wrapper starting in
+  // macOS 10.12, but the system call dates back to macOS 10.5. On older OSes,
+  // the syscall is issued directly.
+  if (__builtin_available(macOS 10.12, *)) {
+    return pthread_chdir_np(path);
+  } else {
+    return syscall(SYS___pthread_chdir, path);
+  }
+}
+
+// The recommended way to unset a per-thread cwd is to set a new value to an
+// invalid file descriptor, per libpthread-218.1.3/private/private.h.
+static int uv__reset_current_thread_directory() {
+  if (__builtin_available(macOS 10.12, *)) {
+    return pthread_fchdir_np(-1);
+  } else {
+    return syscall(SYS___pthread_fchdir, -1);
+  }
+}
+
 
 void uv__spawn_init_posix_spawn_fncs(void) {
   /* Try to locate all non-portable functions at runtime */
@@ -501,12 +523,23 @@ int uv__spawn_set_posix_spawn_file_actions(posix_spawn_file_actions_t* actions,
 
   /* Set the current working directory if requested */
   if (options->cwd != NULL) {
-    if (posix_spawn_fncs->file_actions.addchdir_np == NULL) {
-      err = ENOSYS;
-      goto error;
-    }
+    if (strchr(options->file, '/') != NULL) {
+      /* There is a bug in macOS where a relative executable
+       * path like "../myprogram" will cause `posix_spawnp` to
+       * successfully launch the program, but erroneously return
+       * ENOENT when used with posix_spawn_file_actions_addchdir_np
+       * because of the stat call performed on the executable path
+       * which was introduced in macOS 10.15.
+       * Ref: https://opensource.apple.com/source/Libc/Libc-1439.40.11/sys/posix_spawn.c.auto.html */
+      err = uv__change_current_thread_directory(options->cwd);
+    } else {
+      if (posix_spawn_fncs->file_actions.addchdir_np == NULL) {
+        err = ENOSYS;
+        goto error;
+      }
 
-    err = posix_spawn_fncs->file_actions.addchdir_np(actions, options->cwd);
+      err = posix_spawn_fncs->file_actions.addchdir_np(actions, options->cwd);
+    }
     if (err != 0)
       goto error;
   }
@@ -628,6 +661,9 @@ int uv__spawn_resolve_and_spawn(const uv_process_options_t* options,
     do
       err = posix_spawnp(pid, options->file, actions, attrs, options->args, env);
     while (err == EINTR);
+    /* Restore the thread's working directory if it was changed. */
+    if (options->cwd != NULL)
+      uv__reset_current_thread_directory();
     return err;
   }

vtjnash · 2021-08-09T17:33:51Z

The bug here sounds almost identical to this reported problem from 2006: https://gcc.gnu.org/legacy-ml/gcc/2006-09/msg00631.html. Is there anyone here who can tell me if rdar://problem/4736203 is still a known issue in the mach kernel? I can provide code to reproduce, but currently involves re-building all of nodejs with this PR and then running one particular unit test a small number of times.

vtjnash · 2021-08-17T23:56:48Z

I can reproduce this kernel bug without libuv also simply by calling posix_spawn a few times:
https://gist.github.com/vtjnash/679174286cf282c298e5d11752343b8a

The specific conditions under which it occurs seems to be if posix_spawn returns an error, it will revoke any pending SIGCHLD, so we might be able to work around it by calling if (posix_spawn()) kill(getpid(), SIGCHLD);, but it is awful and incomplete, and really seems to require an Apple kernel developer to correct this.

EDIT: filed FB9529664 with Apple

jpcanepa · 2021-08-20T18:29:32Z

I hope this time the code lands. Ping me if I can help somehow, I may manage to squeeze some time.

deepak1556 · 2021-08-30T15:12:17Z

Adding here for visibility, FB9578526 tracks #3257 (comment)

KishanBagaria · 2021-10-20T13:43:22Z

What's blocking this – how can we help?

vtjnash · 2021-10-20T16:00:27Z

Apple has confirmed this triggers an old kernel bug in xnu, but has not yet gotten back to me about fixing it.

KishanBagaria · 2021-10-20T18:15:05Z

#3064 (comment) says "we had to revert this because it was causing failures in the Node.js test suite."

Can we use posix_spawn when it's working fine (not triggering the bug / failure in test suite) and the original function in all other cases as a stopgap? This should be what most people want until we have the proper fix.

vtjnash · 2021-10-20T21:12:52Z

It is a show-stopper-type bug that breaks all usages.

KishanBagaria · 2021-10-20T21:16:38Z

Ah that's unfortunate, is the bug also present in the patch that VS Code and others apply?

vtjnash · 2021-10-20T23:55:57Z

It is present in all programs that eventually want to exit.

1bitphoenix · 2021-11-11T03:41:32Z

@vtjnash
Can you please provide some context/idea around what that old kernel bug in xnu bug actually is, if it isn't resolved by the patch #3257 (comment)?

vtjnash · 2021-11-11T15:52:49Z

mach will sometimes fail to deliver signals as required, and as needed for proper function of uv_spawn. This seems to be greatly exacerbated by using posix_spawn. A partial workaround would be to switch to kevent notifications of process exit, but that is possibly a non-trivial amount of work, and only reduces the scope of the issue, but doesn't solve it.

Fixes: libuv#3050 Refs: libuv#3086 Refs: libuv#3064 Refs: libuv#3107 Refs: libuv#3064 This reverts commit 217fdf4, then fixes several issues with it: * remove error fast-cleanup code that triggers a nodejs bug Refs: libuv#3107 (comment) * protect posix_spawn from EINTR This is not a documented valid error, but seems to have been observed. * ignore setuid/setgid syscall This kernel function is not permitted unless the process is setuid root, so disable this syscall. Falling back to fork/exec should be okay for the rare cases that the user decides they need to do setuid(getuid()) or setuid(geteuid()) for the child. Refs: libuv#3107 (comment) * improve posix_spawn path search Ports the improvements in musl back to this function * fix some additional problems and formatting issues We previously might fail to start a watcher, in rare failure cases, resulting in a zombie that we would fail to kill. Also avoid creating the signal-pipe unless required (addresses a review comment from Apple) * fix fd->fd mapping reuse There was a chance that when duplicating the fd's into stdio_count+fd we might be closing a currently opened fd with that value.

Fix a regression that sneaked into posix spawn changes. Refs: libuv#3257

Remove the `TARGET_OS_IPHONE` ifdef to include posix spawn headers for iOS build. Previously libuv#3257 introduced posix spawn with \_\_APPLE\_\_ platform only, which resulted in a number of spawn related definitions not found for iOS (such as `uv__posix_spawn_fncs_tag`). Co-authored-by: Jameson Nash <[email protected]>

Notable changes: - Build regression fixes for various platform updates (libuv/libuv#3428, libuv/libuv#3419, libuv/libuv#3423, libuv/libuv#3413, libuv/libuv#3431) - Support for GNU/Hurd (libuv/libuv#3450) - Release tool improvements (libuv/libuv-release-tool#13) - Better performing rw locks on Win32 (libuv/libuv#3383) - Support for posix_spawn API (libuv/libuv#3257) - Fix regression on OpenBSD (libuv/libuv#3506) - Add uv_available_parallelism() (libuv/libuv#3499) - Don't use thread-unsafe strtok() (libuv/libuv#3524) - Fix hang after NOTE_EXIT (libuv/libuv#3521) - Better align order-of-events behavior between platforms (libuv/libuv#3598) - Fix fs event not fired if the watched file is moved/removed/recreated (libuv/libuv#3540) - Fix pipe resource leak if closed during connect (and other bugs) (libuv/libuv#3611) - Don't error when killing a zombie process (libuv/libuv#3625) - Avoid posix_spawnp() cwd bug (libuv/libuv#3597) - Skip EVFILT_PROC events when invalidating events for an fd (libuv/libuv#3629) Fixes: nodejs#42290

Notable changes: - Build regression fixes for various platform updates (libuv/libuv#3428, libuv/libuv#3419, libuv/libuv#3423, libuv/libuv#3413, libuv/libuv#3431) - Support for GNU/Hurd (libuv/libuv#3450) - Release tool improvements (libuv/libuv-release-tool#13) - Better performing rw locks on Win32 (libuv/libuv#3383) - Support for posix_spawn API (libuv/libuv#3257) - Fix regression on OpenBSD (libuv/libuv#3506) - Add uv_available_parallelism() (libuv/libuv#3499) - Don't use thread-unsafe strtok() (libuv/libuv#3524) - Fix hang after NOTE_EXIT (libuv/libuv#3521) - Better align order-of-events behavior between platforms (libuv/libuv#3598) - Fix fs event not fired if the watched file is moved/removed/recreated (libuv/libuv#3540) - Fix pipe resource leak if closed during connect (and other bugs) (libuv/libuv#3611) - Don't error when killing a zombie process (libuv/libuv#3625) - Avoid posix_spawnp() cwd bug (libuv/libuv#3597) - Skip EVFILT_PROC events when invalidating events for an fd (libuv/libuv#3629) Fixes: #42290 PR-URL: #42340 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

Notable changes: - Build regression fixes for various platform updates (libuv/libuv#3428, libuv/libuv#3419, libuv/libuv#3423, libuv/libuv#3413, libuv/libuv#3431) - Support for GNU/Hurd (libuv/libuv#3450) - Release tool improvements (libuv/libuv-release-tool#13) - Better performing rw locks on Win32 (libuv/libuv#3383) - Support for posix_spawn API (libuv/libuv#3257) - Fix regression on OpenBSD (libuv/libuv#3506) - Add uv_available_parallelism() (libuv/libuv#3499) - Don't use thread-unsafe strtok() (libuv/libuv#3524) - Fix hang after NOTE_EXIT (libuv/libuv#3521) - Better align order-of-events behavior between platforms (libuv/libuv#3598) - Fix fs event not fired if the watched file is moved/removed/recreated (libuv/libuv#3540) - Fix pipe resource leak if closed during connect (and other bugs) (libuv/libuv#3611) - Don't error when killing a zombie process (libuv/libuv#3625) - Avoid posix_spawnp() cwd bug (libuv/libuv#3597) - Skip EVFILT_PROC events when invalidating events for an fd (libuv/libuv#3629) Fixes: nodejs#42290 PR-URL: nodejs#42340 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

Notable changes: - Build regression fixes for various platform updates (libuv/libuv#3428, libuv/libuv#3419, libuv/libuv#3423, libuv/libuv#3413, libuv/libuv#3431) - Support for GNU/Hurd (libuv/libuv#3450) - Release tool improvements (libuv/libuv-release-tool#13) - Better performing rw locks on Win32 (libuv/libuv#3383) - Support for posix_spawn API (libuv/libuv#3257) - Fix regression on OpenBSD (libuv/libuv#3506) - Add uv_available_parallelism() (libuv/libuv#3499) - Don't use thread-unsafe strtok() (libuv/libuv#3524) - Fix hang after NOTE_EXIT (libuv/libuv#3521) - Better align order-of-events behavior between platforms (libuv/libuv#3598) - Fix fs event not fired if the watched file is moved/removed/recreated (libuv/libuv#3540) - Fix pipe resource leak if closed during connect (and other bugs) (libuv/libuv#3611) - Don't error when killing a zombie process (libuv/libuv#3625) - Avoid posix_spawnp() cwd bug (libuv/libuv#3597) - Skip EVFILT_PROC events when invalidating events for an fd (libuv/libuv#3629) Fixes: #42290 PR-URL: #42340 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>

This adds a workaround for an xnu kernel bug that sometimes results in SIGCHLD not being delivered. The workaround is to use kevent to listen for EVFILT_PROC/NOTE_EXIT events instead of relying on SIGCHLD on *BSD. Apple rdar: FB9529664 Refs: libuv/libuv#3257

The kernel may return ESRCH if the child has already exited here. This is rather annoying, and means we must indirectly handle notification to our event loop of the process exit. Refs: libuv/libuv#3441 Refs: libuv/libuv#3257

Fix a regression that sneaked into posix spawn changes. Refs: libuv/libuv#3257

Remove the `TARGET_OS_IPHONE` ifdef to include posix spawn headers for iOS build. Previously libuv/libuv#3257 introduced posix spawn with \_\_APPLE\_\_ platform only, which resulted in a number of spawn related definitions not found for iOS (such as `uv__posix_spawn_fncs_tag`). Co-authored-by: Jameson Nash <[email protected]>

This adds a workaround for an xnu kernel bug that sometimes results in SIGCHLD not being delivered. The workaround is to use kevent to listen for EVFILT_PROC/NOTE_EXIT events instead of relying on SIGCHLD on *BSD. Apple rdar: FB9529664 Refs: libuv/libuv#3257

The kernel may return ESRCH if the child has already exited here. This is rather annoying, and means we must indirectly handle notification to our event loop of the process exit. Refs: libuv/libuv#3441 Refs: libuv/libuv#3257

Fix a regression that sneaked into posix spawn changes. Refs: libuv/libuv#3257

Remove the `TARGET_OS_IPHONE` ifdef to include posix spawn headers for iOS build. Previously libuv/libuv#3257 introduced posix spawn with \_\_APPLE\_\_ platform only, which resulted in a number of spawn related definitions not found for iOS (such as `uv__posix_spawn_fncs_tag`). Co-authored-by: Jameson Nash <[email protected]>

vtjnash commented Jul 27, 2021

View reviewed changes

Comment thread src/unix/process.c Outdated

vtjnash commented Jul 27, 2021

View reviewed changes

Comment thread src/unix/process.c

vtjnash mentioned this pull request Jul 27, 2021

unix: protect fork in uv_spawn from signals #3251

Merged

This was referenced Aug 27, 2021

Include cwd in terminal title microsoft/vscode#29816

Closed

Unable to open files in relative directories from the Terminal microsoft/vscode#116097

Closed

vs code spawns lsof processes which gobble up CPU microsoft/vscode#78438

Closed

This was referenced Sep 8, 2021

child_process spawn locks up renderer for 300 - 3000ms on signed app on macOS Big Sur electron/electron#26143

Closed

Slow spawn from ext host process when built from sources microsoft/vscode#132654

Closed

sergiou87 mentioned this pull request Sep 17, 2021

Upgrade to Electron v13 desktop/desktop#12978

Closed

stale bot added the stale label Sep 21, 2021

vtjnash commented Sep 24, 2021

View reviewed changes

Comment thread src/unix/process.c Outdated

stale bot removed the stale label Sep 24, 2021

mweststrate mentioned this pull request Sep 29, 2021

Bump electron from 11.2.3 to 13.1.7 in /desktop facebook/flipper#2606

Closed

libuv deleted a comment from stale bot Oct 20, 2021

idunno101 mentioned this pull request Oct 29, 2021

uv_spawn is slow when parent process allocates mmap with MAP_JIT flag #3050

Closed

yantze mentioned this pull request Dec 1, 2021

electron 13 兼容 opensumi/core#68

Closed

JeffroMF pushed a commit to JeffroMF/libuv that referenced this pull request May 16, 2022

process: reset the signal mask if the fork fails (libuv#3537)

5ff25bd

Fix a regression that sneaked into posix spawn changes. Refs: libuv#3257

XVilka mentioned this pull request May 29, 2022

Update default libuv from 1.40.0 to 1.44.1 rizinorg/rizin#2644

Closed

4 tasks

fantix mentioned this pull request Sep 13, 2022

Bump libuv to 1.44.2 MagicStack/uvloop#499

Closed

rgrinberg mentioned this pull request Mar 10, 2023

Use posix_spawn on macOS 12 janestreet/spawn#47

Draft

arcanis mentioned this pull request Jun 30, 2023

[Bug?]: Spawn error when running script files with Yarn on Node v19.1.0+ yarnpkg/berry#5196

Closed

1 task

liujinye-sys pushed a commit to open-vela/apps_system_libuv that referenced this pull request Jul 23, 2025

process: reset the signal mask if the fork fails (#3537)

75f4219

Fix a regression that sneaked into posix spawn changes. Refs: libuv/libuv#3257

liujinye-sys pushed a commit to open-vela/apps_system_libuv that referenced this pull request Dec 16, 2025

process: reset the signal mask if the fork fails (#3537)

e08bcac

Fix a regression that sneaked into posix spawn changes. Refs: libuv/libuv#3257

Conversation

vtjnash commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vtjnash commented Jul 27, 2021

Uh oh!

deepak1556 commented Aug 2, 2021

Uh oh!

vtjnash commented Aug 9, 2021

Uh oh!

vtjnash commented Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpcanepa commented Aug 20, 2021

Uh oh!

deepak1556 commented Aug 30, 2021

Uh oh!

Uh oh!

KishanBagaria commented Oct 20, 2021

Uh oh!

vtjnash commented Oct 20, 2021

Uh oh!

KishanBagaria commented Oct 20, 2021

Uh oh!

vtjnash commented Oct 20, 2021

Uh oh!

KishanBagaria commented Oct 20, 2021

Uh oh!

vtjnash commented Oct 20, 2021

Uh oh!

1bitphoenix commented Nov 11, 2021

Uh oh!

vtjnash commented Nov 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

vtjnash commented Jul 26, 2021 •

edited

Loading

vtjnash commented Aug 17, 2021 •

edited

Loading