Skip to content

Commit 78b7aaa

Browse files
tglsfdctglsfdc
committed
Clean up some lack-of-STRICT issues in the core code, too.
A scan for missed proisstrict markings in the core code turned up these functions: brin_summarize_new_values pg_stat_reset_single_table_counters pg_stat_reset_single_function_counters pg_create_logical_replication_slot pg_create_physical_replication_slot pg_drop_replication_slot The first three of these take OID, so a null argument will normally look like a zero to them, resulting in "ERROR: could not open relation with OID 0" for brin_summarize_new_values, and no action for the pg_stat_reset_XXX functions. The other three will dump core on a null argument, though this is mitigated by the fact that they won't do so until after checking that the caller is superuser or has rolreplication privilege. In addition, the pg_logical_slot_get/peek[_binary]_changes family was intentionally marked nonstrict, but failed to make nullness checks on all the arguments; so again a null-pointer-dereference crash is possible but only for superusers and rolreplication users. Add the missing ARGISNULL checks to the latter functions, and mark the former functions as strict in pg_proc. Make that change in the back branches too, even though we can't force initdb there, just so that installations initdb'd in future won't have the issue. Since none of these bugs rise to the level of security issues (and indeed the pg_stat_reset_XXX functions hardly misbehave at all), it seems sufficient to do this. In addition, fix some order-of-operations oddities in the slot_get_changes family, mostly cosmetic, but not the part that moves the function's last few operations into the PG_TRY block. As it stood, there was significant risk for an error to exit without clearing historical information from the system caches. The slot_get_changes bugs go back to 9.4 where that code was introduced. Back-patch appropriate subsets of the pg_proc changes into all active branches, as well.
1 parent acbdda4 commit 78b7aaa

File tree

2 files changed

+45
-47
lines changed

2 files changed

+45
-47
lines changed

‎src/backend/replication/logical/logicalfuncs.c

Lines changed: 40 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -274,25 +274,31 @@ logical_read_local_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr,
274274
static Datum
275275
pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool binary)
276276
{
277-
Name name = PG_GETARG_NAME(0);
277+
Name name;
278278
XLogRecPtr upto_lsn;
279279
int32 upto_nchanges;
280-
281280
ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
282281
MemoryContext per_query_ctx;
283282
MemoryContext oldcontext;
284-
285283
XLogRecPtr end_of_wal;
286284
XLogRecPtr startptr;
287-
288285
LogicalDecodingContext *ctx;
289-
290286
ResourceOwner old_resowner = CurrentResourceOwner;
291287
ArrayType *arr;
292288
Size ndim;
293289
List *options = NIL;
294290
DecodingOutputState *p;
295291

292+
check_permissions();
293+
294+
CheckLogicalDecodingRequirements();
295+
296+
if (PG_ARGISNULL(0))
297+
ereport(ERROR,
298+
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
299+
errmsg("slot name must not be null")));
300+
name = PG_GETARG_NAME(0);
301+
296302
if (PG_ARGISNULL(1))
297303
upto_lsn = InvalidXLogRecPtr;
298304
else
@@ -303,6 +309,12 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
303309
else
304310
upto_nchanges = PG_GETARG_INT32(2);
305311

