From db8855b66f5cfd9761b1763fdc6b8d93179607df Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 28 Feb 2024 14:00:30 -0500 Subject: [PATCH] Fix mis-rounding and overflow hazards in date_bin(). In the case where the target timestamp is before the origin timestamp and their difference is already an exact multiple of the stride, the code incorrectly subtracted the stride anyway. Also detect several integer-overflow cases that previously produced bogus results. (The submitted patch tried to avoid overflow, but I'm not convinced it's right, and problematic cases are so far out of the plausibly-useful range that they don't seem worth sweating over. Let's just use overflow-detecting arithmetic and throw errors.) timestamp_bin() and timestamptz_bin() are basically identical and so had identical bugs. Fix both. Report and patch by Moaaz Assali, adjusted some by me. Back-patch to v14 where date_bin() was introduced. Discussion: https://postgr.es/m/CALkF+nvtuas-2kydG-WfofbRSJpyODAJWun==W-yO5j2R4meqA@mail.gmail.com --- src/backend/utils/adt/timestamp.c | 72 +++++++++++++++++------ src/test/regress/expected/timestamp.out | 14 +++++ src/test/regress/expected/timestamptz.out | 14 +++++ src/test/regress/sql/timestamp.sql | 8 +++ src/test/regress/sql/timestamptz.sql | 8 +++ 5 files changed, 97 insertions(+), 19 deletions(-) diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c index 27073cb327..58348f914b 100644 --- a/src/backend/utils/adt/timestamp.c +++ b/src/backend/utils/adt/timestamp.c @@ -3924,8 +3924,9 @@ timestamp_bin(PG_FUNCTION_ARGS) Timestamp timestamp = PG_GETARG_TIMESTAMP(1); Timestamp origin = PG_GETARG_TIMESTAMP(2); Timestamp result, - tm_diff, stride_usecs, + tm_diff, + tm_modulo, tm_delta; if (TIMESTAMP_NOT_FINITE(timestamp)) @@ -3941,24 +3942,40 @@ timestamp_bin(PG_FUNCTION_ARGS) (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("timestamps cannot be binned into intervals containing months or years"))); - stride_usecs = stride->day * USECS_PER_DAY + stride->time; + if (unlikely(pg_mul_s64_overflow(stride->day, USECS_PER_DAY, &stride_usecs)) || + unlikely(pg_add_s64_overflow(stride_usecs, stride->time, &stride_usecs))) + ereport(ERROR, + (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), + errmsg("interval out of range"))); if (stride_usecs <= 0) ereport(ERROR, (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), errmsg("stride must be greater than zero"))); - tm_diff = timestamp - origin; - tm_delta = tm_diff - tm_diff % stride_usecs; + if (unlikely(pg_sub_s64_overflow(timestamp, origin, &tm_diff))) + ereport(ERROR, + (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), + errmsg("interval out of range"))); + + /* These calculations cannot overflow */ + tm_modulo = tm_diff % stride_usecs; + tm_delta = tm_diff - tm_modulo; + result = origin + tm_delta; /* - * Make sure the returned timestamp is at the start of the bin, even if - * the origin is in the future. + * We want to round towards -infinity, not 0, when tm_diff is negative and + * not a multiple of stride_usecs. This adjustment *can* cause overflow, + * since the result might now be out of the range origin .. timestamp. */ - if (origin > timestamp && stride_usecs > 1) - tm_delta -= stride_usecs; - - result = origin + tm_delta; + if (tm_modulo < 0) + { + if (unlikely(pg_sub_s64_overflow(result, stride_usecs, &result)) || + !IS_VALID_TIMESTAMP(result)) + ereport(ERROR, + (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), + errmsg("timestamp out of range"))); + } PG_RETURN_TIMESTAMP(result); } @@ -4109,6 +4126,7 @@ timestamptz_bin(PG_FUNCTION_ARGS) TimestampTz result, stride_usecs, tm_diff, + tm_modulo, tm_delta; if (TIMESTAMP_NOT_FINITE(timestamp)) @@ -4124,24 +4142,40 @@ timestamptz_bin(PG_FUNCTION_ARGS) (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("timestamps cannot be binned into intervals containing months or years"))); - stride_usecs = stride->day * USECS_PER_DAY + stride->time; + if (unlikely(pg_mul_s64_overflow(stride->day, USECS_PER_DAY, &stride_usecs)) || + unlikely(pg_add_s64_overflow(stride_usecs, stride->time, &stride_usecs))) + ereport(ERROR, + (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), + errmsg("interval out of range"))); if (stride_usecs <= 0) ereport(ERROR, (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), errmsg("stride must be greater than zero"))); - tm_diff = timestamp - origin; - tm_delta = tm_diff - tm_diff % stride_usecs; + if (unlikely(pg_sub_s64_overflow(timestamp, origin, &tm_diff))) + ereport(ERROR, + (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), + errmsg("interval out of range"))); + + /* These calculations cannot overflow */ + tm_modulo = tm_diff % stride_usecs; + tm_delta = tm_diff - tm_modulo; + result = origin + tm_delta; /* - * Make sure the returned timestamp is at the start of the bin, even if - * the origin is in the future. + * We want to round towards -infinity, not 0, when tm_diff is negative and + * not a multiple of stride_usecs. This adjustment *can* cause overflow, + * since the result might now be out of the range origin .. timestamp. */ - if (origin > timestamp && stride_usecs > 1) - tm_delta -= stride_usecs; - - result = origin + tm_delta; + if (tm_modulo < 0) + { + if (unlikely(pg_sub_s64_overflow(result, stride_usecs, &result)) || + !IS_VALID_TIMESTAMP(result)) + ereport(ERROR, + (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE), + errmsg("timestamp out of range"))); + } PG_RETURN_TIMESTAMPTZ(result); } diff --git a/src/test/regress/expected/timestamp.out b/src/test/regress/expected/timestamp.out index 79f8180955..2ee8712a96 100644 --- a/src/test/regress/expected/timestamp.out +++ b/src/test/regress/expected/timestamp.out @@ -699,6 +699,13 @@ SELECT date_bin('5 min'::interval, timestamp '2020-02-01 01:01:01', timestamp '2 Sat Feb 01 00:57:30 2020 (1 row) +-- test roundoff edge case when source < origin +SELECT date_bin('30 minutes'::interval, timestamp '2024-02-01 15:00:00', timestamp '2024-02-01 17:00:00'); + date_bin +-------------------------- + Thu Feb 01 15:00:00 2024 +(1 row) + -- disallow intervals with months or years SELECT date_bin('5 months'::interval, timestamp '2020-02-01 01:01:01', timestamp '2001-01-01'); ERROR: timestamps cannot be binned into intervals containing months or years @@ -710,6 +717,13 @@ ERROR: stride must be greater than zero -- disallow negative intervals SELECT date_bin('-2 days'::interval, timestamp '1970-01-01 01:00:00' , timestamp '1970-01-01 00:00:00'); ERROR: stride must be greater than zero +-- test overflow cases +select date_bin('15 minutes'::interval, timestamp '294276-12-30', timestamp '4000-12-20 BC'); +ERROR: interval out of range +select date_bin('200000000 days'::interval, '2024-02-01'::timestamp, '2024-01-01'::timestamp); +ERROR: interval out of range +select date_bin('365000 days'::interval, '4400-01-01 BC'::timestamp, '4000-01-01 BC'::timestamp); +ERROR: timestamp out of range -- Test casting within a BETWEEN qualifier SELECT d1 - timestamp without time zone '1997-01-02' AS diff FROM TIMESTAMP_TBL diff --git a/src/test/regress/expected/timestamptz.out b/src/test/regress/expected/timestamptz.out index eba84191d3..790cae15ab 100644 --- a/src/test/regress/expected/timestamptz.out +++ b/src/test/regress/expected/timestamptz.out @@ -743,6 +743,13 @@ SELECT date_bin('5 min'::interval, timestamptz '2020-02-01 01:01:01+00', timesta Fri Jan 31 16:57:30 2020 PST (1 row) +-- test roundoff edge case when source < origin +SELECT date_bin('30 minutes'::interval, timestamptz '2024-02-01 15:00:00', timestamptz '2024-02-01 17:00:00'); + date_bin +------------------------------ + Thu Feb 01 15:00:00 2024 PST +(1 row) + -- disallow intervals with months or years SELECT date_bin('5 months'::interval, timestamp with time zone '2020-02-01 01:01:01+00', timestamp with time zone '2001-01-01+00'); ERROR: timestamps cannot be binned into intervals containing months or years @@ -754,6 +761,13 @@ ERROR: stride must be greater than zero -- disallow negative intervals SELECT date_bin('-2 days'::interval, timestamp with time zone '1970-01-01 01:00:00+00' , timestamp with time zone '1970-01-01 00:00:00+00'); ERROR: stride must be greater than zero +-- test overflow cases +select date_bin('15 minutes'::interval, timestamptz '294276-12-30', timestamptz '4000-12-20 BC'); +ERROR: interval out of range +select date_bin('200000000 days'::interval, '2024-02-01'::timestamptz, '2024-01-01'::timestamptz); +ERROR: interval out of range +select date_bin('365000 days'::interval, '4400-01-01 BC'::timestamptz, '4000-01-01 BC'::timestamptz); +ERROR: timestamp out of range -- Test casting within a BETWEEN qualifier SELECT d1 - timestamp with time zone '1997-01-02' AS diff FROM TIMESTAMPTZ_TBL diff --git a/src/test/regress/sql/timestamp.sql b/src/test/regress/sql/timestamp.sql index ebc969f36c..bc378036fe 100644 --- a/src/test/regress/sql/timestamp.sql +++ b/src/test/regress/sql/timestamp.sql @@ -259,6 +259,9 @@ FROM ( -- shift bins using the origin parameter: SELECT date_bin('5 min'::interval, timestamp '2020-02-01 01:01:01', timestamp '2020-02-01 00:02:30'); +-- test roundoff edge case when source < origin +SELECT date_bin('30 minutes'::interval, timestamp '2024-02-01 15:00:00', timestamp '2024-02-01 17:00:00'); + -- disallow intervals with months or years SELECT date_bin('5 months'::interval, timestamp '2020-02-01 01:01:01', timestamp '2001-01-01'); SELECT date_bin('5 years'::interval, timestamp '2020-02-01 01:01:01', timestamp '2001-01-01'); @@ -269,6 +272,11 @@ SELECT date_bin('0 days'::interval, timestamp '1970-01-01 01:00:00' , timestamp -- disallow negative intervals SELECT date_bin('-2 days'::interval, timestamp '1970-01-01 01:00:00' , timestamp '1970-01-01 00:00:00'); +-- test overflow cases +select date_bin('15 minutes'::interval, timestamp '294276-12-30', timestamp '4000-12-20 BC'); +select date_bin('200000000 days'::interval, '2024-02-01'::timestamp, '2024-01-01'::timestamp); +select date_bin('365000 days'::interval, '4400-01-01 BC'::timestamp, '4000-01-01 BC'::timestamp); + -- Test casting within a BETWEEN qualifier SELECT d1 - timestamp without time zone '1997-01-02' AS diff FROM TIMESTAMP_TBL diff --git a/src/test/regress/sql/timestamptz.sql b/src/test/regress/sql/timestamptz.sql index a107abc5a4..875479e496 100644 --- a/src/test/regress/sql/timestamptz.sql +++ b/src/test/regress/sql/timestamptz.sql @@ -234,6 +234,9 @@ FROM ( -- shift bins using the origin parameter: SELECT date_bin('5 min'::interval, timestamptz '2020-02-01 01:01:01+00', timestamptz '2020-02-01 00:02:30+00'); +-- test roundoff edge case when source < origin +SELECT date_bin('30 minutes'::interval, timestamptz '2024-02-01 15:00:00', timestamptz '2024-02-01 17:00:00'); + -- disallow intervals with months or years SELECT date_bin('5 months'::interval, timestamp with time zone '2020-02-01 01:01:01+00', timestamp with time zone '2001-01-01+00'); SELECT date_bin('5 years'::interval, timestamp with time zone '2020-02-01 01:01:01+00', timestamp with time zone '2001-01-01+00'); @@ -244,6 +247,11 @@ SELECT date_bin('0 days'::interval, timestamp with time zone '1970-01-01 01:00:0 -- disallow negative intervals SELECT date_bin('-2 days'::interval, timestamp with time zone '1970-01-01 01:00:00+00' , timestamp with time zone '1970-01-01 00:00:00+00'); +-- test overflow cases +select date_bin('15 minutes'::interval, timestamptz '294276-12-30', timestamptz '4000-12-20 BC'); +select date_bin('200000000 days'::interval, '2024-02-01'::timestamptz, '2024-01-01'::timestamptz); +select date_bin('365000 days'::interval, '4400-01-01 BC'::timestamptz, '4000-01-01 BC'::timestamptz); + -- Test casting within a BETWEEN qualifier SELECT d1 - timestamp with time zone '1997-01-02' AS diff FROM TIMESTAMPTZ_TBL