Feedback-ID: ie0a040ee:Fastmail
Message-ID: <479d6c7e-362c-41d8-a263-8d126ecdfd77@eisentraut.org>
Date: Fri, 4 Sep 2026 12:25:10 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: BUG #19545: Integer truncation of `GinTuple.keylen` causes
 out-of-bounds read in parallel GIN index build
To: Ewan Young <kdbase.hack@gmail.com>, Heikki Linnakangas <hlinnaka@iki.fi>
Cc: 1217816127@qq.com, pgsql-bugs@lists.postgresql.org
References: <19545-0f25b7e47351e8fc@postgresql.org>
 <CAON2xHMXYLwkq4UaNbUtmaw1ZKne6OqdyyiACFQUbCj+XRBvQQ@mail.gmail.com>
 <d02fd089-b326-417a-ab2c-aea88e459c63@iki.fi>
 <CAON2xHPV-7O0MsFuizJv8cbUG7ZJfLCAqzkGYgoxF9WBe1v9Jg@mail.gmail.com>
 <d6680315-bc66-48c3-a310-bdad88a86a59@iki.fi>
 <CAON2xHN5qGXcn1Z00pQAogf4y5rCQ1K04yvrYAORBa5i+ucLBQ@mail.gmail.com>
Content-Language: en-US
From: Peter Eisentraut <peter@eisentraut.org>
In-Reply-To: 
 <CAON2xHN5qGXcn1Z00pQAogf4y5rCQ1K04yvrYAORBa5i+ucLBQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/479d6c7e-362c-41d8-a263-8d126ecdfd77%40eisentraut.org>
Precedence: bulk

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.)