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

Support the macOS-only POSIX_SPAWN_CLOEXEC_DEFAULT flag and posix_spawn_file_actions_addinherit_np() in os.posix_spawn #109154

Open
mrpippy opened this issue Sep 8, 2023 · 8 comments
Assignees
Labels
3.13 bugs and security fixes OS-mac type-feature A feature request or enhancement

Comments

@mrpippy
Copy link

mrpippy commented Sep 8, 2023

Feature or enhancement

Proposal:

macOS supports a non-standard posix_spawn() flag, POSIX_SPAWN_CLOEXEC_DEFAULT, which makes "only file descriptors explicitly created by the file_actions argument available in the spawned process; all of the other file descriptors are automatically closed in the spawned process."

This would be useful for subprocess, where close_fds defaulting to True means that posix_spawn() is not used by default. POSIX_SPAWN_CLOEXEC_DEFAULT combined with (the also non-standard) posix_spawn_file_actions_addinherit_np() would provide a correct way to use posix_spawn() when close_fds=True.

A Chromium bug indicates that POSIX_SPAWN_CLOEXEC_DEFAULT was added in OS X 10.7 but causes kernel panics there, for safety this should probably only be used on OS X 10.8 and later.

This could be added as a cloexec_default=False argument to os.posix_spawn() and os.posix_spawnp().

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

Links to previous discussion of this feature:

#79718 was the original issue for using os.posix_spawn() in subprocess.
#86904 proposed changing close_fds to default to False, so posix_spawn() could be used more often

@mrpippy mrpippy added the type-feature A feature request or enhancement label Sep 8, 2023
@ned-deily
Copy link
Member

@ronaldoussoren @gpshead

@mrpippy
Copy link
Author

mrpippy commented Sep 8, 2023

also @vstinner

@ronaldoussoren
Copy link
Contributor

Using this API looks useful on first glance. There's no need to change the API of subprocess for this though, POSIX_SPAWN_CLOEXEC_DEFAULT behaves the same as the documented behaviour for close_fds=True.

@ronaldoussoren
Copy link
Contributor

@gpshead: a quick API design question: there's two ways to implement this for macOS:

  1. Directly expose the underlying API by adding a new flag argument (cloexec_default=False) and add a POSIX_SPAWN_INHERIT alternative to file actions
  2. By implementing the POSIX_SPAWN_CLOSEFROM option from gh-113117: Support posix_spawn in subprocess.Popen with close_fds=True #113118
  3. Both...

The disadvantage of 1 is that this makes part of the API non-portable. The disadvantage of 2 is more house keeping in C code.

BTW. Both libc and macOS also have posix_spawn_file_actions_addchdir which is useful to implement the cwd option in subprocess (glibc with an _np suffix, but the same API as macOS). Both also have a variant using fchdir instead of chdir.

@kulikjak
Copy link
Contributor

BTW. Both libc and macOS also have posix_spawn_file_actions_addchdir which is useful to implement the cwd option in subprocess (glibc with an _np suffix, but the same API as macOS). Both also have a variant using fchdir instead of chdir

Hi, I started looking into posix_spawn_file_actions_addchdir (alongside the posix_spawn_file_actions_addclosefrom_np) and should have a PR ready soon (I have yet to solve an incorrect exception message when chdir fails).

@gpshead
Copy link
Member

gpshead commented Dec 17, 2023

  1. Directly expose the underlying API by adding a new flag argument (cloexec_default=False) and add a POSIX_SPAWN_INHERIT alternative to file actions
  2. By implementing the POSIX_SPAWN_CLOSEFROM option from gh-113117: Support posix_spawn in subprocess.Popen with close_fds=True #113118
  3. Both...

The disadvantage of 1 is that this makes part of the API non-portable. The disadvantage of 2 is more house keeping in C code.

I like adding a new platform specific flag to the os module named POSIX_SPAWN_ADDINHERIT (similar to the C API name as our other constants are), POSIX_SPAWN_CLOSEFROM is already one such platform specific flag in the other PR. The os module is a place where we intentionally expose direct platform APIs rather than emulate behaviors. I think that is more valuable. Higher level APIs can use those raw os APIs to provide common interfaces, subprocess being one of those.

We should not add a new subprocess.Popen parameter. We should just use the POSIX_SPAWN_ADDINHERIT API when available to implement subprocess's use of posix_spawn when close_fds=True. Similar to how the other PR does it for the ...CLOSEFROM non-posix API variant.

As for macOS version, I'd be conservative and use 11+ as the version cutoff before we try to use this non-posix addition - Apple doesn't support versions older than 11 anyways AFAICT.

@gpshead gpshead added the 3.13 bugs and security fixes label Dec 17, 2023
@ronaldoussoren
Copy link
Contributor

  1. Directly expose the underlying API by adding a new flag argument (cloexec_default=False) and add a POSIX_SPAWN_INHERIT alternative to file actions
  2. By implementing the POSIX_SPAWN_CLOSEFROM option from gh-113117: Support posix_spawn in subprocess.Popen with close_fds=True #113118
  3. Both...

The disadvantage of 1 is that this makes part of the API non-portable. The disadvantage of 2 is more house keeping in C code.

I like adding a new platform specific flag to the os module named POSIX_SPAWN_ADDINHERIT (similar to the C API name as our other constants are), POSIX_SPAWN_CLOSEFROM is already one such platform specific flag in the other PR.

Emulating CLOSEFROM is easy enough and has the advantage of matching the Linux interface. For better or worse there will be a lot of code that is developed on Linux and requires extra work to run on macOS (e.g. there are still complaints about the start_method being different on macOS even if there are good reasons for that).

The os module is a place where we intentionally expose direct platform APIs rather than emulate behaviors. I think that is more valuable. Higher level APIs can use those raw os APIs to provide common interfaces, subprocess being one of those.

Except where we don't expose platforms directly (think of the dirfd and follow_symlink parameters for a lot of functions instead of exposing the underlying API directly). I totally agree with that design choice, but we are slightly moving away from directly exposing platform APIs.

All in all I think it would be better to pick my option 3: expose both the native functionality and emulate the linux API unless it turns out to be non-trivial to do the latter.

We should not add a new subprocess.Popen parameter. We should just use the POSIX_SPAWN_ADDINHERIT API when available to implement subprocess's use of posix_spawn when close_fds=True. Similar to how the other PR does it for the ...CLOSEFROM non-posix API variant.

I agree with not changing the interface of subprocess.Popen for this. The new parameter is needed for os.posix_spawn, to implement closefds using posix_spawn you need POSIX_SPAWN_CLOEXEC_DEFAULT on macOS, combined with the posix_spawn_file_actions_addinherit_np to avoid closing descriptors that need to be passed unchanged to the new proces.

As for macOS version, I'd be conservative and use 11+ as the version cutoff before we try to use this non-posix addition - Apple doesn't support versions older than 11 anyways AFAICT.

The API is supported on macOS 10.7 or later, I can test on 10.9 to check if works there. If it does we get a consistent behaviour on all platforms supported by our installer

@gpshead
Copy link
Member

gpshead commented Dec 17, 2023

If you're willing to write CLOSEFROM posix_spawn emulation using the macOS API and a test for the os module, I'm happy to review it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.13 bugs and security fixes OS-mac type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

6 participants