public inbox for [email protected]
help / color / mirror / Atom feedFw: Re: heap_force_common in contrib/pg_surgery/heap_surgery.c has an off by one stack buffer overflow
10+ messages / 5 participants
[nested] [flat]
* Fw: Re: heap_force_common in contrib/pg_surgery/heap_surgery.c has an off by one stack buffer overflow
@ 2026-06-03 16:22 [email protected] <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: [email protected] @ 2026-06-03 16:22 UTC (permalink / raw)
To: pgsql-bugs
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Fw: Re: heap_force_common in contrib/pg_surgery/heap_surgery.c has an off by one stack buffer overflow
@ 2026-06-03 22:31 surya poondla <[email protected]>
parent: [email protected] <[email protected]>
0 siblings, 2 replies; 10+ messages in thread
From: surya poondla @ 2026-06-03 22:31 UTC (permalink / raw)
To: [email protected] <[email protected]>; +Cc: pgsql-bugs
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[]);
server closed the connection unexpectedly
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
Attachments:
[application/octet-stream] 0001-Fix-off-by-one-stack-buffer-overflow-in-pg_surgery.patch (2.2K, 3-0001-Fix-off-by-one-stack-buffer-overflow-in-pg_surgery.patch)
download | inline diff:
From b4c62d4d32683341b3a344936e8ac8ad958d9f7b Mon Sep 17 00:00:00 2001
From: spoondla <[email protected]>
Date: Wed, 3 Jun 2026 15:17:00 -0700
Subject: [PATCH] Fix off-by-one stack buffer overflow in pg_surgery Issue:
heap_force_common() declared include_this_tid[] with MaxHeapTuplesPerPage
slots and indexed it directly with a 1-based OffsetNumber, so an input TID
whose offset number equals MaxHeapTuplesPerPage wrote one byte past the end
of the stack array and this woulod cause a seg fault and crash postgres
Fix:
Updated the size of include_this_tid[] as MaxHeapTuplesPerPage + 1 so every legal
1-based offset has a slot, and reject offno > MaxHeapTuplesPerPage in
the per-TID validation so a corrupted page header that reports an
impossibly large maxoffset cannot reach the array either.
---
contrib/pg_surgery/heap_surgery.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/contrib/pg_surgery/heap_surgery.c b/contrib/pg_surgery/heap_surgery.c
index ae4e7c0136c..19169131815 100644
--- a/contrib/pg_surgery/heap_surgery.c
+++ b/contrib/pg_surgery/heap_surgery.c
@@ -92,7 +92,8 @@ heap_force_common(FunctionCallInfo fcinfo, HeapTupleForceOption heap_force_opt)
Relation rel;
OffsetNumber curr_start_ptr,
next_start_ptr;
- bool include_this_tid[MaxHeapTuplesPerPage];
+ /* Sized +1 because OffsetNumbers are 1-based and can reach MaxHeapTuplesPerPage. */
+ bool include_this_tid[MaxHeapTuplesPerPage + 1];
if (RecoveryInProgress())
ereport(ERROR,
@@ -193,7 +194,9 @@ heap_force_common(FunctionCallInfo fcinfo, HeapTupleForceOption heap_force_opt)
ItemId itemid;
/* Check whether the offset number is valid. */
- if (offno == InvalidOffsetNumber || offno > maxoffset)
+ if (offno == InvalidOffsetNumber ||
+ offno > maxoffset ||
+ offno > MaxHeapTuplesPerPage)
{
ereport(NOTICE,
errmsg("skipping tid (%u, %u) for relation \"%s\" because the item number is out of range",
@@ -228,7 +231,7 @@ heap_force_common(FunctionCallInfo fcinfo, HeapTupleForceOption heap_force_opt)
}
/* Mark it for processing. */
- Assert(offno < MaxHeapTuplesPerPage);
+ Assert(offno <= MaxHeapTuplesPerPage);
include_this_tid[offno] = true;
}
--
2.39.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Fw: Re: heap_force_common in contrib/pg_surgery/heap_surgery.c has an off by one stack buffer overflow
@ 2026-06-04 02:47 王跃林 <[email protected]>
parent: surya poondla <[email protected]>
1 sibling, 0 replies; 10+ messages in thread
From: 王跃林 @ 2026-06-04 02:47 UTC (permalink / raw)
To: surya poondla <[email protected]>; +Cc: pgsql-bugs
Thanks for your reply!
王跃林
[email protected]
Original:
From：surya poondla <[email protected]>Date：2026-06-04 06:31:27(中国 (GMT+08:00))To：[email protected]<[email protected]>Cc：pgsql-bugs<[email protected]>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=# 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[]);
server closed the connection unexpectedly
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
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Fw: Re: heap_force_common in contrib/pg_surgery/heap_surgery.c has an off by one stack buffer overflow
@ 2026-06-04 07:31 Michael Paquier <[email protected]>
parent: surya poondla <[email protected]>
1 sibling, 1 reply; 10+ messages in thread
From: Michael Paquier @ 2026-06-04 07:31 UTC (permalink / raw)
To: surya poondla <[email protected]>; +Cc: [email protected] <[email protected]>; pgsql-bugs
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.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Fw: Re: heap_force_common in contrib/pg_surgery/heap_surgery.c has an off by one stack buffer overflow
@ 2026-06-04 09:12 Ashutosh Sharma <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Ashutosh Sharma @ 2026-06-04 09:12 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: surya poondla <[email protected]>; [email protected] <[email protected]>; pgsql-bugs
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.
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Fw: Re: heap_force_common in contrib/pg_surgery/heap_surgery.c has an off by one stack buffer overflow
@ 2026-06-04 23:17 Michael Paquier <[email protected]>
parent: Ashutosh Sharma <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Michael Paquier @ 2026-06-04 23:17 UTC (permalink / raw)
To: Ashutosh Sharma <[email protected]>; +Cc: surya poondla <[email protected]>; [email protected] <[email protected]>; pgsql-bugs
On Thu, Jun 04, 2026 at 02:42:23PM +0530, Ashutosh Sharma wrote:
> 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.
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
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Fw: Re: heap_force_common in contrib/pg_surgery/heap_surgery.c has an off by one stack buffer overflow
@ 2026-06-05 06:57 Michael Paquier <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Michael Paquier @ 2026-06-05 06:57 UTC (permalink / raw)
To: Ashutosh Sharma <[email protected]>; +Cc: surya poondla <[email protected]>; [email protected] <[email protected]>; pgsql-bugs
On Fri, Jun 05, 2026 at 08:17:15AM +0900, Michael Paquier wrote:
> 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. :)
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);
Attachments:
[text/plain] surgery-oneoff.patch (784B, 2-surgery-oneoff.patch)
download | inline diff:
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);
[application/pgp-signature] signature.asc (833B, 3-signature.asc)
download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Fw: Re: heap_force_common in contrib/pg_surgery/heap_surgery.c has an off by one stack buffer overflow
@ 2026-06-05 08:00 Ashutosh Sharma <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Ashutosh Sharma @ 2026-06-05 08:00 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: surya poondla <[email protected]>; [email protected] <[email protected]>; pgsql-bugs
Hi Michael,
Thanks for the patch.
On Fri, Jun 5, 2026 at 12:27 PM Michael Paquier <[email protected]> wrote:
>
> On Fri, Jun 05, 2026 at 08:17:15AM +0900, Michael Paquier wrote:
> > 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. :)
>
> 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.
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Fw: Re: heap_force_common in contrib/pg_surgery/heap_surgery.c has an off by one stack buffer overflow
@ 2026-06-06 00:17 Michael Paquier <[email protected]>
parent: Ashutosh Sharma <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Michael Paquier @ 2026-06-06 00:17 UTC (permalink / raw)
To: Ashutosh Sharma <[email protected]>; +Cc: surya poondla <[email protected]>; [email protected] <[email protected]>; pgsql-bugs
On Fri, Jun 05, 2026 at 01:30:42PM +0530, Ashutosh Sharma wrote:
> Since "offno" is already 1-based, there doesn't seem to be a need to
> subtract 1 from it and adjust the comparison accordingly.
Sure. Changed as you have suggested here, and backpatched down to
v14.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, 2-signature.asc)
download
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: Fw: Re: heap_force_common in contrib/pg_surgery/heap_surgery.c has an off by one stack buffer overflow
@ 2026-06-06 01:08 surya poondla <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 0 replies; 10+ messages in thread
From: surya poondla @ 2026-06-06 01:08 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Ashutosh Sharma <[email protected]>; [email protected] <[email protected]>; pgsql-bugs
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
^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2026-06-06 01:08 UTC | newest]
Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-06-03 16:22 Fw: Re: heap_force_common in contrib/pg_surgery/heap_surgery.c has an off by one stack buffer overflow [email protected] <[email protected]>
2026-06-03 22:31 ` surya poondla <[email protected]>
2026-06-04 02:47 ` 王跃林 <[email protected]>
2026-06-04 07:31 ` Michael Paquier <[email protected]>
2026-06-04 09:12 ` Ashutosh Sharma <[email protected]>
2026-06-04 23:17 ` Michael Paquier <[email protected]>
2026-06-05 06:57 ` Michael Paquier <[email protected]>
2026-06-05 08:00 ` Ashutosh Sharma <[email protected]>
2026-06-06 00:17 ` Michael Paquier <[email protected]>
2026-06-06 01:08 ` surya poondla <[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