Fix overflow in pg_size_pretty
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]/messages/by-id/CAAvxfHdBPOyEGS7s+xf4iaW0-cgiq25jpYdWBqQqvLtLe_t6tw@mail.gmail.com. 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]: /messages/by-id/CAAvxfHdBPOyEGS7s+xf4iaW0-cgiq25jpYdWBqQqvLtLe_t6tw@mail.gmail.com
/messages/by-id/CAAvxfHdBPOyEGS7s+xf4iaW0-cgiq25jpYdWBqQqvLtLe_t6tw@mail.gmail.com
Thanks,
Joseph Koshakow
Attachments:
v1-0001-Fix-overflow-in-pg_size_pretty.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Fix-overflow-in-pg_size_pretty.patchDownload
From 6ec885412f2e0f3a3e019ec1906901e39c6d517a Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <koshy44@gmail.com>
Date: Sat, 27 Jul 2024 15:06:09 -0400
Subject: [PATCH] Fix overflow in pg_size_pretty
This commit removes an overflow from pg_size_pretty that causes
PG_INT64_MIN to by displayed with the bytes unit instead of the PB
unit.
---
src/backend/utils/adt/dbsize.c | 3 ++-
src/include/common/int.h | 13 +++++++++++++
src/test/regress/expected/dbsize.out | 6 ++++++
src/test/regress/sql/dbsize.sql | 2 ++
4 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c
index 25d7110c13..5648f40101 100644
--- a/src/backend/utils/adt/dbsize.c
+++ b/src/backend/utils/adt/dbsize.c
@@ -21,6 +21,7 @@
#include "catalog/pg_tablespace.h"
#include "commands/dbcommands.h"
#include "commands/tablespace.h"
+#include "common/int.h"
#include "miscadmin.h"
#include "storage/fd.h"
#include "utils/acl.h"
@@ -577,7 +578,7 @@ pg_size_pretty(PG_FUNCTION_ARGS)
uint8 bits;
/* use this unit if there are no more units or we're below the limit */
- if (unit[1].name == NULL || i64abs(size) < unit->limit)
+ if (unit[1].name == NULL || pg_abs_s64(size) < unit->limit)
{
if (unit->round)
size = half_rounded(size);
diff --git a/src/include/common/int.h b/src/include/common/int.h
index 7fc046e78a..d86eb6dd5e 100644
--- a/src/include/common/int.h
+++ b/src/include/common/int.h
@@ -258,6 +258,19 @@ pg_mul_s64_overflow(int64 a, int64 b, int64 *result)
#endif
}
+static inline uint64
+pg_abs_s64(int64 a)
+{
+ if (unlikely(a == PG_INT64_MIN))
+ {
+ return ((uint64) PG_INT64_MAX) + 1;
+ }
+ else
+ {
+ return (uint64) i64abs(a);
+ }
+}
+
/*------------------------------------------------------------------------
* Overflow routines for unsigned integers
*------------------------------------------------------------------------
diff --git a/src/test/regress/expected/dbsize.out b/src/test/regress/expected/dbsize.out
index f1121a87aa..3398a3eceb 100644
--- a/src/test/regress/expected/dbsize.out
+++ b/src/test/regress/expected/dbsize.out
@@ -12,6 +12,12 @@ SELECT size, pg_size_pretty(size), pg_size_pretty(-1 * size) FROM
1000000000000000 | 909 TB | -909 TB
(6 rows)
+SELECT pg_size_pretty(-9223372036854775808);
+ pg_size_pretty
+----------------
+ -8192 PB
+(1 row)
+
SELECT size, pg_size_pretty(size), pg_size_pretty(-1 * size) FROM
(VALUES (10::numeric), (1000::numeric), (1000000::numeric),
(1000000000::numeric), (1000000000000::numeric),
diff --git a/src/test/regress/sql/dbsize.sql b/src/test/regress/sql/dbsize.sql
index b34cf33385..e3e18e948e 100644
--- a/src/test/regress/sql/dbsize.sql
+++ b/src/test/regress/sql/dbsize.sql
@@ -3,6 +3,8 @@ SELECT size, pg_size_pretty(size), pg_size_pretty(-1 * size) FROM
(1000000000::bigint), (1000000000000::bigint),
(1000000000000000::bigint)) x(size);
+SELECT pg_size_pretty(-9223372036854775808);
+
SELECT size, pg_size_pretty(size), pg_size_pretty(-1 * size) FROM
(VALUES (10::numeric), (1000::numeric), (1000000::numeric),
(1000000000::numeric), (1000000000000::numeric),
--
2.34.1
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
Attachments:
v2-0001-Fix-overflow-in-pg_size_pretty.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Fix-overflow-in-pg_size_pretty.patchDownload
From 1224087ab4e13a107b51ac17c77e83dc7db37ef9 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <koshy44@gmail.com>
Date: Sat, 27 Jul 2024 15:06:09 -0400
Subject: [PATCH] Fix overflow in pg_size_pretty
This commit removes an overflow from pg_size_pretty that causes
PG_INT64_MIN to by displayed with the bytes unit instead of the PB
unit.
---
src/backend/utils/adt/dbsize.c | 3 ++-
src/include/common/int.h | 13 +++++++++++++
src/test/regress/expected/dbsize.out | 6 ++++++
src/test/regress/sql/dbsize.sql | 2 ++
4 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c
index 25d7110c13..5648f40101 100644
--- a/src/backend/utils/adt/dbsize.c
+++ b/src/backend/utils/adt/dbsize.c
@@ -21,6 +21,7 @@
#include "catalog/pg_tablespace.h"
#include "commands/dbcommands.h"
#include "commands/tablespace.h"
+#include "common/int.h"
#include "miscadmin.h"
#include "storage/fd.h"
#include "utils/acl.h"
@@ -577,7 +578,7 @@ pg_size_pretty(PG_FUNCTION_ARGS)
uint8 bits;
/* use this unit if there are no more units or we're below the limit */
- if (unit[1].name == NULL || i64abs(size) < unit->limit)
+ if (unit[1].name == NULL || pg_abs_s64(size) < unit->limit)
{
if (unit->round)
size = half_rounded(size);
diff --git a/src/include/common/int.h b/src/include/common/int.h
index 7fc046e78a..d86eb6dd5e 100644
--- a/src/include/common/int.h
+++ b/src/include/common/int.h
@@ -258,6 +258,19 @@ pg_mul_s64_overflow(int64 a, int64 b, int64 *result)
#endif
}
+static inline uint64
+pg_abs_s64(int64 a)
+{
+ if (unlikely(a == PG_INT64_MIN))
+ {
+ return ((uint64) PG_INT64_MAX) + 1;
+ }
+ else
+ {
+ return (uint64) i64abs(a);
+ }
+}
+
/*------------------------------------------------------------------------
* Overflow routines for unsigned integers
*------------------------------------------------------------------------
diff --git a/src/test/regress/expected/dbsize.out b/src/test/regress/expected/dbsize.out
index f1121a87aa..eac878c3ec 100644
--- a/src/test/regress/expected/dbsize.out
+++ b/src/test/regress/expected/dbsize.out
@@ -12,6 +12,12 @@ SELECT size, pg_size_pretty(size), pg_size_pretty(-1 * size) FROM
1000000000000000 | 909 TB | -909 TB
(6 rows)
+SELECT pg_size_pretty((-9223372036854775808)::bigint);
+ pg_size_pretty
+----------------
+ -8192 PB
+(1 row)
+
SELECT size, pg_size_pretty(size), pg_size_pretty(-1 * size) FROM
(VALUES (10::numeric), (1000::numeric), (1000000::numeric),
(1000000000::numeric), (1000000000000::numeric),
diff --git a/src/test/regress/sql/dbsize.sql b/src/test/regress/sql/dbsize.sql
index b34cf33385..e1ad202016 100644
--- a/src/test/regress/sql/dbsize.sql
+++ b/src/test/regress/sql/dbsize.sql
@@ -3,6 +3,8 @@ SELECT size, pg_size_pretty(size), pg_size_pretty(-1 * size) FROM
(1000000000::bigint), (1000000000000::bigint),
(1000000000000000::bigint)) x(size);
+SELECT pg_size_pretty((-9223372036854775808)::bigint);
+
SELECT size, pg_size_pretty(size), pg_size_pretty(-1 * size) FROM
(VALUES (10::numeric), (1000::numeric), (1000000::numeric),
(1000000000::numeric), (1000000000000::numeric),
--
2.34.1
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
Attachments:
pg_size_pretty_bigint_fix.patchapplication/octet-stream; name=pg_size_pretty_bigint_fix.patchDownload
diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c
index 25d7110c13..ef67289d29 100644
--- a/src/backend/utils/adt/dbsize.c
+++ b/src/backend/utils/adt/dbsize.c
@@ -574,10 +574,11 @@ pg_size_pretty(PG_FUNCTION_ARGS)
for (unit = size_pretty_units; unit->name != NULL; unit++)
{
+ uint64 usize = size < 0 ? (uint64) (-size) : (uint64) size;
uint8 bits;
/* use this unit if there are no more units or we're below the limit */
- if (unit[1].name == NULL || i64abs(size) < unit->limit)
+ if (unit[1].name == NULL || usize < unit->limit)
{
if (unit->round)
size = half_rounded(size);
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
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
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
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
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 instructionSimilar with gcc:
$ gcc int.c -o int -O2 -ftrapv
$ ./int
2147483648
$ gcc int2.c -o int2 -O2 -ftrapv
$ ./int2
AbortedI 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]https://www.gnu.org/software/autoconf/manual/autoconf-2.63/html_node/Integer-Overflow-Basics.html.
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
https://www.gnu.org/software/autoconf/manual/autoconf-2.63/html_node/Integer-Overflow-Basics.html
Attachments:
v3-0001-Fix-overflow-in-pg_size_pretty.patchtext/x-patch; charset=US-ASCII; name=v3-0001-Fix-overflow-in-pg_size_pretty.patchDownload
From 1811b94ba4c08a0de972e8ded4892cf294e9f687 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <koshy44@gmail.com>
Date: Sat, 27 Jul 2024 15:06:09 -0400
Subject: [PATCH] Fix overflow in pg_size_pretty
This commit removes an overflow from pg_size_pretty that causes
PG_INT64_MIN to by displayed with the bytes unit instead of the PB
unit.
---
src/backend/utils/adt/dbsize.c | 3 ++-
src/test/regress/expected/dbsize.out | 6 ++++++
src/test/regress/sql/dbsize.sql | 2 ++
3 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c
index 25d7110c13..8d58fc24ce 100644
--- a/src/backend/utils/adt/dbsize.c
+++ b/src/backend/utils/adt/dbsize.c
@@ -575,9 +575,10 @@ pg_size_pretty(PG_FUNCTION_ARGS)
for (unit = size_pretty_units; unit->name != NULL; unit++)
{
uint8 bits;
+ uint64 usize = size < 0 ? -(uint64) size : (uint64) size;
/* use this unit if there are no more units or we're below the limit */
- if (unit[1].name == NULL || i64abs(size) < unit->limit)
+ if (unit[1].name == NULL || usize < unit->limit)
{
if (unit->round)
size = half_rounded(size);
diff --git a/src/test/regress/expected/dbsize.out b/src/test/regress/expected/dbsize.out
index f1121a87aa..eac878c3ec 100644
--- a/src/test/regress/expected/dbsize.out
+++ b/src/test/regress/expected/dbsize.out
@@ -12,6 +12,12 @@ SELECT size, pg_size_pretty(size), pg_size_pretty(-1 * size) FROM
1000000000000000 | 909 TB | -909 TB
(6 rows)
+SELECT pg_size_pretty((-9223372036854775808)::bigint);
+ pg_size_pretty
+----------------
+ -8192 PB
+(1 row)
+
SELECT size, pg_size_pretty(size), pg_size_pretty(-1 * size) FROM
(VALUES (10::numeric), (1000::numeric), (1000000::numeric),
(1000000000::numeric), (1000000000000::numeric),
diff --git a/src/test/regress/sql/dbsize.sql b/src/test/regress/sql/dbsize.sql
index b34cf33385..e1ad202016 100644
--- a/src/test/regress/sql/dbsize.sql
+++ b/src/test/regress/sql/dbsize.sql
@@ -3,6 +3,8 @@ SELECT size, pg_size_pretty(size), pg_size_pretty(-1 * size) FROM
(1000000000::bigint), (1000000000000::bigint),
(1000000000000000::bigint)) x(size);
+SELECT pg_size_pretty((-9223372036854775808)::bigint);
+
SELECT size, pg_size_pretty(size), pg_size_pretty(-1 * size) FROM
(VALUES (10::numeric), (1000::numeric), (1000000::numeric),
(1000000000::numeric), (1000000000000::numeric),
--
2.34.1
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