Subject: Re: Remove dependence on integer wrapping

From: Nathan Bossart <[email-masked]>
Date: Fri, 16 Aug 2024 11:52:16 -0500

On Thu, Aug 15, 2024 at 10:49:46PM -0400, Joseph Koshakow wrote:
> This updated version LGTM, I agree it's a good use of pg_abs_s32().

Committed.

-- 
nathan

---

From: Alexander Lakhin <[email-masked]>
Date: Fri, 16 Aug 2024 21:00:00 +0300

Hello Nathan and Joe,

>> This updated version LGTM, I agree it's a good use of pg_abs_s32().

Thank you for working on that issue!

I've tried `make check` with CC=gcc-13 CPPFLAGS="-O0 -ftrapv" and got a
server crash:
2024-08-16 17:14:36.102 UTC postmaster[1982703] LOG:   (PID 1983867) was terminated by signal 6: Aborted
2024-08-16 17:14:36.102 UTC postmaster[1982703] DETAIL:  Failed process was running: select '[]'::jsonb ->> -2147483648;
with the stack trace
...
#5  0x0000556aec224a11 in __negvsi2 ()
#6  0x0000556aec046238 in jsonb_array_element_text (fcinfo=0x556aedd70240) at jsonfuncs.c:993
#7  0x0000556aebc90b68 in ExecInterpExpr (state=0x556aedd70160, econtext=0x556aedd706a0, isnull=0x7ffdf82211e4)
     at execExprInterp.c:765
...
(gdb) f 6
#6  0x0000556aec046238 in jsonb_array_element_text (fcinfo=0x556aedd70240) at jsonfuncs.c:993
993                     if (-element > nelements)
(gdb) p element
$1 = -2147483648

Sp it looks like jsonb_array_element_text() still needs the same
treatment as jsonb_array_element().

Moreover, I tried to use "-ftrapv" on 32-bit Debian and came across
another failure:
select '9223372036854775807'::int8 * 2147483648::int8;
server closed the connection unexpectedly
...
#4  0xb722226a in __GI_abort () at ./stdlib/abort.c:79
#5  0x004cb2e1 in __mulvdi3.cold ()
#6  0x00abe7ab in pg_mul_s64_overflow (a=9223372036854775807, b=2147483648, result=0xbff1da68)
     at ../../../../src/include/common/int.h:264
#7  0x00abfbff in int8mul (fcinfo=0x14d9d04) at int8.c:496
#8  0x00782675 in ExecInterpExpr (state=0x14d9c4c, econtext=0x14da15c, isnull=0xbff1dc3f) at execExprInterp.c:765

Whilst
select '9223372036854775807'::int8 * 2147483647::int8;
emits
ERROR:  bigint out of range

I've also discovered another trap-triggering case for a 64-bit platform:
select 1 union all select 1 union all select 1 union all select 1 union all
select 1 union all select 1 union all select 1 union all select 1 union all
select 1 union all select 1 union all select 1 union all select 1 union all
select 1 union all select 1 union all select 1 union all select 1 union all
select 1 union all select 1 union all select 1 union all select 1 union all
select 1 union all select 1 union all select 1 union all select 1 union all
select 1 union all select 1 union all select 1 union all select 1 union all
select 1 union all select 1 union all select 1;

server closed the connection unexpectedly
...
#5  0x00005576cfb1c9f3 in __negvdi2 ()
#6  0x00005576cf627c68 in bms_singleton_member (a=0x5576d09f7fb0) at bitmapset.c:691
#7  0x00005576cf72be0f in fix_append_rel_relids (root=0x5576d09df198, varno=31, subrelids=0x5576d09f7fb0)
     at prepjointree.c:3830
#8  0x00005576cf7278c2 in pull_up_simple_subquery (root=0x5576d09df198, jtnode=0x5576d09f7470, rte=0x5576d09de300,
     lowest_outer_join=0x0, containing_appendrel=0x5576d09f7368) at prepjointree.c:1277
...
(gdb) f 6
#6  0x00005576cf627c68 in bms_singleton_member (a=0x5576d09f7fb0) at bitmapset.c:691
691                             if (result >= 0 || HAS_MULTIPLE_ONES(w))
(gdb) p/x w
$1 = 0x8000000000000000

Best regards,
Alexander

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 16 Aug 2024 13:35:01 -0500

> treatment as jsonb_array_element().

D'oh.  I added a test for that but didn't actually fix the code.  I think
we just need something like the following.

diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index 1f8ea51e6a..69cdd84393 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -990,7 +990,7 @@ jsonb_array_element_text(PG_FUNCTION_ARGS)
     {
         uint32      nelements = JB_ROOT_COUNT(jb);

-        if (-element > nelements)
+        if (pg_abs_s32(element) > nelements)
             PG_RETURN_NULL();
         else
             element += nelements;

> #8  0x00782675 in ExecInterpExpr (state=0x14d9c4c, econtext=0x14da15c, isnull=0xbff1dc3f) at execExprInterp.c:765

Hm.  It looks like that is pointing to __builtin_mul_overflow(), which
seems strange.

> 691                             if (result >= 0 || HAS_MULTIPLE_ONES(w))

At a glance, this appears to be caused by the RIGHTMOST_ONE macro:

	#define RIGHTMOST_ONE(x) ((signedbitmapword) (x) & -((signedbitmapword) (x)))

-- 
nathan

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 16 Aug 2024 13:56:05 -0500

> 	#define RIGHTMOST_ONE(x) ((signedbitmapword) (x) & -((signedbitmapword) (x)))

I believe hand-rolling the two's complement calculation should be
sufficient to avoid depending on -fwrapv here.  godbolt.org indicates that
it produces roughly the same code, too.

diff --git a/src/backend/nodes/bitmapset.c b/src/backend/nodes/bitmapset.c
index cd05c642b0..d37a997c0e 100644
--- a/src/backend/nodes/bitmapset.c
+++ b/src/backend/nodes/bitmapset.c
@@ -67,7 +67,7 @@
  * we get zero.
  *----------
  */
-#define RIGHTMOST_ONE(x) ((signedbitmapword) (x) & -((signedbitmapword) (x)))
+#define RIGHTMOST_ONE(x) ((bitmapword) (x) & (~((bitmapword) (x)) + 1))

 #define HAS_MULTIPLE_ONES(x)    ((bitmapword) RIGHTMOST_ONE(x) != (x))

-- 
nathan

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 16 Aug 2024 15:11:52 -0500

> +        if (pg_abs_s32(element) > nelements)

I've committed this one.

-- 
nathan

---

From: Joseph Koshakow <[email-masked]>
Date: Sat, 17 Aug 2024 15:16:01 -0400

Hi,

I wanted to take this opportunity to provide a brief summary of
outstanding work.

> Also there are several trap-producing cases with date types:
> SELECT to_date('100000000', 'CC');
> SELECT to_timestamp('1000000000,999', 'Y,YYY');
> SELECT make_date(-2147483648, 1, 1);

This is resolved with Matthew's patches, which I've rebased, squashed
and attached to this email. They still require a review.

----

> SET temp_buffers TO 1000000000;
>
> CREATE TEMP TABLE t(i int PRIMARY KEY);
> INSERT INTO t VALUES(1);
>
> #4  0x00007f385cdd37f3 in __GI_abort () at ./stdlib/abort.c:79
> #5  0x00005620071c4f51 in __addvsi3 ()
> #6  0x0000562007143f3c in init_htab (hashp=0x562008facb20,
nelem=610070812) at dynahash.c:720
>
> (gdb) f 6
> #6  0x0000560915207f3c in init_htab (hashp=0x560916039930,
nelem=1000000000) at dynahash.c:720
> 720             hctl->high_mask = (nbuckets << 1) - 1;
> (gdb) p nbuckets
> $1 = 1073741824

I've taken a look at this and my current proposal is to convert
`nbuckets` to 64 bit integer which would prevent the overflow. I'm
hoping to look into if this is feasible soon.

----

