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

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.

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