Have _posixsubprocess.c stop using boolean flags to say if gid and uid values were supplied and action is required. Such an implicit "either initialized or look somewhere else" confused both the reader (another mental connection to constantly track between functions) and a compiler (warnings on potentially uninitialized variables being passed). Instead, we can utilize a special group/user id as a flag value -1 defined by POSIX but used nowhere else. Namely:
gid: call_setgid = False → gid = -1
uid: call_setuid = False → uid = -1
groups: call_setgroups = False → groups = NULL (obtained with (groups_list != Py_None) ? groups : NULL)
This PR is required for #94519.
builtins and extension module functions and methods that expect boolean values for parameters now accept any Python object rather than just a bool or int type. This is more consistent with how native Python code itself behaves.
Previously, the optional restrictions on subinterpreters were: disallow fork, subprocess, and threads. By default, we were disallowing all three for "isolated" interpreters. We always allowed all three for the main interpreter and those created through the legacy `Py_NewInterpreter()` API.
Those settings were a bit conservative, so here we've adjusted the optional restrictions to: fork, exec, threads, and daemon threads. The default for "isolated" interpreters disables fork, exec, and daemon threads. Regular threads are allowed by default. We continue always allowing everything For the main interpreter and the legacy API.
In the code, we add `_PyInterpreterConfig.allow_exec` and `_PyInterpreterConfig.allow_daemon_threads`. We also add `Py_RTFLAGS_DAEMON_THREADS` and `Py_RTFLAGS_EXEC`.
(see https://github.com/python/cpython/issues/98608)
This change does the following:
1. change the argument to a new `_PyInterpreterConfig` struct
2. rename the function to `_Py_NewInterpreterFromConfig()`, inspired by `Py_InitializeFromConfig()` (takes a `_PyInterpreterConfig` instead of `isolated_subinterpreter`)
3. split up the boolean `isolated_subinterpreter` into the corresponding multiple granular settings
* allow_fork
* allow_subprocess
* allow_threads
4. add `PyInterpreterState.feature_flags` to store those settings
5. add a function for checking if a feature is enabled on an opaque `PyInterpreterState *`
6. drop `PyConfig._isolated_interpreter`
The existing default (see `Py_NewInterpeter()` and `Py_Initialize*()`) allows fork, subprocess, and threads and the optional "isolated" interpreter (see the `_xxsubinterpreters` module) disables all three. None of that changes here; the defaults are preserved.
Note that the given `_PyInterpreterConfig` will not be used outside `_Py_NewInterpreterFromConfig()`, nor preserved. This contrasts with how `PyConfig` is currently preserved, used, and even modified outside `Py_InitializeFromConfig()`. I'd rather just avoid that mess from the start for `_PyInterpreterConfig`. We can preserve it later if we find an actual need.
This change allows us to follow up with a number of improvements (e.g. stop disallowing subprocess and support disallowing exec instead).
(Note that this PR adds "private" symbols. We'll probably make them public, and add docs, in a separate change.)
One more thing that can help prevent people from using `preexec_fn`.
Also adds conditional skips to two tests exposing ASAN flakiness on the Ubuntu 20.04 Address Sanitizer Github CI system. When that build is run on more modern systems the "problem" does not show up. It seems ASAN implementation related.
Co-authored-by: Zackery Spytz <zspytz@gmail.com>
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
#92301: subprocess: Prefer `close_range()` to procfs-based fd closing.
`close_range()` is much faster for large number of file descriptors, e.g.
4 times faster for 1000 descriptors in a Linux 5.16-based environment.
We prefer close_range() only if it's known to be async-signal-safe.
Just in case there is ever an issue with _posixsubprocess's use of
vfork() due to the complexity of using it properly and potential
directions that Linux platforms where it defaults to on could take, this
adds a failsafe so that users can disable its use entirely by setting
a global flag.
No known reason to disable it exists. But it'd be a shame to encounter
one and not be able to use CPython without patching and rebuilding it.
See the linked issue for some discussion on reasoning.
Also documents the existing way to disable posix_spawn.
bpo-47151: Fallback to fork when vfork fails in subprocess. An OS kernel can specifically decide to disallow vfork() in a process. No need for that to prevent us from launching subprocesses.
setup.py no longer defines Py_BUILD_CORE_MODULE. Instead every
module defines the macro before #include "Python.h" unless
Py_BUILD_CORE_BUILTIN is already defined.
Py_BUILD_CORE_BUILTIN is defined for every module that is built by
Modules/Setup.
The PR also simplifies Modules/Setup. Makefile and makesetup
already define Py_BUILD_CORE_BUILTIN and include Modules/internal
for us.
Signed-off-by: Christian Heimes <christian@python.org>
* bpo-42146: Unify cleanup in subprocess_fork_exec()
Also ignore errors from _enable_gc():
* They are always suppressed by the current code due to a bug.
* _enable_gc() is only used if `preexec_fn != None`, which is unsafe.
* We don't have a good way to handle errors in case we successfully
created a child process.
Co-authored-by: Gregory P. Smith <greg@krypto.org>
Fix memory leak in subprocess.Popen() in case of uid/gid overflow
Also add a test that would catch this leak with `--huntrleaks`.
Alas, the test for `extra_groups` also exposes an inconsistency
in our error reporting: we use a custom ValueError for `extra_groups`,
but propagate OverflowError for `user` and `group`.
It should just be a syscall updating a couple of fields in the kernel side
process info. Confirming, in glibc is appears to be a shim for the setsid
syscall (based on not finding any code implementing anything special for it)
and in uclibc (*much* easier to read) it is clearly just a setsid syscall shim.
A breadcrumb _suggesting_ that it is not allowed on Darwin/macOS comes from
a commit in emacs: https://lists.gnu.org/archive/html/bug-gnu-emacs/2017-04/msg00297.html
but I don't have a way to verify if that is true or not.
As we are not supporting vfork on macOS today I just left a note in a comment.
Using POSIX_CALL() is incorrect since pthread_sigmask() returns
the error number instead of setting errno.
Also handle failure of the first call to pthread_sigmask()
in the parent process, and explain why we don't handle failure
of the second call in a comment.
* bpo-35823: subprocess: Use vfork() instead of fork() on Linux when safe
When used to run a new executable image, fork() is not a good choice
for process creation, especially if the parent has a large working set:
fork() needs to copy page tables, which is slow, and may fail on systems
where overcommit is disabled, despite that the child is not going to
touch most of its address space.
Currently, subprocess is capable of using posix_spawn() instead, which
normally provides much better performance. However, posix_spawn() does not
support many of child setup operations exposed by subprocess.Popen().
Most notably, it's not possible to express `close_fds=True`, which
happens to be the default, via posix_spawn(). As a result, most users
can't benefit from faster process creation, at least not without
changing their code.
However, Linux provides vfork() system call, which creates a new process
without copying the address space of the parent, and which is actually
used by C libraries to efficiently implement posix_spawn(). Due to sharing
of the address space and even the stack with the parent, extreme care
is required to use vfork(). At least the following restrictions must hold:
* No signal handlers must execute in the child process. Otherwise, they
might clobber memory shared with the parent, potentially confusing it.
* Any library function called after vfork() in the child must be
async-signal-safe (as for fork()), but it must also not interact with any
library state in a way that might break due to address space sharing
and/or lack of any preparations performed by libraries on normal fork().
POSIX.1 permits to call only execve() and _exit(), and later revisions
remove vfork() specification entirely. In practice, however, almost all
operations needed by subprocess.Popen() can be safely implemented on
Linux.
* Due to sharing of the stack with the parent, the child must be careful
not to clobber local variables that are alive across vfork() call.
Compilers are normally aware of this and take extra care with vfork()
(and setjmp(), which has a similar problem).
* In case the parent is privileged, special attention must be paid to vfork()
use, because sharing an address space across different privilege domains
is insecure[1].
This patch adds support for using vfork() instead of fork() on Linux
when it's possible to do safely given the above. In particular:
* vfork() is not used if credential switch is requested. The reverse case
(simple subprocess.Popen() but another application thread switches
credentials concurrently) is not possible for pure-Python apps because
subprocess.Popen() and functions like os.setuid() are mutually excluded
via GIL. We might also consider to add a way to opt-out of vfork() (and
posix_spawn() on platforms where it might be implemented via vfork()) in
a future PR.
* vfork() is not used if `preexec_fn != None`.
With this change, subprocess will still use posix_spawn() if possible, but
will fallback to vfork() on Linux in most cases, and, failing that,
to fork().
[1] https://ewontfix.com/7
Co-authored-by: Gregory P. Smith [Google LLC] <gps@google.com>
This API is relatively lightweight and organizationally, given that it's
used by multiple modules, it makes sense to move it to fileutils.
Requires making sure that _posixsubprocess is compiled with the appropriate
Py_BUIILD_CORE_BUILTIN macro.
Such an API can be used both for os.closerange and subprocess. For the latter, this yields potential improvement for platforms that have fdwalk but wouldn't have used it there. This will prove even more beneficial later for platforms that have close_range(2), as the new API will prefer that over all else if it's available.
The new API is structured to look more like close_range(2), closing from [start, end] rather than the [low, high) of os.closerange().
Automerge-Triggered-By: @gpshead
```
Direct leak of 8 byte(s) in 1 object(s) allocated from:
#0 0x7f008bf19667 in __interceptor_malloc (/lib64/libasan.so.6+0xb0667)
#1 0x7f007a0bee4a in subprocess_fork_exec /home/heimes/dev/python/cpython/Modules/_posixsubprocess.c:774
#2 0xe0305b in cfunction_call Objects/methodobject.c:546
```
Signed-off-by: Christian Heimes <christian@python.org>
An isolated subinterpreter cannot spawn threads, spawn a child
process or call os.fork().
* Add private _Py_NewInterpreter(isolated_subinterpreter) function.
* Add isolated=True keyword-only parameter to
_xxsubinterpreters.create().
* Allow again os.fork() in "non-isolated" subinterpreters.
Optimize the subprocess module on FreeBSD using closefrom().
A single close(fd) syscall is cheap, but when sysconf(_SC_OPEN_MAX)
is high, the loop calling close(fd) on each file descriptor can take
several milliseconds.
The workaround on FreeBSD to improve performance was to load and
mount the fdescfs kernel module, but this is not enabled by default.
Initial patch by Ed Maste (emaste), Conrad Meyer (cem), Kyle Evans
(kevans) and Kubilay Kocak (koobs):
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=242274
* Rename _PyInterpreterState_Get() to PyInterpreterState_Get() and
move it the limited C API.
* Add _PyInterpreterState_Get() alias to PyInterpreterState_Get() for
backward compatibility with Python 3.8.
The bulk of this patch was generated automatically with:
for name in \
PyObject_Vectorcall \
Py_TPFLAGS_HAVE_VECTORCALL \
PyObject_VectorcallMethod \
PyVectorcall_Function \
PyObject_CallOneArg \
PyObject_CallMethodNoArgs \
PyObject_CallMethodOneArg \
;
do
echo $name
git grep -lwz _$name | xargs -0 sed -i "s/\b_$name\b/$name/g"
done
old=_PyObject_FastCallDict
new=PyObject_VectorcallDict
git grep -lwz $old | xargs -0 sed -i "s/\b$old\b/$new/g"
and then cleaned up:
- Revert changes to in docs & news
- Revert changes to backcompat defines in headers
- Nudge misaligned comments
* subprocess: Add user, group and extra_groups paremeters to subprocess.Popen
This adds a `user` parameter to the Popen constructor that will call
setreuid() in the child before calling exec(). This allows processes
running as root to safely drop privileges before running the subprocess
without having to use a preexec_fn.
This also adds a `group` parameter that will call setregid() in
the child process before calling exec().
Finally an `extra_groups` parameter was added that will call
setgroups() to set the supplimental groups.
Summary:
Eliminate uses of `_Py_IDENTIFIER` from `_posixsubprocess`, replacing them with interned strings.
Also tries to find an existing version of the module, which will allow subinterpreters.
https://bugs.python.org/issue38069
Adds configure flags for msan and ubsan builds to make it easier to enable.
These also encode the detail that address sanitizer and memory sanitizer
should disable pymalloc.
Define MEMORY_SANITIZER when appropriate at build time and adds workarounds
to existing code to mark things as initialized where the sanitizer is otherwise unable to
determine that. This lets our build succeed under the memory sanitizer. not all tests
pass without sanitizer failures yet but we're in pretty good shape after this.
When os.fork() is called (on platforms that support it) all threads but the current one are destroyed in the child process. Consequently we must ensure that all but the associated interpreter are likewise destroyed. The main interpreter is critical for runtime operation, so we must ensure that fork only happens in the main interpreter.
https://bugs.python.org/issue34651
[bpo-34658](https://www.bugs.python.org/issue34658): Fix a rare interpreter unhandled exception state SystemError only
seen when using subprocess with a preexec_fn while an after_parent handler has
been registered with os.register_at_fork and the fork system call fails.
https://bugs.python.org/issue34658
When subprocess.Popen() stdin= stdout= or stderr= handles are specified
and appear in pass_fds=, don't close the original fds after dup'ing them.
This implementation and unittest primarily came from @izbyshev (see the PR)
See also b89b52f284
This also removes the old manual p2cread, c2pwrite, and errwrite closing logic
as inheritable flags and _close_open_fds takes care of that properly today without special treatment.
This code is within child_exec() where it is the only thread so there is no
race condition between the dup and _Py_set_inheritable_async_safe call.
bpo-32844: subprocess: Fix a potential misredirection of a low fd to stderr.
When redirecting, subprocess attempts to achieve the following state:
each fd to be redirected to is less than or equal to the fd
it is redirected from, which is necessary because redirection
occurs in the ascending order of destination descriptors.
It fails to do so in a couple of corner cases,
for example, if 1 is redirected to 2 and 0 is closed in the parent.
Fix a rare but potential pre-exec child process deadlock in subprocess on POSIX systems when marking file descriptors inheritable on exec in the child process. This bug appears to have been introduced in 3.4 with the inheritable file descriptors support.
This also changes Python/fileutils.c `set_inheritable` to use the "slow" two `fcntl` syscall path instead of the "fast" single `ioctl` syscall path when asked to be async signal safe (by way of being asked not to raise exceptions). `ioctl` is not a POSIX async-signal-safe approved function.
ref: http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html
* bpo-16500: Allow registering at-fork handlers
* Address Serhiy's comments
* Add doc for new C API
* Add doc for new Python-facing function
* Add NEWS entry + doc nit
Oleg Iarygin
Serhiy Storchaka
Eric Snow
Victor Stinner
Gregory P. Smith
Alexey Izbyshev
Christian Heimes
Jakub Kulík
David CARLIER
Kyle Evans
Hai Shi
Petr Viktorin
Orivej Desh
Patrick McLean
Dino Viehland
Jeroen Demeyer
Alexey Izbyshev
luzpaz
Antoine Pitrou
Xavier de Gaye