Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
39 changes: 39 additions & 0 deletions Lib/test/test_subprocess.py
Original file line number Diff line number Diff line change
Expand Up @@ -3476,6 +3476,45 @@ def test_fork_exec(self):
if not gc_enabled:
gc.disable()

@support.cpython_only
def test_fork_exec_args_concurrent_mutation(self):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lets not add this test. it is perhaps useful for illustration purposes, but this is not specific to subprocess. it's C API misuse. FWIW, thanks! overall the rest of the PR looks good.

I've pointed claude fable at the antipattern to find and fixup others and improve the docs as a future prevention matter: #151416

# An argv item's __fspath__() can run Python that drops the args
# sequence's last reference to that item while fork_exec() is still
# converting it. See: https://github.com/python/cpython/issues/151403
import _posixsubprocess

def fork_exec(args):
return _posixsubprocess.fork_exec(
args, [b"false"],
True, (), None, [b"env"],
-1, -1, -1, -1,
1, 2, 3, 4,
True, True, 0,
None, None, None, -1,
None)

# __fspath__() returns a non-str/bytes, forcing the error path that
# reports the offending item's type. Clearing the list there drops
# the item's last reference: only fork_exec() keeping its own
# reference avoids a use-after-free.
class Evil:
def __fspath__(self):
args.clear()
return 12345
args = [Evil()]
with self.assertRaises(TypeError):
fork_exec(args)

# Changing the list length is reported as a RuntimeError before the
# next item is converted.
class Shrink:
def __fspath__(self):
args.clear()
return "/nonexistent"
args = [Shrink(), "/second"]
with self.assertRaises(RuntimeError):
fork_exec(args)

@support.cpython_only
def test_fork_exec_sorted_fd_sanity_check(self):
# Issue #23564: sanity check the fork_exec() fds_to_keep sanity check.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Fixed a crash in :class:`subprocess.Popen` (and ``_posixsubprocess.fork_exec``)
when an ``argv`` item's :meth:`~object.__fspath__` concurrently mutates the
``args`` sequence being converted.
8 changes: 7 additions & 1 deletion Modules/_posixsubprocess.c
Original file line number Diff line number Diff line change
Expand Up @@ -1090,8 +1090,14 @@ subprocess_fork_exec_impl(PyObject *module, PyObject *process_args,
goto cleanup;
}
borrowed_arg = PySequence_Fast_GET_ITEM(fast_args, arg_num);
if (PyUnicode_FSConverter(borrowed_arg, &converted_arg) == 0)
/* borrowed_arg is only borrowed; its __fspath__() may run Python
that drops fast_args' last reference to it. */
Py_INCREF(borrowed_arg);
if (PyUnicode_FSConverter(borrowed_arg, &converted_arg) == 0) {
Py_DECREF(borrowed_arg);
goto cleanup;
}
Py_DECREF(borrowed_arg);
PyTuple_SET_ITEM(converted_args, arg_num, converted_arg);
}

Expand Down
Loading