Repository navigation
gh-160303: Fix bytearray state after a failed del b[:n] - #160304
Conversation
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.
cmaloney
left a comment
cmaloney
left a comment
There was a problem hiding this comment.
@vstinner recently made it so that failed allocations keep the pre-existing bytes (#157242). That means we should always have a valid bytes. Backported to 3.15 (3.14 has a different internal bytearray structure).
bytearray implements a fast delete at front by having an ob_start member which can be incremented. Also fast delete at end: Leave ob_alloc as is and call Py_SET_SIZE to set the new size. The "effective" bytes being smaller than the allocation is okay.
The bug report sounds like we're getting that pointer manipulation off? I would strongly prefer not allocating a new bytes here with PyBytes_FromStringAndSize
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.
|
Reworked to keep ... we could have a failed shrink not raise at all, since the larger block is valid? I still like the error myself. |
…hecked 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(). Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
gpshead
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @gpshead for commit 8601525 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F160304%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
|
Thanks @gpshead for the PR 🌮🎉.. I'm working now to backport this PR to: 3.15. |
|
GH-161836 is a backport of this pull request to the 3.15 branch. |
…0304) (#161836) After a failed `del b[:n]`, `bytearray_setslice_linear()` moved `ob_start` back, but `bytearray_resize_storage()` had already realigned the data and reset `ob_start`, so the bytearray then started `n` bytes before its buffer. The front-delete path now moves `ob_start` and the size before the shrinking resize and no longer restores anything, so a failed shrink leaves the bytes removed and the larger block in place, raising `MemoryError` as the non-front path already does. The checks in `bytearray_resize_lock_held()` are split from a new `bytearray_resize_unchecked()` so the slice code can call it directly without changing other callers, and the non-front failure path now writes the trailing null byte it was missing. Fixes #160303 ANT-2026-QRZ6IDTC <!-- gh-issue-number: gh-160303 --> * Issue: gh-160303 <!-- /gh-issue-number --> (cherry picked from commit 35e2c5a) Co-authored-by: Gregory P. Smith <68491+gpshead@users.noreply.github.com>
…GH-160304) * pythongh-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. * pythongh-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. * pythongh-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().
After a failed
del b[:n],bytearray_setslice_linear()movedob_startback, butbytearray_resize_storage()had already realigned the data and resetob_start, so the bytearray then startednbytes before its buffer. The front-delete path now movesob_startand the size before the shrinking resize and no longer restores anything, so a failed shrink leaves the bytes removed and the larger block in place, raisingMemoryErroras the non-front path already does. The checks inbytearray_resize_lock_held()are split from a newbytearray_resize_unchecked()so the slice code can call it directly without changing other callers, and the non-front failure path now writes the trailing null byte it was missing.Fixes #160303
ANT-2026-QRZ6IDTC
bytearrayreads and writes outside its buffer afterdel b[:n]fails withMemoryError#160303