Fix overflow hazard in interval rounding

Started by Joseph Koshakowalmost 2 years ago6 messages
#1Joseph Koshakow
koshy44@gmail.com
1 attachment(s)

Hi all,

Attached is a patch that fixes some overflow/underflow hazards that I
discovered in the interval rounding code.

The lines look a bit long, but I did run the following before committing:
`$ curl https://buildfarm.postgresql.org/cgi-bin/typedefs.pl -o
src/tools/pgindent/typedefs.list && src/tools/pgindent/pgindent
src/backend/utils/adt/timestamp.c`

Thanks,
Joe Koshakow

Attachments:

v1-0001-Fix-overflow-hazard-in-interval-rounding.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Fix-overflow-hazard-in-interval-rounding.patchDownload
From 389b0d1e3f3cca6fca1e45fdd202b1ca066326c2 Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <koshy44@gmail.com>
Date: Tue, 13 Feb 2024 13:06:13 -0500
Subject: [PATCH] Fix overflow hazard in interval rounding

This commit fixes overflow/underflow hazards present in the interval
rounding code used to parse intervals.
---
 src/backend/utils/adt/timestamp.c      | 18 ++++++++++--------
 src/test/regress/expected/interval.out |  9 +++++++++
 src/test/regress/sql/interval.sql      |  5 +++++
 3 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index c38f88dba7..a3b65a755f 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -1509,17 +1509,19 @@ AdjustIntervalForTypmod(Interval *interval, int32 typmod,
 
 			if (interval->time >= INT64CONST(0))
 			{
-				interval->time = ((interval->time +
-								   IntervalOffsets[precision]) /
-								  IntervalScales[precision]) *
-					IntervalScales[precision];
+				if (pg_add_s64_overflow(interval->time, IntervalOffsets[precision], &interval->time))
+					ereport(ERROR,
+							errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+							errmsg("interval out of range"));
+				interval->time = (interval->time / IntervalScales[precision]) * IntervalScales[precision];
 			}
 			else
 			{
-				interval->time = -(((-interval->time +
-									 IntervalOffsets[precision]) /
-									IntervalScales[precision]) *
-								   IntervalScales[precision]);
+				if (pg_sub_s64_overflow(IntervalOffsets[precision], interval->time, &interval->time))
+					ereport(ERROR,
+							errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+							errmsg("interval out of range"));
+				interval->time = -((interval->time / IntervalScales[precision]) * IntervalScales[precision]);
 			}
 		}
 	}
diff --git a/src/test/regress/expected/interval.out b/src/test/regress/expected/interval.out
index b79b6fcd4d..055930ccac 100644
--- a/src/test/regress/expected/interval.out
+++ b/src/test/regress/expected/interval.out
@@ -929,6 +929,15 @@ SELECT interval '1 2:03:04.5678' minute to second(2);
  1 day 02:03:04.57
 (1 row)
 
