Date: Mon, 22 Jul 2024 17:27:13 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Joseph Koshakow <koshy44@gmail.com>
Cc: jian he <jian.universality@gmail.com>,
	Alexander Lakhin <exclusion@gmail.com>,
	Tom Lane <tgl@sss.pgh.pa.us>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
	Andres Freund <andres@anarazel.de>
Subject: Re: Remove dependence on integer wrapping
Message-ID: <Zp7cwd_6NXlHt6Ye@nathan>
References: <Zpa0m_OVw_iWYicV@nathan>
 <CAAvxfHeFDyNZj+rjcuMkh+VTSYmhb_ZPpbdqZUJcjnT=X3ampA@mail.gmail.com>
 <CACJufxFkTACY6MprpewMncdOpL9dpZBcAGtc6WLfnScLknS3DA@mail.gmail.com>
 <CAAvxfHfeUmVMXU6883in2H-jJ+Vn2=mcSgVpsNDRTTK=OYDjoA@mail.gmail.com>
 <ZpqesU46GIKUvtmS@nathan>
 <Zpq0XUQ9neudWYgC@nathan>
 <CAAvxfHfdPgtqFqsb=Nxa1oc6GsQ_U+k3rfneo-EjYG4bne_ksw@mail.gmail.com>
 <Zp54I4WMbWW6AQnP@nathan>
 <CAAvxfHdWseDnK0gLYY51fdw-_pE=5Q2=K=pToVv5jo3YNVfHoQ@mail.gmail.com>
 <Zp7Q4erYWVjtWzQr@nathan>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="3HSg2+A/6oTs+Iqd"
Content-Disposition: inline
In-Reply-To: <Zp7Q4erYWVjtWzQr@nathan>
Archived-At: <https://www.postgresql.org/message-id/Zp7cwd_6NXlHt6Ye%40nathan>
Precedence: bulk


--3HSg2+A/6oTs+Iqd
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Mon, Jul 22, 2024 at 04:36:33PM -0500, Nathan Bossart wrote:
> Okay.  I'll plan on committing v13-0002 in the next couple of days, then.

Actually, I think my concerns about prohibiting more than necessary go away
if we do the subtraction first.  If "upperIndx[i] - lowerIndx[i]"
overflows, we know the array size is too big.  Similarly, if adding one to
that result overflows, we again know the the array size is too big.  This
appears to be how the surrounding code handles this problem (e.g.,
ReadArrayDimensions()).  Thoughts?

-- 
nathan

--3HSg2+A/6oTs+Iqd
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename="v14-0001-Remove-overflow-from-array_set_slice.patch"

From 8794c740492bd5113d405638b03518693dfeb0db Mon Sep 17 00:00:00 2001
From: Joseph Koshakow <koshy44@gmail.com>
Date: Sat, 6 Jul 2024 14:35:00 -0400
Subject: [PATCH v14 1/1] Remove overflow from array_set_slice

This commit removes an overflow from array_set_slice that allows seting
absurd slice ranges.
---
 src/backend/utils/adt/arrayfuncs.c   | 9 ++++++++-
 src/test/regress/expected/arrays.out | 6 ++++++
 src/test/regress/sql/arrays.sql      | 5 +++++
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c
index d6641b570d..e5c7e57a5d 100644
--- a/src/backend/utils/adt/arrayfuncs.c
+++ b/src/backend/utils/adt/arrayfuncs.c
@@ -2887,7 +2887,14 @@ array_set_slice(Datum arraydatum,
 						 errdetail("When assigning to a slice of an empty array value,"
 								   " slice boundaries must be fully specified.")));
 
-			dim[i] = 1 + upperIndx[i] - lowerIndx[i];
+			/* compute "upperIndx[i] - lowerIndx[i] + 1", detecting overflow */
+			if (pg_sub_s32_overflow(upperIndx[i], lowerIndx[i], &dim[i]) ||
+				pg_add_s32_overflow(dim[i], 1, &dim[i]))
+				ereport(ERROR,
+						(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+						 errmsg("array size exceeds the maximum allowed (%d)",
+								(int) MaxArraySize)));
+
 			lb[i] = lowerIndx[i];
 		}
 
diff --git a/src/test/regress/expected/arrays.out b/src/test/regress/expected/arrays.out
index 23404982f7..7301c96bba 100644
--- a/src/test/regress/expected/arrays.out
+++ b/src/test/regress/expected/arrays.out
@@ -2699,3 +2699,9 @@ SELECT array_sample('{1,2,3,4,5,6}'::int[], -1); -- fail
 ERROR:  sample size must be between 0 and 6
 SELECT array_sample('{1,2,3,4,5,6}'::int[], 7); --fail
 ERROR:  sample size must be between 0 and 6
+-- overflow in array slicing
+CREATE TEMP TABLE arroverflowtest (i int[]);
+INSERT INTO arroverflowtest(i[0:2147483647]) VALUES ('{1}');
+ERROR:  array size exceeds the maximum allowed (134217727)
+INSERT INTO arroverflowtest(i[-2147483648:1]) VALUES ('{1}');
+ERROR:  array size exceeds the maximum allowed (134217727)
diff --git a/src/test/regress/sql/arrays.sql b/src/test/regress/sql/arrays.sql
index 50aa539fdc..d6ef059a1d 100644
--- a/src/test/regress/sql/arrays.sql
+++ b/src/test/regress/sql/arrays.sql
@@ -825,3 +825,8 @@ SELECT array_dims(array_sample('[-1:2][2:3]={{1,2},{3,NULL},{5,6},{7,8}}'::int[]
 SELECT array_dims(array_sample('{{{1,2},{3,NULL}},{{5,6},{7,8}},{{9,10},{11,12}}}'::int[], 2));
 SELECT array_sample('{1,2,3,4,5,6}'::int[], -1); -- fail
 SELECT array_sample('{1,2,3,4,5,6}'::int[], 7); --fail
+
+-- overflow in array slicing
+CREATE TEMP TABLE arroverflowtest (i int[]);
+INSERT INTO arroverflowtest(i[0:2147483647]) VALUES ('{1}');
+INSERT INTO arroverflowtest(i[-2147483648:1]) VALUES ('{1}');
-- 
2.39.3 (Apple Git-146)


--3HSg2+A/6oTs+Iqd--