Skip to content

Commit 72ad712

Browse files
DimitrisJimDimitrisJim
committed
Saturate slice before getting a lock on the underlying collection.
1 parent fac6e31 commit 72ad712

File tree

8 files changed

+94
-42
lines changed

8 files changed

+94
-42
lines changed

‎Lib/test/test_xml_etree.py

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2515,7 +2515,6 @@ def __index__(self):
25152515
e.append(ET.Element('child'))
25162516
e[0:10:X()] # shouldn't crash
25172517

2518-
@unittest.skip("TODO: RUSTPYTHON, hangs")
25192518
def test_ass_subscr(self):
25202519
# Issue #27863
25212520
class X:

‎extra_tests/snippets/bad_indexing.py

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
""" Test that indexing ops don't hang when an object with a mutating
2+
__index__ is used."""
3+
from testutils import assert_raises
4+
from array import array
5+
6+
7+
class BadIndex:
8+
def __index__(self):
9+
# assign ourselves, makes it easy to re-use with
10+
# all mutable collections.
11+
e[:] = e
12+
return 1
13+
14+
15+
def run_setitem():
16+
with assert_raises(IndexError):
17+
e[BadIndex()] = 42
18+
e[BadIndex():0:-1] = e
19+
e[0:BadIndex():1] = e
20+
e[0:10:BadIndex()] = e
21+
22+
23+
def run_delitem():
24+
del e[BadIndex():0:-1]
25+
del e[0:BadIndex():1]
26+
del e[0:10:BadIndex()]
27+
28+
# Check types
29+
instances = [list(), bytearray(), array('b')]
30+
for e in instances:
31+
print("Trying for ", type(e).__name__)
32+
run_setitem()
33+
print("setitem ok")
34+
run_delitem()
35+
print("delitem ok")

