Fixes and tests for various "holding pointers when arbitrary Python code

can run" bugs as discussed in

[ 848856 ] couple of new list.sort bugs

Lib/test/test_sort.py

@@ -193,6 +193,51 @@ class TestDecorateSortUndecorate(unittest.TestCase):
         self.assertRaises(ZeroDivisionError, data.sort, None, lambda x: 1/x)
         self.assertEqual(data, dup)
 
+    def test_key_with_mutation(self):
+        data = range(10)
+        def k(x):
+            del data[:]
+            data[:] = range(20)
+            return x
+        self.assertRaises(ValueError, data.sort, key=k)
+
+    def test_key_with_mutating_del(self):
+        data = range(10)
+        class SortKiller(object):
+            def __init__(self, x):
+                pass
+            def __del__(self):
+                del data[:]
+                data[:] = range(20)
+        self.assertRaises(ValueError, data.sort, key=SortKiller)
+
+    def test_key_with_mutating_del_and_exception(self):
+        data = range(10)
+        ## dup = data[:]
+        class SortKiller(object):
+            def __init__(self, x):
+                if x > 2:
+                    raise RuntimeError
+            def __del__(self):
+                del data[:]
+                data[:] = range(20)
+        self.assertRaises(RuntimeError, data.sort, key=SortKiller)
+        ## major honking subtlety: we *can't* do:
+        ##
+        ## self.assertEqual(data, dup)
+        ##
+        ## because there is a reference to a SortKiller in the
+        ## traceback and by the time it dies we're outside the call to
+        ## .sort() and so the list protection gimmicks are out of
+        ## date (this cost some brain cells to figure out...).
+
+    def test_key_with_exception(self):
+        # Verify that the wrapper has been removed
+        data = range(-2,2)
+        dup = data[:]
+        self.assertRaises(ZeroDivisionError, data.sort, None, lambda x: 1/x)
+        self.assertEqual(data, dup)
+
     def test_reverse(self):
         data = range(100)
         random.shuffle(data)

Objects/listobject.c

@@ -1848,7 +1848,7 @@ listsort(PyListObject *self, PyObject *args, PyObject *kwds)
 	PyObject *result = NULL;	/* guilty until proved innocent */
 	int reverse = 0;
 	PyObject *keyfunc = NULL;
-	int i, len = 0;
+	int i;
 	PyObject *key, *value, *kvpair;
 	static char *kwlist[] = {"cmp", "key", "reverse", 0};
 
@@ -1870,35 +1870,6 @@ listsort(PyListObject *self, PyObject *args, PyObject *kwds)
 	} else
 		Py_XINCREF(compare);
 
-	if (keyfunc != NULL) {
-		len = PyList_GET_SIZE(self);
-		for (i=0 ; i < len ; i++) {
-			value = PyList_GET_ITEM(self, i);
-			key = PyObject_CallFunctionObjArgs(keyfunc, value, 
-							   NULL);
-			if (key == NULL) {
-				for (i=i-1 ; i>=0 ; i--) {
-					kvpair = PyList_GET_ITEM(self, i);
-					value = sortwrapper_getvalue(kvpair);
-					PyList_SET_ITEM(self, i, value);
-					Py_DECREF(kvpair);
-				}
-				goto dsu_fail;
-			}
-			kvpair = build_sortwrapper(key, value);
-			if (kvpair == NULL)
-				goto dsu_fail;
-			PyList_SET_ITEM(self, i, kvpair);
-		}
-	}
-
-	/* Reverse sort stability achieved by initially reversing the list,
-	applying a stable forward sort, then reversing the final result. */
-	if (reverse && self->ob_size > 1)
-		reverse_slice(self->ob_item, self->ob_item + self->ob_size);
-
-	merge_init(&ms, compare);
-
 	/* The list is temporarily made empty, so that mutations performed
 	 * by comparison functions can't affect the slice of memory we're
 	 * sorting (allowing mutations during sorting is a core-dump
@@ -1909,6 +1880,46 @@ listsort(PyListObject *self, PyObject *args, PyObject *kwds)
 	self->ob_size = 0;
 	self->ob_item = empty_ob_item = PyMem_NEW(PyObject *, 0);
 
+	if (keyfunc != NULL) {
+		for (i=0 ; i < saved_ob_size ; i++) {
+			value = saved_ob_item[i];
+			key = PyObject_CallFunctionObjArgs(keyfunc, value, 
+							   NULL);
+			if (key == NULL) {
+				for (i=i-1 ; i>=0 ; i--) {
+					kvpair = saved_ob_item[i];
+					value = sortwrapper_getvalue(kvpair);
+					saved_ob_item[i] = value;
+					Py_DECREF(kvpair);
+				}
+				if (self->ob_item != empty_ob_item 
+				    || self->ob_size) {
+					/* If the list changed *as well* we
+					   have two errors.  We let the first
+					   one "win", but we shouldn't let
+					   what's in the list currently
+					   leak. */
+					(void)list_ass_slice(
+						self, 0, self->ob_size,
+						(PyObject *)NULL);
+				}
+				
+				goto dsu_fail;
+			}
+			kvpair = build_sortwrapper(key, value);
+			if (kvpair == NULL)
+				goto dsu_fail;
+			saved_ob_item[i] = kvpair;
+		}
+	}
+
+	/* Reverse sort stability achieved by initially reversing the list,
+	applying a stable forward sort, then reversing the final result. */
+	if (reverse && saved_ob_size > 1)
+		reverse_slice(saved_ob_item, saved_ob_item + saved_ob_size);
+
+	merge_init(&ms, compare);
+
 	nremaining = saved_ob_size;
 	if (nremaining < 2)
 		goto succeed;
@@ -1959,6 +1970,15 @@ listsort(PyListObject *self, PyObject *args, PyObject *kwds)
 succeed:
 	result = Py_None;
 fail:
+	if (keyfunc != NULL) {
+		for (i=0 ; i < saved_ob_size ; i++) {
+			kvpair = saved_ob_item[i];
+			value = sortwrapper_getvalue(kvpair);
+			saved_ob_item[i] = value;
+			Py_DECREF(kvpair);
+		}
+	}
+
 	if (self->ob_item != empty_ob_item || self->ob_size) {
 		/* The user mucked with the list during the sort. */
 		(void)list_ass_slice(self, 0, self->ob_size, (PyObject *)NULL);
@@ -1968,26 +1988,17 @@ fail:
 			result = NULL;
 		}
 	}
+
+	if (reverse && saved_ob_size > 1)
+		reverse_slice(saved_ob_item, saved_ob_item + saved_ob_size);
+
+	merge_freemem(&ms);
+
+dsu_fail:
 	if (self->ob_item == empty_ob_item)
 		PyMem_FREE(empty_ob_item);
 	self->ob_size = saved_ob_size;
 	self->ob_item = saved_ob_item;
-	merge_freemem(&ms);
-
-	if (keyfunc != NULL) {
-		len = PyList_GET_SIZE(self);
-		for (i=0 ; i < len ; i++) {
-			kvpair = PyList_GET_ITEM(self, i);
-			value = sortwrapper_getvalue(kvpair);
-			PyList_SET_ITEM(self, i, value);
-			Py_DECREF(kvpair);
-		}
-	}
-
-	if (reverse && self->ob_size > 1)
-		reverse_slice(self->ob_item, self->ob_item + self->ob_size);
-
-dsu_fail:
 	Py_XDECREF(compare);
 	Py_XINCREF(result);
 	return result;