Emulate POSIX_SPAWN_CLOEXEC_DEFAULT in fork/exec #79

iCharlesHu · 2025-06-13T22:57:30Z

POSIX_SPAWN_CLOEXEC_DEFAULT is only available on Darwin. Emulate POSIX_SPAWN_CLOEXEC_DEFAULT on other platforms by calling close after fork, before exec.

This commit also removes _subprocess_posix_spawn_fallback because we can't emulate POSIX_SPAWN_CLOEXEC_DEFAULT in a thread-safe manner while using posix_spawn.

Resolves: #46

Sources/_SubprocessCShims/process_shims.c

jakepetroules · 2025-06-13T23:16:51Z

Sources/_SubprocessCShims/process_shims.c

+        hi = getdtablesize();
+    }
+#else
+    int hi = 1024;


Where does '1024' come from and how safe is it to default to this vs making this #error and requiring a specific implementation for new platforms?

Also, the following additional platforms support getdtablesize: FreeBSD, OpenBSD, NetBSD, Solaris, QNX.

Instead of #error, how about if I just call getdtablesize unconditionally here and have it naturally error on platforms that don't support it.

lin72h · 2025-06-14T21:50:45Z

FreeBSD PR #1698 adds support for POSIX O_CLOFORK, which could simplify this implementation.

iCharlesHu · 2025-06-16T17:55:52Z

FreeBSD PR #1698 adds support for POSIX O_CLOFORK, which could simplify this implementation.

Ahh thanks. Good to know.

parkera · 2025-06-26T23:36:06Z

Sources/_SubprocessCShims/process_shims.c

