Subject: Re: BUG #16329: Valgrind detects an invalid read when building a gist
 index with buffering
From: Alexander Lakhin <exclusion@gmail.com>
To: pgsql-bugs@lists.postgresql.org
References: <16329-7a6aa9b6fa1118a1@postgresql.org>
 <ca1f896b-e5d1-53a7-5bc3-9451ab9aca22@gmail.com>
Message-ID: <e91e45fd-a614-bb87-e9fc-c556c2b95fe5@gmail.com>
Date: Mon, 6 Apr 2020 09:00:00 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.4.1
MIME-Version: 1.0
In-Reply-To: <ca1f896b-e5d1-53a7-5bc3-9451ab9aca22@gmail.com>
Content-Type: multipart/mixed;
 boundary="------------A66D539AE50A59F872972C48"
Content-Language: en-US
Precedence: bulk

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

Hello hackers,
30.03.2020 08:00, Alexander Lakhin wrote:
> 30.03.2020 00:00, PG Bug reporting form wrote:
>> The following bug has been logged on the website:
>>
>> Bug reference:      16329
>> Logged by:          Alexander Lakhin
>> Email address:      exclusion@gmail.com
>> PostgreSQL version: 12.2
>> Operating system:   Ubuntu 18.04
>> Description:        
>>
>> Running the following query under valgrind:
>> create table gist_point_tbl(id int4, p point);
>> insert into gist_point_tbl (id, p)
>> select g, point(g*10+1, g*10+1) from generate_series(1, 30000) g;
>> create index gist_pointidx2 on gist_point_tbl using gist(p) with (buffering
>> = on, fillfactor=50);
>>
>> leads to a memory access error:
>> ==00:00:00:08.755 18585== Invalid read of size 2
>> ==00:00:00:08.756 18585==    at 0x1F94CA: gistBuildCallback
>> (gistbuild.c:495)
>>
Please look at the patch that modifies the gist regression test to make
the issue visible and fixes it by avoiding access to the memory context
that can be reset in gistProcessEmptyingQueue().

Best regards,
Alexander

--------------A66D539AE50A59F872972C48
Content-Type: text/x-patch; charset=UTF-8;
 name="fix-gistbuild.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="fix-gistbuild.patch"

diff --git a/src/backend/access/gist/gistbuild.c b/src/backend/access/gist/gistbuild.c
index 671b5e9186..cee81d58ad 100644
--- a/src/backend/access/gist/gistbuild.c
+++ b/src/backend/access/gist/gistbuild.c
@@ -456,6 +456,10 @@ gistBuildCallback(Relation index,
 	itup = gistFormTuple(buildstate->giststate, index, values, isnull, true);
 	itup->t_tid = *tid;
 
+	/* Update tuple count and total size. */
+	buildstate->indtuples += 1;
+	buildstate->indtuplesSize += IndexTupleSize(itup);
+
 	if (buildstate->bufferingMode == GIST_BUFFERING_ACTIVE)
 	{
 		/* We have buffers, so use them. */
@@ -471,10 +475,6 @@ gistBuildCallback(Relation index,
 					 buildstate->giststate, buildstate->heaprel, true);
 	}
 
-	/* Update tuple count and total size. */
-	buildstate->indtuples += 1;
-	buildstate->indtuplesSize += IndexTupleSize(itup);
-
 	MemoryContextSwitchTo(oldCtx);
 	MemoryContextReset(buildstate->giststate->tempCxt);
 
diff --git a/src/test/regress/expected/gist.out b/src/test/regress/expected/gist.out
index 90edb4061d..8515a63c07 100644
--- a/src/test/regress/expected/gist.out
+++ b/src/test/regress/expected/gist.out
@@ -22,9 +22,12 @@ ERROR:  value 101 out of bounds for option "fillfactor"
 DETAIL:  Valid values are between "10" and "100".
 -- Insert enough data to create a tree that's a couple of levels deep.
 insert into gist_point_tbl (id, p)
-select g,        point(g*10, g*10) from generate_series(1, 10000) g;
+select g,        point(g*10, g*10) from generate_series(1, 20000) g;
 insert into gist_point_tbl (id, p)
 select g+100000, point(g*10+1, g*10+1) from generate_series(1, 10000) g;
+-- Test buffering index build with data
+create index gist_pointidx6 on gist_point_tbl using gist(p) with (buffering = on, fillfactor=50);
+drop index gist_pointidx6;
 -- To test vacuum, delete some entries from all over the index.
 delete from gist_point_tbl where id % 2 = 1;
 -- And also delete some concentration of values.
diff --git a/src/test/regress/sql/gist.sql b/src/test/regress/sql/gist.sql
index b9d398ea94..0efb265ec3 100644
--- a/src/test/regress/sql/gist.sql
+++ b/src/test/regress/sql/gist.sql
@@ -20,11 +20,15 @@ create index gist_pointidx5 on gist_point_tbl using gist(p) with (fillfactor=101
 
 -- Insert enough data to create a tree that's a couple of levels deep.
 insert into gist_point_tbl (id, p)
-select g,        point(g*10, g*10) from generate_series(1, 10000) g;
+select g,        point(g*10, g*10) from generate_series(1, 20000) g;
 
 insert into gist_point_tbl (id, p)
 select g+100000, point(g*10+1, g*10+1) from generate_series(1, 10000) g;
 
+-- Test buffering index build with data
+create index gist_pointidx6 on gist_point_tbl using gist(p) with (buffering = on, fillfactor=50);
+drop index gist_pointidx6;
+
 -- To test vacuum, delete some entries from all over the index.
 delete from gist_point_tbl where id % 2 = 1;
 

--------------A66D539AE50A59F872972C48--