Re: BUG #17584: SQL crashes PostgreSQL when using ICU collation - Mailing list pgsql-bugs

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

diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.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 *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);
         }

         /*