Fix access-to-already-freed-memory issue in pgoutput.

While pgoutput caches relation synchronization information in
RelationSyncCache that resides in CacheMemoryContext, each entry's
information (such as row filter expressions and column lists) is
stored in the entry's private memory context (entry_cxt in
RelationSyncEntry), which is a descendant memory context of the
decoding context. If a logical decoding invoked via SQL functions like
pg_logical_slot_get_binary_changes fails with an error, subsequent
logical decoding executions could access already-freed memory of the
entry's cache, resulting in a crash.

With this change, it's ensured that RelationSyncCache is cleaned up
even in error cases by using a memory context reset callback function.

Backpatch to 15, where entry_cxt was introduced for column filtering
and row filtering.

While the backbranches v13 and v14 have a similar issue where
RelationSyncCache persists even after an error when pgoutput is used
via SQL API, we decided not to backport this fix. This decision was
made because v13 is approaching its final minor release, and we won't
have an chance to fix any new issues that might arise. Additionally,
since using pgoutput via SQL API is not a common use case, the risk
outwights the benefit. If we receive bug reports, we can consider
backporting the fixes then.

Author: vignesh C <vignesh21@gmail.com>
Co-authored-by: Masahiko Sawada <sawada.mshk@gmail.com>
Reviewed-by: Zhijie Hou <houzj.fnst@fujitsu.com>
Reviewed-by: Euler Taveira <euler@eulerto.com>
Discussion: https://postgr.es/m/CALDaNm0x-aCehgt8Bevs2cm=uhmwS28MvbYq1=s2Ekf0aDPkOA@mail.gmail.com
Backpatch-through: 15

diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index 99518c6b6dd1e8ed8e5325cfb37d426f4d4084ae..972a88e7c183698fa1d3f7c1bb29defd067997e0 100644 (file)
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -229,6 +229,7 @@ static bool get_schema_sent_in_streamed_txn(RelationSyncEntry *entry,
                                            TransactionId xid);
 static void init_tuple_slot(PGOutputData *data, Relation relation,
                            RelationSyncEntry *entry);
+static void pgoutput_memory_context_reset(void *arg);
 
 /* row filter routines */
 static EState *create_estate_for_relation(Relation rel);
@@ -419,11 +420,18 @@ parse_output_parameters(List *options, PGOutputData *data)
 }
 
 /*
- * Callback of PGOutputData->context in charge of cleaning pubctx.
+ * Memory context reset callback of PGOutputData->context.
  */
 static void
-pgoutput_pubctx_reset_callback(void *arg)
+pgoutput_memory_context_reset(void *arg)
 {
+   if (RelationSyncCache)
+   {
+       hash_destroy(RelationSyncCache);
+       RelationSyncCache = NULL;
+   }
+
+   /* Better safe than sorry */
    pubctx = NULL;
 }
 
@@ -452,8 +460,12 @@ pgoutput_startup(LogicalDecodingContext *ctx, OutputPluginOptions *opt,
                                   "logical replication publication list context",
                                   ALLOCSET_SMALL_SIZES);
 
+   /*
+    * Ensure to cleanup RelationSyncCache even when logical decoding invoked
+    * via SQL interface ends up with an error.
+    */
    mcallback = palloc0(sizeof(MemoryContextCallback));
-   mcallback->func = pgoutput_pubctx_reset_callback;
+   mcallback->func = pgoutput_memory_context_reset;
    MemoryContextRegisterResetCallback(ctx->context, mcallback);
 
    ctx->output_plugin_private = data;
@@ -1729,14 +1741,7 @@ pgoutput_origin_filter(LogicalDecodingContext *ctx,
 static void
 pgoutput_shutdown(LogicalDecodingContext *ctx)
 {
-   if (RelationSyncCache)
-   {
-       hash_destroy(RelationSyncCache);
-       RelationSyncCache = NULL;
-   }
-
-   /* Better safe than sorry */
-   pubctx = NULL;
+   pgoutput_memory_context_reset(NULL);
 }
 
 /*