Skip to content

Commit d54225c

Browse files
committed
gh-151370: Fix marshal.dumps() crash on concurrent container mutation
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 gh-151295 (bytes.join).
1 parent 65047f2 commit d54225c

3 files changed

Lines changed: 95 additions & 3 deletions

File tree

Lib/test/test_marshal.py

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -243,6 +243,51 @@ def test_array(self):
243243
new = marshal.loads(marshal.dumps(a))
244244
self.assertEqual(new, b"abc")
245245

246+
def test_concurrent_buffer_mutation(self):
247+
# An item's __buffer__() runs Python while the container is being
248+
# marshalled, so the container can be concurrently mutated (simulated
249+
# here by mutating it inside __buffer__).
250+
# See: https://github.com/python/cpython/issues/151370
251+
def make(kind):
252+
# The item is only referenced from the container, so clearing the
253+
# container inside __buffer__ drops its last reference to it.
254+
class Item:
255+
def __buffer__(self, flags):
256+
container.clear()
257+
return memoryview(bytearray(4))
258+
item = Item()
259+
if kind is dict:
260+
container = {item: 1, 2: item}
261+
elif kind is set:
262+
container = {item, b'other'}
263+
else:
264+
container = [item, 1, 2]
265+
return container
266+
267+
# A size change while marshalling is reported as a RuntimeError.
268+
for kind in (list, dict, set):
269+
with self.subTest(kind=kind):
270+
self.assertRaises(RuntimeError, marshal.dumps, make(kind))
271+
272+
# The length is unchanged, so the size recheck cannot fire: only
273+
# keeping the item alive across its __buffer__() avoids the crash.
274+
class Replacer:
275+
def __buffer__(self, flags):
276+
container[0] = b'.'
277+
return memoryview(bytearray(4))
278+
container = [Replacer()]
279+
result = marshal.loads(marshal.dumps(container))
280+
self.assertEqual(result, [b'\x00' * 4])
281+
282+
# Growing the set during marshalling is reported too, rather than
283+
# writing past the buffer pre-sized to the original length.
284+
class Grower:
285+
def __buffer__(self, flags):
286+
grown.add(b'grown')
287+
return memoryview(bytearray(4))
288+
grown = {Grower(), 1, 2}
289+
self.assertRaises(RuntimeError, marshal.dumps, grown)
290+
246291

247292
class BugsTestCase(unittest.TestCase):
248293
def test_bug_5888452(self):
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
Fixed a crash (use-after-free) in :func:`marshal.dumps` that could occur if an
2+
item's :meth:`~object.__buffer__` concurrently mutates the list, dict, or set
3+
being serialized. A resulting change in the container's size is now reported
4+
as a :exc:`RuntimeError`.

Python/marshal.c

Lines changed: 46 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@ module marshal
106106
#define WFERR_NESTEDTOODEEP 2
107107
#define WFERR_NOMEMORY 3
108108
#define WFERR_CODE_NOT_ALLOWED 4
109+
#define WFERR_CHANGED_SIZE 5
109110

