Skip to content

Commit 35e2c5a

Browse files
authored
gh-160303: Fix bytearray state after a failed del b[:n] (GH-160304)
* gh-160303: Fix bytearray state after a failed del b[:n] When shrinking the storage of a bytearray which has a logical offset, allocate the new buffer before modifying the object, so that bytearray_setslice_linear() can restore ob_start if the allocation fails. * gh-160303: Keep ob_start instead of allocating in del b[:n] Move ob_start and the size before the shrinking resize, so a failed shrink leaves the bytes removed and the larger block in place, as the non-front path already does. Split the checks out of bytearray_resize_lock_held() so the slice code can resize directly. Move the tests into test_resize_error. * gh-160303: Test failed shrinks not at the front; narrow the unchecked resize Only the front path of bytearray_setslice_linear() calls bytearray_resize_unchecked(): it has already advanced ob_start and reduced the size, so bytearray_resize_lock_held() would return early without releasing memory. Other slices go through bytearray_resize_lock_held() again and keep the failure fixup to themselves. test_resize_error now also covers slices in the middle and at the end, and checks the null byte after the data with float().
1 parent 9f03afd commit 35e2c5a

3 files changed

Lines changed: 91 additions & 38 deletions

File tree

‎Lib/test/test_bytes.py‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1661,6 +1661,10 @@ def test_resize(self):
16611661
def test_resize_error(self):
16621662
# gh-157242: If bytearray.resize() fails (MemoryError),
16631663
# the bytearray must be left unchanged.
1664+
#
1665+
# gh-160303: If a slice is deleted or replaced by shorter data and
1666+
# shrinking the memory block fails (MemoryError), the bytearray
1667+
# must be modified anyway.
16641668

16651669
offset = 3
16661670
for logical_offset in (False, True):
@@ -1687,6 +1691,33 @@ def test_resize_error(self):
16871691
ba.resize(1)
16881692
self.assertEqual(ba, expected)
16891693

1694+
# shrink bytearray with a slice at the front, in the middle
1695+
# and at the end
1696+
data = b'0123456789'
1697+
if logical_offset:
1698+
data = data[offset:]
1699+
for start, stop in ((0, -2), (1, -1), (2, len(data))):
1700+
for replacement in (b'', b'.'):
1701+
with self.subTest(start=start, stop=stop,
1702+
replacement=replacement):
1703+
ba = bytearray(b'0123456789')
1704+
if logical_offset:
1705+
del ba[:offset]
1706+
index = slice(start, stop)
1707+
value = bytearray(replacement)
1708+
expected = data[:start] + replacement + data[stop:]
1709+
with inject_memory_error(self):
1710+
if replacement:
1711+
ba[index] = value
1712+
else:
1713+
del ba[index]
1714+
self.assertEqual(ba, expected)
1715+
# float() relies on the null byte after the data.
1716+
self.assertEqual(float(ba), float(expected))
1717+
ba.append(ord('5'))
1718+
del ba[:1]
1719+
self.assertEqual(ba, expected[1:] + b'5')
1720+
16901721
def test_take_bytes(self):
16911722
ba = bytearray(b'ab')
16921723
self.assertEqual(ba.take_bytes(), b'ab')
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
Fix :class:`bytearray` reading and writing memory outside of its buffer
2+
after ``del b[:n]`` or ``b[:n] = shorter`` failed with :exc:`MemoryError`.
3+
The bytes are now removed even if shrinking the memory block fails;
4+
:exc:`MemoryError` is still raised. Also fix the missing null byte after the
5+
data when shrinking fails for a slice which is not at the front.

‎Objects/bytearrayobject.c‎

Lines changed: 55 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -266,37 +266,31 @@ bytearray_resize_storage(PyByteArrayObject *self,
266266
}
267267

268268

