Correct overflow check in PyTuple_New()

[sir-sigurd]

Currently this overflow check overestimates tuple size. It's introduced in this form in 3ce5d92#diff-cde4fb3109c243b2c2badb10eeab7fcdR63.

[gnprice]

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.

[gnprice]

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 PyObject_GC_NewVar really has a precondition, that the nitems it's passed (size in this caller) not be so big that the allocation will overflow. Specifically _PyObject_VAR_SIZE(tp, nitems) needs to not overflow. All the trickiness in this overflow check is basically an algebraic rearrangement of what _PyObject_VAR_SIZE(&PyTuple_Type, size) would say, plus substituting in the values actually found at &PyTuple_Type.

So, a sort of minimal fix would be to make something with a name like PyObject_GC_NewVar_WouldOverflow that encodes that. Maybe that in turn would just be a macro to delegate to a _PyObject_VAR_SIZE_WOULD_OVERFLOW, so that each layer only has to know what it actually knows.

I think that immediately raises a next question, though, which is: are other call sites of PyObject_GC_NewVar checking properly for overflow? At a quick grep, I see some that don't appear to be checking.

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 _PyObject_VAR_SIZE itself would be augmented to first do this check, and only if it won't overflow then go and actually multiply.

There are only a handful of call sites of _PyObject_VAR_SIZE, so it wouldn't even be much of a migration to then take care of each of them.

[Yhg1s]

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.)

[gnprice]

Thanks @Yhg1s for merging this (and @sir-sigurd for the PR)!

Filed bpo-38079 for the followup I suggested.