On Fri, Feb 11, 2022 at 4:58 PM Joseph Koshakow <kosh...@gmail.com> wrote:
>
> 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.
Hey Tom,
I was originally going to fix the fractional issue in a follow-up
patch, but I took a look at it and decided to make a new patch
with both fixes. I tried to make the handling of fractional and
non-fractional units consistent. I've attached the patch below.
While investigating this, I've noticed a couple more overflow
issues with Intervals, but I think they're best handled in separate
patches. I've included the ones I've found in this email.
postgres=# CREATE TEMP TABLE INTERVAL_TBL_OF (f1 interval);
CREATE TABLE
postgres=# INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('0.1 weeks
2147483647 hrs');
INSERT 0 1
postgres=# SELECT * FROM INTERVAL_TBL_OF;
ERROR: interval out of range
postgres=# SELECT justify_days(INTERVAL '2147483647 months 2147483647 days');
justify_days
-----------------------------------
-172991738 years -4 mons -23 days
(1 row)
Cheers,
Joe Koshakow
From de041874b73600312e7ce71931c297f0aad3aa06 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.
Additionally fractional date/time units are rolled down into the next
smallest unit. For example 0.1 months is 3 days. When adding these
fraction units, there is no check for overflow, allowing the interval
to overflow. This commit adds an overflow check for all of the
fractional units.
Signed-off-by: Joseph Koshakow <kosh...@gmail.com>
---
src/backend/utils/adt/datetime.c | 103 +++++++++++++++++++------
src/test/regress/expected/interval.out | 56 ++++++++++++++
src/test/regress/sql/interval.sql | 15 ++++
3 files changed, 152 insertions(+), 22 deletions(-)
diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c
index 7926258c06..f89cc7c29b 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"
@@ -44,10 +45,13 @@ static int DecodeDate(char *str, int fmask, int *tmask, bool *is2digits,
struct pg_tm *tm);
static char *AppendSeconds(char *cp, int sec, fsec_t fsec,
int precision, bool fillzeros);
-static void AdjustFractSeconds(double frac, struct pg_tm *tm, fsec_t *fsec,
+static int AdjustFractSeconds(double frac, struct pg_tm *tm, fsec_t *fsec,
int scale);
-static void AdjustFractDays(double frac, struct pg_tm *tm, fsec_t *fsec,
+static int AdjustFractDays(double frac, struct pg_tm *tm, fsec_t *fsec,
int scale);
+static int AdjustFractMonths(double frac, struct pg_tm *tm, int scale);
+static int AdjustDays(int val, struct pg_tm *tm, int multiplier);
+static int AdjustYears(int val, struct pg_tm *tm, int multiplier);
static int DetermineTimeZoneOffsetInternal(struct pg_tm *tm, pg_tz *tzp,
pg_time_t *tp);
static bool DetermineTimeZoneAbbrevOffsetInternal(pg_time_t t,
@@ -499,34 +503,76 @@ AppendTimestampSeconds(char *cp, struct pg_tm *tm, fsec_t fsec)
/*
* Multiply frac by scale (to produce seconds) and add to *tm & *fsec.
* We assume the input frac is less than 1 so overflow is not an issue.
+ * Returns 0 if successful, 1 if tm overflows.
*/
-static void
+static int
AdjustFractSeconds(double frac, struct pg_tm *tm, fsec_t *fsec, int scale)
{
int sec;
if (frac == 0)
- return;
+ return 0;
frac *= scale;
sec = (int) frac;
- tm->tm_sec += sec;
+ if (pg_add_s32_overflow(tm->tm_sec, sec, &tm->tm_sec))
+ return 1;
frac -= sec;
*fsec += rint(frac * 1000000);
+ return 0;
}
/* As above, but initial scale produces days */
-static void
+static int
AdjustFractDays(double frac, struct pg_tm *tm, fsec_t *fsec, int scale)
{
int extra_days;
if (frac == 0)
- return;
+ return 0;
frac *= scale;
extra_days = (int) frac;
- tm->tm_mday += extra_days;
+ if (pg_add_s32_overflow(tm->tm_mday, extra_days, &tm->tm_mday))
+ return 1;
frac -= extra_days;
- AdjustFractSeconds(frac, tm, fsec, SECS_PER_DAY);
+ return AdjustFractSeconds(frac, tm, fsec, SECS_PER_DAY);
+}
+
+/* As above, but initial scale produces months */
+static int
+AdjustFractMonths(double frac, struct pg_tm *tm, int scale)
+{
+ int months = rint(frac * MONTHS_PER_YEAR * scale);
+
+ if (pg_add_s32_overflow(tm->tm_mon, months, &tm->tm_mon))
+ return 1;
+ return 0;
+}
+
+/*
+ * Multiply val by multiplier (to produce days) and add to *tm.
+ * Returns 0 if successful, 1 if tm overflows.
+ */
+static int
+AdjustDays(int val, struct pg_tm *tm, int multiplier)
+{
+ int extra_days;
+ if (pg_mul_s32_overflow(val, multiplier, &extra_days))
+ return 1;
+ if (pg_add_s32_overflow(tm->tm_mday, extra_days, &tm->tm_mday))
+ return 1;
+ return 0;
+}
+
+/* As above, but initial val produces years */
+static int
+AdjustYears(int val, struct pg_tm *tm, int multiplier)
+{
+ int years;
+ if (pg_mul_s32_overflow(val, multiplier, &years))
+ return 1;
+ if (pg_add_s32_overflow(tm->tm_year, years, &tm->tm_year))
+ return 1;
+ return 0;
}
/* Fetch a fractional-second value with suitable error checking */
@@ -3275,56 +3321,69 @@ DecodeInterval(char **field, int *ftype, int nf, int range,
case DTK_MINUTE:
tm->tm_min += val;
- AdjustFractSeconds(fval, tm, fsec, SECS_PER_MINUTE);
+ if (AdjustFractSeconds(fval, tm, fsec, SECS_PER_MINUTE))
+ return DTERR_FIELD_OVERFLOW;
tmask = DTK_M(MINUTE);
break;
case DTK_HOUR:
tm->tm_hour += val;
- AdjustFractSeconds(fval, tm, fsec, SECS_PER_HOUR);
+ if (AdjustFractSeconds(fval, tm, fsec, SECS_PER_HOUR))
+ return DTERR_FIELD_OVERFLOW;
tmask = DTK_M(HOUR);
type = DTK_DAY; /* set for next field */
break;
case DTK_DAY:
tm->tm_mday += val;
- AdjustFractSeconds(fval, tm, fsec, SECS_PER_DAY);
+ if (AdjustFractSeconds(fval, tm, fsec, SECS_PER_DAY))
+ return DTERR_FIELD_OVERFLOW;
tmask = DTK_M(DAY);
break;
case DTK_WEEK:
- tm->tm_mday += val * 7;
- AdjustFractDays(fval, tm, fsec, 7);
+ if (AdjustDays(val, tm, 7))
+ return DTERR_FIELD_OVERFLOW;
+ if (AdjustFractDays(fval, tm, fsec, 7))
+ return DTERR_FIELD_OVERFLOW;
tmask = DTK_M(WEEK);
break;
case DTK_MONTH:
tm->tm_mon += val;
- AdjustFractDays(fval, tm, fsec, DAYS_PER_MONTH);
+ if (AdjustFractDays(fval, tm, fsec, DAYS_PER_MONTH))
+ return DTERR_FIELD_OVERFLOW;
tmask = DTK_M(MONTH);
break;
case DTK_YEAR:
tm->tm_year += val;
- tm->tm_mon += rint(fval * MONTHS_PER_YEAR);
+ if (AdjustFractMonths(fval, tm, 1))
+ return DTERR_FIELD_OVERFLOW;
tmask = DTK_M(YEAR);
break;
case DTK_DECADE:
- tm->tm_year += val * 10;
- tm->tm_mon += rint(fval * MONTHS_PER_YEAR * 10);
+ if (AdjustYears(val, tm, 10))
+ return DTERR_FIELD_OVERFLOW;
+ if (AdjustFractMonths(fval, tm, 10))
+ return DTERR_FIELD_OVERFLOW;
tmask = DTK_M(DECADE);
break;
case DTK_CENTURY:
- tm->tm_year += val * 100;
- tm->tm_mon += rint(fval * MONTHS_PER_YEAR * 100);
+ if (AdjustYears(val, tm, 100))
+ return DTERR_FIELD_OVERFLOW;
+ if (AdjustFractMonths(fval, tm, 100))
+ return DTERR_FIELD_OVERFLOW;
tmask = DTK_M(CENTURY);
break;
case DTK_MILLENNIUM:
- tm->tm_year += val * 1000;
- tm->tm_mon += rint(fval * MONTHS_PER_YEAR * 1000);
+ if (AdjustYears(val, tm, 1000))
+ return DTERR_FIELD_OVERFLOW;
+ if (AdjustFractMonths(fval, tm, 1000))
+ return DTERR_FIELD_OVERFLOW;
tmask = DTK_M(MILLENNIUM);
break;
diff --git a/src/test/regress/expected/interval.out b/src/test/regress/expected/interval.out
index accd4a7d90..de39ed3b6a 100644
--- a/src/test/regress/expected/interval.out
+++ b/src/test/regress/expected/interval.out
@@ -232,6 +232,62 @@ 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...
+ ^
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('0.1 millennium 2147483647 months');
+ERROR: interval field value out of range: "0.1 millennium 2147483647 months"
+LINE 1: INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('0.1 millennium 214...
+ ^
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('0.1 centuries 2147483647 months');
+ERROR: interval field value out of range: "0.1 centuries 2147483647 months"
+LINE 1: INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('0.1 centuries 2147...
+ ^
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('0.1 decades 2147483647 months');
+ERROR: interval field value out of range: "0.1 decades 2147483647 months"
+LINE 1: INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('0.1 decades 214748...
+ ^
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('0.1 yrs 2147483647 months');
+ERROR: interval field value out of range: "0.1 yrs 2147483647 months"
+LINE 1: INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('0.1 yrs 2147483647...
+ ^
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('0.1 months 2147483647 days');
+ERROR: interval field value out of range: "0.1 months 2147483647 days"
+LINE 1: INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('0.1 months 2147483...
+ ^
+SELECT INTERVAL '0.1 weeks 2147483647 hrs';
+ERROR: interval out of range
+SELECT INTERVAL '0.1 days 2147483647 hrs';
+ERROR: interval out of range
-- 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..7307da824c 100644
--- a/src/test/regress/sql/interval.sql
+++ b/src/test/regress/sql/interval.sql
@@ -72,6 +72,21 @@ 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');
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('0.1 millennium 2147483647 months');
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('0.1 centuries 2147483647 months');
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('0.1 decades 2147483647 months');
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('0.1 yrs 2147483647 months');
+INSERT INTO INTERVAL_TBL_OF (f1) VALUES ('0.1 months 2147483647 days');
+SELECT INTERVAL '0.1 weeks 2147483647 hrs';
+SELECT INTERVAL '0.1 days 2147483647 hrs';
-- Test edge-case overflow detection in interval multiplication
select extract(epoch from '256 microseconds'::interval * (2^55)::float8);
--
2.25.1
