Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
33 changes: 22 additions & 11 deletions crates/vm/src/builtins/bytearray.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,11 +173,6 @@ impl PyByteArray {
fn inner(&self) -> PyDetachingRwLockReadGuard<'_, PyBytesInner> {
self.inner.read()
}
#[inline]
fn inner_mut(&self) -> PyDetachingRwLockWriteGuard<'_, PyBytesInner> {
self.inner.write()
}

fn __len__(&self) -> usize {
self.borrow_buf().len()
}
Expand Down Expand Up @@ -781,9 +776,18 @@ impl Initializer for PyByteArray {
type Args = ByteInnerNewOptions;

fn init(zelf: &Py<Self>, options: Self::Args, vm: &VirtualMachine) -> PyResult<()> {
// First unpack bytearray and *then* get a lock to set it.
let mut inner = options.get_inner(bytearray_from_object, vm)?;
core::mem::swap(&mut *zelf.inner_mut(), &mut inner);
// Reject exports and clear existing contents before converting source.
zelf.try_resizable(vm)?.elements.clear();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Allow no-op initialization of exported empty bytearrays

When an empty bytearray already has a live memoryview, __init__() and __init__(0) perform no resize; CPython permits both and still invokes conversion callbacks for an int-like zero. The unconditional resize guard here instead raises BufferError before options conversion, regressing the previously successful no-op and suppressing those callbacks. Skip the initial guard/clear when the current length is zero and only reject if the eventual result actually needs a resize.

AGENTS.md reference: AGENTS.md:L7-L7

Useful? React with 👍 / 👎.

// Conversion can call Python, so do not hold the lock across it and
// check again for exports acquired by a callback or another thread.
let (mut inner, zero_count) =
options.get_inner_with_zero_count(bytearray_from_object, vm)?;
// A zero integer count performs no further writes, including when its
// __index__ callback changed this bytearray or acquired an export.
if zero_count {
return Ok(());
Comment on lines +935 to +936

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Release the previous allocation for zero-count initialization

When a large existing bytearray is reinitialized with an integer (or int-like value) of zero, this early return follows Vec::clear(), which preserves capacity, so bytearray.__alloc__() continues to expose the old allocation and potentially gigabytes remain held for the object's lifetime. The old implementation swapped in the newly created empty inner and released that storage; replace the backing vector while performing the initial clear so the zero-count callback behavior is kept without retaining the old allocation.

Useful? React with 👍 / 👎.

}
core::mem::swap(&mut *zelf.try_resizable(vm)?, &mut inner);
Comment on lines +935 to +938

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Treat source-less initialization as complete after clearing

When bytearray.__init__() is called without a source and another thread acquires a memoryview after the initial clear, get_inner_with_zero_count leaves this flag false, so the second resize guard raises BufferError. The call has already discarded the old contents and has no remaining write to perform, making a completed clear appear to fail; mark the missing-source case as no-further-write or keep the initial guard through that callback-free path.

Useful? React with 👍 / 👎.

Ok(())
}
}
Expand Down Expand Up @@ -841,9 +845,11 @@ impl AsBuffer for PyByteArray {
}

fn as_buffer(zelf: &Py<Self>, _vm: &VirtualMachine) -> PyResult<PyBuffer> {
// Keep the length stable until PyBuffer::new retains the export.
let inner = zelf.inner();
Ok(PyBuffer::new(
zelf.to_owned().into(),
BufferDescriptor::simple(zelf.__len__(), false),
BufferDescriptor::simple(inner.elements.len(), false),
&BUFFER_METHODS,
))
}
Expand All @@ -855,7 +861,12 @@ impl BufferResizeGuard for PyByteArray {
fn try_resizable_opt(&self) -> Option<Self::Resizable<'_>> {
// An export is a borrow someone else still holds, so it is answered
// before the lock rather than by waiting on it.
(self.exports.load(Ordering::SeqCst) == 0).then(|| self.inner.write())
if self.exports.load(Ordering::SeqCst) != 0 {
return None;
}
let inner = self.inner.write();
// An export may have been acquired while we waited for the lock.
(self.exports.load(Ordering::SeqCst) == 0).then_some(inner)
}
}

