Skip to content

Commit 38b2a4f

Browse files
committed
gh-151403: Fix use-after-free when an argv item's __fspath__ mutates args
1 parent c375992 commit 38b2a4f

3 files changed

Lines changed: 49 additions & 1 deletion

File tree

Lib/test/test_subprocess.py

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3476,6 +3476,45 @@ def test_fork_exec(self):
34763476
if not gc_enabled:
34773477
gc.disable()
34783478

3479+
@support.cpython_only
3480+
def test_fork_exec_args_concurrent_mutation(self):
3481+
# An argv item's __fspath__() can run Python that drops the args
3482+
# sequence's last reference to that item while fork_exec() is still
3483+
# converting it. See: https://github.com/python/cpython/issues/151403
3484+
import _posixsubprocess
3485+
3486+
def fork_exec(args):
3487+
return _posixsubprocess.fork_exec(
3488+
args, [b"false"],
3489+
True, (), None, [b"env"],
3490+
-1, -1, -1, -1,
3491+
1, 2, 3, 4,
3492+
True, True, 0,
3493+
None, None, None, -1,
3494+
None)
3495+
3496+
# __fspath__() returns a non-str/bytes, forcing the error path that
3497+
# reports the offending item's type. Clearing the list there drops
3498+
# the item's last reference: only fork_exec() keeping its own
3499+
# reference avoids a use-after-free.
3500+
class Evil:
3501+
def __fspath__(self):
3502+
args.clear()
3503+
return 12345
3504+
args = [Evil()]
3505+
with self.assertRaises(TypeError):
3506+
fork_exec(args)
3507+
3508+
# Changing the list length is reported as a RuntimeError before the
3509+
# next item is converted.
3510+
class Shrink:
3511+
def __fspath__(self):
3512+
args.clear()
3513+
return "/nonexistent"
3514+
args = [Shrink(), "/second"]
3515+
with self.assertRaises(RuntimeError):
3516+
fork_exec(args)
3517+
34793518
@support.cpython_only
34803519
def test_fork_exec_sorted_fd_sanity_check(self):
34813520
# Issue #23564: sanity check the fork_exec() fds_to_keep sanity check.
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
Fixed a crash in :class:`subprocess.Popen` (and ``_posixsubprocess.fork_exec``)
2+
when an ``argv`` item's :meth:`~object.__fspath__` concurrently mutates the
3+
``args`` sequence being converted.

Modules/_posixsubprocess.c

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1090,8 +1090,14 @@ subprocess_fork_exec_impl(PyObject *module, PyObject *process_args,
10901090
goto cleanup;
10911091
}
10921092
borrowed_arg = PySequence_Fast_GET_ITEM(fast_args, arg_num);
1093-
if (PyUnicode_FSConverter(borrowed_arg, &converted_arg) == 0)
1093+
/* borrowed_arg is only borrowed; its __fspath__() may run Python
1094+
that drops fast_args' last reference to it. */
1095+
Py_INCREF(borrowed_arg);
1096+
if (PyUnicode_FSConverter(borrowed_arg, &converted_arg) == 0) {
1097+
Py_DECREF(borrowed_arg);
10941098
goto cleanup;
1099+
}
1100+
Py_DECREF(borrowed_arg);
10951101
PyTuple_SET_ITEM(converted_args, arg_num, converted_arg);
10961102
}
10971103

0 commit comments

Comments
 (0)