Re: Remove dependence on integer wrapping
>
> On Mon, Jul 22, 2024 at 5:52 PM Alexander Lakhin
> wrote:
>
> > Also there are several trap-producing cases with date types:
> > SELECT to_date('1', 'CC');
>
> Hi, I’ve attached a patch that fixes the to_date() overflow. Patches 1
> through 3 remain unchanged.
>
> Thank you,
> Matthew Kim
>
v13-0003-Remove-dependence-on-integer-wrapping-for-jsonb.patch
Description: Binary data
v13-0004-Handle-overflows-in-do_to_timestamp.patch
Description: Binary data
v13-0001-Remove-dependence-on-integer-wrapping.patch
Description: Binary data
v13-0002-Remove-overflow-from-array_set_slice.patch
Description: Binary data
Re: Remove dependence on integer wrapping
On Mon, Jul 22, 2024 at 5:52 PM Alexander Lakhin wrote: > Also there are several trap-producing cases with date types: > SELECT make_date(-2147483648, 1, 1); Hi, I’ve attached a patch that fixes the make_date overflow. I’ve upgraded the patches accordingly to reflect Joseph’s committed fix. Thank you, Matthew Kim v16-0004-Handle-negative-years-overflow-in-make_date.patch Description: Binary data v16-0002-Remove-dependence-on-integer-wrapping-for-jsonb.patch Description: Binary data v16-0001-Remove-dependence-on-integer-wrapping.patch Description: Binary data v16-0003-Handle-overflows-in-do_to_timestamp.patch Description: Binary data
Re: Remove dependence on integer wrapping
I've updated patch 0004 to check the return value of pg_mul_s32_overflow().
Since tm.tm_year overflowed, the error message is hardcoded.
Thanks,
Matthew Kim
From c142581fb5f4a26de40cdca0d8ca7d39abdb2e15 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 6 Jul 2024 15:41:09 -0400
Subject: [PATCH 2/4] Remove dependence on integer wrapping for jsonb
This commit updates various jsonb operators and functions to no longer
rely on integer wrapping for correctness. Not all compilers support
-fwrapv, so it's best not to rely on it.
---
src/backend/utils/adt/jsonfuncs.c | 6 +++---
src/test/regress/expected/jsonb.out | 12
src/test/regress/sql/jsonb.sql | 2 ++
3 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index 48c3f88140..24b13a599f 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -946,7 +946,7 @@ jsonb_array_element(PG_FUNCTION_ARGS)
{
uint32 nelements = JB_ROOT_COUNT(jb);
- if (-element > nelements)
+ if (element == PG_INT32_MIN || -element > nelements)
PG_RETURN_NULL();
else
element += nelements;
@@ -5425,7 +5425,7 @@ setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
if (idx < 0)
{
- if (-idx > nelems)
+ if (idx == PG_INT32_MIN || -idx > nelems)
{
/*
* If asked to keep elements position consistent, it's not allowed
@@ -5437,7 +5437,7 @@ setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
errmsg("path element at position %d is out of range: %d",
level + 1, idx)));
else
-idx = INT_MIN;
+idx = PG_INT32_MIN;
}
else
idx = nelems + idx;
diff --git a/src/test/regress/expected/jsonb.out b/src/test/regress/expected/jsonb.out
index e66d760189..a9d93052fc 100644
--- a/src/test/regress/expected/jsonb.out
+++ b/src/test/regress/expected/jsonb.out
@@ -680,6 +680,18 @@ select '"foo"'::jsonb -> 'z';
(1 row)
+select '[]'::jsonb -> -2147483648;
+ ?column?
+--
+
+(1 row)
+
+select jsonb_delete_path('{"a":[]}', '{"a",-2147483648}');
+ jsonb_delete_path
+---
+ {"a": []}
+(1 row)
+
select '{"a": [{"b": "c"}, {"b": "cc"}]}'::jsonb ->> null::text;
?column?
--
diff --git a/src/test/regress/sql/jsonb.sql b/src/test/regress/sql/jsonb.sql
index 97bc2242a1..6a18577ead 100644
--- a/src/test/regress/sql/jsonb.sql
+++ b/src/test/regress/sql/jsonb.sql
@@ -204,6 +204,8 @@ select '[{"b": "c"}, {"b": "cc"}]'::jsonb -> 'z';
select '{"a": "c", "b": null}'::jsonb -> 'b';
select '"foo"'::jsonb -> 1;
select '"foo"'::jsonb -> 'z';
+select '[]'::jsonb -> -2147483648;
+select jsonb_delete_path('{"a":[]}', '{"a",-2147483648}');
select '{"a": [{"b": "c"}, {"b": "cc"}]}'::jsonb ->> null::text;
select '{"a": [{"b": "c"}, {"b": "cc"}]}'::jsonb ->> null::int;
--
2.39.3 (Apple Git-146)
From ceec949dd73b7344d9a459e20c48696f0b8a11c6 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 8 Jun 2024 22:16:46 -0400
Subject: [PATCH 1/4] Remove dependence on integer wrapping
This commit updates various parts of the code to no longer rely on
integer wrapping for correctness. Not all compilers support -fwrapv, so
it's best not to rely on it.
---
src/backend/utils/adt/cash.c | 14 ++--
src/backend/utils/adt/numeric.c| 5 +-
src/backend/utils/adt/numutils.c | 34 -
src/backend/utils/adt/timestamp.c | 28 +--
src/include/common/int.h | 86 ++
src/interfaces/ecpg/pgtypeslib/timestamp.c | 11 +--
src/test/regress/expected/timestamp.out| 13
src/test/regress/expected/timestamptz.out | 13
src/test/regress/sql/timestamp.sql | 4 +
src/test/regress/sql/timestamptz.sql | 4 +
10 files changed, 154 insertions(+), 58 deletions(-)
diff --git a/src/backend/utils/adt/cash.c b/src/backend/utils/adt/cash.c
index b20c358486..5cea5a1c86 100644
--- a/src/backend/utils/adt/cash.c
+++ b/src/backend/utils/adt/cash.c
@@ -387,6 +387,7 @@ Datum
cash_out(PG_FUNCTION_ARGS)
{
Cash value = PG_GETARG_CASH(0);
+ uint64 uvalue;
char *result;
char buf[128];
char *bufptr;
@@ -429,8 +430,6 @@ cash_out(PG_FUNCTION_ARGS)
if (value < 0)
{
- /* make the amount positive for digit-reconstruction loop */
- value = -value;
/* set up formatting data */
signsymb
Re: Remove dependence on integer wrapping
On Mon, Jul 22, 2024 at 5:52 PM Alexander Lakhin
wrote:
> Also there are several trap-producing cases with date types:
> SELECT to_timestamp('10,999', 'Y,YYY');
I attached patch 5 that fixes the to_timestamp overflow.
Thank you,
Matthew Kim
From a01c5d67894de89f14f0dfc54d32e92258a1a3a7 Mon Sep 17 00:00:00 2001
From: Matthew Kim <38759997+friendlymatthew@users.noreply.github.com>
Date: Tue, 9 Jul 2024 18:25:10 -0400
Subject: [PATCH 3/5] Handle overflows in do_to_timestamp().
This commit handles overflow when formatting timestamps with the 'CC'
pattern.
---
src/backend/utils/adt/formatting.c | 25 +++--
src/test/regress/expected/horology.out | 2 ++
src/test/regress/sql/horology.sql | 1 +
3 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
index 68069fcfd3..decf0b6123 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"
@@ -4797,11 +4798,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/horology.out b/src/test/regress/expected/horology.out
index 241713cc51..311c688f89 100644
--- a/src/test/regress/expected/horology.out
+++ b/src/test/regress/expected/horology.out
@@ -3778,6 +3778,8 @@ SELECT to_date('-02-01','-MM-DD'); -- allowed, though it shouldn't be
02-01-0001 BC
(1 row)
+SELECT to_date('1', 'CC');
+ERROR: date out of range: "1"
-- to_char's TZ format code produces zone abbrev if known
SELECT to_char('2012-12-12 12:00'::timestamptz, '-MM-DD HH:MI:SS TZ');
to_char
diff --git a/src/test/regress/sql/horology.sql b/src/test/regress/sql/horology.sql
index e5cf12ff63..12a035cf57 100644
--- a/src/test/regress/sql/horology.sql
+++ b/src/test/regress/sql/horology.sql
@@ -660,6 +660,7 @@ SELECT to_date('2016 365', ' DDD'); -- ok
SELECT to_date('2016 366', ' DDD'); -- ok
SELECT to_date('2016 367', ' DDD');
SELECT to_date('-02-01','-MM-DD'); -- allowed, though it shouldn't be
+SELECT to_date('1', 'CC');
-- to_char's TZ format code produces zone abbrev if known
SELECT to_char('2012-12-12 12:00'::timestamptz, '-MM-DD HH:MI:SS TZ');
--
2.39.3 (Apple Git-146)
From e22eb524a85dd3f2a363b61f5d91bfe34b0470d2 Mon Sep 17 00:00:00 2001
From: Matthew Kim <38759997+friendlymatthew@users.noreply.github.com>
Date: Wed, 7 Aug 2024 18:58:48 -0700
Subject: [PATCH 4/5] Handle overflow when taking absolute value of BC years
make_date formats negative years as BC years by taking the absolute value. This commit safely takes the absolute value of tm.tm_year
---
src/backend/utils/adt/date.c | 5 -
src/test/regress/expected/date.out | 2 ++
src/test/regress/sql/date.sql | 1 +
3 files changed, 7 insertions(+), 1 deletion(-)
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, f
