Fix overflow in justify_interval related functions

Started by Joseph Koshakowalmost 4 years ago12 messages

koshy44@gmail.com

almost 4 years ago

1 attachment(s)

I mentioned this issue briefly in another thread, but the
justify_interval, justify_days, and justify_hours functions
all have the potential to overflow. The attached patch fixes
this issue.

Cheers,
Joe Koshakow

Attachments:

0001-Check-for-overflow-in-justify_interval-functions.patchtext/x-patch; charset=US-ASCII; name=0001-Check-for-overflow-in-justify_interval-functions.patchDownload

From 4400f2e6886097e3b75d455aeec1ffa9cbc88510 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <koshy44@gmail.com>
Date: Sun, 13 Feb 2022 13:26:16 -0500
Subject: [PATCH] Check for overflow in justify_interval functions

justify_interval, justify_hours, and justify_days didn't check for
overflow when moving hours to days or days to hours. This commit adds
check for overflow and returns an appropriate error.
---
 src/backend/utils/adt/timestamp.c      | 20 ++++++++++++++++----
 src/test/regress/expected/interval.out |  8 ++++++++
 src/test/regress/sql/interval.sql      |  6 ++++++
 3 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 36f8a84bcc..fb4b3ae0aa 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -2718,11 +2718,17 @@ interval_justify_interval(PG_FUNCTION_ARGS)
 	result->time = span->time;
 
 	TMODULO(result->time, wholeday, USECS_PER_DAY);
-	result->day += wholeday;	/* could overflow... */
+	if (pg_add_s32_overflow(result->day, wholeday, &result->day))
+		ereport(ERROR,
+				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+				errmsg("interval out of range")));
 
 	wholemonth = result->day / DAYS_PER_MONTH;
 	result->day -= wholemonth * DAYS_PER_MONTH;
-	result->month += wholemonth;
+	if (pg_add_s32_overflow(result->month, wholemonth, &result->month))
+		ereport(ERROR,
+				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+				errmsg("interval out of range")));
 
 	if (result->month > 0 &&
 		(result->day < 0 || (result->day == 0 && result->time < 0)))
@@ -2772,7 +2778,10 @@ interval_justify_hours(PG_FUNCTION_ARGS)
 	result->time = span->time;
 
 	TMODULO(result->time, wholeday, USECS_PER_DAY);
