Skip to content

WIP: gh-114467: Support posix_spawn in subprocess.Popen with cwd != None #114468

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.

Sign up for GitHub

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

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

WIP: gh-114467: Support posix_spawn in subprocess.Popen with cwd != None #114468

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9c1aefc
Add configure check for posix_spawn_file_actions_addchdir_np
kulikjak Jan 22, 2024
655e555
Add support for os.POSIX_SPAWN_CHDIR and posix_spawn_file_actions_add…
kulikjak Jan 22, 2024
915e021
Add part of documentation
kulikjak Jan 22, 2024
6a1b61a
Minor fix
kulikjak Jan 22, 2024
0be683f
better check for cwd being empty
kulikjak Jan 23, 2024
12553e1
Implement better error reporting for non-existent executable and/or c…
kulikjak Feb 1, 2024
d4c31c1
Merge branch 'python:main' into posix_spawn-cwd-support
kulikjak Mar 24, 2025
7f77bf9
fix reference counting
kulikjak Mar 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions Doc/library/os.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4757,11 +4757,18 @@ written in Python, such as a mail server's external command delivery program.

Performs ``os.closerange(fd, INF)``.

.. data:: POSIX_SPAWN_CHDIR

(``os.POSIX_SPAWN_CHDIR``, *path*)

Performs ``os.chdir(path)``.

These tuples correspond to the C library
:c:func:`!posix_spawn_file_actions_addopen`,
:c:func:`!posix_spawn_file_actions_addclose`,
:c:func:`!posix_spawn_file_actions_adddup2`, and
:c:func:`!posix_spawn_file_actions_addclosefrom_np` API calls used to prepare
:c:func:`!posix_spawn_file_actions_adddup2`,
:c:func:`!posix_spawn_file_actions_addclosefrom_np`, and
:c:func:`!posix_spawn_file_actions_addchdir_np` API calls used to prepare
for the :c:func:`!posix_spawn` call itself.

The *setpgroup* argument will set the process group of the child to the value
Expand Down Expand Up @@ -4805,8 +4812,11 @@ written in Python, such as a mail server's external command delivery program.

.. versionchanged:: 3.13
*env* parameter accepts ``None``.
``os.POSIX_SPAWN_CLOSEFROM`` is available on platforms where
:c:func:`!posix_spawn_file_actions_addclosefrom_np` exists.

.. versionchanged:: 3.14
``os.POSIX_SPAWN_CLOSEFROM`` and ``os.POSIX_SPAWN_CHDIR`` are available
on platforms where :c:func:`!posix_spawn_file_actions_addclosefrom_np`
and :c:func:`!posix_spawn_file_actions_addchdir_np` exist.

.. availability:: Unix, not WASI, not Android, not iOS.

Expand Down
11 changes: 8 additions & 3 deletions Lib/subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -749,6 +749,7 @@ def _use_posix_spawn():
# These are primarily fail-safe knobs for negatives. A True value does not
# guarantee the given libc/syscall API will be used.
_USE_POSIX_SPAWN = _use_posix_spawn()
_HAVE_POSIX_SPAWN_CHDIR = hasattr(os, 'POSIX_SPAWN_CHDIR')
_HAVE_POSIX_SPAWN_CLOSEFROM = hasattr(os, 'POSIX_SPAWN_CLOSEFROM')


Expand Down Expand Up @@ -1757,7 +1758,7 @@ def _get_handles(self, stdin, stdout, stderr):
errread, errwrite)


