From 9aba487a7734de31b04d2d770cc1cb254b57a97e Mon Sep 17 00:00:00 2001 From: Huseyin Demir Date: Fri, 4 Sep 2026 08:47:14 +0200 Subject: [PATCH] pg_dump: skip pg_init_privs entries for non-existent roles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before commit 53428740391, PostgreSQL did not record role dependencies for pg_init_privs entries in pg_shdepend, meaning DROP ROLE could leave behind ACL entries whose grantee or grantor OID no longer exists in pg_authid. Cross-cluster restores can produce the same situation on modern releases. Dangling entries caused pg_dump to emit invalid SQL such as "GRANT ... TO "87868"" with a numeric OID as role name, which fails on restore or pg_upgrade. A dangling grantor is just as damaging but less visible: it surfaces as SET SESSION AUTHORIZATION "87868" wrapping an otherwise valid grant. Additionally, a column that was never explicitly granted anything (its actual attacl stays NULL) but carries an all-dangling pg_init_privs entry could produce a spurious "REVOKE ... FROM "87868"" naming the dangling OID, because the column-ACL query picks up a column whenever it has any pg_init_privs row, regardless of whether attacl is NULL. (The analogous scenario for a plain function with proacl NULL cannot occur: pg_dump only considers a function for ACL output at all when its actual ACL is non-NULL, independent of pg_init_privs.) Fix by filtering each aclitem whose grantor or non-PUBLIC grantee OID does not appear in pg_roles (used instead of pg_authid to support non-superuser pg_dump). The filtering is applied server-side in the queries that fetch pg_init_privs data (getAdditionalACLs and the column-level ACL prepared statement), where the OID check is authoritative. This avoids modifying the WHERE clauses in getAggregates/getFuncs, preserving pg_dump performance on large schemas. If all entries for an object are dangling the result is NULL and no ACL is emitted, which is correct -- we cannot restore grants involving roles that do not exist. Note this also means no binary_upgrade_set_record_init_privs() block is emitted for the object, so the upgraded cluster has no pg_init_privs row for it at all and nothing recreates one. Its actual privileges are correct, but a later pg_dump has no initial privileges to diff against and falls back to the object-type default, re-emitting the GRANT and REVOKE commands the extension's script already performs. For an extension whose script does REVOKE ALL ON FUNCTION orphan_reset() FROM PUBLIC; GRANT SELECT ON orphan_view TO PUBLIC; a dump of the upgraded cluster carries both statements alongside CREATE EXTENSION, where a dump of the same database with the extension freshly installed carries CREATE EXTENSION alone. That is harmless on restore but permanent, and it makes the extension's members look as though someone had changed their privileges. Preserving the row would mean inventing a grantor for the dangling entries, so dropping it is the better of the two; without this fix the same case does not merely look untidy, it fails the upgrade outright, emitting GRANT ... TO "16384" inside the binary_upgrade_set_record_init_privs() block. A new TAP test, 008_pg_dump_dangling_initprivs.pl, covers a dangling grantee and a dangling grantor on a function, a column with a real grant mixed with a dangling entry, a never-granted column with an all-dangling entry, a legitimate all-digit role name that must not be mistaken for a dangling OID, the PUBLIC grantee case in both directions, the same dangling-grantee scenario on an aggregate (getAggregates is a separate query from getFuncs), and a run under --binary-upgrade, which has its own ACL-reconstruction path in dumpACL that a plain dump never exercises. That run also asserts that no SET SESSION AUTHORIZATION names a bare numeric OID, since a dangling grantor reaches the dump through that line rather than through the GRANT itself, whose grantee may be perfectly valid, so a GRANT/REVOKE-only check cannot see it. Each case fails when the corresponding piece of the fix is reverted, and only then. The test is registered in src/bin/pg_dump/meson.build; without that it runs only under the make build, whose prove_check globs t/*.pl, and never under meson, which is what CI uses. Backpatched to 15 only. getAdditionalACLs() does not exist before v15 (it arrived in 0c9d84427f); v14's pg_dump instead LEFT JOINs pg_init_privs in a dozen per-object queries. What makes this filter cheap is that getAdditionalACLs() runs once per dump; v14 has no such place, so the same expression would land in queries returning a row per object, which is the shape that drew performance objections during review. Since pg_upgrade runs the new cluster's pg_dump, an upgrade from v14 is already covered by the newer copy; a v14 backpatch would only help pg_dump run out of a v14 installation. Author: Hüseyin Demir Author: Rui Zhao Reviewed-by: Rui Zhao Discussion: https://postgr.es/m/19483-80de42dc4e62cfd6%40postgresql.org Backpatch-through: 15 --- src/bin/pg_dump/meson.build | 1 + src/bin/pg_dump/pg_dump.c | 37 ++- .../t/008_pg_dump_dangling_initprivs.pl | 234 ++++++++++++++++++ 3 files changed, 269 insertions(+), 3 deletions(-) create mode 100644 src/bin/pg_dump/t/008_pg_dump_dangling_initprivs.pl diff --git a/src/bin/pg_dump/meson.build b/src/bin/pg_dump/meson.build index 79bd5036841..5c338d5c988 100644 --- a/src/bin/pg_dump/meson.build +++ b/src/bin/pg_dump/meson.build @@ -103,6 +103,7 @@ tests += { 't/004_pg_dump_parallel.pl', 't/005_pg_dump_filterfile.pl', 't/006_pg_dump_compress.pl', + 't/008_pg_dump_dangling_initprivs.pl', 't/010_dump_connstr.pl', ], }, diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index b5b257f9983..51e99867745 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -237,6 +237,35 @@ static int nsequences = 0; fmtQualifiedId((obj)->dobj.namespace->dobj.name, \ (obj)->dobj.name) +/* + * SQL expression that filters dangling role OIDs from a pg_init_privs + * aclitem[] column. An aclitem is dangling when its grantor or non-PUBLIC + * grantee OID no longer exists in pg_roles. We use pg_roles rather than + * pg_authid so that non-superuser pg_dump works. + * + * Applied only in queries that fetch pg_init_privs data (not in WHERE clauses + * of per-object queries) to avoid running this subquery per function/aggregate. + */ +#define SAFE_INITPRIVS(col) \ + "NULLIF(\n" \ + " ARRAY(\n" \ + " SELECT elt FROM pg_catalog.unnest(" col ") AS elt\n" \ + " WHERE NOT EXISTS (\n" \ + " SELECT 1 FROM pg_catalog.aclexplode(ARRAY[elt]) ace\n" \ + " WHERE NOT EXISTS (\n" \ + " SELECT 1 FROM pg_catalog.pg_roles\n" \ + " WHERE oid = ace.grantor\n" \ + " )\n" \ + " OR (ace.grantee <> 0\n" \ + " AND NOT EXISTS (\n" \ + " SELECT 1 FROM pg_catalog.pg_roles\n" \ + " WHERE oid = ace.grantee\n" \ + " ))\n" \ + " )\n" \ + " ),\n" \ + " ARRAY[]::pg_catalog.aclitem[]\n" \ + ")" + static void help(const char *progname); static void setup_connection(Archive *AH, const char *dumpencoding, const char *dumpsnapshot, @@ -10737,8 +10766,9 @@ getAdditionalACLs(Archive *fout) /* Fetch initial-privileges data */ printfPQExpBuffer(query, - "SELECT objoid, classoid, objsubid, privtype, initprivs " - "FROM pg_init_privs"); + "SELECT pip.objoid, pip.classoid, pip.objsubid, pip.privtype,\n" + " " SAFE_INITPRIVS("pip.initprivs") " AS initprivs\n" + "FROM pg_catalog.pg_init_privs pip"); res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK); @@ -16891,7 +16921,8 @@ dumpTable(Archive *fout, const TableInfo *tbinfo) "SELECT at.attname, " "at.attacl, " "'{}' AS acldefault, " - "pip.privtype, pip.initprivs " + "pip.privtype,\n" + " " SAFE_INITPRIVS("pip.initprivs") " AS initprivs\n" "FROM pg_catalog.pg_attribute at " "LEFT JOIN pg_catalog.pg_init_privs pip ON " "(at.attrelid = pip.objoid " diff --git a/src/bin/pg_dump/t/008_pg_dump_dangling_initprivs.pl b/src/bin/pg_dump/t/008_pg_dump_dangling_initprivs.pl new file mode 100644 index 00000000000..46067d0dab9 --- /dev/null +++ b/src/bin/pg_dump/t/008_pg_dump_dangling_initprivs.pl @@ -0,0 +1,234 @@ +# Copyright (c) 2024-2026, PostgreSQL Global Development Group +# +# Tests that pg_dump silently skips pg_init_privs entries that reference +# roles no longer present in pg_authid, rather than emitting invalid GRANT +# statements with numeric OIDs as role names. + +use strict; +use warnings FATAL => 'all'; + +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +my $node = PostgreSQL::Test::Cluster->new('main'); +$node->init; +$node->start; + +$node->safe_psql('postgres', 'CREATE DATABASE regress_dangling'); + +# --- Setup --- +# Simulate dangling pg_init_privs entries by inserting grants for a role +# and then deleting the role directly from pg_authid (bypassing pg_shdepend). +$node->safe_psql( + 'regress_dangling', + q{ +SET allow_system_table_mods = true; + +-- Roles for testing +CREATE ROLE ghost_grantee; +CREATE ROLE ghost_grantor; +CREATE ROLE "007"; +CREATE ROLE regress_col_grantee; +CREATE ROLE ghost_col_revoke; + +-- Case 1: dangling grantee (function) +CREATE FUNCTION public.test_func_grantee() RETURNS int LANGUAGE sql AS 'SELECT 1'; +REVOKE ALL ON FUNCTION public.test_func_grantee() FROM PUBLIC; +INSERT INTO pg_init_privs (objoid, classoid, objsubid, privtype, initprivs) +SELECT p.oid, + (SELECT oid FROM pg_class WHERE relname = 'pg_proc'), + 0, 'e', + ARRAY[('ghost_grantee=X/' || quote_ident(current_user))::aclitem] +FROM pg_proc p +WHERE p.proname = 'test_func_grantee' + AND p.pronamespace = 'public'::regnamespace; + +-- Case 2: dangling grantor (function) +CREATE FUNCTION public.test_func_grantor() RETURNS int LANGUAGE sql AS 'SELECT 2'; +REVOKE ALL ON FUNCTION public.test_func_grantor() FROM PUBLIC; +INSERT INTO pg_init_privs (objoid, classoid, objsubid, privtype, initprivs) +SELECT p.oid, + (SELECT oid FROM pg_class WHERE relname = 'pg_proc'), + 0, 'e', + ARRAY[(quote_ident(current_user) || '=X/ghost_grantor')::aclitem] +FROM pg_proc p +WHERE p.proname = 'test_func_grantor' + AND p.pronamespace = 'public'::regnamespace; + +-- Case 3: dangling column-level grantee (table) +CREATE TABLE public.test_tbl (id int, secret text); +GRANT SELECT (secret) ON public.test_tbl TO regress_col_grantee; +INSERT INTO pg_init_privs (objoid, classoid, objsubid, privtype, initprivs) +SELECT c.oid, + (SELECT oid FROM pg_class WHERE relname = 'pg_class'), + 2, 'e', + ARRAY[('ghost_grantee=r/' || quote_ident(current_user))::aclitem] +FROM pg_class c +WHERE c.relname = 'test_tbl' + AND c.relnamespace = 'public'::regnamespace; + +-- Case 4: a column that was *never* explicitly granted anything (its +-- actual attacl stays NULL) but carries an all-dangling pg_init_privs +-- entry. The column-ACL query picks up a column whenever it has *any* +-- pg_init_privs row, regardless of whether attacl is NULL, so this is a +-- genuinely reachable "spurious REVOKE" path -- unlike a plain function +-- with NULL proacl, which pg_dump never even considers for ACL output +-- (DUMP_COMPONENT_ACL is only set there when the actual ACL is non-NULL). +CREATE TABLE public.test_tbl_nullacl (id int, secret text); +INSERT INTO pg_init_privs (objoid, classoid, objsubid, privtype, initprivs) +SELECT c.oid, + (SELECT oid FROM pg_class WHERE relname = 'pg_class'), + 2, 'e', + ARRAY[('ghost_col_revoke=r/' || quote_ident(current_user))::aclitem] +FROM pg_class c +WHERE c.relname = 'test_tbl_nullacl' + AND c.relnamespace = 'public'::regnamespace; + +-- Case 5: valid all-digit role "007" with a grant (must NOT be filtered) +CREATE FUNCTION public.test_func_007() RETURNS int LANGUAGE sql AS 'SELECT 7'; +GRANT EXECUTE ON FUNCTION public.test_func_007() TO "007"; + +-- Case 6: PUBLIC grant whose grantor is dangling (must be filtered) +CREATE FUNCTION public.test_func_public_ghost() RETURNS int LANGUAGE sql AS 'SELECT 6'; +REVOKE ALL ON FUNCTION public.test_func_public_ghost() FROM PUBLIC; +INSERT INTO pg_init_privs (objoid, classoid, objsubid, privtype, initprivs) +SELECT p.oid, + (SELECT oid FROM pg_class WHERE relname = 'pg_proc'), + 0, 'e', + ARRAY['=X/ghost_grantor'::aclitem] +FROM pg_proc p +WHERE p.proname = 'test_func_public_ghost' + AND p.pronamespace = 'public'::regnamespace; + +-- Case 7: PUBLIC grant with a valid grantor (must NOT be filtered) +CREATE FUNCTION public.test_func_public_ok() RETURNS int LANGUAGE sql AS 'SELECT 7'; +REVOKE ALL ON FUNCTION public.test_func_public_ok() FROM PUBLIC; +INSERT INTO pg_init_privs (objoid, classoid, objsubid, privtype, initprivs) +SELECT p.oid, + (SELECT oid FROM pg_class WHERE relname = 'pg_proc'), + 0, 'e', + ARRAY[('=X/' || quote_ident(current_user))::aclitem] +FROM pg_proc p +WHERE p.proname = 'test_func_public_ok' + AND p.pronamespace = 'public'::regnamespace; + +-- Case 8: dangling grantee on an aggregate. Aggregates are stored in +-- pg_proc like plain functions and share the same ACL-diff machinery, but +-- they are fetched by a separate query (getAggregates(), not getFuncs()), +-- so this exercises a genuinely different code path than case 1. +CREATE AGGREGATE public.test_agg_grantee (int4) (SFUNC = int4pl, STYPE = int4, INITCOND = '0'); +REVOKE ALL ON FUNCTION public.test_agg_grantee(int4) FROM PUBLIC; +INSERT INTO pg_init_privs (objoid, classoid, objsubid, privtype, initprivs) +SELECT p.oid, + (SELECT oid FROM pg_class WHERE relname = 'pg_proc'), + 0, 'e', + ARRAY[('ghost_grantee=X/' || quote_ident(current_user))::aclitem] +FROM pg_proc p +WHERE p.proname = 'test_agg_grantee' + AND p.pronamespace = 'public'::regnamespace; + +-- Now delete the ghost roles to create dangling OIDs +DELETE FROM pg_authid WHERE rolname = 'ghost_grantee'; +DELETE FROM pg_authid WHERE rolname = 'ghost_grantor'; +DELETE FROM pg_authid WHERE rolname = 'ghost_col_revoke'; + + +}); + +my $tempdir = PostgreSQL::Test::Utils::tempdir; +my $dump_file = "$tempdir/dangling.sql"; + +# pg_dump must succeed even with dangling pg_init_privs entries. +command_ok( + [ + 'pg_dump', + '--port' => $node->port, + '--schema-only', + '-f' => $dump_file, + 'regress_dangling', + ], + 'pg_dump succeeds with dangling pg_init_privs entries'); + +my $dump = slurp_file($dump_file); + +# --- Case 1: dangling grantee --- +like($dump, qr/CREATE FUNCTION public\.test_func_grantee/, + 'case 1: function is present in dump'); +unlike($dump, qr/GRANT\b.*\btest_func_grantee/, + 'case 1: no GRANT for function with dangling grantee'); + +# --- Case 2: dangling grantor --- +like($dump, qr/CREATE FUNCTION public\.test_func_grantor/, + 'case 2: function is present in dump'); +unlike($dump, qr/GRANT\b.*\btest_func_grantor/, + 'case 2: no GRANT for function with dangling grantor'); + +# --- Case 3: column-level dangling --- +like($dump, qr/CREATE TABLE public\.test_tbl/, + 'case 3: table is present in dump'); +unlike($dump, qr/REVOKE\b.*\btest_tbl\b/, + 'case 3: no column-level REVOKE naming a dangling OID'); + +# --- Case 4: never-granted column with all-dangling initprivs --- +like($dump, qr/CREATE TABLE public\.test_tbl_nullacl/, + 'case 4: table is present in dump'); +unlike($dump, qr/REVOKE\b.*\btest_tbl_nullacl\b/, + 'case 4: no spurious REVOKE for never-granted column with all-dangling initprivs'); + +# --- Case 5: valid all-digit role "007" --- +like($dump, qr/CREATE FUNCTION public\.test_func_007/, + 'case 5: function is present in dump'); +like($dump, qr/GRANT\b.*\btest_func_007\b.*TO\s+"007"/, + 'case 5: GRANT to valid all-digit role "007" is preserved'); + +# --- General: no numeric OID as role name (other than the valid "007") --- +# Match any GRANT/REVOKE naming a role as "digits", except the valid "007". +unlike($dump, qr/^(?:GRANT|REVOKE)\b.*\b(?:TO|FROM)\s+"(?!007")[0-9]+"/m, + 'no GRANT/REVOKE with bare numeric OID as role name (other than valid "007")'); + +# --- Case 6: PUBLIC entry with dangling grantor is filtered --- +unlike($dump, qr/GRANT\b.*\btest_func_public_ghost/, + 'case 6: no GRANT for PUBLIC entry with dangling grantor'); + +# --- Case 7: PUBLIC entry with valid grantor survives the filter --- +like($dump, qr/GRANT\b.*\btest_func_public_ok/, + 'case 7: PUBLIC entry with valid grantor is preserved'); + +# --- Case 8: dangling grantee on an aggregate (getAggregates() path) --- +like($dump, qr/CREATE AGGREGATE public\.test_agg_grantee/, + 'case 8: aggregate is present in dump'); +unlike($dump, qr/GRANT\b.*\btest_agg_grantee/, + 'case 8: no GRANT for aggregate with dangling grantee'); + +# --- Case 9: --binary-upgrade has its own ACL-reconstruction code path in +# dumpACL() (the binary_upgrade_set_record_init_privs() preamble) that a +# plain dump never exercises, so it needs its own check against the same +# dangling data. --- +my $dump_file_bu = "$tempdir/dangling_binary_upgrade.sql"; +command_ok( + [ + 'pg_dump', + '--port' => $node->port, + '--schema-only', + '--binary-upgrade', + '-f' => $dump_file_bu, + 'regress_dangling', + ], + 'pg_dump --binary-upgrade succeeds with dangling pg_init_privs entries'); + +my $dump_bu = slurp_file($dump_file_bu); + +unlike( + $dump_bu, + qr/^(?:GRANT|REVOKE)\b.*\b(?:TO|FROM)\s+"(?!007")[0-9]+"/m, + 'case 9: no GRANT/REVOKE with bare numeric OID under --binary-upgrade'); + +# A dangling *grantor* never appears as a numeric role name in GRANT or +# REVOKE: it appears in the SET SESSION AUTHORIZATION that dumpACL() wraps +# the initial-privileges block in, which is the command that failed in the +# original report. The check above cannot see that, so check it separately. +unlike($dump_bu, qr/^SET SESSION AUTHORIZATION "(?!007")[0-9]+"/m, + 'case 9: no SET SESSION AUTHORIZATION naming a bare numeric OID'); + +done_testing(); -- 2.50.1 (Apple Git-155)