public inbox for [email protected]
help / color / mirror / Atom feedBUG #19545: Integer truncation of `GinTuple.keylen` causes out-of-bounds read in parallel GIN index build
9+ messages / 5 participants
[nested] [flat]
* BUG #19545: Integer truncation of `GinTuple.keylen` causes out-of-bounds read in parallel GIN index build
@ 2026-07-07 14:13 PG Bug reporting form <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: PG Bug reporting form @ 2026-07-07 14:13 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]
The following bug has been logged on the website:
Bug reference: 19545
Logged by: Yuelin Wang
Email address: [email protected]
PostgreSQL version: 19beta1
Operating system: Linux (Ubuntu 24.04, x86_64)
Description:
### Summary
`GinTuple.keylen` is declared `uint16` (max 65535), but in
`_gin_build_tuple()` the local `keylen` is an `int` taken from
`VARSIZE_ANY(key)` (up to ~1 GB for a varlena `NORM_KEY`). The store
`tuple->keylen = keylen` **truncates** when the key exceeds 65535 bytes. The
tuple's *memory layout* (palloc size, key memcpy, TID-array pointer) uses
the full `int` keylen and is correct. Only the stored field is truncated. On
read-back the truncated value mis-computes the posting-list region, so the
decoder walks attacker-controlled bytes past the allocation, which is a heap
out-of-bounds read.
### Details
```c
int keylen; /* 2251 */
keylen = VARSIZE_ANY(DatumGetPointer(key)); /* 2278: up to ~1GB */
tuple->keylen = keylen; /* 2327: stored into uint16 ->
truncation */
```
On read-back `_gin_parse_tuple_items()` (2419-2420) uses the truncated
`a->keylen`, so the posting-list pointer lands ~64 KB early inside the
attacker's key content and `ginPostingListDecodeAllSegments()` decodes
attacker-controlled bytes off the end of the allocation. This is
parallel-only because the `GinMaxItemSize` guard lives in the leader's
`GinFormTuple`, whereas a parallel worker reparses the serialized `GinTuple`
*before* any size gate (present in 19devel).
### Proof of Concept
`array_ops`' `extractValue` returns full-size array-element datums, so a
`text[]` element > 65535 bytes flows in as `NORM_KEY`:
```sql
CREATE SCHEMA vuln_001_sch;
CREATE TABLE vuln_001_sch.vuln_001_t (a text[]);
-- one 100000-byte element per row (> 65535 -> keylen truncation), enough
rows for real sort work
INSERT INTO vuln_001_sch.vuln_001_t
SELECT ARRAY[repeat('x', 100000)] FROM generate_series(1, 300);
-- force a parallel maintenance build (all settable by a non-superuser)
ALTER TABLE vuln_001_sch.vuln_001_t SET (parallel_workers = 4);
SET max_parallel_maintenance_workers = 4;
SET min_parallel_table_scan_size = '0';
CREATE INDEX vuln_001_idx ON vuln_001_sch.vuln_001_t USING gin (a);
```
Run:
```
psql -h /tmp -p 36901 -U ylwang -d postgres -f vuln_001.sql
```
### Result
The parallel build crashed 3 workers (PIDs 114475 to 114477) during `CREATE
INDEX ... USING gin`, on exactly the predicted path
(`ginPostingListDecodeAllSegments`):
```
DEBUG: building index "vuln_001_idx" ... with request for 4 parallel
workers
server closed the connection unexpectedly
parallel worker ... ExceptionalCondition ...
parallel worker ... ginPostingListDecodeAllSegments+0x163
TRAP: failed
Assert("OffsetNumberIsValid(ItemPointerGetOffsetNumber(&segment->first))"),
File: "ginpostinglist.c", Line: 324
```
### Fix
Widen the field to hold real key lengths by declaring `GinTuple.keylen` as
`int`/`uint32` in `gin_tuple.h`. Alternatively, reject oversized keys
explicitly in `_gin_build_tuple()` by calling `ereport(ERROR)` when `keylen
> UINT16_MAX`, so the parallel path fails as cleanly as the serial one.
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #19545: Integer truncation of `GinTuple.keylen` causes out-of-bounds read in parallel GIN index build
@ 2026-07-08 06:27 Ewan Young <[email protected]>
parent: PG Bug reporting form <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Ewan Young @ 2026-07-08 06:27 UTC (permalink / raw)
To: [email protected]; [email protected]
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. 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.
Verified on master with your PoC:
- Before: the parallel build aborts in ginPostingListDecodeAllSegments
(Assert at ginpostinglist.c:324), as reported.
- After: the parallel build succrrect
results -- your 100000-byte element compresses well under the
entry-tree item limit, so it irial build.
- A genuinely incompressible key wider than the item limit now fails
cleanly ("index row requires N) in both
the parallel and serial paths, instead of crashing.
I didn't add a regression test -- forcing parallel maintenance workers plus
a >64 kB key deterministically in -- but I'm
happy to add one if preferred.
Attaching v1 against master; this should go back to the branches that have
parallel GIN builds.
On Wed, Jul 8, 2026 at 7:58 AM PG Bug reporting form
<[email protected]> wrote:
>
> The following bug has been logged on the website:
>
> Bug reference: 19545
> Logged by: Yuelin Wang
> Email address: [email protected]
> PostgreSQL version: 19beta1
> Operating system: Linux (Ubuntu 24.04, x86_64)
> Description:
>
> ### Summary
>
> `GinTuple.keylen` is declared `uint16` (max 65535), but in
> `_gin_build_tuple()` the local `keylen` is an `int` taken from
> `VARSIZE_ANY(key)` (up to ~1 GB for a varlena `NORM_KEY`). The store
> `tuple->keylen = keylen` **truncates** when the key exceeds 65535 bytes. The
> tuple's *memory layout* (palloc size, key memcpy, TID-array pointer) uses
> the full `int` keylen and is correct. Only the stored field is truncated. On
> read-back the truncated value mis-computes the posting-list region, so the
> decoder walks attacker-controlled bytes past the allocation, which is a heap
> out-of-bounds read.
>
> ### Details
>
> ```c
> int keylen; /* 2251 */
> keylen = VARSIZE_ANY(DatumGetPointer(key)); /* 2278: up to ~1GB */
> tuple->keylen = keylen; /* 2327: stored into uint16 ->
> truncation */
> ```
>
> On read-back `_gin_parse_tuple_items()` (2419-2420) uses the truncated
> `a->keylen`, so the posting-list pointer lands ~64 KB early inside the
> attacker's key content and `ginPostingListDecodeAllSegments()` decodes
> attacker-controlled bytes off the end of the allocation. This is
> parallel-only because the `GinMaxItemSize` guard lives in the leader's
> `GinFormTuple`, whereas a parallel worker reparses the serialized `GinTuple`
> *before* any size gate (present in 19devel).
>
> ### Proof of Concept
>
> `array_ops`' `extractValue` returns full-size array-element datums, so a
> `text[]` element > 65535 bytes flows in as `NORM_KEY`:
>
> ```sql
> CREATE SCHEMA vuln_001_sch;
> CREATE TABLE vuln_001_sch.vuln_001_t (a text[]);
>
> -- one 100000-byte element per row (> 65535 -> keylen truncation), enough
> rows for real sort work
> INSERT INTO vuln_001_sch.vuln_001_t
> SELECT ARRAY[repeat('x', 100000)] FROM generate_series(1, 300);
>
> -- force a parallel maintenance build (all settable by a non-superuser)
> ALTER TABLE vuln_001_sch.vuln_001_t SET (parallel_workers = 4);
> SET max_parallel_maintenance_workers = 4;
> SET min_parallel_table_scan_size = '0';
>
> CREATE INDEX vuln_001_idx ON vuln_001_sch.vuln_001_t USING gin (a);
> ```
>
> Run:
>
> ```
> psql -h /tmp -p 36901 -U ylwang -d postgres -f vuln_001.sql
> ```
>
> ### Result
>
> The parallel build crashed 3 workers (PIDs 114475 to 114477) during `CREATE
> INDEX ... USING gin`, on exactly the predicted path
> (`ginPostingListDecodeAllSegments`):
>
> ```
> DEBUG: building index "vuln_001_idx" ... with request for 4 parallel
> workers
> server closed the connection unexpectedly
> parallel worker ... ExceptionalCondition ...
> parallel worker ... ginPostingListDecodeAllSegments+0x163
> TRAP: failed
> Assert("OffsetNumberIsValid(ItemPointerGetOffsetNumber(&segment->first))"),
> File: "ginpostinglist.c", Line: 324
> ```
>
> ### Fix
>
> Widen the field to hold real key lengths by declaring `GinTuple.keylen` as
> `int`/`uint32` in `gin_tuple.h`. Alternatively, reject oversized keys
> explicitly in `_gin_build_tuple()` by calling `ereport(ERROR)` when `keylen
> > UINT16_MAX`, so the parallel path fails as cleanly as the serial one.
>
>
>
>
--
Regards,
Ewan Young
Attachments:
[application/octet-stream] v1-0001-Widen-GinTuple.keylen-to-uint32-to-fix-parallel-G.patch (2.4K, ../../CAON2xHMXYLwkq4UaNbUtmaw1ZKne6OqdyyiACFQUbCj+XRBvQQ@mail.gmail.com/2-v1-0001-Widen-GinTuple.keylen-to-uint32-to-fix-parallel-G.patch)
download | inline diff:
From b78b6b6f0e71403cf93fb15d4ed4470a7dbb9ad9 Mon Sep 17 00:00:00 2001
From: Ewan Young <[email protected]>
Date: Wed, 8 Jul 2026 22:18:56 +0800
Subject: [PATCH v1] Widen GinTuple.keylen to uint32 to fix parallel GIN builds
with large keys
During a parallel GIN index build, _gin_build_tuple() computes the key
length as an int from VARSIZE_ANY() (up to ~1GB for a varlena key) and
lays out the serialized GinTuple -- its palloc size, the key memcpy, and
the offset of the compressed TID list -- using that full length. But
GinTuple.keylen was declared uint16, so the stored field wrapped around
for keys larger than 65535 bytes while the rest of the tuple used the
untruncated length.
On read-back GinTupleGetFirst() and _gin_parse_tuple_items() recompute
the TID-list offset from the truncated keylen, so it points into the key
data instead of at the compressed posting list.
ginPostingListDecodeAllSegments() then decodes arbitrary key bytes, which
aborts the build (Assert in ginpostinglist.c) or reads past the
allocation on non-assert builds. Any user able to run CREATE INDEX with
parallel workers can hit this, e.g. a gin index on a text[] column with
an element wider than 65535 bytes.
This is specific to parallel builds: the serial path never materializes a
GinTuple. The general index-tuple size limit is still enforced by
GinFormTuple() in both cases.
Widen keylen to uint32 so the stored length matches the length used to
build the tuple. A parallel build then correctly indexes large keys that
compress below the entry-tree item limit (as a serial build already does),
and still rejects genuinely oversized keys cleanly via GinFormTuple()
instead of crashing.
Reported-by: Yuelin Wang <[email protected]>
Bug: #19545
---
src/include/access/gin_tuple.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/include/access/gin_tuple.h b/src/include/access/gin_tuple.h
index 7bde05e2de4..a116f3a1a77 100644
--- a/src/include/access/gin_tuple.h
+++ b/src/include/access/gin_tuple.h
@@ -23,7 +23,7 @@ typedef struct GinTuple
{
int tuplen; /* length of the whole tuple */
OffsetNumber attrnum; /* attnum of index key */
- uint16 keylen; /* bytes in data for key value */
+ uint32 keylen; /* bytes in data for key value */
int16 typlen; /* typlen for key */
bool typbyval; /* typbyval for key */
signed char category; /* category: normal or NULL? */
--
2.47.3
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #19545: Integer truncation of `GinTuple.keylen` causes out-of-bounds read in parallel GIN index build
@ 2026-07-08 07:52 Heikki Linnakangas <[email protected]>
parent: Ewan Young <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Heikki Linnakangas @ 2026-07-08 07:52 UTC (permalink / raw)
To: Ewan Young <[email protected]>; [email protected]; [email protected]
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.
> 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
would be a little arbitrary to cut off at 65535, but then again, it
seems a little silly to continue when we know it's just going to fail
later on. I think it would make sense to check if keylen >
GinMaxItemSize. It might still fail later in GinFormTuple(), because the
index tuple headers take some space, but still.
- Heikki
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #19545: Integer truncation of `GinTuple.keylen` causes out-of-bounds read in parallel GIN index build
@ 2026-07-08 11:34 Ewan Young <[email protected]>
parent: Heikki Linnakangas <[email protected]>
0 siblings, 2 replies; 9+ messages in thread
From: Ewan Young @ 2026-07-08 11:34 UTC (permalink / raw)
To: Heikki Linnakangas <[email protected]>; +Cc: [email protected]; [email protected]
Hi Heikki,
Thanks a lot for taking a look, and for the good questions!
On Wed, Jul 8, 2026 at 3:52 PM Heikki Linnakangas <[email protected]> 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.
>
> > 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
So a "keylen > GinMaxItemSize" check in _gin_build_tuple() would end up
rejecting, in a parallel build, keys that a serial build happily accepts,
i.e. it would make parallel builds stricter than serial ones. And a genuinely
incompressible oversized key already fails cleanly in GinFormTuple()
("index row size ... exceeds maximum"), with the server staying up, both
serially and (with this patch) in parallel.
So my inclination is to keep GinFormTuple() as the single size gate: it
enforces the real, post-compression limit and keeps the two build paths
behaving identically. I've attached that variant too (it errors when the
uncompressed keylen exceeds GinMaxItemSize). Happy to go whichever
way you think is best.
> would be a little arbitrary to cut off at 65535, but then again, it
> seems a little silly to continue when we know it's just going to fail
> later on. I think it would make sense to check if keylen >
> GinMaxItemSize. It might still fail later in GinFormTuple(), because the
> index tuple headers take some space, but still.
>
> - Heikki
>
--
Regards,
Ewan Young
Attachments:
[application/octet-stream] v2-0001-Fix-parallel-GIN-index-build-keys-larger-than-65535.patch (2.7K, ../../CAON2xHPV-7O0MsFuizJv8cbUG7ZJfLCAqzkGYgoxF9WBe1v9Jg@mail.gmail.com/2-v2-0001-Fix-parallel-GIN-index-build-keys-larger-than-65535.patch)
download | inline diff:
From 439897dad9470f1836c18c75b409bc152483870f Mon Sep 17 00:00:00 2001
From: Ewan Young <[email protected]>
Date: Thu, 9 Jul 2026 00:42:19 +0800
Subject: [PATCH v2] Fix parallel GIN index build with keys larger than 65535
bytes
During a parallel GIN build each key is serialized into a GinTuple, a
transient representation used only while sorting. _gin_build_tuple()
lays out the whole tuple -- the palloc size, the key memcpy, and the
offset to the posting list -- from a local "int keylen". But the
stored GinTuple.keylen field was uint16, so a key wider than 65535
bytes had its stored length silently truncated.
On read-back, GinTupleGetFirst() and _gin_parse_tuple_items() recompute
the posting-list offset from the truncated keylen and land inside the
key data. ginPostingListDecodeAllSegments() then decodes garbage,
tripping an assertion (or reading past the allocation in a non-assert
build). Only parallel builds are affected, because only the parallel
path materializes a GinTuple.
Widen keylen so the stored length matches the length the rest of
_gin_build_tuple() already computes. While here, make the type used
for the key length consistent -- it was uint16 in GinTuple, Size in
GinBuffer, and int in the _gin_build_tuple() local variable -- by using
int throughout, matching the tuplen and nitems fields of GinTuple.
Bug: #19545
Reported-by: Yuelin Wang <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/access/gin/gininsert.c | 2 +-
src/include/access/gin_tuple.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/gin/gininsert.c b/src/backend/access/gin/gininsert.c
index cb9ed3b563c..01a7ef23372 100644
--- a/src/backend/access/gin/gininsert.c
+++ b/src/backend/access/gin/gininsert.c
@@ -1190,7 +1190,7 @@ typedef struct GinBuffer
OffsetNumber attnum;
GinNullCategory category;
Datum key; /* 0 if no key (and keylen == 0) */
- Size keylen; /* number of bytes (not typlen) */
+ int keylen; /* number of bytes (not typlen) */
/* type info */
int16 typlen;
diff --git a/src/include/access/gin_tuple.h b/src/include/access/gin_tuple.h
index 7bde05e2de4..9ca578652e7 100644
--- a/src/include/access/gin_tuple.h
+++ b/src/include/access/gin_tuple.h
@@ -23,7 +23,7 @@ typedef struct GinTuple
{
int tuplen; /* length of the whole tuple */
OffsetNumber attrnum; /* attnum of index key */
- uint16 keylen; /* bytes in data for key value */
+ int keylen; /* bytes in data for key value */
int16 typlen; /* typlen for key */
bool typbyval; /* typbyval for key */
signed char category; /* category: normal or NULL? */
--
2.47.3
[application/octet-stream] v2-0001-alt-Fix-parallel-GIN-index-build-fail-fast-variant.patch (3.8K, ../../CAON2xHPV-7O0MsFuizJv8cbUG7ZJfLCAqzkGYgoxF9WBe1v9Jg@mail.gmail.com/3-v2-0001-alt-Fix-parallel-GIN-index-build-fail-fast-variant.patch)
download | inline diff:
From 18925af4dd7115a15bdf8b8ea9604b51e45081f6 Mon Sep 17 00:00:00 2001
From: Ewan Young <[email protected]>
Date: Thu, 9 Jul 2026 00:52:01 +0800
Subject: [PATCH v2] Fix parallel GIN index build with keys larger than 65535 (fail-fast variant)
bytes
During a parallel GIN build each key is serialized into a GinTuple, a
transient representation used only while sorting. _gin_build_tuple()
lays out the whole tuple -- the palloc size, the key memcpy, and the
offset to the posting list -- from a local "int keylen". But the
stored GinTuple.keylen field was uint16, so a key wider than 65535
bytes had its stored length silently truncated.
On read-back, GinTupleGetFirst() and _gin_parse_tuple_items() recompute
the posting-list offset from the truncated keylen and land inside the
key data. ginPostingListDecodeAllSegments() then decodes garbage,
tripping an assertion (or reading past the allocation in a non-assert
build). Only parallel builds are affected, because only the parallel
path materializes a GinTuple.
Widen keylen so the stored length matches the length the rest of
_gin_build_tuple() already computes, and make the type consistent --
it was uint16 in GinTuple, Size in GinBuffer, and int in the
_gin_build_tuple() local variable -- by using int throughout.
In addition, reject in _gin_build_tuple() any key whose (uncompressed)
length exceeds GinMaxItemSize, so a parallel build fails fast rather
than serializing a GinTuple that GinFormTuple() would later be unable
to store.
Bug: #19545
Reported-by: Yuelin Wang <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/access/gin/gininsert.c | 19 ++++++++++++++++++-
src/include/access/gin_tuple.h | 2 +-
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/gin/gininsert.c b/src/backend/access/gin/gininsert.c
index cb9ed3b563c..c2365afd1fd 100644
--- a/src/backend/access/gin/gininsert.c
+++ b/src/backend/access/gin/gininsert.c
@@ -1190,7 +1190,7 @@ typedef struct GinBuffer
OffsetNumber attnum;
GinNullCategory category;
Datum key; /* 0 if no key (and keylen == 0) */
- Size keylen; /* number of bytes (not typlen) */
+ int keylen; /* number of bytes (not typlen) */
/* type info */
int16 typlen;
@@ -2281,6 +2281,23 @@ _gin_build_tuple(OffsetNumber attrnum, unsigned char category,
else
elog(ERROR, "unexpected typlen value (%d)", typlen);
+ /*
+ * Reject a key that cannot possibly be stored in the index, so that a
+ * parallel build fails fast instead of serializing a GinTuple that the
+ * leader will be unable to write out. This mirrors the GinMaxItemSize
+ * limit enforced by GinFormTuple() when the merged tuple is finally
+ * written.
+ *
+ * Note the key here is uncompressed; GinFormTuple() applies inline
+ * compression (via index_form_tuple) before its own check, so this is
+ * more conservative and the GinFormTuple() check is still required.
+ */
+ if (keylen > GinMaxItemSize)
+ ereport(ERROR,
+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("index row size %d exceeds maximum %zu for a GIN index",
+ keylen, (Size) GinMaxItemSize)));
+
/* compress the item pointers */
ncompressed = 0;
compresslen = 0;
diff --git a/src/include/access/gin_tuple.h b/src/include/access/gin_tuple.h
index 7bde05e2de4..9ca578652e7 100644
--- a/src/include/access/gin_tuple.h
+++ b/src/include/access/gin_tuple.h
@@ -23,7 +23,7 @@ typedef struct GinTuple
{
int tuplen; /* length of the whole tuple */
OffsetNumber attrnum; /* attnum of index key */
- uint16 keylen; /* bytes in data for key value */
+ int keylen; /* bytes in data for key value */
int16 typlen; /* typlen for key */
bool typbyval; /* typbyval for key */
signed char category; /* category: normal or NULL? */
--
2.47.3
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #19545: Integer truncation of `GinTuple.keylen` causes out-of-bounds read in parallel GIN index build
@ 2026-07-08 14:36 Heikki Linnakangas <[email protected]>
parent: Ewan Young <[email protected]>
1 sibling, 1 reply; 9+ messages in thread
From: Heikki Linnakangas @ 2026-07-08 14:36 UTC (permalink / raw)
To: Ewan Young <[email protected]>; +Cc: [email protected]; [email protected]
On 08/07/2026 14:34, Ewan Young wrote:
> On Wed, Jul 8, 2026 at 3:52 PM Heikki Linnakangas <[email protected]> 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
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #19545: Integer truncation of `GinTuple.keylen` causes out-of-bounds read in parallel GIN index build
@ 2026-07-08 17:14 Peter Eisentraut <[email protected]>
parent: Ewan Young <[email protected]>
1 sibling, 0 replies; 9+ messages in thread
From: Peter Eisentraut @ 2026-07-08 17:14 UTC (permalink / raw)
To: Ewan Young <[email protected]>; Heikki Linnakangas <[email protected]>; +Cc: [email protected]; [email protected]
On 08.07.26 13:34, Ewan Young wrote:
> Hi Heikki,
>
> Thanks a lot for taking a look, and for the good questions!
>
> On Wed, Jul 8, 2026 at 3:52 PM Heikki Linnakangas <[email protected]> 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.
Size (or size_t) is the correct type for sizes of objects in memory.
Note that the return type of VARSIZE_ANY() is already Size, so by using
int you are still doing a type truncation, and by using a signed type
you are introducing unnecessary potential for confusion.
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #19545: Integer truncation of `GinTuple.keylen` causes out-of-bounds read in parallel GIN index build
@ 2026-07-09 05:16 Ewan Young <[email protected]>
parent: Heikki Linnakangas <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Ewan Young @ 2026-07-09 05:16 UTC (permalink / raw)
To: Heikki Linnakangas <[email protected]>; Peter Eisentraut <[email protected]>; +Cc: [email protected]; [email protected]
Hi Heikki and Peter,
On Wed, Jul 8, 2026 at 10:36 PM Heikki Linnakangas <[email protected]> wrote:
>
> On 08/07/2026 14:34, Ewan Young wrote:
> > On Wed, Jul 8, 2026 at 3:52 PM Heikki Linnakangas <[email protected]> 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).
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.
>
> 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 like the idea, but they're not only used to seed GinBuffer -- typbyval
is also read on the sort's hot path, in _gin_parse_tuple_key() (and thus
_gin_compare_tuples()), which only receives the GinTuple, not the index
tupdesc. So dropping them means threading the attr metadata into the
tuplesort comparator, which felt like a larger, separable cleanup than
this fix. Happy to look at it as a follow-up if you think it's
worthwhile, but I'd lean toward not blocking the bug fix on it.
>
> >>> 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.
Thanks -- leaving GinFormTuple() as the single size gate, then.
> Size (or size_t) is the correct type for sizes of objects in memory.
>
> Note that the return type of VARSIZE_ANY() is already Size, so by using
> int you are still doing a type truncation, and by using a signed type
> you are introducing unnecessary potential for confusion.
Agreed, that's clearly better. v3 (attached) uses Size for
GinTuple.keylen (GinBuffer.keylen already was Size), and also for the
local in _gin_build_tuple(), which was the int that truncated
VARSIZE_ANY() in the first place.
Thanks again for the review!
>
> - Heikki
>
--
Regards,
Ewan Young
Attachments:
[application/octet-stream] v3-0001-Fix-parallel-GIN-index-build-with-keys-larger-tha.patch (3.7K, ../../CAON2xHN5qGXcn1Z00pQAogf4y5rCQ1K04yvrYAORBa5i+ucLBQ@mail.gmail.com/2-v3-0001-Fix-parallel-GIN-index-build-with-keys-larger-tha.patch)
download | inline diff:
From 2e2d5c5cec2cefd04ae64486a8333e4ed4191112 Mon Sep 17 00:00:00 2001
From: Ewan Young <[email protected]>
Date: Thu, 9 Jul 2026 18:19:51 +0800
Subject: [PATCH v3] Fix parallel GIN index build with keys larger than 65535
bytes
During a parallel GIN build each key is serialized into a GinTuple, a
transient representation used only while sorting. _gin_build_tuple()
lays out the whole tuple -- the palloc size, the key memcpy, and the
offset to the posting list -- from a local keylen holding VARSIZE_ANY()
of the key. But the stored GinTuple.keylen field was uint16, so a key
wider than 65535 bytes had its stored length silently truncated.
On read-back, GinTupleGetFirst() and _gin_parse_tuple_items() recompute
the posting-list offset from the truncated keylen and land inside the
key data. ginPostingListDecodeAllSegments() then decodes garbage,
tripping an assertion (or reading past the allocation in a non-assert
build). Only parallel builds are affected, because only the parallel
path materializes a GinTuple.
Widen GinTuple.keylen to Size so the stored length matches what
_gin_build_tuple() computes. Size is the natural type here: it's what
VARSIZE_ANY() returns and what GinBuffer.keylen already uses. The
_gin_build_tuple() local was int, which truncated VARSIZE_ANY() the
same way; make it Size too.
Widening keylen also moves GinTuple.data, which happened to land at an
8-byte boundary before. While the Size layout keeps data MAXALIGNed,
GinBufferKeyEquals() should not depend on that: it read an unaligned
Datum via "*(Datum *) tup->data" for byval keys. Use
_gin_parse_tuple_key() there instead, which copies the key out with
memcpy() and so makes no assumption about the alignment of the data
array, matching how the key is read everywhere else.
Bug: #19545
Reported-by: Yuelin Wang <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/access/gin/gininsert.c | 10 ++++++----
src/include/access/gin_tuple.h | 2 +-
2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/src/backend/access/gin/gininsert.c b/src/backend/access/gin/gininsert.c
index cb9ed3b563c..53d2a3f9f70 100644
--- a/src/backend/access/gin/gininsert.c
+++ b/src/backend/access/gin/gininsert.c
@@ -1376,10 +1376,12 @@ GinBufferKeyEquals(GinBuffer *buffer, GinTuple *tup)
return true;
/*
- * For the tuple, get either the first sizeof(Datum) bytes for byval
- * types, or a pointer to the beginning of the data array.
+ * Get the key from the tuple. We use _gin_parse_tuple_key() rather than
+ * reading tup->data directly, because for byval types the data is only
+ * stored/read with memcpy() (the data array is not guaranteed to be
+ * aligned enough to dereference as a Datum).
*/
- tupkey = (buffer->typbyval) ? *(Datum *) tup->data : PointerGetDatum(tup->data);
+ tupkey = _gin_parse_tuple_key(tup);
r = ApplySortComparator(buffer->key, false,
tupkey, false,
@@ -2248,7 +2250,7 @@ _gin_build_tuple(OffsetNumber attrnum, unsigned char category,
char *ptr;
Size tuplen;
- int keylen;
+ Size keylen;
dlist_mutable_iter iter;
dlist_head segments;
diff --git a/src/include/access/gin_tuple.h b/src/include/access/gin_tuple.h
index 7bde05e2de4..99a31578ac1 100644
--- a/src/include/access/gin_tuple.h
+++ b/src/include/access/gin_tuple.h
@@ -23,7 +23,7 @@ typedef struct GinTuple
{
int tuplen; /* length of the whole tuple */
OffsetNumber attrnum; /* attnum of index key */
- uint16 keylen; /* bytes in data for key value */
+ Size keylen; /* bytes in data for key value */
int16 typlen; /* typlen for key */
bool typbyval; /* typbyval for key */
signed char category; /* category: normal or NULL? */
--
2.47.3
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #19545: Integer truncation of `GinTuple.keylen` causes out-of-bounds read in parallel GIN index build
@ 2026-07-09 05:28 Tom Lane <[email protected]>
parent: Ewan Young <[email protected]>
0 siblings, 1 reply; 9+ messages in thread
From: Tom Lane @ 2026-07-09 05:28 UTC (permalink / raw)
To: Ewan Young <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; Peter Eisentraut <[email protected]>; [email protected]; [email protected]
Ewan Young <[email protected]> writes:
> Agreed, that's clearly better. v3 (attached) uses Size for
> GinTuple.keylen (GinBuffer.keylen already was Size), and also for the
> local in _gin_build_tuple(), which was the int that truncated
> VARSIZE_ANY() in the first place.
Am I reading this correctly that you propose using Size for the
length of the key value (keylen) along with int for the length of the
whole tuple (tuplen)?
{
int tuplen; /* length of the whole tuple */
OffsetNumber attrnum; /* attnum of index key */
- uint16 keylen; /* bytes in data for key value */
+ Size keylen; /* bytes in data for key value */
int16 typlen; /* typlen for key */
bool typbyval; /* typbyval for key */
signed char category; /* category: normal or NULL? */
Please explain how that's sane.
I kind of agree with the upthread comment that we should just reject
key lengths exceeding BLCKSZ or so up-front, rather than fooling
around with these field widths. This patch widens GinTuple
noticeably, and will do so more if we also widen tuplen. Is that
free?
regards, tom lane
^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: BUG #19545: Integer truncation of `GinTuple.keylen` causes out-of-bounds read in parallel GIN index build
@ 2026-07-09 08:32 Ewan Young <[email protected]>
parent: Tom Lane <[email protected]>
0 siblings, 0 replies; 9+ messages in thread
From: Ewan Young @ 2026-07-09 08:32 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; Peter Eisentraut <[email protected]>; [email protected]; [email protected]
Hi Tom,
Thanks for taking a look at this, and for pushing back on the field widths.
It made me reconsider the whole approach.
On Thu, Jul 9, 2026 at 1:28 PM Tom Lane <[email protected]> wrote:
>
> Ewan Young <[email protected]> writes:
> > Agreed, that's clearly better. v3 (attached) uses Size for
> > GinTuple.keylen (GinBuffer.keylen already was Size), and also for the
> > local in _gin_build_tuple(), which was the int that truncated
> > VARSIZE_ANY() in the first place.
>
> Am I reading this correctly that you propose using Size for the
> length of the key value (keylen) along with int for the length of the
> whole tuple (tuplen)?
>
> {
> int tuplen; /* length of the whole tuple */
> OffsetNumber attrnum; /* attnum of index key */
> - uint16 keylen; /* bytes in data for key value */
> + Size keylen; /* bytes in data for key value */
> int16 typlen; /* typlen for key */
> bool typbyval; /* typbyval for key */
> signed char category; /* category: normal or NULL? */
>
> Please explain how that's sane.
It isn't -- you're right, and let me back up and lay out the root cause,
because I think it also settles which way to fix it.
In a parallel build each key is serialized into a transient GinTuple for
the tuplesort. _gin_build_tuple() computes the key length into a
full-width local, and lays out the whole tuple from it -- the allocation,
the key copy, and the offset to the posting list that follows the key.
But it then stores that length in GinTuple.keylen, which is uint16, so
for a key longer than 65535 bytes the stored length is truncated.
On read-back (during the merge in _gin_process_worker_data() /
_gin_parallel_merge()), GinTupleGetFirst() and _gin_parse_tuple_items()
recompute the posting-list offset as SHORTALIGN(offsetof(data) + keylen)
using the *truncated* keylen. That lands ~64kB too early, inside the key
bytes, and ginPostingListDecodeAllSegments() then decodes those bytes as
a posting list -- tripping the OffsetNumberIsValid() assert with
assertions on, or reading past the allocation without.
The key point is that this only happens in a parallel build, because only
the parallel path materializes a GinTuple; a serial build inserts the key
straight into the index via GinFormTuple(). So for a >65535-byte key the
two builds already diverge today:
serial: indexes it if it compresses, else clean "index row size
... exceeds maximum"
parallel: crashes
A parallel build is supposed to be equivalent to a serial one -- it's an
optimization that should be transparent to the user, not something that
changes whether CREATE INDEX succeeds.
>
> I kind of agree with the upthread comment that we should just reject
> key lengths exceeding BLCKSZ or so up-front, rather than fooling
I looked into that, and it runs into the equivalence point above, plus
two others:
* _gin_build_tuple() only runs in the parallel path, so a reject there
makes a parallel build refuse keys a serial build indexes fine -- i.e.
it replaces the crash with a *different* serial/parallel divergence,
where the same CREATE INDEX on the same data succeeds or fails
depending on max_parallel_maintenance_workers.
* It would reject keys that serial builds accept today. The real limit is
enforced by GinFormTuple() on the *compressed* tuple, so a
large-but-compressible key is valid and indexes fine:
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, 100000-byte key
Heikki reached the same conclusion earlier in the thread.
* The threshold would be arbitrary: at _gin_build_tuple() time the key is
necessarily uncompressed (it has to stay uncompressed so the tuplesort
comparator can compare key values), so an uncompressed-length cap
doesn't correspond to the GinMaxItemSize limit, which only applies
after compression.
To avoid the divergence we'd have to apply the cap in the serial path
too, which changes long-standing behavior (rejecting keys that index fine
today).
> around with these field widths. This patch widens GinTuple
> noticeably, and will do so more if we also widen tuplen. Is that
> free?
Not free, but small: with keylen as int it's +4 bytes per GinTuple
(offsetof(data) 16 -> 20), tuplen stays int so nothing widens beyond
that, and there's no per-tuple runtime cost.
>
> regards, tom lane
--
Regards,
Ewan Young
^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2026-07-09 08:32 UTC | newest]
Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-07-07 14:13 BUG #19545: Integer truncation of `GinTuple.keylen` causes out-of-bounds read in parallel GIN index build PG Bug reporting form <[email protected]>
2026-07-08 06:27 ` Ewan Young <[email protected]>
2026-07-08 07:52 ` Heikki Linnakangas <[email protected]>
2026-07-08 11:34 ` Ewan Young <[email protected]>
2026-07-08 14:36 ` Heikki Linnakangas <[email protected]>
2026-07-09 05:16 ` Ewan Young <[email protected]>
2026-07-09 05:28 ` Tom Lane <[email protected]>
2026-07-09 08:32 ` Ewan Young <[email protected]>
2026-07-08 17:14 ` Peter Eisentraut <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox