bpo-27945: Fixed various segfaults with dict. #1657
Conversation
Based on patches by Duane Griffin and Tim Mitchell.
serhiy-storchaka
added
type-bug
|
@serhiy-storchaka, thanks for your PR! By analyzing the history of the files in this pull request, we identified @methane, @benjaminp and @loewis to be potential reviewers. |
Lib/test/test_dict.py
Outdated
| d = {} # this is required to exist so that d can be constructed! | ||
| d = {X(1): 1, X(2): 2} | ||
| with self.assertRaises(RuntimeError): | ||
| dict.fromkeys(d) |
There was a problem hiding this comment.
These, test_fromkeys_operator_modifying_dict_operand and test_fromkeys_operator_modifying_dict_operand, are the same, both testing dicts.
Based on patches by Duane Griffin and Tim Mitchell.. (cherry picked from commit 753bca3)
Based on patches by Duane Griffin and Tim Mitchell.. (cherry picked from commit 753bca3)
| Py_INCREF(key); | ||
| Py_INCREF(value); | ||
| PyTuple_SET_ITEM(result, 0, key); /* steals reference */ | ||
| PyTuple_SET_ITEM(result, 1, value); /* steals reference */ |
There was a problem hiding this comment.
@serhiy-storchaka , just my curious, simply moving the two Py_INCREF forward is also enough here right? So any advantage here or just personal style?
There was a problem hiding this comment.
Hmm... You are right! Py_INCREF(result) prevents reentering in that branch or modifying the tuple by other code. Shame on me that I missed such simple solution!
There was a problem hiding this comment.
But on other hand, Py_DECREF() can trigger the garbage collecting. The garbage collector can see the tuple with references to freed objects and expose it to user code via gc.get_objects() or gc.get_referrers(). Doing some operations with this tuple (repr() or hash()) can cause a crash. We already encountered with similar issues.
So yes, there are reasons for writing the code in that way.
There was a problem hiding this comment.
Ahh, nice point! Stupid me! I didn't recognise it's just like Py_SETREF(PyTuple_GET_ITEM(result, 0), key). But it seems to mean that in your case, users are possible to see a mutable tuple, which smells no good.
There was a problem hiding this comment.
At that point there is only one owner of the tuple. From user's point of view there is no difference between mutating this tuple or deallocating it and allocating a new tuple at the same memory address.
Based on patches by Duane Griffin and Tim Mitchell.. (cherry picked from commit 753bca3)
) (pythonGH-1678) Based on patches by Duane Griffin and Tim Mitchell. (cherry picked from commit 753bca3). (cherry picked from commit 2f7f533)
) (pythonGH-1678) Based on patches by Duane Griffin and Tim Mitchell. (cherry picked from commit 753bca3). (cherry picked from commit 2f7f533)
Based on patches by Duane Griffin and Tim Mitchell.