Thread: BUG #15519: Casting float4 into int4 gets the wrong sign instead of"integer out of range" error

The following bug has been logged on the website:

Bug reference:      15519
Logged by:          Victor Petrovykh
Email address:      victor@magic.io
PostgreSQL version: 10.5
Operating system:   Gentoo Linux 4.14.20
Description:

Offending examples:
SELECT ((2147483647::float4) - 1.0::float4)::int4;
SELECT ((2147483590::float4) - 1.0::float4)::int4;
SELECT ((2147483647::float4) + 1.0::float4)::int4;

They all produce the same result: -2147483648

I understand that a float4 cannot represent large integers with the same
precision as int4, that's OK. What surprised me is that instead of getting
an "overflow error" or "integer out of range" I simply got a negative result
for a value that is actually close to maximum int4. To contrast this, the
query:
SELECT ((2147483647::float4) + 200.0::float4)::int4;
The above produces the expected "ERROR:  integer out of range"

>>>>> "PG" == PG Bug reporting form <noreply@postgresql.org> writes:

 PG> The following bug has been logged on the website:
 PG> Bug reference:      15519
 PG> Logged by:          Victor Petrovykh
 PG> Email address:      victor@magic.io
 PG> PostgreSQL version: 10.5
 PG> Operating system:   Gentoo Linux 4.14.20
 PG> Description:        

 PG> Offending examples:
 PG> SELECT ((2147483647::float4) - 1.0::float4)::int4;
 PG> SELECT ((2147483590::float4) - 1.0::float4)::int4;
 PG> SELECT ((2147483647::float4) + 1.0::float4)::int4;

 PG> They all produce the same result: -2147483648

The bug here is that the ftoi4 conversion function thinks it can do
this:

    if (num < INT_MIN || num > INT_MAX || isnan(num))

but this causes INT_MIN and INT_MAX to be implicitly converted to floats
(not doubles), which can't represent their values exactly and ends up
with the conditions being false in the edge cases. (Specifically,
(float)INT_MAX is a bit larger than INT_MAX.)

Probably this should be changed to use ((float8) INT_MAX) etc. instead?

 PG> I understand that a float4 cannot represent large integers with the
 PG> same precision as int4, that's OK. What surprised me is that
 PG> instead of getting an "overflow error" or "integer out of range" I
 PG> simply got a negative result for a value that is actually close to
 PG> maximum int4. To contrast this, the query:

 PG> SELECT ((2147483647::float4) + 200.0::float4)::int4;
 PG> The above produces the expected "ERROR:  integer out of range"

Likewise you also get that error if you cast the float4 to a float8
before casting to integer.

-- 
Andrew (irc:RhodiumToad)

=?utf-8?q?PG_Bug_reporting_form?= <noreply@postgresql.org> writes:
> Offending examples:
> SELECT ((2147483647::float4) - 1.0::float4)::int4;
> SELECT ((2147483590::float4) - 1.0::float4)::int4;
> SELECT ((2147483647::float4) + 1.0::float4)::int4;

> They all produce the same result: -2147483648

Huh, interesting.  The code underlying this looks sane enough
at first glance:

    float4        num = PG_GETARG_FLOAT4(0);

    if (num < INT_MIN || num > INT_MAX || isnan(num))
        ereport(ERROR,
                (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
                 errmsg("integer out of range")));

    PG_RETURN_INT32((int32) rint(num));

I think what is happening is that the compiler is interpreting
"num > INT_MAX" as something to be done in float4 arithmetic,
so it computes (float4) INT_MAX which rounds off to be the
equivalent of exactly 2147483648.  Then the problematic input
values, which all also round to 2147483648, get past the if-test
and result in undetected overflow in the cast to int32.

Perhaps we could fix this by writing

    if (num < (float8) INT_MIN || num > (float8) INT_MAX || isnan(num))

but I don't have a huge amount of confidence in that either, especially
not for the similar coding in ftoi8 and dtoi8, where the comparison values
are large enough that they'd not be exactly represented by float8 either.
Maybe we need an explicit check for the integer result being of the wrong
sign, to catch just-barely-overflowing cases like these.

            regards, tom lane

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "PG" == PG Bug reporting form <noreply@postgresql.org> writes:
>  PG> Offending examples:
>  PG> SELECT ((2147483647::float4) - 1.0::float4)::int4;
>  PG> SELECT ((2147483590::float4) - 1.0::float4)::int4;
>  PG> SELECT ((2147483647::float4) + 1.0::float4)::int4;
>  PG> They all produce the same result: -2147483648

> The bug here is that the ftoi4 conversion function thinks it can do
> this:
>     if (num < INT_MIN || num > INT_MAX || isnan(num))
> Probably this should be changed to use ((float8) INT_MAX) etc. instead?

