gh-151403: Fix use-after-free when an argv item's __fspath__ mutates args#151404
Conversation
| gc.disable() | ||
|
|
||
| @support.cpython_only | ||
| def test_fork_exec_args_concurrent_mutation(self): |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Done — dropped the test. Agreed it's general C-API misuse rather than subprocess-specific. Thanks for the quick review!
38b2a4f to
80d6ec7
Compare
|
Done — dropped the test. Agreed it's general C-API misuse rather than subprocess-specific. Thanks for the quick review! |
|
Thanks @tonghuaroot for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14, 3.15. |
|
GH-151445 is a backport of this pull request to the 3.15 branch. |
|
GH-151446 is a backport of this pull request to the 3.14 branch. |
|
GH-151447 is a backport of this pull request to the 3.13 branch. |
_posixsubprocess.fork_exec()converts eachargsitem withPyUnicode_FSConverter(), which runs the item's__fspath__(). The item is onlyborrowed from the (incref'd)
argssequence. If__fspath__()drops thesequence's last reference to the item and then returns a non-
str/bytesobject, the error path in
PyOS_FSPath()readsPy_TYPE()of the now-freed item— a use-after-free.
It is reachable from
subprocess.Popenwhen a laterargvitem is aPathLikewhose
__fspath__()mutates theargslist, e.g.subprocess.Popen(["/bin/true", Evil()]).The between-iteration length recheck only catches a shrink between iterations;
the free here happens inside the current iteration's
PyUnicode_FSConverter()call. The fix keeps the borrowed item alive with an
Py_INCREFacross theconversion.
Same family as gh-151295 (
bytes.join, GH-151296) and gh-151370(
marshal.dumps, GH-151371). Triggering requires a custom__fspath__, so thisis a robustness / crash-hardening fix with no security impact; the NEWS entry is
under
Library/.