Skip to content

Commit beb3714

Browse files
StanFromIrelandcmaloneyvstinner
authored andcommitted
gh-156995: Fix bytearray.take_bytes() corrupting shared single-byte singletons (GH-156996)
(cherry picked from commit fd569ea) Co-authored-by: Stan Ulbrych <stan@python.org> Co-authored-by: Cody Maloney <cmaloney@users.noreply.github.com> Co-authored-by: Victor Stinner <vstinner@python.org>
1 parent 311df8c commit beb3714

5 files changed

Lines changed: 75 additions & 30 deletions

File tree

Lib/test/test_bytes.py

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1611,6 +1611,14 @@ def test_take_bytes(self):
16111611
self.assertRaises(BufferError, ba.take_bytes)
16121612
self.assertEqual(ba.take_bytes(), b'abc')
16131613

1614+
# Leaving one byte must not adopt the shared single-byte bytes object
1615+
# as the buffer.
1616+
ba = bytearray(b'abc')
1617+
self.assertEqual(ba.take_bytes(2), b'ab')
1618+
ba[0] = ord('A')
1619+
self.assertEqual(ba, bytearray(b'A'))
1620+
self.assertEqual(ord(b'c'), ord('c'))
1621+
16141622
@support.cpython_only # tests an implementation detail
16151623
def test_take_bytes_optimization(self):
16161624
# Validate optimization around taking lots of little chunks out of a
@@ -3031,5 +3039,18 @@ def resize_stress(ba):
30313039
with threading_helper.start_threads(threads):
30323040
pass
30333041

3042+
@threading_helper.reap_threads
3043+
@threading_helper.requires_working_threading()
3044+
def test_free_threading_bytearray_resize_other_thread(self):
3045+
# Shrinking a bytearray whose buffer another thread owns must not
3046+
# adopt the immortal single-byte bytes object a the buffer.
3047+
ba = bytearray(b'abc')
3048+
thread = threading.Thread(target=ba.resize, args=(1,))
3049+
with threading_helper.start_threads([thread]):
3050+
pass
3051+
ba[0] = ord('X')
3052+
self.assertEqual(ba, bytearray(b'X'))
3053+
self.assertEqual(ord(b'a'), ord('a'))
3054+
30343055
if __name__ == "__main__":
30353056
unittest.main()

Lib/test/test_capi/test_bytes.py

Lines changed: 34 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import sys
12
import unittest
23
from test.support import import_helper
34

@@ -231,26 +232,41 @@ def test_decodeescape(self):
231232

232233
def test_resize(self):
233234
"""Test _PyBytes_Resize()"""
234-
resize = _testcapi.bytes_resize
235+
_resize = _testcapi.bytes_resize
236+
237+
def resize(obj, size, new):
238+
result = _resize(obj, size, new)
239+
if 1 <= len(result):
240+
if new or size != len(obj):
241+
# gh-156995: Make sure that the result is a fresh object.
242+
# Previously, _PyBytes_Resize(&obj, 1) returned a singleton
243+
# if _PyObject_IsUniquelyReferenced() is false.
244+
self.assertEqual(sys.getrefcount(result), 1)
245+
self.assertFalse(sys._is_immortal(result))
246+
else:
247+
# check that the result is the empty bytes string singleton
248+
self.assertTrue(sys._is_immortal(result))
249+
return result
235250

236251
for new in True, False:
237-
self.assertEqual(resize(b'abc', 0, new), b'')
238-
self.assertEqual(resize(b'abc', 1, new), b'a')
239-
self.assertEqual(resize(b'abc', 2, new), b'ab')
240-
self.assertEqual(resize(b'abc', 3, new), b'abc')
241-
b = resize(b'abc', 4, new)
242-
self.assertEqual(len(b), 4)
243-
self.assertEqual(b[:3], b'abc')
244-
245-
self.assertEqual(resize(b'a', 0, new), b'')
246-
self.assertEqual(resize(b'a', 1, new), b'a')
247-
b = resize(b'a', 2, new)
248-
self.assertEqual(len(b), 2)
249-
self.assertEqual(b[:1], b'a')
250-
251-
self.assertEqual(resize(b'', 0, new), b'')
252-
self.assertEqual(len(resize(b'', 1, new)), 1)
253-
self.assertEqual(len(resize(b'', 2, new)), 2)
252+
with self.subTest(new=new):
253+
self.assertEqual(resize(b'abc', 0, new), b'')
254+
self.assertEqual(resize(b'abc', 1, new), b'a')
255+
self.assertEqual(resize(b'abc', 2, new), b'ab')
256+
self.assertEqual(resize(b'abc', 3, new), b'abc')
257+
b = resize(b'abc', 4, new)
258+
self.assertEqual(len(b), 4)
259+
self.assertEqual(b[:3], b'abc')
260+
261+
self.assertEqual(resize(b'a', 0, new), b'')
262+
self.assertEqual(resize(b'a', 1, new), b'a')
263+
b = resize(b'a', 2, new)
264+
self.assertEqual(len(b), 2)
265+
self.assertEqual(b[:1], b'a')
266+
267+
self.assertEqual(resize(b'', 0, new), b'')
268+
self.assertEqual(len(resize(b'', 1, new)), 1)
269+
self.assertEqual(len(resize(b'', 2, new)), 2)
254270

