MIME-Version: 1.0
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>
 <2411429.1783574907@sss.pgh.pa.us>
In-Reply-To: <2411429.1783574907@sss.pgh.pa.us>
From: Ewan Young <kdbase.hack@gmail.com>
Date: Thu, 9 Jul 2026 16:32:45 +0800
Message-ID: 
 <CAON2xHNvjocuWWVVpmymn2X9LoMfquSxCpb1sihAqVCvBroTeg@mail.gmail.com>
Subject: Re: BUG #19545: Integer truncation of `GinTuple.keylen` causes
 out-of-bounds read in parallel GIN index build
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>,
 Peter Eisentraut <peter@eisentraut.org>, 1217816127@qq.com,
	pgsql-bugs@lists.postgresql.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAON2xHNvjocuWWVVpmymn2X9LoMfquSxCpb1sihAqVCvBroTeg%40mail.gmail.com>
Precedence: bulk

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=E2=80=AFPM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Ewan Young <kdbase.hack@gmail.com> 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 =3D 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


--=20
Regards,
Ewan Young