Subject: Fw: Re: heap_force_common in contrib/pg_surgery/heap_surgery.c has an off by one stack buffer overflow

From: [email-masked]<[email-masked]>
Date: Thu, 4 Jun 2026 00:22:53 +0800 (GMT+08:00)

---Original---

From: "Noah Misch"<[email-masked]>

Date: Wed, Jun 3, 2026 23:25 PM

To: "王跃林"<[email-masked]>;

Cc: "security"<[email-masked]>;

Subject: Re: heap_force_common in contrib/pg_surgery/heap_surgery.c has an off by one stack buffer overflow

On Sat, May 23, 2026 at 11:56:59AM +0800, 王跃林 wrote:
> PoC
>
>  CREATE EXTENSION IF NOT EXISTS pg_surgery
>  â
>  CREATE TABLE vuln_005_t()
>  â
>  INSERT INTO vuln_005_t SELECT FROM generate_series(1, 291)
>  â
>  SELECT heap_force_freeze('vuln_005_t'::regclass,
>                           ARRAY['(0, 291)']::tid[])
>
>    The final statement triggers the bug.
>
> Results
>
>    The debug build crashed with:
>  TRAP: failed Assert("offno < MaxHeapTuplesPerPage"), File: "heap_surgery.c", Li
> ne: 231
>  server closed the connection unexpectedly
>
>    In a release build, the assertion is removed and the out of bounds
>    write remains.

Thanks for the report.  The function is superuser-only, so this is not a vuln.
Please report the overflow bug to [email-masked].

---

From: surya poondla <[email-masked]>
Date: Wed, 3 Jun 2026 15:31:27 -0700

Hi 王跃林,

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.

psql (19beta1)

Type "help" for help.

postgres=# CREATE EXTENSION IF NOT EXISTS pg_surgery;

CREATE EXTENSION

postgres=# CREATE TABLE vuln_005_t();

CREATE TABLE

postgres=# INSERT INTO vuln_005_t SELECT FROM generate_series(1, 291);

INSERT 0 291

postgres=# SELECT heap_force_freeze('vuln_005_t'::regclass, ARRAY['(0,
291)']::tid[]);

        This probably means the server terminated abnormally

        before or while processing the request.

The connection to the server was lost. Attempting reset: Failed.

The connection to the server was lost. Attempting reset: Failed.

!?> q

-?> q

-?>

!?> quit

Proposed patch attached.  It does two things:
1. Resize include_this_tid[] to MaxHeapTuplesPerPage + 1 so every legal
1-based offset has a slot.  This removes the structural off-by-one
2. Extend the per-TID input check to also reject offno > MaxHeapTuplesPerPage,
so a corrupted page whose pd_lower lets max offset exceed the structural
maximum cannot reach the array either.

With the patch I no longer see the crash

postgres=#   DROP TABLE IF EXISTS vuln_005_t;

DROP TABLE

postgres=#   DROP EXTENSION IF EXISTS pg_surgery;

DROP EXTENSION

postgres=#

postgres=# CREATE EXTENSION pg_surgery;

CREATE EXTENSION

postgres=# CREATE TABLE vuln_005_t();

CREATE TABLE

postgres=# INSERT INTO vuln_005_t SELECT FROM generate_series(1, 291);

INSERT 0 291

postgres=# SELECT count(*) FROM vuln_005_t;

 count

-------

   291

(1 row)

postgres=# SELECT heap_force_freeze('vuln_005_t'::regclass, ARRAY['(0,
291)']::tid[]);

 heap_force_freeze

-------------------

(1 row)

Regards,
Surya Poondla

---

From: 王跃林 <[email-masked]>
Date: Thu, 4 Jun 2026 10:47:05 +0800 (GMT+08:00)

Thanks for your reply!

 王跃林
[email-masked]

Original:
From：surya poondla <[email-masked]>Date：2026-06-04 06:31:27(中国 (GMT+08:00))To：[email-masked]<[email-masked]>Cc：pgsql-bugs<[email-masked]>Subject：Re: Fw: Re: heap_force_common in contrib/pg_surgery/heap_surgery.c has an off by one stack buffer overflowHi 王跃林,

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.

