postgres.git / summary / log / commit / refs

commit    9e0b4b1ab5ef3f34013375b16bb6689e33f67aa5
Author:   Michael Paquier <michael@paquier.xyz>
Date:     Sat Aug 02 08:08:50 2025 +0000
First released in: 16.10, 17.6, 18.0

    Fix use-after-free with INSERT ON CONFLICT changes in reorderbuffer.c
    
    In ReorderBufferProcessTXN(), used to send the data of a transaction to
    an output plugin, INSERT ON CONFLICT changes (INTERNAL_SPEC_INSERT) are
    delayed until a confirmation record arrives (INTERNAL_SPEC_CONFIRM),
    updating the change being processed.
    
    8c58624df462 has added an extra step after processing a change to update
    the progress of the transaction, by calling the callback
    update_progress_txn() based on the LSN stored in a change after a
    threshold of CHANGES_THRESHOLD (100) is reached.  This logic has missed
    the fact that for an INSERT ON CONFLICT change the data is freed once
    processed, hence update_progress_txn() could be called pointing to a LSN
    value that's already been freed.  This could result in random crashes,
    depending on the workload.
    
    Per discussion, this issue is fixed by reusing in update_progress_txn()
    the LSN from the change processed found at the beginning of the loop,
    meaning that for a INTERNAL_SPEC_CONFIRM change the progress is updated
    using the LSN of the INTERNAL_SPEC_CONFIRM change, and not the LSN from
    its INTERNAL_SPEC_INSERT change.  This is actually more correct, as we
    want to update the progress to point to the INTERNAL_SPEC_CONFIRM
    change.
    
    Masahiko Sawada has found a nice trick to reproduce the issue: hardcode
    CHANGES_THRESHOLD at 1 and run test_decoding (test "ddl" being enough)
    on an instance running valgrind.  The bug has been analyzed by Ethan
    Mertz, who also originally suggested the solution used in this patch.
    
    Issue introduced by 8c58624df462, so backpatch down to v16.
    
    Author: Ethan Mertz <ethan.mertz@gmail.com>
    Co-authored-by: Michael Paquier <michael@paquier.xyz>
    Reviewed-by: Amit Kapila <amit.kapila16@gmail.com>
    Reviewed-by: Masahiko Sawada <sawada.mshk@gmail.com>
    Discussion: https://postgr.es/m/aIsQqDZ7x4LAQ6u1@paquier.xyz
    Backpatch-through: 16


 src/backend/replication/logical/reorderbuffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index d85a9f2bb20..4bd1f7af061 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -2516,7 +2516,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, prev_lsn);
 				changes_count = 0;
 			}
 		}


[parent: 39ff056365e4]