Message-ID: <d6680315-bc66-48c3-a310-bdad88a86a59@iki.fi>
Date: Wed, 8 Jul 2026 17:36:04 +0300
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>
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>
Content-Language: en-US
From: Heikki Linnakangas <hlinnaka@iki.fi>
In-Reply-To: 
 <CAON2xHPV-7O0MsFuizJv8cbUG7ZJfLCAqzkGYgoxF9WBe1v9Jg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/d6680315-bc66-48c3-a310-bdad88a86a59%40iki.fi>
Precedence: bulk

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

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

- Heikki