gh-151370: Fix marshal.dumps() crash on concurrent container mutation#151371
Open
tonghuaroot wants to merge 1 commit into
Open
gh-151370: Fix marshal.dumps() crash on concurrent container mutation#151371tonghuaroot wants to merge 1 commit into
tonghuaroot wants to merge 1 commit into
Conversation
…tation An item's __buffer__() (PEP 688) runs Python while the list, dict or set is being serialized, and that Python can mutate or free the container, causing a use-after-free, an out-of-bounds access, or an abort. Keep the serialized object alive across w_complex_object() and report a size change as a RuntimeError. Same family as pythongh-151295 (bytes.join).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
w_complex_object()serializes alist,dictorsetby handing each itemto
w_object(). For an item that supports the buffer protocol this reachesPyObject_GetBuffer(), which runs the item's__buffer__()(PEP 688) — purePython that can concurrently mutate, or drop the last reference to, the very
container being serialized. That crashes the interpreter in three places:
n = PyList_GET_SIZE(v)and indexes withPyList_GET_ITEM(v, i), so shrinking the list reads past the end; and thebuffer branch keeps using the item after
PyObject_GetBuffer()returns, sofreeing it is a use-after-free.
PyDict_Next()returns borrowedkey/value; the key's__buffer__()can drop the dict's last reference tovalue, after whichw_object(value)runs on freed memory.assert(i == n)fires,or a
NULLslot reachesPyList_Sort) or grow it (writing past thepairslist that was pre-sized to the original length).
The container itself can also be freed: an item's
__buffer__()may drop thecaller's last reference to the
list/dict/setwhilew_complex_object()is still iterating it.
The fix follows the
bytes.joinfix in gh-151295:w_object()now keeps a reference to the object it is serializing acrossw_complex_object()(Py_INCREF/Py_DECREF), so neither an item nor thecontainer can be freed by the Python that
__buffer__()runs. Thedictbranch additionally keeps its borrowed
key/valuealive across the twow_object()calls.list,dictandsetbranchesrecheck the container's size and report a change as
RuntimeErrorinstead ofcontinuing from a now-stale index or iterator position; the
setalso boundsits write index against the pre-sized
pairslist.The size change is reported through marshal's existing error-code mechanism:
w_object()records aWFERR_*code and the top-level switch in_PyMarshal_WriteObjectToString()maps it to an exception. That switch isunconditional, so an exception set inside the loop would be overwritten;
producing the precise
RuntimeErrortherefore requires a new code(
WFERR_CHANGED_SIZE) and a matching case, mirroring the codes already defined.Same family as gh-151295 (
bytes.join), fixed the same way.This change was prepared with the assistance of an AI tool; I reviewed and
verified the fix and the regression test myself: a fresh
mainbuild withbefore/after reproducers, the full
test_marshalsuite, and a refleak run.