Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 14 Jun 2023 13:21:40 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Alexander Lakhin <exclusion@gmail.com>
Cc: Richard Guo <guofenglinux@gmail.com>,
	Ankit Kumar Pandey <itsankitkp@gmail.com>,
	David Rowley <dgrowleyml@gmail.com>,
	Teodor Sigaev <teodor@sigaev.ru>, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17888: Incorrect memory access in gist__int_ops for an
 input array with many elements
Message-ID: <ZIlAVBmmAhVaVGHR@paquier.xyz>
References: <17888-f72930e6b5ce8c14@postgresql.org>
 <CAMbWs48s6Uhv9wngJVtv=7DGEyN9a-SzwBKsfz2hY7yS6z7ZQw@mail.gmail.com>
 <f7ab52bb-e0ee-4acd-9bb1-ac28d22e39f5@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="3HBXALYs/JrijTK8"
Content-Disposition: inline
In-Reply-To: <f7ab52bb-e0ee-4acd-9bb1-ac28d22e39f5@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/ZIlAVBmmAhVaVGHR%40paquier.xyz>
Precedence: bulk


--3HBXALYs/JrijTK8
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Wed, May 31, 2023 at 09:00:00AM +0300, Alexander Lakhin wrote:
> The NOTICE appeared there in commit 08ee64ebf5 (from year 2005) in an
> attempt to get rid of flags (ISLEAFKEY(in)) [1] and probably expressed
> an intention to play gently (don't break compatibility?).

Good question.  This commit or its related thread don't tell much.
FWIW, these are two things that don't make much sense to me:
1) to allow the compression to work with that many elements with its
code unable to handle that, while storing incorrect data.
2) the decompression fails entirely to cope with that.

> The patch proposed at [2] looks correct to me. Thank you, Ankit!
> Though I would add a simple case to the intarray regression test and also
> add errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED) to other two places in
> _int_gist.c for the sake of consistency.

The error is different in ~9.6 and 10~, where we would not get a crash
before that.  But We do get a lot of write past chunk end WARNINGs
instead.

Saying that, being able to corrupt the server at will with a gist
operator that can be installed is no good either, so your suggestion
of made of the two patches is OK by me.  Based on my read of the code,
this prevents the compression to store data it would not be able to
handle on decompression.  Now, I have to admit that I am not the most
verse with this area.

Likely that's something we'd better backpatch, to avoid people playing
with that too much in the back-branches.  Thoughts or objections?
--
Michael

--3HBXALYs/JrijTK8
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmSJQFQACgkQnvQgOdby
QH18Rg/9E4ePwg3lnrflswbid0vMxwi2dVYUFDqh1AKwYzImI/M4zTeS8b1X8P1I
/+IdXkFdTsaub/WdKfsPX4zo2w9vA/y4KJdnPWcm3R6GwjCYJay6Du4Mr2Nj+ti+
0yQ7gKk1aYEOMI1PZFMRoJQu4oe/XF0EgnqvMLAfO9IP0ios3eOX/dW/YjeCCtOP
rW455ldMgr+lyB9ZYx/hsbgOL5SmM25YJhSeFKi103JDlRGJh6bN+CB8XN4aPot+
WNh6aNGd5c/LDK/z/wj0fk5cDv/MSeuGB3obOW4lygOfSGprGukXxCxYSVAVaXCg
XDo4evM//NQA7UxX0Ovzj4qr+/5Ayrw/pR41k2XWX3TPjDUazN1CQYp6632cqNtJ
gnatoGIMha8qlhz1y8K8Iv/GfD7cxh001RuEFo7CkM1YG3CqQ8Co1mmdz3XEnVh2
pUs/BEC5yyRP6kLu4jUQbf0GnlsibNOwIuRSuvXrw+D0GyA32VW4MEioyrG+BULR
7AyStVvrF8oga3m1NafjyUGQNfr85vEubNZxi2+y0z+9MFx8j7Fd2eOrf1nsI2TY
tIfQpwyxHGsE8ZQvxEiFuom841ZFUckcnnIvioPC1w5K7HBkuySnTJeHzXlwQsN0
1o7bMv4fLXbfVXFgHulXhT/QJ/K7UZtA32nixJ78VDt4DbZMdjM=
=RVzl
-----END PGP SIGNATURE-----

--3HBXALYs/JrijTK8--