Re: [PATCH] Fix possible overflow on tuplesort.c

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
>> When multiplying variables, the overflow will take place anyway, and only
>> then will the meaningless product be explicitly promoted to type int64.
>> It is one of the operands that should have been cast instead to avoid the
>> overflow.
>>
>> - if (state->availMem < (int64) ((newmemtupsize - memtupsize) * sizeof(SortTuple)))
>> + if (state->availMem < ((int64) (newmemtupsize - memtupsize) * sizeof(SortTuple)))

> Doesn't sizeof() return a 64-bit wide value already?

Not on 32-bit machines. However, on a 32-bit machine the clamp just
above here would prevent overflow anyway. In general, said clamp
ensures that the value computed here is less than MaxAllocHugeSize,
so computing it in size_t width is enough. So in fact an overflow is
impossible here, but it requires looking at more than this one line of
code to see it. I would expect a static analyzer to understand it though.

I think the actual point of this cast is to ensure that the comparison to
availMem is done in signed not unsigned arithmetic --- which is critical
because availMem might be negative. The proposed change would indeed
break that, since multiplying a signed value by size_t is presumably going
to produce an unsigned value. We could use two casts, but I don't see the
point.

regards, tom lane