Horton

Thread

Commits

  1. Fix incorrect return value for pg_size_pretty(bigint)

  1. Fix overflow in pg_size_pretty

    Joseph Koshakow <koshy44@gmail.com> — 2024-07-27T19:18:00Z

    Hi all,

Attached is a patch that resolves an overflow in pg_size_pretty() that
resulted in unexpected behavior when PG_INT64_MIN was passed in as an
argument.

The pg_abs_s64() helper function is extracted and simplified from patch
0001 from [0]. I didn't add similar functions for other sized integers
since they'd be unused, but I'd be happy to add them if others
disagree.

`SELECT -9223372036854775808::bigint` results in an out of range error,
even though `-9223372036854775808` can fit in a `bigint` and
`SELECT pg_typeof(-9223372036854775808)` returns `bigint`. That's why
the `::bigint` cast is omitted from my test.

[0]
https://www.postgresql.org/message-id/flat/CAAvxfHdBPOyEGS7s+xf4iaW0-cgiq25jpYdWBqQqvLtLe_t6tw@mail.gmail.com

Thanks,
Joseph Koshakow

  2. Re: Fix overflow in pg_size_pretty

    Joseph Koshakow <koshy44@gmail.com> — 2024-07-27T21:16:38Z

    On Sat, Jul 27, 2024 at 3:18 PM Joseph Koshakow <koshy44@gmail.com> wrote:
>
> `SELECT -9223372036854775808::bigint` results in an out of range error,
> even though `-9223372036854775808` can fit in a `bigint` and
> `SELECT pg_typeof(-9223372036854775808)` returns `bigint`. That's why
> the `::bigint` cast is omitted from my test.

Turns out it was just an order of operations issue. Fix is attached.

Thanks,
Joseph Koshakow

  3. Re: Fix overflow in pg_size_pretty

    David Rowley <dgrowleyml@gmail.com> — 2024-07-27T22:28:23Z

    On Sun, 28 Jul 2024 at 07:18, Joseph Koshakow <koshy44@gmail.com> wrote:
> Attached is a patch that resolves an overflow in pg_size_pretty() that
> resulted in unexpected behavior when PG_INT64_MIN was passed in as an
> argument.

Could we just fix this more simply by assigning the absolute value of
the signed variable into an unsigned type?  It's a bit less code and
gets rid of the explicit test for PG_INT64_MIN.

David

  4. Re: Fix overflow in pg_size_pretty

    Joseph Koshakow <koshy44@gmail.com> — 2024-07-27T23:06:30Z

    On Sat, Jul 27, 2024 at 6:28 PM David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Sun, 28 Jul 2024 at 07:18, Joseph Koshakow <koshy44@gmail.com> wrote:
>> Attached is a patch that resolves an overflow in pg_size_pretty() that
>> resulted in unexpected behavior when PG_INT64_MIN was passed in as an
>> argument.
>
> Could we just fix this more simply by assigning the absolute value of
> the signed variable into an unsigned type?

I might be misunderstanding, but my previous patch does assign the
absolute value of the signed variable into an unsigned type.

> It's a bit less code and
> gets rid of the explicit test for PG_INT64_MIN.

> + uint64 usize = size < 0 ? (uint64) (-size) : (uint64) size;

I think that the explicit test for PG_INT64_MIN is still required. If
`size` is equal to PG_INT64_MIN then `-size` will overflow. You end up
with the correct behavior if `size` wraps around, but that's only
guaranteed on platforms that support the `-fwrapv` flag.

Thanks,
Joseph Koshakow

  5. Re: Fix overflow in pg_size_pretty

    David Rowley <dgrowleyml@gmail.com> — 2024-07-28T00:00:21Z

    On Sun, 28 Jul 2024 at 11:06, Joseph Koshakow <koshy44@gmail.com> wrote:
> > + uint64 usize = size < 0 ? (uint64) (-size) : (uint64) size;
>
> I think that the explicit test for PG_INT64_MIN is still required. If
> `size` is equal to PG_INT64_MIN then `-size` will overflow. You end up
> with the correct behavior if `size` wraps around, but that's only
> guaranteed on platforms that support the `-fwrapv` flag.

What if we spelt it out the same way as pg_lltoa() does?

i.e: uint64 usize = size < 0 ? 0 - (uint64) size : (uint64) size;

David

  6. Re: Fix overflow in pg_size_pretty

    Joseph Koshakow <koshy44@gmail.com> — 2024-07-28T01:10:23Z

    On Sat, Jul 27, 2024 at 8:00 PM David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Sun, 28 Jul 2024 at 11:06, Joseph Koshakow <koshy44@gmail.com> wrote:
>> > + uint64 usize = size < 0 ? (uint64) (-size) : (uint64) size;
>>
>> I think that the explicit test for PG_INT64_MIN is still required. If
>> `size` is equal to PG_INT64_MIN then `-size` will overflow. You end up
>> with the correct behavior if `size` wraps around, but that's only
>> guaranteed on platforms that support the `-fwrapv` flag.
>
> What if we spelt it out the same way as pg_lltoa() does?
>
> i.e: uint64 usize = size < 0 ? 0 - (uint64) size : (uint64) size;