> CREATE FUNCTION check_foreign_key () RETURNS trigger AS .../refint.so'
LANGUAGE C;
> CREATE TABLE t (i int4 NOT NULL);
> CREATE TRIGGER check_fkey BEFORE DELETE ON t FOR EACH ROW EXECUTE
PROCEDURE
>    check_foreign_key (2147483647, 'cascade', 'i', "ft", "i");
> INSERT INTO t VALUES (1);
> DELETE FROM t;
>
> #4  0x00007f57f0bef7f3 in __GI_abort () at ./stdlib/abort.c:79
> #5  0x00007f57f1671351 in __addvsi3 () from .../src/test/regress/refint.so
> #6  0x00007f57f1670234 in check_foreign_key (fcinfo=0x7ffebf523650) at
refint.c:321
>
> (gdb) f 6
> #6  0x00007f3400ef9234 in check_foreign_key (fcinfo=0x7ffd6e16a600) at
refint.c:321
> 321             nkeys = (nargs - nrefs) / (nrefs + 1);
> (gdb) p nargs
> $1 = 3
> (gdb) p nrefs
> $2 = 2147483647

I have not looked into this yet, though I was unable to reproduce it
immediately.

    test=# CREATE FUNCTION check_foreign_key () RETURNS trigger AS
'.../refint.so' LANGUAGE C;
    ERROR:  could not access file ".../refint.so": No such file or directory

I think I just have to play around with the path.

----

>> #8  0x00782675 in ExecInterpExpr (state=0x14d9c4c, econtext=0x14da15c,
isnull=0xbff1dc3f) at execExprInterp.c:765
> Hm.  It looks like that is pointing to __builtin_mul_overflow(), which

Agreed that this looks strange. The docs [0] seem to indicate that this
shouldn't happen.

> These built-in functions promote the first two operands into infinite
> precision signed type and perform addition on those promoted
> operands.
...
> As the addition is performed in infinite signed precision, these
> built-in functions have fully defined behavior for all argument
> values.
...
> The first built-in function allows arbitrary integral types for
> operands and the result type must be pointer to some integral type
> other than enumerated or boolean type

The docs for the mul functions say that they behave the same as
addition. Alexander, is it possible that you're compiling with
something other than GCC?

----

> +++ b/src/backend/nodes/bitmapset.c

This approach seems to resolve the issue locally for me, and I think it
falls out cleanly from the comment in the code above.

Thanks,
Joseph Koshakow

[0] https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html

---

From: Joseph Koshakow <[email-masked]>
Date: Sat, 17 Aug 2024 17:52:48 -0400

>> Here's what it looks like is happening:
>>
>> 1. When inserting into the table, we create a new dynamic hash table
>> and set `nelem` equal to `temp_buffers`, which is 1000000000.
>>
>> 2. `nbuckets` is then set to the the next highest power of 2 from
>>    `nelem`, which is 1073741824.
>>
>>     /*
>>      * Allocate space for the next greater power of two number of
buckets,
>>      * assuming a desired maximum load factor of 1.
>>      */
>>     nbuckets = next_pow2_int(nelem);
>>
>> 3. Shift `nbuckets` to the left by 1. This would equal 2147483648,
>> which is larger than `INT_MAX`, which causes an overflow.
>>
>>     hctl->high_mask = (nbuckets << 1) - 1;
>>
>> The max value allowed for `temp_buffers` is `INT_MAX / 2` (1073741823),
>> So any value of `temp_buffers` in the range (536870912, 1073741823]
>> would cause this overflow. Without `-ftrapv`, `nbuckets` would wrap
>> around to -2147483648, which is likely to cause all sorts of havoc, I'm
>> just not sure what exactly.
>>
>> Also, `nbuckets = next_pow2_int(nelem);`, by itself is a bit sketchy
>> considering that `nelem` is a `long` and `nbuckets` is an `int`.
>> Potentially, the fix here is to just convert `nbuckets` to a `long`. >>
I plan on checking if that's feasible.
> Yeah, the minimum value that triggers the trap is 536870913 and the
maximum
> accepted is 1073741823.
>
> Without -ftrapv, hctl->high_mask is set to 2147483647 on my machine,
> when nbuckets is 1073741824, and the INSERT apparently succeeds.

> hoping to look into if this is feasible soon.

I've both figured out why the INSERT still succeeds and a simple
solution to this. After `nbuckets` wraps around to -2147483648, we
subtract 1 which causes it to wrap back around to 2147483647. Which
explains the result seen by Alexander.

By the way,

>> Also, `nbuckets = next_pow2_int(nelem);`, by itself is a bit sketchy
>> considering that `nelem` is a `long` and `nbuckets` is an `int`.

It turns out I was wrong about this, `next_pow2_int` will always return
a value that fits into an `int`.

    hctl->high_mask = (nbuckets << 1) - 1;

This calculation is used to ultimately populate the field
`uint32 high_mask`. I'm not very familiar with this hash table
implementation and I'm not entirely sure what it would take to convert
this to a `uint64`, but from poking around it looks like it would have
a huge blast radius.

The largest possible (theoretical) value for `nbuckets` is
`1073741824`, the largest power of 2 that fits into an `int`. So, the
largest possible value for `nbuckets << 1` is `2147483648`. This can
fully fit in a `uint32`, so the simple fix for this case is to cast
`nbuckets` to a `uint32` before shifting. I've attached this fix,
Alexander if you have time I would appreciate if you were able to test
it.

I noticed another potential issue with next_pow2_int. The
implementation is in dynahash.c and is as follows

    /* calculate first power of 2 >= num, bounded to what will fit in an
int */
    static int
    next_pow2_int(long num)
    {
        if (num > INT_MAX / 2)
            num = INT_MAX / 2;
        return 1 << my_log2(num);
    }

I'm pretty sure that `INT_MAX / 2` is not a power of 2, as `INT_MAX`
is not a power of 2. It should be `num = INT_MAX / 2 + 1;` I've also
attached a patch with this fix.

Thanks,
Joseph Koshakow
From 529452218496b9cd89464e1fad9e48c6279cef86 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <[email-masked]>
Date: Sat, 17 Aug 2024 17:50:11 -0400
Subject: [PATCH v25 3/3] Fix next_pow2_int

This commit fixes the `next_pow2_int` function so that it always
returns a power of 2.
---
 src/backend/utils/hash/dynahash.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index 5aed981085..ee2bbe5dc8 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -1819,8 +1819,8 @@ next_pow2_long(long num)
 static int
 next_pow2_int(long num)
 {
-	if (num > INT_MAX / 2)
-		num = INT_MAX / 2;
+	if (num > INT_MAX / 2 + 1)
+		num = INT_MAX / 2 + 1;
 	return 1 << my_log2(num);
 }
 
-- 
2.34.1

From 397669bb89e51610928e2404c02a467ac431b280 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <[email-masked]>
Date: Sat, 17 Aug 2024 17:47:10 -0400
Subject: [PATCH v25 2/3] Remove dependence on -fwrapv semantics in dynahash

This commit updates the logic for creating a new dynamic hash table to
no longer rely on integer wrapping for correctness.
---
 src/backend/utils/hash/dynahash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/utils/hash/dynahash.c b/src/backend/utils/hash/dynahash.c
index 5d9c62b652..5aed981085 100644
--- a/src/backend/utils/hash/dynahash.c
+++ b/src/backend/utils/hash/dynahash.c
@@ -717,7 +717,7 @@ init_htab(HTAB *hashp, long nelem)
 		nbuckets <<= 1;
 
 	hctl->max_bucket = hctl->low_mask = nbuckets - 1;
