Repository navigation
bpo-27945: Fixed various segfaults with dict. - #1657
serhiy-storchaka merged 5 commits into
Conversation
Sorry, something went wrong.
Based on patches by Duane Griffin and Tim Mitchell.
|
@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. |
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
There was a problem hiding this comment.
Good catch!
Sorry, something went wrong.
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?
Sorry, something went wrong.
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!
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
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.