Skip to content

Commit 4732806

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 4732806

3 files changed

Lines changed: 92 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: 43 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: serializing it may run Python (an item's __buffer__)
495+
that drops the caller's last reference. */
496+
Py_INCREF(v);
493497
w_complex_object(v, flag, p);
498+
Py_DECREF(v);
499+
}
494500

495501
p->depth--;
496502
}
@@ -603,6 +609,11 @@ 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+
/* Serializing an item may have resized the list; n is stale. */
613+
if (PyList_GET_SIZE(v) != n) {
614+
p->error = WFERR_CHANGED_SIZE;
615+
return;
616+
}
606617
}
607618
}
608619
else if (PyAnyDict_CheckExact(v)) {
@@ -621,10 +632,22 @@ w_complex_object(PyObject *v, char flag, WFILE *p)
621632
W_TYPE(TYPE_DICT, p);
622633
}
623634
/* This one is NULL object terminated! */
635+
n = PyDict_GET_SIZE(v);
624636
pos = 0;
625637
while (PyDict_Next(v, &pos, &key, &value)) {
638+
/* key and value are only borrowed; an item's __buffer__() may run
639+
Python that drops the dict's last reference to them. */
640+
Py_INCREF(key);
641+
Py_INCREF(value);
626642
w_object(key, p);
627643
w_object(value, p);
644+
Py_DECREF(key);
645+
Py_DECREF(value);
646+
/* It may also have resized the dict, making pos stale. */
647+
if (PyDict_GET_SIZE(v) != n) {
648+
p->error = WFERR_CHANGED_SIZE;
649+
return;
650+
}
628651
}
629652
w_object((PyObject *)NULL, p);
630653
if (PyFrozenDict_CheckExact(v)) {
@@ -654,6 +677,13 @@ w_complex_object(PyObject *v, char flag, WFILE *p)
654677
Py_ssize_t i = 0;
655678
Py_BEGIN_CRITICAL_SECTION(v);
656679
while (_PySet_NextEntryRef(v, &pos, &value, &hash)) {
680+
/* An earlier element may have grown the set; i >= n avoids
681+
writing past the pre-sized pairs list. */
682+
if (i >= n) {
683+
p->error = WFERR_CHANGED_SIZE;
684+
Py_DECREF(value);
685+
break;
686+
}
657687
PyObject *dump = _PyMarshal_WriteObjectToString(value,
658688
p->version, p->allow_code);
659689
if (dump == NULL) {
@@ -669,11 +699,17 @@ w_complex_object(PyObject *v, char flag, WFILE *p)
669699
PyList_SET_ITEM(pairs, i++, pair);
670700
}
671701
Py_END_CRITICAL_SECTION();
672-
if (p->error == WFERR_UNMARSHALLABLE || p->error == WFERR_NOMEMORY) {
702+
if (p->error == WFERR_UNMARSHALLABLE || p->error == WFERR_NOMEMORY
703+
|| p->error == WFERR_CHANGED_SIZE) {
704+
Py_DECREF(pairs);
705+
return;
706+
}
707+
if (i != n || PySet_GET_SIZE(v) != n) {
708+
/* The set was mutated while being marshalled. */
709+
p->error = WFERR_CHANGED_SIZE;
673710
Py_DECREF(pairs);
674711
return;
675712
}
676-
assert(i == n);
677713
if (PyList_Sort(pairs)) {
678714
p->error = WFERR_NOMEMORY;
679715
Py_DECREF(pairs);
@@ -1941,6 +1977,10 @@ _PyMarshal_WriteObjectToString(PyObject *x, int version, int allow_code)
19411977
PyErr_SetString(PyExc_ValueError,
19421978
"marshalling code objects is disallowed");
19431979
break;
1980+
case WFERR_CHANGED_SIZE:
1981+
PyErr_SetString(PyExc_RuntimeError,
1982+
"dict, list or set changed size during iteration");
1983+
break;
19441984
default:
19451985
case WFERR_UNMARSHALLABLE:
19461986
PyErr_SetString(PyExc_ValueError,

0 commit comments

Comments
 (0)