Merge pull request #3280 from DimitrisJim/bad_indexing

Fix deadlocks when slicing mutable sequences.

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:

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()

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<Self>, 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)
+                }
             }
         }
 

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)
+            }
         }
     }
 

vm/src/builtins/list.rs

@@ -213,8 +213,9 @@ impl PyList {
     fn setslice(&self, slice: PySliceRef, sec: ArgIterable, vm: &VirtualMachine) -> PyResult<()> {
         let items: Result<Vec<PyObjectRef>, _> = 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]

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<Self>, 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);

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<Option<BigInt>> {
-        if let Some(obj) = &self.start {
-            to_index_value(vm, obj)
-        } else {
-            Ok(None)
-        }
-    }
-
-    pub fn stop_index(&self, vm: &VirtualMachine) -> PyResult<Option<BigInt>> {
-        to_index_value(vm, &self.stop)
-    }
-
-    pub fn step_index(&self, vm: &VirtualMachine) -> PyResult<Option<BigInt>> {
-        if let Some(obj) = &self.step {
-            to_index_value(vm, obj)
-        } else {
-            Ok(None)
-        }
+    pub fn to_saturated(&self, vm: &VirtualMachine) -> PyResult<SaturatedSlice> {
+        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<Option<BigInt>> {
+/// 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<Self> {
+        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<usize>, Option<usize>, 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<Option<isize>> {
     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")]

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<BigInt>, stop: &Option<BigInt>) -> Range<usize> {
-        saturate_range(start, stop, self.len())
-    }
-
-    fn get_slice_items(&self, vm: &VirtualMachine, slice: &PySlice) -> PyResult<Self::Sliced> {
-        let (range, step, is_negative_step) = convert_slice(slice, self.len(), vm)?;
+    fn get_slice_items(
+        &self,
+        _vm: &VirtualMachine,
+        slice: SaturatedSlice,
+    ) -> PyResult<Self::Sliced> {
+        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<usize> {
         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<BigInt>,
-    stop: &Option<BigInt>,
-    len: usize,
-) -> Range<usize> {
-    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<usize>, Option<usize>, 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))
-}