269+
/* Resize without the checks of bytearray_resize_lock_held(): the caller must
270+
have checked that the bytearray can be resized.
271+
272+
Unlike bytearray_resize_lock_held(), this does not return early if
273+
requested_size is already the size of the bytearray. A caller which
274+
removed bytes by advancing ob_start and reducing the size uses that to
275+
release the memory. */
269276
static int
270-
bytearray_resize_lock_held(PyObject *self, Py_ssize_t requested_size)
277+
bytearray_resize_unchecked(PyByteArrayObject *obj, Py_ssize_t requested_size)
271278
{
272-
_Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(self);
273-
PyByteArrayObject *obj = ((PyByteArrayObject *)self);
279+
_Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(obj);
280+
PyObject *self = (PyObject *)obj;
274281

275282
assert(obj->ob_bytes_object != NULL);
283+
assert(requested_size >= 0);
284+
assert(obj->ob_exports == 0);
276285

277286
/* All computations are done unsigned to avoid integer overflows
278287
(see issue #22335). */
279288
size_t alloc = (size_t) obj->ob_alloc;
280289
size_t logical_offset = (size_t) (obj->ob_start - obj->ob_bytes);
281290
size_t size = (size_t) requested_size;
282291

283-
assert(self != NULL);
284-
assert(PyByteArray_Check(self));
285292
assert(logical_offset <= alloc);
286293

287-
if (requested_size < 0) {
288-
PyErr_Format(PyExc_ValueError,
289-
"Can only resize to positive sizes, got %zd", requested_size);
290-
return -1;
291-
}
292-
293-
if (requested_size == Py_SIZE(self)) {
294-
return 0;
295-
}
296-
if (!_canresize(obj)) {
297-
return -1;
298-
}
299-
300294
/* Resize to 0 resets to empty bytes (see issue #153419). */
301295
if (requested_size == 0) {
302296
Py_SETREF(obj->ob_bytes_object,
@@ -348,6 +342,30 @@ bytearray_resize_lock_held(PyObject *self, Py_ssize_t requested_size)
348342
return 0;
349343
}
350344

345+
static int
346+
bytearray_resize_lock_held(PyObject *self, Py_ssize_t requested_size)
347+
{
348+
_Py_CRITICAL_SECTION_ASSERT_OBJECT_LOCKED(self);
349+
PyByteArrayObject *obj = ((PyByteArrayObject *)self);
350+
351+
assert(self != NULL);
352+
assert(PyByteArray_Check(self));
353+
354+
if (requested_size < 0) {
355+
PyErr_Format(PyExc_ValueError,
356+
"Can only resize to positive sizes, got %zd", requested_size);
357+
return -1;
358+
}
359+
360+
if (requested_size == Py_SIZE(self)) {
361+
return 0;
362+
}
363+
if (!_canresize(obj)) {
364+
return -1;
365+
}
366+
return bytearray_resize_unchecked(obj, requested_size);
367+
}
368+
351369
int
352370
PyByteArray_Resize(PyObject *self, Py_ssize_t requested_size)
353371
{
@@ -612,14 +630,22 @@ bytearray_setslice_linear(PyByteArrayObject *self,
612630
return -1;
613631

614632
if (lo == 0) {
615-
/* Shrink the buffer by advancing its logical start */
633+
/* Shrink the buffer by advancing its logical start. The bytes
634+
are removed at this point; the resize below only releases
635+
memory. */
616636
self->ob_start -= growth;
637+
Py_SET_SIZE(self, Py_SIZE(self) + growth);
617638
/*
618639
0 lo hi old_size
619640
| |<----avail----->|<-----tail------>|
620641
| |<-bytes_len->|<-----tail------>|
621642
0 new_lo new_hi new_size
622643
*/
644+
if (bytearray_resize_unchecked(self, Py_SIZE(self)) < 0) {
645+
/* The operation is completed, but a MemoryError is still
646+
raised and the memory block is not shrunk. */
647+
res = -1;
648+
}
623649
}
624650
else {
625651
/*
@@ -630,26 +656,17 @@ bytearray_setslice_linear(PyByteArrayObject *self,
630656
*/
631657
memmove(buf + lo + bytes_len, buf + hi,
632658
Py_SIZE(self) - hi);
633-
}
634-
if (bytearray_resize_lock_held((PyObject *)self,
635-
Py_SIZE(self) + growth) < 0) {
636-
/* Issue #19578: Handling the memory allocation failure here is
637-
tricky here because the bytearray object has already been
638-
modified. Depending on growth and lo, the behaviour is
639-
different.
640-
641-
If growth < 0 and lo != 0, the operation is completed, but a
642-
MemoryError is still raised and the memory block is not
643-
shrunk. Otherwise, the bytearray is restored in its previous
644-
state and a MemoryError is raised. */
645-
if (lo == 0) {
646-
self->ob_start += growth;
647-
return -1;
659+
if (bytearray_resize_lock_held((PyObject *)self,
660+
Py_SIZE(self) + growth) < 0) {
661+
/* Issue #19578: memmove() removed bytes, the bytearray
662+
object cannot be restored in its previous state.
663+
664+
The operation is completed, but a MemoryError is still
665+
raised and the memory block is not shrunk. */
666+
Py_SET_SIZE(self, Py_SIZE(self) + growth);
667+
bytearray_write_trailing_null_byte(self);
668+
res = -1;
648669
}
649-
/* memmove() removed bytes, the bytearray object cannot be
650-
restored in its previous state. */
651-
Py_SET_SIZE(self, Py_SIZE(self) + growth);
652-
res = -1;
653670
}
654671
buf = PyByteArray_AS_STRING(self);
655672
}

0 commit comments

Comments
 (0)