From 7279eaac4416c9952616b8908bc1f8286764a03f Mon Sep 17 00:00:00 2001 From: Huseyin Demir Date: Wed, 24 Jun 2026 07:38:51 +0200 Subject: [PATCH] pg_dump: skip pg_init_privs entries for non-existent roles pg_init_privs, introduced in PostgreSQL 9.6, records the "initial" privilege state of objects -- either set by initdb (privtype 'i') or by CREATE EXTENSION scripts (privtype 'e'). pg_dump reads this catalog to determine which ACL changes the user made on top of the defaults, so it can emit the right GRANT/REVOKE statements. 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 even on modern releases. Dangling entries caused pg_dump to emit invalid SQL such as "GRANT ... TO "87868"" or "SET SESSION AUTHORIZATION "87868"" with a numeric OID as role name, which fails on restore or pg_upgrade. A further issue: getAggregates() and getFuncs() use pip.initprivs unfiltered in their WHERE clause. When all initprivs entries for an object are dangling and the live proacl is NULL, the comparison "p.proacl IS DISTINCT FROM pip.initprivs" evaluates to true, causing the object to be selected for ACL output. dumpACL() then computes the delta from acldefault to NULL and emits a spurious REVOKE that breaks restore. Fix by adding dangling-role filtering directly inside buildACLCommands(). A role name that consists entirely of digits is potentially a dangling OID reference; we resolve the ambiguity by querying pg_authid once at the start of the dump for any legitimate all-digit role names (normally none exist). The sorted list is passed as new digitRoles/nDigitRoles parameters to buildACLCommands(), which skips any REVOKE or GRANT item whose grantee or grantor matches a dangling reference. This approach adds zero overhead in the common case (most role names contain non-digit characters and are rejected at the first character), and confines the fix to the single function that generates all ACL output. A similar issue in pg_dumpall was fixed by commit 74b4438a70b. This bug has existed since pg_init_privs was introduced in 9.6. Backpatch to 14 (oldest currently supported branch). Author: Huseyin Demir Discussion: https://postgr.es/m/19483-80de42dc4e62cfd6%40postgresql.org Backpatch-through: 14 --- src/bin/pg_dump/dumputils.c | 52 +++++- src/bin/pg_dump/dumputils.h | 3 +- src/bin/pg_dump/pg_dump.c | 29 +++- src/bin/pg_dump/pg_dumpall.c | 6 +- .../t/008_pg_dump_dangling_initprivs.pl | 162 ++++++++++++++++++ 5 files changed, 245 insertions(+), 7 deletions(-) create mode 100644 src/bin/pg_dump/t/008_pg_dump_dangling_initprivs.pl diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c index dfb1f603a43..956e1597717 100644 --- a/src/bin/pg_dump/dumputils.c +++ b/src/bin/pg_dump/dumputils.c @@ -28,6 +28,8 @@ static bool parseAclItem(const char *item, const char *type, PQExpBuffer grantee, PQExpBuffer grantor, PQExpBuffer privs, PQExpBuffer privswgo); static char *dequoteAclUserName(PQExpBuffer output, char *input); +static bool is_dangling_role_ref(const char *name, + char **digitRoles, int nDigitRoles); static void AddAcl(PQExpBuffer aclbuf, const char *keyword, const char *subname); @@ -69,6 +71,40 @@ sanitize_line(const char *str, bool want_hyphen) } +/* + * is_dangling_role_ref + * Return true if 'name' looks like a dangling numeric OID reference. + * + * aclitemout() falls back to printing a missing role's OID as bare decimal + * digits when it cannot find the role in pg_authid. Any name that is not + * all-decimal is certainly a real role name. For all-decimal names we + * resolve the ambiguity against digitRoles[], a sorted array of rolnames + * from pg_authid that consist entirely of digits (fetched once per run). + * If digitRoles is NULL we conservatively assume all-digit names are valid. + * The empty string means PUBLIC and is never dangling. + */ +static bool +is_dangling_role_ref(const char *name, char **digitRoles, int nDigitRoles) +{ + const char *p; + + /* Empty string means PUBLIC -- never dangling */ + if (*name == '\0') + return false; + + /* Fast path: any non-digit character means it is a real role name */ + for (p = name; *p; p++) + if (!isdigit((unsigned char) *p)) + return false; + + /* All-digit name: resolve ambiguity against the known-valid list */ + if (digitRoles == NULL) + return false; /* no list supplied, assume valid */ + + return bsearch(&name, digitRoles, nDigitRoles, + sizeof(char *), pg_qsort_strcmp) == NULL; +} + /* * Build GRANT/REVOKE command(s) for an object. * @@ -104,7 +140,8 @@ bool buildACLCommands(const char *name, const char *subname, const char *nspname, const char *type, const char *acls, const char *baseacls, const char *owner, const char *prefix, int remoteVersion, - PQExpBuffer sql) + PQExpBuffer sql, + char **digitRoles, int nDigitRoles) { bool ok = true; char **aclitems = NULL; @@ -218,6 +255,11 @@ buildACLCommands(const char *name, const char *subname, const char *nspname, break; } + /* skip entries whose grantee or grantor no longer exists */ + if (is_dangling_role_ref(grantee->data, digitRoles, nDigitRoles) || + is_dangling_role_ref(grantor->data, digitRoles, nDigitRoles)) + continue; + if (privs->len > 0) { appendPQExpBuffer(firstsql, "%sREVOKE %s ON %s ", @@ -263,6 +305,11 @@ buildACLCommands(const char *name, const char *subname, const char *nspname, if (parseAclItem(grantitems[i], type, name, subname, remoteVersion, grantee, grantor, privs, privswgo)) { + /* skip entries whose grantee or grantor no longer exists */ + if (is_dangling_role_ref(grantee->data, digitRoles, nDigitRoles) || + is_dangling_role_ref(grantor->data, digitRoles, nDigitRoles)) + continue; + /* * If the grantor isn't the owner, we'll need to use SET SESSION * AUTHORIZATION to become the grantor. Issue the SET/RESET only @@ -390,7 +437,8 @@ buildDefaultACLCommands(const char *type, const char *nspname, */ if (!buildACLCommands("", NULL, NULL, type, acls, acldefault, owner, - prefix->data, remoteVersion, sql)) + prefix->data, remoteVersion, sql, + NULL, 0)) { destroyPQExpBuffer(prefix); return false; diff --git a/src/bin/pg_dump/dumputils.h b/src/bin/pg_dump/dumputils.h index d231ce1d654..f7aeebb9099 100644 --- a/src/bin/pg_dump/dumputils.h +++ b/src/bin/pg_dump/dumputils.h @@ -41,7 +41,8 @@ extern char *sanitize_line(const char *str, bool want_hyphen); extern bool buildACLCommands(const char *name, const char *subname, const char *nspname, const char *type, const char *acls, const char *baseacls, const char *owner, const char *prefix, int remoteVersion, - PQExpBuffer sql); + PQExpBuffer sql, + char **digitRoles, int nDigitRoles); extern bool buildDefaultACLCommands(const char *type, const char *nspname, const char *acls, const char *acldefault, const char *owner, diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index a0f7f8e2168..6b4f6984430 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -153,6 +153,10 @@ static bool dosync = true; /* Issue fsync() to make dump durable on disk. */ static Oid g_last_builtin_oid; /* value of the last builtin oid */ +/* Sorted list of all-digit role names fetched once from pg_authid */ +static char **digitRoleNames = NULL; +static int nDigitRoleNames = 0; + /* The specified names/patterns should to match at least one entity */ static int strict_names = 0; @@ -10899,6 +10903,25 @@ getAdditionalACLs(Archive *fout) /* Fetch initial-privileges data */ if (fout->remoteVersion >= 90600) { + PGresult *rolres; + + /* + * Collect all role names that consist entirely of digits. We need + * this to distinguish legitimate all-digit role names (e.g. "007") + * from dangling numeric OIDs that aclitemout() prints for missing + * roles. This list is normally empty; it is stored in file-scope + * statics so that buildACLCommands() can use it throughout the dump. + */ + printfPQExpBuffer(query, + "SELECT rolname FROM pg_catalog.pg_authid " + "WHERE rolname ~ '^[0-9]+$' ORDER BY 1"); + rolres = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK); + nDigitRoleNames = PQntuples(rolres); + digitRoleNames = pg_malloc_array(char *, nDigitRoleNames); + for (i = 0; i < nDigitRoleNames; i++) + digitRoleNames[i] = pg_strdup(PQgetvalue(rolres, i, 0)); + PQclear(rolres); + printfPQExpBuffer(query, "SELECT objoid, classoid, objsubid, privtype, initprivs " "FROM pg_init_privs"); @@ -16610,7 +16633,8 @@ dumpACL(Archive *fout, DumpId objDumpId, DumpId altDumpId, appendPQExpBufferStr(sql, "SELECT pg_catalog.binary_upgrade_set_record_init_privs(true);\n"); if (!buildACLCommands(name, subname, nspname, type, initprivs, acldefault, owner, - "", fout->remoteVersion, sql)) + "", fout->remoteVersion, sql, + digitRoleNames, nDigitRoleNames)) pg_fatal("could not parse initial ACL list (%s) or default (%s) for object \"%s\" (%s)", initprivs, acldefault, name, type); appendPQExpBufferStr(sql, "SELECT pg_catalog.binary_upgrade_set_record_init_privs(false);\n"); @@ -16635,7 +16659,8 @@ dumpACL(Archive *fout, DumpId objDumpId, DumpId altDumpId, if (!buildACLCommands(name, subname, nspname, type, acls, baseacls, owner, - "", fout->remoteVersion, sql)) + "", fout->remoteVersion, sql, + digitRoleNames, nDigitRoleNames)) pg_fatal("could not parse ACL list (%s) or default (%s) for object \"%s\" (%s)", acls, baseacls, name, type); diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c index c1f43113c53..59264b85a1b 100644 --- a/src/bin/pg_dump/pg_dumpall.c +++ b/src/bin/pg_dump/pg_dumpall.c @@ -1575,7 +1575,8 @@ dumpRoleGUCPrivs(PGconn *conn) if (!buildACLCommands(fparname, NULL, NULL, "PARAMETER", paracl, acldefault, - parowner, "", server_version, buf)) + parowner, "", server_version, buf, + NULL, 0)) { pg_log_error("could not parse ACL list (%s) for parameter \"%s\"", paracl, parname); @@ -1738,7 +1739,8 @@ dumpTablespaces(PGconn *conn) if (!skip_acls && !buildACLCommands(fspcname, NULL, NULL, "TABLESPACE", spcacl, acldefault, - spcowner, "", server_version, buf)) + spcowner, "", server_version, buf, + NULL, 0)) { pg_log_error("could not parse ACL list (%s) for tablespace \"%s\"", spcacl, spcname); 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..dcf33a4af05 --- /dev/null +++ b/src/bin/pg_dump/t/008_pg_dump_dangling_initprivs.pl @@ -0,0 +1,162 @@ +# Copyright (c) 2021-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 +# or SET SESSION AUTHORIZATION statements with numeric OIDs as role names. +# +# Five cases are tested: +# 1. Dangling grantee -- the role that received the privilege was dropped. +# 2. Dangling grantor -- the role that granted the privilege was dropped. +# 3. Column-level dangling grantee (objsubid != 0). +# 4. Spurious REVOKE -- a pg_catalog function whose only initprivs entry is +# all-dangling; without filtering, the object is incorrectly selected for +# ACL output and dumpACL() emits a spurious REVOKE. +# 5. Legitimate all-digit role -- a role named "007" must NOT be filtered. + +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'); + +$node->safe_psql( + 'regress_dangling', + q{ +SET allow_system_table_mods = true; + +-- Case 1: dangling grantee. +CREATE ROLE ghost_grantee; + +CREATE FUNCTION public.test_func_grantee() RETURNS int LANGUAGE sql AS 'SELECT 1'; + +INSERT INTO pg_init_privs (objoid, classoid, objsubid, privtype, initprivs) +SELECT p.oid, + (SELECT oid FROM pg_catalog.pg_class WHERE relname = 'pg_proc'), + 0, + 'e', + ARRAY[('ghost_grantee=X/' || current_user)::aclitem] +FROM pg_catalog.pg_proc p +WHERE p.proname = 'test_func_grantee' +AND p.pronamespace = 'public'::regnamespace; + +-- Case 2: dangling grantor. +CREATE ROLE ghost_grantor; + +CREATE FUNCTION public.test_func_grantor() RETURNS int LANGUAGE sql AS 'SELECT 2'; + +INSERT INTO pg_init_privs (objoid, classoid, objsubid, privtype, initprivs) +SELECT p.oid, + (SELECT oid FROM pg_catalog.pg_class WHERE relname = 'pg_proc'), + 0, + 'e', + ARRAY[('=X/' || 'ghost_grantor')::aclitem] +FROM pg_catalog.pg_proc p +WHERE p.proname = 'test_func_grantor' +AND p.pronamespace = 'public'::regnamespace; + +-- Case 3: column-level dangling grantee (objsubid != 0). +CREATE ROLE ghost_col_role; + +CREATE TABLE public.test_tbl (col1 int); + +INSERT INTO pg_init_privs (objoid, classoid, objsubid, privtype, initprivs) +SELECT t.oid, + (SELECT oid FROM pg_catalog.pg_class WHERE relname = 'pg_class'), + 1, + 'e', + ARRAY[('ghost_col_role=r/' || current_user)::aclitem] +FROM pg_catalog.pg_class t +WHERE t.relname = 'test_tbl' +AND t.relnamespace = 'public'::regnamespace; + +-- Case 4: spurious REVOKE caused by all-dangling initprivs and NULL proacl. +CREATE ROLE ghost_catalog_role; + +INSERT INTO pg_init_privs (objoid, classoid, objsubid, privtype, initprivs) +SELECT p.oid, + (SELECT oid FROM pg_catalog.pg_class WHERE relname = 'pg_proc'), + 0, + 'i', + ARRAY[('ghost_catalog_role=X/' || current_user)::aclitem] +FROM pg_catalog.pg_proc p +WHERE p.proname = 'int4' +AND p.pronamespace = (SELECT oid FROM pg_catalog.pg_namespace + WHERE nspname = 'pg_catalog') +LIMIT 1; + +-- Case 5: legitimate all-digit role name must be preserved. +-- The role "007" exists and receives a GRANT. The initprivs entry also +-- references "007" (as grantee). filterDanglingAclItems must NOT remove +-- this entry. We set up initprivs with a DIFFERENT content than proacl +-- so that the GRANT to "007" shows up in the delta output. +CREATE ROLE "007"; + +CREATE FUNCTION public.test_func_007() RETURNS int LANGUAGE sql AS 'SELECT 7'; +GRANT EXECUTE ON FUNCTION public.test_func_007() TO "007"; + +-- Remove ghost roles, leaving dangling OIDs in pg_init_privs. +-- Do NOT remove "007" -- it is a legitimate role. +DELETE FROM pg_catalog.pg_authid +WHERE rolname IN ('ghost_grantee', 'ghost_grantor', 'ghost_col_role', + 'ghost_catalog_role'); +}); + +my $tempdir = PostgreSQL::Test::Utils::tempdir; +my $dump_file = "$tempdir/dangling.sql"; + +# pg_dump must succeed even though pg_init_privs has dangling role OIDs. +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); + +# The user-defined objects must still appear in the dump. +like($dump, qr/CREATE FUNCTION public\.test_func_grantee/, + 'function with dangling grantee is present in dump'); +like($dump, qr/CREATE FUNCTION public\.test_func_grantor/, + 'function with dangling grantor is present in dump'); +like($dump, qr/CREATE TABLE public\.test_tbl/, + 'table with dangling column initprivs is present in dump'); +like($dump, qr/CREATE FUNCTION public\.test_func_007/, + 'function for legitimate all-digit role is present in dump'); + +# No statement should reference a bare numeric OID as a role name. +unlike($dump, qr/SESSION AUTHORIZATION\s+"[0-9]+"/, + 'no SET SESSION AUTHORIZATION with numeric OID as role name'); + +# No GRANT at all for the dangling-grantee function. +unlike($dump, qr/GRANT\b.*\btest_func_grantee/, + 'no GRANT for test_func_grantee when all initprivs entries are dangling'); + +# No GRANT at all for the dangling-grantor function. +unlike($dump, qr/GRANT\b.*\btest_func_grantor/, + 'no GRANT for test_func_grantor when all initprivs entries are dangling'); + +# No column GRANT for the dangling column initprivs. +unlike($dump, qr/GRANT\b.*\btest_tbl\b.*\(col1\)/, + 'no column GRANT for test_tbl when column initprivs entries are dangling'); + +# No spurious REVOKE for the catalog function whose only initprivs entry was +# all-dangling. +unlike($dump, qr/REVOKE\b.*\bint4\s*\(/, + 'no spurious REVOKE for int4 when all initprivs entries are dangling'); + +# Case 5: the legitimate all-digit role "007" MUST still get its GRANT. +like($dump, qr/GRANT\b.*\btest_func_007\b.*TO\s+"007"/, + 'GRANT to legitimate all-digit role "007" is preserved'); + +done_testing(); -- 2.50.1 (Apple Git-155)