Strange GiST logic leading to uninitialized memory access in pg_trgm gist code

From: Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)uk>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Strange GiST logic leading to uninitialized memory access in pg_trgm gist code
Date: 2020-11-11 09:53:40
Message-ID: 87k0usfdxg.fsf@news-spur.riddles.org.uk
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

(From a report by user "ftzdomino" on IRC of a segfault while loading
data with a pg_trgm gist index)

If gtrgm_picksplit is invoked on a vector of exactly 2 items (which I
think is rare, but it can happen if gistSplit recurses or I think in
cases of secondary splits), then it tries to access cache[2] without
ever having initialized it, causing hilarity to ensue.

What I don't entirely understand is why the code is insisting on
treating the last item as special: given N items, it tries to find seeds
from the first N-1 items only. This would make a vague sort of sense if
the N'th item was always the just-inserted one, but this doesn't appear
to be always the case (e.g. recursive calls in gistSplit, or secondary
splits), and even if it were it's not clear that it would be correct
logic.

What gtrgm_picksplit currently does, as I read it, is:

take first N-1 items
(note that entryvec->n is N+1, it sets maxoff = entryvec->n - 2)
populate a cache of their signatures
find the two furthest apart as seeds
if we didn't choose two, then default to items 1 and 2 as seeds
(note here that if N=2 then item 2 is not cached)
make datums from the cache entries of the two seeds
(explodes here when N=2)
Increase maxoff and construct the cache entry for item N
Split all N items using the two seeds

Now the obvious simple fix is just to reorder those last two operations,
and the original reporter verified that doing so fixed their problem
(patch attached). But I'd really like to understand the logic here and
whether there is any reason to have this special treatment at all - why
would it not be better to just cache all N items upfront and consider
them all as potential seeds?

Another issue I don't understand yet is that even though this code is
largely unchanged since 8.x, the original reporter could not reproduce
the crash on any version before 13.0.

Anyone have any ideas? (If not, I'll commit and backpatch something like
the attached patch at some suitable time.)

--
Andrew (irc:RhodiumToad)

Attachment Content-Type Size
tgrm.patch text/x-patch 1.1 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Eisentraut 2020-11-11 10:07:59 Re: warn_unused_results
Previous Message Amit Langote 2020-11-11 09:52:01 Re: ModifyTable overheads in generic plans