def _posix_spawn(self, args, executable, env, restore_signals, close_fds,
def _posix_spawn(self, args, executable, env, restore_signals, close_fds, cwd,
p2cread, p2cwrite,
c2pread, c2pwrite,
errread, errwrite):
Expand All @@ -1784,6 +1785,9 @@ def _posix_spawn(self, args, executable, env, restore_signals, close_fds,
if fd != -1:
file_actions.append((os.POSIX_SPAWN_DUP2, fd, fd2))

if cwd is not None:
file_actions.append((os.POSIX_SPAWN_CHDIR, cwd))

if close_fds:
file_actions.append((os.POSIX_SPAWN_CLOSEFROM, 3))

Expand Down Expand Up @@ -1836,7 +1840,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
and preexec_fn is None
and (not close_fds or _HAVE_POSIX_SPAWN_CLOSEFROM)
and not pass_fds
and cwd is None
and (cwd is None or _HAVE_POSIX_SPAWN_CHDIR)
and (p2cread == -1 or p2cread > 2)
and (c2pwrite == -1 or c2pwrite > 2)
and (errwrite == -1 or errwrite > 2)
Expand All @@ -1846,7 +1850,8 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
and gids is None
and uid is None
and umask < 0):
self._posix_spawn(args, executable, env, restore_signals, close_fds,
self._posix_spawn(args, executable, env, restore_signals,
close_fds, cwd,
p2cread, p2cwrite,
c2pread, c2pwrite,
errread, errwrite)
Expand Down
1 change: 1 addition & 0 deletions Lib/test/test_subprocess.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1841,6 +1841,7 @@ def _get_chdir_exception(self):
self._nonexistent_dir)
return desired_exception

@mock.patch("subprocess._HAVE_POSIX_SPAWN_CHDIR", new=False)
def test_exception_cwd(self):
"""Test error in the child raised in the parent for a bad cwd."""
desired_exception = self._get_chdir_exception()
Expand Down
44 changes: 42 additions & 2 deletions Modules/posixmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7127,6 +7127,9 @@ enum posix_spawn_file_actions_identifier {
#ifdef HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCLOSEFROM_NP
,POSIX_SPAWN_CLOSEFROM
#endif
#ifdef HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCHDIR_NP
,POSIX_SPAWN_CHDIR
#endif
};

#if defined(HAVE_SCHED_SETPARAM) || defined(HAVE_SCHED_SETSCHEDULER) || defined(POSIX_SPAWN_SETSCHEDULER) || defined(POSIX_SPAWN_SETSCHEDPARAM)
Expand Down Expand Up @@ -7277,7 +7280,7 @@ parse_posix_spawn_flags(PyObject *module, const char *func_name, PyObject *setpg
static int
parse_file_actions(PyObject *file_actions,
posix_spawn_file_actions_t *file_actionsp,
PyObject *temp_buffer)
PyObject *temp_buffer, PyObject** cwd)
{
PyObject *seq;
PyObject *file_action = NULL;
Expand Down Expand Up @@ -7384,6 +7387,27 @@ parse_file_actions(PyObject *file_actions,
}
break;
}
#endif
#ifdef HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCHDIR_NP
case POSIX_SPAWN_CHDIR: {
PyObject *path;
if (!PyArg_ParseTuple(file_action, "OO&"
";A chdir file_action tuple must have 2 elements",
&tag_obj, PyUnicode_FSConverter, &path))
{
goto fail;
}
errno = posix_spawn_file_actions_addchdir_np(file_actionsp,
PyBytes_AS_STRING(path));
if (errno) {
posix_error();
Py_DECREF(path);
goto fail;
}
Py_XDECREF(*cwd);
*cwd = path;
Comment on lines +7407 to +7408
Copy link
Member

@serhiy-storchaka serhiy-storchaka Mar 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

POSIX_SPAWN_CHDIR can be passed multiple times with different arguments. In thet case the temporary bytes object saved in *cwd can be deallocated, and the posix_spawn_file_actions_addchdir_np() argument can became a bad pointer.

I think that we need to use temp_buffer here.

Please add a test for posix_spawn() with multiple POSIX_SPAWN_CHDIR actions.

Copy link
Contributor Author

@kulikjak kulikjak Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I am not sure how to handle multiple POSIX_SPAWN_CHDIR actions.

We can either:

  1. call posix_spawn_file_actions_addchdir_np for each POSIX_SPAWN_CHDIR in which case posix_spawn will fail when any of those doesn't exist (or when other error occurs as you pointed out below) even if that one wouldn't be the final one. In such case, the error message might be pretty big.

  2. we can omit all but the last one, in which case it will not behave as some might expect. But we would have only two paths to display in the error message.

As for the bad pointer, it's possible, but cwd always holds the latest one so whether it's called with one or muliple ones shouldn't matter here? But I might be completely wrong. Also, if we want to save all the paths (in case 1. is the prefered one), that might change.

Copy link

@dmcooke dmcooke May 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

posix_spawn_file_actions_addchdir_np also affects how files are found for later posix_spawn_file_actions_add_open, so you need to do them all.

break;
}
#endif
default: {
PyErr_SetString(PyExc_TypeError,
Expand Down Expand Up @@ -7421,6 +7445,7 @@ py_posix_spawn(int use_posix_spawnp, PyObject *module, path_t *path, PyObject *a
Py_ssize_t argc, envc;
PyObject *result = NULL;
PyObject *temp_buffer = NULL;
PyObject *cwd = NULL;
pid_t pid;
int err_code;

Expand Down Expand Up @@ -7486,7 +7511,7 @@ py_posix_spawn(int use_posix_spawnp, PyObject *module, path_t *path, PyObject *a
if (!temp_buffer) {
goto exit;
}
if (parse_file_actions(file_actions, &file_actions_buf, temp_buffer)) {
if (parse_file_actions(file_actions, &file_actions_buf, temp_buffer, &cwd)) {
goto exit;
}
file_actionsp = &file_actions_buf;
Expand Down Expand Up @@ -7518,6 +7543,17 @@ py_posix_spawn(int use_posix_spawnp, PyObject *module, path_t *path, PyObject *a

if (err_code) {
errno = err_code;
#ifdef HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCHDIR_NP
if (errno == ENOENT && cwd != NULL) {
Copy link
Member

@serhiy-storchaka serhiy-storchaka Mar 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the only common error possible here. For example, you can get EPERM or EACCES if any of the executable or the cwd paths are not readable for the user, or ENAMETOOLONG if they are too long, or ELOOP if they contain symlink loops, or ENOTDIR if any of intermediate path component is not a directory. It is not possible to handle all errors.

Maybe use PyErr_SetFromErrnoWithFilenameObjects() (note s at end) with too path arguments? It will perhaps not work if there are multiple POSIX_SPAWN_CHDIR actions, but this is an extremely uncommon case.

Copy link
Contributor Author

@kulikjak kulikjak Apr 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with PyErr_SetFromErrnoWithFilenameObjects is that the exception looks like this:

  File "/usr/lib/python3.13/subprocess.py", line 1799, in _posix_spawn
    self.pid = os.posix_spawn(executable, args, env, **kwargs)
               ~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: '/usr/bin/ls' -> b'/nonexistent'

and I don't like the arrow, which is why I used the PyErr_Format. But otherwise it works nicely and the change is simpler:

-PyErr_SetFromErrnoWithFilenameObject(PyExc_OSError, path->object);
+PyErr_SetFromErrnoWithFilenameObjects(PyExc_OSError, path->object, cwd);

Multiple POSIX_SPAWN_CHDIR actions make it more complicated...

/* ENOENT can occur when either the path of the executable or the
* cwd given via file_actions doesn't exist. Since it's not feasible
* to determine which of those paths caused the problem, we return
* an exception with both. */
PyErr_Format(PyExc_FileNotFoundError, "Either '%S' or '%s' doesn't exist.",
path->object, PyBytes_AS_STRING(cwd));
goto exit;
}
#endif
PyErr_SetFromErrnoWithFilenameObject(PyExc_OSError, path->object);
goto exit;
}
Expand All @@ -7539,6 +7575,7 @@ py_posix_spawn(int use_posix_spawnp, PyObject *module, path_t *path, PyObject *a
if (argvlist) {
free_string_array(argvlist, argc);
}
Py_XDECREF(cwd);
Py_XDECREF(temp_buffer);
return result;
}
Expand Down Expand Up @@ -17576,6 +17613,9 @@ all_ins(PyObject *m)
#ifdef HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCLOSEFROM_NP
if (PyModule_AddIntMacro(m, POSIX_SPAWN_CLOSEFROM)) return -1;
#endif
#ifdef HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCHDIR_NP
if (PyModule_AddIntMacro(m, POSIX_SPAWN_CHDIR)) return -1;
#endif
#endif

#if defined(HAVE_SPAWNV) || defined (HAVE_RTPSPAWN)
Expand Down
6 changes: 6 additions & 0 deletions configure
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5144,7 +5144,7 @@ AC_CHECK_FUNCS([ \
lockf lstat lutimes madvise mbrtowc memrchr mkdirat mkfifo mkfifoat \
mknod mknodat mktime mmap mremap nice openat opendir pathconf pause pipe \
pipe2 plock poll posix_fadvise posix_fallocate posix_openpt posix_spawn posix_spawnp \
posix_spawn_file_actions_addclosefrom_np \
posix_spawn_file_actions_addchdir_np posix_spawn_file_actions_addclosefrom_np \
pread preadv preadv2 process_vm_readv \
pthread_cond_timedwait_relative_np pthread_condattr_setclock pthread_init \
pthread_kill pthread_get_name_np pthread_getname_np pthread_set_name_np
Expand Down
4 changes: 4 additions & 0 deletions pyconfig.h.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -949,6 +949,10 @@
/* Define to 1 if you have the 'posix_spawnp' function. */
#undef HAVE_POSIX_SPAWNP

/* Define to 1 if you have the 'posix_spawn_file_actions_addchdir_np'
function. */
#undef HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCHDIR_NP

/* Define to 1 if you have the 'posix_spawn_file_actions_addclosefrom_np'
function. */
#undef HAVE_POSIX_SPAWN_FILE_ACTIONS_ADDCLOSEFROM_NP
Expand Down
Loading