From: Tom Lane <tgl@sss.pgh.pa.us>
To: James Inform <james.inform@pharmapp.de>
cc: pgsql-bugs@lists.postgresql.org, Euler Taveira <euler@eulerto.com>,
        Peter Geoghegan <pg@bowt.ie>
Subject: Re: BUG #17584: SQL crashes PostgreSQL when using ICU collation
In-reply-to: <92087cd5-5410-a4e9-41fe-4ff310439190@pharmapp.de>
References: <17584-95c79b4a7d771f44@postgresql.org>
 <f6d74de8-df28-4ef7-8d9e-10f2ab77ea8e@www.fastmail.com>
 <a9851f48-b281-8313-52f4-59ece14adb86@pharmapp.de>
 <465044.1660428315@sss.pgh.pa.us>
 <92087cd5-5410-a4e9-41fe-4ff310439190@pharmapp.de>
Comments: In-reply-to James Inform <james.inform@pharmapp.de>
	message dated "Sun, 14 Aug 2022 00:20:42 +0200"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <541929.1660437012.0@sss.pgh.pa.us>
Date: Sat, 13 Aug 2022 20:30:17 -0400
Message-ID: <541938.1660437017@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/541938.1660437017%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <541929.1660437012.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable

James Inform <james.inform@pharmapp.de> writes:
> Here is the attachment.

Thanks for the test case!  After tracing through it, I find that
the bug is not particularly related to ICU.  It's in
varstr_abbrev_convert(): that function will sometimes reallocate
buffers in CurrentMemoryContext, which could be a shorter-lived
context than the one the SortSupport object belongs to.  If so,
we'll eventually be scribbling on memory that doesn't belong
to us, and the observed problems are gripes from the scribble-ees.

I found this by valgrind'ing the test case, which eventually
printed this:

=3D=3D00:00:00:27.370 497120=3D=3D More than 10000000 total errors detecte=
d.  I'm not reporting any more.
=3D=3D00:00:00:27.370 497120=3D=3D Final error counts will be inaccurate. =
 Go fix your program!

and awhile later suffered an OOM kill.  I got a good laugh
out of that --- never saw that valgrind message before.

Attached is a quick draft fix.  Some notes:

* I'm not really proposing the added Asserts for commit,
though they helped provide confidence that I was on the
right track.

* We need to look around and see if the same mistake appears
in any other sortsupport code.

* The bug could never have existed at all if these buffer
resizings had been done with repalloc().  I get that the
idea is to avoid copying data we don't care about, but
this coding is still feeling like an antipattern.  I wonder
if it'd be worth inventing a variant of repalloc that makes
the chunk bigger without preserving its contents.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="fix-sort-support-misallocation-wip.patch";
	charset="us-ascii"
Content-ID: <541929.1660437012.2@sss.pgh.pa.us>
Content-Description: fix-sort-support-misallocation-wip.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varle=
na.c
index 919138eaf3..90688376a6 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -2330,12 +2330,14 @@ varstrfastcmp_locale(char *a1p, int len1, char *a2=
p, int len2, SortSupport ssup)
 =

 	if (len1 >=3D sss->buflen1)
 	{
+		Assert(ssup->ssup_cxt =3D=3D GetMemoryChunkContext(sss->buf1));
 		pfree(sss->buf1);
 		sss->buflen1 =3D Max(len1 + 1, Min(sss->buflen1 * 2, MaxAllocSize));
 		sss->buf1 =3D MemoryContextAlloc(ssup->ssup_cxt, sss->buflen1);
 	}
 	if (len2 >=3D sss->buflen2)
 	{
+		Assert(ssup->ssup_cxt =3D=3D GetMemoryChunkContext(sss->buf2));
 		pfree(sss->buf2);
 		sss->buflen2 =3D Max(len2 + 1, Min(sss->buflen2 * 2, MaxAllocSize));
 		sss->buf2 =3D MemoryContextAlloc(ssup->ssup_cxt, sss->buflen2);
@@ -2518,9 +2520,10 @@ varstr_abbrev_convert(Datum original, SortSupport s=
sup)
 		/* By convention, we use buffer 1 to store and NUL-terminate */
 		if (len >=3D sss->buflen1)
 		{
+			Assert(ssup->ssup_cxt =3D=3D GetMemoryChunkContext(sss->buf1));
 			pfree(sss->buf1);
 			sss->buflen1 =3D Max(len + 1, Min(sss->buflen1 * 2, MaxAllocSize));
-			sss->buf1 =3D palloc(sss->buflen1);
+			sss->buf1 =3D MemoryContextAlloc(ssup->ssup_cxt, sss->buflen1);
 		}
 =

 		/* Might be able to reuse strxfrm() blob from last call */
@@ -2607,10 +2610,11 @@ varstr_abbrev_convert(Datum original, SortSupport =
ssup)
 			/*
 			 * Grow buffer and retry.
 			 */
+			Assert(ssup->ssup_cxt =3D=3D GetMemoryChunkContext(sss->buf2));
 			pfree(sss->buf2);
 			sss->buflen2 =3D Max(bsize + 1,
 							   Min(sss->buflen2 * 2, MaxAllocSize));
-			sss->buf2 =3D palloc(sss->buflen2);
+			sss->buf2 =3D MemoryContextAlloc(ssup->ssup_cxt, sss->buflen2);
 		}
 =

 		/*

------- =_aaaaaaaaaa0--