Expand Down Expand Up @@ -907,7 +918,7 @@ impl AsSequence for PyByteArray {
if let Some(value) = value {
zelf._setitem_by_index(i, &value, vm)
} else {
zelf.borrow_buf_mut().delitem_by_index(vm, i)
zelf.try_resizable(vm)?.elements.delitem_by_index(vm, i)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Validate the sequence index before rejecting an export

When a C extension calls PySequence_DelItem on a bytearray with a live export and a positive index at or beyond the current length, this guard runs before delitem_by_index can validate the bounds, so the call now raises BufferError; CPython's bytearray sequence slot reports IndexError for such an index without attempting a resize. Validate the index before acquiring the resize guard, then revalidate under the guard, so only a valid deletion is treated as a resize.

AGENTS.md reference: AGENTS.md:L7-L7

Useful? React with 👍 / 👎.

}
}),
contains: atomic_func!(|seq, other, vm| {
Expand Down
19 changes: 16 additions & 3 deletions crates/vm/src/bytes_inner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -225,12 +225,24 @@ impl ByteInnerNewOptions {
/// read: `bytes()` and `bytearray()` differ in whether they ask it how long
/// it is.
pub fn get_inner(self, from_object: FromObject, vm: &VirtualMachine) -> PyResult<PyBytesInner> {
match (self.source, self.encoding, self.errors) {
self.get_inner_with_zero_count(from_object, vm)
.map(|(inner, _)| inner)
}

pub(crate) fn get_inner_with_zero_count(
self,
from_object: FromObject,
vm: &VirtualMachine,
) -> PyResult<(PyBytesInner, bool)> {
let mut zero_count = false;
let inner = match (self.source, self.encoding, self.errors) {
(OptionalArg::Present(obj), OptionalArg::Missing, OptionalArg::Missing) => {
// Try __index__ first to handle int-like objects that might raise custom exceptions
if let Some(index_result) = obj.try_index_opt(vm) {
match index_result {
Ok(index) => Self::get_value_from_size(&index, vm),
Ok(index) => Self::get_value_from_size(&index, vm).inspect(|inner| {
zero_count = inner.elements.is_empty();
}),
Err(e) => {
// Only propagate non-TypeError exceptions
// TypeError means the object doesn't support __index__, so fall back
Expand Down Expand Up @@ -266,7 +278,8 @@ impl ByteInnerNewOptions {
(OptionalArg::Present(_), OptionalArg::Missing, OptionalArg::Present(_)) => {
Err(vm.new_type_error(STRING_WITHOUT_ENCODING.to_owned()))
}
}
}?;
Ok((inner, zero_count))
}
}

Expand Down
11 changes: 7 additions & 4 deletions crates/vm/src/stdlib/_io.rs
Original file line number Diff line number Diff line change
Expand Up @@ -762,10 +762,13 @@ mod _io {
)));
}
let n = n as usize;
let mut bytes = b.borrow_buf_mut();
bytes.truncate(n);
// FIXME: try to use Arc::unwrap on the bytearray to get at the inner buffer
bytes.clone().to_pyobject(vm)
let mut inner = b.try_resizable(vm)?;
let bytes = &mut inner.elements;
bytes
.try_reserve_exact(n.saturating_sub(bytes.len()))
.map_err(|_| vm.no_memory_error())?;
bytes.resize(n, 0);
core::mem::take(bytes).to_pyobject(vm)
}
})
} else {
Expand Down
136 changes: 136 additions & 0 deletions extra_tests/snippets/builtin_bytearray_exports.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
"""Resizing paths must respect outstanding bytearray exports."""

import io

from testutils import assert_raises

for args in ((), (b"abc",), (b"x",), (b"",), (object(),)):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Remove the prohibited test-logic additions

Remove this new regression snippet and the two sibling builtin_bytearray_* snippets: the repository's critical test policy allows test-file edits only for adding or removing expected-failure markers, whereas these files introduce new assertions, control flow, and test data. Keeping them in this change violates the contribution rules even if the tests pass.

AGENTS.md reference: AGENTS.md:L273-L279

Useful? React with 👍 / 👎.

value = bytearray(b"abc")
with memoryview(value) as view:
with assert_raises(BufferError):
value.__init__(*args)
assert value == b"abc"
assert view.tobytes() == b"abc"
value.__init__(b"released")
assert value == b"released"

value = bytearray(b"abc")
with assert_raises(TypeError):
value.__init__(object())
assert value == b""
value = bytearray(b"abc")
value.__init__(value)
assert value == b""


class ExportDuringInit:
def __init__(self, count, payload=b""):
self.count = count
self.payload = payload
self.calls = 0

def __index__(self):
self.calls += 1
value.extend(self.payload)
self.view = memoryview(value)
return self.count


value = bytearray(b"abc")
source = ExportDuringInit(2)
with assert_raises(BufferError):
value.__init__(source)
assert value == b""
assert source.view.tobytes() == b""
assert source.calls == 1
source.view.release()
value.extend(b"released")
assert value == b"released"

value = bytearray(b"abc")
source = ExportDuringInit(0)
value.__init__(source)
assert value == b""
assert source.view.tobytes() == b""
source.view.release()

value = bytearray(b"abc")
source = ExportDuringInit(0, b"xy")
value.__init__(source)
assert value == b"xy"
assert source.view.tobytes() == b"xy"
assert source.calls == 1
source.view.release()


class FailingIndex:
def __index__(self):
assert value == b""
raise ValueError("conversion failed")


value = bytearray(b"abc")
with assert_raises(ValueError):
value.__init__(FailingIndex())
assert value == b""

for key in (0, slice(0, 1)):
value = bytearray(b"abc")
with memoryview(value) as view:
with assert_raises(BufferError):
del value[key]
assert view.tobytes() == b"abc"
del value[key]
assert value == b"bc"


class Reader(io.RawIOBase):
def __init__(self, count, retain, payload=b"abcd"):
self.count = count
self.retain = retain
self.payload = payload

def readinto(self, buffer):
buffer[:] = self.payload
self.buffer = buffer
if self.retain:
self.view = memoryview(buffer)
return self.count


for count in (0, 2, 4):
reader = Reader(count, False)
assert reader.read(4) == b"abcd"[:count]
assert reader.buffer == b""

reader = Reader(count, True)
with assert_raises(BufferError):
reader.read(4)
assert reader.view.tobytes() == b"abcd"
reader.view.release()
reader.buffer.clear()

for count in (-1, 5):
reader = Reader(count, True)
with assert_raises(ValueError):
reader.read(4)
assert reader.view.tobytes() == b"abcd"
reader.view.release()

reader = Reader(None, True)
assert reader.read(4) is None
assert reader.view.tobytes() == b"abcd"
reader.view.release()

# The callback can shrink the temporary buffer, so slicing by the original
# requested size without checking the current buffer is not safe.
reader = Reader(2, False, b"xy")
assert reader.read(4) == b"xy"
assert reader.buffer == b""

reader = Reader(4, False, b"xy")
result = reader.read(4)
assert len(result) == 4
assert result[:2] == b"xy"
# The contents of the newly grown region are unspecified by CPython.
assert reader.buffer == b""
55 changes: 55 additions & 0 deletions extra_tests/snippets/builtin_bytearray_threading.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
"""Acquiring a bytearray export must be atomic with respect to resizing."""

import threading


def exercise_exports():
payload = b"x" * 65536
value = bytearray(payload)
barrier = threading.Barrier(3, timeout=5)
errors = []

def worker(operation):
try:
for _ in range(256):
barrier.wait()
operation()
barrier.wait()
except BaseException as exc:
errors.append(exc)
barrier.abort()

def resize():
try:
value.clear()
value.extend(payload)
except BufferError:
pass

def decode():
result = value.decode("ascii")
assert result in ("", "x" * len(payload))

def export():
with memoryview(value) as view:
size = len(view)
assert size in (0, len(payload))
# The acquired export keeps its original length and contents even
# if the resizing thread runs between these two reads.
assert view.tobytes() == payload[:size]
assert len(view) == size

threads = [
threading.Thread(target=worker, args=(operation,), daemon=True)
for operation in (resize, decode, export)
]
for thread in threads:
thread.start()
for thread in threads:
thread.join(timeout=10)
assert not any(thread.is_alive() for thread in threads), "worker did not finish"
if errors:
raise errors[0]


exercise_exports()
Loading