-    char * _Nullable const args[_Nonnull],
-    char * _Nullable const env[_Nullable],
-    gid_t * _Nullable process_group_id
+int _shims_snprintf(


Why do we need this shim?

On Linux this function isn't exposed via import Glibc

Sources/_SubprocessCShims/process_shims.c

Tests/SubprocessTests/SubprocessTests+Unix.swift

iCharlesHu · 2025-07-03T04:40:32Z

Addressed review comments.

Sources/_SubprocessCShims/process_shims.c

Tests/SubprocessTests/SubprocessTests+Unix.swift

lin72h · 2025-07-07T03:27:45Z

Just a heads up - the PR has merged. I think the timing means it'll likely make it into the upcoming FreeBSD 15 release in December.

itingliu

The Linux CI failure looks relevant

[7/22] Compiling CSystem shims.c
/__w/swift-subprocess/swift-subprocess/Sources/_SubprocessCShims/process_shims.c:318:28: error: use of undeclared identifier 'val'
  318 |     if (errno == ERANGE || val <= 0 || val > INT_MAX) {
      |

Sources/_SubprocessCShims/process_shims.c

ricardobranco777 · 2025-07-26T15:57:43Z

Now that O_CLOFORK is merged in all *BSD, work can be started on support O_CLOFORK flags.

POSIX_SPAWN_CLOEXEC_DEFAULT is only available on Darwin. Emulate POSIX_SPAWN_CLOEXEC_DEFAULT on other platforms by calling close after fork, before exec. This commit also removes _subprocess_posix_spawn_fallback because we can't emulate POSIX_SPAWN_CLOEXEC_DEFAULT in a thread-safe manner while using posix_spawn.

jakepetroules · 2025-08-12T22:16:29Z

Sources/_SubprocessCShims/process_shims.c

+
+#if __DARWIN_NSIG               /* Darwin */
+#  define _SUBPROCESS_SIG_MAX __DARWIN_NSIG
+#elif defined(NSIG_MAX)         /* POSIX issue 8 */


super nit: the POSIX one should go first, being the standard thing, shouldn't it?

jakepetroules · 2025-08-12T22:17:39Z

Sources/_SubprocessCShims/process_shims.c

+
+static int _highest_possibly_open_fd(void) {
+#if defined(__linux__)
+    int hi = _highest_possibly_open_fd_dir_linux("/dev/fd");


nit: might be worth a comment that this is only needed for the Linux-older-than-5.9 path where we don't have close_range which makes this more efficient.

jakepetroules · 2025-08-12T22:21:58Z

Tests/SubprocessTests/SubprocessTests+Unix.swift

+        )
+        // Depending on the platform, standard error should be something like
+        // `/bin/bash: 7: Bad file descriptor
+        #expect(!result.standardError!.isEmpty)


nit: avoid force unwraps:

Suggested change

#expect(!result.standardError!.isEmpty)

let standardError = try #require(result.standardError)

#expect(!standardError.isEmpty)

jakepetroules · 2025-08-12T22:38:18Z

Sources/_SubprocessCShims/process_shims.c

+        // Close all other file descriptors
+        rc = -1;
+        errno = ENOSYS;
+#if __has_include(<linux/close_range.h>) || defined(__FreeBSD__)


Sorry, one more thing. This breaks the build on Android because it doesn't have close_range.

/Volumes/Sources/OpenSource/swift-subprocess/Sources/_SubprocessCShims/process_shims.c:640:14: error: call to undeclared function 'close_range'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
640 | rc = close_range(STDERR_FILENO + 1, pipefd[1] - 1, 0);
| ^
1 error generated.
[8/22] Compiling CSystem shims.c

It was introduced in NDK r34 so you need an availability guard...

This will fix it:

Suggested change

#if __has_include(<linux/close_range.h>) || defined(__FreeBSD__)

#if (__has_include(<linux/close_range.h>) && (!defined(__ANDROID__) || __ANDROID_API__ >= 34)) || defined(__FreeBSD__)

iCharlesHu requested review from parkera and itingliu June 13, 2025 22:57

iCharlesHu force-pushed the charles/fork-exec-close-fd branch from 6e9afb5 to 3e47e05 Compare June 13, 2025 23:01

jakepetroules reviewed Jun 13, 2025

View reviewed changes

Sources/_SubprocessCShims/process_shims.c Outdated Show resolved Hide resolved

iCharlesHu force-pushed the charles/fork-exec-close-fd branch from 3e47e05 to 2d9a937 Compare June 13, 2025 23:11

jakepetroules reviewed Jun 13, 2025

View reviewed changes

ricardobranco777 mentioned this pull request Jun 15, 2025

Add support for POSIX O_CLOFORK freebsd/freebsd-src#1698

Closed

7 tasks

iCharlesHu force-pushed the charles/fork-exec-close-fd branch from 2d9a937 to cd79141 Compare June 16, 2025 18:24

iCharlesHu requested a review from cthielen June 17, 2025 22:05

This was referenced Jun 20, 2025

Add support for POSIX O_CLOFORK DragonFlyBSD/DragonFlyBSD#28

Closed

Add support for POSIX O_CLOFORK openbsd/src#46

Closed

Add support for POSIX O_CLOFORK NetBSD/src#53

Closed

parkera reviewed Jun 26, 2025

View reviewed changes

Sources/_SubprocessCShims/process_shims.c Outdated Show resolved Hide resolved

cthielen reviewed Jun 26, 2025

View reviewed changes

Sources/_SubprocessCShims/process_shims.c Outdated Show resolved Hide resolved

Sources/_SubprocessCShims/process_shims.c Show resolved Hide resolved

Tests/SubprocessTests/SubprocessTests+Unix.swift Outdated Show resolved Hide resolved

iCharlesHu force-pushed the charles/fork-exec-close-fd branch from cd79141 to c217b39 Compare July 3, 2025 04:40

jakepetroules requested changes Jul 3, 2025

View reviewed changes

jakepetroules mentioned this pull request Jul 4, 2025

OpenBSD / FreeBSD Support for Subprocess #115

Open

itingliu reviewed Jul 7, 2025

View reviewed changes

Sources/_SubprocessCShims/process_shims.c Outdated Show resolved Hide resolved

iCharlesHu force-pushed the charles/fork-exec-close-fd branch 3 times, most recently from e791d2e to d78a5c8 Compare August 12, 2025 17:49

iCharlesHu added 2 commits August 12, 2025 13:38

Use NSIG_MAX insead of a constant

6176f07

iCharlesHu force-pushed the charles/fork-exec-close-fd branch from d78a5c8 to 6176f07 Compare August 12, 2025 20:42

jakepetroules approved these changes Aug 12, 2025

View reviewed changes

jakepetroules requested changes Aug 12, 2025

View reviewed changes

	#expect(!result.standardError!.isEmpty)
	let standardError = try #require(result.standardError)
	#expect(!standardError.isEmpty)

	#if __has_include(<linux/close_range.h>) \|\| defined(__FreeBSD__)
	#if (__has_include(<linux/close_range.h>) && (!defined(__ANDROID__) \|\| __ANDROID_API__ >= 34)) \|\| defined(__FreeBSD__)

Emulate POSIX_SPAWN_CLOEXEC_DEFAULT in fork/exec #79

Are you sure you want to change the base?

Emulate POSIX_SPAWN_CLOEXEC_DEFAULT in fork/exec #79

Conversation

iCharlesHu commented Jun 13, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lin72h commented Jun 14, 2025

Uh oh!

iCharlesHu commented Jun 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iCharlesHu commented Jul 3, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lin72h commented Jul 7, 2025

Uh oh!

itingliu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ricardobranco777 commented Jul 26, 2025 •

edited

Loading