[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
Attachments:
tuplesort_avoid_possible_overflow.patchapplication/octet-stream; name=tuplesort_avoid_possible_overflow.patchDownload
diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index de38c6c7e0..9f29133b4f 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -1579,7 +1579,7 @@ grow_memtuples(Tuplesortstate *state)
* both old and new arrays as separate chunks. But we'll check LACKMEM
* explicitly below just in case.)
*/
- if (state->availMem < (int64) ((newmemtupsize - memtupsize) * sizeof(SortTuple)))
+ if (state->availMem < ((int64)(newmemtupsize - memtupsize) * sizeof(SortTuple)))
goto noalloc;
/* OK, do it */
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
<https://en.cppreference.com/w/cpp/types/size_t>.
regards,
Ranier Vilela