Repository navigation
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. |
|
|
||
| @support.nomemtest | ||
| def test_resize_error(self): | ||
| # gh-157242: If bytearray.resize() fails (MemoryError), |
There was a problem hiding this comment.
This comment needs adjusting for the new guarantees
| } | ||
|
|
||
| static int | ||
| bytearray_resize_lock_held(PyObject *self, Py_ssize_t requested_size) |
There was a problem hiding this comment.
When I'd use bytearray_resize_unchecked vs. bytearray_resize_lock_held doesn't make a lot of sense to me... all these checks seem like they're still valid to do for the "bytearray_setslice_linear" case which now skips them with bytearray_resize_unchecked
| The operation is completed, but a MemoryError is still | ||
| raised and the memory block is not shrunk. */ | ||
| Py_SET_SIZE(self, Py_SIZE(self) + growth); | ||
| bytearray_write_trailing_null_byte(self); |
There was a problem hiding this comment.
From my claude:
Right, no test in the suite covers it, as far as this run can show. I removed both the Py_SET_SIZE and the NUL write from the PR's failure branch, then ran the full suite on the debug build. The result was SUCCESS: 476 test files OK and 53,289 tests run.
I think we need a new remove from end test? (For most cases bytearray assumes bytes handles its trailing NULL byte)
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