-	hctl->high_mask = (nbuckets << 1) - 1;
+	hctl->high_mask = ((uint32) nbuckets << 1) - 1;
 
 	/*
 	 * Figure number of directory segments needed, round up to a power of 2
-- 
2.34.1

From 15e5a79f198a029fc6436c409a00a1592e23a46d Mon Sep 17 00:00:00 2001
From: Matthew Kim <[email-masked]>
Date: Tue, 9 Jul 2024 18:25:10 -0400
Subject: [PATCH v25 1/3] Remove dependence on -fwrapv semantics in some
 date/time functions.

This commit updates a few date/time functions, such as
to_timestamp() and make_date(), to no longer rely on signed integer
wrapping for correctness. This is intended to move us closer towards
removing -fwrapv, which may enable some compiler optimizations.
However, there is presently no plan to actually remove that compiler
option in the near future.
---
 src/backend/utils/adt/date.c           |  5 +++-
 src/backend/utils/adt/formatting.c     | 35 +++++++++++++++++++++++---
 src/test/regress/expected/date.out     |  2 ++
 src/test/regress/expected/horology.out |  4 +++
 src/test/regress/sql/date.sql          |  1 +
 src/test/regress/sql/horology.sql      |  2 ++
 6 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 9c854e0e5c..baa677fb96 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -257,7 +257,10 @@ make_date(PG_FUNCTION_ARGS)
 	if (tm.tm_year < 0)
 	{
 		bc = true;
-		tm.tm_year = -tm.tm_year;
+		if (pg_mul_s32_overflow(tm.tm_year, -1, &tm.tm_year))
+			ereport(ERROR,
+					(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+					 errmsg("date field value out of range")));
 	}
 
 	dterr = ValidateDate(DTK_DATE_M, false, false, bc, &tm);
diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
index 68069fcfd3..e2cb57a9e1 100644
--- a/src/backend/utils/adt/formatting.c
+++ b/src/backend/utils/adt/formatting.c
@@ -77,6 +77,7 @@
 
 #include "catalog/pg_collation.h"
 #include "catalog/pg_type.h"
+#include "common/int.h"
 #include "common/unicode_case.h"
 #include "common/unicode_category.h"
 #include "mb/pg_wchar.h"
@@ -3809,7 +3810,15 @@ DCH_from_char(FormatNode *node, const char *in, TmFromChar *out,
 						ereturn(escontext,,
 								(errcode(ERRCODE_INVALID_DATETIME_FORMAT),
 								 errmsg("invalid input string for \"Y,YYY\"")));
-					years += (millennia * 1000);
+					if (pg_mul_s32_overflow(millennia, 1000, &millennia))
+						ereturn(escontext,,
+								(errcode(ERRCODE_INVALID_DATETIME_FORMAT),
+								 errmsg("invalid input string for \"Y,YYY\"")));
+					if (pg_add_s32_overflow(years, millennia, &years))
+						ereturn(escontext,,
+								(errcode(ERRCODE_INVALID_DATETIME_FORMAT),
+								 errmsg("invalid input string for \"Y,YYY\"")));
+
 					if (!from_char_set_int(&out->year, years, n, escontext))
 						return;
 					out->yysz = 4;
@@ -4797,11 +4806,31 @@ do_to_timestamp(text *date_txt, text *fmt, Oid collid, bool std,
 		if (tmfc.bc)
 			tmfc.cc = -tmfc.cc;
 		if (tmfc.cc >= 0)
+		{
 			/* +1 because 21st century started in 2001 */
-			tm->tm_year = (tmfc.cc - 1) * 100 + 1;
+			/* tm->tm_year = (tmfc.cc - 1) * 100 + 1; */
+			if (pg_mul_s32_overflow((tmfc.cc - 1), 100, &tm->tm_year) ||
+				pg_add_s32_overflow(tm->tm_year, 1, &tm->tm_year))
+			{
+				ereport(ERROR,
+						(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+						 errmsg("date out of range: \"%s\"",
+								text_to_cstring(date_txt))));
+			}
+		}
 		else
+		{
 			/* +1 because year == 599 is 600 BC */
-			tm->tm_year = tmfc.cc * 100 + 1;
+			/* tm->tm_year = tmfc.cc * 100 + 1; */
+			if (pg_mul_s32_overflow(tmfc.cc, 100, &tm->tm_year) ||
+				pg_add_s32_overflow(tm->tm_year, 1, &tm->tm_year))
+			{
+				ereport(ERROR,
+						(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+						 errmsg("date out of range: \"%s\"",
+								text_to_cstring(date_txt))));
+			}
+		}
 		fmask |= DTK_M(YEAR);
 	}
 
diff --git a/src/test/regress/expected/date.out b/src/test/regress/expected/date.out
index f5949f3d17..9293e045b0 100644
--- a/src/test/regress/expected/date.out
+++ b/src/test/regress/expected/date.out
@@ -1532,3 +1532,5 @@ select make_time(10, 55, 100.1);
 ERROR:  time field value out of range: 10:55:100.1
 select make_time(24, 0, 2.1);
 ERROR:  time field value out of range: 24:00:2.1