‎stdlib/src/array.rs

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ mod array {
1313
use crate::vm::{
1414
builtins::{
1515
PyByteArray, PyBytes, PyBytesRef, PyDictRef, PyFloat, PyInt, PyIntRef, PyList,
16-
PyListRef, PySliceRef, PyStr, PyStrRef, PyTupleRef, PyTypeRef,
16+
PyListRef, PyStr, PyStrRef, PyTupleRef, PyTypeRef,
1717
},
1818
class_or_notimplemented,
1919
function::{
@@ -23,7 +23,7 @@ mod array {
2323
BufferInternal, BufferOptions, BufferResizeGuard, PyBuffer, PyIterReturn,
2424
PyMappingMethods,
2525
},
26-
sliceable::{PySliceableSequence, PySliceableSequenceMut, SequenceIndex},
26+
sliceable::{PySliceableSequence, PySliceableSequenceMut, SaturatedIndices, SequenceIndex},
2727
slots::{
2828
AsBuffer, AsMapping, Comparable, Iterable, IteratorIterable, PyComparisonOp,
2929
SlotConstructor, SlotIterator,
@@ -276,7 +276,10 @@ mod array {
276276
v.get(pos_index).unwrap().into_pyresult(vm)
277277
}
278278
SequenceIndex::Slice(slice) => {
279-
let elements = v.get_slice_items(vm, &slice)?;
279+
// TODO: Use interface similar to set/del item. This can
280+
// still hang.
281+
let slice = slice.to_saturated_indices(vm)?;
282+
let elements = v.get_slice_items(vm, slice)?;
280283
let array: PyArray = ArrayContentType::$n(elements).into();
281284
Ok(array.into_object(vm))
282285
}
@@ -287,13 +290,13 @@ mod array {
287290

288291
fn setitem_by_slice(
289292
&mut self,
290-
slice: PySliceRef,
293+
slice: SaturatedIndices,
291294
items: &ArrayContentType,
292295
vm: &VirtualMachine
293296
) -> PyResult<()> {
294297
match self {
295298
$(Self::$n(elements) => if let ArrayContentType::$n(items) = items {
296-
elements.set_slice_items(vm, &slice, items)
299+
elements.set_slice_items(vm, slice, items)
297300
} else {
298301
Err(vm.new_type_error(
299302
"bad argument type for built-in operation".to_owned()
@@ -304,13 +307,13 @@ mod array {
304307

305308
fn setitem_by_slice_no_resize(
306309
&mut self,
307-
slice: PySliceRef,
310+
slice: SaturatedIndices,
308311
items: &ArrayContentType,
309312
vm: &VirtualMachine
310313
) -> PyResult<()> {
311314
match self {
312315
$(Self::$n(elements) => if let ArrayContentType::$n(items) = items {
313-
elements.set_slice_items_no_resize(vm, &slice, items)
316+
elements.set_slice_items_no_resize(vm, slice, items)
314317
} else {
315318
Err(vm.new_type_error(
316319
"bad argument type for built-in operation".to_owned()
@@ -350,12 +353,12 @@ mod array {
350353

351354
fn delitem_by_slice(
352355
&mut self,
353-
slice: PySliceRef,
356+
slice: SaturatedIndices,
354357
vm: &VirtualMachine
355358
) -> PyResult<()> {
356359
match self {
357360
$(ArrayContentType::$n(elements) => {
358-
elements.delete_slice(vm, &slice)
361+
elements.delete_slice(vm, slice)
359362
})*
360363
}
361364
}
@@ -982,6 +985,7 @@ mod array {
982985
match SequenceIndex::try_from_object_for(vm, needle, "array")? {
983986
SequenceIndex::Int(i) => zelf.write().setitem_by_idx(i, obj, vm),
984987
SequenceIndex::Slice(slice) => {
988+
let slice = slice.to_saturated_indices(vm)?;
985989
let cloned;
986990
let guard;
987991
let items = if zelf.is(&obj) {
@@ -1014,7 +1018,10 @@ mod array {
10141018
fn delitem(zelf: PyRef<Self>, needle: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> {
10151019
match SequenceIndex::try_from_object_for(vm, needle, "array")? {
10161020
SequenceIndex::Int(i) => zelf.try_resizable(vm)?.delitem_by_idx(i, vm),
1017-
SequenceIndex::Slice(slice) => zelf.try_resizable(vm)?.delitem_by_slice(slice, vm),
1021+
SequenceIndex::Slice(slice) => {
1022+
let slice = slice.to_saturated_indices(vm)?;
1023+
zelf.try_resizable(vm)?.delitem_by_slice(slice, vm)
1024+
}
10181025
}
10191026
}
10201027

‎vm/src/builtins/bytearray.rs

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -182,16 +182,17 @@ impl PyByteArray {
182182
}
183183
}
184184
SequenceIndex::Slice(slice) => {
185+
let slice = slice.to_saturated_indices(vm)?;
185186
let items = if zelf.is(&value) {
186187
zelf.borrow_buf().to_vec()
187188
} else {
188189
bytes_from_object(vm, &value)?
189190
};
190191
if let Ok(mut w) = zelf.try_resizable(vm) {
191-
w.elements.set_slice_items(vm, &slice, items.as_slice())
192+
w.elements.set_slice_items(vm, slice, items.as_slice())
192193
} else {
193194
zelf.borrow_buf_mut()
194-
.set_slice_items_no_resize(vm, &slice, items.as_slice())
195+
.set_slice_items_no_resize(vm, slice, items.as_slice())
195196
}
196197
}
197198
}
@@ -212,17 +213,21 @@ impl PyByteArray {
212213

213214
#[pymethod(magic)]
214215
pub fn delitem(&self, needle: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> {
215-
let elements = &mut self.try_resizable(vm)?.elements;
216216
match SequenceIndex::try_from_object_for(vm, needle, Self::NAME)? {
217217
SequenceIndex::Int(int) => {
218+
let elements = &mut self.try_resizable(vm)?.elements;
218219
if let Some(idx) = elements.wrap_index(int) {
219220
elements.remove(idx);
220221
Ok(())
221222
} else {
222223
Err(vm.new_index_error("index out of range".to_owned()))
223224
}
224225
}
225-
SequenceIndex::Slice(slice) => elements.delete_slice(vm, &slice),
226+
SequenceIndex::Slice(slice) => {
227+
let slice = slice.to_saturated_indices(vm)?;
228+
let elements = &mut self.try_resizable(vm)?.elements;
229+
elements.delete_slice(vm, slice)
230+
}
226231
}
227232
}
228233

‎vm/src/builtins/list.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -213,8 +213,9 @@ impl PyList {
213213
fn setslice(&self, slice: PySliceRef, sec: ArgIterable, vm: &VirtualMachine) -> PyResult<()> {
214214
let items: Result<Vec<PyObjectRef>, _> = sec.iter(vm)?.collect();
215215
let items = items?;
216+
let slice = slice.to_saturated_indices(vm)?;
216217
let mut elements = self.borrow_vec_mut();
217-
elements.set_slice_items(vm, &slice, items.as_slice())
218+
elements.set_slice_items(vm, slice, items.as_slice())
218219
}
219220

220221
#[pymethod(magic)]
@@ -373,7 +374,8 @@ impl PyList {
373374
}
374375

375376
fn delslice(&self, slice: PySliceRef, vm: &VirtualMachine) -> PyResult<()> {
376-
self.borrow_vec_mut().delete_slice(vm, &slice)
377+
let slice = slice.to_saturated_indices(vm)?;
378+
self.borrow_vec_mut().delete_slice(vm, slice)
377379
}
378380

379381
#[pymethod]

‎vm/src/builtins/memory.rs

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,4 @@
1-
use super::{
2-
slice::SaturatedIndices, PyBytes, PyBytesRef, PyList, PyListRef, PySliceRef, PyStr, PyStrRef,
3-
PyTypeRef,
4-
};
1+
use super::{PyBytes, PyBytesRef, PyList, PyListRef, PySliceRef, PyStr, PyStrRef, PyTypeRef};
52
use crate::common::{
63
borrow::{BorrowedValue, BorrowedValueMut},
74
hash::PyHash,
@@ -12,7 +9,7 @@ use crate::{
129
bytesinner::bytes_to_hex,
1310
function::{FuncArgs, IntoPyObject, OptionalArg},
1411
protocol::{BufferInternal, BufferOptions, PyBuffer, PyMappingMethods},
15-
sliceable::{wrap_index, SequenceIndex},
12+
sliceable::{wrap_index, SaturatedIndices, SequenceIndex},
1613
slots::{AsBuffer, AsMapping, Comparable, Hashable, PyComparisonOp, SlotConstructor},
1714
stdlib::pystruct::FormatSpec,
1815
utils::Either,
@@ -256,7 +253,8 @@ impl PyMemoryView {
256253
fn getitem_by_slice(zelf: PyRef<Self>, slice: PySliceRef, vm: &VirtualMachine) -> PyResult {
257254
// slicing a memoryview return a new memoryview
258255
let len = zelf.buffer.options.len;
259-
let (range, step, is_negative_step) = SaturatedIndices::new(&slice, vm)?.adjust_indices(len);
256+
let (range, step, is_negative_step) =
257+
SaturatedIndices::new(&slice, vm)?.adjust_indices(len);
260258
let abs_step = step.unwrap();
261259
let step = if is_negative_step {
262260
-(abs_step as isize)

‎vm/src/builtins/slice.rs

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ impl PySlice {
7171
))
7272
}
7373

74-
pub fn to_raw_indices(&self, vm: &VirtualMachine) -> PyResult<SaturatedIndices> {
74+
pub fn to_saturated_indices(&self, vm: &VirtualMachine) -> PyResult<SaturatedIndices> {
7575
SaturatedIndices::new(self, vm)
7676
}
7777

@@ -247,13 +247,14 @@ impl Comparable for PySlice {
247247

248248
impl Unhashable for PySlice {}
249249

250-
/// A saturated slice with values ranging in [isize::MIN, isize::MAX]. Used for
250+
/// A saturated slice with values ranging in [isize::MIN, isize::MAX]. Used for
251251
/// slicable sequences that require indices in the aforementioned range.
252-
///
252+
///
253253
/// Invokes `__index__` on the PySliceRef during construction so as to separate the
254254
/// transformation from PyObject into isize and the adjusting of the slice to a given
255255
/// sequence length. The reason this is important is due to the fact that an objects
256-
/// `__index__` might get a lock on the sequence and cause a deadlock.
256+
/// `__index__` might get a lock on the sequence and cause a deadlock.
257+
#[derive(Copy, Clone, Debug)]
257258
pub struct SaturatedIndices {
258259
start: isize,
259260
stop: isize,

‎vm/src/sliceable.rs

Lines changed: 20 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,9 @@ use num_traits::ToPrimitive;
22
use std::ops::Range;
33

44
use crate::builtins::int::PyInt;
5-
use crate::builtins::slice::{saturate_index, PySlice, PySliceRef, SaturatedIndices};
5+
// export through slicable module, not slice.
6+
pub use crate::builtins::slice::SaturatedIndices;
7+
use crate::builtins::slice::{saturate_index, PySlice, PySliceRef};
68
use crate::utils::Either;
79
use crate::VirtualMachine;
810
use crate::{PyObjectRef, PyResult, TypeProtocol};
@@ -23,11 +25,10 @@ pub trait PySliceableSequenceMut {
2325
fn set_slice_items_no_resize(
2426
&mut self,
2527
vm: &VirtualMachine,
26-
slice: &PySlice,
28+
slice: SaturatedIndices,
2729
items: &[Self::Item],
2830
) -> PyResult<()> {
29-
let (range, step, is_negative_step) =
30-
SaturatedIndices::new(slice, vm)?.adjust_indices(self.as_slice().len());
31+
let (range, step, is_negative_step) = slice.adjust_indices(self.as_slice().len());
3132
if !is_negative_step && step == Some(1) {
3233
return if range.end - range.start == items.len() {
3334
self.do_set_range(range, items);
@@ -82,11 +83,10 @@ pub trait PySliceableSequenceMut {
8283
fn set_slice_items(
8384
&mut self,
8485
vm: &VirtualMachine,
85-
slice: &PySlice,
86+
slice: SaturatedIndices,
8687
items: &[Self::Item],
8788
) -> PyResult<()> {
88-
let (range, step, is_negative_step) =
89-
SaturatedIndices::new(slice, vm)?.adjust_indices(self.as_slice().len());
89+
let (range, step, is_negative_step) = slice.adjust_indices(self.as_slice().len());
9090
if !is_negative_step && step == Some(1) {
9191
self.do_set_range(range, items);
9292
return Ok(());
@@ -136,9 +136,8 @@ pub trait PySliceableSequenceMut {
136136
}
137137
}
138138

139-
fn delete_slice(&mut self, vm: &VirtualMachine, slice: &PySlice) -> PyResult<()> {
140-
let (range, step, is_negative_step) =
141-
SaturatedIndices::new(slice, vm)?.adjust_indices(self.as_slice().len());
139+
fn delete_slice(&mut self, _vm: &VirtualMachine, slice: SaturatedIndices) -> PyResult<()> {
140+
let (range, step, is_negative_step) = slice.adjust_indices(self.as_slice().len());
142141
if range.start >= range.end {
143142
return Ok(());
144143
}
@@ -233,9 +232,12 @@ pub trait PySliceableSequence {
233232
saturate_index(p, self.len() as isize)
234233
}
235234

236-
fn get_slice_items(&self, vm: &VirtualMachine, slice: &PySlice) -> PyResult<Self::Sliced> {
237-
let (range, step, is_negative_step) =
238-
SaturatedIndices::new(slice, vm)?.adjust_indices(self.len());
235+
fn get_slice_items(
236+
&self,
237+
_vm: &VirtualMachine,
238+
slice: SaturatedIndices,
239+
) -> PyResult<Self::Sliced> {
240+
let (range, step, is_negative_step) = slice.adjust_indices(self.len());
239241
if range.start >= range.end {
240242
return Ok(Self::empty());
241243
}
@@ -273,7 +275,10 @@ pub trait PySliceableSequence {
273275
})?;
274276
Ok(Either::A(self.do_get(pos_index)))
275277
}
276-
SequenceIndex::Slice(slice) => Ok(Either::B(self.get_slice_items(vm, &slice)?)),
278+
SequenceIndex::Slice(slice) => {
279+
let slice = slice.to_saturated_indices(vm)?;
280+
Ok(Either::B(self.get_slice_items(vm, slice)?))
281+
}
277282
}
278283
}
279284
}
@@ -390,4 +395,4 @@ pub(crate) fn wrap_index(p: isize, len: usize) -> Option<usize> {
390395
} else {
391396
Some(p)
392397
}
393-
}
398+
}

0 commit comments

Comments
 (0)