On Sat, Jul 27, 2024 at 11:42 PM David Rowley <dgrowle...@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
From 1811b94ba4c08a0de972e8ded4892cf294e9f687 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <kosh...@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
