From 87cc89f975a00e400a9e3e37710ef9eb02822e04 Mon Sep 17 00:00:00 2001 From: sobolevn Date: Thu, 13 Jun 2024 12:18:27 +0300 Subject: [PATCH 1/6] gh-120384: Fix array-out-of-bounds crash in `list_ass_subscript` --- Lib/test/test_list.py | 14 ++++++ ...-06-13-12-17-52.gh-issue-120384.w1UBGl.rst | 2 + Objects/listobject.c | 44 ++++++++++++++----- 3 files changed, 48 insertions(+), 12 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2024-06-13-12-17-52.gh-issue-120384.w1UBGl.rst diff --git a/Lib/test/test_list.py b/Lib/test/test_list.py index d21429fae09b37..4d2d54705fc894 100644 --- a/Lib/test/test_list.py +++ b/Lib/test/test_list.py @@ -245,6 +245,20 @@ def __lt__(self, other): with self.assertRaises(TypeError): a[0] < a + def test_list_index_modifing_operand(self): + # See gh-120384 + class evil: + def __init__(self, lst): + self.lst = lst + def __iter__(self): + yield from self.lst + self.lst.clear() + + lst = list(range(5)) + operand = evil(lst) + with self.assertRaises(ValueError): + lst[::-1] = operand + @cpython_only def test_preallocation(self): iterable = [0] * 10 diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-06-13-12-17-52.gh-issue-120384.w1UBGl.rst b/Misc/NEWS.d/next/Core and Builtins/2024-06-13-12-17-52.gh-issue-120384.w1UBGl.rst new file mode 100644 index 00000000000000..fbd12e625aaa0a --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2024-06-13-12-17-52.gh-issue-120384.w1UBGl.rst @@ -0,0 +1,2 @@ +Fix an array out of bounds crash in ``list_ass_subscript``, which could be +invoked via some specificly tailored evil input. diff --git a/Objects/listobject.c b/Objects/listobject.c index 6829d5d28656cf..ebf0cb2794a206 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -3581,6 +3581,23 @@ list_subscript(PyObject* _self, PyObject* item) } } +static Py_ssize_t +adjust_slice_indexes(PyListObject *lst, + Py_ssize_t *start, Py_ssize_t *stop, + Py_ssize_t step) +{ + Py_ssize_t slicelength = PySlice_AdjustIndices(Py_SIZE(lst), start, stop, + step); + + /* Make sure s[5:2] = [..] inserts at the right place: + before 5, not before 2. */ + if ((step < 0 && *start < *stop) || + (step > 0 && *start > *stop)) + *stop = *start; + + return slicelength; +} + static int list_ass_subscript(PyObject* _self, PyObject* item, PyObject* value) { @@ -3594,22 +3611,11 @@ list_ass_subscript(PyObject* _self, PyObject* item, PyObject* value) return list_ass_item((PyObject *)self, i, value); } else if (PySlice_Check(item)) { - Py_ssize_t start, stop, step, slicelength; + Py_ssize_t start, stop, step; if (PySlice_Unpack(item, &start, &stop, &step) < 0) { return -1; } - slicelength = PySlice_AdjustIndices(Py_SIZE(self), &start, &stop, - step); - - if (step == 1) - return list_ass_slice(self, start, stop, value); - - /* Make sure s[5:2] = [..] inserts at the right place: - before 5, not before 2. */ - if ((step < 0 && start < stop) || - (step > 0 && start > stop)) - stop = start; if (value == NULL) { /* delete slice */ @@ -3618,6 +3624,12 @@ list_ass_subscript(PyObject* _self, PyObject* item, PyObject* value) Py_ssize_t i; int res; + Py_ssize_t slicelength = adjust_slice_indexes(self, &start, &stop, + step); + + if (step == 1) + return list_ass_slice(self, start, stop, value); + if (slicelength <= 0) return 0; @@ -3695,6 +3707,14 @@ list_ass_subscript(PyObject* _self, PyObject* item, PyObject* value) if (!seq) return -1; + Py_ssize_t slicelength = adjust_slice_indexes(self, &start, &stop, + step); + + if (step == 1) { + Py_DECREF(seq); + return list_ass_slice(self, start, stop, value); + } + if (PySequence_Fast_GET_SIZE(seq) != slicelength) { PyErr_Format(PyExc_ValueError, "attempt to assign sequence of " From 73ac63cf2ea1f9692d3e93ae13d94cdd7e1cf28f Mon Sep 17 00:00:00 2001 From: sobolevn Date: Thu, 13 Jun 2024 13:30:14 +0300 Subject: [PATCH 2/6] Add more tests --- Lib/test/test_list.py | 22 ++++++++++++++++++++++ Objects/listobject.c | 3 +-- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_list.py b/Lib/test/test_list.py index 4d2d54705fc894..edb221ff5b237c 100644 --- a/Lib/test/test_list.py +++ b/Lib/test/test_list.py @@ -101,6 +101,28 @@ def test_empty_slice(self): x[:] = x self.assertEqual(x, []) + def test_slice_assign_iterator(self): + class MyIterator: + def __init__(self, lst): + self.lst = lst + self.index = 0 + def __iter__(self): + return self + def __next__(self): + try: + res = self.lst[self.index] + except IndexError: + raise StopIteration(self.index) + self.index += 1 + return res + + x = list(range(5)) + x[0:3] = MyIterator(list(reversed(range(3)))) + self.assertEqual(x, [2, 1, 0, 3, 4]) + + x[:] = MyIterator(list(reversed(range(3)))) + self.assertEqual(x, [2, 1, 0]) + def test_list_resize_overflow(self): # gh-97616: test new_allocated * sizeof(PyObject*) overflow # check in list_resize() diff --git a/Objects/listobject.c b/Objects/listobject.c index ebf0cb2794a206..90afb132dda3e0 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -3711,8 +3711,7 @@ list_ass_subscript(PyObject* _self, PyObject* item, PyObject* value) step); if (step == 1) { - Py_DECREF(seq); - return list_ass_slice(self, start, stop, value); + return list_ass_slice(self, start, stop, seq); } if (PySequence_Fast_GET_SIZE(seq) != slicelength) { From df77cb5678d4f49a67aaf81377df4353479627c1 Mon Sep 17 00:00:00 2001 From: sobolevn Date: Thu, 13 Jun 2024 13:49:15 +0300 Subject: [PATCH 3/6] Fix reflick --- Objects/listobject.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Objects/listobject.c b/Objects/listobject.c index 90afb132dda3e0..a05ddeabeb2e24 100644 --- a/Objects/listobject.c +++ b/Objects/listobject.c @@ -3711,7 +3711,9 @@ list_ass_subscript(PyObject* _self, PyObject* item, PyObject* value) step); if (step == 1) { - return list_ass_slice(self, start, stop, seq); + int res = list_ass_slice(self, start, stop, seq); + Py_DECREF(seq); + return res; } if (PySequence_Fast_GET_SIZE(seq) != slicelength) { From 738662ee3f3f5c6f11f6540df9270d5b0f8fb270 Mon Sep 17 00:00:00 2001 From: sobolevn Date: Thu, 13 Jun 2024 15:48:00 +0300 Subject: [PATCH 4/6] Move test to `list_tests` --- Lib/test/list_tests.py | 8 ++++++++ Lib/test/test_list.py | 22 ---------------------- 2 files changed, 8 insertions(+), 22 deletions(-) diff --git a/Lib/test/list_tests.py b/Lib/test/list_tests.py index 89cd10f76a318e..6b7ed95e712316 100644 --- a/Lib/test/list_tests.py +++ b/Lib/test/list_tests.py @@ -191,6 +191,14 @@ def test_setslice(self): self.assertRaises(TypeError, a.__setitem__) + def test_slice_assign_iterator(self): + x = self.type2test(range(5)) + x[0:3] = iter(self.type2test(reversed(range(3)))) + self.assertEqual(x, [2, 1, 0, 3, 4]) + + x[:] = iter(self.type2test(reversed(range(3)))) + self.assertEqual(x, self.type2test([2, 1, 0])) + def test_delslice(self): a = self.type2test([0, 1]) del a[1:2] diff --git a/Lib/test/test_list.py b/Lib/test/test_list.py index edb221ff5b237c..4d2d54705fc894 100644 --- a/Lib/test/test_list.py +++ b/Lib/test/test_list.py @@ -101,28 +101,6 @@ def test_empty_slice(self): x[:] = x self.assertEqual(x, []) - def test_slice_assign_iterator(self): - class MyIterator: - def __init__(self, lst): - self.lst = lst - self.index = 0 - def __iter__(self): - return self - def __next__(self): - try: - res = self.lst[self.index] - except IndexError: - raise StopIteration(self.index) - self.index += 1 - return res - - x = list(range(5)) - x[0:3] = MyIterator(list(reversed(range(3)))) - self.assertEqual(x, [2, 1, 0, 3, 4]) - - x[:] = MyIterator(list(reversed(range(3)))) - self.assertEqual(x, [2, 1, 0]) - def test_list_resize_overflow(self): # gh-97616: test new_allocated * sizeof(PyObject*) overflow # check in list_resize() From 1b6e0611737cac9657d65148222c32fbefee7d64 Mon Sep 17 00:00:00 2001 From: sobolevn Date: Thu, 13 Jun 2024 15:48:41 +0300 Subject: [PATCH 5/6] Move test to `list_tests` --- Lib/test/list_tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/list_tests.py b/Lib/test/list_tests.py index 6b7ed95e712316..aa6318c355ab75 100644 --- a/Lib/test/list_tests.py +++ b/Lib/test/list_tests.py @@ -194,7 +194,7 @@ def test_setslice(self): def test_slice_assign_iterator(self): x = self.type2test(range(5)) x[0:3] = iter(self.type2test(reversed(range(3)))) - self.assertEqual(x, [2, 1, 0, 3, 4]) + self.assertEqual(x, self.type2test([2, 1, 0, 3, 4])) x[:] = iter(self.type2test(reversed(range(3)))) self.assertEqual(x, self.type2test([2, 1, 0])) From d10c4e57cc2f069f89055b71dc7e6c3bea2b9899 Mon Sep 17 00:00:00 2001 From: sobolevn Date: Thu, 13 Jun 2024 20:56:28 +0300 Subject: [PATCH 6/6] Address review --- Lib/test/list_tests.py | 4 ++-- .../2024-06-13-12-17-52.gh-issue-120384.w1UBGl.rst | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/Lib/test/list_tests.py b/Lib/test/list_tests.py index aa6318c355ab75..dbc5ef4f9f2cd5 100644 --- a/Lib/test/list_tests.py +++ b/Lib/test/list_tests.py @@ -193,10 +193,10 @@ def test_setslice(self): def test_slice_assign_iterator(self): x = self.type2test(range(5)) - x[0:3] = iter(self.type2test(reversed(range(3)))) + x[0:3] = reversed(range(3)) self.assertEqual(x, self.type2test([2, 1, 0, 3, 4])) - x[:] = iter(self.type2test(reversed(range(3)))) + x[:] = reversed(range(3)) self.assertEqual(x, self.type2test([2, 1, 0])) def test_delslice(self): diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-06-13-12-17-52.gh-issue-120384.w1UBGl.rst b/Misc/NEWS.d/next/Core and Builtins/2024-06-13-12-17-52.gh-issue-120384.w1UBGl.rst index fbd12e625aaa0a..4a4db821ce29b8 100644 --- a/Misc/NEWS.d/next/Core and Builtins/2024-06-13-12-17-52.gh-issue-120384.w1UBGl.rst +++ b/Misc/NEWS.d/next/Core and Builtins/2024-06-13-12-17-52.gh-issue-120384.w1UBGl.rst @@ -1,2 +1,3 @@ Fix an array out of bounds crash in ``list_ass_subscript``, which could be -invoked via some specificly tailored evil input. +invoked via some specificly tailored input: including concurrent modification +of a list object, where one thread assigns a slice and another clears it.