+SELECT make_date(-2147483648, 1, 1);
+ERROR:  date field value out of range
diff --git a/src/test/regress/expected/horology.out b/src/test/regress/expected/horology.out
index 241713cc51..df02d268c0 100644
--- a/src/test/regress/expected/horology.out
+++ b/src/test/regress/expected/horology.out
@@ -3448,6 +3448,8 @@ SELECT i, to_timestamp('2018-11-02 12:34:56.123456', 'YYYY-MM-DD HH24:MI:SS.FF'
 
 SELECT i, to_timestamp('2018-11-02 12:34:56.123456789', 'YYYY-MM-DD HH24:MI:SS.FF' || i) FROM generate_series(1, 6) i;
 ERROR:  date/time field value out of range: "2018-11-02 12:34:56.123456789"
+SELECT to_timestamp('1000000000,999', 'Y,YYY');
+ERROR:  invalid input string for "Y,YYY"
 SELECT to_date('1 4 1902', 'Q MM YYYY');  -- Q is ignored
   to_date   
 ------------
@@ -3778,6 +3780,8 @@ SELECT to_date('0000-02-01','YYYY-MM-DD');  -- allowed, though it shouldn't be
  02-01-0001 BC
 (1 row)
 
+SELECT to_date('100000000', 'CC');
+ERROR:  date out of range: "100000000"
 -- to_char's TZ format code produces zone abbrev if known
 SELECT to_char('2012-12-12 12:00'::timestamptz, 'YYYY-MM-DD HH:MI:SS TZ');
          to_char         
diff --git a/src/test/regress/sql/date.sql b/src/test/regress/sql/date.sql
index 1c58ff6966..9a4e5832b9 100644
--- a/src/test/regress/sql/date.sql
+++ b/src/test/regress/sql/date.sql
@@ -373,3 +373,4 @@ select make_date(2013, 13, 1);
 select make_date(2013, 11, -1);
 select make_time(10, 55, 100.1);
 select make_time(24, 0, 2.1);
+SELECT make_date(-2147483648, 1, 1);
diff --git a/src/test/regress/sql/horology.sql b/src/test/regress/sql/horology.sql
index e5cf12ff63..db532ee3c0 100644
--- a/src/test/regress/sql/horology.sql
+++ b/src/test/regress/sql/horology.sql
@@ -558,6 +558,7 @@ SELECT i, to_timestamp('2018-11-02 12:34:56.1234', 'YYYY-MM-DD HH24:MI:SS.FF' ||
 SELECT i, to_timestamp('2018-11-02 12:34:56.12345', 'YYYY-MM-DD HH24:MI:SS.FF' || i) FROM generate_series(1, 6) i;
 SELECT i, to_timestamp('2018-11-02 12:34:56.123456', 'YYYY-MM-DD HH24:MI:SS.FF' || i) FROM generate_series(1, 6) i;
 SELECT i, to_timestamp('2018-11-02 12:34:56.123456789', 'YYYY-MM-DD HH24:MI:SS.FF' || i) FROM generate_series(1, 6) i;
+SELECT to_timestamp('1000000000,999', 'Y,YYY');
 
 SELECT to_date('1 4 1902', 'Q MM YYYY');  -- Q is ignored
 SELECT to_date('3 4 21 01', 'W MM CC YY');
@@ -660,6 +661,7 @@ SELECT to_date('2016 365', 'YYYY DDD');  -- ok
 SELECT to_date('2016 366', 'YYYY DDD');  -- ok
 SELECT to_date('2016 367', 'YYYY DDD');
 SELECT to_date('0000-02-01','YYYY-MM-DD');  -- allowed, though it shouldn't be
+SELECT to_date('100000000', 'CC');
 
 -- to_char's TZ format code produces zone abbrev if known
 SELECT to_char('2012-12-12 12:00'::timestamptz, 'YYYY-MM-DD HH:MI:SS TZ');
-- 
2.34.1

---

From: Alexander Lakhin <[email-masked]>
Date: Sun, 18 Aug 2024 06:00:00 +0300

Hello Joe,

> and attached to this email. They still require a review.

I've filed a separate bug report about date/time conversion issues
yesterday. Maybe it was excessive, but it also demonstrates other
problematic cases:
https://www.postgresql.org/message-id/18585-db646741dd649abd%40postgresql.org

Best regards,
Alexander

---

From: Alexander Lakhin <[email-masked]>
Date: Sun, 18 Aug 2024 15:00:00 +0300

> Alexander if you have time I would appreciate if you were able to test

Yes, I've tested v25-0002-*.patch and can confirm that this fix works
as well.

Best regards,
Alexander

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 20 Aug 2024 16:21:33 -0500

I've combined all the current proposed changes into one patch.  I've also
introduced signed versions of the negation functions into int.h to avoid
relying on multiplication.

-- 
nathan
>From 2364ba4028f879a22b9f69f999aee3ea9c013ec0 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Tue, 20 Aug 2024 16:12:39 -0500
Subject: [PATCH v26 1/1] Remove dependence on -fwrapv semantics in more
 places.

---
 src/backend/nodes/bitmapset.c          |  2 +-
 src/backend/utils/adt/date.c           |  6 +++-
 src/backend/utils/adt/formatting.c     | 28 +++++++++++++--
 src/backend/utils/hash/dynahash.c      |  6 ++--
 src/include/common/int.h               | 48 ++++++++++++++++++++++++++
 src/test/regress/expected/date.out     |  2 ++
 src/test/regress/expected/horology.out |  4 +++
 src/test/regress/expected/union.out    | 43 +++++++++++++++++++++++
 src/test/regress/sql/date.sql          |  1 +
 src/test/regress/sql/horology.sql      |  2 ++
 src/test/regress/sql/union.sql         |  9 +++++
 11 files changed, 143 insertions(+), 8 deletions(-)

 
diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 9c854e0e5c..0782e84776 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -257,7 +257,11 @@ make_date(PG_FUNCTION_ARGS)
 	if (tm.tm_year < 0)
 	{
 		bc = true;
-		tm.tm_year = -tm.tm_year;
+		if (pg_neg_s32_overflow(tm.tm_year, &tm.tm_year))
+			ereport(ERROR,
+					(errcode(ERRCODE_DATETIME_FIELD_OVERFLOW),
+					 errmsg("date field value out of range: %d-%02d-%02d",
+							tm.tm_year, tm.tm_mon, tm.tm_mday)));
 	}
 
index 68069fcfd3..76bb3a79b5 100644
 
 #include "catalog/pg_collation.h"
 #include "catalog/pg_type.h"
+#include "common/int.h"
 #include "common/unicode_case.h"
 #include "common/unicode_category.h"
 #include "mb/pg_wchar.h"
@@ -3809,7 +3810,12 @@ DCH_from_char(FormatNode *node, const char *in, TmFromChar *out,
 						ereturn(escontext,,
 								(errcode(ERRCODE_INVALID_DATETIME_FORMAT),
 								 errmsg("invalid input string for \"Y,YYY\"")));
-					years += (millennia * 1000);
+					if (pg_mul_s32_overflow(millennia, 1000, &millennia) ||
+						pg_add_s32_overflow(years, millennia, &years))
+						ereturn(escontext,,
+								(errcode(ERRCODE_INVALID_DATETIME_FORMAT),
+								 errmsg("invalid input string for \"Y,YYY\"")));
+
 					if (!from_char_set_int(&out->year, years, n, escontext))
 						return;
 					out->yysz = 4;
@@ -4797,11 +4803,27 @@ do_to_timestamp(text *date_txt, text *fmt, Oid collid, bool std,
 		if (tmfc.bc)
 			tmfc.cc = -tmfc.cc;
 		if (tmfc.cc >= 0)
+		{
 			/* +1 because 21st century started in 2001 */
-			tm->tm_year = (tmfc.cc - 1) * 100 + 1;
+			/* tm->tm_year = (tmfc.cc - 1) * 100 + 1 */
+			if (pg_mul_s32_overflow(tmfc.cc - 1, 100, &tm->tm_year) ||
+				pg_add_s32_overflow(tm->tm_year, 1, &tm->tm_year))
+				ereport(ERROR,
+						(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+						 errmsg("date out of range: \"%s\"",
+								text_to_cstring(date_txt))));
+		}
 		else
+		{
 			/* +1 because year == 599 is 600 BC */
-			tm->tm_year = tmfc.cc * 100 + 1;
+			/* tm->tm_year = tmfc.cc * 100 + 1 */
+			if (pg_mul_s32_overflow(tmfc.cc, 100, &tm->tm_year) ||
+				pg_add_s32_overflow(tm->tm_year, 1, &tm->tm_year))
+				ereport(ERROR,
+						(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+						 errmsg("date out of range: \"%s\"",
+								text_to_cstring(date_txt))));
+		}
 		fmask |= DTK_M(YEAR);
 	}
 
index 5d9c62b652..ee2bbe5dc8 100644

 	/*
 
diff --git a/src/include/common/int.h b/src/include/common/int.h
index 3b1590d676..6b50aa67b9 100644
--- a/src/include/common/int.h
+++ b/src/include/common/int.h
@@ -117,6 +117,22 @@ pg_mul_s16_overflow(int16 a, int16 b, int16 *result)
 #endif
 }
 
+static inline bool
+pg_neg_s16_overflow(int16 a, int16 *result)
+{
+#if defined(HAVE__BUILTIN_OP_OVERFLOW)
+	return __builtin_sub_overflow(0, a, result);
+#else
+	if (unlikely(a == PG_INT16_MIN))
+	{
+		*result = 0x5EED;		/* to avoid spurious warnings */
+		return true;
+	}
+	*result = -a;
+	return false;
+#endif
+}
+
 static inline uint16
 pg_abs_s16(int16 a)
 {
@@ -185,6 +201,22 @@ pg_mul_s32_overflow(int32 a, int32 b, int32 *result)
 #endif
 }
 
+static inline bool
+pg_neg_s32_overflow(int32 a, int32 *result)
+{
+#if defined(HAVE__BUILTIN_OP_OVERFLOW)
+	return __builtin_sub_overflow(0, a, result);
+#else
+	if (unlikely(a == PG_INT32_MIN))
+	{
+		*result = 0x5EED;		/* to avoid spurious warnings */
+		return true;
+	}
+	*result = -a;
+	return false;
+#endif
+}
+
 static inline uint32
 pg_abs_s32(int32 a)
 {
@@ -300,6 +332,22 @@ pg_mul_s64_overflow(int64 a, int64 b, int64 *result)
 #endif
 }
 
+static inline bool
+pg_neg_s64_overflow(int64 a, int64 *result)
+{
+#if defined(HAVE__BUILTIN_OP_OVERFLOW)
+	return __builtin_sub_overflow(0, a, result);
+#else
+	if (unlikely(a == PG_INT64_MIN))
+	{
+		*result = 0x5EED;		/* to avoid spurious warnings */
+		return true;
+	}
+	*result = -a;
+	return false;
+#endif
+}
+
 static inline uint64
 pg_abs_s64(int64 a)
 {
diff --git a/src/test/regress/expected/date.out b/src/test/regress/expected/date.out
index f5949f3d17..c8f76c205d 100644
--- a/src/test/regress/expected/date.out
+++ b/src/test/regress/expected/date.out
@@ -1532,3 +1532,5 @@ select make_time(10, 55, 100.1);
 ERROR:  time field value out of range: 10:55:100.1
 select make_time(24, 0, 2.1);
 ERROR:  time field value out of range: 24:00:2.1
+SELECT make_date(-2147483648, 1, 1);
+ERROR:  date field value out of range: -2147483648-01-01
diff --git a/src/test/regress/expected/horology.out b/src/test/regress/expected/horology.out
index 241713cc51..df02d268c0 100644
--- a/src/test/regress/expected/horology.out
+++ b/src/test/regress/expected/horology.out
@@ -3448,6 +3448,8 @@ SELECT i, to_timestamp('2018-11-02 12:34:56.123456', 'YYYY-MM-DD HH24:MI:SS.FF'

+SELECT to_date('100000000', 'CC');
+ERROR:  date out of range: "100000000"
 -- to_char's TZ format code produces zone abbrev if known
 SELECT to_char('2012-12-12 12:00'::timestamptz, 'YYYY-MM-DD HH:MI:SS TZ');
          to_char         
diff --git a/src/test/regress/expected/union.out b/src/test/regress/expected/union.out
index 0fd0e1c38b..444744848e 100644
--- a/src/test/regress/expected/union.out
+++ b/src/test/regress/expected/union.out
@@ -59,6 +59,49 @@ SELECT 1.1 AS two UNION SELECT 2.2 ORDER BY 1;
  2.2
 (2 rows)
 
+SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL
+SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL
+SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL
+SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL
+SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL
+SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL
+SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL
+SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1;
+ ?column? 
+----------
+        1
+        1
+        1
+        1
+        1
+        1
+        1
+        1
+        1
+        1
+        1
+        1
+        1
+        1
+        1
+        1
+        1
+        1
+        1
+        1
+        1
+        1
+        1
+        1
+        1
+        1
+        1
+        1
+        1
+        1
+        1
+(31 rows)
+
 -- Mixed types
 SELECT 1.1 AS two UNION SELECT 2 ORDER BY 1;
  two 

 -- to_char's TZ format code produces zone abbrev if known
 SELECT to_char('2012-12-12 12:00'::timestamptz, 'YYYY-MM-DD HH:MI:SS TZ');
diff --git a/src/test/regress/sql/union.sql b/src/test/regress/sql/union.sql
index f8826514e4..e1954a92d2 100644
--- a/src/test/regress/sql/union.sql
+++ b/src/test/regress/sql/union.sql
@@ -20,6 +20,15 @@ SELECT 1 AS three UNION SELECT 2 UNION ALL SELECT 2 ORDER BY 1;
 
 SELECT 1.1 AS two UNION SELECT 2.2 ORDER BY 1;
 
+SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL
+SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL
+SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL
+SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL
+SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL
+SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL
+SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1 UNION ALL
+SELECT 1 UNION ALL SELECT 1 UNION ALL SELECT 1;
+
 -- Mixed types
 
 SELECT 1.1 AS two UNION SELECT 2 ORDER BY 1;
-- 
2.39.3 (Apple Git-146)

---

From: Alexander Lakhin <[email-masked]>
Date: Wed, 21 Aug 2024 10:00:00 +0300

Hello Nathan,

> relying on multiplication.

Thank you for taking care of this!

I'd like to add some info to show how big the iceberg is.

Beside other trap-triggered places in date/time conversion functions, I
also discovered:
1)
CREATE TABLE jt(j jsonb); INSERT INTO jt VALUES('[]'::jsonb);
UPDATE jt SET j[0][-2147483648] = '0';

#4  0x00007f15ab00d7f3 in __GI_abort () at ./stdlib/abort.c:79
#5  0x00005570113b2591 in __addvsi3 ()
#6  0x00005570111d55a0 in push_null_elements (ps=0x7fff37385fb8, num=-2147483648) at jsonfuncs.c:1707
#7  0x00005570111d5749 in push_path (st=0x7fff37385fb8, level=0, path_elems=0x55701300c880, path_nulls=0x55701300d520,
     path_len=2, newval=0x7fff37386030) at jsonfuncs.c:1770

The "problematic" code:
         while (num-- > 0)
                 *ps = 0;
looks innocent to me, but is not for good enough for -ftrapv.
I think there could be other similar places and this raises two questions:
can they be reached with INT_MIN and what to do if so?

By the way, the same can be seen with CC=clang CPPFLAGS="-ftrapv". Please
look at the code produced by both compilers for x86_64:
https://godbolt.org/z/vjszjf4b3
(clang generates ud1, while gcc uses call __addvsi3)

The aside question is: should jsonb subscripting accept negative indexes
when the target array is not initialized yet?

Compare:
CREATE TABLE jt(j jsonb); INSERT INTO jt VALUES('[]'::jsonb);
UPDATE jt SET j[0][-1] = '0';
SELECT * FROM jt;
    j
-------
  [[0]]

with
CREATE TABLE jt(j jsonb); INSERT INTO jt VALUES('[[]]'::jsonb);
UPDATE jt SET j[0][-1] = '0';
ERROR:  path element at position 2 is out of range: -1

2)
SELECT x, lag(x, -2147483648) OVER (ORDER BY x) FROM (SELECT 1) x;

