public inbox for [email protected]  

help / color / mirror / Atom feed

From: Ashutosh Sharma <[email protected]>
To: Michael Paquier <[email protected]>
Cc: surya poondla <[email protected]>
Cc: [email protected] <[email protected]>
Cc: [email protected]
Subject: Re: Fw: Re: heap_force_common in contrib/pg_surgery/heap_surgery.c has an off by one stack buffer overflow
Date: Thu, 4 Jun 2026 14:42:23 +0530
Message-ID: <CAE9k0P=sDshMaBDZzEfNPVo62w7PmnecKhtXX-Zn=AzeQ3k5kA@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<CAOVWO5p-nQ2ki88uAUO5TNWNZDmX-ZZZmJ3307K0xnsg4q75rA@mail.gmail.com>
	<[email protected]>

Hi Michael,

On Thu, Jun 4, 2026 at 1:01 PM Michael Paquier <[email protected]> wrote:
>
> On Wed, Jun 03, 2026 at 03:31:27PM -0700, surya poondla wrote:
> > Thank you for reporting the issue, I am able to reproduce it on master.
> > The include_this_tid[] array is sized MaxHeapTuplesPerPage but indexed
> > using 1-based OffsetNumber,
> > so the largest legal offset (MaxHeapTuplesPerPage itself) lands one slot
> > past the end.
>
> -    bool        include_this_tid[MaxHeapTuplesPerPage];
> +    /* Sized +1 because OffsetNumbers are 1-based and can reach MaxHeapTuplesPerPage. */
> +    bool        include_this_tid[MaxHeapTuplesPerPage + 1];
>
> The offset number begins at 1.  Hence, instead of making this array
> larger by one, you could keep it at the same size and adjust the array
> index to use (offno - 1) instead.

I think Surya's approach is worth considering here. Making the helper
array 1-based aligns it naturally with PostgreSQL's convention, where
page offsets are 1-based, and that consistency has real readability
benefits.

With a 1-based helper array:

bool include_this_tid[MaxHeapTuplesPerPage + 1];

we can write:

include_this_tid[offno] = true;

if (!include_this_tid[curoff])
    continue;

i.e. we can simply "mark this offset number" and "check whether this
offset number is included" - no mental translation required.

With a zero-based helper array:

bool include_this_tid[MaxHeapTuplesPerPage];

every access has to do a conversion:

include_this_tid[offno - 1] = true;

if (!include_this_tid[curoff - 1])
    continue;

This works correctly, but it places an ongoing burden on anyone
reading or modifying the code - they need to keep in mind that page
offsets are 1-based, that this particular array is 0-based, that the
subtraction must be applied consistently, that it should be skipped
for InvalidOffsetNumber, and that the two index spaces should not be
inadvertently mixed in future edits.

These are admittedly small risks, but they are real ones. Keeping the
array 1-based eliminates that entire class of potential confusion and
makes the code easier to maintain going forward. I'd lean toward
Surya's approach for that reason.

--
With Regards,
Ashutosh Sharma.

view thread (10+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Fw: Re: heap_force_common in contrib/pg_surgery/heap_surgery.c has an off by one stack buffer overflow
  In-Reply-To: <CAE9k0P=sDshMaBDZzEfNPVo62w7PmnecKhtXX-Zn=AzeQ3k5kA@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox