From: Heikki Linnakangas <hlinnaka@iki.fi>
Subject: Crash in BRIN summarization
To: pgsql-hackers <pgsql-hackers@postgresql.org>
Message-ID: <e6e1d6eb-0a67-36aa-e779-bcca59167c14@iki.fi>
Date: Wed, 28 Aug 2019 12:22:17 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
Content-Type: multipart/mixed;
 boundary="------------B61B52F709A7BF999237404A"
Content-Language: en-US
Precedence: bulk

This is a multi-part message in MIME format.
--------------B61B52F709A7BF999237404A
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit

I bumped into a little bug in BRIN, while hacking on something 
unrelated. This causes a segfault, or an assertion failure if assertions 
are enabled:

CREATE TABLE brintest (n numrange);
CREATE INDEX brinidx ON brintest USING brin (n);

INSERT INTO brintest VALUES ('empty');
INSERT INTO brintest VALUES (numrange(0, 2^1000::numeric));
INSERT INTO brintest VALUES ('(-1, 0)');

SELECT brin_desummarize_range('brinidx',0);
SELECT brin_summarize_range('brinidx',0) ;

gdb backtrace:

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x0000560fef71f4b2 in GetMemoryChunkContext (pointer=0x7fed65a8fc30) 
at ../../../../src/include/utils/memutils.h:129
129		AssertArg(MemoryContextIsValid(context));
(gdb) bt
#0  0x0000560fef71f4b2 in GetMemoryChunkContext (pointer=0x7fed65a8fc30) 
at ../../../../src/include/utils/memutils.h:129
#1  0x0000560fef721290 in pfree (pointer=0x7fed65a8fc30) at mcxt.c:1033
#2  0x0000560fef0bd13f in brin_inclusion_add_value 
(fcinfo=0x7ffc4cd36220) at brin_inclusion.c:239
#3  0x0000560fef6ee543 in FunctionCall4Coll (flinfo=0x560ff05d0cf0, 
collation=0, arg1=94626457109880, arg2=94626457868896, 
arg3=140657589550088, arg4=0) at fmgr.c:1214
#4  0x0000560fef0b39b7 in brinbuildCallback (index=0x7fed64f37460, 
htup=0x560ff0685d68, values=0x7ffc4cd365d0, isnull=0x7ffc4cd365b0, 
tupleIsAlive=true, brstate=0x560ff06859e0)
     at brin.c:650
#5  0x0000560fef12e418 in heapam_index_build_range_scan 
(heapRelation=0x7fed64f37248, indexRelation=0x7fed64f37460, 
indexInfo=0x560ff0685b48, allow_sync=false, anyvisible=true,
     progress=false, start_blockno=0, numblocks=1, 
callback=0x560fef0b37c9 <brinbuildCallback>, 
callback_state=0x560ff06859e0, scan=0x560ff0685d18) at heapam_handler.c:1663
#6  0x0000560fef0b2858 in table_index_build_range_scan 
(table_rel=0x7fed64f37248, index_rel=0x7fed64f37460, 
index_info=0x560ff0685b48, allow_sync=false, anyvisible=true, 
progress=false,
     start_blockno=0, numblocks=1, callback=0x560fef0b37c9 
<brinbuildCallback>, callback_state=0x560ff06859e0, scan=0x0) at 
../../../../src/include/access/tableam.h:1544
#7  0x0000560fef0b4dac in summarize_range (indexInfo=0x560ff0685b48, 
state=0x560ff06859e0, heapRel=0x7fed64f37248, heapBlk=0, heapNumBlks=1) 
at brin.c:1240
#8  0x0000560fef0b50d9 in brinsummarize (index=0x7fed64f37460, 
heapRel=0x7fed64f37248, pageRange=0, include_partial=true, 
numSummarized=0x7ffc4cd36928, numExisting=0x0) at brin.c:1375
#9  0x0000560fef0b43bf in brin_summarize_range (fcinfo=0x560ff06840d0) 
at brin.c:933
#10 0x0000560fef339acc in ExecInterpExpr (state=0x560ff0683fe8, 
econtext=0x560ff0683ce8, isnull=0x7ffc4cd36c17) at execExprInterp.c:650