psql (19beta1)
Type "help" for help.

postgres=# SELECT heap_force_freeze('vuln_005_t'::regclass, ARRAY['(0, 291)']::tid[]);

Proposed patch attached.  It does two things:
1. Resize include_this_tid[] to MaxHeapTuplesPerPage + 1 so every legal 1-based offset has a slot.  This removes the structural off-by-one
2. Extend the per-TID input check to also reject offno > MaxHeapTuplesPerPage, so a corrupted page whose pd_lower lets max offset exceed the structural maximum cannot reach the array either.

postgres=# SELECT heap_force_freeze('vuln_005_t'::regclass, ARRAY['(0, 291)']::tid[]);
 heap_force_freeze 
-------------------

(1 row)

Regards,
Surya Poondla

---

From: Michael Paquier <[email-masked]>
Date: Thu, 4 Jun 2026 16:31:20 +0900

> so the largest legal offset (MaxHeapTuplesPerPage itself) lands one slot

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

---

From: Ashutosh Sharma <[email-masked]>
Date: Thu, 4 Jun 2026 14:42:23 +0530

Hi Michael,

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

---

From: Michael Paquier <[email-masked]>
Date: Fri, 5 Jun 2026 08:17:15 +0900

> Surya's approach for that reason.

That depends on the code path involved:
- pruneheap.c has "processed" and "htsv", that use a +1 index to avoid
the substract, where we also worry about performance.
- heapam_handler.c has in_index, that uses a -1 index.

At the end, the first pattern is an outlier, we don't need to worry
about performance in pg_surgery, and we're talking about three lines
of code in pg_surgery to change (two for include_this_tid, one for the
assertion).  With all that in mind, I'd just do a -1 conversion and
call it a day.  :)
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Fri, 5 Jun 2026 15:57:06 +0900

> assertion).  With all that in mind, I'd just do a -1 conversion and

Which implies something like the simpler patch attached.
--
Michael
diff --git a/contrib/pg_surgery/heap_surgery.c b/contrib/pg_surgery/heap_surgery.c
index ae4e7c0136cc..6a92d2bd5fec 100644
--- a/contrib/pg_surgery/heap_surgery.c
+++ b/contrib/pg_surgery/heap_surgery.c
@@ -228,8 +228,8 @@ heap_force_common(FunctionCallInfo fcinfo, HeapTupleForceOption heap_force_opt)
 			}
 
 			/* Mark it for processing. */
-			Assert(offno < MaxHeapTuplesPerPage);
-			include_this_tid[offno] = true;
+			Assert((offno - 1) < MaxHeapTuplesPerPage);
+			include_this_tid[offno - 1] = true;
 		}
 
 		/*
@@ -247,7 +247,7 @@ heap_force_common(FunctionCallInfo fcinfo, HeapTupleForceOption heap_force_opt)
 		{
 			ItemId		itemid;
 
-			if (!include_this_tid[curoff])
+			if (!include_this_tid[curoff - 1])
 				continue;
 
 			itemid = PageGetItemId(page, curoff);

---

From: Ashutosh Sharma <[email-masked]>
Date: Fri, 5 Jun 2026 13:30:42 +0530

Hi Michael,

Thanks for the patch.

> Which implies something like the simpler patch attached.

I have one small comment:

"+ Assert((offno - 1) < MaxHeapTuplesPerPage);"

I think this can be simplified to:

Assert(offno <= MaxHeapTuplesPerPage);

Since "offno" is already 1-based, there doesn't seem to be a need to
subtract 1 from it and adjust the comparison accordingly.

--
With Regards,
Ashutosh Sharma.

---

From: Michael Paquier <[email-masked]>
Date: Sat, 6 Jun 2026 09:17:00 +0900

> subtract 1 from it and adjust the comparison accordingly.

Sure.  Changed as you have suggested here, and backpatched down to
v14.
--
Michael

---

From: surya poondla <[email-masked]>
Date: Fri, 5 Jun 2026 18:08:45 -0700

Hi Ashutosh, Michael,

Thank you for raising great points.

Yes, the "subtraction of 1" for index change looks good.

Thank you Michael for pushing the code.

Regards,
Surya Poondla