python · JelleZijlstra · Feb 24, 2023 · Feb 10, 2023 · Feb 10, 2023 · Feb 10, 2023
diff --git a/Lib/test/test_iter.py b/Lib/test/test_iter.py
@@ -7,6 +7,9 @@
 from test.support import check_free_after_iterating, ALWAYS_EQ, NEVER_EQ
 import pickle
 import collections.abc
+import functools
+import contextlib
+import builtins

 # Test result of triple loop (too big to inline)
 TRIPLETS = [(0, 0, 0), (0, 0, 1), (0, 0, 2),
@@ -91,6 +94,12 @@ def __call__(self):
            raise IndexError # Emergency stop
        return i

+class EmptyIterClass:
+    def __len__(self):
+        return 0
+    def __getitem__(self, i):
+        raise StopIteration
+
 # Main test suite

 class TestCase(unittest.TestCase):
@@ -238,6 +247,81 @@ def test_mutating_seq_class_exhausted_iter(self):
        self.assertEqual(list(empit), [5, 6])
        self.assertEqual(list(a), [0, 1, 2, 3, 4, 5, 6])

+    def test_reduce_mutating_builtins_iter(self):
+        # This is a reproducer of issue #101765
+        # where iter `__reduce__` calls could lead to a segfault or SystemError
+        # depending on the order of C argument evaluation, which is undefined
+
+        # Backup builtins
+        builtins_dict = builtins.__dict__
+        orig = {"iter": iter, "reversed": reversed}
+
+        def run(builtin_name, item, init=None, sentinel=None):
+            if init is not None:
+                args = init if isinstance(init, tuple) else (init,)
+                item = item(*args)
+            it = iter(item) if sentinel is None else iter(item, sentinel)
+
+            class CustomStr:
+                def __init__(self, name, iterator):
+                    self.name = name
+                    self.iterator = iterator
+                def __hash__(self):
+                    return hash(self.name)
+                def __eq__(self, other):
+                    # Here we exhaust our iterator, possibly changing
+                    # its `it_seq` pointer to NULL
+                    # The `__reduce__` call should correctly get
+                    # the pointers after this call
+                    list(self.iterator)
+                    return other == self.name
+
+            # del is required here
+            # to avoid calling the last test case's custom __eq__
+            del builtins_dict[builtin_name]
+            builtins_dict[CustomStr(builtin_name, it)] = orig[builtin_name]
+
+            return it.__reduce__()
+
+        types = [
+            (EmptyIterClass, ()),
+            (bytes, 8),
+            (bytearray, 8),
+            (tuple, range(8)),
+            (lambda: 0, None, 0),
+            (tuple[int],)  # GenericAlias
+        ]
+
+        try:
+            run_iter = functools.partial(run, "iter")
+            # The returned value of `__reduce__` should not only be valid
+            # but also *empty*, as `it` was exhausted during `__eq__`
+            # i.e "xyz" returns (iter, ("",))
+            self.assertEqual(run_iter(str, "xyz"), (orig["iter"], ("",)))
+            self.assertEqual(run_iter(list, range(8)), (orig["iter"], ([],)))
+
+            # _PyEval_GetBuiltin is also called for `reversed` in a branch of
+            # listiter_reduce_general
+            self.assertEqual(
+                run("reversed", orig["reversed"], list(range(8))),
+                (iter, ([],))
+            )
+
+            for case in types:
+                self.assertEqual(run_iter(*case), (orig["iter"], ((),)))
+        finally:
+            # del is required here
+            # to avoid calling our custom __eq__
+            # we also need to supress KeyErrors in case
+            # a failed test deletes the key without setting anything
+            with contextlib.suppress(KeyError):
+                del builtins_dict["iter"]
+            with contextlib.suppress(KeyError):
+                del builtins_dict["reversed"]
+            # Restore original builtins
+            for key, func in orig.items():
+                builtins_dict[key] = func
+
    # Test a new_style class with __iter__ but no next() method
    def test_new_style_iter_class(self):
        class IterClass(object):

diff --git a/Misc/NEWS.d/next/Core and Builtins/2023-02-10-07-21-47.gh-issue-101765.MO5LlC.rst b/Misc/NEWS.d/next/Core and Builtins/2023-02-10-07-21-47.gh-issue-101765.MO5LlC.rst
@@ -0,0 +1 @@
+Fix SystemError / segmentation fault in iter ``__reduce__`` when calling ``__builtins__.__dict__["iter"]`` mutates the iter object.
diff --git a/Objects/bytearrayobject.c b/Objects/bytearrayobject.c
@@ -2391,11 +2391,17 @@ PyDoc_STRVAR(length_hint_doc,
 static PyObject *
 bytearrayiter_reduce(bytesiterobject *it, PyObject *Py_UNUSED(ignored))
 {
+    PyObject *iter = _PyEval_GetBuiltin(&_Py_ID(iter));
+
+    /* _PyEval_GetBuiltin can invoke arbitrary code.
+     * calls must be *before* access of `it` pointers,
+     * since C parameter eval order is undefined.
+     * see issue #101765 */
+
    if (it->it_seq != NULL) {
-        return Py_BuildValue("N(O)n", _PyEval_GetBuiltin(&_Py_ID(iter)),
-                             it->it_seq, it->it_index);
+        return Py_BuildValue("N(O)n", iter, it->it_seq, it->it_index);
    } else {
-        return Py_BuildValue("N(())", _PyEval_GetBuiltin(&_Py_ID(iter)));
+        return Py_BuildValue("N(())", iter);
    }
 }


diff --git a/Objects/bytesobject.c b/Objects/bytesobject.c
@@ -3169,11 +3169,17 @@ PyDoc_STRVAR(length_hint_doc,
 static PyObject *
 striter_reduce(striterobject *it, PyObject *Py_UNUSED(ignored))
 {
+    PyObject *iter = _PyEval_GetBuiltin(&_Py_ID(iter));
+
+    /* _PyEval_GetBuiltin can invoke arbitrary code.
+     * calls must be *before* access of `it` pointers,
+     * since C parameter eval order is undefined.
+     * see issue #101765 */
+
    if (it->it_seq != NULL) {
-        return Py_BuildValue("N(O)n", _PyEval_GetBuiltin(&_Py_ID(iter)),
-                             it->it_seq, it->it_index);
+        return Py_BuildValue("N(O)n", iter, it->it_seq, it->it_index);
    } else {
-        return Py_BuildValue("N(())", _PyEval_GetBuiltin(&_Py_ID(iter)));
+        return Py_BuildValue("N(())", iter);
    }
 }


diff --git a/Objects/genericaliasobject.c b/Objects/genericaliasobject.c
@@ -877,8 +877,18 @@ ga_iter_clear(PyObject *self) {
 static PyObject *
 ga_iter_reduce(PyObject *self, PyObject *Py_UNUSED(ignored))
 {
+    PyObject *iter = _PyEval_GetBuiltin(&_Py_ID(iter));
    gaiterobject *gi = (gaiterobject *)self;
-    return Py_BuildValue("N(O)", _PyEval_GetBuiltin(&_Py_ID(iter)), gi->obj);
+
+    /* _PyEval_GetBuiltin can invoke arbitrary code.
+     * call must be *before* access of `gi` pointers,
+     * since C parameter eval order is undefined.
+     * see issue #101765 */
+
+    if (gi->obj)
+        return Py_BuildValue("N(O)", iter, gi->obj);
+    else
+        return Py_BuildValue("N(())", iter);
 }

 static PyMethodDef ga_iter_methods[] = {

diff --git a/Objects/iterobject.c b/Objects/iterobject.c
@@ -102,11 +102,17 @@ PyDoc_STRVAR(length_hint_doc, "Private method returning an estimate of len(list(
 static PyObject *
 iter_reduce(seqiterobject *it, PyObject *Py_UNUSED(ignored))
 {
+    PyObject *iter = _PyEval_GetBuiltin(&_Py_ID(iter));
+
+    /* _PyEval_GetBuiltin can invoke arbitrary code.
+     * calls must be *before* access of `it` pointers,
+     * since C parameter eval order is undefined.
+     * see issue #101765 */
+
    if (it->it_seq != NULL)
-        return Py_BuildValue("N(O)n", _PyEval_GetBuiltin(&_Py_ID(iter)),
-                             it->it_seq, it->it_index);
+        return Py_BuildValue("N(O)n", iter, it->it_seq, it->it_index);
    else
-        return Py_BuildValue("N(())", _PyEval_GetBuiltin(&_Py_ID(iter)));
+        return Py_BuildValue("N(())", iter);
 }

 PyDoc_STRVAR(reduce_doc, "Return state information for pickling.");
@@ -239,11 +245,17 @@ calliter_iternext(calliterobject *it)
 static PyObject *
 calliter_reduce(calliterobject *it, PyObject *Py_UNUSED(ignored))
 {
+    PyObject *iter = _PyEval_GetBuiltin(&_Py_ID(iter));
+
+    /* _PyEval_GetBuiltin can invoke arbitrary code.
+     * calls must be *before* access of `it` pointers,
+     * since C parameter eval order is undefined.
+     * see issue #101765 */
+
    if (it->it_callable != NULL && it->it_sentinel != NULL)
-        return Py_BuildValue("N(OO)", _PyEval_GetBuiltin(&_Py_ID(iter)),
-                             it->it_callable, it->it_sentinel);
+        return Py_BuildValue("N(OO)", iter, it->it_callable, it->it_sentinel);
    else
-        return Py_BuildValue("N(())", _PyEval_GetBuiltin(&_Py_ID(iter)));
+        return Py_BuildValue("N(())", iter);
 }

 static PyMethodDef calliter_methods[] = {

diff --git a/Objects/listobject.c b/Objects/listobject.c
@@ -3444,18 +3444,23 @@ listiter_reduce_general(void *_it, int forward)
 {
    PyObject *list;

+    /* _PyEval_GetBuiltin can invoke arbitrary code.
+     * calls must be *before* access of `_it` pointers,
+     * since C parameter eval order is undefined.
+     * see issue #101765 */
+
    /* the objects are not the same, index is of different types! */
    if (forward) {
+        PyObject *iter = _PyEval_GetBuiltin(&_Py_ID(iter));
        _PyListIterObject *it = (_PyListIterObject *)_it;
        if (it->it_seq) {
-            return Py_BuildValue("N(O)n", _PyEval_GetBuiltin(&_Py_ID(iter)),
-                                 it->it_seq, it->it_index);
+            return Py_BuildValue("N(O)n", iter, it->it_seq, it->it_index);
        }
    } else {
+        PyObject *reversed = _PyEval_GetBuiltin(&_Py_ID(reversed));
        listreviterobject *it = (listreviterobject *)_it;
        if (it->it_seq) {
-            return Py_BuildValue("N(O)n", _PyEval_GetBuiltin(&_Py_ID(reversed)),
-                                 it->it_seq, it->it_index);
+            return Py_BuildValue("N(O)n", reversed, it->it_seq, it->it_index);
        }
    }
    /* empty iterator, create an empty list */

diff --git a/Objects/tupleobject.c b/Objects/tupleobject.c
@@ -1048,11 +1048,17 @@ PyDoc_STRVAR(length_hint_doc, "Private method returning an estimate of len(list(
 static PyObject *
 tupleiter_reduce(_PyTupleIterObject *it, PyObject *Py_UNUSED(ignored))
 {
+    PyObject *iter = _PyEval_GetBuiltin(&_Py_ID(iter));
+
+    /* _PyEval_GetBuiltin can invoke arbitrary code.
+     * calls must be *before* access of `it` pointers,
+     * since C parameter eval order is undefined.
+     * see issue #101765 */
+
    if (it->it_seq)
-        return Py_BuildValue("N(O)n", _PyEval_GetBuiltin(&_Py_ID(iter)),
-                             it->it_seq, it->it_index);
+        return Py_BuildValue("N(O)n", iter, it->it_seq, it->it_index);
    else
-        return Py_BuildValue("N(())", _PyEval_GetBuiltin(&_Py_ID(iter)));
+        return Py_BuildValue("N(())", iter);
 }

 static PyObject *

diff --git a/Objects/unicodeobject.c b/Objects/unicodeobject.c
@@ -14784,14 +14784,20 @@ PyDoc_STRVAR(length_hint_doc, "Private method returning an estimate of len(list(
 static PyObject *
 unicodeiter_reduce(unicodeiterobject *it, PyObject *Py_UNUSED(ignored))
 {
+    PyObject *iter = _PyEval_GetBuiltin(&_Py_ID(iter));
+
+    /* _PyEval_GetBuiltin can invoke arbitrary code.
+     * calls must be *before* access of `it` pointers,
+     * since C parameter eval order is undefined.
+     * see issue #101765 */
+
    if (it->it_seq != NULL) {
-        return Py_BuildValue("N(O)n", _PyEval_GetBuiltin(&_Py_ID(iter)),
-                             it->it_seq, it->it_index);
+        return Py_BuildValue("N(O)n", iter, it->it_seq, it->it_index);
    } else {
        PyObject *u = unicode_new_empty();
        if (u == NULL)
            return NULL;
-        return Py_BuildValue("N(N)", _PyEval_GetBuiltin(&_Py_ID(iter)), u);
+        return Py_BuildValue("N(N)", iter, u);
    }
 }
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		Fix SystemError / segmentation fault in iter ``__reduce__`` when calling ``__builtins__.__dict__["iter"]`` mutates the iter object.
carljm marked this conversation as resolved. Show resolved Hide resolved
-Original file line number
+Diff line change
@@ -2391,11 +2391,17 @@ PyDoc_STRVAR(length_hint_doc,
     static PyObject *
     bytearrayiter_reduce(bytesiterobject *it, PyObject *Py_UNUSED(ignored))
     {
+        PyObject *iter = _PyEval_GetBuiltin(&_Py_ID(iter));
+        /* _PyEval_GetBuiltin can invoke arbitrary code.
+         * calls must be *before* access of `it` pointers,
+         * since C parameter eval order is undefined.
             Copy link

  
      
    
  

  
      

  
  Member


      

  

  
    
      

      
            JelleZijlstra
  

      

      

      


        Feb 22, 2023


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

    
      The reason will be displayed to describe this comment to others. Learn more.
    

    
      
      


  


  
    
      I think parameter eval order isn't the issue and shouldn't be mentioned. The problem is that the _PyEval_GetBuiltin call invalidates the previous if check.
For example, this code would also be buggy:
    if (it->it_seq != NULL) {
        PyObject *iter = _PyEval_GetBuiltin(&_Py_ID(iter));
        return Py_BuildValue("N(O)n", iter, it->it_seq, it->it_index);
    }

The parameter evaluation order makes things a bit more unpredictable, but the bug is that the if check is done too early, not that we're relying on evaluation order.
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
      
        
            
    All reactions
  


          
          
        
      
    

    



  
        
  
    
        
    
  


      
          
  
      
            Copy link

  
      
    
  

  
      

  
    Contributor


      

  Author


  

  
    
      

      
            ionite34
  

      

      

      


        Feb 24, 2023


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

    
      The reason will be displayed to describe this comment to others. Learn more.
    

    
      
      


  


  
    
      Hm, yeah. I think this might be better?
    /* _PyEval_GetBuiltin can invoke arbitrary code,
     * call must be before access of iterator pointers.
     * see issue #101765 */

    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
      
        
            
    All reactions
  


          
          
        
      
    

    



  
        
  
    
        
    
  


      
          
  
      
            Copy link

  
      
    
  

  
      

  
  Member


      

  

  
    
      

      
            JelleZijlstra
  

      

      

      


        Feb 24, 2023


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

    
      The reason will be displayed to describe this comment to others. Learn more.
    

    
      
      


  


  
    
      sounds good!
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
      
        
            
    All reactions
  


          
          
        
      
    

    



  
        
  
    
        
    
  


      
          
  
      
            Copy link

  
      
    
  

  
      

  
    Contributor


      

  Author


  

  
    
      

      
            ionite34
  

      

      

      


        Feb 24, 2023


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

    
      The reason will be displayed to describe this comment to others. Learn more.
    

    
      
      


  


  
    
      Updated all comments to the above in 9b664c2
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
      
        
            
    All reactions
+         * see issue #101765 */
         if (it->it_seq != NULL) {
-            return Py_BuildValue("N(O)n", _PyEval_GetBuiltin(&_Py_ID(iter)),
-                                 it->it_seq, it->it_index);
+            return Py_BuildValue("N(O)n", iter, it->it_seq, it->it_index);
         } else {
-            return Py_BuildValue("N(())", _PyEval_GetBuiltin(&_Py_ID(iter)));
+            return Py_BuildValue("N(())", iter);
         }
     }
-          Expand Down