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

Hi Peter,

Thanks for picking this up again, and for the detailed guidance.

On Fri, Sep 4, 2026 at 6:25 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>
> On 09.07.26 07:16, Ewan Young wrote:
> >> 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.
>
> In don't think the use of _gin_parse_tuple_key() is sufficient because
> it only handles the byval side, but the byref side can still fail
> because of misalignment. Fixing that by copying out byref values as
> well seems like a more extensive change.

Good point, I had only looked at the byval side. Dropped that hunk.

>
> The easiest fix (considering backpatching) is that we ensure that
> GinTuple.data is maxaligned. On 64-bit platforms, we get that by

Done in the attached v5, as two patches:

master: keylen (and the local in _gin_build_tuple()) becomes Size, and
data gets alignas(MAXIMUM_ALIGNOF). I put alignas between the type and
the name, because pgindent mangles a leading alignas on a struct member
that isn't the first one (the existing alignas members in the tree all
are); pg_crc32c_armv8.c has precedent for that ordering. Easy to flip
if you'd rather.

REL_18_STABLE: the union variant. A flexible array member isn't
allowed in a union, so the union goes on keylen instead, with the
PGAlignedBlock members:

union
{
Size keylen;
double force_align_d;
int64 force_align_i64;
} k;

That makes it 8 bytes and MAXALIGN'ed even where Size is 4, and the
fields after it add up to 8 bytes, so data lands at offset 24 on every
platform. A StaticAssertDecl on offsetof(GinTuple, data) guards that.
If you'd rather have the uint64 variant, I'm happy to switch, it's a
quick change.

typlen etc. left alone, as suggested.

One correction to my earlier reply to Tom: with Size the header grows
from 16 to 24 bytes on 64-bit, i.e. 8 bytes per GinTuple, not 4.

Tested both branches with -fsanitize=alignment,undefined: the reported
case now builds and passes gin_index_check(); an incompressible 128kB
key fails with the same error in serial and parallel builds; parallel
builds over int4/int8/float8/text/numeric/timestamp arrays and jsonb
are amcheck-clean and match seqscan results; make check passes with no
sanitizer reports.

Thanks again for the review.

> changing keylen to type Size, and that's the correct type anyway
> relative to the surrounding code, so that seems sound. There is no
> point in trying for a smaller type (like uint32); that wouldn't buy
> anything unless you want to consider more extensive surgery in that struct.
>
> (I suggest leaving the type of the .typlen field out of this discussion.
> It would be more correct to use Size for that as well, considering the
> surrounding code. But then we might realize that .nitems is also of the
> wrong type, and the types of GinTuple and GinBuffer don't match
> completely, and so on. Let's leave that for another day.)
>
> On 32-bit platforms, it's more complicated because some of them have
> MAXIMUM_ALIGNOF 4 and some 8. So using a 4-byte Size would make the
> offset of .data 20 but that wouldn't work on platforms with
> MAXIMUM_ALIGNOF == 8.
>
> In PG19 and later we can force the alignment directly using
>
> alignas(MAXIMUM_ALIGNOF) char data[FLEXIBLE_ARRAY_MEMBER];
>
> In PG18, we could either force the alignment using some union trick, or
> we could brute-force the issue by making keylen of type uint64, possibly
> combined with a static assertion about the alignment of the .data field
> somewhere.
>
> I would prefer the union trick. That seems consistent with how
> alignment is forced elsewhere (before the introduction of alignas).
>
> (The code is new in PG18, commit 8492feb98f6.)
>

--
Regards,
Ewan Young