On Thu, Jun 13, 2024 at 12:00 AM Alexander Lakhin <exclus...@gmail.com> 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('100000000', 'CC'); > SELECT to_timestamp('1000000000,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 <kosh...@gmail.com> 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, &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(&base_prod); set_var_from_var(base, &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 <limits.h> #include <ctype.h> +#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, &result)) 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, &result)) 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, &result)) goto out_of_range; - return -((int32) tmp); + return result; } if (unlikely(tmp > PG_INT32_MAX)) @@ -595,10 +595,9 @@ slow: if (neg) { - /* check the negative equivalent will fit without overflowing */ - if (tmp > (uint32) (-(PG_INT32_MIN + 1)) + 1) + if (pg_neg_u32_overflow(tmp, &result)) goto out_of_range; - return -((int32) tmp); + return result; } if (tmp > PG_INT32_MAX) @@ -655,6 +654,7 @@ pg_strtoint64_safe(const char *s, Node *escontext) uint64 tmp = 0; bool neg = false; unsigned char digit; + int64 result; /* * The majority of cases are likely to be base-10 digits without any @@ -714,10 +714,9 @@ pg_strtoint64_safe(const char *s, Node *escontext) if (neg) { - /* check the negative equivalent will fit without overflowing */ - if (unlikely(tmp > (uint64) (-(PG_INT64_MIN + 1)) + 1)) + if (pg_neg_u64_overflow(tmp, &result)) goto out_of_range; - return -((int64) tmp); + return result; } if (unlikely(tmp > PG_INT64_MAX)) @@ -857,10 +856,9 @@ slow: if (neg) { - /* check the negative equivalent will fit without overflowing */ - if (tmp > (uint64) (-(PG_INT64_MIN + 1)) + 1) + if (pg_neg_u64_overflow(tmp, &result)) goto out_of_range; - return -((int64) tmp); + return result; } if (tmp > PG_INT64_MAX) diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c index e4715605a2..c419c46540 100644 --- a/src/backend/utils/adt/timestamp.c +++ b/src/backend/utils/adt/timestamp.c @@ -617,19 +617,8 @@ make_timestamp_internal(int year, int month, int day, time = (((hour * MINS_PER_HOUR + min) * SECS_PER_MINUTE) * USECS_PER_SEC) + (int64) rint(sec * USECS_PER_SEC); - result = date * USECS_PER_DAY + time; - /* check for major overflow */ - if ((result - time) / USECS_PER_DAY != date) - ereport(ERROR, - (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), - errmsg("timestamp out of range: %d-%02d-%02d %d:%02d:%02g", - year, month, day, - hour, min, sec))); - - /* check for just-barely overflow (okay except time-of-day wraps) */ - /* caution: we want to allow 1999-12-31 24:00:00 */ - if ((result < 0 && date > 0) || - (result > 0 && date < -1)) + if (pg_mul_s64_overflow(date, USECS_PER_DAY, &result) || + pg_add_s64_overflow(result, time, &result)) ereport(ERROR, (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), errmsg("timestamp out of range: %d-%02d-%02d %d:%02d:%02g", @@ -2009,17 +1998,8 @@ tm2timestamp(struct pg_tm *tm, fsec_t fsec, int *tzp, Timestamp *result) date = date2j(tm->tm_year, tm->tm_mon, tm->tm_mday) - POSTGRES_EPOCH_JDATE; time = time2t(tm->tm_hour, tm->tm_min, tm->tm_sec, fsec); - *result = date * USECS_PER_DAY + time; - /* check for major overflow */ - if ((*result - time) / USECS_PER_DAY != date) - { - *result = 0; /* keep compiler quiet */ - return -1; - } - /* check for just-barely overflow (okay except time-of-day wraps) */ - /* caution: we want to allow 1999-12-31 24:00:00 */ - if ((*result < 0 && date > 0) || - (*result > 0 && date < -1)) + if (pg_mul_s64_overflow(date, USECS_PER_DAY, result) || + pg_add_s64_overflow(*result, time, result)) { *result = 0; /* keep compiler quiet */ return -1; diff --git a/src/include/common/int.h b/src/include/common/int.h index 7fc046e78a..3065d586d4 100644 --- a/src/include/common/int.h +++ b/src/include/common/int.h @@ -154,6 +154,23 @@ pg_mul_s32_overflow(int32 a, int32 b, int32 *result) #endif } +static inline uint32 +pg_abs_s32(int32 a) +{ + if (unlikely(a == PG_INT32_MIN)) + { + return ((uint32) PG_INT32_MAX) + 1; + } + else if (a < 0) + { + return (uint32) -a; + } + else + { + return (uint32) a; + } +} + /* * INT64 */ @@ -258,6 +275,37 @@ pg_mul_s64_overflow(int64 a, int64 b, int64 *result) #endif } +static inline bool +pg_neg_s64_overflow(int64 a, int64 *result) +{ + if (unlikely(a == PG_INT64_MIN)) + { + return true; + } + else + { + *result = -a; + return false; + } +} + +static inline uint64 +pg_abs_s64(int64 a) +{ + if (unlikely(a == PG_INT64_MIN)) + { + return ((uint64) PG_INT64_MAX) + 1; + } + else if (a < 0) + { + return (uint64) -a; + } + else + { + return (uint64) a; + } +} + /*------------------------------------------------------------------------ * Overflow routines for unsigned integers *------------------------------------------------------------------------ @@ -318,6 +366,25 @@ pg_mul_u16_overflow(uint16 a, uint16 b, uint16 *result) #endif } +static inline bool +pg_neg_u16_overflow(uint16 a, int16 *result) +{ + if (unlikely(a > ((uint16) PG_INT16_MAX) + 1)) + { + return true; + } + else if (unlikely(a == ((uint16) PG_INT16_MAX) + 1)) + { + *result = PG_INT16_MIN; + return false; + } + else + { + *result = -((int16) a); + return false; + } +} + /* * INT32 */ @@ -373,6 +440,25 @@ pg_mul_u32_overflow(uint32 a, uint32 b, uint32 *result) #endif } +static inline bool +pg_neg_u32_overflow(uint32 a, int32 *result) +{ + if (unlikely(a > ((uint32) PG_INT32_MAX) + 1)) + { + return true; + } + else if (unlikely(a == ((uint32) PG_INT32_MAX) + 1)) + { + *result = PG_INT32_MIN; + return false; + } + else + { + *result = -((int32) a); + return false; + } +} + /* * UINT64 */ @@ -438,6 +524,25 @@ pg_mul_u64_overflow(uint64 a, uint64 b, uint64 *result) #endif } +static inline bool +pg_neg_u64_overflow(uint64 a, int64 *result) +{ + if (unlikely(a > ((uint64) PG_INT64_MAX) + 1)) + { + return true; + } + else if (unlikely(a == ((uint64) PG_INT64_MAX) + 1)) + { + *result = PG_INT64_MIN; + return false; + } + else + { + *result = -((int64) a); + return false; + } +} + /*------------------------------------------------------------------------ * * Comparison routines for integer types. diff --git a/src/interfaces/ecpg/pgtypeslib/timestamp.c b/src/interfaces/ecpg/pgtypeslib/timestamp.c index f1b143fbd2..93d4cc323d 100644 --- a/src/interfaces/ecpg/pgtypeslib/timestamp.c +++ b/src/interfaces/ecpg/pgtypeslib/timestamp.c @@ -11,6 +11,7 @@ #error -ffast-math is known to break this code #endif +#include "common/int.h" #include "dt.h" #include "pgtypes_date.h" #include "pgtypes_timestamp.h" @@ -48,14 +49,8 @@ tm2timestamp(struct tm *tm, fsec_t fsec, int *tzp, timestamp * result) dDate = date2j(tm->tm_year, tm->tm_mon, tm->tm_mday) - date2j(2000, 1, 1); time = time2t(tm->tm_hour, tm->tm_min, tm->tm_sec, fsec); - *result = (dDate * USECS_PER_DAY) + time; - /* check for major overflow */ - if ((*result - time) / USECS_PER_DAY != dDate) - return -1; - /* check for just-barely overflow (okay except time-of-day wraps) */ - /* caution: we want to allow 1999-12-31 24:00:00 */ - if ((*result < 0 && dDate > 0) || - (*result > 0 && dDate < -1)) + if (pg_mul_s64_overflow(dDate, USECS_PER_DAY, result) || + pg_add_s64_overflow(*result, time, result)) return -1; if (tzp != NULL) *result = dt2local(*result, -(*tzp)); diff --git a/src/test/regress/expected/timestamp.out b/src/test/regress/expected/timestamp.out index cf337da517..e287260051 100644 --- a/src/test/regress/expected/timestamp.out +++ b/src/test/regress/expected/timestamp.out @@ -2201,3 +2201,16 @@ select age(timestamp '-infinity', timestamp 'infinity'); select age(timestamp '-infinity', timestamp '-infinity'); ERROR: interval out of range +-- test timestamp near POSTGRES_EPOCH_JDATE +select timestamp '1999-12-31 24:00:00'; + timestamp +-------------------------- + Sat Jan 01 00:00:00 2000 +(1 row) + +select make_timestamp(1999, 12, 31, 24, 0, 0); + make_timestamp +-------------------------- + Sat Jan 01 00:00:00 2000 +(1 row) + diff --git a/src/test/regress/expected/timestamptz.out b/src/test/regress/expected/timestamptz.out index bfb3825ff6..d01d174983 100644 --- a/src/test/regress/expected/timestamptz.out +++ b/src/test/regress/expected/timestamptz.out @@ -3286,3 +3286,16 @@ SELECT age(timestamptz '-infinity', timestamptz 'infinity'); SELECT age(timestamptz '-infinity', timestamptz '-infinity'); ERROR: interval out of range +-- test timestamp near POSTGRES_EPOCH_JDATE +select timestamptz '1999-12-31 24:00:00'; + timestamptz +------------------------------ + Sat Jan 01 00:00:00 2000 PST +(1 row) + +select make_timestamptz(1999, 12, 31, 24, 0, 0); + make_timestamptz +------------------------------ + Sat Jan 01 00:00:00 2000 PST +(1 row) + diff --git a/src/test/regress/sql/timestamp.sql b/src/test/regress/sql/timestamp.sql index 820ef7752a..748469576d 100644 --- a/src/test/regress/sql/timestamp.sql +++ b/src/test/regress/sql/timestamp.sql @@ -424,3 +424,7 @@ select age(timestamp 'infinity', timestamp 'infinity'); select age(timestamp 'infinity', timestamp '-infinity'); select age(timestamp '-infinity', timestamp 'infinity'); select age(timestamp '-infinity', timestamp '-infinity'); + +-- test timestamp near POSTGRES_EPOCH_JDATE +select timestamp '1999-12-31 24:00:00'; +select make_timestamp(1999, 12, 31, 24, 0, 0); diff --git a/src/test/regress/sql/timestamptz.sql b/src/test/regress/sql/timestamptz.sql index ccfd90d646..c71d5489b4 100644 --- a/src/test/regress/sql/timestamptz.sql +++ b/src/test/regress/sql/timestamptz.sql @@ -668,3 +668,7 @@ SELECT age(timestamptz 'infinity', timestamptz 'infinity'); SELECT age(timestamptz 'infinity', timestamptz '-infinity'); SELECT age(timestamptz '-infinity', timestamptz 'infinity'); SELECT age(timestamptz '-infinity', timestamptz '-infinity'); + +-- test timestamp near POSTGRES_EPOCH_JDATE +select timestamptz '1999-12-31 24:00:00'; +select make_timestamptz(1999, 12, 31, 24, 0, 0); -- 2.34.1
From b62972223b6dcdd3199e7ef40efb8fea1c693dc8 Mon Sep 17 00:00:00 2001 From: Joseph Koshakow <kosh...@gmail.com> Date: Thu, 13 Jun 2024 22:39:25 -0400 Subject: [PATCH 2/2] Handle overflow in money arithmetic --- src/backend/utils/adt/cash.c | 40 ++++++++++++++++++++++++----- src/test/regress/expected/money.out | 29 +++++++++++++++++++++ src/test/regress/sql/money.sql | 16 ++++++++++++ 3 files changed, 78 insertions(+), 7 deletions(-) diff --git a/src/backend/utils/adt/cash.c b/src/backend/utils/adt/cash.c index f6f095a57b..9d7ddcb58c 100644 --- a/src/backend/utils/adt/cash.c +++ b/src/backend/utils/adt/cash.c @@ -617,6 +617,11 @@ cash_pl(PG_FUNCTION_ARGS) Cash c2 = PG_GETARG_CASH(1); Cash result; + if (pg_add_s64_overflow(c1, c2, &result)) + ereport(ERROR, + (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), + errmsg("money out of range"))); + result = c1 + c2; PG_RETURN_CASH(result); @@ -633,7 +638,10 @@ cash_mi(PG_FUNCTION_ARGS) Cash c2 = PG_GETARG_CASH(1); Cash result; - result = c1 - c2; + if (pg_sub_s64_overflow(c1, c2, &result)) + ereport(ERROR, + (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), + errmsg("money out of range"))); PG_RETURN_CASH(result); } @@ -770,7 +778,10 @@ cash_mul_int8(PG_FUNCTION_ARGS) int64 i = PG_GETARG_INT64(1); Cash result; - result = c * i; + if (pg_mul_s64_overflow(c, i, &result)) + ereport(ERROR, + (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), + errmsg("money out of range"))); PG_RETURN_CASH(result); } @@ -785,7 +796,10 @@ int8_mul_cash(PG_FUNCTION_ARGS) Cash c = PG_GETARG_CASH(1); Cash result; - result = i * c; + if (pg_mul_s64_overflow(i, c, &result)) + ereport(ERROR, + (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), + errmsg("money out of range"))); PG_RETURN_CASH(result); } @@ -820,7 +834,10 @@ cash_mul_int4(PG_FUNCTION_ARGS) int32 i = PG_GETARG_INT32(1); Cash result; - result = c * i; + if (pg_mul_s64_overflow(c, (int64) i, &result)) + ereport(ERROR, + (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), + errmsg("money out of range"))); PG_RETURN_CASH(result); } @@ -835,7 +852,10 @@ int4_mul_cash(PG_FUNCTION_ARGS) Cash c = PG_GETARG_CASH(1); Cash result; - result = i * c; + if (pg_mul_s64_overflow((int64) i, c, &result)) + ereport(ERROR, + (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), + errmsg("money out of range"))); PG_RETURN_CASH(result); } @@ -872,7 +892,10 @@ cash_mul_int2(PG_FUNCTION_ARGS) int16 s = PG_GETARG_INT16(1); Cash result; - result = c * s; + if (pg_mul_s64_overflow(c, (int64) s, &result)) + ereport(ERROR, + (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), + errmsg("money out of range"))); PG_RETURN_CASH(result); } @@ -886,7 +909,10 @@ int2_mul_cash(PG_FUNCTION_ARGS) Cash c = PG_GETARG_CASH(1); Cash result; - result = s * c; + if (pg_mul_s64_overflow((int64) s, c, &result)) + ereport(ERROR, + (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE), + errmsg("money out of range"))); PG_RETURN_CASH(result); } diff --git a/src/test/regress/expected/money.out b/src/test/regress/expected/money.out index 7fd4e31804..950e6410a4 100644 --- a/src/test/regress/expected/money.out +++ b/src/test/regress/expected/money.out @@ -528,3 +528,32 @@ SELECT '-92233720368547758.08'::money::numeric; -92233720368547758.08 (1 row) +-- Test overflow checks +SELECT '92233720368547758.07'::money + '0.01'::money; +ERROR: money out of range +SELECT '-92233720368547758.08'::money - '0.01'::money; +ERROR: money out of range +SELECT '92233720368547758.07'::money * 2::int8; +ERROR: money out of range +SELECT '-92233720368547758.08'::money * 2::int8; +ERROR: money out of range +SELECT 2::int8 * '92233720368547758.07'::money ; +ERROR: money out of range +SELECT 2::int8 * '-92233720368547758.08'::money; +ERROR: money out of range +SELECT '92233720368547758.07'::money * 2::int4; +ERROR: money out of range +SELECT '-92233720368547758.08'::money * 2::int4; +ERROR: money out of range +SELECT 2::int4 * '92233720368547758.07'::money ; +ERROR: money out of range +SELECT 2::int4 * '-92233720368547758.08'::money; +ERROR: 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