Analysis:

This is a memory management issue in the brin_inclusion_add_value() 
function:

	/* Finally, merge the new value to the existing union. */
	finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_MERGE);
	Assert(finfo != NULL);
	result = FunctionCall2Coll(finfo, colloid,
							   column->bv_values[INCLUSION_UNION], newval);
	if (!attr->attbyval)
		pfree(DatumGetPointer(column->bv_values[INCLUSION_UNION]));
	column->bv_values[INCLUSION_UNION] = result;

This assumes that the merge function returns a newly-palloc'd value. 
That's a shaky assumption; if one of the arguments is an empty range, 
range_merge() returns the other argument, rather than a newly 
constructed value. And surely we can't assume assume that for 
user-defined opclasses.

When that happens, we store the 'newval' directly in 
column->bv_values[INCLUSION_UNION], but it is allocated in a 
shortly-lived memory context (or points directly to a buffer in the 
buffer cache). On the next call, we try to pfree() the old value, but 
the memory context that contained it was already reset.

It took a while to work out the script to reproduce it. The first value 
passed to brin_inclusion_add_value() must be an empty range, so that 
column->bv_values[INCLUSION_UNION] is set to an empty range. On the 
second call, the new value must be non-empty, so that range_merge() 
returns 'newval' unchanged. And it must be a very large value, because 
if it uses a short varlen header, the PG_GETARG_RANGE_P() call in 
range_merge() will make a copy of it.

brin_inclusion_union() has a similar issue, but I didn't write a script 
to reproduce that. Fix attached.

- Heikki

--------------B61B52F709A7BF999237404A
Content-Type: text/x-patch;
 name="fix-brin-inclusion-memory_handling-bug.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="fix-brin-inclusion-memory_handling-bug.patch"

diff --git a/src/backend/access/brin/brin_inclusion.c b/src/backend/access/brin/brin_inclusion.c
index 86788024ef..cbe14cb3b1 100644
--- a/src/backend/access/brin/brin_inclusion.c
+++ b/src/backend/access/brin/brin_inclusion.c
@@ -235,8 +235,13 @@ brin_inclusion_add_value(PG_FUNCTION_ARGS)
 	Assert(finfo != NULL);
 	result = FunctionCall2Coll(finfo, colloid,
 							   column->bv_values[INCLUSION_UNION], newval);
-	if (!attr->attbyval)
+	if (!attr->attbyval && result != column->bv_values[INCLUSION_UNION])
+	{
 		pfree(DatumGetPointer(column->bv_values[INCLUSION_UNION]));
+
+		if (result == newval)
+			result = datumCopy(result, attr->attbyval, attr->attlen);
+	}
 	column->bv_values[INCLUSION_UNION] = result;
 
 	PG_RETURN_BOOL(true);
@@ -574,8 +579,13 @@ brin_inclusion_union(PG_FUNCTION_ARGS)
 	result = FunctionCall2Coll(finfo, colloid,
 							   col_a->bv_values[INCLUSION_UNION],
 							   col_b->bv_values[INCLUSION_UNION]);
-	if (!attr->attbyval)
+	if (!attr->attbyval && result != col_a->bv_values[INCLUSION_UNION])
+	{
 		pfree(DatumGetPointer(col_a->bv_values[INCLUSION_UNION]));
+
+		if (result == col_b->bv_values[INCLUSION_UNION])
+			result = datumCopy(result, attr->attbyval, attr->attlen);
+	}
 	col_a->bv_values[INCLUSION_UNION] = result;
 
 	PG_RETURN_VOID();

--------------B61B52F709A7BF999237404A--