From 99b0c5da3e10a9f64ed2790ed34d8de3075edb20 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 21 Jul 2020 11:40:47 -0400 Subject: [PATCH] Avoid direct C access to possibly-null pg_subscription_rel.srsublsn. This coding technique is unsafe, since we'd be accessing off the end of the tuple if the field is null. SIGSEGV is pretty improbable, but perhaps not impossible. Also, returning garbage for the LSN doesn't seem like a great idea, even if callers aren't looking at it today. Also update docs to point out explicitly that pg_subscription.subslotname and pg_subscription_rel.srsublsn can be null. Perhaps we should mark these two fields BKI_FORCE_NULL, so that they'd be correctly labeled in databases that are initdb'd in the future. But we can't force that for existing databases, and on balance it's not too clear that having a mix of different catalog contents in the field would be wise. Apply to v10 (where this code came in) through v12. Already fixed in v13 and HEAD. Discussion: https://postgr.es/m/732838.1595278439@sss.pgh.pa.us --- doc/src/sgml/catalogs.sgml | 9 ++++++--- src/backend/catalog/pg_subscription.c | 18 ++++++++++++++++-- src/include/catalog/pg_subscription_rel.h | 13 +++++++++++-- 3 files changed, 33 insertions(+), 7 deletions(-) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index 7c2cef606c4..a141576d61b 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -6695,8 +6695,9 @@ SCRAM-SHA-256$<iteration count>:&l subslotname name - Name of the replication slot in the upstream database. Also used - for local replication origin name. + Name of the replication slot in the upstream database (also used + for the local replication origin name); + null represents NONE @@ -6778,7 +6779,9 @@ SCRAM-SHA-256$<iteration count>:&l pg_lsn - End LSN for s and r states. + Remote LSN of the state change used for synchronization coordination + when in s or r states, + otherwise null diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c index 8705d8b1d36..0ce08f7c5b5 100644 --- a/src/backend/catalog/pg_subscription.c +++ b/src/backend/catalog/pg_subscription.c @@ -457,13 +457,20 @@ GetSubscriptionRelations(Oid subid) { Form_pg_subscription_rel subrel; SubscriptionRelState *relstate; + Datum d; + bool isnull; subrel = (Form_pg_subscription_rel) GETSTRUCT(tup); relstate = (SubscriptionRelState *) palloc(sizeof(SubscriptionRelState)); relstate->relid = subrel->srrelid; relstate->state = subrel->srsubstate; - relstate->lsn = subrel->srsublsn; + d = SysCacheGetAttr(SUBSCRIPTIONRELMAP, tup, + Anum_pg_subscription_rel_srsublsn, &isnull); + if (isnull) + relstate->lsn = InvalidXLogRecPtr; + else + relstate->lsn = DatumGetLSN(d); res = lappend(res, relstate); } @@ -509,13 +516,20 @@ GetSubscriptionNotReadyRelations(Oid subid) { Form_pg_subscription_rel subrel; SubscriptionRelState *relstate; + Datum d; + bool isnull; subrel = (Form_pg_subscription_rel) GETSTRUCT(tup); relstate = (SubscriptionRelState *) palloc(sizeof(SubscriptionRelState)); relstate->relid = subrel->srrelid; relstate->state = subrel->srsubstate; - relstate->lsn = subrel->srsublsn; + d = SysCacheGetAttr(SUBSCRIPTIONRELMAP, tup, + Anum_pg_subscription_rel_srsublsn, &isnull); + if (isnull) + relstate->lsn = InvalidXLogRecPtr; + else + relstate->lsn = DatumGetLSN(d); res = lappend(res, relstate); } diff --git a/src/include/catalog/pg_subscription_rel.h b/src/include/catalog/pg_subscription_rel.h index 556cb94841d..b907b4468d7 100644 --- a/src/include/catalog/pg_subscription_rel.h +++ b/src/include/catalog/pg_subscription_rel.h @@ -34,8 +34,17 @@ CATALOG(pg_subscription_rel,6102,SubscriptionRelRelationId) BKI_WITHOUT_OIDS Oid srsubid; /* Oid of subscription */ Oid srrelid; /* Oid of relation */ char srsubstate; /* state of the relation in subscription */ - XLogRecPtr srsublsn; /* remote lsn of the state change used for - * synchronization coordination */ + + /* + * Although srsublsn is a fixed-width type, it is allowed to be NULL, so + * we prevent direct C code access to it just as for a varlena field. + */ +#ifdef CATALOG_VARLEN /* variable-length fields start here */ + + XLogRecPtr srsublsn; /* remote LSN of the state change used for + * synchronization coordination, or NULL if + * not valid */ +#endif } FormData_pg_subscription_rel; typedef FormData_pg_subscription_rel *Form_pg_subscription_rel; -- 2.39.5