#4  0x00007fa7d00f47f3 in __GI_abort () at ./stdlib/abort.c:79
#5  0x00005623a7336851 in __negvsi2 ()
#6  0x00005623a726ae35 in leadlag_common (fcinfo=0x7ffd59cca950, forward=false, withoffset=true, withdefault=false)
     at windowfuncs.c:551
#7  0x00005623a726af19 in window_lag_with_offset (fcinfo=0x7ffd59cca950) at windowfuncs.c:594

As to 32-bit Debian, I wrote about before, I use gcc (Debian 12.2.0-14).
Please look at the demo code (and it's assembly, produced with
gcc -S -ftrapv t.c) attached:
gcc -Wall -Wextra -fsanitize=signed-integer-overflow -Wstrict-overflow=5 \
  -O0 -ftrapv t.c -o t && ./t
Aborted (core dumped)

#4  0xb762226a in __GI_abort () at ./stdlib/abort.c:79
#5  0x00495077 in __mulvdi3.cold ()
#6  0x00495347 in pg_mul_s64_overflow ()

(It looks like -Wstrict-overflow can't help with the static analysis
desired in such cases.)

Moreover, I got `make check` failed with -ftrapv on aarch64 (using gcc 8.3)
as follows:
#1  0x0000007e1edc48e8 in __GI_abort () at abort.c:79
#2  0x0000005ee66b71cc in __subvdi3 ()
#3  0x0000005ee6560e24 in int8gcd_internal (arg1=-9223372036854775808, arg2=1) at int8.c:623
#4  0x0000005ee62f576c in ExecInterpExpr (state=0x5eeaba9d18, econtext=0x5eeaba95f0, isnull=<optimized out>)
     at execExprInterp.c:770
...
#13 0x0000005ee64e5d84 in exec_simple_query (
     query_string=query_string@entry=0x5eeaac7500 "SELECT a, b, gcd(a, b), gcd(a, -b), gcd(b, a), gcd(-b, a)\nFROM 
(VALUES (0::int8, 0::int8),\n", ' ' <repeats 13 times>, "(0::int8, 29893644334::int8),\n", ' ' <repeats 13 times>, 
"(288484263558::int8, 29893644334::int8),\n", ' ' <repeats 12 times>...) at postgres.c:1284

So I wonder whether enabling -ftrapv can really help us prepare the code
for -fno-wrapv?

Best regards,
Alexander

---

From: Nathan Bossart <[email-masked]>
Date: Wed, 21 Aug 2024 10:37:25 -0500

> I'd like to add some info to show how big the iceberg is.

Hm.  It seems pretty clear that removing -fwrapv won't be happening anytime
soon.  I don't mind trying to fix a handful of cases from time to time, but
unless there's a live bug, I'm probably not going to treat this stuff as
high priority.

-- 
nathan

---

From: Joseph Koshakow <[email-masked]>
Date: Sat, 24 Aug 2024 08:44:40 -0400

> unless there's a live bug, I'm probably not going to treat this stuff as

