Skip to content

Commit aa7e04c

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 605afec commit aa7e04c

File tree

2 files changed

+46
-48
lines changed

2 files changed

+46
-48
lines changed

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

Lines changed: 40 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -276,25 +276,31 @@ logical_read_local_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr,
276276
static Datum
277277
pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool binary)
278278
{
279-
Name name = PG_GETARG_NAME(0);
279+
Name name;
280280
XLogRecPtr upto_lsn;
281281
int32 upto_nchanges;
282-
283282
ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
284283
MemoryContext per_query_ctx;
285284
MemoryContext oldcontext;
286-
287285
XLogRecPtr end_of_wal;
288286
XLogRecPtr startptr;
289-
290287
LogicalDecodingContext *ctx;
291-
292288
ResourceOwner old_resowner = CurrentResourceOwner;
293289
ArrayType *arr;
294290
Size ndim;
295291
List *options = NIL;
296292
DecodingOutputState *p;
297293

294+
check_permissions();
295+
296+
CheckLogicalDecodingRequirements();
297+
298+
if (PG_ARGISNULL(0))
299+
ereport(ERROR,
300+
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
301+
errmsg("slot name must not be null")));
302+
name = PG_GETARG_NAME(0);
303+
298304
if (PG_ARGISNULL(1))
299305
upto_lsn = InvalidXLogRecPtr;
300306
else
@@ -305,6 +311,12 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
305311
else
306312
upto_nchanges = PG_GETARG_INT32(2);
307313

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

327-
check_permissions();
328-
329-
CheckLogicalDecodingRequirements();
330-
331-
arr = PG_GETARG_ARRAYTYPE_P(3);
332-
ndim = ARR_NDIM(arr);
333-
334339
per_query_ctx = rsinfo->econtext->ecxt_per_query_memory;
335340
oldcontext = MemoryContextSwitchTo(per_query_ctx);
336341

342+
/* Deconstruct options array */
343+
ndim = ARR_NDIM(arr);
337344
if (ndim > 1)
338345
{
339346
ereport(ERROR,
@@ -382,7 +389,6 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
382389
else
383390
end_of_wal = GetXLogReplayRecPtr(NULL);
384391

385-
CheckLogicalDecodingRequirements();
386392
ReplicationSlotAcquire(NameStr(*name));
387393

388394
PG_TRY();
@@ -444,6 +450,23 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
444450
break;
445451
CHECK_FOR_INTERRUPTS();
446452
}
453+
454+
tuplestore_donestoring(tupstore);
455+
456+
CurrentResourceOwner = old_resowner;
457+
458+
/*
459+
* Next time, start where we left off. (Hunting things, the family
460+
* business..)
461+
*/
462+
if (ctx->reader->EndRecPtr != InvalidXLogRecPtr && confirm)
463+
LogicalConfirmReceivedLocation(ctx->reader->EndRecPtr);
464+
465+
/* free context, call shutdown callback */
466+
FreeDecodingContext(ctx);
467+
468+
ReplicationSlotRelease();
469+
InvalidateSystemCaches();
447470
}
448471
PG_CATCH();
449472
{
@@ -454,23 +477,6 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
454477
}
455478
PG_END_TRY();
456479

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

@@ -480,9 +486,7 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
480486
Datum
481487
pg_logical_slot_get_changes(PG_FUNCTION_ARGS)
482488
{
483-
Datum ret = pg_logical_slot_get_changes_guts(fcinfo, true, false);
484-
485-
return ret;
489+
return pg_logical_slot_get_changes_guts(fcinfo, true, false);
486490
}
487491

