Skip to content

Commit c69a74d

Browse files
kylorend3rhackorum
kylorend3r
authored and
hackorum
committed
pg_dump: skip pg_init_privs entries for non-existent roles
Before commit 5342874, 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 0c9d844); 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 <huseyin.d3r@gmail.com> Author: Rui Zhao <zhaorui126@gmail.com> Reviewed-by: Rui Zhao <zhaorui126@gmail.com> Discussion: https://postgr.es/m/19483-80de42dc4e62cfd6%40postgresql.org Backpatch-through: 15
1 parent d733fca commit c69a74d

3 files changed

Lines changed: 269 additions & 3 deletions

File tree

‎src/bin/pg_dump/meson.build‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,7 @@ tests += {
103103
't/004_pg_dump_parallel.pl',
104104
't/005_pg_dump_filterfile.pl',
105105
't/006_pg_dump_compress.pl',
106+
't/008_pg_dump_dangling_initprivs.pl',
106107
't/010_dump_connstr.pl',
107108
],
108109
},

‎src/bin/pg_dump/pg_dump.c‎

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,35 @@ static int nsequences = 0;
238238
fmtQualifiedId((obj)->dobj.namespace->dobj.name, \
239239
(obj)->dobj.name)
240240

241+
/*
242+
* SQL expression that filters dangling role OIDs from a pg_init_privs
243+
* aclitem[] column. An aclitem is dangling when its grantor or non-PUBLIC
244+
* grantee OID no longer exists in pg_roles. We use pg_roles rather than
245+
* pg_authid so that non-superuser pg_dump works.
246+
*
247+
* Applied only in queries that fetch pg_init_privs data (not in WHERE clauses
248+
* of per-object queries) to avoid running this subquery per function/aggregate.
249+
*/
250+
#define SAFE_INITPRIVS(col) \
251+
"NULLIF(\n" \
252+
" ARRAY(\n" \
253+
" SELECT elt FROM pg_catalog.unnest(" col ") AS elt\n" \
254+
" WHERE NOT EXISTS (\n" \
255+
" SELECT 1 FROM pg_catalog.aclexplode(ARRAY[elt]) ace\n" \
256+
" WHERE NOT EXISTS (\n" \
257+
" SELECT 1 FROM pg_catalog.pg_roles\n" \
258+
" WHERE oid = ace.grantor\n" \
259+
" )\n" \
260+
" OR (ace.grantee <> 0\n" \
261+
" AND NOT EXISTS (\n" \
262+
" SELECT 1 FROM pg_catalog.pg_roles\n" \
263+
" WHERE oid = ace.grantee\n" \
264+
" ))\n" \
265+
" )\n" \
266+
" ),\n" \
267+
" ARRAY[]::pg_catalog.aclitem[]\n" \
268+
")"
269+
241270
static void help(const char *progname);
242271
static void setup_connection(Archive *AH,
243272
const char *dumpencoding, const char *dumpsnapshot,
@@ -10719,8 +10748,9 @@ getAdditionalACLs(Archive *fout)
1071910748

1072010749
/* Fetch initial-privileges data */
1072110750
printfPQExpBuffer(query,
10722-
"SELECT objoid, classoid, objsubid, privtype, initprivs "
10723-
"FROM pg_init_privs");
10751+
"SELECT pip.objoid, pip.classoid, pip.objsubid, pip.privtype,\n"
10752+
" " SAFE_INITPRIVS("pip.initprivs") " AS initprivs\n"
10753+
"FROM pg_catalog.pg_init_privs pip");
1072410754

1072510755
res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
1072610756

@@ -16861,7 +16891,8 @@ dumpTable(Archive *fout, const TableInfo *tbinfo)
1686116891
"SELECT at.attname, "
1686216892
"at.attacl, "
1686316893
"'{}' AS acldefault, "
16864-
"pip.privtype, pip.initprivs "
16894+
"pip.privtype,\n"
16895+
" " SAFE_INITPRIVS("pip.initprivs") " AS initprivs\n"
1686516896
"FROM pg_catalog.pg_attribute at "
1686616897
"LEFT JOIN pg_catalog.pg_init_privs pip ON "
1686716898
"(at.attrelid = pip.objoid "

‎src/bin/pg_dump/t/008_pg_dump_dangling_initprivs.pl‎

