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

Поиск
Список
Период
Сортировка

[PATCH] Fix possible overflow on tuplesort.c

От
Ranier Vilela
Дата:
Hi,

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.

regards,
Ranier Vilela
Вложения

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

От
Alvaro Herrera
Дата:
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



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

От
Tom Lane
Дата:
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



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

От
Ranier Vilela
Дата:
Em qui., 23 de abr. de 2020 às 16:43, Alvaro Herrera <alvherre@2ndquadrant.com> escreveu:
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?
Sizeof return size_t.
Both versions are constant expressions of type std::size_t.
 
regards,
Ranier Vilela