255271
self.assertRaises(SystemError, resize, b'abc', -1, False)
256272
self.assertRaises(SystemError, resize, bytearray(b'abc'), 3, False)
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
Fix :class:`bytearray` sharing its buffer with the single-byte :class:`bytes`
2+
object of the same value, so that writing to the bytearray modified that
3+
:class:`bytes` object. This happened with :meth:`bytearray.take_bytes` when
4+
exactly one byte remained, and on the free-threaded build when a bytearray was
5+
shrunk to one byte from another thread.

Objects/bytearrayobject.c

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,10 @@ _getbytevalue(PyObject* arg, int *value)
4545

4646
static void
4747
bytearray_reinit_from_bytes(PyByteArrayObject *self, Py_ssize_t size,
48-
Py_ssize_t alloc) {
48+
Py_ssize_t alloc)
49+
{
50+
/* Only the empty bytes may be immortal. */
51+
assert((alloc == 0) == _Py_IsImmortal(self->ob_bytes_object));
4952
self->ob_bytes = self->ob_start = PyBytes_AS_STRING(self->ob_bytes_object);
5053
Py_SET_SIZE(self, size);
5154
FT_ATOMIC_STORE_SSIZE_RELAXED(self->ob_alloc, alloc);
@@ -1616,12 +1619,14 @@ bytearray_take_bytes_impl(PyByteArrayObject *self, PyObject *n)
16161619
return ret;
16171620
}
16181621

1619-
// Copy remaining bytes to a new bytes.
1620-
PyObject *remaining = PyBytes_FromStringAndSize(self->ob_start + to_take,
1621-
remaining_length);
1622+
// Copy remaining bytes to a new bytes. Allocate and then copy
1623+
// so we don't get a shared immortal one-character singleton!
1624+
PyObject *remaining = PyBytes_FromStringAndSize(NULL, remaining_length);
16221625
if (remaining == NULL) {
16231626
return NULL;
16241627
}
1628+
memcpy(PyBytes_AS_STRING(remaining), self->ob_start + to_take,
1629+
remaining_length);
16251630

16261631
// If the bytes are offset inside the buffer must first align.
16271632
if (self->ob_start != self->ob_bytes) {

Objects/bytesobject.c

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3368,14 +3368,12 @@ _PyBytes_Resize(PyObject **pv, Py_ssize_t newsize)
33683368
return 0;
33693369
}
33703370
if (!_PyObject_IsUniquelyReferenced(v)) {
3371-
if (oldsize < newsize) {
3372-
*pv = _PyBytes_FromSize(newsize, 0);
3373-
if (*pv) {
3374-
memcpy(PyBytes_AS_STRING(*pv), PyBytes_AS_STRING(v), oldsize);
3375-
}
3376-
}
3377-
else {
3378-
*pv = PyBytes_FromStringAndSize(PyBytes_AS_STRING(v), newsize);
3371+
// Allocate and then copy so we don't get a shared immortal
3372+
// one-character singleton!
3373+
*pv = _PyBytes_FromSize(newsize, 0);
3374+
if (*pv) {
3375+
memcpy(PyBytes_AS_STRING(*pv), PyBytes_AS_STRING(v),
3376+
Py_MIN(oldsize, newsize));
33793377
}
33803378
Py_DECREF(v);
33813379
return (*pv == NULL) ? -1 : 0;

0 commit comments

Comments
 (0)