Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
> Joseph Koshakow <koshy44(at)gmail(dot)com> writes:
> > The attached patch fixes an overflow bug in DecodeInterval when applying
> > the units week, decade, century, and millennium. The overflow check logic
> > was modelled after the overflow check at the beginning of `int
> > tm2interval(struct pg_tm *tm, fsec_t fsec, Interval *span);` in timestamp.c.
>
>
> Good catch, but I don't think that tm2interval code is best practice
> anymore. Rather than bringing "double" arithmetic into the mix,
> you should use the overflow-detecting arithmetic functions in
> src/include/common/int.h. The existing code here is also pretty
> faulty in that it doesn't notice addition overflow when combining
> multiple units. So for example, instead of
>
>
> tm->tm_mday += val * 7;
>
>
> I think we should write something like
>
>
> if (pg_mul_s32_overflow(val, 7, &tmp))
> return DTERR_FIELD_OVERFLOW;
> if (pg_add_s32_overflow(tm->tm_mday, tmp, &tm->tm_mday))
> return DTERR_FIELD_OVERFLOW;
>
>
> Perhaps some macros could be used to make this more legible?
>
>
> regards, tom lane
>
>
> @postgresql
Thanks for the feedback Tom, I've attached an updated patch with
your suggestions. Feel free to rename the horribly named macro.
Also while fixing this I noticed that fractional intervals can also
cause an overflow issue.
postgres=# SELECT INTERVAL '0.1 months 2147483647 days';
interval
------------------
-2147483646 days
(1 row)
I haven't looked into it, but it's probably a similar cause.
From 49b5beb4132a0c9e793e1fd7b91a4da0c96a6196 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <kosh...@gmail.com>
Date: Fri, 11 Feb 2022 14:18:32 -0500
Subject: [PATCH] Check for overflow when decoding an interval
When decoding an interval there are various date units which are
aliases for some multiple of another unit. For example a week is 7 days
and a decade is 10 years. When decoding these specific units, there is
no check for overflow, allowing the interval to overflow. This commit
adds an overflow check for all of these units.
---
src/backend/utils/adt/datetime.c | 22 ++++++++++++++----
src/test/regress/expected/interval.out | 32 ++++++++++++++++++++++++++
src/test/regress/sql/interval.sql | 8 +++++++
3 files changed, 58 insertions(+), 4 deletions(-)
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index 7926258c06..59d391adda 100644
--- a/src/backend/utils/adt/datetime.c
+++ b/src/backend/utils/adt/datetime.c
@@ -21,6 +21,7 @@
#include "access/htup_details.h"
#include "access/xact.h"
#include "catalog/pg_type.h"
+#include "common/int.h"
#include "common/string.h"
#include "funcapi.h"
#include "miscadmin.h"
@@ -3106,6 +3107,19 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
int val;
double fval;
+ /*
+ * Multiply src by mult and add it to dest while checking for overflow.
+ */
+#define SAFE_MUL_ADD(src, mult, dst) \
+ do \
+ { \
+ int tmp; \
+ if (pg_mul_s32_overflow(src, mult, &tmp)) \
+ return DTERR_FIELD_OVERFLOW; \
+ if (pg_add_s32_overflow(dst, tmp, &(dst))) \
+ return DTERR_FIELD_OVERFLOW; \
+ } while (0)
+
*dtype = DTK_DELTA;
type = IGNORE_DTF;
ClearPgTm(tm, fsec);
@@ -3293,7 +3307,7 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
break;
case DTK_WEEK:
- tm->tm_mday += val * 7;
+ SAFE_MUL_ADD(val, 7, tm->tm_mday);
AdjustFractDays(fval, tm, fsec, 7);
tmask = DTK_M(WEEK);
break;
@@ -3311,19 +3325,19 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
break;
case DTK_DECADE:
- tm->tm_year += val * 10;
+ SAFE_MUL_ADD(val, 10, tm->tm_year);
tm->tm_mon += rint(fval * MONTHS_PER_YEAR * 10);
tmask = DTK_M(DECADE);
break;
case DTK_CENTURY:
- tm->tm_year += val * 100;
+ SAFE_MUL_ADD(val, 100, tm->tm_year);
tm->tm_mon += rint(fval * MONTHS_PER_YEAR * 100);
tmask = DTK_M(CENTURY);
break;
case DTK_MILLENNIUM:
- tm->tm_year += val * 1000;
+ SAFE_MUL_ADD(val, 1000, tm->tm_year);
tm->tm_mon += rint(fval * MONTHS_PER_YEAR * 1000);
tmask = DTK_M(MILLENNIUM);
break;
diff --git a/src/test/regress/expected/interval.out b/src/test/regress/expected/interval.out
index accd4a7d90..88865483fa 100644
--- a/src/test/regress/expected/interval.out
+++ b/src/test/regress/expected/interval.out
@@ -232,6 +232,38 @@ INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 years');
ERROR: interval out of range
LINE 1: INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 years'...
^
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('2147483647 weeks');
+ERROR: interval field value out of range: "2147483647 weeks"
+LINE 1: INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('2147483647 weeks')...
+ ^
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 weeks');
+ERROR: interval field value out of range: "-2147483648 weeks"
+LINE 1: INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 weeks'...
+ ^
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('2147483647 decades');
+ERROR: interval field value out of range: "2147483647 decades"
+LINE 1: INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('2147483647 decades...
+ ^
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 decades');
+ERROR: interval field value out of range: "-2147483648 decades"
+LINE 1: INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 decade...
+ ^
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('2147483647 centuries');
+ERROR: interval field value out of range: "2147483647 centuries"
+LINE 1: INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('2147483647 centuri...
+ ^
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 centuries');
+ERROR: interval field value out of range: "-2147483648 centuries"
+LINE 1: INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 centur...
+ ^
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('2147483647 millennium');
+ERROR: interval field value out of range: "2147483647 millennium"
+LINE 1: INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('2147483647 millenn...
+ ^
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 millennium');
+ERROR: interval field value out of range: "-2147483648 millennium"
+LINE 1: INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 millen...
+ ^
-- Test edge-case overflow detection in interval multiplication
select extract(epoch from '256 microseconds'::interval * (2^55)::float8);
ERROR: interval out of range
diff --git a/src/test/regress/sql/interval.sql b/src/test/regress/sql/interval.sql
index 6d532398bd..70b642cef2 100644
--- a/src/test/regress/sql/interval.sql
+++ b/src/test/regress/sql/interval.sql
@@ -72,6 +72,14 @@ INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('2147483648 days');
INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483649 days');
INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('2147483647 years');
INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 years');
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('2147483647 weeks');
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 weeks');
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('2147483647 decades');
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 decades');
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('2147483647 centuries');
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 centuries');
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('2147483647 millennium');
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('-2147483648 millennium');
-- Test edge-case overflow detection in interval multiplication
select extract(epoch from '256 microseconds'::interval * (2^55)::float8);
--
2.25.1