110111
typedef struct {
111112
FILE *fp;
@@ -489,8 +490,13 @@ w_object(PyObject *v, WFILE *p)
489490
else if (v == Py_True) {
490491
w_byte(TYPE_TRUE, p);
491492
}
492-
else if (!w_ref(v, &flag, p))
493+
else if (!w_ref(v, &flag, p)) {
494+
/* Keep v alive while it is serialized: this may run Python (an item's
495+
__buffer__) that drops the caller's last reference to v. */
496+
Py_INCREF(v);
493497
w_complex_object(v, flag, p);
498+
Py_DECREF(v);
499+
}
494500

495501
p->depth--;
496502
}
@@ -603,6 +609,12 @@ w_complex_object(PyObject *v, char flag, WFILE *p)
603609
W_SIZE(n, p);
604610
for (i = 0; i < n; i++) {
605611
w_object(PyList_GET_ITEM(v, i), p);
612+
/* w_object() may run Python (e.g. an item's __buffer__) that
613+
resizes the list, leaving the captured size and index stale. */
614+
if (PyList_GET_SIZE(v) != n) {
615+
p->error = WFERR_CHANGED_SIZE;
616+
return;
617+
}
606618
}
607619
}
608620
else if (PyAnyDict_CheckExact(v)) {
@@ -621,10 +633,23 @@ w_complex_object(PyObject *v, char flag, WFILE *p)
621633
W_TYPE(TYPE_DICT, p);
622634
}
623635
/* This one is NULL object terminated! */
636+
n = PyDict_GET_SIZE(v);
624637
pos = 0;
625638
while (PyDict_Next(v, &pos, &key, &value)) {
639+
/* key and value are only borrowed; an item's __buffer__() may run
640+
Python that drops the dict's last reference to them. */
641+
Py_INCREF(key);
642+
Py_INCREF(value);
626643
w_object(key, p);
627644
w_object(value, p);
645+
Py_DECREF(key);
646+
Py_DECREF(value);
647+
/* That Python may also have resized the dict, making the
648+
PyDict_Next() position stale. */
649+
if (PyDict_GET_SIZE(v) != n) {
650+
p->error = WFERR_CHANGED_SIZE;
651+
return;
652+
}
628653
}
629654
w_object((PyObject *)NULL, p);
630655
if (PyFrozenDict_CheckExact(v)) {
@@ -654,6 +679,13 @@ w_complex_object(PyObject *v, char flag, WFILE *p)
654679
Py_ssize_t i = 0;
655680
Py_BEGIN_CRITICAL_SECTION(v);
656681
while (_PySet_NextEntryRef(v, &pos, &value, &hash)) {
682+
/* A previously serialized element's __buffer__ may have grown the
683+
set; i >= n stops a write past the pre-sized pairs list. */
684+
if (i >= n) {
685+
p->error = WFERR_CHANGED_SIZE;
686+
Py_DECREF(value);
687+
break;
688+
}
657689
PyObject *dump = _PyMarshal_WriteObjectToString(value,
658690
p->version, p->allow_code);
659691
if (dump == NULL) {
@@ -669,11 +701,18 @@ w_complex_object(PyObject *v, char flag, WFILE *p)
669701
PyList_SET_ITEM(pairs, i++, pair);
670702
}
671703
Py_END_CRITICAL_SECTION();
672-
if (p->error == WFERR_UNMARSHALLABLE || p->error == WFERR_NOMEMORY) {
704+
if (p->error == WFERR_UNMARSHALLABLE || p->error == WFERR_NOMEMORY
705+
|| p->error == WFERR_CHANGED_SIZE) {
706+
Py_DECREF(pairs);
707+
return;
708+
}
709+
if (i != n || PySet_GET_SIZE(v) != n) {
710+
/* The set was mutated while being marshalled (e.g. cleared by the
711+
last element's __buffer__, or an under-yield). */
712+
p->error = WFERR_CHANGED_SIZE;
673713
Py_DECREF(pairs);
674714
return;
675715
}
676-
assert(i == n);
677716
if (PyList_Sort(pairs)) {
678717
p->error = WFERR_NOMEMORY;
679718
Py_DECREF(pairs);
@@ -1941,6 +1980,10 @@ _PyMarshal_WriteObjectToString(PyObject *x, int version, int allow_code)
19411980
PyErr_SetString(PyExc_ValueError,
19421981
"marshalling code objects is disallowed");
19431982
break;
1983+
case WFERR_CHANGED_SIZE:
1984+
PyErr_SetString(PyExc_RuntimeError,
1985+
"dict, list or set changed size during iteration");
1986+
break;
19441987
default:
19451988
case WFERR_UNMARSHALLABLE:
19461989
PyErr_SetString(PyExc_ValueError,

0 commit comments

Comments
 (0)