312+
if (PG_ARGISNULL(3))
313+
ereport(ERROR,
314+
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
315+
errmsg("options array must not be null")));
316+
arr = PG_GETARG_ARRAYTYPE_P(3);
317+
306318
/* check to see if caller supports us returning a tuplestore */
307319
if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo))
308320
ereport(ERROR,
@@ -322,16 +334,11 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
322334
if (get_call_result_type(fcinfo, NULL, &p->tupdesc) != TYPEFUNC_COMPOSITE)
323335
elog(ERROR, "return type must be a row type");
324336

325-
check_permissions();
326-
327-
CheckLogicalDecodingRequirements();
328-
329-
arr = PG_GETARG_ARRAYTYPE_P(3);
330-
ndim = ARR_NDIM(arr);
331-
332337
per_query_ctx = rsinfo->econtext->ecxt_per_query_memory;
333338
oldcontext = MemoryContextSwitchTo(per_query_ctx);
334339

340+
/* Deconstruct options array */
341+
ndim = ARR_NDIM(arr);
335342
if (ndim > 1)
336343
{
337344
ereport(ERROR,
@@ -380,7 +387,6 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
380387
else
381388
end_of_wal = GetXLogReplayRecPtr(NULL);
382389

383-
CheckLogicalDecodingRequirements();
384390
ReplicationSlotAcquire(NameStr(*name));
385391

386392
PG_TRY();
@@ -442,6 +448,23 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
442448
break;
443449
CHECK_FOR_INTERRUPTS();
444450
}
451+
452+
tuplestore_donestoring(tupstore);
453+
454+
CurrentResourceOwner = old_resowner;
455+
456+
/*
457+
* Next time, start where we left off. (Hunting things, the family
458+
* business..)
459+
*/
460+
if (ctx->reader->EndRecPtr != InvalidXLogRecPtr && confirm)
461+
LogicalConfirmReceivedLocation(ctx->reader->EndRecPtr);
462+
463+
/* free context, call shutdown callback */
464+
FreeDecodingContext(ctx);
465+
466+
ReplicationSlotRelease();
467+
InvalidateSystemCaches();
445468
}
446469
PG_CATCH();
447470
{
@@ -452,23 +475,6 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
452475
}
453476
PG_END_TRY();
454477

455-
tuplestore_donestoring(tupstore);
456-
457-
CurrentResourceOwner = old_resowner;
458-
459-
/*
460-
* Next time, start where we left off. (Hunting things, the family
461-
* business..)
462-
*/
463-
if (ctx->reader->EndRecPtr != InvalidXLogRecPtr && confirm)
464-
LogicalConfirmReceivedLocation(ctx->reader->EndRecPtr);
465-
466-
/* free context, call shutdown callback */
467-
FreeDecodingContext(ctx);
468-
469-
ReplicationSlotRelease();
470-
InvalidateSystemCaches();
471-
472478
return (Datum) 0;
473479
}
474480

@@ -478,9 +484,7 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
478484
Datum
479485
pg_logical_slot_get_changes(PG_FUNCTION_ARGS)
480486
{
481-
Datum ret = pg_logical_slot_get_changes_guts(fcinfo, true, false);
482-
483-
return ret;
487+
return pg_logical_slot_get_changes_guts(fcinfo, true, false);
484488
}
485489

