python · encukou · May 22, 2019 · May 17, 2019 · May 22, 2019 · encukou
diff --git a/Lib/test/test_call.py b/Lib/test/test_call.py
@@ -8,6 +8,7 @@
 import struct
 import collections
 import itertools
+import gc


 class FunctionCalls(unittest.TestCase):
@@ -457,6 +458,22 @@ def test_fastcall_keywords(self):
                result = _testcapi.pyobject_fastcallkeywords(func, args, kwnames)
                self.check_result(result, expected)

+    def test_fastcall_clearing_dict(self):
+        # Test bpo-36907: the point of the test is just checking that this
+        # does not crash.
+        class IntWithDict:
+            __slots__ = ["kwargs"]
+            def __init__(self, **kwargs):
+                self.kwargs = kwargs
+            def __index__(self):
+                self.kwargs.clear()
+                gc.collect()
+                return 0
+        x = IntWithDict(dont_inherit=IntWithDict())
+        # We test the argument handling of "compile" here, the compilation
+        # itself is not relevant. When we pass flags=x below, x.__index__() is
+        # called, which changes the keywords dict.
+        compile("pass", "", "exec", x, **x.kwargs)

 if __name__ == "__main__":
    unittest.main()
diff --git a/Misc/NEWS.d/next/Core and Builtins/2019-05-17-12-28-24.bpo-36907.rk7kgp.rst b/Misc/NEWS.d/next/Core and Builtins/2019-05-17-12-28-24.bpo-36907.rk7kgp.rst
@@ -0,0 +1,2 @@
+Fix a crash when calling a C function with a keyword dict (``f(**kwargs)``)
+and changing the dict ``kwargs`` while that function is running.
diff --git a/Objects/call.c b/Objects/call.c
@@ -544,10 +544,14 @@ _PyMethodDef_RawFastCallDict(PyMethodDef *method, PyObject *self,
        }

        result = (*fastmeth) (self, stack, nargs, kwnames);
-        if (stack != args) {
+        if (kwnames != NULL) {
+            Py_ssize_t i, n = nargs + PyTuple_GET_SIZE(kwnames);
+            for (i = 0; i < n; i++) {
+                Py_DECREF(stack[i]);
+            }
            PyMem_Free((PyObject **)stack);
+            Py_DECREF(kwnames);
        }
-        Py_XDECREF(kwnames);
        break;
    }

@@ -1334,8 +1338,11 @@ _PyStack_UnpackDict(PyObject *const *args, Py_ssize_t nargs, PyObject *kwargs,
        return -1;
    }

-    /* Copy position arguments (borrowed references) */
-    memcpy(stack, args, nargs * sizeof(stack[0]));
+    /* Copy positional arguments */
+    for (i = 0; i < nargs; i++) {
+        Py_INCREF(args[i]);
+        stack[i] = args[i];
+    }

    kwstack = stack + nargs;
    pos = i = 0;
@@ -1344,8 +1351,8 @@ _PyStack_UnpackDict(PyObject *const *args, Py_ssize_t nargs, PyObject *kwargs,
       called in the performance critical hot code. */
    while (PyDict_Next(kwargs, &pos, &key, &value)) {
        Py_INCREF(key);
+        Py_INCREF(value);
        PyTuple_SET_ITEM(kwnames, i, key);
-        /* The stack contains borrowed references */
        kwstack[i] = value;
        i++;
    }
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		Fix a crash when calling a C function with a keyword dict (``f(**kwargs)``)
		and changing the dict ``kwargs`` while that function is running.
-Original file line number
+Diff line change
@@ -544,10 +544,14 @@ _PyMethodDef_RawFastCallDict(PyMethodDef *method, PyObject *self,
             }
             result = (*fastmeth) (self, stack, nargs, kwnames);
-            if (stack != args) {
+            if (kwnames != NULL) {
+                Py_ssize_t i, n = nargs + PyTuple_GET_SIZE(kwnames);
+                for (i = 0; i < n; i++) {
+                    Py_DECREF(stack[i]);
+                }
                 PyMem_Free((PyObject **)stack);
+                Py_DECREF(kwnames);
             }
-            Py_XDECREF(kwnames);
             break;
         }
@@ -1334,8 +1338,11 @@ _PyStack_UnpackDict(PyObject *const *args, Py_ssize_t nargs, PyObject *kwargs,
             return -1;
         }
-        /* Copy position arguments (borrowed references) */
-        memcpy(stack, args, nargs * sizeof(stack[0]));
+        /* Copy positional arguments */
+        for (i = 0; i < nargs; i++) {
+            Py_INCREF(args[i]);
+            stack[i] = args[i];
+        }
                 Outdated
          
        

                


  
    
        
        
  
    
      
          
    
  


        
            
  
      
              Copy link

  
        
      
    

    
        

  
  Member


        

    
  

  
    
      
          
      

      
            encukou
  

      

      

      


        May 17, 2019


      
    

  


        
      
  
  
  
    

    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.
    

    
      
      


  


  
    
      Is there a reason to not keep memcpy for the bulk copy, and only add an INCREF loop?
    
  
  



    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

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


  
  


          
              
  
    
    
      
        
            
    All reactions
  


          
          
        
      
    


      



    
        
  
    
      
          
    
  


        
            
  
      
              Copy link

  
        
      
    

    
        

  
    Contributor


        

  Author


    
  

  
    
      
          
      

      
            jdemeyer
  

      

      

      


        May 17, 2019


      
    

  


        
      
  
  
  
    

    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.
    

    
      
      


  


  
    
      
Is there a reason to not keep memcpy for the bulk copy, and only add an INCREF loop?

Not really, it's mostly a matter of style. I like doing the INCREF at the same time as the assignment. I also find the for loop variant simpler to understand.
    
  
  



    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

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


  
  


          
              
  
    
    
      
        
            
    All reactions
         kwstack = stack + nargs;
         pos = i = 0;
@@ -1344,8 +1351,8 @@ _PyStack_UnpackDict(PyObject *const *args, Py_ssize_t nargs, PyObject *kwargs,
            called in the performance critical hot code. */
         while (PyDict_Next(kwargs, &pos, &key, &value)) {
             Py_INCREF(key);
+            Py_INCREF(value);
             PyTuple_SET_ITEM(kwnames, i, key);
-            /* The stack contains borrowed references */
             kwstack[i] = value;
             i++;
         }
-          Expand Down