488492
/*
@@ -491,9 +495,7 @@ pg_logical_slot_get_changes(PG_FUNCTION_ARGS)
491495
Datum
492496
pg_logical_slot_peek_changes(PG_FUNCTION_ARGS)
493497
{
494-
Datum ret = pg_logical_slot_get_changes_guts(fcinfo, false, false);
495-
496-
return ret;
498+
return pg_logical_slot_get_changes_guts(fcinfo, false, false);
497499
}
498500

499501
/*
@@ -502,9 +504,7 @@ pg_logical_slot_peek_changes(PG_FUNCTION_ARGS)
502504
Datum
503505
pg_logical_slot_get_binary_changes(PG_FUNCTION_ARGS)
504506
{
505-
Datum ret = pg_logical_slot_get_changes_guts(fcinfo, true, true);
506-
507-
return ret;
507+
return pg_logical_slot_get_changes_guts(fcinfo, true, true);
508508
}
509509

510510
/*
@@ -513,7 +513,5 @@ pg_logical_slot_get_binary_changes(PG_FUNCTION_ARGS)
513513
Datum
514514
pg_logical_slot_peek_binary_changes(PG_FUNCTION_ARGS)
515515
{
516-
Datum ret = pg_logical_slot_get_changes_guts(fcinfo, false, true);
517-
518-
return ret;
516+
return pg_logical_slot_get_changes_guts(fcinfo, false, true);
519517
}

‎src/include/catalog/pg_proc.h

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -602,7 +602,7 @@ DATA(insert OID = 3800 ( brincostestimate PGNSP PGUID 12 1 0 0 0 f f f f t f v
602602
DESCR("brin(internal)");
603603
DATA(insert OID = 3801 ( brinoptions PGNSP PGUID 12 1 0 0 0 f f f f t f s 2 0 17 "1009 16" _null_ _null_ _null_ _null_ _null_ brinoptions _null_ _null_ _null_ ));
604604
DESCR("brin(internal)");
605-
DATA(insert OID = 3952 ( brin_summarize_new_values PGNSP PGUID 12 1 0 0 0 f f f f f f v 1 0 23 "2205" _null_ _null_ _null_ _null_ _null_ brin_summarize_new_values _null_ _null_ _null_ ));
605+
DATA(insert OID = 3952 ( brin_summarize_new_values PGNSP PGUID 12 1 0 0 0 f f f f t f v 1 0 23 "2205" _null_ _null_ _null_ _null_ _null_ brin_summarize_new_values _null_ _null_ _null_ ));
606606
DESCR("brin: standalone scan new table pages");
607607

608608
DATA(insert OID = 339 ( poly_same PGNSP PGUID 12 1 0 0 0 f f f f t f i 2 0 16 "604 604" _null_ _null_ _null_ _null_ _null_ poly_same _null_ _null_ _null_ ));
@@ -2912,9 +2912,9 @@ DATA(insert OID = 2274 ( pg_stat_reset PGNSP PGUID 12 1 0 0 0 f f f f f f v
29122912
DESCR("statistics: reset collected statistics for current database");
29132913
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_ _null_ pg_stat_reset_shared _null_ _null_ _null_ ));
29142914
DESCR("statistics: reset collected statistics shared across the cluster");
2915-
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_ _null_ pg_stat_reset_single_table_counters _null_ _null_ _null_ ));
2915+
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_ _null_ pg_stat_reset_single_table_counters _null_ _null_ _null_ ));
29162916
DESCR("statistics: reset collected statistics for a single table or index in the current database");
2917-
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_ _null_ pg_stat_reset_single_function_counters _null_ _null_ _null_ ));
2917+
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_ _null_ pg_stat_reset_single_function_counters _null_ _null_ _null_ ));
29182918
DESCR("statistics: reset collected statistics for a single function in the current database");
29192919

29202920
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_ _null_ pg_trigger_depth _null_ _null_ _null_ ));
@@ -5187,13 +5187,13 @@ DATA(insert OID = 3473 ( spg_range_quad_leaf_consistent PGNSP PGUID 12 1 0 0 0
51875187
DESCR("SP-GiST support for quad tree over range");
51885188

51895189
/* replication slots */
5190-
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_ _null_ pg_create_physical_replication_slot _null_ _null_ _null_ ));
5190+
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_ _null_ pg_create_physical_replication_slot _null_ _null_ _null_ ));
51915191
DESCR("create a physical replication slot");
5192-
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_ _null_ pg_drop_replication_slot _null_ _null_ _null_ ));
5192+
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_ _null_ pg_drop_replication_slot _null_ _null_ _null_ ));
51935193
DESCR("drop a replication slot");
51945194
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,23,28,28,3220}" "{o,o,o,o,o,o,o,o,o}" "{slot_name,plugin,slot_type,datoid,active,active_pid,xmin,catalog_xmin,restart_lsn}" _null_ _null_ pg_get_replication_slots _null_ _null_ _null_ ));
51955195
DESCR("information about replication slots currently in use");
5196-
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_ _null_ pg_create_logical_replication_slot _null_ _null_ _null_ ));
5196+
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_ _null_ pg_create_logical_replication_slot _null_ _null_ _null_ ));
51975197
DESCR("set up a logical replication slot");
51985198
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_ _null_ pg_logical_slot_get_changes _null_ _null_ _null_ ));
51995199
DESCR("get changes from replication slot");

0 commit comments

Comments
 (0)