Thread: Negative value of numGroups

Hi,

Using sqlancer I've found curious vulnerability. In some places of the 
code we convert LONG_MAX to double. After value of 2^53 double doesn't 
correspond to a long value precisely. So, LONG_MAX can be converted into 
'LONG_MAX + 1' double value. And string:

(long) Min(numGroups, (double) LONG_MAX);

can return negative value, if numGroups > LONG_MAX.
Maybe it isn't practical issue right now, but this annoying thing 
doesn't allow to pass sqlancer tests sometimes, for example, with options:

--extensions='pg_stat_statements' --oracle HAVING

It is not difficult to fix the problem in-place, of course. But maybe do 
better: change the type of numGroups field in any Plan nodes to double 
and convert it into specific type right before usage?

-- 
Regards
Andrey Lepikhov
Postgres Professional

On 13/5/2022 11:56, Andrey Lepikhov wrote:
> (long) Min(numGroups, (double) LONG_MAX);
> 
> can return negative value, if numGroups > LONG_MAX.
So, because this problem still breaks sqlancer tests I prepared a patch 
(with regression test) that fixes it.

-- 
regards,
Andrey Lepikhov
Postgres Professional

Andrey Lepikhov <a.lepikhov@postgrespro.ru> writes:
> On 13/5/2022 11:56, Andrey Lepikhov wrote:
>> (long) Min(numGroups, (double) LONG_MAX);
>> 
>> can return negative value, if numGroups > LONG_MAX.

I see your point, but I don't think that repeating the same finicky
and undocumented coding pattern in multiple places is a future-proof
fix.  I'm inclined to think we should invent a function along the
lines of "long clamp_double_to_long(double x)".  Given where it's
used, maybe putting it beside clamp_row_est() would be good.

            regards, tom lane

On 5/17/22 20:34, Tom Lane wrote:
> Andrey Lepikhov <a.lepikhov@postgrespro.ru> writes:
>> On 13/5/2022 11:56, Andrey Lepikhov wrote:
>>> (long) Min(numGroups, (double) LONG_MAX);
>>>
>>> can return negative value, if numGroups > LONG_MAX.
> 
> I see your point, but I don't think that repeating the same finicky
> and undocumented coding pattern in multiple places is a future-proof
> fix.  I'm inclined to think we should invent a function along the
> lines of "long clamp_double_to_long(double x)".  Given where it's
> used, maybe putting it beside clamp_row_est() would be good.
> 
>             regards, tom lane
Next version of the patch.
As I see, now we trying to use Cardinality type instead of double.
So, I named casting routine as clamp_cardinality_to_long.

-- 
Regards
Andrey Lepikhov
Postgres Professional

Andrey Lepikhov <a.lepikhov@postgrespro.ru> writes:
> Next version of the patch.
> As I see, now we trying to use Cardinality type instead of double.
> So, I named casting routine as clamp_cardinality_to_long.

Pushed with a bit of rewriting of comments etc.  I left out the
test case too, because (a) for me, it did not reveal any difference
between patched and unpatched code, and (b) even if it had, it was
far too expensive to put into the core regression tests.

            regards, tom lane