I think I'm also going to take a step back because I'm a bit
fatigued on the overflow work. My goal here wasn't necessarily to
remove -fwrapv, because I think it will always be a useful safeguard.
Instead I wanted to add -ftrapv to builds with asserts enabled to try
and prevent future overflow based bugs. Though, it looks like that
won't happen anytime soon either.

FWIW, Matthew's patch actually does resolve a bug with `to_timestamp`
and `to_date`. It converts the following incorrect queries

    test=# SELECT to_timestamp('2147483647,999', 'Y,YYY');
              to_timestamp
    ---------------------------------
     0001-01-01 00:00:00-04:56:02 BC
    (1 row)

    test=# SELECT to_date('-2147483648', 'CC');
      to_date
    ------------
     0001-01-01
    (1 row)

into errors

    test=# SELECT to_timestamp('2147483647,999', 'Y,YYY');
    ERROR:  invalid input string for "Y,YYY"
    test=# SELECT to_date('-2147483648', 'CC');
    ERROR:  date out of range: "-2147483648"

So, it might be worth committing only his changes before moving on.

Thanks,
Joseph Koshakow

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 5 Dec 2024 16:50:34 -0600

>     test=# SELECT to_timestamp('2147483647,999', 'Y,YYY');

Good point.  Here is a v27 patch that extracts the bug fix portions of the
v26 patch.  If/when this is committed, I think we should close the
commitfest entry.

-- 
nathan
>From 13e74f283a0994274e656a7d3e430d360884ae13 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Thu, 5 Dec 2024 16:39:00 -0600
Subject: [PATCH v27 1/1] fix date/time overflows

---
 src/backend/utils/adt/date.c           |  6 +++-
 src/backend/utils/adt/formatting.c     | 32 +++++++++++++++--
 src/include/common/int.h               | 48 ++++++++++++++++++++++++++
 src/test/regress/expected/date.out     |  2 ++
 src/test/regress/expected/horology.out |  6 ++++
 src/test/regress/sql/date.sql          |  1 +
 src/test/regress/sql/horology.sql      |  4 +++
 7 files changed, 95 insertions(+), 4 deletions(-)

index 8130f3e8ac..d5ee96aa6c 100644
 
index 2bcc185708..9eb7d08169 100644
 
@@ -3826,7 +3827,12 @@ DCH_from_char(FormatNode *node, const char *in, TmFromChar *out,
+								 errmsg("value for \"Y,YYY\" in source string is out of range")));
@@ -4814,11 +4820,31 @@ do_to_timestamp(text *date_txt, text *fmt, Oid collid, bool std,
+				DateTimeParseError(DTERR_FIELD_OVERFLOW, NULL,
+								   text_to_cstring(date_txt), "timestamp",
+								   escontext);
+				goto fail;
+				DateTimeParseError(DTERR_FIELD_OVERFLOW, NULL,
+								   text_to_cstring(date_txt), "timestamp",
+								   escontext);
+				goto fail;
+			}
+		}
 		fmask |= DTK_M(YEAR);
 	}

+static inline bool
 
+static inline bool
 
