From: Michael Paquier <michael@paquier.xyz>
To: Postgres bugs <pgsql-bugs@lists.postgresql.org>
Cc: Amit Kapila <amit.kapila16@gmail.com>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Ethan Mertz <ethan.mertz@gmail.com>
Subject: Use-after-free in reorderbuffer.c for INSERT ON CONFLICT
Date: Thu, 31 Jul 2025 15:43:52 +0900
Message-ID: <aIsQqDZ7x4LAQ6u1@paquier.xyz> (raw)
Hi all,
Ethan Mertz (colleague, in CC) has found a bug in reorderbuffer.c when
processing a REORDER_BUFFER_CHANGE_INTERNAL_SPEC_CONFIRM change, based
on the data gathered from a customer issue. The bug is a
use-after-free, causing random crashes, that can be summarized like
that:
1) "specinsert" is saved as a new change to process:
change = specinsert;
change->action = REORDER_BUFFER_CHANGE_INSERT;
2) A bit later on the change and specinsert are freed when we are done
with the speculative insert:
change_done:
/*
* If speculative insertion was confirmed, the record
* isn't needed anymore.
*/
if (specinsert != NULL)
{
ReorderBufferFreeChange(rb, specinsert, true);
specinsert = NULL;
}
3) Finally, a couple of lines down, we then do the following things,
and attempt to use a reference to change->lsn that has been freed:
#define CHANGES_THRESHOLD 100
if (++changes_count >= CHANGES_THRESHOLD)
{
rb->update_progress_txn(rb, txn, change->lsn);
changes_count = 0;
}
This issue has been introduced in 8c58624df462, down to REL_16_STABLE.
Committer of the related change is added in CC (aka Amit K.).
Sawada-san (also in CC) has come up with an imaginative trick to
easily reproduce the issue, as of:
--- a/src/backend/replication/logical/reorderbuffer.c+++ b/src/backend/replication/logical/reorderbuffer.c@@ -2595,7 +2595,7 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
* overhead, but testing showed there is no noticeable overhead if
* we do it after every ~100 changes.
*/
-#define CHANGES_THRESHOLD 100+#define CHANGES_THRESHOLD 1
if (++changes_count >= CHANGES_THRESHOLD)
{
When running the test "ddl" from test_decoding, an instance running
valgrind comes up with that, pointing immediately at the problem:
==28821== Invalid read of size 8
==28821== at 0x222B2D8: ReorderBufferProcessTXN (reorderbuffer.c:2602)
==28821== by 0x222C6D6: ReorderBufferReplay (reorderbuffer.c:2864)
==28821== by 0x222C754: ReorderBufferCommit (reorderbuffer.c:2888)
==28821== by 0x21EDA48: DecodeCommit (decode.c:743)
==28821== by 0x21EA54C: xact_decode (decode.c:242)
==28821== by 0x21E98AA: LogicalDecodingProcessRecord (decode.c:116)
==28821== by 0x22093A4: pg_logical_slot_get_changes_guts (logicalfuncs.c:266)
==28821== by 0x22097BC: pg_logical_slot_get_changes (logicalfuncs.c:333)
==28821== by 0x1AF25D3: ExecMakeTableFunctionResult (execSRF.c:234)
==28821== by 0x1B65606: FunctionNext (nodeFunctionscan.c:94)
==28821== by 0x1AF7306: ExecScanFetch (execScan.h:126)
==28821== by 0x1AF748D: ExecScanExtended (execScan.h:187)
==28821== Address 0x107fae08 is 7,656 bytes inside a recently re-allocated block of size 8,192 alloc'd
==28821== at 0x4844818: malloc (vg_replace_malloc.c:446)
==28821== by 0x2AD3573: SlabAllocFromNewBlock (slab.c:565)
==28821== by 0x2AD3EF7: SlabAlloc (slab.c:656)
==28821== by 0x2AC5282: MemoryContextAlloc (mcxt.c:1237)
==28821== by 0x221E17A: ReorderBufferAllocChange (reorderbuffer.c:511)
==28821== by 0x222F64B: ReorderBufferAddNewTupleCids (reorderbuffer.c:3444)
==28821== by 0x2248439: SnapBuildProcessNewCid (snapbuild.c:700)
==28821== by 0x21EB9D8: heap2_decode (decode.c:437)
==28821== by 0x21E98AA: LogicalDecodingProcessRecord (decode.c:116)
==28821== by 0x22093A4: pg_logical_slot_get_changes_guts (logicalfuncs.c:266)
==28821== by 0x22097BC: pg_logical_slot_get_changes (logicalfuncs.c:333)
==28821== by 0x1AF25D3: ExecMakeTableFunctionResult (execSRF.c:234)
A simple solution suggested by Ethan would be to use the "prev_lsn"
guessed from the change at the beginning of the loop, rather than the
problematic change->lsn. But that does not seem completely right to
me because we can switch to "specinsert" as the change to process,
hence wouldn't we want to call update_progress_txn() based on the LSN
of the actual change we are looking at? All that leads me to the
attached. Note that I am not the most familiar with this area of the
code, so please take my arguments with a grain of salt.
Comments and thoughts are welcome.
--
Michael
Attachments:
[text/x-diff] 0001-Fix-use-after-free-in-reorderbuffer.c-with-ON-CONFLI.patch (1.7K, ../aIsQqDZ7x4LAQ6u1@paquier.xyz/2-0001-Fix-use-after-free-in-reorderbuffer.c-with-ON-CONFLI.patch)
download | inline diff:
From aa0226ea2b75e7523fec4ff8a6d00e46ebdbf08e Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Thu, 31 Jul 2025 15:31:36 +0900
Subject: [PATCH] Fix use-after-free in reorderbuffer.c with ON CONFLICT
changes
---
src/backend/replication/logical/reorderbuffer.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.cindex 5febd154b6ba..09256f32478f 100644--- a/src/backend/replication/logical/reorderbuffer.c+++ b/src/backend/replication/logical/reorderbuffer.c@@ -2267,6 +2267,7 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
{
Relation relation = NULL;
Oid reloid;
+ XLogRecPtr change_lsn = InvalidXLogRecPtr;
CHECK_FOR_INTERRUPTS();
@@ -2292,6 +2293,7 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
Assert(prev_lsn == InvalidXLogRecPtr || prev_lsn <= change->lsn);
prev_lsn = change->lsn;
+ change_lsn = change->lsn;
/*
* Set the current xid to detect concurrent aborts. This is
@@ -2317,6 +2319,7 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
elog(ERROR, "invalid ordering of speculative insertion changes");
Assert(specinsert->data.tp.oldtuple == NULL);
change = specinsert;
+ change_lsn = change->lsn;
change->action = REORDER_BUFFER_CHANGE_INSERT;
/* intentionally fall through */
@@ -2599,7 +2602,7 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
if (++changes_count >= CHANGES_THRESHOLD)
{
- rb->update_progress_txn(rb, txn, change->lsn);+ rb->update_progress_txn(rb, txn, change_lsn);
changes_count = 0;
}
}
--
2.50.0
[application/pgp-signature] signature.asc (832B, ../aIsQqDZ7x4LAQ6u1@paquier.xyz/3-signature.asc)
download
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: pgsql-bugs@postgresql.org
Cc: michael@paquier.xyz, pgsql-bugs@lists.postgresql.org, amit.kapila16@gmail.com, sawada.mshk@gmail.com, ethan.mertz@gmail.com
Subject: Re: Use-after-free in reorderbuffer.c for INSERT ON CONFLICT
In-Reply-To: <aIsQqDZ7x4LAQ6u1@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox