Skip to content

Commit c424703

Browse files
committed
gh-151295: Fix use-after-free in bytes.join()/bytearray.join() via re-entrant __buffer__
1 parent 871047d commit c424703

3 files changed

Lines changed: 50 additions & 0 deletions

File tree

Lib/test/test_bytes.py

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -645,6 +645,44 @@ def test_join(self):
645645
with self.assertRaises(TypeError):
646646
dot_join([memoryview(b"ab"), "cd", b"ef"])
647647

648+
def test_join_reentrant_buffer_mutation(self):
649+
# An item's __buffer__() may run Python that drops the last reference
650+
# to that item by mutating the joined sequence.
651+
# See: https://github.com/python/cpython/issues/151295
652+
def make_seq(mutate):
653+
# The mutating item is only referenced from the list slot, so
654+
# mutate() drops its last reference mid-join.
655+
class Item:
656+
def __buffer__(self, flags):
657+
mutate(seq)
658+
return memoryview(b'x')
659+
seq = [b'a', Item(), b'c']
660+
return seq
661+
662+
class Benign:
663+
def __buffer__(self, flags):
664+
return memoryview(b'x')
665+
666+
for sep in (self.type2test(b''), self.type2test(b'::')):
667+
with self.subTest(sep=sep):
668+
# Clearing the list changes its length, which is reported as a
669+
# RuntimeError.
670+
seq = make_seq(lambda seq: seq.clear())
671+
self.assertRaises(RuntimeError, sep.join, seq)
672+
673+
# Replacing the item in place keeps the list length unchanged,
674+
# so the size-change recheck cannot fire; only keeping the item
675+
# alive across __buffer__() prevents the use-after-free, and
676+
# the join uses the buffer returned by __buffer__().
677+
def replace(seq):
678+
seq[1] = b'z'
679+
seq = make_seq(replace)
680+
self.assertEqual(sep.join(seq), sep.join([b'a', b'x', b'c']))
681+
682+
# A benign __buffer__() that does not mutate joins normally.
683+
self.assertEqual(sep.join([Benign(), b'Y', Benign()]),
684+
sep.join([b'x', b'Y', b'x']))
685+
648686
def test_count(self):
649687
b = self.type2test(b'mississippi')
650688
i = 105
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 :meth:`bytes.join` and
2+
:meth:`bytearray.join` that could occur if an item's
3+
:meth:`~object.__buffer__` concurrently mutates the sequence being joined.
4+
The mutation is now reported as a :exc:`RuntimeError` instead.

Objects/stringlib/join.h

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,13 +68,21 @@ STRINGLIB(bytes_join)(PyObject *sep, PyObject *iterable)
6868
buffers[i].len = PyBytes_GET_SIZE(item);
6969
}
7070
else {
71+
/* Keep item alive across PyObject_GetBuffer(): item is only
72+
borrowed from the sequence, and its __buffer__() may run
73+
Python that drops that last reference (e.g. by mutating the
74+
sequence being joined), freeing item while the buffer
75+
machinery is still using it. */
76+
Py_INCREF(item);
7177
if (PyObject_GetBuffer(item, &buffers[i], PyBUF_SIMPLE) != 0) {
78+
Py_DECREF(item);
7279
PyErr_Format(PyExc_TypeError,
7380
"sequence item %zd: expected a bytes-like object, "
7481
"%.80s found",
7582
i, Py_TYPE(item)->tp_name);
7683
goto error;
7784
}
85+
Py_DECREF(item);
7886
/* If the backing objects are mutable, then dropping the GIL
7987
* opens up race conditions where another thread tries to modify
8088
* the object which we hold a buffer on it. Such code has data

0 commit comments

Comments
 (0)