python · vstinner · Feb 13, 2019 · Feb 12, 2019 · Feb 12, 2019 · methane
diff --git a/Misc/NEWS.d/next/Core and Builtins/2019-02-12-20-16-34.bpo-35961.7f7Sne.rst b/Misc/NEWS.d/next/Core and Builtins/2019-02-12-20-16-34.bpo-35961.7f7Sne.rst
@@ -0,0 +1,2 @@
+Fix a crash in slice_richcompare(): use strong references rather than stolen
+references for the two temporary internal tuples.
diff --git a/Objects/sliceobject.c b/Objects/sliceobject.c
@@ -565,14 +565,11 @@ static PyMethodDef slice_methods[] = {
 static PyObject *
 slice_richcompare(PyObject *v, PyObject *w, int op)
 {
-    PyObject *t1;
-    PyObject *t2;
-    PyObject *res;
-
     if (!PySlice_Check(v) || !PySlice_Check(w))
         Py_RETURN_NOTIMPLEMENTED;
 
     if (v == w) {
+        PyObject *res;
         /* XXX Do we really need this shortcut?
            There's a unit test for it, but is that fair? */
         switch (op) {
@@ -589,34 +586,27 @@ slice_richcompare(PyObject *v, PyObject *w, int op)
         return res;
     }
 
-    t1 = PyTuple_New(3);
-    if (t1 == NULL)
+
+    PyObject *t1 = PyTuple_Pack(3,
+                                ((PySliceObject *)v)->start,
+                                ((PySliceObject *)v)->stop,
+                                ((PySliceObject *)v)->step);
+    if (t1 == NULL) {
         return NULL;
-    t2 = PyTuple_New(3);
+    }
+
+    PyObject *t2 = PyTuple_Pack(3,
+                                ((PySliceObject *)w)->start,
+                                ((PySliceObject *)w)->stop,
+                                ((PySliceObject *)w)->step);
     if (t2 == NULL) {
         Py_DECREF(t1);
         return NULL;
     }
 
-    PyTuple_SET_ITEM(t1, 0, ((PySliceObject *)v)->start);
-    PyTuple_SET_ITEM(t1, 1, ((PySliceObject *)v)->stop);
-    PyTuple_SET_ITEM(t1, 2, ((PySliceObject *)v)->step);
-    PyTuple_SET_ITEM(t2, 0, ((PySliceObject *)w)->start);
-    PyTuple_SET_ITEM(t2, 1, ((PySliceObject *)w)->stop);
-    PyTuple_SET_ITEM(t2, 2, ((PySliceObject *)w)->step);
-
-    res = PyObject_RichCompare(t1, t2, op);
-
-    PyTuple_SET_ITEM(t1, 0, NULL);
-    PyTuple_SET_ITEM(t1, 1, NULL);
-    PyTuple_SET_ITEM(t1, 2, NULL);
-    PyTuple_SET_ITEM(t2, 0, NULL);
-    PyTuple_SET_ITEM(t2, 1, NULL);
-    PyTuple_SET_ITEM(t2, 2, NULL);
-
+    PyObject *res = PyObject_RichCompare(t1, t2, op);
     Py_DECREF(t1);
     Py_DECREF(t2);
-
     return res;
 }
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		Fix a crash in slice_richcompare(): use strong references rather than stolen
Copy link Copy Markdown Member methane Feb 13, 2019 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. s/stolen/borrowed/ Copy link Copy Markdown Member Author vstinner Feb 13, 2019 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. Well, @pablogsal and @tim-one said the opposite... I will stay with stolen :-) Copy link Copy Markdown Member methane Feb 13, 2019 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. Oh, I'm sorry about it. I'm concur with you (#11830 (comment)). "Owner" has responsibility to "DECREF". In this case, tuples don't DECREF range members. But it is not important enough to continue discussion. Copy link Copy Markdown Member tim-one Feb 13, 2019 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. @methane , please read the docs I already linked to. After `PyTuple_SET_ITEM()`, it is the tuple's responsibility to decref the item it grabbed. And it would indeed do so, if it were not for the bizarre-looking code at the end to force all the items to `NULL` without touching their refcounts.. As the docs say, the tuple stole the references from the slice object, and at that point - which is the point at which gc blows up - it was the slice object that was using "borrowed" references. As the docs say, `PyTuple_SET_ITEM()` transfers ownership. Which is the fundamental bug here: two objects "believe" they own a reference, but the refcount only accounts for one of them. It's more-or-less arbitrary to pick which one "really" owns it and which "really" borrows it, but the docs are 100% clear about that `PyTuple_SET_ITEM()` made the tuple the owner at the time gc blows up. Copy link Copy Markdown Member methane Feb 14, 2019 • edited Loading Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. See https://docs.python.org/3/c-api/intro.html#reference-count-details "“Owning a reference” means being responsible for calling Py_DECREF on it when the reference is no longer needed." `PyTuple_SET_ITEM()` is defined as stole reference because the reference will be `Py_DECREF`ed by tuple. But in this case, tuple never call `Py_DECREF` for the reference. It will be overwritten soon. Tuple "believes" it stoled (owns) the reference, but it is "borrowed" reference actually. If tuple really stoled the reference, there were no problem. Copy link Copy Markdown Member methane Feb 14, 2019 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. In other words, "Use strong reference rather than stolen reference" doesn't make sense, because "stolen reference" is "strong reference". It was "borrowed reference which tuple believed stolen (strong) reference".
		references for the two temporary internal tuples.