bpo-35961: Fix a crash in slice_richcompare() #11830
Conversation
Fix a crash in slice_richcompare(): use strong references rather than borrowed references for the two temporary internal tuples. The crash (or assertion error) occurred if a garbage collection occurred during slice_richcompare(), especially while calling PyObject_RichCompare(t1, t2, op).
Misc/NEWS.d/next/Core and Builtins/2019-02-12-20-16-34.bpo-35961.7f7Sne.rst
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,2 @@ | |||
| Fix a crash in slice_richcompare(): use strong references rather than stolen | |||
There was a problem hiding this comment.
Well, @pablogsal and @tim-one said the opposite... I will stay with stolen :-)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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_DECREFed 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.
There was a problem hiding this comment.
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".
|
I like this too. |
|
AppVeyor failed because of a random failure (ENV_CHANGED): test_multiprocessing_spawn and test_threading. I rescheduled the job. |
|
|
Thanks @vstinner for the PR |
|
GH-11839 is a backport of this pull request to the 3.7 branch. |
Fix a crash in slice_richcompare(): use strong references rather than stolen references for the two temporary internal tuples. The crash (or assertion error) occurred if a garbage collection occurred during slice_richcompare(), especially while calling PyObject_RichCompare(t1, t2, op). (cherry picked from commit dcb68f4) Co-authored-by: Victor Stinner <vstinner@redhat.com>
Fix a crash in slice_richcompare(): use strong references rather than stolen references for the two temporary internal tuples. The crash (or assertion error) occurred if a garbage collection occurred during slice_richcompare(), especially while calling PyObject_RichCompare(t1, t2, op). (cherry picked from commit dcb68f4) Co-authored-by: Victor Stinner <vstinner@redhat.com>
Fix a crash in slice_richcompare(): use strong references rather than
borrowed references for the two temporary internal tuples.
The crash (or assertion error) occurred if a garbage collection
occurred during slice_richcompare(), especially while calling
PyObject_RichCompare(t1, t2, op).
https://bugs.python.org/issue35961