Correct overflow check in PyTuple_New()#14838
Conversation
gnprice
left a comment
There was a problem hiding this comment.
Nice catch! I am curious how you discovered this. 🙂
From the linked commit, I definitely agree that a sign seems to have gone wrong in the algebra there, and this fixes the logic to work like it did before.
Unpeeling a couple of layers to see the allocation, I also agree that this is a correct fix! So I'm in favor of merge.
|
I feel like there's an opportunity for a more complete fix as a followup to this. My next question on reading this is, OK, how can we write this so it's hard to get wrong? That commit you linked to, with the algebra mistake, is a few weeks shy of 11 years old. 😉 I think the point is that So, a sort of minimal fix would be to make something with a name like I think that immediately raises a next question, though, which is: are other call sites of And: why should they have to? It seems like the best place for this check is immediately before the possibly-overflowing computation; or if not there, then the closer to it the better. So ideally There are only a handful of call sites of |
|
Let's merge this fix, and move the discussion on a better way to express this to a bug. (Please open that bug if you still want to have that discussion.) |
|
Thanks @Yhg1s for merging this (and @sir-sigurd for the PR)! Filed bpo-38079 for the followup I suggested. |
Currently this overflow check overestimates tuple size. It's introduced in this form in 3ce5d92#diff-cde4fb3109c243b2c2badb10eeab7fcdR63.