public inbox for [email protected]
help / color / mirror / Atom feedFrom: Ewan Young <[email protected]>
To: Heikki Linnakangas <[email protected]>
To: Peter Eisentraut <[email protected]>
Cc: [email protected]
Cc: [email protected]
Subject: Re: BUG #19545: Integer truncation of `GinTuple.keylen` causes out-of-bounds read in parallel GIN index build
Date: Thu, 9 Jul 2026 13:16:54 +0800
Message-ID: <CAON2xHN5qGXcn1Z00pQAogf4y5rCQ1K04yvrYAORBa5i+ucLBQ@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<CAON2xHMXYLwkq4UaNbUtmaw1ZKne6OqdyyiACFQUbCj+XRBvQQ@mail.gmail.com>
<[email protected]>
<CAON2xHPV-7O0MsFuizJv8cbUG7ZJfLCAqzkGYgoxF9WBe1v9Jg@mail.gmail.com>
<[email protected]>
Hi Heikki and Peter,
On Wed, Jul 8, 2026 at 10:36 PM Heikki Linnakangas <[email protected]> wrote:
>
> On 08/07/2026 14:34, Ewan Young wrote:
> > On Wed, Jul 8, 2026 at 3:52 PM Heikki Linnakangas <[email protected]> wrote:
> >>
> >> On 08/07/2026 09:27, Ewan Young wrote:
> >>> Hi Yuelin,
> >>>
> >>> Thanks for the very precise report -- I reproduced it on master and your
> >>> analysis is exactly right. _gin_build_tuple() builds the whole GinTuple
> >>> (palloc size, key memcpy, TID-list offset) from the int keylen, but the
> >>> stored GinTuple.keylen is uint16, so a key wider than 65535 bytes has its
> >>> stored length truncated. On read-back GinTupleGetFirst() and
> >>> _gin_parse_tuple_items() recompute the posting-list offset from the
> >>> truncated value, and ginPostingListDecodeAllSegments() then walks the key
> >>> bytes, aborting (or reading past the allocation on non-assert builds)
> >>> exactly as you saw. It's parallel-only because only the parallel path
> >>> serializes a GinTuple.
> >>>
> >>> I went with your fix A -- widening keylen to uint32 (attached). It's the
> >>> minimal root-cause fix: the stored length now matches the length the rest
> >>> of the function already uses.
> >>
> >> Ugh, the datatypes used for keylen are all over the place. In GinTuple
> >> struct it was 'uint16', in GinBuffer it's Size, and in the
> >> _gin_build_tuple() function's local variable it's 'int'. Would be good
> >> to make them consistent.
> >
> > Good point, agreed. v2 (attached) uses int for keylen everywhere: in
> > GinTuple (was uint16) and in GinBuffer (was Size); the local in
> > _gin_build_tuple() was already int. int matches the tuplen and nitems
> > fields of GinTuple and is plenty wide (a key can't exceed the 1GB varlena
> > limit), so it seemed like the natural choice.
>
> When I built this with "-fsanitize=alignment,undefined" flag, it
> triggers a sanity check in the 'jsonb' test:
>
> (gdb) bt
> #0 __pthread_kill_implementation (threadid=281473024218464,
> signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44
> #1 0x0000ffff89fa7e24 [PAC] in __pthread_kill_internal
> (threadid=<optimized out>, signo=6) at ./nptl/pthread_kill.c:89
> #2 0x0000ffff89f56940 in __GI_raise (sig=sig@entry=6) at
> ../sysdeps/posix/raise.c:26
> #3 0x0000ffff89f41a84 [PAC] in __GI_abort () at ./stdlib/abort.c:77
> #4 0x0000ffff8a0fc600 [PAC] in __sanitizer::Abort () at
> ../../../../src/libsanitizer/sanitizer_common/sanitizer_posix_libcdep.cpp:143
> #5 0x0000ffff8a10bc94 [PAC] in __sanitizer::Die () at
> ../../../../src/libsanitizer/sanitizer_common/sanitizer_termination.cpp:58
> #6 0x0000ffff8a0e7da0 [PAC] in __ubsan::ScopedReport::~ScopedReport
> (this=this@entry=0xffffd8a433d0, __in_chrg=<optimized out>)
> at ../../../../src/libsanitizer/ubsan/ubsan_diag.cpp:402
> #7 0x0000ffff8a0eb10c [PAC] in handleTypeMismatchImpl (Data=<optimized
> out>, Pointer=187650525702668, Opts=...)
> at ../../../../src/libsanitizer/ubsan/ubsan_handlers.cpp:137
> #8 0x0000ffff8a0ebc0c [PAC] in
> __ubsan::__ubsan_handle_type_mismatch_v1_abort (Data=<optimized out>,
> Pointer=<optimized out>)
> at ../../../../src/libsanitizer/ubsan/ubsan_handlers.cpp:147
> #9 0x0000aaaab7d3f2e4 [PAC] in GinBufferKeyEquals
> (buffer=0xaaaacaee4330, tup=0xaaaacaed29f8) at
> ../src/backend/access/gin/gininsert.c:1382
> #10 0x0000aaaab7d414c0 in GinBufferCanAddKey (buffer=0xaaaacaee4330,
> tup=0xaaaacaed29f8) at ../src/backend/access/gin/gininsert.c:1633
> #11 0x0000aaaab7d42500 in _gin_process_worker_data
> (state=0xffffd8a43890, worker_sort=0xaaaacae16fb0, progress=true) at
> ../src/backend/access/gin/gininsert.c:1899
> #12 0x0000aaaab7d43618 in _gin_parallel_scan_and_build
> (state=0xffffd8a43890, ginshared=0xffff8b9b83a0,
> sharedsort=0xffff8b9b8340, heap=0xffff7d7012a8,
> index=0xffff7d708768, sortmem=21845, progress=true) at
> ../src/backend/access/gin/gininsert.c:2085
> #13 0x0000aaaab7d42378 in _gin_leader_participate_as_worker
> (buildstate=0xffffd8a43890, heap=0xffff7d7012a8, index=0xffff7d708768)
> at ../src/backend/access/gin/gininsert.c:1834
> #14 0x0000aaaab7d3d7b0 in _gin_begin_parallel
> (buildstate=0xffffd8a43890, heap=0xffff7d7012a8, index=0xffff7d708768,
> isconcurrent=true, request=2)
> at ../src/backend/access/gin/gininsert.c:1103
> #15 0x0000aaaab7d3ae3c in ginbuild (heap=0xffff7d7012a8,
> index=0xffff7d708768, indexInfo=0xaaaacad8f5b0) at
> ../src/backend/access/gin/gininsert.c:700
> #16 0x0000aaaab807c110 in index_build (heapRelation=0xffff7d7012a8,
> indexRelation=0xffff7d708768, indexInfo=0xaaaacad8f5b0, isreindex=false,
> parallel=true, progress=true)
> at ../src/backend/catalog/index.c:3099
> #17 0x0000aaaab8074514 in index_concurrently_build
> (heapRelationId=41578, indexRelationId=41993) at
> ../src/backend/catalog/index.c:1543
>
> That's this line:
>
> #9 0x0000aaaab7d3f2e4 [PAC] in GinBufferKeyEquals
> (buffer=0xaaaacaee4330, tup=0xaaaacaed29f8) at
> ../src/backend/access/gin/gininsert.c:1382
> 1382 tupkey = (buffer->typbyval) ? *(Datum *) tup->data :
> PointerGetDatum(tup->data);
>
> So we have a hidden assumption that 'data' is Datum-aligned.
>
>
> In _gin_parse_tuple_key() we do this instead:
>
> Datum key;
> ...
> if (a->typbyval)
> {
> memcpy(&key, a->data, a->keylen);
> return key;
> }
>
> That one doesn't require the alignment. I would be inclined to always
> use memcpy() when 'typbyval==true', as above, to not be sensitive to the
> alignment. However, I think we assume that it's aligned for the
> 'typbyval==false' case anyway, as we just do DatumGetPoint(a->data).
Good catch, and this is really surfaced by widening keylen: on master
GinTuple.data lands at offset 16, which is MAXALIGN'd, so that read is
(accidentally) fine; growing the header pushed data off an 8-byte
boundary and exposed the unaligned Datum load.
Rather than pad data back to MAXALIGN (which grows every GinTuple), I did
what you suggest here -- read the key via the existing
_gin_parse_tuple_key() helper, which already copies byval keys out with
memcpy() and so makes no alignment assumption. That also removes the
duplicated "byval ? deref : pointer" logic, so the key is now read the
same way everywhere; the byref branch is unchanged.
With the _gin_parse_tuple_key() change the sanitizer is clean -- both at that
4-aligned offset and with Size (where data happens to be back to
MAXALIGN'd), so the fix doesn't depend on the realignment.
>
> The straightforward fix is to add padding to make 'data' MAXALIGNed. It
> makes GinTuples larger, which is bad for performance, but it's probably
> fine.
>
> That said, I actually wonder why we need to store 'typbyval' and
> 'typlen' in GinTuple at all. That information could be looked up using
> 'attrnum'. Maybe 'typbyval' is good for performance in the comparison
> functions, but AFAICS GinTuple->typbyval is only used to copy it into
> GinBuffer in GinBufferStoreTuple(), which I think could easily afford to
> look it up.
I like the idea, but they're not only used to seed GinBuffer -- typbyval
is also read on the sort's hot path, in _gin_parse_tuple_key() (and thus
_gin_compare_tuples()), which only receives the GinTuple, not the index
tupdesc. So dropping them means threading the attr metadata into the
tuplesort comparator, which felt like a larger, separable cleanup than
this fix. Happy to look at it as a follow-up if you think it's
worthwhile, but I'd lean toward not blocking the bug fix on it.
>
> >>> I preferred it over an explicit ereport at
> >>> UINT16_MAX, since 65535 isn't a meaningful GIN limit -- the entry-tree item
> >>> limit is much smaller and is applied to the (compressed) tuple by
> >>> GinFormTuple() -- so rejecting there would be an arbitrary cutoff.
> >>
> >> Hmm, we don't compress the key data though, so a tuple with a key larger
> >> than 65535 will inevitably fail in GinFormTuple(), right? I agree it
> >
> > That was my first thought too, but it turns out we do compress it, just
> > not in _gin_build_tuple(). GinFormTuple() builds the on-page tuple via
> > index_form_tuple(), whose TOAST_INDEX_HACK path compresses a compressible
> > key over TOAST_INDEX_TARGET (~BLCKSZ/16) inline before the GinMaxItemSize
> > check runs. So that check sees the *compressed* key, and a large but
> > compressible key sails through. It's only the parallel path's GinTuple
> > that keeps the key uncompressed, which is exactly where the uint16
> > truncation bit us.
> >
> > Concretely, unpatched master indexes a key far larger than GinMaxItemSize
> > just fine in a serial build:
> >
> > CREATE TABLE t (a text[]);
> > INSERT INTO t SELECT ARRAY[repeat('x',100000)] FROM generate_series(1,50);
> > SET max_parallel_maintenance_workers = 0; -- force a serial build
> > CREATE INDEX ON t USING gin (a); -- succeeds; key is
> > 100000 bytes
>
> Oh, ok, I stand corrected. Let's keep that working then.
Thanks -- leaving GinFormTuple() as the single size gate, then.
> Size (or size_t) is the correct type for sizes of objects in memory.
>
> Note that the return type of VARSIZE_ANY() is already Size, so by using
> int you are still doing a type truncation, and by using a signed type
> you are introducing unnecessary potential for confusion.
Agreed, that's clearly better. v3 (attached) uses Size for
GinTuple.keylen (GinBuffer.keylen already was Size), and also for the
local in _gin_build_tuple(), which was the int that truncated
VARSIZE_ANY() in the first place.
Thanks again for the review!
>
> - Heikki
>
--
Regards,
Ewan Young
Attachments:
[application/octet-stream] v3-0001-Fix-parallel-GIN-index-build-with-keys-larger-tha.patch (3.7K, ../CAON2xHN5qGXcn1Z00pQAogf4y5rCQ1K04yvrYAORBa5i+ucLBQ@mail.gmail.com/2-v3-0001-Fix-parallel-GIN-index-build-with-keys-larger-tha.patch)
download | inline diff:
From 2e2d5c5cec2cefd04ae64486a8333e4ed4191112 Mon Sep 17 00:00:00 2001
From: Ewan Young <[email protected]>
Date: Thu, 9 Jul 2026 18:19:51 +0800
Subject: [PATCH v3] Fix parallel GIN index build with keys larger than 65535
bytes
During a parallel GIN build each key is serialized into a GinTuple, a
transient representation used only while sorting. _gin_build_tuple()
lays out the whole tuple -- the palloc size, the key memcpy, and the
offset to the posting list -- from a local keylen holding VARSIZE_ANY()
of the key. But the stored GinTuple.keylen field was uint16, so a key
wider than 65535 bytes had its stored length silently truncated.
On read-back, GinTupleGetFirst() and _gin_parse_tuple_items() recompute
the posting-list offset from the truncated keylen and land inside the
key data. ginPostingListDecodeAllSegments() then decodes garbage,
tripping an assertion (or reading past the allocation in a non-assert
build). Only parallel builds are affected, because only the parallel
path materializes a GinTuple.
Widen GinTuple.keylen to Size so the stored length matches what
_gin_build_tuple() computes. Size is the natural type here: it's what
VARSIZE_ANY() returns and what GinBuffer.keylen already uses. The
_gin_build_tuple() local was int, which truncated VARSIZE_ANY() the
same way; make it Size too.
Widening keylen also moves GinTuple.data, which happened to land at an
8-byte boundary before. While the Size layout keeps data MAXALIGNed,
GinBufferKeyEquals() should not depend on that: it read an unaligned
Datum via "*(Datum *) tup->data" for byval keys. Use
_gin_parse_tuple_key() there instead, which copies the key out with
memcpy() and so makes no assumption about the alignment of the data
array, matching how the key is read everywhere else.
Bug: #19545
Reported-by: Yuelin Wang <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/access/gin/gininsert.c | 10 ++++++----
src/include/access/gin_tuple.h | 2 +-
2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/src/backend/access/gin/gininsert.c b/src/backend/access/gin/gininsert.c
index cb9ed3b563c..53d2a3f9f70 100644
--- a/src/backend/access/gin/gininsert.c
+++ b/src/backend/access/gin/gininsert.c
@@ -1376,10 +1376,12 @@ GinBufferKeyEquals(GinBuffer *buffer, GinTuple *tup)
return true;
/*
- * For the tuple, get either the first sizeof(Datum) bytes for byval
- * types, or a pointer to the beginning of the data array.
+ * Get the key from the tuple. We use _gin_parse_tuple_key() rather than
+ * reading tup->data directly, because for byval types the data is only
+ * stored/read with memcpy() (the data array is not guaranteed to be
+ * aligned enough to dereference as a Datum).
*/
- tupkey = (buffer->typbyval) ? *(Datum *) tup->data : PointerGetDatum(tup->data);
+ tupkey = _gin_parse_tuple_key(tup);
r = ApplySortComparator(buffer->key, false,
tupkey, false,
@@ -2248,7 +2250,7 @@ _gin_build_tuple(OffsetNumber attrnum, unsigned char category,
char *ptr;
Size tuplen;
- int keylen;
+ Size keylen;
dlist_mutable_iter iter;
dlist_head segments;
diff --git a/src/include/access/gin_tuple.h b/src/include/access/gin_tuple.h
index 7bde05e2de4..99a31578ac1 100644
--- a/src/include/access/gin_tuple.h
+++ b/src/include/access/gin_tuple.h
@@ -23,7 +23,7 @@ typedef struct GinTuple
{
int tuplen; /* length of the whole tuple */
OffsetNumber attrnum; /* attnum of index key */
- uint16 keylen; /* bytes in data for key value */
+ Size keylen; /* bytes in data for key value */
int16 typlen; /* typlen for key */
bool typbyval; /* typbyval for key */
signed char category; /* category: normal or NULL? */
--
2.47.3
view thread (9+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: BUG #19545: Integer truncation of `GinTuple.keylen` causes out-of-bounds read in parallel GIN index build
In-Reply-To: <CAON2xHN5qGXcn1Z00pQAogf4y5rCQ1K04yvrYAORBa5i+ucLBQ@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox