Re: Remove dependence on integer wrapping
On Thu, Jun 13, 2024 at 12:00 AM Alexander Lakhin
wrote:
> SELECT '[]'::jsonb -> -2147483648;
>
> #4 0x7efe232d67f3 in __GI_abort () at ./stdlib/abort.c:79
> #5 0x55e8fde9f211 in __negvsi2 ()
> #6 0x55e8fdcca62c in jsonb_array_element (fcinfo=0x55e8fec28220) at
jsonfuncs.c:948
>
> (gdb) f 6
> #6 0x55e14cb9362c in jsonb_array_element (fcinfo=0x55e14d493220) at
jsonfuncs.c:948
> 948 if (-element > nelements)
> (gdb) p element
> $1 = -2147483648
>
> ---
> SELECT jsonb_delete_path('{"a":[]}', '{"a",-2147483648}');
>
> #4 0x7f1873bef7f3 in __GI_abort () at ./stdlib/abort.c:79
> #5 0x564a009d2211 in __negvsi2 ()
> #6 0x564a00807c89 in setPathArray (it=0x7fff865c7380,
path_elems=0x564a017baf20, path_nulls=0x564a017baf40,
> path_len=2, st=0x7fff865c7388, level=1, newval=0x0, nelems=2,
op_type=2) at jsonfuncs.c:5407
>
> (gdb) f 6
> #6 0x55985e823c89 in setPathArray (it=0x7ffc22258fe0,
path_elems=0x559860286f20, path_nulls=0x559860286f40,
> path_len=2, st=0x7ffc22258fe8, level=1, newval=0x0, nelems=0,
op_type=2) at jsonfuncs.c:5407
> 5407if (-idx > nelems)
> (gdb) p idx
> $1 = -2147483648
I've added another patch, 0004, to resolve the jsonb wrap-arounds.
The other patches, 0001, 0002, and 0003 are unchanged but have their
version number incremented.
Thanks,
Joe Koshakow
From c8725de5f6e1ed476992c33f87b7e7c9475a952b Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 6 Jul 2024 15:41:09 -0400
Subject: [PATCH 4/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 | 4 ++--
src/test/regress/expected/jsonb.out | 12
src/test/regress/sql/jsonb.sql | 2 ++
3 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index 48c3f88140..8783c57303 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 == INT_MIN || -idx > nelems)
{
/*
* If asked to keep elements position consistent, it's not allowed
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.34.1
From f3747da14c887f97e50d9a3104881cbd3d5f60de 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 | 7 +-
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 | 105 +
src/interfaces/ecpg/pgtypeslib/timestamp.c | 1
Re: Remove dependence on integer wrapping
On Thu, Jun 13, 2024 at 12:00 AM Alexander Lakhin
wrote:
>
> And one more with array...
> CREATE TABLE t (ia int[]);
> INSERT INTO t(ia[2147483647:2147483647]) VALUES ('{}');
I've added another patch, 0003, to resolve this wrap-around. In fact I
discovered a bug that the following statement is accepted and inserts
an empty array into the table.
INSERT INTO t(ia[-2147483648:2147483647]) VALUES ('{}');
My patch resolves this bug as well.
The other patches, 0001 and 0002, are unchanged but have their version
number incremented.
As a reminder, 0001 is reviewed and waiting for v18 and a committer.
0002 and 0003 are unreviewed. So, I'm going to mark this as waiting for
a reviewer.
Thanks,
Joe Koshakow
From f3747da14c887f97e50d9a3104881cbd3d5f60de Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 8 Jun 2024 22:16:46 -0400
Subject: [PATCH 1/3] 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 | 7 +-
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 | 105 +
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, 169 insertions(+), 55 deletions(-)
diff --git a/src/backend/utils/adt/cash.c b/src/backend/utils/adt/cash.c
index 32fbad2f57..f6f095a57b 100644
--- a/src/backend/utils/adt/cash.c
+++ b/src/backend/utils/adt/cash.c
@@ -352,8 +352,11 @@ cash_out(PG_FUNCTION_ARGS)
if (value < 0)
{
- /* make the amount positive for digit-reconstruction loop */
- value = -value;
+ /*
+ * make the amount positive for digit-reconstruction loop, we can
+ * leave INT64_MIN unchanged
+ */
+ pg_neg_s64_overflow(value, );
/* set up formatting data */
signsymbol = (*lconvert->negative_sign != '\0') ? lconvert->negative_sign : "-";
sign_posn = lconvert->n_sign_posn;
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index 5510a203b0..4ea2d9b0b4 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -8110,15 +8110,14 @@ int64_to_numericvar(int64 val, NumericVar *var)
/* int64 can require at most 19 decimal digits; add one for safety */
alloc_var(var, 20 / DEC_DIGITS);
+ uval = pg_abs_s64(val);
if (val < 0)
{
var->sign = NUMERIC_NEG;
- uval = -val;
}
else
{
var->sign = NUMERIC_POS;
- uval = val;
}
var->dscale = 0;
if (val == 0)
@@ -11222,7 +11221,7 @@ power_var_int(const NumericVar *base, int exp, int exp_dscale,
* Now we can proceed with the multiplications.
*/
neg = (exp < 0);
- mask = abs(exp);
+ mask = pg_abs_s32(exp);
init_var(_prod);
set_var_from_var(base, _prod);
diff --git a/src/backend/utils/adt/numutils.c b/src/backend/utils/adt/numutils.c
index adc1e8a4cb..a3d7d6bf01 100644
--- a/src/backend/utils/adt/numutils.c
+++ b/src/backend/utils/adt/numutils.c
@@ -18,6 +18,7 @@
#include
#include
+#include "common/int.h"
#include "port/pg_bitutils.h"
#include "utils/builtins.h"
@@ -131,6 +132,7 @@ pg_strtoint16_safe(const char *s, Node *escontext)
uint16 tmp = 0;
bool neg = false;
unsigned char digit;
+ int16 result;
/*
* The majority of cases are likely to be base-10 digits without any
@@ -190,10 +192,9 @@ pg_strtoint16_safe(const char *s, Node *escontext)
if (neg)
{
- /* check the negative equivalent will fit without overflowing */
- if (unlikely(tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1))
+ if (pg_neg_u16_overflow(tmp, ))
goto out_of_range;
- return -((int16) tmp);
+ return result;
}
if (unlikely(tmp > PG_INT16_MAX))
@@ -333,10 +334,9 @@ slow:
if (neg)
{
- /* check the negative equivalent will fit without overflowing */
- if (tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1)
+ if (pg_neg_u16_overflow(tmp, ))
goto out_of_range;
- return -((int16) tmp);
+ return result;
}
if (tmp > PG_INT16_MAX)
@@ -393,6 +393,7 @@ pg_strtoint32_safe(const char *s, Node *escontext)
uint32 tmp = 0;
bool neg = false;
unsigned char digit;
+ int32 result;
/*
* The majority of cases are likely to be base-10 digits without any
@@ -452,10 +453,9 @@ pg_strtoint32_safe(const char *s, Node *escontext)
if (neg)
{
- /* check the negative equivalent will fit without overflowing */
- if (unlikely(tmp > (uint32) (-(PG_INT32_MIN + 1)) + 1))
+ if (pg_neg_u32_overflow(tmp, ))
goto out_of_range;
- return -((int32) tmp);
+ return result;
}
Re: Remove dependence on integer wrapping
On Thu, Jun 13, 2024 at 10:56 PM Joseph Koshakow wrote:
On Thu, Jun 13, 2024 at 10:48 PM Joseph Koshakow
wrote:
>I've attached
>v4-0002-Handle-overflow-in-money-arithmetic.patch which adds some
>overflow checks and tests. I didn't address the float
multiplication
>because I didn't see any helper methods in int.h. I did some some
>useful helpers in float.h, but they raise an error directly instead
>of returning a bool. Would those be appropriate for use with the
>money type? If not I can refactor out the inner parts into a new
method
>that returns a bool.
>v4-0001-Remove-dependence-on-integer-wrapping.patch is unchanged, I
>just incremented the version number.
I added overflow handling for float arithmetic to the `money` type.
v6-0002-Handle-overflow-in-money-arithmetic.patch is ready for review.
v6-0001-Remove-dependence-on-integer-wrapping.patch is unchanged, I
just incremented the version number.
Thanks,
Joe Koshakow
From 6eec604618ee76227ee33fcddcc121d9915ff0ab Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 8 Jun 2024 22:16:46 -0400
Subject: [PATCH 1/2] 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 | 7 +-
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 | 105 +
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, 169 insertions(+), 55 deletions(-)
diff --git a/src/backend/utils/adt/cash.c b/src/backend/utils/adt/cash.c
index 32fbad2f57..f6f095a57b 100644
--- a/src/backend/utils/adt/cash.c
+++ b/src/backend/utils/adt/cash.c
@@ -352,8 +352,11 @@ cash_out(PG_FUNCTION_ARGS)
if (value < 0)
{
- /* make the amount positive for digit-reconstruction loop */
- value = -value;
+ /*
+ * make the amount positive for digit-reconstruction loop, we can
+ * leave INT64_MIN unchanged
+ */
+ pg_neg_s64_overflow(value, );
/* set up formatting data */
signsymbol = (*lconvert->negative_sign != '\0') ? lconvert->negative_sign : "-";
sign_posn = lconvert->n_sign_posn;
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index 5510a203b0..4ea2d9b0b4 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -8110,15 +8110,14 @@ int64_to_numericvar(int64 val, NumericVar *var)
/* int64 can require at most 19 decimal digits; add one for safety */
alloc_var(var, 20 / DEC_DIGITS);
+ uval = pg_abs_s64(val);
if (val < 0)
{
var->sign = NUMERIC_NEG;
- uval = -val;
}
else
{
var->sign = NUMERIC_POS;
- uval = val;
}
var->dscale = 0;
if (val == 0)
@@ -11222,7 +11221,7 @@ power_var_int(const NumericVar *base, int exp, int exp_dscale,
* Now we can proceed with the multiplications.
*/
neg = (exp < 0);
- mask = abs(exp);
+ mask = pg_abs_s32(exp);
init_var(_prod);
set_var_from_var(base, _prod);
diff --git a/src/backend/utils/adt/numutils.c b/src/backend/utils/adt/numutils.c
index adc1e8a4cb..a3d7d6bf01 100644
--- a/src/backend/utils/adt/numutils.c
+++ b/src/backend/utils/adt/numutils.c
@@ -18,6 +18,7 @@
#include
#include
+#include "common/int.h"
#include "port/pg_bitutils.h"
#include "utils/builtins.h"
@@ -131,6 +132,7 @@ pg_strtoint16_safe(const char *s, Node *escontext)
uint16 tmp = 0;
bool neg = false;
unsigned char digit;
+ int16 result;
/*
* The majority of cases are likely to be base-10 digits without any
@@ -190,10 +192,9 @@ pg_strtoint16_safe(const char *s, Node *escontext)
if (neg)
{
- /* check the negative equivalent will fit without overflowing */
- if (unlikely(tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1))
+ if (pg_neg_u16_overflow(tmp, ))
goto out_of_range;
- return -((int16) tmp);
+ return result;
}
if (unlikely(tmp > PG_INT16_MAX))
@@ -333,10 +334,9 @@ slow:
if (neg)
{
- /* check the negative equivalent will fit without overflowing */
- if (tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1)
+ if (pg_neg_u16_overflow(tmp, ))
goto out_of_range;
- return -((int16) tmp);
+ return result;
}
if (tmp > PG_INT16_MAX)
@@ -393,6 +393,7 @@ pg_strtoint32_safe(const char *s, Node *escontext)
uint32 tmp = 0;
bool neg = false;
unsigned char digit;
+ int32 result;
/*
* The majority of cases are likely to be base-10 digits wit
Re: Remove dependence on integer wrapping
14.06.2024 05:48, Joseph Koshakow wrote:
v4-0001-Remove-dependence-on-integer-wrapping.patch is unchanged, I
just incremented the version number.
> Also there are several trap-producing cases with date types:
> SELECT to_date('1', 'CC');
> SELECT to_timestamp('10,999', 'Y,YYY');
> SELECT make_date(-2147483648, 1, 1);
>
> And one more with array...
> CREATE TABLE t (ia int[]);
> INSERT INTO t(ia[2147483647:2147483647]) VALUES ('{}');
I'll try and get patches to address these too in the next couple of
weeks unless someone beats me to it.
> I think it's not the whole iceberg too.
+1
After sending my message, I toyed with -ftrapv a little time more and
found other cases:
SELECT '[]'::jsonb -> -2147483648;
#4 0x7efe232d67f3 in __GI_abort () at ./stdlib/abort.c:79
#5 0x55e8fde9f211 in __negvsi2 ()
#6 0x55e8fdcca62c in jsonb_array_element (fcinfo=0x55e8fec28220) at
jsonfuncs.c:948
(gdb) f 6
#6 0x55e14cb9362c in jsonb_array_element (fcinfo=0x55e14d493220) at
jsonfuncs.c:948
948 if (-element > nelements)
(gdb) p element
$1 = -2147483648
---
SELECT jsonb_delete_path('{"a":[]}', '{"a",-2147483648}');
#4 0x7f1873bef7f3 in __GI_abort () at ./stdlib/abort.c:79
#5 0x564a009d2211 in __negvsi2 ()
#6 0x564a00807c89 in setPathArray (it=0x7fff865c7380,
path_elems=0x564a017baf20, path_nulls=0x564a017baf40,
path_len=2, st=0x7fff865c7388, level=1, newval=0x0, nelems=2, op_type=2) at
jsonfuncs.c:5407
(gdb) f 6
#6 0x55985e823c89 in setPathArray (it=0x7ffc22258fe0,
path_elems=0x559860286f20, path_nulls=0x559860286f40,
path_len=2, st=0x7ffc22258fe8, level=1, newval=0x0, nelems=0, op_type=2) at
jsonfuncs.c:5407
5407 if (-idx > nelems)
(gdb) p idx
$1 = -2147483648
---
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 0x7f57f0bef7f3 in __GI_abort () at ./stdlib/abort.c:79
#5 0x7f57f1671351 in __addvsi3 () from .../src/test/regress/refint.so
#6 0x7f57f1670234 in check_foreign_key (fcinfo=0x7ffebf523650) at
refint.c:321
(gdb) f 6
#6 0x7f3400ef9234 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
---
And the most interesting case to me:
SET temp_buffers TO 10;
CREATE TEMP TABLE t(i int PRIMARY KEY);
INSERT INTO t VALUES(1);
#4 0x7f385cdd37f3 in __GI_abort () at ./stdlib/abort.c:79
#5 0x5620071c4f51 in __addvsi3 ()
#6 0x562007143f3c in init_htab (hashp=0x562008facb20, nelem=610070812) at
dynahash.c:720
(gdb) f 6
#6 0x560915207f3c in init_htab (hashp=0x560916039930, nelem=10) at
dynahash.c:720
720 hctl->high_mask = (nbuckets << 1) - 1;
(gdb) p nbuckets
$1 = 1073741824
Best regards,
Alexander
Re: Remove dependence on integer wrapping
RROR: money out of range
+SELECT '92233720368547758.07'::money * 2::int2;
+ERROR: money out of range
+SELECT '-92233720368547758.08'::money * 2::int2;
+ERROR: money out of range
+SELECT 2::int2 * '92233720368547758.07'::money ;
+ERROR: money out of range
+SELECT 2::int2 * '-92233720368547758.08'::money;
+ERROR: money out of range
diff --git a/src/test/regress/sql/money.sql b/src/test/regress/sql/money.sql
index 81c92dd960..36b2e029fd 100644
--- a/src/test/regress/sql/money.sql
+++ b/src/test/regress/sql/money.sql
@@ -135,3 +135,19 @@ SELECT '12345678901234567'::money::numeric;
SELECT '-12345678901234567'::money::numeric;
SELECT '92233720368547758.07'::money::numeric;
SELECT '-92233720368547758.08'::money::numeric;
+
+-- Test overflow checks
+SELECT '92233720368547758.07'::money + '0.01'::money;
+SELECT '-92233720368547758.08'::money - '0.01'::money;
+SELECT '92233720368547758.07'::money * 2::int8;
+SELECT '-92233720368547758.08'::money * 2::int8;
+SELECT 2::int8 * '92233720368547758.07'::money ;
+SELECT 2::int8 * '-92233720368547758.08'::money;
+SELECT '92233720368547758.07'::money * 2::int4;
+SELECT '-92233720368547758.08'::money * 2::int4;
+SELECT 2::int4 * '92233720368547758.07'::money ;
+SELECT 2::int4 * '-92233720368547758.08'::money;
+SELECT '92233720368547758.07'::money * 2::int2;
+SELECT '-92233720368547758.08'::money * 2::int2;
+SELECT 2::int2 * '92233720368547758.07'::money ;
+SELECT 2::int2 * '-92233720368547758.08'::money;
\ No newline at end of file
--
2.34.1
From 31e8de30a82e60151848439143169e562bc848a3 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 8 Jun 2024 22:16:46 -0400
Subject: [PATCH 1/2] 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 | 7 +-
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 | 105 +
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, 169 insertions(+), 55 deletions(-)
diff --git a/src/backend/utils/adt/cash.c b/src/backend/utils/adt/cash.c
index 32fbad2f57..f6f095a57b 100644
--- a/src/backend/utils/adt/cash.c
+++ b/src/backend/utils/adt/cash.c
@@ -352,8 +352,11 @@ cash_out(PG_FUNCTION_ARGS)
if (value < 0)
{
- /* make the amount positive for digit-reconstruction loop */
- value = -value;
+ /*
+ * make the amount positive for digit-reconstruction loop, we can
+ * leave INT64_MIN unchanged
+ */
+ pg_neg_s64_overflow(value, );
/* set up formatting data */
signsymbol = (*lconvert->negative_sign != '\0') ? lconvert->negative_sign : "-";
sign_posn = lconvert->n_sign_posn;
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index 5510a203b0..4ea2d9b0b4 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -8110,15 +8110,14 @@ int64_to_numericvar(int64 val, NumericVar *var)
/* int64 can require at most 19 decimal digits; add one for safety */
alloc_var(var, 20 / DEC_DIGITS);
+ uval = pg_abs_s64(val);
if (val < 0)
{
var->sign = NUMERIC_NEG;
- uval = -val;
}
else
{
var->sign = NUMERIC_POS;
- uval = val;
}
var->dscale = 0;
if (val == 0)
@@ -11222,7 +11221,7 @@ power_var_int(const NumericVar *base, int exp, int exp_dscale,
* Now we can proceed with the multiplications.
*/
neg = (exp < 0);
- mask = abs(exp);
+ mask = pg_abs_s32(exp);
init_var(_prod);
set_var_from_var(base, _prod);
diff --git a/src/backend/utils/adt/numutils.c b/src/backend/utils/adt/numutils.c
index adc1e8a4cb..a3d7d6bf01 100644
--- a/src/backend/utils/adt/numutils.c
+++ b/src/backend/utils/adt/numutils.c
@@ -18,6 +18,7 @@
#include
#include
+#include "common/int.h"
#include "port/pg_bitutils.h"
#include "utils/builtins.h"
@@ -131,6 +132,7 @@ pg_strtoint16_safe(const char *s, Node *escontext)
uint16 tmp = 0;
bool neg = false;
unsigned char digit;
+ int16 result;
/*
* The majority of cases are likely to be base-10 digits without any
@@ -190,10 +192,9 @@ pg_strtoint16_safe(const char *s, Node *escontext)
if (neg)
{
- /* check the negative equivalent will fit without overflowing */
- if (unlikely(tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1))
+ if (pg_neg_u16_overflow(tmp, ))
goto out_of_range;
- return -((int16) tmp);
+ return result;
}
if (unlikely(tmp > PG_INT16_MAX))
@@ -333,10 +334,9 @@ slow:
if (neg)
{
- /* check the negative equivalen
Re: Remove dependence on integer wrapping
On Thu, Jun 13, 2024 at 12:00 AM Alexander Lakhin
wrote:
>
>Let me remind you of bug #18240. Yes, that was about float8, but with
>-ftrapv we can get into the trap with:
>SELECT 1_000_000_000::money * 1_000_000_000::int;
>server closed the connection unexpectedly
Interesting, it looks like there's no overflow handling of any money
arithmetic. I've attached
v4-0002-Handle-overflow-in-money-arithmetic.patch which adds some
overflow checks and tests. I didn't address the float multiplication
because I didn't see any helper methods in int.h. I did some some
useful helpers in float.h, but they raise an error directly instead
of returning a bool. Would those be appropriate for use with the
money type? If not I can refactor out the inner parts into a new method
that returns a bool.
v4-0001-Remove-dependence-on-integer-wrapping.patch is unchanged, I
just incremented the version number.
>Also there are several trap-producing cases with date types:
>SELECT to_date('1', 'CC');
>SELECT to_timestamp('10,999', 'Y,YYY');
>SELECT make_date(-2147483648, 1, 1);
>
>And one more with array...
>CREATE TABLE t (ia int[]);
>INSERT INTO t(ia[2147483647:2147483647]) VALUES ('{}');
I'll try and get patches to address these too in the next couple of
weeks unless someone beats me to it.
>I think it's not the whole iceberg too.
+1
Thanks,
Joe Koshakow
From 31e8de30a82e60151848439143169e562bc848a3 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 8 Jun 2024 22:16:46 -0400
Subject: [PATCH 1/2] 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 | 7 +-
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 | 105 +
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, 169 insertions(+), 55 deletions(-)
diff --git a/src/backend/utils/adt/cash.c b/src/backend/utils/adt/cash.c
index 32fbad2f57..f6f095a57b 100644
--- a/src/backend/utils/adt/cash.c
+++ b/src/backend/utils/adt/cash.c
@@ -352,8 +352,11 @@ cash_out(PG_FUNCTION_ARGS)
if (value < 0)
{
- /* make the amount positive for digit-reconstruction loop */
- value = -value;
+ /*
+ * make the amount positive for digit-reconstruction loop, we can
+ * leave INT64_MIN unchanged
+ */
+ pg_neg_s64_overflow(value, );
/* set up formatting data */
signsymbol = (*lconvert->negative_sign != '\0') ? lconvert->negative_sign : "-";
sign_posn = lconvert->n_sign_posn;
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index 5510a203b0..4ea2d9b0b4 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -8110,15 +8110,14 @@ int64_to_numericvar(int64 val, NumericVar *var)
/* int64 can require at most 19 decimal digits; add one for safety */
alloc_var(var, 20 / DEC_DIGITS);
+ uval = pg_abs_s64(val);
if (val < 0)
{
var->sign = NUMERIC_NEG;
- uval = -val;
}
else
{
var->sign = NUMERIC_POS;
- uval = val;
}
var->dscale = 0;
if (val == 0)
@@ -11222,7 +11221,7 @@ power_var_int(const NumericVar *base, int exp, int exp_dscale,
* Now we can proceed with the multiplications.
*/
neg = (exp < 0);
- mask = abs(exp);
+ mask = pg_abs_s32(exp);
init_var(_prod);
set_var_from_var(base, _prod);
diff --git a/src/backend/utils/adt/numutils.c b/src/backend/utils/adt/numutils.c
index adc1e8a4cb..a3d7d6bf01 100644
--- a/src/backend/utils/adt/numutils.c
+++ b/src/backend/utils/adt/numutils.c
@@ -18,6 +18,7 @@
#include
#include
+#include "common/int.h"
#include "port/pg_bitutils.h"
#include "utils/builtins.h"
@@ -131,6 +132,7 @@ pg_strtoint16_safe(const char *s, Node *escontext)
uint16 tmp = 0;
bool neg = false;
unsigned char digit;
+ int16 result;
/*
* The majority of cases are likely to be base-10 digits without any
@@ -190,10 +192,9 @@ pg_strtoint16_safe(const char *s, Node *escontext)
if (neg)
{
- /* check the negative equivalent will fit without overflowing */
- if (unlikely(tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1))
+ if (pg_neg_u16_overflow(tmp, ))
goto out_of_range;
- return -((int16) tmp);
+ return result;
}
if (unlikely(tmp > PG_INT16_MAX))
@@ -333,10 +334,9 @@ slow:
if (neg)
{
- /* check the negative equivalent will fit without overflowing */
- if (tmp
Re: Remove dependence on integer wrapping
10.06.2024 04:57, Tom Lane wrote:
BTW, while I approve of trying to get rid of our need for -fwrapv,
I'm quite scared of actually doing it. Whatever cases you may have
discovered by running the regression tests will be at best the
tip of the iceberg. Is there any chance of using static analysis
to find all the places of concern?
Let me remind you of bug #18240. Yes, that was about float8, but with
-ftrapv we can get into the trap with:
SELECT 1_000_000_000::money * 1_000_000_000::int;
server closed the connection unexpectedly
Also there are several trap-producing cases with date types:
SELECT to_date('1', 'CC');
SELECT to_timestamp('10,999', 'Y,YYY');
SELECT make_date(-2147483648, 1, 1);
And one more with array...
CREATE TABLE t (ia int[]);
INSERT INTO t(ia[2147483647:2147483647]) VALUES ('{}');
I think it's not the whole iceberg too.
Best regards,
Alexander
Re: Remove dependence on integer wrapping
On Mon, Jun 10, 2024 at 2:30 PM Andres Freund wrote: > On 2024-06-09 21:57:54 -0400, Tom Lane wrote: > > BTW, while I approve of trying to get rid of our need for -fwrapv, > > I'm quite scared of actually doing it. IMV it's perfectly fine to defensively assume that we need -fwrapv, even given a total lack of evidence that removing it would cause harm. Doing it to "be more in line with the standard" is a terrible argument. > I think that's a quite fair concern. One potentially relevant datapoint is > that we actually don't have -fwrapv equivalent on all platforms, and I don't > recall a lot of complaints from windows users. That might just be because MSVC inherently doesn't do optimizations that rely on signed wrap being undefined behavior. Last I checked MSVC just doesn't rely on the standard's strict aliasing rules, even as an opt-in thing. > Of course it's quite possible > that they'd never notice... Removing -fwrapv is something that I see as a potential optimization. It should be justified in about the same way as any other optimization. I suspect that it doesn't actually have any clear benefits for us. But if I'm wrong about that then the benefit might still be achievable through other means (something far short of just removing -fwrapv globally). > I think this is a good argument for enabling -ftrapv in development > builds. That gives us at least a *chance* of seeing these issues. +1. I'm definitely prepared to say that code that actively relies on -fwrapv is broken code. -- Peter Geoghegan
Re: Remove dependence on integer wrapping
On Wed, Jun 12, 2024 at 11:45:20AM -0400, Tom Lane wrote: > Nathan Bossart writes: >> Thanks. This looks pretty good to me after a skim, so I'll see about >> committing/back-patching it in the near future. IIUC there is likely more >> to come in this area, but I see no need to wait. > > Uh ... what of this would we back-patch? It seems like v18 > material to me. D'oh. I was under the impression that the numutils.c changes were arguably bug fixes. Even in that case, we should probably split out the other stuff for v18. But you're probably right that we could just wait for all of it. -- nathan
Re: Remove dependence on integer wrapping
Nathan Bossart writes: > Thanks. This looks pretty good to me after a skim, so I'll see about > committing/back-patching it in the near future. IIUC there is likely more > to come in this area, but I see no need to wait. Uh ... what of this would we back-patch? It seems like v18 material to me. regards, tom lane
Re: Remove dependence on integer wrapping
On Tue, Jun 11, 2024 at 09:10:44PM -0400, Joseph Koshakow wrote: > The attached patch has updated this file too. For some reason I was > under the impression that I should leave the ecpg/ files alone, though > I can't remember why. Thanks. This looks pretty good to me after a skim, so I'll see about committing/back-patching it in the near future. IIUC there is likely more to come in this area, but I see no need to wait. (I'm chuckling that we are adding Y2K tests in 2024...) -- nathan
Re: Remove dependence on integer wrapping
On Tue, Jun 11, 2024 at 12:22 PM Nathan Bossart
wrote:
>I personally find that much easier to read. Since the existing
open-coded
>overflow check is apparently insufficient, I think there's a reasonably
>strong case for centralizing this sort of thing so that we don't
continue
>to make the same mistakes.
Sounds good, the attached patch has these changes.
>tm2timestamp() in src/interfaces/ecpg/pgtypeslib/timestamp.c has the
same
>comment. The code there looks very similar to the code for
tm2timestamp()
>in the other timestamp.c...
The attached patch has updated this file too. For some reason I was
under the impression that I should leave the ecpg/ files alone, though
I can't remember why.
Thanks,
Joe Koshakow
From adcf89561cec31499754a7c04da50c408a12724a Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 8 Jun 2024 22:16:46 -0400
Subject: [PATCH] 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 | 7 +-
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 | 105 +
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, 169 insertions(+), 55 deletions(-)
diff --git a/src/backend/utils/adt/cash.c b/src/backend/utils/adt/cash.c
index 32fbad2f57..f6f095a57b 100644
--- a/src/backend/utils/adt/cash.c
+++ b/src/backend/utils/adt/cash.c
@@ -352,8 +352,11 @@ cash_out(PG_FUNCTION_ARGS)
if (value < 0)
{
- /* make the amount positive for digit-reconstruction loop */
- value = -value;
+ /*
+ * make the amount positive for digit-reconstruction loop, we can
+ * leave INT64_MIN unchanged
+ */
+ pg_neg_s64_overflow(value, );
/* set up formatting data */
signsymbol = (*lconvert->negative_sign != '\0') ? lconvert->negative_sign : "-";
sign_posn = lconvert->n_sign_posn;
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index 5510a203b0..4ea2d9b0b4 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -8110,15 +8110,14 @@ int64_to_numericvar(int64 val, NumericVar *var)
/* int64 can require at most 19 decimal digits; add one for safety */
alloc_var(var, 20 / DEC_DIGITS);
+ uval = pg_abs_s64(val);
if (val < 0)
{
var->sign = NUMERIC_NEG;
- uval = -val;
}
else
{
var->sign = NUMERIC_POS;
- uval = val;
}
var->dscale = 0;
if (val == 0)
@@ -11222,7 +11221,7 @@ power_var_int(const NumericVar *base, int exp, int exp_dscale,
* Now we can proceed with the multiplications.
*/
neg = (exp < 0);
- mask = abs(exp);
+ mask = pg_abs_s32(exp);
init_var(_prod);
set_var_from_var(base, _prod);
diff --git a/src/backend/utils/adt/numutils.c b/src/backend/utils/adt/numutils.c
index adc1e8a4cb..a3d7d6bf01 100644
--- a/src/backend/utils/adt/numutils.c
+++ b/src/backend/utils/adt/numutils.c
@@ -18,6 +18,7 @@
#include
#include
+#include "common/int.h"
#include "port/pg_bitutils.h"
#include "utils/builtins.h"
@@ -131,6 +132,7 @@ pg_strtoint16_safe(const char *s, Node *escontext)
uint16 tmp = 0;
bool neg = false;
unsigned char digit;
+ int16 result;
/*
* The majority of cases are likely to be base-10 digits without any
@@ -190,10 +192,9 @@ pg_strtoint16_safe(const char *s, Node *escontext)
if (neg)
{
- /* check the negative equivalent will fit without overflowing */
- if (unlikely(tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1))
+ if (pg_neg_u16_overflow(tmp, ))
goto out_of_range;
- return -((int16) tmp);
+ return result;
}
if (unlikely(tmp > PG_INT16_MAX))
@@ -333,10 +334,9 @@ slow:
if (neg)
{
- /* check the negative equivalent will fit without overflowing */
- if (tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1)
+ if (pg_neg_u16_overflow(tmp, ))
goto out_of_range;
- return -((int16) tmp);
+ return result;
}
if (tmp > PG_INT16_MAX)
@@ -393,6 +393,7 @@ pg_strtoint32_safe(const char *s, Node *escontext)
uint32 tmp = 0;
bool neg = false;
unsigned char digit;
+ int32 result;
/*
* The majority of cases are likely to be base-10 digits without any
@@ -452,10 +453,9 @@ pg_strtoint32_safe(const char *s, Node *escontext)
if (neg)
{
- /* check the negative equivalent will fit without overflowing */
- if (unlikely(tmp > (uint32) (-(PG_INT32_MIN + 1)) + 1))
+ if (pg_neg_u32_overflow(tmp, ))
goto out_of_range;
Re: Remove dependence on integer wrapping
On Tue, Jun 11, 2024 at 09:31:39AM -0400, Joseph Koshakow wrote: > tmp is an uint16 here, it seems like you might have read it as an > int16? We would need some helper method like > > static inline bool > pg_neg_u16_overflow(uint16 a, int16 *result); > > and then we could replace that whole chunk with something like > > if (unlikely(pg_neg_u16_overflow(tmp, ))) > goto out_of_range; > else > return result; > > > that pattern shows up a lot in this file, but I was worried that it > wasn't useful as a general purpose function. Happy to add it > though if you still feel otherwise. I personally find that much easier to read. Since the existing open-coded overflow check is apparently insufficient, I think there's a reasonably strong case for centralizing this sort of thing so that we don't continue to make the same mistakes. >> Ah, I see. Joe's patch does that in one place. It's probably worth doing >> that in the other places this "just-barefly overflow" comment appears >> IMHO. > > The only other place I found this comment was in > `make_timestamp_internal`. I've updated that function and added some > tests. I also manually verified that the behavior matches before and > after this patch. tm2timestamp() in src/interfaces/ecpg/pgtypeslib/timestamp.c has the same comment. The code there looks very similar to the code for tm2timestamp() in the other timestamp.c... -- nathan
Re: Remove dependence on integer wrapping
>> /* check the negative equivalent will fit without overflowing */ >> if (unlikely(tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1)) >> goto out_of_range; >> + >> + /* >> + * special case the minimum integer because its negation cannot be >> + * represented >> + */ >> + if (tmp == ((uint16) PG_INT16_MAX) + 1) >> + return PG_INT16_MIN; >> return -((int16) tmp); > > My first impression is that there appears to be two overflow checks, one of > which sends us to out_of_range, and another that just returns a special > result. Why shouldn't we add a pg_neg_s16_overflow() and replace this > whole chunk with something like this? > >if (unlikely(pg_neg_s16_overflow(tmp, ))) >goto out_of_range; >else >return tmp; tmp is an uint16 here, it seems like you might have read it as an int16? We would need some helper method like static inline bool pg_neg_u16_overflow(uint16 a, int16 *result); and then we could replace that whole chunk with something like if (unlikely(pg_neg_u16_overflow(tmp, ))) goto out_of_range; else return result; that pattern shows up a lot in this file, but I was worried that it wasn't useful as a general purpose function. Happy to add it though if you still feel otherwise. >> + return ((uint32) INT32_MAX) + 1; >> >> + return ((uint64) INT64_MAX) + 1; > > nitpick: Any reason not to use PG_INT32_MAX/PG_INT64_MAX for these? Carelessness, sorry about that, it's been fixed in the attached patch. >> I believe this is a copy-and-paste from 841b4a2d5, which added this: >> >> + *result = (date * INT64CONST(864)) + time; >> + /* check for major overflow */ >> + if ((*result - time) / INT64CONST(864) != date) >> + return -1; >> + /* check for just-barely overflow (okay except time-of-day wraps) */ >> + if ((*result < 0) ? (date >= 0) : (date < 0)) >> + return -1; >> >> I think you could replace the whole thing by using overflow-aware >> multiplication and addition primitives in the result calculation. >> Lines 2-4 basically check for mult overflow and 5-7 for addition >> overflow. > > Ah, I see. Joe's patch does that in one place. It's probably worth doing > that in the other places this "just-barefly overflow" comment appears IMHO. > > I was still confused by the comment about 1999, but I tracked it down to >commit 542eeba [0]. IIUC it literally means that we need special handling >for that date because POSTGRES_EPOCH_JDATE is 2000-01-01. > > [0] https://postgr.es/m/CABUevEx5zUO%3DKRUg06a9qnQ_e9EvTKscL6HxAM_L3xj71R7AQw%40mail.gmail.com > Yeah, I think so, and I think we probably don't need any special care > if we switch to direct tests of overflow-aware primitives. (Though >it'd be worth checking that '1999-12-31 24:00:00'::timestamp still > works. It doesn't look like I actually added a test case for that.) The only other place I found this comment was in `make_timestamp_internal`. I've updated that function and added some tests. I also manually verified that the behavior matches before and after this patch. >> BTW, while I approve of trying to get rid of our need for -fwrapv, >> I'm quite scared of actually doing it. > > I think that's a quite fair concern. One potentially relevant datapoint is > that we actually don't have -fwrapv equivalent on all platforms, and I don't >recall a lot of complaints from windows users. Of course it's quite possible > that they'd never notice... > > I think this is a good argument for enabling -ftrapv in development > builds. That gives us at least a *chance* of seeing these issues. +1, I wouldn't recommend removing -fwrapv immediately after this commit. However, if we can enable -ftrapv in development builds, then we can find overflows much more easily. > Whatever cases you may have discovered by running the regression tests will > be at best the tip of the iceberg. Agreed. > Is there any chance of using static > analysis to find all the places of concern? I'm not personally familiar with any static analysis tools, but I can try and do some research. Andres had previously suggested SQLSmith. I think any kind of fuzz testing with -ftrapv enabled will reveal a lot of issues. Honestly just grepping for +,-,* in certain directories (like backend/utils/adt) would probably be fairly fruitful for anyone with the patience. My previous overflow patch was the result of looking through all the arithmetic in datetime.c. Thanks, Joe Koshakow
Re: Remove dependence on integer wrapping
Hi, On 2024-06-09 21:57:54 -0400, Tom Lane wrote: > BTW, while I approve of trying to get rid of our need for -fwrapv, > I'm quite scared of actually doing it. I think that's a quite fair concern. One potentially relevant datapoint is that we actually don't have -fwrapv equivalent on all platforms, and I don't recall a lot of complaints from windows users. Of course it's quite possible that they'd never notice... I think this is a good argument for enabling -ftrapv in development builds. That gives us at least a *chance* of seeing these issues. > Whatever cases you may have discovered by running the regression tests will > be at best the tip of the iceberg. Is there any chance of using static > analysis to find all the places of concern? The last time I tried removing -fwrapv both gcc and clang found quite a few issues. I think I fixed most of those though, so presumably the issue that remain are ones less easily found by simple static analysis. I wonder if coverity would find more if we built without -fwrapv. GCC has -Wstrict-overflow=n, which at least tells us where the compiler optimizes based on the assumption that there cannot be overflow. It does generate a bunch of noise, but I think it's almost exclusively due to our MemSet() macro. Greetings, Andres Freund
Re: Remove dependence on integer wrapping
Nathan Bossart writes: > On Sun, Jun 09, 2024 at 09:57:54PM -0400, Tom Lane wrote: >> I think you could replace the whole thing by using overflow-aware >> multiplication and addition primitives in the result calculation. > I was still confused by the comment about 1999, but I tracked it down to > commit 542eeba [0]. IIUC it literally means that we need special handling > for that date because POSTGRES_EPOCH_JDATE is 2000-01-01. Yeah, I think so, and I think we probably don't need any special care if we switch to direct tests of overflow-aware primitives. (Though it'd be worth checking that '1999-12-31 24:00:00'::timestamp still works. It doesn't look like I actually added a test case for that.) regards, tom lane
Re: Remove dependence on integer wrapping
On Sun, Jun 09, 2024 at 09:57:54PM -0400, Tom Lane wrote: > Nathan Bossart writes: >> On Sun, Jun 09, 2024 at 04:59:22PM -0400, Joseph Koshakow wrote: >>> The following comment was in the code for parsing timestamps: >>> /* check for just-barely overflow (okay except time-of-day wraps) */ >>> /* caution: we want to allow 1999-12-31 24:00:00 */ >>> >>> I wasn't able to fully understand it even after staring at it for >>> a while. Is the comment suggesting that it is ok for the months field, >>> for example, to wrap around? That doesn't sound right to me I tested >>> the supplied timestamp, 1999-12-31 24:00:00, and it behaves the same >>> before and after the patch. > >> I haven't stared at this for a while like you, but I am likewise confused >> at first glance. This dates back to commit 84df54b, and it looks like this >> comment was present in the first version of the patch in the thread [0]. I >> CTRL+F'd for any discussion about this but couldn't immediately find >> anything. > > I believe this is a copy-and-paste from 841b4a2d5, which added this: > > + *result = (date * INT64CONST(864)) + time; > + /* check for major overflow */ > + if ((*result - time) / INT64CONST(864) != date) > + return -1; > + /* check for just-barely overflow (okay except time-of-day wraps) */ > + if ((*result < 0) ? (date >= 0) : (date < 0)) > + return -1; > > I think you could replace the whole thing by using overflow-aware > multiplication and addition primitives in the result calculation. > Lines 2-4 basically check for mult overflow and 5-7 for addition > overflow. Ah, I see. Joe's patch does that in one place. It's probably worth doing that in the other places this "just-barefly overflow" comment appears IMHO. I was still confused by the comment about 1999, but I tracked it down to commit 542eeba [0]. IIUC it literally means that we need special handling for that date because POSTGRES_EPOCH_JDATE is 2000-01-01. [0] https://postgr.es/m/CABUevEx5zUO%3DKRUg06a9qnQ_e9EvTKscL6HxAM_L3xj71R7AQw%40mail.gmail.com -- nathan
Re: Remove dependence on integer wrapping
Nathan Bossart writes: > On Sun, Jun 09, 2024 at 04:59:22PM -0400, Joseph Koshakow wrote: >> The following comment was in the code for parsing timestamps: >> /* check for just-barely overflow (okay except time-of-day wraps) */ >> /* caution: we want to allow 1999-12-31 24:00:00 */ >> >> I wasn't able to fully understand it even after staring at it for >> a while. Is the comment suggesting that it is ok for the months field, >> for example, to wrap around? That doesn't sound right to me I tested >> the supplied timestamp, 1999-12-31 24:00:00, and it behaves the same >> before and after the patch. > I haven't stared at this for a while like you, but I am likewise confused > at first glance. This dates back to commit 84df54b, and it looks like this > comment was present in the first version of the patch in the thread [0]. I > CTRL+F'd for any discussion about this but couldn't immediately find > anything. I believe this is a copy-and-paste from 841b4a2d5, which added this: + *result = (date * INT64CONST(864)) + time; + /* check for major overflow */ + if ((*result - time) / INT64CONST(864) != date) + return -1; + /* check for just-barely overflow (okay except time-of-day wraps) */ + if ((*result < 0) ? (date >= 0) : (date < 0)) + return -1; I think you could replace the whole thing by using overflow-aware multiplication and addition primitives in the result calculation. Lines 2-4 basically check for mult overflow and 5-7 for addition overflow. BTW, while I approve of trying to get rid of our need for -fwrapv, I'm quite scared of actually doing it. Whatever cases you may have discovered by running the regression tests will be at best the tip of the iceberg. Is there any chance of using static analysis to find all the places of concern? regards, tom lane
Re: Remove dependence on integer wrapping
On Sun, Jun 09, 2024 at 04:59:22PM -0400, Joseph Koshakow wrote: > I originally added the helper functions to int.h thinking I'd find > many more instances of overflow due to integer negation, however I > didn't find that many. So let me know if you think we'd be better > off without the functions. I'd vote for the functions, even if it's just future-proofing at the moment. IMHO it helps with readability, too. > The following comment was in the code for parsing timestamps: > > /* check for just-barely overflow (okay except time-of-day wraps) */ > /* caution: we want to allow 1999-12-31 24:00:00 */ > > I wasn't able to fully understand it even after staring at it for > a while. Is the comment suggesting that it is ok for the months field, > for example, to wrap around? That doesn't sound right to me I tested > the supplied timestamp, 1999-12-31 24:00:00, and it behaves the same > before and after the patch. I haven't stared at this for a while like you, but I am likewise confused at first glance. This dates back to commit 84df54b, and it looks like this comment was present in the first version of the patch in the thread [0]. I CTRL+F'd for any discussion about this but couldn't immediately find anything. > /* check the negative equivalent will fit without overflowing */ > if (unlikely(tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1)) > goto out_of_range; > + > + /* > + * special case the minimum integer because its negation cannot > be > + * represented > + */ > + if (tmp == ((uint16) PG_INT16_MAX) + 1) > + return PG_INT16_MIN; > return -((int16) tmp); My first impression is that there appears to be two overflow checks, one of which sends us to out_of_range, and another that just returns a special result. Why shouldn't we add a pg_neg_s16_overflow() and replace this whole chunk with something like this? if (unlikely(pg_neg_s16_overflow(tmp, ))) goto out_of_range; else return tmp; > + return ((uint32) INT32_MAX) + 1; > + return ((uint64) INT64_MAX) + 1; nitpick: Any reason not to use PG_INT32_MAX/PG_INT64_MAX for these? [0] https://postgr.es/m/flat/CAFj8pRBwqALkzc%3D1WV%2Bh5e%2BDcALY2EizjHCvFi9vHbs%2Bz1OhjA%40mail.gmail.com -- nathan
Remove dependence on integer wrapping
Hi,
In [0] Andres suggested enabling -ftrapv in assert enabled builds. While
I vastly underestimated the complexity of updating `configure` to do
this, I was able to enable the flag locally. Enabling this flag causes
our existing regression tests to trap and fail in multiple different
spots. The attached patch resolves all of these overflows so that all
of our existing tests will pass with the -ftrapv flag enabled.
Some notes on the patch itself are:
I originally added the helper functions to int.h thinking I'd find
many more instances of overflow due to integer negation, however I
didn't find that many. So let me know if you think we'd be better
off without the functions.
I considered using #ifdef to rely on wrapping when -fwrapv was
enabled. This would save us some unnecessary branching when we could
rely on wrapping behavior, but it would mean that we could only enable
-ftrapv when -fwrapv was disabled, greatly reducing its utility.
The following comment was in the code for parsing timestamps:
/* check for just-barely overflow (okay except time-of-day wraps) */
/* caution: we want to allow 1999-12-31 24:00:00 */
I wasn't able to fully understand it even after staring at it for
a while. Is the comment suggesting that it is ok for the months field,
for example, to wrap around? That doesn't sound right to me I tested
the supplied timestamp, 1999-12-31 24:00:00, and it behaves the same
before and after the patch.
Thanks,
Joe Koshakow
[0]
https://www.postgresql.org/message-id/20240213191401.jjhsic7et4tiahjs%40awork3.anarazel.de
From 319bc904858ad8fbcc687a923733defd3358c7b9 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow
Date: Sat, 8 Jun 2024 22:16:46 -0400
Subject: [PATCH] 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 | 7 +++--
src/backend/utils/adt/numeric.c | 5 ++--
src/backend/utils/adt/numutils.c | 35 ++
src/backend/utils/adt/timestamp.c | 13 ++---
src/include/common/int.h | 48 +++
5 files changed, 92 insertions(+), 16 deletions(-)
diff --git a/src/backend/utils/adt/cash.c b/src/backend/utils/adt/cash.c
index 32fbad2f57..f6f095a57b 100644
--- a/src/backend/utils/adt/cash.c
+++ b/src/backend/utils/adt/cash.c
@@ -352,8 +352,11 @@ cash_out(PG_FUNCTION_ARGS)
if (value < 0)
{
- /* make the amount positive for digit-reconstruction loop */
- value = -value;
+ /*
+ * make the amount positive for digit-reconstruction loop, we can
+ * leave INT64_MIN unchanged
+ */
+ pg_neg_s64_overflow(value, );
/* set up formatting data */
signsymbol = (*lconvert->negative_sign != '\0') ? lconvert->negative_sign : "-";
sign_posn = lconvert->n_sign_posn;
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index 5510a203b0..4ea2d9b0b4 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -8110,15 +8110,14 @@ int64_to_numericvar(int64 val, NumericVar *var)
/* int64 can require at most 19 decimal digits; add one for safety */
alloc_var(var, 20 / DEC_DIGITS);
+ uval = pg_abs_s64(val);
if (val < 0)
{
var->sign = NUMERIC_NEG;
- uval = -val;
}
else
{
var->sign = NUMERIC_POS;
- uval = val;
}
var->dscale = 0;
if (val == 0)
@@ -11222,7 +11221,7 @@ power_var_int(const NumericVar *base, int exp, int exp_dscale,
* Now we can proceed with the multiplications.
*/
neg = (exp < 0);
- mask = abs(exp);
+ mask = pg_abs_s32(exp);
init_var(_prod);
set_var_from_var(base, _prod);
diff --git a/src/backend/utils/adt/numutils.c b/src/backend/utils/adt/numutils.c
index adc1e8a4cb..12bef9d63c 100644
--- a/src/backend/utils/adt/numutils.c
+++ b/src/backend/utils/adt/numutils.c
@@ -193,6 +193,13 @@ pg_strtoint16_safe(const char *s, Node *escontext)
/* check the negative equivalent will fit without overflowing */
if (unlikely(tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1))
goto out_of_range;
+
+ /*
+ * special case the minimum integer because its negation cannot be
+ * represented
+ */
+ if (tmp == ((uint16) PG_INT16_MAX) + 1)
+ return PG_INT16_MIN;
return -((int16) tmp);
}
@@ -336,6 +343,13 @@ slow:
/* check the negative equivalent will fit without overflowing */
if (tmp > (uint16) (-(PG_INT16_MIN + 1)) + 1)
goto out_of_range;
+
+ /*
+ * special case the minimum integer because its negation cannot be
+ * represented
+ */
+ if (tmp == ((uint16) PG_INT16_MAX) + 1)
+ return PG_INT16_MIN;
return -((int16) tmp);
}
@@ -598,6 +612,13 @@ slow:
/* check the negative equivalent will fit without overflowing */
if (tmp > (uint32) (-(PG_INT32_MIN + 1)) + 1)
goto out_of_range;
+
+ /*
+ * special case the minimum integer because its negation cannot be
+