After thinking about this for awhile, there's another class of undesirable
behavior here, which can be illustrated by

regression=# select '32766.1'::float4::int2;
 int2
-------
 32766
(1 row)

regression=# select '32767.1'::float4;
 float4
---------
 32767.1
(1 row)

regression=# select '32767.1'::float4::int2;
ERROR:  smallint out of range

It doesn't seem very nice that that fails rather than producing 32767.

I think we could fix that by doing the rint() first, before the
comparisons, which should be safe enough: as the Linux man page for it
points out, it really can't ever fail.

I'm inclined to write ftoi4 as

    /*
     * Get rid of any fractional part in the input.  This is so we don't
     * fail on just-out-of-range values that would round into range.
     */
    num = rint(num);

    /*
     * Range check.  We must be careful here that the boundary values are
     * expressed exactly in the appropriate float domain; we assume float4
     * is going to round off INT_MAX to a power of 2.  Also note assumption
     * that rint() will pass through a NaN or Inf unchanged.
     */
     if (unlikely(num < (float4) INT_MIN || num >= (float4) INT_MAX || isnan(num)))
        ereport(...);

    PG_RETURN_INT32((int32) num);

We could use the same pattern for the other five functions at issue,
but "num >=" would be "num >" in cases where we expect the comparison
value to be represented without roundoff.

Obviously, some regression test cases to verify all these assumptions
would be a good idea.

            regards, tom lane

>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 Tom>     /*
 Tom>      * Range check.  We must be careful here that the boundary values are
 Tom>      * expressed exactly in the appropriate float domain; we assume float4
 Tom>      * is going to round off INT_MAX to a power of 2.  Also note assumption
 Tom>      * that rint() will pass through a NaN or Inf unchanged.
 Tom>      */
 Tom>      if (unlikely(num < (float4) INT_MIN || num >= (float4) INT_MAX || isnan(num)))
 Tom>         ereport(...);

Looking around, another approach I've seen (which I like better) is to
use

    if (num < (float4)INT_MIN || num >= -(float4)INT_MIN || ...

which doesn't rely on assumptions about how the compiler is going to
round INT_MAX. (It does rely on two's complement representation of ints,
but that assumption is known to be safe.)

-- 
Andrew (irc:RhodiumToad)

Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
>  Tom>      if (unlikely(num < (float4) INT_MIN || num >= (float4) INT_MAX || isnan(num)))

>     if (num < (float4)INT_MIN || num >= -(float4)INT_MIN || ...

Meh.  Seems to me that's relying on pretty much the same assumptions
and throwing in an extra dollop of obscurantism on top.

            regards, tom lane

I wrote:
> Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
>> if (num < (float4)INT_MIN || num >= -(float4)INT_MIN || ...

> Meh.  Seems to me that's relying on pretty much the same assumptions
> and throwing in an extra dollop of obscurantism on top.

mmm ... but on the other hand, it means we don't need to write the
test differently depending on whether we think INTxx_MAX will get
rounded.  So that's worth something.

            regards, tom lane

>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:

 Tom> if (unlikely(num < (float4) INT_MIN || num >= (float4) INT_MAX || isnan(num)))

 >> if (num < (float4)INT_MIN || num >= -(float4)INT_MIN || ...

 Tom> Meh. Seems to me that's relying on pretty much the same
 Tom> assumptions

No, because we know that INT_MIN is always exactly representable as a
float (whereas INT_MAX is not), and therefore the cast result will not
depend on any rounding choices whether at compile or run time. Nor does
it depend on knowing that float4 can't represent INT_MAX exactly.

-- 
Andrew (irc:RhodiumToad)

Victor Petrovykh <victor@magic.io> writes:
> Am I missing something in thinking that the cast of a float to int should
> produce an error if the sign of the original value doesn't match the sign
> of the cast result?

Well, yeah, our traditional way of coding this overflow check would
probably have compared the sign of the result to the sign of the input,
but (a) we'd still have needed some ad-hoc range check to avoid getting
fooled by inputs large enough to have wrapped around an even number of
times, and (b) this approach depends on the compiler not taking any
liberties based on an assumption that the program doesn't provoke
integer overflow.  (b) gets more worrisome with each year that goes by,
because the compiler guys keep finding ever-more-creative ways to break
your code if it violates C-spec semantics.  So we really want to write
a test that will fail exactly when the integer coercion would overflow,
not do the coercion and then check to see if it overflowed.

            regards, tom lane

Victor Petrovykh <victor@magic.io> writes:
> I have one more related question: is a fix for this likely to appear in the
> next Postgres release?

Yes, it was pushed last week.

https://git.postgresql.org/gitweb/?p=postgresql.git&a=commitdiff&h=e473e1f2b

            regards, tom lane