+-- these should fail as out-of-range
+SELECT interval '2562047788:00:54.775807' SECOND(2);
+ERROR:  interval out of range
+LINE 1: SELECT interval '2562047788:00:54.775807' SECOND(2);
+                        ^
+SELECT interval '-2562047788:00:54.775807' SECOND(2);
+ERROR:  interval out of range
+LINE 1: SELECT interval '-2562047788:00:54.775807' SECOND(2);
+                        ^
 -- test casting to restricted precision (bug #14479)
 SELECT f1, f1::INTERVAL DAY TO MINUTE AS "minutes",
   (f1 + INTERVAL '1 month')::INTERVAL MONTH::INTERVAL YEAR AS "years"
diff --git a/src/test/regress/sql/interval.sql b/src/test/regress/sql/interval.sql
index 5566ad0e51..838da2cc13 100644
--- a/src/test/regress/sql/interval.sql
+++ b/src/test/regress/sql/interval.sql
@@ -270,6 +270,11 @@ SELECT interval '1 2:03:04.5678' hour to second(2);
 SELECT interval '1 2.3456' minute to second(2);
 SELECT interval '1 2:03.5678' minute to second(2);
 SELECT interval '1 2:03:04.5678' minute to second(2);
+-- these should fail as out-of-range
+SELECT interval '2562047788:00:54.775807' SECOND(2);
+SELECT interval '-2562047788:00:54.775807' SECOND(2);
+
+
 
 -- test casting to restricted precision (bug #14479)
 SELECT f1, f1::INTERVAL DAY TO MINUTE AS "minutes",
-- 
2.34.1

#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Joseph Koshakow (#1)
Re: Fix overflow hazard in interval rounding

Joseph Koshakow <koshy44@gmail.com> writes:

Attached is a patch that fixes some overflow/underflow hazards that I
discovered in the interval rounding code.

I think you need to use ereturn not ereport here; see other error
cases in AdjustIntervalForTypmod.

(We'd need ereport in back branches, but this problem seems to
me to probably not be worth back-patching.)

regards, tom lane

#3Andres Freund
andres@anarazel.de
In reply to: Joseph Koshakow (#1)
Re: Fix overflow hazard in interval rounding

Hi,

On 2024-02-13 13:31:22 -0500, Joseph Koshakow wrote:

Attached is a patch that fixes some overflow/underflow hazards that I
discovered in the interval rounding code.

Random, mildly related thought: I wonder if it's time to, again, look at
enabling -ftrapv in assert enabled builds. I had looked at that a few years
back, and fixed a number of instances, but not all I think. But I think we are
a lot closer to avoiding signed overflows everywhere, and it'd be nice to find
overflow hazards more easily. Many places are broken even with -fwrapv
semantics (which we don't have on all compilers!). Trapping on such overflows
makes it far easier to find problems with tools like sqlsmith.

Greetings,

Andres Freund

#4Joseph Koshakow
koshy44@gmail.com
In reply to: Tom Lane (#2)
1 attachment(s)
Re: Fix overflow hazard in interval rounding

On Tue, Feb 13, 2024 at 1:46 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think you need to use ereturn not ereport here; see other error
cases in AdjustIntervalForTypmod.

Attached is an updated patch that makes this adjustment.

(We'd need ereport in back branches, but this problem seems to
me to probably not be worth back-patching.)

Agreed, this seems like a pretty rare overflow/underflow.

Thanks,
Joe Koshakow

Attachments:

v2-0001-Fix-overflow-hazard-in-interval-rounding.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Fix-overflow-hazard-in-interval-rounding.patchDownload
From 470aa9c8898b4e4ebbad97d6e421377b9a3e03cf Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <koshy44@gmail.com>
Date: Tue, 13 Feb 2024 13:06:13 -0500
Subject: [PATCH] Fix overflow hazard in interval rounding

This commit fixes overflow/underflow hazards present in the interval
rounding code used to parse intervals.
---
 src/backend/utils/adt/timestamp.c      | 18 ++++++++++--------
 src/test/regress/expected/interval.out |  9 +++++++++
 src/test/regress/sql/interval.sql      |  4 ++++
 3 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index c38f88dba7..97566d7e3b 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -1509,17 +1509,19 @@ AdjustIntervalForTypmod(Interval *interval, int32 typmod,
 
 			if (interval->time >= INT64CONST(0))
 			{
-				interval->time = ((interval->time +
-								   IntervalOffsets[precision]) /
-								  IntervalScales[precision]) *
-					IntervalScales[precision];
+				if (pg_add_s64_overflow(interval->time, IntervalOffsets[precision], &interval->time))
+					ereturn(escontext, false,
+							(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+							 errmsg("interval out of range")));
+				interval->time = (interval->time / IntervalScales[precision]) * IntervalScales[precision];
 			}
 			else
 			{
-				interval->time = -(((-interval->time +
-									 IntervalOffsets[precision]) /
-									IntervalScales[precision]) *
-								   IntervalScales[precision]);
+				if (pg_sub_s64_overflow(IntervalOffsets[precision], interval->time, &interval->time))
+					ereturn(escontext, false,
+							(errcode(ERRCODE_DATETIME_VALUE_OUT_OF_RANGE),
+							 errmsg("interval out of range")));
+				interval->time = -((interval->time / IntervalScales[precision]) * IntervalScales[precision]);
 			}
 		}
 	}
diff --git a/src/test/regress/expected/interval.out b/src/test/regress/expected/interval.out
index b79b6fcd4d..055930ccac 100644
--- a/src/test/regress/expected/interval.out
+++ b/src/test/regress/expected/interval.out
@@ -929,6 +929,15 @@ SELECT interval '1 2:03:04.5678' minute to second(2);
  1 day 02:03:04.57
 (1 row)
 
+-- these should fail as out-of-range
+SELECT interval '2562047788:00:54.775807' SECOND(2);
+ERROR:  interval out of range
+LINE 1: SELECT interval '2562047788:00:54.775807' SECOND(2);
+                        ^
+SELECT interval '-2562047788:00:54.775807' SECOND(2);
+ERROR:  interval out of range
+LINE 1: SELECT interval '-2562047788:00:54.775807' SECOND(2);
+                        ^
 -- test casting to restricted precision (bug #14479)
 SELECT f1, f1::INTERVAL DAY TO MINUTE AS "minutes",
   (f1 + INTERVAL '1 month')::INTERVAL MONTH::INTERVAL YEAR AS "years"
diff --git a/src/test/regress/sql/interval.sql b/src/test/regress/sql/interval.sql
index 5566ad0e51..d945a13714 100644
--- a/src/test/regress/sql/interval.sql
+++ b/src/test/regress/sql/interval.sql
@@ -270,6 +270,10 @@ SELECT interval '1 2:03:04.5678' hour to second(2);
 SELECT interval '1 2.3456' minute to second(2);
 SELECT interval '1 2:03.5678' minute to second(2);
 SELECT interval '1 2:03:04.5678' minute to second(2);
+-- these should fail as out-of-range
+SELECT interval '2562047788:00:54.775807' SECOND(2);
+SELECT interval '-2562047788:00:54.775807' SECOND(2);
+
 
 -- test casting to restricted precision (bug #14479)
 SELECT f1, f1::INTERVAL DAY TO MINUTE AS "minutes",
-- 
2.34.1

#5Tom Lane
tgl@sss.pgh.pa.us
In reply to: Joseph Koshakow (#4)
Re: Fix overflow hazard in interval rounding

Joseph Koshakow <koshy44@gmail.com> writes:

On Tue, Feb 13, 2024 at 1:46 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

(We'd need ereport in back branches, but this problem seems to
me to probably not be worth back-patching.)

Agreed, this seems like a pretty rare overflow/underflow.

OK, pushed to HEAD only. I converted the second steps to be like
"a -= a%b" instead of "a = (a/b)*b" to make it a little clearer
that they don't have their own risks of overflow. Maybe it's a
shade faster that way too, not sure.

regards, tom lane

#6Joseph Koshakow
koshy44@gmail.com
In reply to: Andres Freund (#3)
Re: Fix overflow hazard in interval rounding

Hi Andres,

Sorry for such a late reply.

On Tue, Feb 13, 2024 at 2:14 PM Andres Freund <andres@anarazel.de> wrote:

Random, mildly related thought: I wonder if it's time to, again, look at
enabling -ftrapv in assert enabled builds.I had looked at that a few years
back, and fixed a number of instances, but not all I think. But I think

we are

a lot closer to avoiding signed overflows everywhere, and it'd be nice to

find

overflow hazards more easily.

I agree that this would be very helpful.

Many places are broken even with -fwrapv
semantics (which we don't have on all compilers!). Trapping on such

overflows

makes it far easier to find problems with tools like sqlsmith.

Does this mean that some of our existing tests will panic when compiled
with -ftrapv or -fwrapv? If so I'd be interested in resolving the
remaining issues if you could point me in the right direction of how to
set the flag.

Thanks,
Joe Koshakow