Обсуждение: [PATCH] Fix possible overflow on tuplesort.c
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
Вложения
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
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.
Ranier Vilela