Lines changed: 234 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,234 @@
1+
# Copyright (c) 2024-2026, PostgreSQL Global Development Group
2+
#
3+
# Tests that pg_dump silently skips pg_init_privs entries that reference
4+
# roles no longer present in pg_authid, rather than emitting invalid GRANT
5+
# statements with numeric OIDs as role names.
6+
7+
use strict;
8+
use warnings FATAL => 'all';
9+
10+
use PostgreSQL::Test::Cluster;
11+
use PostgreSQL::Test::Utils;
12+
use Test::More;
13+
14+
my $node = PostgreSQL::Test::Cluster->new('main');
15+
$node->init;
16+
$node->start;
17+
18+
$node->safe_psql('postgres', 'CREATE DATABASE regress_dangling');
19+
20+
# --- Setup ---
21+
# Simulate dangling pg_init_privs entries by inserting grants for a role
22+
# and then deleting the role directly from pg_authid (bypassing pg_shdepend).
23+
$node->safe_psql(
24+
'regress_dangling',
25+
q{
26+
SET allow_system_table_mods = true;
27+
28+
-- Roles for testing
29+
CREATE ROLE ghost_grantee;
30+
CREATE ROLE ghost_grantor;
31+
CREATE ROLE "007";
32+
CREATE ROLE regress_col_grantee;
33+
CREATE ROLE ghost_col_revoke;
34+
35+
-- Case 1: dangling grantee (function)
36+
CREATE FUNCTION public.test_func_grantee() RETURNS int LANGUAGE sql AS 'SELECT 1';
37+
REVOKE ALL ON FUNCTION public.test_func_grantee() FROM PUBLIC;
38+
INSERT INTO pg_init_privs (objoid, classoid, objsubid, privtype, initprivs)
39+
SELECT p.oid,
40+
(SELECT oid FROM pg_class WHERE relname = 'pg_proc'),
41+
0, 'e',
42+
ARRAY[('ghost_grantee=X/' || quote_ident(current_user))::aclitem]
43+
FROM pg_proc p
44+
WHERE p.proname = 'test_func_grantee'
45+
AND p.pronamespace = 'public'::regnamespace;
46+
47+
-- Case 2: dangling grantor (function)
48+
CREATE FUNCTION public.test_func_grantor() RETURNS int LANGUAGE sql AS 'SELECT 2';
49+
REVOKE ALL ON FUNCTION public.test_func_grantor() FROM PUBLIC;
50+
INSERT INTO pg_init_privs (objoid, classoid, objsubid, privtype, initprivs)
51+
SELECT p.oid,
52+
(SELECT oid FROM pg_class WHERE relname = 'pg_proc'),
53+
0, 'e',
54+
ARRAY[(quote_ident(current_user) || '=X/ghost_grantor')::aclitem]
55+
FROM pg_proc p
56+
WHERE p.proname = 'test_func_grantor'
57+
AND p.pronamespace = 'public'::regnamespace;
58+
59+
-- Case 3: dangling column-level grantee (table)
60+
CREATE TABLE public.test_tbl (id int, secret text);
61+
GRANT SELECT (secret) ON public.test_tbl TO regress_col_grantee;
62+
INSERT INTO pg_init_privs (objoid, classoid, objsubid, privtype, initprivs)
63+
SELECT c.oid,
64+
(SELECT oid FROM pg_class WHERE relname = 'pg_class'),
65+
2, 'e',
66+
ARRAY[('ghost_grantee=r/' || quote_ident(current_user))::aclitem]
67+
FROM pg_class c
68+
WHERE c.relname = 'test_tbl'
69+
AND c.relnamespace = 'public'::regnamespace;
70+
71+
-- Case 4: a column that was *never* explicitly granted anything (its
72+
-- actual attacl stays NULL) but carries an all-dangling pg_init_privs
73+
-- entry. The column-ACL query picks up a column whenever it has *any*
74+
-- pg_init_privs row, regardless of whether attacl is NULL, so this is a
75+
-- genuinely reachable "spurious REVOKE" path -- unlike a plain function
76+
-- with NULL proacl, which pg_dump never even considers for ACL output
77+
-- (DUMP_COMPONENT_ACL is only set there when the actual ACL is non-NULL).
78+
CREATE TABLE public.test_tbl_nullacl (id int, secret text);
79+
INSERT INTO pg_init_privs (objoid, classoid, objsubid, privtype, initprivs)
80+
SELECT c.oid,
81+
(SELECT oid FROM pg_class WHERE relname = 'pg_class'),
82+
2, 'e',
83+
ARRAY[('ghost_col_revoke=r/' || quote_ident(current_user))::aclitem]
84+
FROM pg_class c
85+
WHERE c.relname = 'test_tbl_nullacl'
86+
AND c.relnamespace = 'public'::regnamespace;
87+
88+
-- Case 5: valid all-digit role "007" with a grant (must NOT be filtered)
89+
CREATE FUNCTION public.test_func_007() RETURNS int LANGUAGE sql AS 'SELECT 7';
90+
GRANT EXECUTE ON FUNCTION public.test_func_007() TO "007";
91+
92+
-- Case 6: PUBLIC grant whose grantor is dangling (must be filtered)
93+
CREATE FUNCTION public.test_func_public_ghost() RETURNS int LANGUAGE sql AS 'SELECT 6';
94+
REVOKE ALL ON FUNCTION public.test_func_public_ghost() FROM PUBLIC;
95+
INSERT INTO pg_init_privs (objoid, classoid, objsubid, privtype, initprivs)
96+
SELECT p.oid,
97+
(SELECT oid FROM pg_class WHERE relname = 'pg_proc'),
98+
0, 'e',
99+
ARRAY['=X/ghost_grantor'::aclitem]
100+
FROM pg_proc p
101+
WHERE p.proname = 'test_func_public_ghost'
102+
AND p.pronamespace = 'public'::regnamespace;
103+
104+
-- Case 7: PUBLIC grant with a valid grantor (must NOT be filtered)
105+
CREATE FUNCTION public.test_func_public_ok() RETURNS int LANGUAGE sql AS 'SELECT 7';
106+
REVOKE ALL ON FUNCTION public.test_func_public_ok() FROM PUBLIC;
107+
INSERT INTO pg_init_privs (objoid, classoid, objsubid, privtype, initprivs)
108+
SELECT p.oid,
109+
(SELECT oid FROM pg_class WHERE relname = 'pg_proc'),
110+
0, 'e',
111+
ARRAY[('=X/' || quote_ident(current_user))::aclitem]
112+
FROM pg_proc p
113+
WHERE p.proname = 'test_func_public_ok'
114+
AND p.pronamespace = 'public'::regnamespace;
115+
116+
-- Case 8: dangling grantee on an aggregate. Aggregates are stored in
117+
-- pg_proc like plain functions and share the same ACL-diff machinery, but
118+
-- they are fetched by a separate query (getAggregates(), not getFuncs()),
119+
-- so this exercises a genuinely different code path than case 1.
120+
CREATE AGGREGATE public.test_agg_grantee (int4) (SFUNC = int4pl, STYPE = int4, INITCOND = '0');
121+
REVOKE ALL ON FUNCTION public.test_agg_grantee(int4) FROM PUBLIC;
122+
INSERT INTO pg_init_privs (objoid, classoid, objsubid, privtype, initprivs)
123+
SELECT p.oid,
124+
(SELECT oid FROM pg_class WHERE relname = 'pg_proc'),
125+
0, 'e',
126+
ARRAY[('ghost_grantee=X/' || quote_ident(current_user))::aclitem]
127+
FROM pg_proc p
128+
WHERE p.proname = 'test_agg_grantee'
129+
AND p.pronamespace = 'public'::regnamespace;
130+
131+
-- Now delete the ghost roles to create dangling OIDs
132+
DELETE FROM pg_authid WHERE rolname = 'ghost_grantee';
133+
DELETE FROM pg_authid WHERE rolname = 'ghost_grantor';
134+
DELETE FROM pg_authid WHERE rolname = 'ghost_col_revoke';
135+
136+
137+
});
138+
139+
my $tempdir = PostgreSQL::Test::Utils::tempdir;
140+
my $dump_file = "$tempdir/dangling.sql";
141+
142+
# pg_dump must succeed even with dangling pg_init_privs entries.
143+
command_ok(
144+
[
145+
'pg_dump',
146+
'--port' => $node->port,
147+
'--schema-only',
148+
'-f' => $dump_file,
149+
'regress_dangling',
150+
],
151+
'pg_dump succeeds with dangling pg_init_privs entries');
152+
153+
my $dump = slurp_file($dump_file);
154+
155+
# --- Case 1: dangling grantee ---
156+
like($dump, qr/CREATE FUNCTION public\.test_func_grantee/,
157+
'case 1: function is present in dump');
158+
unlike($dump, qr/GRANT\b.*\btest_func_grantee/,
159+
'case 1: no GRANT for function with dangling grantee');
160+
161+
# --- Case 2: dangling grantor ---
162+
like($dump, qr/CREATE FUNCTION public\.test_func_grantor/,
163+
'case 2: function is present in dump');
164+
unlike($dump, qr/GRANT\b.*\btest_func_grantor/,
165+
'case 2: no GRANT for function with dangling grantor');
166+
167+
# --- Case 3: column-level dangling ---
168+
like($dump, qr/CREATE TABLE public\.test_tbl/,
169+
'case 3: table is present in dump');
170+
unlike($dump, qr/REVOKE\b.*\btest_tbl\b/,
171+
'case 3: no column-level REVOKE naming a dangling OID');
172+
173+
# --- Case 4: never-granted column with all-dangling initprivs ---
174+
like($dump, qr/CREATE TABLE public\.test_tbl_nullacl/,
175+
'case 4: table is present in dump');
176+
unlike($dump, qr/REVOKE\b.*\btest_tbl_nullacl\b/,
177+
'case 4: no spurious REVOKE for never-granted column with all-dangling initprivs');
178+
179+
# --- Case 5: valid all-digit role "007" ---
180+
like($dump, qr/CREATE FUNCTION public\.test_func_007/,
181+
'case 5: function is present in dump');
182+
like($dump, qr/GRANT\b.*\btest_func_007\b.*TO\s+"007"/,
183+
'case 5: GRANT to valid all-digit role "007" is preserved');
184+
185+
# --- General: no numeric OID as role name (other than the valid "007") ---
186+
# Match any GRANT/REVOKE naming a role as "digits", except the valid "007".
187+
unlike($dump, qr/^(?:GRANT|REVOKE)\b.*\b(?:TO|FROM)\s+"(?!007")[0-9]+"/m,
188+
'no GRANT/REVOKE with bare numeric OID as role name (other than valid "007")');
189+
190+
# --- Case 6: PUBLIC entry with dangling grantor is filtered ---
191+
unlike($dump, qr/GRANT\b.*\btest_func_public_ghost/,
192+
'case 6: no GRANT for PUBLIC entry with dangling grantor');
193+
194+
# --- Case 7: PUBLIC entry with valid grantor survives the filter ---
195+
like($dump, qr/GRANT\b.*\btest_func_public_ok/,
196+
'case 7: PUBLIC entry with valid grantor is preserved');
197+
198+
# --- Case 8: dangling grantee on an aggregate (getAggregates() path) ---
199+
like($dump, qr/CREATE AGGREGATE public\.test_agg_grantee/,
200+
'case 8: aggregate is present in dump');
201+
unlike($dump, qr/GRANT\b.*\btest_agg_grantee/,
202+
'case 8: no GRANT for aggregate with dangling grantee');
203+
204+
# --- Case 9: --binary-upgrade has its own ACL-reconstruction code path in
205+
# dumpACL() (the binary_upgrade_set_record_init_privs() preamble) that a
206+
# plain dump never exercises, so it needs its own check against the same
207+
# dangling data. ---
208+
my $dump_file_bu = "$tempdir/dangling_binary_upgrade.sql";
209+
command_ok(
210+
[
211+
'pg_dump',
212+
'--port' => $node->port,
213+
'--schema-only',
214+
'--binary-upgrade',
215+
'-f' => $dump_file_bu,
216+
'regress_dangling',
217+
],
218+
'pg_dump --binary-upgrade succeeds with dangling pg_init_privs entries');
219+
220+
my $dump_bu = slurp_file($dump_file_bu);
221+
222+
unlike(
223+
$dump_bu,
224+
qr/^(?:GRANT|REVOKE)\b.*\b(?:TO|FROM)\s+"(?!007")[0-9]+"/m,
225+
'case 9: no GRANT/REVOKE with bare numeric OID under --binary-upgrade');
226+
227+
# A dangling *grantor* never appears as a numeric role name in GRANT or
228+
# REVOKE: it appears in the SET SESSION AUTHORIZATION that dumpACL() wraps
229+
# the initial-privileges block in, which is the command that failed in the
230+
# original report. The check above cannot see that, so check it separately.
231+
unlike($dump_bu, qr/^SET SESSION AUTHORIZATION "(?!007")[0-9]+"/m,
232+
'case 9: no SET SESSION AUTHORIZATION naming a bare numeric OID');
233+
234+
done_testing();

0 commit comments

Comments
 (0)