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

I have committed these, thanks.

On 07.09.26 10:36, Ewan Young wrote:
> 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.)
>>
>
>