Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 28 Sep 2023 12:30:39 +0900
From: Michael Paquier <michael@paquier.xyz>
To: exclusion@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18135: Incorrect memory access occurs when attaching a
 partition with an index
Message-ID: <ZRTzX0h68fxZ5Dth@paquier.xyz>
References: <18135-532f4a755e71e4d2@postgresql.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="i0iUL1/uoNIfJPCV"
Content-Disposition: inline
In-Reply-To: <18135-532f4a755e71e4d2@postgresql.org>
Archived-At: 
 <https://www.postgresql.org/message-id/ZRTzX0h68fxZ5Dth%40paquier.xyz>
Precedence: bulk


--i0iUL1/uoNIfJPCV
Content-Type: multipart/mixed; boundary="W1+rAGyY6d5J2O3K"
Content-Disposition: inline


--W1+rAGyY6d5J2O3K
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Sep 27, 2023 at 10:00:01AM +0000, PG Bug reporting form wrote:
> executed under Valgrind, it leads to an incorrect memory access:
> =3D=3D00:00:00:03.947 396156=3D=3D Invalid read of size 2
> =3D=3D00:00:00:03.947 396156=3D=3D    at 0x2E323D: CompareIndexInfo (inde=
x.c:2572)
> =3D=3D00:00:00:03.947 396156=3D=3D    by 0x3D009B: AttachPartitionEnsureI=
ndexes
> (tablecmds.c:18797)
> =3D=3D00:00:00:03.947 396156=3D=3D    by 0x3D8B4F: ATExecAttachPartition
> (tablecmds.c:18578)
> =3D=3D00:00:00:03.947 396156=3D=3D    by 0x3D9A88: ATExecCmd (tablecmds.c=
:5379)
> =3D=3D00:00:00:03.947 396156=3D=3D    by 0x3D9BC7: ATRewriteCatalogs
> (tablecmds.c:5063)

I have just tested that on HEAD and REL_16_STABLE, but fail to see
this report, which is weird (3.19.0 here).  Are you using any specific
option of valgrind I should be aware of?  Here is what I used, for
reference:
valgrind \
  --suppressions=3D$PG_SOURCE/src/tools/valgrind.supp \
  --trace-children=3Dyes --track-origins=3Dyes --read-var-info=3Dyes \
  postgres -D REST_OF_ARGS

> The function CompareIndexInfo() contains the code:
> 		/* ignore expressions at this stage */
> 		if ((info1->ii_IndexAttrNumbers[i] !=3D InvalidAttrNumber) &&
> 			(attmap->attnums[info2->ii_IndexAttrNumbers[i] - 1] !=3D
> 			 info1->ii_IndexAttrNumbers[i]))
> 			return false;
>=20
> where info1->ii_IndexAttrNumbers[i] is checked for InvalidAttrNumber
> (i. e. it's not an expression), but info2->ii_IndexAttrNumbers[i] is not.

Anyway, I can see your point here.  info2's first attnum is 0 so we
look at an imaginary position in attmap->attnums.  So, yes, that's
wrong.

> In addition, there is a check whether both indexes are (are not)
> expression indexes, but it's placed below...

Sure, but this makes the check a bit cheaper if the indexes to compare
use expr and non-expr attributes at the same attnums, no?  Except if I
am missing something, the attached should be sufficient.
--
Michael

--W1+rAGyY6d5J2O3K
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="compare-index-mem.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 716de171a4..bbc9b262a0 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -2570,6 +2570,7 @@ CompareIndexInfo(const IndexInfo *info1, const IndexI=
nfo *info2,
=20
 		/* ignore expressions at this stage */
 		if ((info1->ii_IndexAttrNumbers[i] !=3D InvalidAttrNumber) &&
+			(info2->ii_IndexAttrNumbers[i] !=3D InvalidAttrNumber) &&
 			(attmap->attnums[info2->ii_IndexAttrNumbers[i] - 1] !=3D
 			 info1->ii_IndexAttrNumbers[i]))
 			return false;

--W1+rAGyY6d5J2O3K--

--i0iUL1/uoNIfJPCV
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmUU818ACgkQnvQgOdby
QH2gNg/9G6EX8emZsUg8GLrVJNmJVaHxEffchjH6nJVkUvtRMyZ3r2o9JPQgTKaH
ciSBXsER+COBNE5v4SCuacPuTJCWzaiBgVeg063L3GzeddCUgEl2adOGYBc/Y62P
kaUFUX2Xyh4QBvy5iAkwlU2fA2HOTKqyBCeG+/ekfmHT+Mf0x5nh3IvHESTFkbuf
MZd4KB7wo9T62Onqfd/QPCmZD9Tb4jFbzs+XQJQzv89BSkjicBxotwSWID/GP10g
czSHYnCIco8F8RzkpATWV/pX5WtpfpWxm4eUTNVUSpDMyVyokjifYku3F426HQco
giEDAXyjDXTXhTHBznQrQm/9yQmdBuzf+l6HEopCr35Cg0G5Kasopbp/odE2vG1F
n7t+sAvYCk+w/3fo+Un2C303xvHvC3IUsIR6wF8sq81owB0CQqn68eLOrYvxeHEb
ultVsiNJFG6nekyLcQc/+KK5hJX4krsspYvW/W6eynX/IUB7bcyDOQiRPI7uy7ci
sphfNsv3zJj8tHUqQMpds4h8gZDSGMHyNYHZFWSmbxujvtPIkchSl9y7kpBqtB4a
mLVRJgZRnM/HQWZD4/Upn915C6/mwZtYHzQadWoko6Gvy/Nk5pnqJZzLv0tBcXbs
ScFUWXDsrtDPXOgzGhoGt6e/tDZ5ooXq4m2bs1+D18pHai9E29E=
=7PiK
-----END PGP SIGNATURE-----

--i0iUL1/uoNIfJPCV--