Re: BUG #19545: Integer truncation of `GinTuple.keylen` causes out-of-bounds read in parallel GIN index build

Hi Heikki and Peter,

On Wed, Jul 8, 2026 at 10:36 PM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>
> On 08/07/2026 14:34, Ewan Young wrote:
> > On Wed, Jul 8, 2026 at 3:52 PM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> 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(at)entry=6, no_tid=no_tid(at)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(at)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(at)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