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>
Cc: Peter Geoghegan <pg@bowt.ie>
Subject: Re: BUG #17584: SQL crashes PostgreSQL when using ICU collation
Date: Sat, 13 Aug 2022 20:30:17 -0400
Message-ID: <541938.1660437017@sss.pgh.pa.us> (raw)
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>
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:
==00:00:00:27.370 497120== More than 10000000 total errors detected. I'm not reporting any more.
==00:00:00:27.370 497120== 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
Attachments:
[text/x-diff] fix-sort-support-misallocation-wip.patch (1.7K, ../541938.1660437017@sss.pgh.pa.us/2-fix-sort-support-misallocation-wip.patch)
download | inline diff:diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.cindex 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 *a2p, int len2, SortSupport ssup)
if (len1 >= sss->buflen1)
{
+ Assert(ssup->ssup_cxt == GetMemoryChunkContext(sss->buf1));
pfree(sss->buf1);
sss->buflen1 = Max(len1 + 1, Min(sss->buflen1 * 2, MaxAllocSize));
sss->buf1 = MemoryContextAlloc(ssup->ssup_cxt, sss->buflen1);
}
if (len2 >= sss->buflen2)
{
+ Assert(ssup->ssup_cxt == GetMemoryChunkContext(sss->buf2));
pfree(sss->buf2);
sss->buflen2 = Max(len2 + 1, Min(sss->buflen2 * 2, MaxAllocSize));
sss->buf2 = MemoryContextAlloc(ssup->ssup_cxt, sss->buflen2);
@@ -2518,9 +2520,10 @@ varstr_abbrev_convert(Datum original, SortSupport ssup)
/* By convention, we use buffer 1 to store and NUL-terminate */
if (len >= sss->buflen1)
{
+ Assert(ssup->ssup_cxt == GetMemoryChunkContext(sss->buf1));
pfree(sss->buf1);
sss->buflen1 = Max(len + 1, Min(sss->buflen1 * 2, MaxAllocSize));
- sss->buf1 = palloc(sss->buflen1);+ sss->buf1 = 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 == GetMemoryChunkContext(sss->buf2));
pfree(sss->buf2);
sss->buflen2 = Max(bsize + 1,
Min(sss->buflen2 * 2, MaxAllocSize));
- sss->buf2 = palloc(sss->buflen2);+ sss->buf2 = MemoryContextAlloc(ssup->ssup_cxt, sss->buflen2);
}
/*
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, james.inform@pharmapp.de, euler@eulerto.com, pg@bowt.ie
Subject: Re: BUG #17584: SQL crashes PostgreSQL when using ICU collation
In-Reply-To: <541938.1660437017@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox