This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: INCREF/DECREFs around the rich comparison needs tests
Type: Stage: resolved
Components: Interpreter Core Versions:
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: corona10, rhettinger, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2020-05-03 21:41 by rhettinger, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 19906 merged corona10, 2020-05-04 16:42
Messages (7)
msg367997 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2020-05-03 21:41
In Objects/dictobject.c, if I remove the INCREF/DECREF pair around PyObject_RichCompareBool(), all the tests still pass:

-           Py_INCREF(startkey);
            cmp = PyObject_RichCompareBool(startkey, key, Py_EQ);
-           Py_DECREF(startkey);

It would be nice to have tests demonstrating why it is necessary.  IIRC the reason is that an object could have a custom comparison that clears the enclosing container and frees the element during the comparison.  

Also, it would be nice if PyObject_RichCompareBool() could be redesigned to not fail if a borrowed reference gets deallocated while the function is running.  The incref and decref occur on a hot path, and it is unfortunate that they cause extra writes to objects scattered all over memory, likely causing a lot of cache misses in real programs.
msg368033 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-05-04 10:54
See issue1517.
msg368049 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2020-05-04 13:52
I can crash python interpreter with few lines of code when if we remove those codes.



class S(str):

    def __eq__(self, other):
        d.clear()
        return NotImplemented

    def __hash__(self):
        return hash('test')

d = {S(): 'value'}
'test' in d
msg368050 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2020-05-04 13:53
Would you like to add a test for this case?
msg368052 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2020-05-04 14:00
> Would you like to add a test for this case?

Oh, I mean, is it good to add a test for this case?
msg368072 - (view) Author: Dong-hee Na (corona10) * (Python committer) Date: 2020-05-04 17:30
New changeset 785f5e6d674306052bf865677d885c30561985ae by Dong-hee Na in branch 'master':
bpo-40489: Add test case for dict contain use after free (GH-19906)
https://github.com/python/cpython/commit/785f5e6d674306052bf865677d885c30561985ae
msg368107 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2020-05-05 06:08
Thank you.
History
Date User Action Args
2022-04-11 14:59:30adminsetgithub: 84669
2020-05-05 06:08:07rhettingersetstatus: open -> closed
resolution: fixed
messages: + msg368107

stage: patch review -> resolved
2020-05-04 17:30:49corona10setmessages: + msg368072
2020-05-04 16:42:52corona10setkeywords: + patch
stage: patch review
pull_requests: + pull_request19221
2020-05-04 14:00:53corona10setmessages: + msg368052
2020-05-04 13:53:47corona10setmessages: + msg368050
2020-05-04 13:52:43corona10setnosy: + corona10
messages: + msg368049
2020-05-04 10:54:50serhiy.storchakasetmessages: + msg368033
2020-05-03 21:41:29rhettingercreate