Skip to content

gh-91153: Fix bytearray holding a reference to its internal buffer when calling into potentially mutating __index__ methods #132379

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
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
35 changes: 35 additions & 0 deletions Lib/test/test_bytes.py
Original file line number Diff line number Diff line change
Expand Up @@ -1872,6 +1872,8 @@ def test_repeat_after_setslice(self):
self.assertEqual(b3, b'xcxcxc')

def test_mutating_index(self):
# bytearray slice assignment can call into python code
# that reallocates the internal buffer
# See gh-91153

class Boom:
Expand All @@ -1889,6 +1891,39 @@ def __index__(self):
with self.assertRaises(IndexError):
self._testlimitedcapi.sequence_setitem(b, 0, Boom())

def test_mutating_index_inbounds(self):
# gh-91153 continued
# Ensure buffer is not broken even if length is correct

class MutatesOnIndex:
def __init__(self):
self.ba = bytearray(0x180)

def __index__(self):
self.ba.clear()
self.new_ba = bytearray(0x180) # to catch out-of-bounds writes
self.ba.extend([0] * 0x180) # to check bounds checks
return 0

with self.subTest("skip_bounds_safety"):
instance = MutatesOnIndex()
instance.ba[instance] = ord("?")
self.assertEqual(instance.ba[0], ord("?"), "Assigned bytearray not altered")
self.assertEqual(instance.new_ba, bytearray(0x180), "Wrong object altered")

with self.subTest("skip_bounds_safety_capi"):
instance = MutatesOnIndex()
instance.ba[instance] = ord("?")
self._testlimitedcapi.sequence_setitem(instance.ba, instance, ord("?"))
self.assertEqual(instance.ba[0], ord("?"), "Assigned bytearray not altered")
self.assertEqual(instance.new_ba, bytearray(0x180), "Wrong object altered")

with self.subTest("skip_bounds_safety_slice"):
instance = MutatesOnIndex()
instance.ba[instance:1] = [ord("?")]
self.assertEqual(instance.ba[0], ord("?"), "Assigned bytearray not altered")
self.assertEqual(instance.new_ba, bytearray(0x180), "Wrong object altered")


class AssortedBytesTest(unittest.TestCase):
#
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a crash when a :class:`bytearray` is concurrently mutated during item assignment.
8 changes: 6 additions & 2 deletions Objects/bytearrayobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -709,7 +709,9 @@ bytearray_ass_subscript_lock_held(PyObject *op, PyObject *index, PyObject *value
_Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(op);
PyByteArrayObject *self = _PyByteArray_CAST(op);
Py_ssize_t start, stop, step, slicelen;
char *buf = PyByteArray_AS_STRING(self);
// Do not store a reference to the internal buffer since
// index.__index__() or _getbytevalue() may alter 'self'.
// See https://github.com/python/cpython/issues/91153.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to fully link to it, just the gh- reference should be fine.

Suggested change
// See https://github.com/python/cpython/issues/91153.
// See gh-91153

Copy link
Author

@bast0006 bast0006 Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

picnixz suggested this above, and it's common for the c source to have complete links, so I think it's fine as-is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With complete links, I can open them from my IDE. It's much easier that way IMO


if (_PyIndex_Check(index)) {
Py_ssize_t i = PyNumber_AsSsize_t(index, PyExc_IndexError);
Expand Down Expand Up @@ -744,7 +746,7 @@ bytearray_ass_subscript_lock_held(PyObject *op, PyObject *index, PyObject *value
}
else {
assert(0 <= ival && ival < 256);
buf[i] = (char)ival;
PyByteArray_AS_STRING(self)[i] = (char)ival;
return 0;
}
}
Expand Down Expand Up @@ -805,6 +807,7 @@ bytearray_ass_subscript_lock_held(PyObject *op, PyObject *index, PyObject *value
/* Delete slice */
size_t cur;
Py_ssize_t i;
char* buf = PyByteArray_AS_STRING(self);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also hold PyByteArray_GET_SIZE(self) temporarily here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but I'm not as familiar with this system or C as I would want to be to feel comfortable making that change. Especially since the relevant code is mostly >10 years old and that change is not immediately required to solve the issue.


if (!_canresize(self))
return -1;
Expand Down Expand Up @@ -845,6 +848,7 @@ bytearray_ass_subscript_lock_held(PyObject *op, PyObject *index, PyObject *value
/* Assign slice */
Py_ssize_t i;
size_t cur;
char* buf = PyByteArray_AS_STRING(self);

if (needed != slicelen) {
PyErr_Format(PyExc_ValueError,
Expand Down
Loading