486490
/*
@@ -489,9 +493,7 @@ pg_logical_slot_get_changes(PG_FUNCTION_ARGS)
489493
Datum
490494
pg_logical_slot_peek_changes(PG_FUNCTION_ARGS)
491495
{
492-
Datum ret = pg_logical_slot_get_changes_guts(fcinfo, false, false);
493-
494-
return ret;
496+
return pg_logical_slot_get_changes_guts(fcinfo, false, false);
495497
}
496498

497499
/*
@@ -500,9 +502,7 @@ pg_logical_slot_peek_changes(PG_FUNCTION_ARGS)
500502
Datum
501503
pg_logical_slot_get_binary_changes(PG_FUNCTION_ARGS)
502504
{
503-
Datum ret = pg_logical_slot_get_changes_guts(fcinfo, true, true);
504-
505-
return ret;
505+
return pg_logical_slot_get_changes_guts(fcinfo, true, true);
506506
}
507507

508508
/*
@@ -511,7 +511,5 @@ pg_logical_slot_get_binary_changes(PG_FUNCTION_ARGS)
511511
Datum
512512
pg_logical_slot_peek_binary_changes(PG_FUNCTION_ARGS)
513513
{
514-
Datum ret = pg_logical_slot_get_changes_guts(fcinfo, false, true);
515-
516-
return ret;
514+
return pg_logical_slot_get_changes_guts(fcinfo, false, true);
517515
}

‎src/include/catalog/pg_proc.h

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2812,9 +2812,9 @@ DATA(insert OID = 2274 ( pg_stat_reset PGNSP PGUID 12 1 0 0 0 f f f f f f v
28122812
DESCR("statistics: reset collected statistics for current database");
28132813
DATA(insert OID = 3775 ( pg_stat_reset_shared PGNSP PGUID 12 1 0 0 0 f f f f t f v 1 0 2278 "25" _null_ _null_ _null_ _null_ pg_stat_reset_shared _null_ _null_ _null_ ));
28142814
DESCR("statistics: reset collected statistics shared across the cluster");
2815-
DATA(insert OID = 3776 ( pg_stat_reset_single_table_counters PGNSP PGUID 12 1 0 0 0 f f f f f f v 1 0 2278 "26" _null_ _null_ _null_ _null_ pg_stat_reset_single_table_counters _null_ _null_ _null_ ));
2815+
DATA(insert OID = 3776 ( pg_stat_reset_single_table_counters PGNSP PGUID 12 1 0 0 0 f f f f t f v 1 0 2278 "26" _null_ _null_ _null_ _null_ pg_stat_reset_single_table_counters _null_ _null_ _null_ ));
28162816
DESCR("statistics: reset collected statistics for a single table or index in the current database");
2817-
DATA(insert OID = 3777 ( pg_stat_reset_single_function_counters PGNSP PGUID 12 1 0 0 0 f f f f f f v 1 0 2278 "26" _null_ _null_ _null_ _null_ pg_stat_reset_single_function_counters _null_ _null_ _null_ ));
2817+
DATA(insert OID = 3777 ( pg_stat_reset_single_function_counters PGNSP PGUID 12 1 0 0 0 f f f f t f v 1 0 2278 "26" _null_ _null_ _null_ _null_ pg_stat_reset_single_function_counters _null_ _null_ _null_ ));
28182818
DESCR("statistics: reset collected statistics for a single function in the current database");
28192819

28202820
DATA(insert OID = 3163 ( pg_trigger_depth PGNSP PGUID 12 1 0 0 0 f f f f t f s 0 0 23 "" _null_ _null_ _null_ _null_ pg_trigger_depth _null_ _null_ _null_ ));
@@ -4956,13 +4956,13 @@ DATA(insert OID = 3473 ( spg_range_quad_leaf_consistent PGNSP PGUID 12 1 0 0 0
49564956
DESCR("SP-GiST support for quad tree over range");
49574957

49584958
/* replication slots */
4959-
DATA(insert OID = 3779 ( pg_create_physical_replication_slot PGNSP PGUID 12 1 0 0 0 f f f f f f v 1 0 2249 "19" "{19,19,3220}" "{i,o,o}" "{slot_name,slot_name,xlog_position}" _null_ pg_create_physical_replication_slot _null_ _null_ _null_ ));
4959+
DATA(insert OID = 3779 ( pg_create_physical_replication_slot PGNSP PGUID 12 1 0 0 0 f f f f t f v 1 0 2249 "19" "{19,19,3220}" "{i,o,o}" "{slot_name,slot_name,xlog_position}" _null_ pg_create_physical_replication_slot _null_ _null_ _null_ ));
49604960
DESCR("create a physical replication slot");
4961-
DATA(insert OID = 3780 ( pg_drop_replication_slot PGNSP PGUID 12 1 0 0 0 f f f f f f v 1 0 2278 "19" _null_ _null_ _null_ _null_ pg_drop_replication_slot _null_ _null_ _null_ ));
4961+
DATA(insert OID = 3780 ( pg_drop_replication_slot PGNSP PGUID 12 1 0 0 0 f f f f t f v 1 0 2278 "19" _null_ _null_ _null_ _null_ pg_drop_replication_slot _null_ _null_ _null_ ));
49624962
DESCR("drop a replication slot");
49634963
DATA(insert OID = 3781 ( pg_get_replication_slots PGNSP PGUID 12 1 10 0 0 f f f f f t s 0 0 2249 "" "{19,19,25,26,16,28,28,3220}" "{o,o,o,o,o,o,o,o}" "{slot_name,plugin,slot_type,datoid,active,xmin,catalog_xmin,restart_lsn}" _null_ pg_get_replication_slots _null_ _null_ _null_ ));
49644964
DESCR("information about replication slots currently in use");
4965-
DATA(insert OID = 3786 ( pg_create_logical_replication_slot PGNSP PGUID 12 1 0 0 0 f f f f f f v 2 0 2249 "19 19" "{19,19,25,3220}" "{i,i,o,o}" "{slot_name,plugin,slot_name,xlog_position}" _null_ pg_create_logical_replication_slot _null_ _null_ _null_ ));
4965+
DATA(insert OID = 3786 ( pg_create_logical_replication_slot PGNSP PGUID 12 1 0 0 0 f f f f t f v 2 0 2249 "19 19" "{19,19,25,3220}" "{i,i,o,o}" "{slot_name,plugin,slot_name,xlog_position}" _null_ pg_create_logical_replication_slot _null_ _null_ _null_ ));
49664966
DESCR("set up a logical replication slot");
49674967
DATA(insert OID = 3782 ( pg_logical_slot_get_changes PGNSP PGUID 12 1000 1000 25 0 f f f f f t v 4 0 2249 "19 3220 23 1009" "{19,3220,23,1009,3220,28,25}" "{i,i,i,v,o,o,o}" "{slot_name,upto_lsn,upto_nchanges,options,location,xid,data}" _null_ pg_logical_slot_get_changes _null_ _null_ _null_ ));
49684968
DESCR("get changes from replication slot");

0 commit comments

Comments
 (0)