Обсуждение: [PATCH] Fix possible overflow on tuplesort.c

On 2020-Apr-16, Ranier Vilela wrote:

> 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?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.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