Skip to content

Commit d163fdb

Browse files
tglsfdctglsfdc
committed
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
1 parent 53c2a97 commit d163fdb

File tree

5 files changed

+97
-19
lines changed

5 files changed

+97
-19
lines changed

‎src/backend/utils/adt/timestamp.c

Lines changed: 53 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -4539,8 +4539,9 @@ timestamp_bin(PG_FUNCTION_ARGS)
45394539
Timestamp timestamp = PG_GETARG_TIMESTAMP(1);
45404540
Timestamp origin = PG_GETARG_TIMESTAMP(2);
45414541
Timestamp result,
4542-
tm_diff,
45434542
stride_usecs,
4543+
tm_diff,
4544+
tm_modulo,
45444545
tm_delta;
45454546

45464547
if (TIMESTAMP_NOT_FINITE(timestamp))
@@ -4561,24 +4562,40 @@ timestamp_bin(PG_FUNCTION_ARGS)
45614562
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
45624563
errmsg("timestamps cannot be binned into intervals containing months or years")));
45634564

4564-
stride_usecs = stride->day * USECS_PER_DAY + stride->time;
4565+
if (unlikely(pg_mul_s64_overflow(stride->day, USECS_PER_DAY, &stride_usecs)) ||
4566+
unlikely(pg_add_s64_overflow(stride_usecs, stride->time, &stride_usecs)))
4567+
ereport(ERROR,
4568+
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
4569+
errmsg("interval out of range")));
45654570

45664571
if (stride_usecs <= 0)
45674572
ereport(ERROR,
45684573
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
45694574
errmsg("stride must be greater than zero")));
45704575

4571-
tm_diff = timestamp - origin;
4572-
tm_delta = tm_diff - tm_diff % stride_usecs;
4576+
if (unlikely(pg_sub_s64_overflow(timestamp, origin, &tm_diff)))
4577+
ereport(ERROR,
4578+
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
4579+
errmsg("interval out of range")));
4580+
4581+
/* These calculations cannot overflow */
4582+
tm_modulo = tm_diff % stride_usecs;
4583+
tm_delta = tm_diff - tm_modulo;
4584+
result = origin + tm_delta;
45734585

45744586
/*
4575-
* Make sure the returned timestamp is at the start of the bin, even if
4576-
* the origin is in the future.
4587+
* We want to round towards -infinity, not 0, when tm_diff is negative and
4588+
* not a multiple of stride_usecs. This adjustment *can* cause overflow,
4589+
* since the result might now be out of the range origin .. timestamp.
45774590
*/
4578-
if (origin > timestamp && stride_usecs > 1)
4579-
tm_delta -= stride_usecs;
4580-
4581-
result = origin + tm_delta;
4591+
if (tm_modulo < 0)
4592+
{
4593+
if (unlikely(pg_sub_s64_overflow(result, stride_usecs, &result)) ||
4594+
!IS_VALID_TIMESTAMP(result))
4595+
ereport(ERROR,
4596+
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
4597+
errmsg("timestamp out of range")));
4598+
}
45824599

45834600
PG_RETURN_TIMESTAMP(result);
45844601
}
@@ -4729,6 +4746,7 @@ timestamptz_bin(PG_FUNCTION_ARGS)
47294746
TimestampTz result,
47304747
stride_usecs,
47314748
tm_diff,
4749+
tm_modulo,
47324750
tm_delta;
47334751

47344752
if (TIMESTAMP_NOT_FINITE(timestamp))
@@ -4749,24 +4767,40 @@ timestamptz_bin(PG_FUNCTION_ARGS)
47494767
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
47504768
errmsg("timestamps cannot be binned into intervals containing months or years")));
47514769

4752-
stride_usecs = stride->day * USECS_PER_DAY + stride->time;
4770+
if (unlikely(pg_mul_s64_overflow(stride->day, USECS_PER_DAY, &stride_usecs)) ||
4771+
unlikely(pg_add_s64_overflow(stride_usecs, stride->time, &stride_usecs)))
4772+
ereport(ERROR,
4773+
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
4774+
errmsg("interval out of range")));
47534775

