Array out of bounds assignment in list_ass_subscript

[MechanicPig]

Crash report

What happened?

Root Cause

When step is not 1 in slice assignment, list_ass_subscript first calculates the length of the slice and then converts the input iterable into a list. During the conversion, arbitrary code in Python can be executed to modify the length of the current list or even clear it:

/* Python 3.10 source code */

static int
list_ass_subscript(PyListObject* self, PyObject* item, PyObject* value)
{
    if (_PyIndex_Check(item)) { /* ... */ }
    else if (PySlice_Check(item)) {
        Py_ssize_t start, stop, step, slicelength;

        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) { /* ... */ }
        else {
            /* assign slice */
            PyObject *ins, *seq;
            PyObject **garbage, **seqitems, **selfitems;
            Py_ssize_t i;
            size_t cur;

            /* protect against a[::-1] = a */
            if (self == (PyListObject*)value) {
                seq = list_slice((PyListObject*)value, 0,
                                   PyList_GET_SIZE(value));
            }
            else {
                seq = PySequence_Fast(value,  // <-- call arbitrary code in python
                                      "must assign iterable "
                                      "to extended slice");
            }
            if (!seq)
                return -1;
            /* ... */
            selfitems = self->ob_item;
            seqitems = PySequence_Fast_ITEMS(seq);
            for (cur = start, i = 0; i < slicelength;
                 cur += (size_t)step, i++) {
                garbage[i] = selfitems[cur];
                ins = seqitems[i];
                Py_INCREF(ins);
                selfitems[cur] = ins;  // <-- maybe out of bounds
            }
            /* ... */
        }
        /* ... */
}

POC

class evil:
    def __init__(self, lst):
        self.lst = lst
    def __iter__(self):
        yield from self.lst
        self.lst.clear()


lst = list(range(10))
lst[::-1] = evil(lst)

CPython versions tested on:

3.10, 3.11, 3.12

Operating systems tested on:

Windows

Output from running 'python -VV' on the command line:

Python 3.10.11 (tags/v3.10.11:7d4cc5a, Apr 5 2023, 00:38:17) [MSC v.1929 64 bit (AMD64)]
Python 3.11.8 (tags/v3.11.8:db85d51, Feb 6 2024, 22:03:32) [MSC v.1937 64 bit (AMD64)]
Python 3.12.3 (tags/v3.12.3:f6650f9, Apr 9 2024, 14:05:25) [MSC v.1938 64 bit (AMD64)]

Linked PRs

• gh-120384: Fix array-out-of-bounds crash in list_ass_subscript #120442
• [3.12] gh-120384: Fix array-out-of-bounds crash in list_ass_subscript (GH-120442) #120825
• [3.13] gh-120384: Fix array-out-of-bounds crash in list_ass_subscript (GH-120442) #120826
• [3.11] gh-120384: gh-120298: Fix array-out-of-bounds & use after free list #121345

[Eclips4]

Thanks for the report! Confirmed on current main.

[sobolevn]

Hm, I have an idea about a potential solution, but it is not very straight-forward.

Result with my patch:

» ./python.exe ex.py
Traceback (most recent call last):
  File "/Users/sobolev/Desktop/cpython2/ex.py", line 10, in <module>
    lst[::-1] = evil(lst)
    ~~~^^^^^^
ValueError: attempt to assign sequence of size 10 to extended slice of size 0                                                                         

What I did?

1. I removed this part:

   cpython/Objects/listobject.c

   Lines 3597 to 3612 in 19435d2

   	Py_ssize_t start, stop, step, slicelength;
   	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;

2. I moved PySlice_AdjustIndices calls after the PySequence_Fast call
3. I also added the same call to value == NULL case

What still needs to be done? TODO:

• This looks quite ugly now, probably a new function is required

Diff:

diff --git Objects/listobject.c Objects/listobject.c
index 6829d5d2865..2a32cec19e9 100644
--- Objects/listobject.c
+++ Objects/listobject.c
@@ -3594,23 +3594,6 @@ 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;
-
-        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 */
             PyObject **garbage;
@@ -3618,6 +3601,23 @@ list_ass_subscript(PyObject* _self, PyObject* item, PyObject* value)
             Py_ssize_t i;
             int res;
 
+            Py_ssize_t start, stop, step, slicelength;
+
+            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 (slicelength <= 0)
                 return 0;
 
@@ -3695,6 +3695,25 @@ list_ass_subscript(PyObject* _self, PyObject* item, PyObject* value)
             if (!seq)
                 return -1;
 
+            Py_ssize_t start, stop, step, slicelength;
+
+            if (PySlice_Unpack(item, &start, &stop, &step) < 0) {
+                return -1;
+            }
+            slicelength = PySlice_AdjustIndices(Py_SIZE(self), &start, &stop,
+                                                step);
+
+            if (step == 1) {
+                Py_DECREF(seq);
+                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 (PySequence_Fast_GET_SIZE(seq) != slicelength) {
                 PyErr_Format(PyExc_ValueError,
                     "attempt to assign sequence of "
                                                        

What do others think? Does this seem like a solution? (please, note the TODO item)

[sobolevn]

cc @serhiy-storchaka @vstinner

[serhiy-storchaka]

PySlice_GetIndicesEx() was split on two steps PySlice_Unpack() and PySlice_AdjustIndices() to avoid a similar problem -- using the old value of Py_SIZE(self) which can be invalidated during calling custom __index__() method.

I think that moving PySlice_AdjustIndices() is the right solution, but you can keep PySlice_Unpack() on its current place.