Preserve memory context of VarStringSortSupport buffers.

When enlarging the work buffers of a VarStringSortSupport object,
varstrfastcmp_locale was careful to keep them in the ssup_cxt
memory context; but varstr_abbrev_convert just used palloc().
The latter creates a hazard that the buffers could be freed out
from under the VarStringSortSupport object, resulting in stomping
on whatever gets allocated in that memory later.

In practice, because we only use this code for ICU collations
(cf. 3df9c374e), the problem is confined to use of ICU collations.
I believe it may have been unreachable before the introduction
of incremental sort, too, as traditional sorting usually just
uses one context for the duration of the sort.

We could fix this by making the broken stanzas in varstr_abbrev_convert
match the non-broken ones in varstrfastcmp_locale.  However, it seems
like a better idea to dodge the issue altogether by replacing the
pfree-and-allocate-anew coding with repalloc, which automatically
preserves the chunk's memory context.  This fix does add a few cycles
because repalloc will copy the chunk's content, which the existing
coding assumes is useless.  However, we don't expect that these buffer
enlargement operations are performance-critical.  Besides that, it's
far from obvious that copying the buffer contents isn't required, since
these stanzas make no effort to mark the buffers invalid by resetting
last_returned, cache_blob, etc.  That seems to be safe upon examination,
but it's fragile and could easily get broken in future, which wouldn't
get revealed in testing with short-to-moderate-size strings.

Per bug #17584 from James Inform.  Whether or not the issue is
reachable in the older branches, this code has been broken on its
own terms from its introduction, so patch all the way back.

Discussion: https://postgr.es/m/17584-95c79b4a7d771f44@postgresql.org

1 files changed, 6 insertions, 10 deletions

diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index d9acec50617..7279405b186 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -77,8 +77,8 @@ typedef struct
 	char	   *buf1;			/* 1st string, or abbreviation original string
 								 * buf */
 	char	   *buf2;			/* 2nd string, or abbreviation strxfrm() buf */
-	int			buflen1;
-	int			buflen2;
+	int			buflen1;		/* Allocated length of buf1 */
+	int			buflen2;		/* Allocated length of buf2 */
 	int			last_len1;		/* Length of last buf1 string/strxfrm() input */
 	int			last_len2;		/* Length of last buf2 string/strxfrm() blob */
 	int			last_returned;	/* Last comparison result (cache) */
@@ -2297,15 +2297,13 @@ varstrfastcmp_locale(char *a1p, int len1, char *a2p, int len2, SortSupport ssup)
 
 	if (len1 >= sss->buflen1)
 	{
-		pfree(sss->buf1);
 		sss->buflen1 = Max(len1 + 1, Min(sss->buflen1 * 2, MaxAllocSize));
-		sss->buf1 = MemoryContextAlloc(ssup->ssup_cxt, sss->buflen1);
+		sss->buf1 = repalloc(sss->buf1, sss->buflen1);
 	}
 	if (len2 >= sss->buflen2)
 	{
-		pfree(sss->buf2);
 		sss->buflen2 = Max(len2 + 1, Min(sss->buflen2 * 2, MaxAllocSize));
-		sss->buf2 = MemoryContextAlloc(ssup->ssup_cxt, sss->buflen2);
+		sss->buf2 = repalloc(sss->buf2, sss->buflen2);
 	}
 
 	/*
@@ -2506,9 +2504,8 @@ varstr_abbrev_convert(Datum original, SortSupport ssup)
 		/* By convention, we use buffer 1 to store and NUL-terminate */
 		if (len >= sss->buflen1)
 		{
-			pfree(sss->buf1);
 			sss->buflen1 = Max(len + 1, Min(sss->buflen1 * 2, MaxAllocSize));
-			sss->buf1 = palloc(sss->buflen1);
+			sss->buf1 = repalloc(sss->buf1, sss->buflen1);
 		}
 
 		/* Might be able to reuse strxfrm() blob from last call */
@@ -2595,10 +2592,9 @@ varstr_abbrev_convert(Datum original, SortSupport ssup)
 			/*
 			 * Grow buffer and retry.
 			 */
-			pfree(sss->buf2);
 			sss->buflen2 = Max(bsize + 1,
 							   Min(sss->buflen2 * 2, MaxAllocSize));
-			sss->buf2 = palloc(sss->buflen2);
+			sss->buf2 = repalloc(sss->buf2, sss->buflen2);
 		}
 
 		/*