47544776
if (stride_usecs <= 0)
47554777
ereport(ERROR,
47564778
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
47574779
errmsg("stride must be greater than zero")));
47584780

4759-
tm_diff = timestamp - origin;
4760-
tm_delta = tm_diff - tm_diff % stride_usecs;
4781+
if (unlikely(pg_sub_s64_overflow(timestamp, origin, &tm_diff)))
4782+
ereport(ERROR,
4783+
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
4784+
errmsg("interval out of range")));
4785+
4786+
/* These calculations cannot overflow */
4787+
tm_modulo = tm_diff % stride_usecs;
4788+
tm_delta = tm_diff - tm_modulo;
4789+
result = origin + tm_delta;
47614790

47624791
/*
4763-
* Make sure the returned timestamp is at the start of the bin, even if
4764-
* the origin is in the future.
4792+
* We want to round towards -infinity, not 0, when tm_diff is negative and
4793+
* not a multiple of stride_usecs. This adjustment *can* cause overflow,
4794+
* since the result might now be out of the range origin .. timestamp.
47654795
*/
4766-
if (origin > timestamp && stride_usecs > 1)
4767-
tm_delta -= stride_usecs;
4768-
4769-
result = origin + tm_delta;
4796+
if (tm_modulo < 0)
4797+
{
4798+
if (unlikely(pg_sub_s64_overflow(result, stride_usecs, &result)) ||
4799+
!IS_VALID_TIMESTAMP(result))
4800+
ereport(ERROR,
4801+
(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
4802+
errmsg("timestamp out of range")));
4803+
}
47704804

47714805
PG_RETURN_TIMESTAMPTZ(result);
47724806
}

‎src/test/regress/expected/timestamp.out

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -736,6 +736,13 @@ SELECT date_bin('5 min'::interval, timestamp '2020-02-01 01:01:01', timestamp '2
736736
Sat Feb 01 00:57:30 2020
737737
(1 row)
738738

739+
-- test roundoff edge case when source < origin
740+
SELECT date_bin('30 minutes'::interval, timestamp '2024-02-01 15:00:00', timestamp '2024-02-01 17:00:00');
741+
date_bin
742+
--------------------------
743+
Thu Feb 01 15:00:00 2024
744+
(1 row)
745+
739746
-- disallow intervals with months or years
740747
SELECT date_bin('5 months'::interval, timestamp '2020-02-01 01:01:01', timestamp '2001-01-01');
741748
ERROR: timestamps cannot be binned into intervals containing months or years
@@ -747,6 +754,13 @@ ERROR: stride must be greater than zero
747754
-- disallow negative intervals
748755
SELECT date_bin('-2 days'::interval, timestamp '1970-01-01 01:00:00' , timestamp '1970-01-01 00:00:00');
749756
ERROR: stride must be greater than zero
757+
-- test overflow cases
758+
select date_bin('15 minutes'::interval, timestamp '294276-12-30', timestamp '4000-12-20 BC');
759+
ERROR: interval out of range
760+
select date_bin('200000000 days'::interval, '2024-02-01'::timestamp, '2024-01-01'::timestamp);
761+
ERROR: interval out of range
762+
select date_bin('365000 days'::interval, '4400-01-01 BC'::timestamp, '4000-01-01 BC'::timestamp);
763+
ERROR: timestamp out of range
750764
-- Test casting within a BETWEEN qualifier
751765
SELECT d1 - timestamp without time zone '1997-01-02' AS diff
752766
FROM TIMESTAMP_TBL

‎src/test/regress/expected/timestamptz.out

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -780,6 +780,13 @@ SELECT date_bin('5 min'::interval, timestamptz '2020-02-01 01:01:01+00', timesta
780780
Fri Jan 31 16:57:30 2020 PST
781781
(1 row)
782782

783+
-- test roundoff edge case when source < origin
784+
SELECT date_bin('30 minutes'::interval, timestamptz '2024-02-01 15:00:00', timestamptz '2024-02-01 17:00:00');
785+
date_bin
786+
------------------------------
787+
Thu Feb 01 15:00:00 2024 PST
788+
(1 row)
789+
783790
-- disallow intervals with months or years
784791
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');
785792
ERROR: timestamps cannot be binned into intervals containing months or years
@@ -791,6 +798,13 @@ ERROR: stride must be greater than zero
791798
-- disallow negative intervals
792799
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');
793800
ERROR: stride must be greater than zero
801+
-- test overflow cases
802+
select date_bin('15 minutes'::interval, timestamptz '294276-12-30', timestamptz '4000-12-20 BC');
803+
ERROR: interval out of range
804+
select date_bin('200000000 days'::interval, '2024-02-01'::timestamptz, '2024-01-01'::timestamptz);
805+
ERROR: interval out of range
806+
select date_bin('365000 days'::interval, '4400-01-01 BC'::timestamptz, '4000-01-01 BC'::timestamptz);
807+
ERROR: timestamp out of range
794808
-- Test casting within a BETWEEN qualifier
795809
SELECT d1 - timestamp with time zone '1997-01-02' AS diff
796810
FROM TIMESTAMPTZ_TBL

‎src/test/regress/sql/timestamp.sql

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -268,6 +268,9 @@ FROM (
268268
-- shift bins using the origin parameter:
269269
SELECT date_bin('5 min'::interval, timestamp '2020-02-01 01:01:01', timestamp '2020-02-01 00:02:30');
270270

271+
-- test roundoff edge case when source < origin
272+
SELECT date_bin('30 minutes'::interval, timestamp '2024-02-01 15:00:00', timestamp '2024-02-01 17:00:00');
273+
271274
-- disallow intervals with months or years
272275
SELECT date_bin('5 months'::interval, timestamp '2020-02-01 01:01:01', timestamp '2001-01-01');
273276
SELECT date_bin('5 years'::interval, timestamp '2020-02-01 01:01:01', timestamp '2001-01-01');
@@ -278,6 +281,11 @@ SELECT date_bin('0 days'::interval, timestamp '1970-01-01 01:00:00' , timestamp
278281
-- disallow negative intervals
279282
SELECT date_bin('-2 days'::interval, timestamp '1970-01-01 01:00:00' , timestamp '1970-01-01 00:00:00');
280283

284+
-- test overflow cases
285+
select date_bin('15 minutes'::interval, timestamp '294276-12-30', timestamp '4000-12-20 BC');
286+
select date_bin('200000000 days'::interval, '2024-02-01'::timestamp, '2024-01-01'::timestamp);
287+
select date_bin('365000 days'::interval, '4400-01-01 BC'::timestamp, '4000-01-01 BC'::timestamp);
288+
281289
-- Test casting within a BETWEEN qualifier
282290
SELECT d1 - timestamp without time zone '1997-01-02' AS diff
283291
FROM TIMESTAMP_TBL

‎src/test/regress/sql/timestamptz.sql

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -243,6 +243,9 @@ FROM (
243243
-- shift bins using the origin parameter:
244244
SELECT date_bin('5 min'::interval, timestamptz '2020-02-01 01:01:01+00', timestamptz '2020-02-01 00:02:30+00');
245245

246+
-- test roundoff edge case when source < origin
247+
SELECT date_bin('30 minutes'::interval, timestamptz '2024-02-01 15:00:00', timestamptz '2024-02-01 17:00:00');
248+
246249
-- disallow intervals with months or years
247250
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');
248251
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');
@@ -253,6 +256,11 @@ SELECT date_bin('0 days'::interval, timestamp with time zone '1970-01-01 01:00:0
253256
-- disallow negative intervals
254257
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');
255258

259+
-- test overflow cases
260+
select date_bin('15 minutes'::interval, timestamptz '294276-12-30', timestamptz '4000-12-20 BC');
261+
select date_bin('200000000 days'::interval, '2024-02-01'::timestamptz, '2024-01-01'::timestamptz);
262+
select date_bin('365000 days'::interval, '4400-01-01 BC'::timestamptz, '4000-01-01 BC'::timestamptz);
263+
256264
-- Test casting within a BETWEEN qualifier
257265
SELECT d1 - timestamp with time zone '1997-01-02' AS diff
258266
FROM TIMESTAMPTZ_TBL

0 commit comments

Comments
 (0)