diff --git a/Lib/test/test_xml_etree.py b/Lib/test/test_xml_etree.py index d788155a2..f22b7711a 100644 --- a/Lib/test/test_xml_etree.py +++ b/Lib/test/test_xml_etree.py @@ -2515,7 +2515,6 @@ class BadElementTest(ElementTestCase, unittest.TestCase): e.append(ET.Element('child')) e[0:10:X()] # shouldn't crash - @unittest.skip("TODO: RUSTPYTHON, hangs") def test_ass_subscr(self): # Issue #27863 class X: diff --git a/extra_tests/snippets/bad_indexing.py b/extra_tests/snippets/bad_indexing.py new file mode 100644 index 000000000..af71f2e68 --- /dev/null +++ b/extra_tests/snippets/bad_indexing.py @@ -0,0 +1,32 @@ +""" Test that indexing ops don't hang when an object with a mutating +__index__ is used.""" +from testutils import assert_raises +from array import array + + +class BadIndex: + def __index__(self): + # assign ourselves, makes it easy to re-use with + # all mutable collections. + e[:] = e + return 1 + + +def run_setslice(): + with assert_raises(IndexError): + e[BadIndex()] = 42 + e[BadIndex():0:-1] = e + e[0:BadIndex():1] = e + e[0:10:BadIndex()] = e + + +def run_delslice(): + del e[BadIndex():0:-1] + del e[0:BadIndex():1] + del e[0:10:BadIndex()] + +# Check types +instances = [list(), bytearray(), array('b')] +for e in instances: + run_setslice() + run_delslice() \ No newline at end of file diff --git a/stdlib/src/array.rs b/stdlib/src/array.rs index 8c7a7e07f..597471445 100644 --- a/stdlib/src/array.rs +++ b/stdlib/src/array.rs @@ -13,7 +13,7 @@ mod array { use crate::vm::{ builtins::{ PyByteArray, PyBytes, PyBytesRef, PyDictRef, PyFloat, PyInt, PyIntRef, PyList, - PyListRef, PySliceRef, PyStr, PyStrRef, PyTupleRef, PyTypeRef, + PyListRef, PyStr, PyStrRef, PyTupleRef, PyTypeRef, }, class_or_notimplemented, function::{ @@ -23,7 +23,7 @@ mod array { BufferInternal, BufferOptions, BufferResizeGuard, PyBuffer, PyIterReturn, PyMappingMethods, }, - sliceable::{saturate_index, PySliceableSequence, PySliceableSequenceMut, SequenceIndex}, + sliceable::{PySliceableSequence, PySliceableSequenceMut, SaturatedSlice, SequenceIndex}, slots::{ AsBuffer, AsMapping, Comparable, Iterable, IteratorIterable, PyComparisonOp, SlotConstructor, SlotIterator, @@ -122,14 +122,14 @@ mod array { fn insert( &mut self, - i: usize, + i: isize, obj: PyObjectRef, vm: &VirtualMachine ) -> PyResult<()> { match self { $(ArrayContentType::$n(v) => { let val = <$t>::try_into_from_object(vm, obj)?; - v.insert(i, val); + v.insert(v.saturate_index(i), val); })* } Ok(()) @@ -276,7 +276,10 @@ mod array { v.get(pos_index).unwrap().into_pyresult(vm) } SequenceIndex::Slice(slice) => { - let elements = v.get_slice_items(vm, &slice)?; + // TODO: Use interface similar to set/del item. This can + // still hang. + let slice = slice.to_saturated(vm)?; + let elements = v.get_slice_items(vm, slice)?; let array: PyArray = ArrayContentType::$n(elements).into(); Ok(array.into_object(vm)) } @@ -287,13 +290,13 @@ mod array { fn setitem_by_slice( &mut self, - slice: PySliceRef, + slice: SaturatedSlice, items: &ArrayContentType, vm: &VirtualMachine ) -> PyResult<()> { match self { $(Self::$n(elements) => if let ArrayContentType::$n(items) = items { - elements.set_slice_items(vm, &slice, items) + elements.set_slice_items(vm, slice, items) } else { Err(vm.new_type_error( "bad argument type for built-in operation".to_owned() @@ -304,13 +307,13 @@ mod array { fn setitem_by_slice_no_resize( &mut self, - slice: PySliceRef, + slice: SaturatedSlice, items: &ArrayContentType, vm: &VirtualMachine ) -> PyResult<()> { match self { $(Self::$n(elements) => if let ArrayContentType::$n(items) = items { - elements.set_slice_items_no_resize(vm, &slice, items) + elements.set_slice_items_no_resize(vm, slice, items) } else { Err(vm.new_type_error( "bad argument type for built-in operation".to_owned() @@ -350,12 +353,12 @@ mod array { fn delitem_by_slice( &mut self, - slice: PySliceRef, + slice: SaturatedSlice, vm: &VirtualMachine ) -> PyResult<()> { match self { $(ArrayContentType::$n(elements) => { - elements.delete_slice(vm, &slice) + elements.delete_slice(vm, slice) })* } } @@ -895,7 +898,6 @@ mod array { vm: &VirtualMachine, ) -> PyResult<()> { let mut w = zelf.try_resizable(vm)?; - let i = saturate_index(i, w.len()); w.insert(i, x, vm) } @@ -983,6 +985,7 @@ mod array { match SequenceIndex::try_from_object_for(vm, needle, "array")? { SequenceIndex::Int(i) => zelf.write().setitem_by_idx(i, obj, vm), SequenceIndex::Slice(slice) => { + let slice = slice.to_saturated(vm)?; let cloned; let guard; let items = if zelf.is(&obj) { @@ -1015,7 +1018,10 @@ mod array { fn delitem(zelf: PyRef, needle: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> { match SequenceIndex::try_from_object_for(vm, needle, "array")? { SequenceIndex::Int(i) => zelf.try_resizable(vm)?.delitem_by_idx(i, vm), - SequenceIndex::Slice(slice) => zelf.try_resizable(vm)?.delitem_by_slice(slice, vm), + SequenceIndex::Slice(slice) => { + let slice = slice.to_saturated(vm)?; + zelf.try_resizable(vm)?.delitem_by_slice(slice, vm) + } } } diff --git a/vm/src/builtins/bytearray.rs b/vm/src/builtins/bytearray.rs index 4b0fa0054..28a64ccb6 100644 --- a/vm/src/builtins/bytearray.rs +++ b/vm/src/builtins/bytearray.rs @@ -182,16 +182,17 @@ impl PyByteArray { } } SequenceIndex::Slice(slice) => { + let slice = slice.to_saturated(vm)?; let items = if zelf.is(&value) { zelf.borrow_buf().to_vec() } else { bytes_from_object(vm, &value)? }; if let Ok(mut w) = zelf.try_resizable(vm) { - w.elements.set_slice_items(vm, &slice, items.as_slice()) + w.elements.set_slice_items(vm, slice, items.as_slice()) } else { zelf.borrow_buf_mut() - .set_slice_items_no_resize(vm, &slice, items.as_slice()) + .set_slice_items_no_resize(vm, slice, items.as_slice()) } } } @@ -212,9 +213,9 @@ impl PyByteArray { #[pymethod(magic)] pub fn delitem(&self, needle: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> { - let elements = &mut self.try_resizable(vm)?.elements; match SequenceIndex::try_from_object_for(vm, needle, Self::NAME)? { SequenceIndex::Int(int) => { + let elements = &mut self.try_resizable(vm)?.elements; if let Some(idx) = elements.wrap_index(int) { elements.remove(idx); Ok(()) @@ -222,7 +223,11 @@ impl PyByteArray { Err(vm.new_index_error("index out of range".to_owned())) } } - SequenceIndex::Slice(slice) => elements.delete_slice(vm, &slice), + SequenceIndex::Slice(slice) => { + let slice = slice.to_saturated(vm)?; + let elements = &mut self.try_resizable(vm)?.elements; + elements.delete_slice(vm, slice) + } } } diff --git a/vm/src/builtins/list.rs b/vm/src/builtins/list.rs index 2c2d8e72f..53313373d 100644 --- a/vm/src/builtins/list.rs +++ b/vm/src/builtins/list.rs @@ -213,8 +213,9 @@ impl PyList { fn setslice(&self, slice: PySliceRef, sec: ArgIterable, vm: &VirtualMachine) -> PyResult<()> { let items: Result, _> = sec.iter(vm)?.collect(); let items = items?; + let slice = slice.to_saturated(vm)?; let mut elements = self.borrow_vec_mut(); - elements.set_slice_items(vm, &slice, items.as_slice()) + elements.set_slice_items(vm, slice, items.as_slice()) } #[pymethod(magic)] @@ -373,7 +374,8 @@ impl PyList { } fn delslice(&self, slice: PySliceRef, vm: &VirtualMachine) -> PyResult<()> { - self.borrow_vec_mut().delete_slice(vm, &slice) + let slice = slice.to_saturated(vm)?; + self.borrow_vec_mut().delete_slice(vm, slice) } #[pymethod] diff --git a/vm/src/builtins/memory.rs b/vm/src/builtins/memory.rs index d8c9ec842..efedc5778 100644 --- a/vm/src/builtins/memory.rs +++ b/vm/src/builtins/memory.rs @@ -9,7 +9,7 @@ use crate::{ bytesinner::bytes_to_hex, function::{FuncArgs, IntoPyObject, OptionalArg}, protocol::{BufferInternal, BufferOptions, PyBuffer, PyMappingMethods}, - sliceable::{convert_slice, wrap_index, SequenceIndex}, + sliceable::{wrap_index, SaturatedSlice, SequenceIndex}, slots::{AsBuffer, AsMapping, Comparable, Hashable, PyComparisonOp, SlotConstructor}, stdlib::pystruct::FormatSpec, utils::Either, @@ -253,7 +253,8 @@ impl PyMemoryView { fn getitem_by_slice(zelf: PyRef, slice: PySliceRef, vm: &VirtualMachine) -> PyResult { // slicing a memoryview return a new memoryview let len = zelf.buffer.options.len; - let (range, step, is_negative_step) = convert_slice(&slice, len, vm)?; + let (range, step, is_negative_step) = + SaturatedSlice::with_slice(&slice, vm)?.adjust_indices(len); let abs_step = step.unwrap(); let step = if is_negative_step { -(abs_step as isize) @@ -393,7 +394,8 @@ impl PyMemoryView { return diff_err(); } - let (range, step, is_negative_step) = convert_slice(&slice, zelf.buffer.options.len, vm)?; + let (range, step, is_negative_step) = + SaturatedSlice::with_slice(&slice, vm)?.adjust_indices(zelf.buffer.options.len); let bytes = items.to_contiguous(); assert_eq!(bytes.len(), len * itemsize); diff --git a/vm/src/builtins/slice.rs b/vm/src/builtins/slice.rs index 5ceb5b427..5d8190adb 100644 --- a/vm/src/builtins/slice.rs +++ b/vm/src/builtins/slice.rs @@ -1,5 +1,4 @@ // sliceobject.{h,c} in CPython - use super::{PyInt, PyIntRef, PyTupleRef, PyTypeRef}; use crate::{ function::{FuncArgs, IntoPyObject, OptionalArg}, @@ -8,7 +7,8 @@ use crate::{ TypeProtocol, VirtualMachine, }; use num_bigint::{BigInt, ToBigInt}; -use num_traits::{One, Signed, Zero}; +use num_traits::{One, Signed, ToPrimitive, Zero}; +use std::ops::Range; #[pyclass(module = false, name = "slice")] #[derive(Debug)] @@ -71,24 +71,8 @@ impl PySlice { )) } - pub fn start_index(&self, vm: &VirtualMachine) -> PyResult> { - if let Some(obj) = &self.start { - to_index_value(vm, obj) - } else { - Ok(None) - } - } - - pub fn stop_index(&self, vm: &VirtualMachine) -> PyResult> { - to_index_value(vm, &self.stop) - } - - pub fn step_index(&self, vm: &VirtualMachine) -> PyResult> { - if let Some(obj) = &self.step { - to_index_value(vm, obj) - } else { - Ok(None) - } + pub fn to_saturated(&self, vm: &VirtualMachine) -> PyResult { + SaturatedSlice::with_slice(self, vm) } #[pyslot] @@ -263,17 +247,129 @@ impl Comparable for PySlice { impl Unhashable for PySlice {} -fn to_index_value(vm: &VirtualMachine, obj: &PyObjectRef) -> PyResult> { +/// A saturated slice with values ranging in [isize::MIN, isize::MAX]. Used for +/// slicable sequences that require indices in the aforementioned range. +/// +/// Invokes `__index__` on the PySliceRef during construction so as to separate the +/// transformation from PyObject into isize and the adjusting of the slice to a given +/// sequence length. The reason this is important is due to the fact that an objects +/// `__index__` might get a lock on the sequence and cause a deadlock. +#[derive(Copy, Clone, Debug)] +pub struct SaturatedSlice { + start: isize, + stop: isize, + step: isize, +} + +impl SaturatedSlice { + // Equivalent to PySlice_Unpack. + pub fn with_slice(slice: &PySlice, vm: &VirtualMachine) -> PyResult { + let step = to_isize_index(vm, slice.step_ref(vm))?.unwrap_or(1); + if step == 0 { + return Err(vm.new_value_error("slice step cannot be zero".to_owned())); + } + let start = to_isize_index(vm, slice.start_ref(vm))?.unwrap_or_else(|| { + if step.is_negative() { + isize::MAX + } else { + 0 + } + }); + + let stop = to_isize_index(vm, &slice.stop(vm))?.unwrap_or_else(|| { + if step.is_negative() { + isize::MIN + } else { + isize::MAX + } + }); + Ok(Self { start, stop, step }) + } + + // Equivalent to PySlice_AdjustIndices + /// Convert for usage in indexing the underlying rust collections. Called *after* + /// __index__ has been called on the Slice which might mutate the collection. + pub fn adjust_indices(&self, len: usize) -> (Range, Option, bool) { + // len should always be <= isize::MAX + let ilen = len.to_isize().unwrap_or(isize::MAX); + let (start, stop, step) = (self.start, self.stop, self.step); + let (start, stop, step, is_negative_step) = if step.is_negative() { + ( + if stop == -1 { + ilen.saturating_add(1) + } else { + stop.saturating_add(1) + }, + if start == -1 { + ilen + } else { + start.saturating_add(1) + }, + step.saturating_abs(), + true, + ) + } else { + (start, stop, step, false) + }; + + let step = step.to_usize(); + + let range = saturate_index(start, len)..saturate_index(stop, len); + let range = if range.start >= range.end { + range.start..range.start + } else { + // step overflow + if step.is_none() { + if is_negative_step { + (range.end - 1)..range.end + } else { + range.start..(range.start + 1) + } + } else { + range + } + }; + (range, step, is_negative_step) + } +} + +// Go from PyObjectRef to isize w/o overflow error, out of range values are substituted by +// isize::MIN or isize::MAX depending on type and value of step. +// Equivalent to PyNumber_AsSsize_t with err equal to None. +fn to_isize_index(vm: &VirtualMachine, obj: &PyObjectRef) -> PyResult> { if vm.is_none(obj) { return Ok(None); } - let result = vm.to_index_opt(obj.clone()).unwrap_or_else(|| { Err(vm.new_type_error( "slice indices must be integers or None or have an __index__ method".to_owned(), )) })?; - Ok(Some(result.as_bigint().clone())) + let value = result.as_bigint(); + let is_negative = value.is_negative(); + Ok(Some(value.to_isize().unwrap_or_else(|| { + if is_negative { + isize::MIN + } else { + isize::MAX + } + }))) +} + +// Saturate p in range [0, len] inclusive +pub fn saturate_index(p: isize, len: usize) -> usize { + let len = len.to_isize().unwrap_or(isize::MAX); + let mut p = p; + if p < 0 { + p += len; + if p < 0 { + p = 0; + } + } + if p > len { + p = len; + } + p as usize } #[pyclass(module = false, name = "EllipsisType")] diff --git a/vm/src/sliceable.rs b/vm/src/sliceable.rs index b17f49b93..c0e44425d 100644 --- a/vm/src/sliceable.rs +++ b/vm/src/sliceable.rs @@ -1,8 +1,9 @@ -use num_bigint::BigInt; -use num_traits::{One, Signed, ToPrimitive, Zero}; +use num_traits::ToPrimitive; use std::ops::Range; use crate::builtins::int::PyInt; +// export through slicable module, not slice. +pub use crate::builtins::slice::{saturate_index, SaturatedSlice}; use crate::builtins::slice::{PySlice, PySliceRef}; use crate::utils::Either; use crate::VirtualMachine; @@ -24,10 +25,10 @@ pub trait PySliceableSequenceMut { fn set_slice_items_no_resize( &mut self, vm: &VirtualMachine, - slice: &PySlice, + slice: SaturatedSlice, items: &[Self::Item], ) -> PyResult<()> { - let (range, step, is_negative_step) = convert_slice(slice, self.as_slice().len(), vm)?; + let (range, step, is_negative_step) = slice.adjust_indices(self.as_slice().len()); if !is_negative_step && step == Some(1) { return if range.end - range.start == items.len() { self.do_set_range(range, items); @@ -82,10 +83,10 @@ pub trait PySliceableSequenceMut { fn set_slice_items( &mut self, vm: &VirtualMachine, - slice: &PySlice, + slice: SaturatedSlice, items: &[Self::Item], ) -> PyResult<()> { - let (range, step, is_negative_step) = convert_slice(slice, self.as_slice().len(), vm)?; + let (range, step, is_negative_step) = slice.adjust_indices(self.as_slice().len()); if !is_negative_step && step == Some(1) { self.do_set_range(range, items); return Ok(()); @@ -135,8 +136,8 @@ pub trait PySliceableSequenceMut { } } - fn delete_slice(&mut self, vm: &VirtualMachine, slice: &PySlice) -> PyResult<()> { - let (range, step, is_negative_step) = convert_slice(slice, self.as_slice().len(), vm)?; + fn delete_slice(&mut self, _vm: &VirtualMachine, slice: SaturatedSlice) -> PyResult<()> { + let (range, step, is_negative_step) = slice.adjust_indices(self.as_slice().len()); if range.start >= range.end { return Ok(()); } @@ -231,16 +232,12 @@ pub trait PySliceableSequence { saturate_index(p, self.len()) } - fn saturate_big_index(&self, slice_pos: &BigInt) -> usize { - saturate_big_index(slice_pos, self.len()) - } - - fn saturate_range(&self, start: &Option, stop: &Option) -> Range { - saturate_range(start, stop, self.len()) - } - - fn get_slice_items(&self, vm: &VirtualMachine, slice: &PySlice) -> PyResult { - let (range, step, is_negative_step) = convert_slice(slice, self.len(), vm)?; + fn get_slice_items( + &self, + _vm: &VirtualMachine, + slice: SaturatedSlice, + ) -> PyResult { + let (range, step, is_negative_step) = slice.adjust_indices(self.len()); if range.start >= range.end { return Ok(Self::empty()); } @@ -278,7 +275,10 @@ pub trait PySliceableSequence { })?; Ok(Either::A(self.do_get(pos_index))) } - SequenceIndex::Slice(slice) => Ok(Either::B(self.get_slice_items(vm, &slice)?)), + SequenceIndex::Slice(slice) => { + let slice = slice.to_saturated(vm)?; + Ok(Either::B(self.get_slice_items(vm, slice)?)) + } } } } @@ -396,99 +396,3 @@ pub(crate) fn wrap_index(p: isize, len: usize) -> Option { Some(p) } } - -// return pos is in range [0, len] inclusive -pub fn saturate_index(p: isize, len: usize) -> usize { - let mut p = p; - let len = len.to_isize().unwrap(); - if p < 0 { - p += len; - if p < 0 { - p = 0; - } - } - if p > len { - p = len; - } - p as usize -} - -fn saturate_big_index(slice_pos: &BigInt, len: usize) -> usize { - if let Some(pos) = slice_pos.to_isize() { - saturate_index(pos, len) - } else if slice_pos.is_negative() { - // slice past start bound, round to start - 0 - } else { - // slice past end bound, round to end - len - } -} - -pub(crate) fn saturate_range( - start: &Option, - stop: &Option, - len: usize, -) -> Range { - let start = start.as_ref().map_or(0, |x| saturate_big_index(x, len)); - let stop = stop.as_ref().map_or(len, |x| saturate_big_index(x, len)); - - start..stop -} - -pub(crate) fn convert_slice( - slice: &PySlice, - len: usize, - vm: &VirtualMachine, -) -> PyResult<(Range, Option, bool)> { - let start = slice.start_index(vm)?; - let stop = slice.stop_index(vm)?; - let step = slice.step_index(vm)?.unwrap_or_else(BigInt::one); - - if step.is_zero() { - return Err(vm.new_value_error("slice step cannot be zero".to_owned())); - } - - let (start, stop, step, is_negative_step) = if step.is_negative() { - ( - stop.map(|x| { - if x == -BigInt::one() { - len + BigInt::one() - } else { - x + 1 - } - }), - start.map(|x| { - if x == -BigInt::one() { - BigInt::from(len) - } else { - x + 1 - } - }), - -step, - true, - ) - } else { - (start, stop, step, false) - }; - - let step = step.to_usize(); - - let range = saturate_range(&start, &stop, len); - let range = if range.start >= range.end { - range.start..range.start - } else { - // step overflow - if step.is_none() { - if is_negative_step { - (range.end - 1)..range.end - } else { - range.start..(range.start + 1) - } - } else { - range - } - }; - - Ok((range, step, is_negative_step)) -}