Re: Remove dependence on integer wrapping

On Fri, Jun 14, 2024 at 8:00 AM Alexander Lakhin <exclusion(at)gmail(dot)com>
wrote:
>
> And the most interesting case to me:
> SET temp_buffers TO 1000000000;
>
> CREATE TEMP TABLE t(i int PRIMARY KEY);
> INSERT INTO t VALUES(1);
>
> #4 0x00007f385cdd37f3 in __GI_abort () at ./stdlib/abort.c:79
> #5 0x00005620071c4f51 in __addvsi3 ()
> #6 0x0000562007143f3c in init_htab (hashp=0x562008facb20,
nelem=610070812) at dynahash.c:720
>
> (gdb) f 6
> #6 0x0000560915207f3c in init_htab (hashp=0x560916039930,
nelem=1000000000) at dynahash.c:720
> 720 hctl->high_mask = (nbuckets << 1) - 1;
> (gdb) p nbuckets
> $1 = 1073741824

Alex, are you able to get a full stack trace for this panic? I'm unable
to reproduce this because I don't have enough memory in my system. I've
tried reducing `BLCKSZ` to 1024, which is the lowest value allowed per
my understanding, and I still don't have enough memory.

Here's what it looks like is happening:

1. When inserting into the table, we create a new dynamic hash table
and set `nelem` equal to `temp_buffers`, which is 1000000000.

2. `nbuckets` is then set to the the next highest power of 2 from
`nelem`, which is 1073741824.

/*
* Allocate space for the next greater power of two number of buckets,
* assuming a desired maximum load factor of 1.
*/
nbuckets = next_pow2_int(nelem);

3. Shift `nbuckets` to the left by 1. This would equal 2147483648,
which is larger than `INT_MAX`, which causes an overflow.

hctl->high_mask = (nbuckets << 1) - 1;

The max value allowed for `temp_buffers` is `INT_MAX / 2` (1073741823),
So any value of `temp_buffers` in the range (536870912, 1073741823]
would cause this overflow. Without `-ftrapv`, `nbuckets` would wrap
around to -2147483648, which is likely to cause all sorts of havoc, I'm
just not sure what exactly.

Also, `nbuckets = next_pow2_int(nelem);`, by itself is a bit sketchy
considering that `nelem` is a `long` and `nbuckets` is an `int`.
Potentially, the fix here is to just convert `nbuckets` to a `long`. I
plan on checking if that's feasible.

I also found this commit [0] that increased the max of `nbuckets` from
`INT_MAX / BLCKSZ` to `INT_MAX / 2`, which introduced the possibility
of this overflow. So I plan on reading through that as well.

Thanks,
Joseph Koshakow

[0]
https://github.com/postgres/postgres/commit/0007490e0964d194a606ba79bb11ae1642da3372