-	result->day += wholeday;	/* could overflow... */
+	if (pg_add_s32_overflow(result->day, wholeday, &result->day))
+		ereport(ERROR,
+				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+				errmsg("interval out of range")));
 
 	if (result->day > 0 && result->time < 0)
 	{
@@ -2808,7 +2817,10 @@ interval_justify_days(PG_FUNCTION_ARGS)
 
 	wholemonth = result->day / DAYS_PER_MONTH;
 	result->day -= wholemonth * DAYS_PER_MONTH;
-	result->month += wholemonth;
+	if (pg_add_s32_overflow(result->month, wholemonth, &result->month))
+		ereport(ERROR,
+				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+				errmsg("interval out of range")));
 
 	if (result->month > 0 && result->day < 0)
 	{
diff --git a/src/test/regress/expected/interval.out b/src/test/regress/expected/interval.out
index accd4a7d90..e2bb8095c3 100644
--- a/src/test/regress/expected/interval.out
+++ b/src/test/regress/expected/interval.out
@@ -396,6 +396,10 @@ SELECT justify_days(interval '6 months 36 days 5 hours 4 minutes 3 seconds') as
  @ 7 mons 6 days 5 hours 4 mins 3 secs
 (1 row)
 
+SELECT justify_hours(interval '2147483647 days 24 hrs');
+ERROR:  interval out of range
+SELECT justify_days(interval '2147483647 months 30 days');
+ERROR:  interval out of range
 -- test justify_interval()
 SELECT justify_interval(interval '1 month -1 hour') as "1 month -1 hour";
   1 month -1 hour   
@@ -403,6 +407,10 @@ SELECT justify_interval(interval '1 month -1 hour') as "1 month -1 hour";
  @ 29 days 23 hours
 (1 row)
 
+SELECT justify_interval(interval '2147483647 days 24 hrs');
+ERROR:  interval out of range
+SELECT justify_interval(interval '2147483647 months 30 days');
+ERROR:  interval out of range
 -- test fractional second input, and detection of duplicate units
 SET DATESTYLE = 'ISO';
 SET IntervalStyle TO postgres;
diff --git a/src/test/regress/sql/interval.sql b/src/test/regress/sql/interval.sql
index 6d532398bd..3ebdca5023 100644
--- a/src/test/regress/sql/interval.sql
+++ b/src/test/regress/sql/interval.sql
@@ -149,10 +149,16 @@ select '100000000y 10mon -1000000000d -100000h -10min -10.000001s ago'::interval
 SELECT justify_hours(interval '6 months 3 days 52 hours 3 minutes 2 seconds') as "6 mons 5 days 4 hours 3 mins 2 seconds";
 SELECT justify_days(interval '6 months 36 days 5 hours 4 minutes 3 seconds') as "7 mons 6 days 5 hours 4 mins 3 seconds";
 
+SELECT justify_hours(interval '2147483647 days 24 hrs');
+SELECT justify_days(interval '2147483647 months 30 days');
+
 -- test justify_interval()
 
 SELECT justify_interval(interval '1 month -1 hour') as "1 month -1 hour";
 
+SELECT justify_interval(interval '2147483647 days 24 hrs');
+SELECT justify_interval(interval '2147483647 months 30 days');
+
 -- test fractional second input, and detection of duplicate units
 SET DATESTYLE = 'ISO';
 SET IntervalStyle TO postgres;
-- 
2.25.1

Nathan Bossart

nathandbossart@gmail.com

almost 4 years ago

In reply to: Joseph Koshakow (#1)

Re: Fix overflow in justify_interval related functions

On Sun, Feb 13, 2022 at 01:28:38PM -0500, Joseph Koshakow wrote:

+SELECT justify_hours(interval '2147483647 days 24 hrs');
+ERROR:  interval out of range

The docs [0]https://www.postgresql.org/docs/devel/datatype-datetime.html claim that the maximum value for interval is 178 million
years, but this test case is only ~6 million. Should we instead rework the
logic to avoid overflow for this case?

[0]: https://www.postgresql.org/docs/devel/datatype-datetime.html

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Tom Lane

tgl@sss.pgh.pa.us

almost 4 years ago

In reply to: Nathan Bossart (#2)

Re: Fix overflow in justify_interval related functions

Nathan Bossart <nathandbossart@gmail.com> writes:

On Sun, Feb 13, 2022 at 01:28:38PM -0500, Joseph Koshakow wrote:
+SELECT justify_hours(interval '2147483647 days 24 hrs');
+ERROR:  interval out of range

The docs [0] claim that the maximum value for interval is 178 million
years, but this test case is only ~6 million. Should we instead rework the
logic to avoid overflow for this case?

I think the docs are misleading you on this point. The maximum
value of the months part of an interval is 2^31 months or
about 178Myr, but what we're dealing with here is days, which
likewise caps at 2^31 days. justify_hours is not chartered
to transpose up to months, so it can't avoid that limit.

regards, tom lane

Nathan Bossart

nathandbossart@gmail.com

almost 4 years ago

In reply to: Tom Lane (#3)

Re: Fix overflow in justify_interval related functions

On Mon, Feb 14, 2022 at 01:55:56PM -0500, Tom Lane wrote:

Nathan Bossart <nathandbossart@gmail.com> writes:
On Sun, Feb 13, 2022 at 01:28:38PM -0500, Joseph Koshakow wrote:
+SELECT justify_hours(interval '2147483647 days 24 hrs');
+ERROR:  interval out of range
The docs [0] claim that the maximum value for interval is 178 million
years, but this test case is only ~6 million. Should we instead rework the
logic to avoid overflow for this case?

I think the docs are misleading you on this point. The maximum
value of the months part of an interval is 2^31 months or
about 178Myr, but what we're dealing with here is days, which
likewise caps at 2^31 days. justify_hours is not chartered
to transpose up to months, so it can't avoid that limit.

Makes sense. So we could likely avoid it for justify_interval, but the
others are at the mercy of the interval implementation.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Joseph Koshakow

koshy44@gmail.com

almost 4 years ago

In reply to: Nathan Bossart (#4)

Re: Fix overflow in justify_interval related functions

On Mon, Feb 14, 2022 at 2:15 PM Nathan Bossart <nathandbossart@gmail.com> wrote:

Makes sense. So we could likely avoid it for justify_interval, but the
others are at the mercy of the interval implementation.

I'm not entirely sure what you mean by "it", but for both
justify_interval and justify_days this commit throws an error if we
try to set the months field above 2^31.

+SELECT justify_days(interval '2147483647 months 30 days');
+ERROR:  interval out of range
+SELECT justify_interval(interval '2147483647 months 30 days');
+ERROR:  interval out of range

That's what these are testing.

- Joe Koshakow

Nathan Bossart

nathandbossart@gmail.com

almost 4 years ago

In reply to: Joseph Koshakow (#5)

1 attachment(s)

Re: Fix overflow in justify_interval related functions

On Mon, Feb 14, 2022 at 04:57:07PM -0500, Joseph Koshakow wrote:

On Mon, Feb 14, 2022 at 2:15 PM Nathan Bossart <nathandbossart@gmail.com> wrote:

Makes sense. So we could likely avoid it for justify_interval, but the
others are at the mercy of the interval implementation.

I'm not entirely sure what you mean by "it", but for both
justify_interval and justify_days this commit throws an error if we
try to set the months field above 2^31.
+SELECT justify_days(interval '2147483647 months 30 days');
+ERROR:  interval out of range
+SELECT justify_interval(interval '2147483647 months 30 days');
+ERROR:  interval out of range
That's what these are testing.

I think it's possible to avoid overflow in justify_interval() in some cases
by pre-justifying the days. I've attached a patch to demonstrate.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Attachments:

v2-0001-Check-for-overflow-in-justify_interval-functions.patchtext/x-diff; charset=us-asciiDownload

From d566fcc8cf0179bb915db828b528df55484b63dc Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <koshy44@gmail.com>
Date: Sun, 13 Feb 2022 13:26:16 -0500
Subject: [PATCH v2 1/1] Check for overflow in justify_interval functions

justify_interval, justify_hours, and justify_days didn't check for
overflow when moving hours to days or days to hours. This commit adds
check for overflow and returns an appropriate error.
---
 src/backend/utils/adt/timestamp.c      | 32 ++++++++++++++++++++---
 src/test/regress/expected/interval.out | 36 ++++++++++++++++++++++++++
 src/test/regress/sql/interval.sql      | 12 +++++++++
 3 files changed, 76 insertions(+), 4 deletions(-)

diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 36f8a84bcc..8cda47e9a4 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -2717,12 +2717,30 @@ interval_justify_interval(PG_FUNCTION_ARGS)
 	result->day = span->day;
 	result->time = span->time;
 
+	/* pre-justify days if it might prevent overflow */
+	if ((result->day > 0 && result->time > 0) ||
+		(result->day < 0 && result->time < 0))
+	{
+		wholemonth = result->day / DAYS_PER_MONTH;
+		result->day -= wholemonth * DAYS_PER_MONTH;
+		if (pg_add_s32_overflow(result->month, wholemonth, &result->month))
+			ereport(ERROR,
+					(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+					 errmsg("interval out of range")));
+	}
+
 	TMODULO(result->time, wholeday, USECS_PER_DAY);
-	result->day += wholeday;	/* could overflow... */
+	if (pg_add_s32_overflow(result->day, wholeday, &result->day))
+		ereport(ERROR,
+				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+				errmsg("interval out of range")));
 
 	wholemonth = result->day / DAYS_PER_MONTH;
 	result->day -= wholemonth * DAYS_PER_MONTH;
-	result->month += wholemonth;
+	if (pg_add_s32_overflow(result->month, wholemonth, &result->month))
+		ereport(ERROR,
+				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+				errmsg("interval out of range")));
 
 	if (result->month > 0 &&
 		(result->day < 0 || (result->day == 0 && result->time < 0)))
@@ -2772,7 +2790,10 @@ interval_justify_hours(PG_FUNCTION_ARGS)
 	result->time = span->time;
 
 	TMODULO(result->time, wholeday, USECS_PER_DAY);
-	result->day += wholeday;	/* could overflow... */
+	if (pg_add_s32_overflow(result->day, wholeday, &result->day))
+		ereport(ERROR,
+				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+				errmsg("interval out of range")));
 
 	if (result->day > 0 && result->time < 0)
 	{
@@ -2808,7 +2829,10 @@ interval_justify_days(PG_FUNCTION_ARGS)
 
 	wholemonth = result->day / DAYS_PER_MONTH;
 	result->day -= wholemonth * DAYS_PER_MONTH;
-	result->month += wholemonth;
+	if (pg_add_s32_overflow(result->month, wholemonth, &result->month))
+		ereport(ERROR,
+				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+				errmsg("interval out of range")));
 
 	if (result->month > 0 && result->day < 0)
 	{
diff --git a/src/test/regress/expected/interval.out b/src/test/regress/expected/interval.out
index accd4a7d90..146f7c55d0 100644
--- a/src/test/regress/expected/interval.out
+++ b/src/test/regress/expected/interval.out
@@ -396,6 +396,10 @@ SELECT justify_days(interval '6 months 36 days 5 hours 4 minutes 3 seconds') as
  @ 7 mons 6 days 5 hours 4 mins 3 secs
 (1 row)
 
+SELECT justify_hours(interval '2147483647 days 24 hrs');
+ERROR:  interval out of range
+SELECT justify_days(interval '2147483647 months 30 days');
+ERROR:  interval out of range
 -- test justify_interval()
 SELECT justify_interval(interval '1 month -1 hour') as "1 month -1 hour";
   1 month -1 hour   
@@ -403,6 +407,38 @@ SELECT justify_interval(interval '1 month -1 hour') as "1 month -1 hour";
  @ 29 days 23 hours
 (1 row)
 
+SELECT justify_interval(interval '2147483647 days 24 hrs');
+       justify_interval        
+-------------------------------
+ @ 5965232 years 4 mons 8 days
+(1 row)
+
+SELECT justify_interval(interval '-2147483648 days -24 hrs');
+         justify_interval          
+-----------------------------------
+ @ 5965232 years 4 mons 9 days ago
+(1 row)
+
+SELECT justify_interval(interval '2147483647 months 30 days');
+ERROR:  interval out of range
+SELECT justify_interval(interval '-2147483648 months -30 days');
+ERROR:  interval out of range
+SELECT justify_interval(interval '2147483647 months 30 days -24 hrs');
+         justify_interval         
+----------------------------------
+ @ 178956970 years 7 mons 29 days
+(1 row)
+
+SELECT justify_interval(interval '-2147483648 months -30 days 24 hrs');
+           justify_interval           
+--------------------------------------
+ @ 178956970 years 8 mons 29 days ago
+(1 row)
+
+SELECT justify_interval(interval '2147483647 months -30 days 1440 hrs');
+ERROR:  interval out of range
+SELECT justify_interval(interval '-2147483648 months 30 days -1440 hrs');
+ERROR:  interval out of range
 -- test fractional second input, and detection of duplicate units
 SET DATESTYLE = 'ISO';
 SET IntervalStyle TO postgres;
diff --git a/src/test/regress/sql/interval.sql b/src/test/regress/sql/interval.sql
index 6d532398bd..c31f0eec05 100644
--- a/src/test/regress/sql/interval.sql
+++ b/src/test/regress/sql/interval.sql
@@ -149,10 +149,22 @@ select '100000000y 10mon -1000000000d -100000h -10min -10.000001s ago'::interval
 SELECT justify_hours(interval '6 months 3 days 52 hours 3 minutes 2 seconds') as "6 mons 5 days 4 hours 3 mins 2 seconds";
 SELECT justify_days(interval '6 months 36 days 5 hours 4 minutes 3 seconds') as "7 mons 6 days 5 hours 4 mins 3 seconds";
 
+SELECT justify_hours(interval '2147483647 days 24 hrs');
+SELECT justify_days(interval '2147483647 months 30 days');
+
 -- test justify_interval()
 
 SELECT justify_interval(interval '1 month -1 hour') as "1 month -1 hour";
 
+SELECT justify_interval(interval '2147483647 days 24 hrs');
+SELECT justify_interval(interval '-2147483648 days -24 hrs');
+SELECT justify_interval(interval '2147483647 months 30 days');
+SELECT justify_interval(interval '-2147483648 months -30 days');
+SELECT justify_interval(interval '2147483647 months 30 days -24 hrs');
+SELECT justify_interval(interval '-2147483648 months -30 days 24 hrs');
+SELECT justify_interval(interval '2147483647 months -30 days 1440 hrs');
+SELECT justify_interval(interval '-2147483648 months 30 days -1440 hrs');
+
 -- test fractional second input, and detection of duplicate units
 SET DATESTYLE = 'ISO';
 SET IntervalStyle TO postgres;
-- 
2.25.1

Joseph Koshakow

koshy44@gmail.com

almost 4 years ago

In reply to: Nathan Bossart (#6)

1 attachment(s)

Re: Fix overflow in justify_interval related functions

On Mon, Feb 14, 2022 at 7:59 PM Nathan Bossart <nathandbossart@gmail.com> wrote:

I think it's possible to avoid overflow in justify_interval() in some cases
by pre-justifying the days. I've attached a patch to demonstrate.

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Good catch, I didn't think about that. Though if you are pre-justifying
the days, then I don't think it's possible for the second addition to
days to overflow. The maximum amount the days field could be after
the first justification is 29. I think we can simplify it further to the
attached patch.

- Joe

Attachments:

v3-0001-Check-for-overflow-in-justify_interval-functions.patchtext/x-patch; charset=US-ASCII; name=v3-0001-Check-for-overflow-in-justify_interval-functions.patchDownload

From 8ff8302b90c790d967f770286dfa54432f63662c Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <koshy44@gmail.com>
Date: Sun, 13 Feb 2022 13:26:16 -0500
Subject: [PATCH] Check for overflow in justify_interval functions

justify_interval, justify_hours, and justify_days didn't check for
overflow when moving hours to days or days to hours. This commit adds
check for overflow and returns an appropriate error.

Signed-off-by: Joseph Koshakow <koshy44@gmail.com>
---
 src/backend/utils/adt/timestamp.c      | 29 ++++++++++++++++++---
 src/test/regress/expected/interval.out | 36 ++++++++++++++++++++++++++
 src/test/regress/sql/interval.sql      | 12 +++++++++
 3 files changed, 73 insertions(+), 4 deletions(-)

diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 36f8a84bcc..ce11fcbeef 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -2717,12 +2717,27 @@ interval_justify_interval(PG_FUNCTION_ARGS)
 	result->day = span->day;
 	result->time = span->time;
 
+	/* pre-justify days if it might prevent overflow */
+	if ((result->day > 0 && result->time > 0) ||
+		(result->day < 0 && result->time < 0))
+	{
+		wholemonth = result->day / DAYS_PER_MONTH;
+		result->day -= wholemonth * DAYS_PER_MONTH;
+		if (pg_add_s32_overflow(result->month, wholemonth, &result->month))
+			ereport(ERROR,
+					(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+					 errmsg("interval out of range")));
+	}
+
 	TMODULO(result->time, wholeday, USECS_PER_DAY);
-	result->day += wholeday;	/* could overflow... */
+	result->day += wholeday;
 
 	wholemonth = result->day / DAYS_PER_MONTH;
 	result->day -= wholemonth * DAYS_PER_MONTH;
-	result->month += wholemonth;
+	if (pg_add_s32_overflow(result->month, wholemonth, &result->month))
+		ereport(ERROR,
+				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+				errmsg("interval out of range")));
 
 	if (result->month > 0 &&
 		(result->day < 0 || (result->day == 0 && result->time < 0)))
@@ -2772,7 +2787,10 @@ interval_justify_hours(PG_FUNCTION_ARGS)
 	result->time = span->time;
 
 	TMODULO(result->time, wholeday, USECS_PER_DAY);
-	result->day += wholeday;	/* could overflow... */
+	if (pg_add_s32_overflow(result->day, wholeday, &result->day))
+		ereport(ERROR,
+				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+				errmsg("interval out of range")));
 
 	if (result->day > 0 && result->time < 0)
 	{
@@ -2808,7 +2826,10 @@ interval_justify_days(PG_FUNCTION_ARGS)
 
 	wholemonth = result->day / DAYS_PER_MONTH;
 	result->day -= wholemonth * DAYS_PER_MONTH;
-	result->month += wholemonth;
+	if (pg_add_s32_overflow(result->month, wholemonth, &result->month))
+		ereport(ERROR,
+				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+				errmsg("interval out of range")));
 
 	if (result->month > 0 && result->day < 0)
 	{
diff --git a/src/test/regress/expected/interval.out b/src/test/regress/expected/interval.out
index accd4a7d90..146f7c55d0 100644
--- a/src/test/regress/expected/interval.out
+++ b/src/test/regress/expected/interval.out
@@ -396,6 +396,10 @@ SELECT justify_days(interval '6 months 36 days 5 hours 4 minutes 3 seconds') as
  @ 7 mons 6 days 5 hours 4 mins 3 secs
 (1 row)
 
+SELECT justify_hours(interval '2147483647 days 24 hrs');
+ERROR:  interval out of range
+SELECT justify_days(interval '2147483647 months 30 days');
+ERROR:  interval out of range
 -- test justify_interval()
 SELECT justify_interval(interval '1 month -1 hour') as "1 month -1 hour";
   1 month -1 hour   
@@ -403,6 +407,38 @@ SELECT justify_interval(interval '1 month -1 hour') as "1 month -1 hour";
  @ 29 days 23 hours
 (1 row)
 
+SELECT justify_interval(interval '2147483647 days 24 hrs');
+       justify_interval        
+-------------------------------
+ @ 5965232 years 4 mons 8 days
+(1 row)
+
+SELECT justify_interval(interval '-2147483648 days -24 hrs');
+         justify_interval          
+-----------------------------------
+ @ 5965232 years 4 mons 9 days ago
+(1 row)
+
+SELECT justify_interval(interval '2147483647 months 30 days');
+ERROR:  interval out of range
+SELECT justify_interval(interval '-2147483648 months -30 days');
+ERROR:  interval out of range
+SELECT justify_interval(interval '2147483647 months 30 days -24 hrs');
+         justify_interval         
+----------------------------------
+ @ 178956970 years 7 mons 29 days
+(1 row)
+
+SELECT justify_interval(interval '-2147483648 months -30 days 24 hrs');
+           justify_interval           
+--------------------------------------
+ @ 178956970 years 8 mons 29 days ago
+(1 row)
+
+SELECT justify_interval(interval '2147483647 months -30 days 1440 hrs');
+ERROR:  interval out of range
+SELECT justify_interval(interval '-2147483648 months 30 days -1440 hrs');
+ERROR:  interval out of range
 -- test fractional second input, and detection of duplicate units
 SET DATESTYLE = 'ISO';
 SET IntervalStyle TO postgres;
diff --git a/src/test/regress/sql/interval.sql b/src/test/regress/sql/interval.sql
index 6d532398bd..c31f0eec05 100644
--- a/src/test/regress/sql/interval.sql
+++ b/src/test/regress/sql/interval.sql
@@ -149,10 +149,22 @@ select '100000000y 10mon -1000000000d -100000h -10min -10.000001s ago'::interval
 SELECT justify_hours(interval '6 months 3 days 52 hours 3 minutes 2 seconds') as "6 mons 5 days 4 hours 3 mins 2 seconds";
 SELECT justify_days(interval '6 months 36 days 5 hours 4 minutes 3 seconds') as "7 mons 6 days 5 hours 4 mins 3 seconds";
 
+SELECT justify_hours(interval '2147483647 days 24 hrs');
+SELECT justify_days(interval '2147483647 months 30 days');
+
 -- test justify_interval()
 
 SELECT justify_interval(interval '1 month -1 hour') as "1 month -1 hour";
 
+SELECT justify_interval(interval '2147483647 days 24 hrs');
+SELECT justify_interval(interval '-2147483648 days -24 hrs');
+SELECT justify_interval(interval '2147483647 months 30 days');
+SELECT justify_interval(interval '-2147483648 months -30 days');
+SELECT justify_interval(interval '2147483647 months 30 days -24 hrs');
+SELECT justify_interval(interval '-2147483648 months -30 days 24 hrs');
+SELECT justify_interval(interval '2147483647 months -30 days 1440 hrs');
+SELECT justify_interval(interval '-2147483648 months 30 days -1440 hrs');
+
 -- test fractional second input, and detection of duplicate units
 SET DATESTYLE = 'ISO';
 SET IntervalStyle TO postgres;
-- 
2.25.1

Nathan Bossart

nathandbossart@gmail.com

almost 4 years ago

In reply to: Joseph Koshakow (#7)

Re: Fix overflow in justify_interval related functions

On Mon, Feb 14, 2022 at 08:35:43PM -0500, Joseph Koshakow wrote:

Good catch, I didn't think about that. Though if you are pre-justifying
the days, then I don't think it's possible for the second addition to
days to overflow. The maximum amount the days field could be after
the first justification is 29. I think we can simplify it further to the
attached patch.

LGTM. I've marked this one as ready-for-committer. It's a little weird
that justify_hours() and justify_days() can overflow in cases where there
is still a valid interval representation, but as Tom noted, those functions
have specific charters to follow.

+		ereport(ERROR,
+				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+				errmsg("interval out of range")));

nitpick: I think there is ordinarily an extra space before errmsg() so that
it lines up with errcode().

--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Joseph Koshakow

koshy44@gmail.com

almost 4 years ago

In reply to: Nathan Bossart (#8)

1 attachment(s)

Re: Fix overflow in justify_interval related functions

On Mon, Feb 14, 2022 at 11:23 PM Nathan Bossart
<nathandbossart@gmail.com> wrote:

It's a little weird
that justify_hours() and justify_days() can overflow in cases where there
is still a valid interval representation, but as Tom noted, those functions
have specific charters to follow.

Yes it is a bit weird, but this follows the same behavior as adding
Intervals. The following query overflows:
postgres=# SELECT interval '2147483647 days' + interval '1 day';
ERROR: interval out of range
Even though the following query does not:
postgres=# SELECT justify_days(interval '2147483647 days') + interval '1 day';
?column?
-----------------------------
5965232 years 4 mons 8 days
(1 row)

The reason is, as Tom mentioned, that Interval's months, days, and
time (microseconds) are stored separately. They are only combined
during certain scenarios such as testing for equality, ordering,
justify_* methods, etc. I think the idea behind it is that not every month
has 30 days and not every day has 24 hrs, though I'm not sure
.

+             ereport(ERROR,
+                             (errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+                             errmsg("interval out of range")));
nitpick: I think there is ordinarily an extra space before errmsg() so that
it lines up with errcode().

I've attached a patch to add the space. Thanks so much for your review
and comments!

- Joe Koshakow

Attachments:

v4-0001-Check-for-overflow-in-justify_interval-functions.patchtext/x-patch; charset=US-ASCII; name=v4-0001-Check-for-overflow-in-justify_interval-functions.patchDownload

From 62e3c824c9243fd53ad95b4d4fc3ef2542d8a941 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <koshy44@gmail.com>
Date: Sun, 13 Feb 2022 13:26:16 -0500
Subject: [PATCH] Check for overflow in justify_interval functions

justify_interval, justify_hours, and justify_days didn't check for
overflow when moving hours to days or days to hours. This commit adds
check for overflow and returns an appropriate error.

Signed-off-by: Joseph Koshakow <koshy44@gmail.com>
---
 src/backend/utils/adt/timestamp.c      | 29 ++++++++++++++++++---
 src/test/regress/expected/interval.out | 36 ++++++++++++++++++++++++++
 src/test/regress/sql/interval.sql      | 12 +++++++++
 3 files changed, 73 insertions(+), 4 deletions(-)

diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 36f8a84bcc..f45989f807 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -2717,12 +2717,27 @@ interval_justify_interval(PG_FUNCTION_ARGS)
 	result->day = span->day;
 	result->time = span->time;
 
+	/* pre-justify days if it might prevent overflow */
+	if ((result->day > 0 && result->time > 0) ||
+		(result->day < 0 && result->time < 0))
+	{
+		wholemonth = result->day / DAYS_PER_MONTH;
+		result->day -= wholemonth * DAYS_PER_MONTH;
+		if (pg_add_s32_overflow(result->month, wholemonth, &result->month))
+			ereport(ERROR,
+					(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+					 errmsg("interval out of range")));
+	}
+
 	TMODULO(result->time, wholeday, USECS_PER_DAY);
-	result->day += wholeday;	/* could overflow... */
+	result->day += wholeday;
 
 	wholemonth = result->day / DAYS_PER_MONTH;
 	result->day -= wholemonth * DAYS_PER_MONTH;
-	result->month += wholemonth;
+	if (pg_add_s32_overflow(result->month, wholemonth, &result->month))
+		ereport(ERROR,
+				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+				 errmsg("interval out of range")));
 
 	if (result->month > 0 &&
 		(result->day < 0 || (result->day == 0 && result->time < 0)))
@@ -2772,7 +2787,10 @@ interval_justify_hours(PG_FUNCTION_ARGS)
 	result->time = span->time;
 
 	TMODULO(result->time, wholeday, USECS_PER_DAY);
-	result->day += wholeday;	/* could overflow... */
+	if (pg_add_s32_overflow(result->day, wholeday, &result->day))
+		ereport(ERROR,
+				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+				errmsg("interval out of range")));
 
 	if (result->day > 0 && result->time < 0)
 	{
@@ -2808,7 +2826,10 @@ interval_justify_days(PG_FUNCTION_ARGS)
 
 	wholemonth = result->day / DAYS_PER_MONTH;
 	result->day -= wholemonth * DAYS_PER_MONTH;
-	result->month += wholemonth;
+	if (pg_add_s32_overflow(result->month, wholemonth, &result->month))
+		ereport(ERROR,
+				(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+				errmsg("interval out of range")));
 
 	if (result->month > 0 && result->day < 0)
 	{
diff --git a/src/test/regress/expected/interval.out b/src/test/regress/expected/interval.out
index accd4a7d90..146f7c55d0 100644
--- a/src/test/regress/expected/interval.out
+++ b/src/test/regress/expected/interval.out
@@ -396,6 +396,10 @@ SELECT justify_days(interval '6 months 36 days 5 hours 4 minutes 3 seconds') as
  @ 7 mons 6 days 5 hours 4 mins 3 secs
 (1 row)
 
+SELECT justify_hours(interval '2147483647 days 24 hrs');
+ERROR:  interval out of range
+SELECT justify_days(interval '2147483647 months 30 days');
+ERROR:  interval out of range
 -- test justify_interval()
 SELECT justify_interval(interval '1 month -1 hour') as "1 month -1 hour";
   1 month -1 hour   
@@ -403,6 +407,38 @@ SELECT justify_interval(interval '1 month -1 hour') as "1 month -1 hour";
  @ 29 days 23 hours
 (1 row)
 
+SELECT justify_interval(interval '2147483647 days 24 hrs');
+       justify_interval        
+-------------------------------
+ @ 5965232 years 4 mons 8 days
+(1 row)
+
+SELECT justify_interval(interval '-2147483648 days -24 hrs');
+         justify_interval          
+-----------------------------------
+ @ 5965232 years 4 mons 9 days ago
+(1 row)
+
+SELECT justify_interval(interval '2147483647 months 30 days');
+ERROR:  interval out of range
+SELECT justify_interval(interval '-2147483648 months -30 days');
+ERROR:  interval out of range
+SELECT justify_interval(interval '2147483647 months 30 days -24 hrs');
+         justify_interval         
+----------------------------------
+ @ 178956970 years 7 mons 29 days
+(1 row)
+
+SELECT justify_interval(interval '-2147483648 months -30 days 24 hrs');
+           justify_interval           
+--------------------------------------
+ @ 178956970 years 8 mons 29 days ago
+(1 row)
+
+SELECT justify_interval(interval '2147483647 months -30 days 1440 hrs');
+ERROR:  interval out of range
+SELECT justify_interval(interval '-2147483648 months 30 days -1440 hrs');
+ERROR:  interval out of range
 -- test fractional second input, and detection of duplicate units
 SET DATESTYLE = 'ISO';
 SET IntervalStyle TO postgres;
diff --git a/src/test/regress/sql/interval.sql b/src/test/regress/sql/interval.sql
index 6d532398bd..c31f0eec05 100644
--- a/src/test/regress/sql/interval.sql
+++ b/src/test/regress/sql/interval.sql
@@ -149,10 +149,22 @@ select '100000000y 10mon -1000000000d -100000h -10min -10.000001s ago'::interval
 SELECT justify_hours(interval '6 months 3 days 52 hours 3 minutes 2 seconds') as "6 mons 5 days 4 hours 3 mins 2 seconds";
 SELECT justify_days(interval '6 months 36 days 5 hours 4 minutes 3 seconds') as "7 mons 6 days 5 hours 4 mins 3 seconds";
 
+SELECT justify_hours(interval '2147483647 days 24 hrs');
+SELECT justify_days(interval '2147483647 months 30 days');
+
 -- test justify_interval()
 
 SELECT justify_interval(interval '1 month -1 hour') as "1 month -1 hour";
 
+SELECT justify_interval(interval '2147483647 days 24 hrs');
+SELECT justify_interval(interval '-2147483648 days -24 hrs');
+SELECT justify_interval(interval '2147483647 months 30 days');
+SELECT justify_interval(interval '-2147483648 months -30 days');
+SELECT justify_interval(interval '2147483647 months 30 days -24 hrs');
+SELECT justify_interval(interval '-2147483648 months -30 days 24 hrs');
+SELECT justify_interval(interval '2147483647 months -30 days 1440 hrs');
+SELECT justify_interval(interval '-2147483648 months 30 days -1440 hrs');
+
 -- test fractional second input, and detection of duplicate units
 SET DATESTYLE = 'ISO';
 SET IntervalStyle TO postgres;
-- 
2.25.1

#10

Joseph Koshakow

koshy44@gmail.com

almost 4 years ago

In reply to: Joseph Koshakow (#9)

Re: Fix overflow in justify_interval related functions

Just checking because I'm not very familiar with the process,
are there any outstanding items that I need to do for this patch?

- Joe Koshakow

#11

Nathan Bossart

nathandbossart@gmail.com

almost 4 years ago