diff --git a/Lib/test/test_memoryview.py b/Lib/test/test_memoryview.py index 707540f29..a5150d25d 100644 --- a/Lib/test/test_memoryview.py +++ b/Lib/test/test_memoryview.py @@ -387,7 +387,6 @@ class AbstractMemoryTests: m = self._view(b) self.assertRaises(ValueError, hash, m) - @unittest.expectedFailure # TODO: RUSTPYTHON; re-entrant buffer release not detected def test_hash_use_after_free(self): # Prevent crash in memoryview(v).__hash__ with re-entrant v.__hash__. # Regression test for https://github.com/python/cpython/issues/142664. @@ -457,7 +456,6 @@ class AbstractMemoryTests: self.assertEqual(c.format, "H") self.assertEqual(d.format, "H") - @unittest.expectedFailure # TODO: RUSTPYTHON; re-entrant buffer release not detected def test_hex_use_after_free(self): # Prevent UAF in memoryview.hex(sep) with re-entrant sep.__len__. # Regression test for https://github.com/python/cpython/issues/143195. @@ -694,7 +692,6 @@ class OtherTest(unittest.TestCase): with self.assertRaises(TypeError): pickle.dumps(m, proto) - @unittest.expectedFailure # TODO: RUSTPYTHON; AssertionError: ValueError not raised def test_use_released_memory(self): # gh-92888: Previously it was possible to use a memoryview even after # backing buffer is freed in certain cases. This tests that those diff --git a/crates/stdlib/src/array.rs b/crates/stdlib/src/array.rs index b1fa925e1..7ecd8f4fd 100644 --- a/crates/stdlib/src/array.rs +++ b/crates/stdlib/src/array.rs @@ -670,7 +670,7 @@ pub mod array { #[pyattr] #[pyattr(name = "ArrayType")] - #[pyclass(name = "array")] + #[pyclass(name = "array", unhashable = true)] #[derive(Debug, PyPayload)] pub struct PyArray { array: PyRwLock, diff --git a/crates/vm/src/builtins/bytearray.rs b/crates/vm/src/builtins/bytearray.rs index 93046b493..c8782127b 100644 --- a/crates/vm/src/builtins/bytearray.rs +++ b/crates/vm/src/builtins/bytearray.rs @@ -320,8 +320,10 @@ impl PyByteArray { #[pymethod] fn hex(&self, options: ByteInnerHexOptions, vm: &VirtualMachine) -> PyResult { - let ByteInnerHexOptions { sep, bytes_per_sep } = options; - self.inner().hex(sep, bytes_per_sep, vm) + // Measuring the separator runs Python, so it happens before the buffer + // is borrowed. + let (sep, bytes_per_sep) = options.resolve(vm)?; + Ok(self.inner().hex(sep, bytes_per_sep)) } #[pyclassmethod] diff --git a/crates/vm/src/builtins/bytes.rs b/crates/vm/src/builtins/bytes.rs index 48c0e4312..bfa2fd354 100644 --- a/crates/vm/src/builtins/bytes.rs +++ b/crates/vm/src/builtins/bytes.rs @@ -327,8 +327,8 @@ impl PyBytes { options: ByteInnerHexOptions, vm: &VirtualMachine, ) -> PyResult { - let ByteInnerHexOptions { sep, bytes_per_sep } = options; - self.inner.hex(sep, bytes_per_sep, vm) + let (sep, bytes_per_sep) = options.resolve(vm)?; + Ok(self.inner.hex(sep, bytes_per_sep)) } #[pyclassmethod] diff --git a/crates/vm/src/builtins/memory.rs b/crates/vm/src/builtins/memory.rs index a4c29fe44..0b04de133 100644 --- a/crates/vm/src/builtins/memory.rs +++ b/crates/vm/src/builtins/memory.rs @@ -32,6 +32,9 @@ use crossbeam_utils::atomic::AtomicCell; use itertools::Itertools; use rustpython_common::lock::PyMutex; +/// The most dimensions a view can describe. PyBUF_MAX_NDIM +const MAX_NDIM: usize = 64; + #[derive(FromArgs)] pub struct PyMemoryViewNewArgs { object: PyObjectRef, @@ -59,9 +62,10 @@ pub struct PyMemoryView { // memoryview's options could be different from buffer's options desc: BufferDescriptor, hash: OnceCell, - // exports - // memoryview has no exports count by itself - // instead it relay on the buffer it viewing to maintain the count + /// Buffers handed out of this view that have not been given back yet. The + /// view cannot be released while any of them is outstanding, so that what + /// reads them keeps reading memory that is still there. self->exports + exports: AtomicCell, } impl Constructor for PyMemoryView { @@ -146,6 +150,7 @@ impl PyMemoryView { format_spec, desc, hash: OnceCell::new(), + exports: AtomicCell::new(0), }) } @@ -174,6 +179,7 @@ impl PyMemoryView { format_spec: self.format_spec.clone(), desc: self.desc.clone(), hash: OnceCell::new(), + exports: AtomicCell::new(0), } } @@ -189,6 +195,7 @@ impl PyMemoryView { format_spec: self.format_spec.clone(), desc: self.desc.clone(), hash: OnceCell::new(), + exports: AtomicCell::new(0), } } @@ -274,9 +281,10 @@ impl PyMemoryView { ); } let (shape, _, _) = self.desc.dim_desc[0]; + // ptr_from_index let index = i .wrapped_at(shape) - .ok_or_else(|| vm.new_index_error("index out of range"))?; + .ok_or_else(|| vm.new_index_error("index out of bounds on dimension 1"))?; self.unpack_single(self.desc.fast_position(&[index]) as usize, vm) } @@ -291,12 +299,7 @@ impl PyMemoryView { fn getitem_by_multi_idx(&self, indexes: &[isize], vm: &VirtualMachine) -> PyResult { let pos = self.pos_from_multi_index(indexes, vm)?; - let bytes = self.buffer.obj_bytes(); - format_unpack( - &self.format_spec, - &bytes[pos..pos + self.format_spec.size()], - vm, - ) + self.unpack_single(pos, vm) } fn setitem_by_idx(&self, i: isize, value: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> { @@ -304,9 +307,10 @@ impl PyMemoryView { return Err(vm.new_not_implemented_error("sub-views are not implemented")); } let (shape, _, _) = self.desc.dim_desc[0]; + // ptr_from_index let index = i .wrapped_at(shape) - .ok_or_else(|| vm.new_index_error("index out of range"))?; + .ok_or_else(|| vm.new_index_error("index out of bounds on dimension 1"))?; self.pack_single(self.desc.fast_position(&[index]) as usize, value, vm) } @@ -673,13 +677,37 @@ impl PyMemoryView { Self::from_object_with_flags(&args.object, flags, vm).map(|mv| mv.into_ref(&vm.ctx)) } - #[pymethod] + #[pymethod(name = "release")] + fn py_release(&self, vm: &VirtualMachine) -> PyResult<()> { + // _memory_release: what still reads this view holds it open. + let exports = self.exports.load(); + if !self.released.load() && exports > 0 { + let plural = if exports == 1 { "" } else { "s" }; + return Err( + vm.new_buffer_error(format!("memoryview has {exports} exported buffer{plural}")) + ); + } + self.release(); + Ok(()) + } + + /// Give up the view's share without asking whether anything is reading it. + /// The teardown paths have nowhere to report a refusal. pub fn release(&self) { if self.released.compare_exchange(false, true).is_ok() { self.buffer.release(); } } + /// Count this view as exported while `f` runs, so Python reached from inside + /// it cannot release the memory being read out from under it. + fn while_exported(&self, f: impl FnOnce() -> R) -> R { + self.exports.fetch_add(1); + let result = f(); + self.exports.fetch_sub(1); + result + } + #[pygetset] fn obj(&self, vm: &VirtualMachine) -> PyResult { self.try_not_released(vm)?; @@ -785,9 +813,10 @@ impl PyMemoryView { zelf.try_not_released(vm).map(|_| zelf) } + // memory_exit #[pymethod] - fn __exit__(&self, _args: FuncArgs) { - self.release(); + fn __exit__(&self, _args: FuncArgs, vm: &VirtualMachine) -> PyResult<()> { + self.py_release(vm) } fn __getitem__(zelf: PyRef, needle: PyObjectRef, vm: &VirtualMachine) -> PyResult { @@ -888,9 +917,12 @@ impl PyMemoryView { #[pymethod] fn hex(&self, options: ByteInnerHexOptions, vm: &VirtualMachine) -> PyResult { - let ByteInnerHexOptions { sep, bytes_per_sep } = options; self.try_not_released(vm)?; - self.contiguous_or_collect(|x| bytes_to_hex(x, sep, bytes_per_sep, vm)) + // Measuring the separator runs Python, which must not release the bytes + // being written out. memoryview_hex_impl + let (sep, bytes_per_sep) = self.while_exported(|| options.resolve(vm))?; + self.try_not_released(vm)?; + Ok(self.contiguous_or_collect(|x| bytes_to_hex(x, sep, bytes_per_sep))) } #[pymethod] @@ -984,6 +1016,7 @@ impl PyMemoryView { dim_desc: vec![(self.desc.len / itemsize, itemsize as isize, 0)], }, hash: OnceCell::new(), + exports: AtomicCell::new(0), }; Ok(zelf) } @@ -1020,7 +1053,11 @@ impl PyMemoryView { }; let shape_ndim = shape.len(); - // TODO: MAX_NDIM + if shape_ndim > MAX_NDIM { + return Err(vm.new_value_error(format!( + "memoryview: number of dimensions must not exceed {MAX_NDIM}" + ))); + } if self.desc.ndim() != 1 && shape_ndim != 1 { return Err(vm.new_type_error("memoryview: cast must be 1D -> ND or ND -> 1D")); } @@ -1158,34 +1195,36 @@ enum SubscriptNeedle { // MultiSlice(Vec), } +/// memory_subscript +/// +/// Which kind of key this is follows from the types in it alone, so an item that +/// answers `__index__` but raises on the way reports that rather than making the +/// whole key invalid. is_multiindex / is_multislice impl TryFromObject for SubscriptNeedle { fn try_from_object(vm: &VirtualMachine, obj: PyObjectRef) -> PyResult { - // TODO: number protocol - if let Some(i) = obj.downcast_ref::() { - Ok(Self::Index(i.try_to_primitive(vm)?)) - } else if obj.downcastable::() { - Ok(Self::Slice(unsafe { obj.downcast_unchecked::() })) - } else if let Ok(i) = obj.try_index(vm) { - Ok(Self::Index(i.try_to_primitive(vm)?)) - } else { - if let Some(tuple) = obj.downcast_ref::() { - if tuple.iter().all(|x| x.downcastable::()) { - let v = tuple - .iter() - .map(|x| { - unsafe { x.downcast_unchecked_ref::() } - .try_to_primitive::(vm) - }) - .try_collect()?; - return Ok(Self::MultiIndex(v)); - } else if tuple.iter().all(|x| x.downcastable::()) { - return Err(vm.new_not_implemented_error( - "multi-dimensional slicing is not implemented", - )); - } - } - Err(vm.new_type_error("memoryview: invalid slice key")) + if obj.number().is_index() { + return Ok(Self::Index(obj.try_index(vm)?.try_to_primitive(vm)?)); } + if obj.downcastable::() { + return Ok(Self::Slice(unsafe { obj.downcast_unchecked::() })); + } + if let Some(tuple) = obj.downcast_ref::() { + if tuple.iter().all(|x| x.number().is_index()) { + // ptr_from_tuple: each item is converted where it sits, and the + // conversion can run Python that releases the view. + let indices = tuple + .iter() + .map(|x| x.try_index(vm)?.try_to_primitive::(vm)) + .try_collect()?; + return Ok(Self::MultiIndex(indices)); + } + if tuple.iter().all(|x| x.downcastable::()) { + return Err( + vm.new_not_implemented_error("multi-dimensional slicing is not implemented") + ); + } + } + Err(vm.new_type_error("memoryview: invalid slice key")) } } @@ -1193,9 +1232,18 @@ static BUFFER_METHODS: BufferMethods = BufferMethods { obj_bytes: |buffer| buffer.obj_as::().buffer.obj_bytes(), obj_bytes_mut: |buffer| buffer.obj_as::().buffer.obj_bytes_mut(), // memory_releasebuf / memory_getbuf: a consumer's export of this view is a - // share of the acquisition the view is looking at. - release: |buffer| buffer.obj_as::().buffer.release_share(), - retain: |buffer| buffer.obj_as::().buffer.retain_share(), + // share of the acquisition the view is looking at, and one more reason the + // view itself cannot be released. + release: |buffer| { + let mv = buffer.obj_as::(); + mv.exports.fetch_sub(1); + mv.buffer.release_share(); + }, + retain: |buffer| { + let mv = buffer.obj_as::(); + mv.exports.fetch_add(1); + mv.buffer.retain_share(); + }, }; impl AsBuffer for PyMemoryView { @@ -1308,6 +1356,12 @@ impl Hashable for PyMemoryView { vm.new_value_error("memoryview: hashing is restricted to formats 'B', 'b' or 'c'") ); } + // A view is no more hashable than what it looks at, and asking that runs + // Python, which must not release the memory the hash is taken over. + // memory_hash + if !zelf.buffer.obj.downcastable::() { + zelf.while_exported(|| zelf.buffer.obj.hash(vm))?; + } let val = zelf.contiguous_or_collect(|bytes| vm.state.hash_secret.hash_bytes(bytes)); let _ = zelf.hash.set(val); Ok(*zelf.hash.get().unwrap()) diff --git a/crates/vm/src/bytes_inner.rs b/crates/vm/src/bytes_inner.rs index 65a9dc0a0..e16c636a9 100644 --- a/crates/vm/src/bytes_inner.rs +++ b/crates/vm/src/bytes_inner.rs @@ -497,13 +497,8 @@ impl PyBytesInner { swapcase_ascii(self.as_bytes()) } - pub fn hex( - &self, - sep: OptionalArg>, - bytes_per_sep: OptionalArg, - vm: &VirtualMachine, - ) -> PyResult { - bytes_to_hex(self.elements.as_slice(), sep, bytes_per_sep, vm) + pub fn hex(&self, sep: Option, bytes_per_sep: OptionalArg) -> String { + bytes_to_hex(self.elements.as_slice(), sep, bytes_per_sep) } pub fn fromhex(bytes: &[u8], vm: &VirtualMachine) -> PyResult> { @@ -1211,6 +1206,42 @@ pub(crate) struct ByteInnerHexOptions { pub bytes_per_sep: OptionalArg, } +impl ByteInnerHexOptions { + /// The separator byte, and how many bytes go between two of them. + /// + /// Measuring the separator runs Python, so it happens here, before the + /// bytes to be written out are borrowed. _Py_strhex_impl + pub(crate) fn resolve(self, vm: &VirtualMachine) -> PyResult<(Option, OptionalArg)> { + let Self { sep, bytes_per_sep } = self; + let OptionalArg::Present(sep) = sep else { + return Ok((None, bytes_per_sep)); + }; + + let s_guard; + let b_guard; + let (obj, bytes) = match &sep { + Either::A(s) => { + s_guard = s.as_wtf8(); + (s.as_object(), s_guard.as_bytes()) + } + Either::B(b) => { + b_guard = b.as_bytes(); + (b.as_object(), b_guard) + } + }; + if obj.length(vm)? != 1 { + return Err(vm.new_value_error("sep must be length 1.")); + } + // An object that claims a length it does not have separates with NUL, + // which is what reading past the end of its data gives. + let sep = bytes.first().copied().unwrap_or(0); + if sep > 127 { + return Err(vm.new_value_error("sep must be ASCII.")); + } + Ok((Some(sep), bytes_per_sep)) + } +} + fn hex_impl_no_sep(bytes: &[u8]) -> String { let mut buf: Vec = vec![0; bytes.len() * 2]; hex::encode_to_slice(bytes, buf.as_mut_slice()).unwrap(); @@ -1265,44 +1296,13 @@ fn hex_impl(bytes: &[u8], sep: u8, bytes_per_sep: isize) -> String { pub(crate) fn bytes_to_hex( bytes: &[u8], - sep: OptionalArg>, + sep: Option, bytes_per_sep: OptionalArg, - vm: &VirtualMachine, -) -> PyResult { - if bytes.is_empty() { - return Ok("".to_owned()); - } - - if let OptionalArg::Present(sep) = sep { - let bytes_per_sep = bytes_per_sep.unwrap_or(1); - if bytes_per_sep == 0 { - return Ok(hex_impl_no_sep(bytes)); - } - - let s_guard; - let b_guard; - let sep = match &sep { - Either::A(s) => { - s_guard = s.as_wtf8(); - s_guard.as_bytes() - } - Either::B(bytes) => { - b_guard = bytes.as_bytes(); - b_guard - } - }; - - if sep.len() != 1 { - return Err(vm.new_value_error("sep must be length 1.")); - } - let sep = sep[0]; - if sep > 127 { - return Err(vm.new_value_error("sep must be ASCII.")); - } - - Ok(hex_impl(bytes, sep, bytes_per_sep)) - } else { - Ok(hex_impl_no_sep(bytes)) +) -> String { + let bytes_per_sep = bytes_per_sep.unwrap_or(1); + match sep { + Some(sep) if bytes_per_sep != 0 && !bytes.is_empty() => hex_impl(bytes, sep, bytes_per_sep), + _ => hex_impl_no_sep(bytes), } } diff --git a/crates/vm/src/protocol/buffer.rs b/crates/vm/src/protocol/buffer.rs index 050c568b7..9a184ee85 100644 --- a/crates/vm/src/protocol/buffer.rs +++ b/crates/vm/src/protocol/buffer.rs @@ -608,13 +608,15 @@ impl BufferDescriptor { /// panic if indices.len() != ndim pub fn position(&self, indices: &[isize], vm: &VirtualMachine) -> PyResult { let mut pos = self.offset; - for (i, (shape, stride, suboffset)) in indices + for (dim, (i, (shape, stride, suboffset))) in indices .iter() .copied() .zip_eq(self.dim_desc.iter().copied()) + .enumerate() { + // The dimension is named the way a person counts it. lookup_dimension let i = i.wrapped_at(shape).ok_or_else(|| { - vm.new_index_error(format!("index out of bounds on dimension {i}")) + vm.new_index_error(format!("index out of bounds on dimension {}", dim + 1)) })?; pos += i as isize * stride + suboffset; } diff --git a/extra_tests/snippets/builtin_memoryview.py b/extra_tests/snippets/builtin_memoryview.py index 34928041c..979f584a2 100644 --- a/extra_tests/snippets/builtin_memoryview.py +++ b/extra_tests/snippets/builtin_memoryview.py @@ -675,3 +675,178 @@ test_cast_between_non_byte_formats() test_cast_to_zero_dim() test_hash_restricted_to_byte_formats() test_tobytes_order() + + +def test_index_key_goes_through_index(): + class I: + def __index__(self): + return 1 + + c = memoryview(bytes(range(24))).cast("B", shape=(4, 6)) + assert c[I(), 2] == 8 + assert c[1, I()] == 7 + assert memoryview(b"\x00\x01")[(I(),)] == 1 + + w = memoryview(bytearray(range(24))).cast("B", shape=(4, 6)) + w[I(), 2] = 99 + assert w[1, 2] == 99 + + # What the key is follows from its types, so an item that answers + # __index__ but raises reports that rather than the key being invalid. + class Boom: + def __index__(self): + raise RuntimeError("boom") + + for key in [Boom(), (Boom(), 0), (0, Boom())]: + try: + c[key] + raise AssertionError("no error") + except RuntimeError as e: + assert str(e) == "boom", e + + try: + c[object(), 0] + raise AssertionError("no error") + except TypeError as e: + assert "invalid slice key" in str(e), e + + +test_index_key_goes_through_index() + + +def test_index_error_names_the_dimension(): + c = memoryview(bytearray(range(24))).cast("B", shape=(2, 3, 4)) + for key, dimension in [((0, 3, 0), 2), ((0, 0, -5), 3), ((2, 0, 0), 1)]: + try: + c[key] + raise AssertionError("no error") + except IndexError as e: + assert str(e) == f"index out of bounds on dimension {dimension}", e + + try: + memoryview(bytearray(range(4)))[100] + raise AssertionError("no error") + except IndexError as e: + assert str(e) == "index out of bounds on dimension 1", e + + +test_index_error_names_the_dimension() + + +def test_release_refuses_while_exported(): + ba = bytearray(b"abc") + mv = memoryview(ba) + exported = mv.__buffer__(0) + for release in [lambda: mv.release(), lambda: mv.__exit__()]: + try: + release() + raise AssertionError("released while exported") + except BufferError as e: + assert str(e) == "memoryview has 1 exported buffer", e + del exported + mv.release() + mv.release() + + +test_release_refuses_while_exported() + + +def test_hash_asks_the_exporter(): + # A view is no more hashable than what it looks at. + assert hash(memoryview(b"abc")) == hash(b"abc") + try: + hash(memoryview(bytearray(b"abc")).toreadonly()) + raise AssertionError("hashed a view on an unhashable exporter") + except TypeError as e: + assert "unhashable type: 'bytearray'" in str(e), e + + # Releasing the view from inside that hash is refused rather than obeyed. + class E(bytes): + def __hash__(self): + mv.release() + return 123 + + mv = memoryview(E(b"abcd")) + try: + hash(mv) + raise AssertionError("released the view being hashed") + except BufferError as e: + assert str(e) == "memoryview has 1 exported buffer", e + + +test_hash_asks_the_exporter() + + +def test_hex_measures_the_separator(): + # The separator is measured the way len() measures it, and read where it lies. + class One(bytes): + def __len__(self): + return 1 + + class Two(bytes): + def __len__(self): + return 2 + + for target in [b"abcd", bytearray(b"abcd"), memoryview(b"abcd")]: + assert target.hex(One(b"::")) == "61:62:63:64" + assert target.hex(b":") == "61:62:63:64" + # An object claiming a length it does not have separates with NUL. + assert target.hex(One(b"")) == "61\x0062\x0063\x0064" + for bad in [Two(b":"), b"::"]: + try: + target.hex(bad) + raise AssertionError("no error") + except ValueError as e: + assert str(e) == "sep must be length 1.", e + # The separator is checked before anything is written out. + try: + target.hex(b"::", 0) + raise AssertionError("no error") + except ValueError as e: + assert str(e) == "sep must be length 1.", e + + assert b"".hex(b":") == "" + try: + b"".hex(b"::") + raise AssertionError("no error") + except ValueError as e: + assert str(e) == "sep must be length 1.", e + + # Releasing the view from inside that measurement is refused. + ba = bytearray(b"A" * 8) + mv = memoryview(ba) + + class S(bytes): + def __len__(self): + mv.release() + return 1 + + try: + mv.hex(S(b":")) + raise AssertionError("released the view being written out") + except BufferError as e: + assert str(e) == "memoryview has 1 exported buffer", e + + +test_hex_measures_the_separator() + + +def test_cast_bounds_the_dimensions(): + mv = memoryview(bytearray(range(8))) + assert mv.cast("B", (1,) * 63 + (8,)).ndim == 64 + for shape in [(1,) * 64 + (8,), (1,) * 99 + (8,), [1] * 64 + [8]]: + try: + mv.cast("B", shape) + raise AssertionError("cast past the limit") + except ValueError as e: + assert str(e) == "memoryview: number of dimensions must not exceed 64", e + + # The limit is answered before the shape is looked at any further. + try: + mv.cast("B", (2, 4)).cast("B", (1,) * 64 + (8,)) + raise AssertionError("cast past the limit") + except ValueError as e: + assert str(e) == "memoryview: number of dimensions must not exceed 64", e + + +test_cast_bounds_the_dimensions()