+static inline bool
index c9cec70c38..674fcc2456 100644
index 6d7dd5c988..5f1b9ab075 100644
@@ -3453,6 +3453,8 @@ SELECT i, to_timestamp('2018-11-02 12:34:56.123456', 'YYYY-MM-DD HH24:MI:SS.FF'
 
 SELECT i, to_timestamp('2018-11-02 12:34:56.123456789', 'YYYY-MM-DD HH24:MI:SS.FF' || i) FROM generate_series(1, 6) i;
 ERROR:  date/time field value out of range: "2018-11-02 12:34:56.123456789"
+SELECT to_timestamp('1000000000,999', 'Y,YYY');
+ERROR:  value for "Y,YYY" in source string is out of range
 SELECT to_date('1 4 1902', 'Q MM YYYY');  -- Q is ignored
   to_date   
 ------------
@@ -3783,6 +3785,10 @@ SELECT to_date('0000-02-01','YYYY-MM-DD');  -- allowed, though it shouldn't be
  02-01-0001 BC
 (1 row)
 
+SELECT to_date('100000000', 'CC');
+ERROR:  date/time field value out of range: "100000000"
+SELECT to_date('-100000000', 'CC');
+ERROR:  date/time field value out of range: "-100000000"
 -- to_char's TZ format code produces zone abbrev if known
 SELECT to_char('2012-12-12 12:00'::timestamptz, 'YYYY-MM-DD HH:MI:SS TZ');
          to_char         
diff --git a/src/test/regress/sql/date.sql b/src/test/regress/sql/date.sql
index 1c58ff6966..9a4e5832b9 100644
--- a/src/test/regress/sql/date.sql
+++ b/src/test/regress/sql/date.sql
@@ -373,3 +373,4 @@ select make_date(2013, 13, 1);
 select make_date(2013, 11, -1);
 select make_time(10, 55, 100.1);
 select make_time(24, 0, 2.1);
+SELECT make_date(-2147483648, 1, 1);
diff --git a/src/test/regress/sql/horology.sql b/src/test/regress/sql/horology.sql
index 0fe3c783e6..553cf83b15 100644
--- a/src/test/regress/sql/horology.sql
+++ b/src/test/regress/sql/horology.sql
@@ -559,6 +559,8 @@ SELECT i, to_timestamp('2018-11-02 12:34:56.12345', 'YYYY-MM-DD HH24:MI:SS.FF' |
 SELECT i, to_timestamp('2018-11-02 12:34:56.123456', 'YYYY-MM-DD HH24:MI:SS.FF' || i) FROM generate_series(1, 6) i;
 SELECT i, to_timestamp('2018-11-02 12:34:56.123456789', 'YYYY-MM-DD HH24:MI:SS.FF' || i) FROM generate_series(1, 6) i;
 
+SELECT to_timestamp('1000000000,999', 'Y,YYY');
+
 SELECT to_date('1 4 1902', 'Q MM YYYY');  -- Q is ignored
 SELECT to_date('3 4 21 01', 'W MM CC YY');
 SELECT to_date('2458872', 'J');
@@ -660,6 +662,8 @@ SELECT to_date('2016 365', 'YYYY DDD');  -- ok
 SELECT to_date('2016 366', 'YYYY DDD');  -- ok
 SELECT to_date('2016 367', 'YYYY DDD');
 SELECT to_date('0000-02-01','YYYY-MM-DD');  -- allowed, though it shouldn't be
+SELECT to_date('100000000', 'CC');
+SELECT to_date('-100000000', 'CC');
 
2.39.5 (Apple Git-154)

---

From: Joseph Koshakow <[email-masked]>
Date: Thu, 5 Dec 2024 20:00:12 -0500

> v26 patch.  If/when this is committed, I think we should close the

I looked through this patch and it looks good to me, assuming cfbot is
happy. Also I agree, we should close the commitfest entry.

Thanks,
Joseph Koshakow

---

From: Nathan Bossart <[email-masked]>
Date: Thu, 5 Dec 2024 21:58:29 -0600

> happy. Also I agree, we should close the commitfest entry.

Thanks for reviewing.  In v28, I fixed a silly mistake revealed by cfbot's
Windows run.  I also went ahead and tried to fix most of the issues
reported in a nearby thread [0].  The only one I haven't tracked down yet
is the "ISO week" one (case 7).

[0] https://postgr.es/m/18585-db646741dd649abd%40postgresql.org

-- 
nathan
>From c96de41b95f8380ba7ff15c9baef36713be6a43c Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Thu, 5 Dec 2024 21:50:50 -0600
Subject: [PATCH v28 1/1] Fix various overflow hazards in date and timestamp
 functions.

Reported-by: Alexander Lakhin
Author: Matthew Kim, Nathan Bossart
Reviewed-by: Joseph Koshakow, Jian He
Discussion: https://postgr.es/m/31ad2cd1-db94-bdb3-f91a-65ffdb4bef95%40gmail.com
Discussion: https://postgr.es/m/18585-db646741dd649abd%40postgresql.org
---
 src/backend/utils/adt/date.c           |   9 ++-
 src/backend/utils/adt/formatting.c     | 104 +++++++++++++++++++++++--
 src/include/common/int.h               |  48 ++++++++++++
 src/test/regress/expected/date.out     |   2 +
 src/test/regress/expected/horology.out |  16 ++++
 src/test/regress/sql/date.sql          |   1 +
 src/test/regress/sql/horology.sql      |   8 ++
 7 files changed, 179 insertions(+), 9 deletions(-)

diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 8130f3e8ac..da61ac0e86 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -256,8 +256,15 @@ make_date(PG_FUNCTION_ARGS)
 	/* Handle negative years as BC */
 	if (tm.tm_year < 0)
 	{
+		int			year = tm.tm_year;
+
 		bc = true;
-		tm.tm_year = -tm.tm_year;
+		if (pg_neg_s32_overflow(year, &year))
+			ereport(ERROR,
+					(errcode(ERRCODE_DATETIME_FIELD_OVERFLOW),
+					 errmsg("date field value out of range: %d-%02d-%02d",
+							tm.tm_year, tm.tm_mon, tm.tm_mday)));
+		tm.tm_year = year;
 	}
 
index 2bcc185708..a9daa5c591 100644
 
 #include "catalog/pg_collation.h"
 #include "catalog/pg_type.h"
+#include "common/int.h"
 #include "common/unicode_case.h"
 #include "common/unicode_category.h"
 #include "mb/pg_wchar.h"
@@ -3826,7 +3827,14 @@ DCH_from_char(FormatNode *node, const char *in, TmFromChar *out,
 						ereturn(escontext,,
 								(errcode(ERRCODE_INVALID_DATETIME_FORMAT),
 								 errmsg("invalid input string for \"Y,YYY\"")));
-					years += (millennia * 1000);
+
+					/* years += (millennia * 1000); */
+					if (pg_mul_s32_overflow(millennia, 1000, &millennia) ||
+						pg_add_s32_overflow(years, millennia, &years))
+						ereturn(escontext,,
+								(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+								 errmsg("value for \"Y,YYY\" in source string is out of range")));
+
 					if (!from_char_set_int(&out->year, years, n, escontext))
 						return;
 					out->yysz = 4;
@@ -4785,10 +4793,35 @@ do_to_timestamp(text *date_txt, text *fmt, Oid collid, bool std,
 			tm->tm_year = tmfc.year % 100;
 			if (tm->tm_year)
 			{
+				int			tmp;
+
 				if (tmfc.cc >= 0)
-					tm->tm_year += (tmfc.cc - 1) * 100;
+				{
+					/* tm->tm_year += (tmfc.cc - 1) * 100; */
+					tmp = tmfc.cc - 1;
+					if (pg_mul_s32_overflow(tmp, 100, &tmp) ||
+						pg_add_s32_overflow(tm->tm_year, tmp, &tm->tm_year))
+					{
+						DateTimeParseError(DTERR_FIELD_OVERFLOW, NULL,
+										   text_to_cstring(date_txt), "timestamp",
+										   escontext);
+						goto fail;
+					}
+				}
 				else
-					tm->tm_year = (tmfc.cc + 1) * 100 - tm->tm_year + 1;
+				{
+					/* tm->tm_year = (tmfc.cc + 1) * 100 - tm->tm_year + 1; */
+					tmp = tmfc.cc + 1;
+					if (pg_mul_s32_overflow(tmp, 100, &tmp) ||
+						pg_sub_s32_overflow(tmp, tm->tm_year, &tmp) ||
+						pg_add_s32_overflow(tmp, 1, &tm->tm_year))
+					{
+						DateTimeParseError(DTERR_FIELD_OVERFLOW, NULL,
+										   text_to_cstring(date_txt), "timestamp",
+										   escontext);
+						goto fail;
+					}
+				}
 			}
 			else
 			{
@@ -4814,11 +4847,31 @@ do_to_timestamp(text *date_txt, text *fmt, Oid collid, bool std,
 		if (tmfc.bc)
 			tmfc.cc = -tmfc.cc;
 		if (tmfc.cc >= 0)
+		{
 			/* +1 because 21st century started in 2001 */
-			tm->tm_year = (tmfc.cc - 1) * 100 + 1;
+			/* tm->tm_year = (tmfc.cc - 1) * 100 + 1; */
+			if (pg_mul_s32_overflow(tmfc.cc - 1, 100, &tm->tm_year) ||
+				pg_add_s32_overflow(tm->tm_year, 1, &tm->tm_year))
+			{
+				DateTimeParseError(DTERR_FIELD_OVERFLOW, NULL,
+								   text_to_cstring(date_txt), "timestamp",
+								   escontext);
+				goto fail;
+			}
+		}
 		else
+		{
 			/* +1 because year == 599 is 600 BC */
-			tm->tm_year = tmfc.cc * 100 + 1;
+			/* tm->tm_year = tmfc.cc * 100 + 1; */
+			if (pg_mul_s32_overflow(tmfc.cc, 100, &tm->tm_year) ||
+				pg_add_s32_overflow(tm->tm_year, 1, &tm->tm_year))
+			{
+				DateTimeParseError(DTERR_FIELD_OVERFLOW, NULL,
+								   text_to_cstring(date_txt), "timestamp",
+								   escontext);
+				goto fail;
+			}
+		}
 		fmask |= DTK_M(YEAR);
 	}
 
@@ -4843,11 +4896,35 @@ do_to_timestamp(text *date_txt, text *fmt, Oid collid, bool std,
 			fmask |= DTK_DATE_M;
 		}
 		else
-			tmfc.ddd = (tmfc.ww - 1) * 7 + 1;
+		{
+			int			tmp = 0;
+
+			/* tmfc.ddd = (tmfc.ww - 1) * 7 + 1; */
+			if (pg_sub_s32_overflow(tmfc.ww, 1, &tmp) ||
+				pg_mul_s32_overflow(tmp, 7, &tmp) ||
+				pg_add_s32_overflow(tmp, 1, &tmfc.ddd))
+			{
+				DateTimeParseError(DTERR_FIELD_OVERFLOW, NULL,
+								   date_str, "timestamp", escontext);
+				goto fail;
+			}
+		}
 	}
 
 	if (tmfc.w)
-		tmfc.dd = (tmfc.w - 1) * 7 + 1;
+	{
+		int			tmp = 0;
+
+		/* tmfc.dd = (tmfc.w - 1) * 7 + 1; */
+		if (pg_sub_s32_overflow(tmfc.w, 1, &tmp) ||
+			pg_mul_s32_overflow(tmp, 7, &tmp) ||
+			pg_add_s32_overflow(tmp, 1, &tmfc.dd))
+		{
+			DateTimeParseError(DTERR_FIELD_OVERFLOW, NULL,
+							   date_str, "timestamp", escontext);
+			goto fail;
+		}
+	}
 	if (tmfc.dd)
 	{
 		tm->tm_mday = tmfc.dd;
@@ -4912,7 +4989,18 @@ do_to_timestamp(text *date_txt, text *fmt, Oid collid, bool std,
 	}
 
 	if (tmfc.ms)
-		*fsec += tmfc.ms * 1000;
+	{
+		int			tmp = 0;
+
+		/* *fsec += tmfc.ms * 1000; */
+		if (pg_mul_s32_overflow(tmfc.ms, 1000, &tmp) ||
+			pg_add_s32_overflow(*fsec, tmp, fsec))
+		{
+			DateTimeParseError(DTERR_FIELD_OVERFLOW, NULL,
+							   date_str, "timestamp", escontext);
+			goto fail;
+		}
+	}
 	if (tmfc.us)
 		*fsec += tmfc.us;
 	if (fprec)
diff --git a/src/include/common/int.h b/src/include/common/int.h
index 3b1590d676..6b50aa67b9 100644
--- a/src/include/common/int.h
+++ b/src/include/common/int.h
@@ -117,6 +117,22 @@ pg_mul_s16_overflow(int16 a, int16 b, int16 *result)
 #endif
 }
 
+static inline bool
 
+static inline bool
 
+static inline bool
+pg_neg_s64_overflow(int64 a, int64 *result)
+{
+#if defined(HAVE__BUILTIN_OP_OVERFLOW)
+	return __builtin_sub_overflow(0, a, result);
+#else
+	if (unlikely(a == PG_INT64_MIN))
+	{
+		*result = 0x5EED;		/* to avoid spurious warnings */
+		return true;
+	}
+	*result = -a;
+	return false;
+#endif
+}
+
 static inline uint64
 pg_abs_s64(int64 a)
 {
diff --git a/src/test/regress/expected/date.out b/src/test/regress/expected/date.out
index c9cec70c38..dcab9e76f4 100644
--- a/src/test/regress/expected/date.out
+++ b/src/test/regress/expected/date.out
@@ -1528,6 +1528,8 @@ select make_date(2013, 13, 1);
 ERROR:  date field value out of range: 2013-13-01
 select make_date(2013, 11, -1);
 ERROR:  date field value out of range: 2013-11--1
+SELECT make_date(-2147483648, 1, 1);
+ERROR:  date field value out of range: -2147483648-01-01
 select make_time(10, 55, 100.1);
 ERROR:  time field value out of range: 10:55:100.1
 select make_time(24, 0, 2.1);
diff --git a/src/test/regress/expected/horology.out b/src/test/regress/expected/horology.out
index 6d7dd5c988..b87b88f038 100644
--- a/src/test/regress/expected/horology.out
+++ b/src/test/regress/expected/horology.out
@@ -3743,6 +3743,14 @@ SELECT to_timestamp('2015-02-11 86000', 'YYYY-MM-DD SSSSS');  -- ok
 
 SELECT to_timestamp('2015-02-11 86400', 'YYYY-MM-DD SSSSS');
 ERROR:  date/time field value out of range: "2015-02-11 86400"
+SELECT to_timestamp('1000000000,999', 'Y,YYY');
+ERROR:  value for "Y,YYY" in source string is out of range
+SELECT to_timestamp('0.-2147483648', 'SS.MS');
+ERROR:  date/time field value out of range: "0.-2147483648"
+SELECT to_timestamp('613566758', 'W');
+ERROR:  date/time field value out of range: "613566758"
+SELECT to_timestamp('2024 613566758 1', 'YYYY WW D');
+ERROR:  date/time field value out of range: "2024 613566758 1"
 SELECT to_date('2016-13-10', 'YYYY-MM-DD');
 ERROR:  date/time field value out of range: "2016-13-10"
 SELECT to_date('2016-02-30', 'YYYY-MM-DD');
@@ -3783,6 +3791,14 @@ SELECT to_date('0000-02-01','YYYY-MM-DD');  -- allowed, though it shouldn't be
  02-01-0001 BC
 (1 row)
 
+SELECT to_date('100000000', 'CC');
+ERROR:  date/time field value out of range: "100000000"
+SELECT to_date('-100000000', 'CC');
+ERROR:  date/time field value out of range: "-100000000"
+SELECT to_date('-2147483648 01', 'CC YY');
+ERROR:  date/time field value out of range: "-2147483648 01"
+SELECT to_date('2147483647 01', 'CC YY');
+ERROR:  date/time field value out of range: "2147483647 01"
 -- to_char's TZ format code produces zone abbrev if known
 SELECT to_char('2012-12-12 12:00'::timestamptz, 'YYYY-MM-DD HH:MI:SS TZ');
          to_char         
diff --git a/src/test/regress/sql/date.sql b/src/test/regress/sql/date.sql
index 1c58ff6966..805aec706c 100644
--- a/src/test/regress/sql/date.sql
+++ b/src/test/regress/sql/date.sql
@@ -371,5 +371,6 @@ select make_date(0, 7, 15);
 select make_date(2013, 2, 30);
 select make_date(2013, 13, 1);
 select make_date(2013, 11, -1);
+SELECT make_date(-2147483648, 1, 1);
 select make_time(10, 55, 100.1);
 select make_time(24, 0, 2.1);
diff --git a/src/test/regress/sql/horology.sql b/src/test/regress/sql/horology.sql
index 0fe3c783e6..808083a6d8 100644
--- a/src/test/regress/sql/horology.sql
+++ b/src/test/regress/sql/horology.sql
@@ -650,6 +650,10 @@ SELECT to_timestamp('2015-02-11 86000', 'YYYY-MM-DD SSSS');  -- ok
 SELECT to_timestamp('2015-02-11 86400', 'YYYY-MM-DD SSSS');
 SELECT to_timestamp('2015-02-11 86000', 'YYYY-MM-DD SSSSS');  -- ok
 SELECT to_timestamp('2015-02-11 86400', 'YYYY-MM-DD SSSSS');
+SELECT to_timestamp('1000000000,999', 'Y,YYY');
+SELECT to_timestamp('0.-2147483648', 'SS.MS');
+SELECT to_timestamp('613566758', 'W');
+SELECT to_timestamp('2024 613566758 1', 'YYYY WW D');
 SELECT to_date('2016-13-10', 'YYYY-MM-DD');
 SELECT to_date('2016-02-30', 'YYYY-MM-DD');
 SELECT to_date('2016-02-29', 'YYYY-MM-DD');  -- ok
@@ -660,6 +664,10 @@ SELECT to_date('2016 365', 'YYYY DDD');  -- ok
 SELECT to_date('2016 366', 'YYYY DDD');  -- ok
 SELECT to_date('2016 367', 'YYYY DDD');
 SELECT to_date('0000-02-01','YYYY-MM-DD');  -- allowed, though it shouldn't be
+SELECT to_date('100000000', 'CC');
+SELECT to_date('-100000000', 'CC');
+SELECT to_date('-2147483648 01', 'CC YY');
+SELECT to_date('2147483647 01', 'CC YY');

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 6 Dec 2024 10:22:44 -0600

> is the "ISO week" one (case 7).

The overflow hazard for the ISO week case seems to be in isoweek2j(), which
would require quite a bit of restructuring to work with the soft-error
handling from its callers.  I'm not feeling particularly inspired to do
that, so here's a v29 with a comment added above that function.

-- 
nathan
>From 99032579840c75c15acb88e6d9fb9a8a76a39dcd Mon Sep 17 00:00:00 2001
From: Nathan Bossart <[email-masked]>
Date: Fri, 6 Dec 2024 10:16:51 -0600
Subject: [PATCH v29 1/1] Fix various overflow hazards in date and timestamp
 functions.

Backpatch-through: 13
 src/backend/utils/adt/timestamp.c      |   4 +
 8 files changed, 183 insertions(+), 9 deletions(-)

 

 	if (tmfc.w)
 
 	if (tmfc.ms)
-		*fsec += tmfc.ms * 1000;
+	{
+		int			tmp = 0;
+
+		/* *fsec += tmfc.ms * 1000; */
+		if (pg_mul_s32_overflow(tmfc.ms, 1000, &tmp) ||
+			pg_add_s32_overflow(*fsec, tmp, fsec))
+		{
+			DateTimeParseError(DTERR_FIELD_OVERFLOW, NULL,
+							   date_str, "timestamp", escontext);
+			goto fail;
+		}
+	}
 	if (tmfc.us)
 		*fsec += tmfc.us;
 	if (fprec)
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 57fcfefdaf..18d7d8a108 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -5104,6 +5104,10 @@ interval_trunc(PG_FUNCTION_ARGS)
  *
  *	Return the Julian day which corresponds to the first day (Monday) of the given ISO 8601 year and week.
  *	Julian days are used to convert between ISO week dates and Gregorian dates.
+ *
+ *	XXX: This function has integer overflow hazards, but restructuring it to
+ *	work with the soft-error handling that its callers do is likely more
+ *	trouble than it's worth.
  */
 int
 isoweek2j(int year, int week)
diff --git a/src/include/common/int.h b/src/include/common/int.h
index 3b1590d676..6b50aa67b9 100644
--- a/src/include/common/int.h
+++ b/src/include/common/int.h
@@ -117,6 +117,22 @@ pg_mul_s16_overflow(int16 a, int16 b, int16 *result)
 #endif
 }
 
+static inline bool
 
+static inline bool
 
+static inline bool