Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 8e5eef5

Browse files
committed
Fix dereference of dangling pointer in GiST index buffering build.
gistBuildCallback tried to fetch the size of an index tuple that might have already been freed by gistProcessEmptyingQueue. While this seems to usually be harmless in production builds, in principle it could result in a SIGSEGV, or more likely a bogus value for indtuplesSize leading to poor page-split decisions later in the build. The memory management here is confusing and could stand to be refactored, but for the moment it seems to be enough to fetch the tuple size sooner. AFAICT the indtuples[Size] totals aren't used in between these places; even if they were, the updated values shouldn't be any worse to use. So just move the incrementing of the totals up. It's not very clear why our valgrind-using buildfarm animals haven't noticed this problem, because the relevant code path does seem to be exercised according to the code coverage report. I think the reason that we didn't fix this bug after the first report is that I'd wanted to try to understand that better. However, now that it's been re-discovered let's just be pragmatic and fix it already. Original report by Alexander Lakhin (bug #16329), later rediscovered by Egor Chindyaskin (bug #17874). Patch by Alexander Lakhin (commentary by Pavel Borisov and me). Back-patch to all supported branches. Discussion: https://postgr.es/m/16329-7a6aa9b6fa1118a1@postgresql.org Discussion: https://postgr.es/m/17874-63ca6c7ce42d2103@postgresql.org
1 parent 3aa9613 commit 8e5eef5

File tree

1 file changed

+13
-4
lines changed

1 file changed

+13
-4
lines changed

src/backend/access/gist/gistbuild.c

+13-4
Original file line numberDiff line numberDiff line change
@@ -901,6 +901,19 @@ gistBuildCallback(Relation index,
901901
true);
902902
itup->t_tid = *tid;
903903

904+
/* Update tuple count and total size. */
905+
buildstate->indtuples += 1;
906+
buildstate->indtuplesSize += IndexTupleSize(itup);
907+
908+
/*
909+
* XXX In buffering builds, the tempCxt is also reset down inside
910+
* gistProcessEmptyingQueue(). This is not great because it risks
911+
* confusion and possible use of dangling pointers (for example, itup
912+
* might be already freed when control returns here). It's generally
913+
* better that a memory context be "owned" by only one function. However,
914+
* currently this isn't causing issues so it doesn't seem worth the amount
915+
* of refactoring that would be needed to avoid it.
916+
*/
904917
if (buildstate->buildMode == GIST_BUFFERING_ACTIVE)
905918
{
906919
/* We have buffers, so use them. */
@@ -916,10 +929,6 @@ gistBuildCallback(Relation index,
916929
buildstate->giststate, buildstate->heaprel, true);
917930
}
918931

919-
/* Update tuple count and total size. */
920-
buildstate->indtuples += 1;
921-
buildstate->indtuplesSize += IndexTupleSize(itup);
922-
923932
MemoryContextSwitchTo(oldCtx);
924933
MemoryContextReset(buildstate->giststate->tempCxt);
925934

0 commit comments

Comments
 (0)