pg.ddx.io pgsql-committers@postgresql.org mailing list archivehelp / color / mirror / Atom feed
pgsql: Fix use-after-free with INSERT ON CONFLICT changes in reorderbuf 4+ messages / 1 participants [nested] [flat]
* pgsql: Fix use-after-free with INSERT ON CONFLICT changes in reorderbuf @ 2025-08-02 08:09 Michael Paquier <michael@paquier.xyz> 0 siblings, 0 replies; 4+ messages in thread From: Michael Paquier @ 2025-08-02 08:09 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org 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 Branch ------ REL_18_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/11de339aadf828ef3d67ba220ece3cea211ca01c Modified Files -------------- src/backend/replication/logical/reorderbuffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) ^ permalink raw reply [nested|flat] 4+ messages in thread
* pgsql: Fix use-after-free with INSERT ON CONFLICT changes in reorderbuf @ 2025-08-02 08:09 Michael Paquier <michael@paquier.xyz> 0 siblings, 0 replies; 4+ messages in thread From: Michael Paquier @ 2025-08-02 08:09 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org 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 Branch ------ REL_16_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/ec96e88122c88866b1143c75048f28111b4eb344 Modified Files -------------- src/backend/replication/logical/reorderbuffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) ^ permalink raw reply [nested|flat] 4+ messages in thread
* pgsql: Fix use-after-free with INSERT ON CONFLICT changes in reorderbuf @ 2025-08-02 08:09 Michael Paquier <michael@paquier.xyz> 0 siblings, 0 replies; 4+ messages in thread From: Michael Paquier @ 2025-08-02 08:09 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org 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 Branch ------ REL_17_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/9e0b4b1ab5ef3f34013375b16bb6689e33f67aa5 Modified Files -------------- src/backend/replication/logical/reorderbuffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) ^ permalink raw reply [nested|flat] 4+ messages in thread
* pgsql: Fix use-after-free with INSERT ON CONFLICT changes in reorderbuf @ 2025-08-02 08:09 Michael Paquier <michael@paquier.xyz> 0 siblings, 0 replies; 4+ messages in thread From: Michael Paquier @ 2025-08-02 08:09 UTC (permalink / raw) To: pgsql-committers@lists.postgresql.org 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 Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/3b3fa949009393541e552b8ae42cc2b03be25549 Modified Files -------------- src/backend/replication/logical/reorderbuffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2025-08-02 08:09 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2025-08-02 08:09 pgsql: Fix use-after-free with INSERT ON CONFLICT changes in reorderbuf Michael Paquier <michael@paquier.xyz> 2025-08-02 08:09 pgsql: Fix use-after-free with INSERT ON CONFLICT changes in reorderbuf Michael Paquier <michael@paquier.xyz> 2025-08-02 08:09 pgsql: Fix use-after-free with INSERT ON CONFLICT changes in reorderbuf Michael Paquier <michael@paquier.xyz> 2025-08-02 08:09 pgsql: Fix use-after-free with INSERT ON CONFLICT changes in reorderbuf Michael Paquier <michael@paquier.xyz>
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