Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions Lib/test/test_marshal.py
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,51 @@ def test_array(self):
new = marshal.loads(marshal.dumps(a))
self.assertEqual(new, b"abc")

def test_concurrent_buffer_mutation(self):
# An item's __buffer__() runs Python while the container is being
# marshalled, so the container can be concurrently mutated (simulated
# here by mutating it inside __buffer__).
# See: https://github.com/python/cpython/issues/151370
def make(kind):
# The item is only referenced from the container, so clearing the
# container inside __buffer__ drops its last reference to it.
class Item:
def __buffer__(self, flags):
container.clear()
return memoryview(bytearray(4))
item = Item()
if kind is dict:
container = {item: 1, 2: item}
elif kind is set:
container = {item, b'other'}
else:
container = [item, 1, 2]
return container

# A size change while marshalling is reported as a RuntimeError.
for kind in (list, dict, set):
with self.subTest(kind=kind):
self.assertRaises(RuntimeError, marshal.dumps, make(kind))

# The length is unchanged, so the size recheck cannot fire: only
# keeping the item alive across its __buffer__() avoids the crash.
class Replacer:
def __buffer__(self, flags):
container[0] = b'.'
return memoryview(bytearray(4))
container = [Replacer()]
result = marshal.loads(marshal.dumps(container))
self.assertEqual(result, [b'\x00' * 4])

# Growing the set during marshalling is reported too, rather than
# writing past the buffer pre-sized to the original length.
class Grower:
def __buffer__(self, flags):
grown.add(b'grown')
return memoryview(bytearray(4))
grown = {Grower(), 1, 2}
self.assertRaises(RuntimeError, marshal.dumps, grown)


class BugsTestCase(unittest.TestCase):
def test_bug_5888452(self):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Fixed a crash (use-after-free) in :func:`marshal.dumps` that could occur if an
item's :meth:`~object.__buffer__` concurrently mutates the list, dict, or set
being serialized. A resulting change in the container's size is now reported
as a :exc:`RuntimeError`.
49 changes: 46 additions & 3 deletions Python/marshal.c
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ module marshal
#define WFERR_NESTEDTOODEEP 2
#define WFERR_NOMEMORY 3
#define WFERR_CODE_NOT_ALLOWED 4
#define WFERR_CHANGED_SIZE 5

typedef struct {
FILE *fp;
Expand Down Expand Up @@ -489,8 +490,13 @@ w_object(PyObject *v, WFILE *p)
else if (v == Py_True) {
w_byte(TYPE_TRUE, p);
}
else if (!w_ref(v, &flag, p))
else if (!w_ref(v, &flag, p)) {
/* Keep v alive while it is serialized: this may run Python (an item's
__buffer__) that drops the caller's last reference to v. */
Py_INCREF(v);
w_complex_object(v, flag, p);
Py_DECREF(v);
}

p->depth--;
}
Expand Down Expand Up @@ -603,6 +609,12 @@ w_complex_object(PyObject *v, char flag, WFILE *p)
W_SIZE(n, p);
for (i = 0; i < n; i++) {
w_object(PyList_GET_ITEM(v, i), p);
/* w_object() may run Python (e.g. an item's __buffer__) that
resizes the list, leaving the captured size and index stale. */
if (PyList_GET_SIZE(v) != n) {
p->error = WFERR_CHANGED_SIZE;
return;
}
}
}
else if (PyAnyDict_CheckExact(v)) {
Expand All @@ -621,10 +633,23 @@ w_complex_object(PyObject *v, char flag, WFILE *p)
W_TYPE(TYPE_DICT, p);
}
/* This one is NULL object terminated! */
n = PyDict_GET_SIZE(v);
pos = 0;
while (PyDict_Next(v, &pos, &key, &value)) {
/* key and value are only borrowed; an item's __buffer__() may run
Python that drops the dict's last reference to them. */
Py_INCREF(key);
Py_INCREF(value);
w_object(key, p);
w_object(value, p);
Py_DECREF(key);
Py_DECREF(value);
/* That Python may also have resized the dict, making the
PyDict_Next() position stale. */
if (PyDict_GET_SIZE(v) != n) {
p->error = WFERR_CHANGED_SIZE;
return;
}
}
w_object((PyObject *)NULL, p);
if (PyFrozenDict_CheckExact(v)) {
Expand Down Expand Up @@ -654,6 +679,13 @@ w_complex_object(PyObject *v, char flag, WFILE *p)
Py_ssize_t i = 0;
Py_BEGIN_CRITICAL_SECTION(v);
while (_PySet_NextEntryRef(v, &pos, &value, &hash)) {
/* A previously serialized element's __buffer__ may have grown the
set; i >= n stops a write past the pre-sized pairs list. */
if (i >= n) {
p->error = WFERR_CHANGED_SIZE;
Py_DECREF(value);
break;
}
PyObject *dump = _PyMarshal_WriteObjectToString(value,
p->version, p->allow_code);
if (dump == NULL) {
Expand All @@ -669,11 +701,18 @@ w_complex_object(PyObject *v, char flag, WFILE *p)
PyList_SET_ITEM(pairs, i++, pair);
}
Py_END_CRITICAL_SECTION();
if (p->error == WFERR_UNMARSHALLABLE || p->error == WFERR_NOMEMORY) {
if (p->error == WFERR_UNMARSHALLABLE || p->error == WFERR_NOMEMORY
|| p->error == WFERR_CHANGED_SIZE) {
Py_DECREF(pairs);
return;
}
if (i != n || PySet_GET_SIZE(v) != n) {
/* The set was mutated while being marshalled (e.g. cleared by the
last element's __buffer__, or an under-yield). */
p->error = WFERR_CHANGED_SIZE;
Py_DECREF(pairs);
return;
}
assert(i == n);
if (PyList_Sort(pairs)) {
p->error = WFERR_NOMEMORY;
Py_DECREF(pairs);
Expand Down Expand Up @@ -1941,6 +1980,10 @@ _PyMarshal_WriteObjectToString(PyObject *x, int version, int allow_code)
PyErr_SetString(PyExc_ValueError,
"marshalling code objects is disallowed");
break;
case WFERR_CHANGED_SIZE:
PyErr_SetString(PyExc_RuntimeError,
"dict, list or set changed size during iteration");
break;
default:
case WFERR_UNMARSHALLABLE:
PyErr_SetString(PyExc_ValueError,
Expand Down
Loading