My understanding of pg_lltoa() is that it produces an underflow and
relies wrapping around from 0 to PG_UINT64_MAX. In fact the following
SQL, which relies on pg_lltoa() under the hood, panics with `-ftrapv`
enabled (which panics on underflows and overflows):

    SELECT int8out(-9223372036854775808);

So we should actually probably modify pg_lltoa() to use pg_abs_s64()
too.

Thanks,
Joe Koshakow

  7. Re: Fix overflow in pg_size_pretty

    David Rowley <dgrowleyml@gmail.com> — 2024-07-28T03:42:36Z

    On Sun, 28 Jul 2024 at 13:10, Joseph Koshakow <koshy44@gmail.com> wrote:
>
> On Sat, Jul 27, 2024 at 8:00 PM David Rowley <dgrowleyml@gmail.com> wrote:
> > What if we spelt it out the same way as pg_lltoa() does?
> >
> > i.e: uint64 usize = size < 0 ? 0 - (uint64) size : (uint64) size;
>
> My understanding of pg_lltoa() is that it produces an underflow and
> relies wrapping around from 0 to PG_UINT64_MAX. In fact the following
> SQL, which relies on pg_lltoa() under the hood, panics with `-ftrapv`
> enabled (which panics on underflows and overflows):
>
>     SELECT int8out(-9223372036854775808);

I didn't test to see where that's coming from, but I did test the two
attached .c files.  int.c uses the 0 - (unsigned int) var method and
int2.c uses (unsigned int) (-var).  Using clang and -ftrapv, I get:

$ clang int.c -o int -O2 -ftrapv
$ ./int
2147483648
$ clang int2.c -o int2 -O2 -ftrapv
$ ./int2
Illegal instruction

Similar with gcc:
$ gcc int.c -o int -O2 -ftrapv
$ ./int
2147483648
$ gcc int2.c -o int2 -O2 -ftrapv
$ ./int2
Aborted

I suspect your trap must be coming from somewhere else. It looks to me
like the "uint64 usize = size < 0 ? 0 - (uint64) size : (uint64)
size;" will be fine.

David

  8. Re: Fix overflow in pg_size_pretty

    Joseph Koshakow <koshy44@gmail.com> — 2024-07-28T04:30:10Z

    On Sat, Jul 27, 2024 at 11:42 PM David Rowley <dgrowleyml@gmail.com> wrote:
>
> I didn't test to see where that's coming from, but I did test the two
> attached .c files.  int.c uses the 0 - (unsigned int) var method and
> int2.c uses (unsigned int) (-var).  Using clang and -ftrapv, I get:
>
> $ clang int.c -o int -O2 -ftrapv
> $ ./int
> 2147483648
> $ clang int2.c -o int2 -O2 -ftrapv
> $ ./int2
> Illegal instruction
>
> Similar with gcc:
> $ gcc int.c -o int -O2 -ftrapv
> $ ./int
> 2147483648
> $ gcc int2.c -o int2 -O2 -ftrapv
> $ ./int2
> Aborted
>
> I suspect your trap must be coming from somewhere else. It looks to me
> like the "uint64 usize = size < 0 ? 0 - (uint64) size : (uint64)
> size;" will be fine.

My mistake, you're absolutely right. The trap is coming from
`pg_strtoint64_safe()`.

    return -((int64) tmp);

Which I had already addressed in the other thread and completely forgot
about.

I did some more research and it looks like unsigned integer arithmetic
is guaranteed to wrap around, unlike signed integer arithmetic [0].

Attached is an updated patch with your approach. I removed the 0 from
the negative case because I think it was unnecessary, but happy to add
it back in if I missed something.

Thanks for the review!

Thanks,
Joseph Koshakow

[0]
https://www.gnu.org/software/autoconf/manual/autoconf-2.63/html_node/Integer-Overflow-Basics.html

  9. Re: Fix overflow in pg_size_pretty

    David Rowley <dgrowleyml@gmail.com> — 2024-07-28T10:34:49Z

    On Sun, 28 Jul 2024 at 16:30, Joseph Koshakow <koshy44@gmail.com> wrote:
> Attached is an updated patch with your approach. I removed the 0 from
> the negative case because I think it was unnecessary, but happy to add
> it back in if I missed something.

I made a few adjustments and pushed this.  I did keep the 0 - part as
some compilers don't seem to like not having the 0.  e.g MSVC gives:

../src/backend/utils/adt/dbsize.c(578): warning C4146: unary minus
operator applied to unsigned type, result still unsigned

I thought a bit about if it was really worth keeping the regression
test or not and in the end decided it was likely worthwhile keeping
it, so I expanded it slightly to cover both PG_INT64_MIN and
PG_INT64_MAX values. It looks slightly less like we're earmarking the
fact that there was a bug that way, and also seems to be of some
additional value.

PG15 did see quite a significant rewrite of the pg_size_pretty code.
The bug does still exist in PG14 and earlier, but on looking at what
it would take to fix it there I got a bit unexcited at the risk to
reward ratio of adjusting that code and just left it alone. I've
backpatched only as far as PG15. I'm sure someone else will feel I
should have done something else there, but that's the judgement call I
made.

Thanks for the patch.

David