Skip to content

Commit 277602d

Browse files
tglsfdctglsfdc
committed
Fix missing role dependencies for some schema and type ACLs.
This patch fixes several related cases in which pg_shdepend entries were never made, or were lost, for references to roles appearing in the ACLs of schemas and/or types. While that did no immediate harm, if a referenced role were later dropped, the drop would be allowed and would leave a dangling reference in the object's ACL. That still wasn't a big problem for normal database usage, but it would cause obscure failures in subsequent dump/reload or pg_upgrade attempts, taking the form of attempts to grant privileges to all-numeric role names. (I think I've seen field reports matching that symptom, but can't find any right now.) Several cases are fixed here: 1. ALTER DOMAIN SET/DROP DEFAULT would lose the dependencies for any existing ACL entries for the domain. This case is ancient, dating back as far as we've had pg_shdepend tracking at all. 2. If a default type privilege applies, CREATE TYPE recorded the ACL properly but forgot to install dependency entries for it. This dates to the addition of default privileges for types in 9.2. 3. If a default schema privilege applies, CREATE SCHEMA recorded the ACL properly but forgot to install dependency entries for it. This dates to the addition of default privileges for schemas in v10 (commit ab89e46). Another somewhat-related problem is that when creating a relation rowtype or implicit array type, TypeCreate would apply any available default type privileges to that type, which we don't really want since such an object isn't supposed to have privileges of its own. (You can't, for example, drop such privileges once they've been added to an array type.) ab89e46 is also to blame for a race condition in the regression tests: privileges.sql transiently installed globally-applicable default privileges on schemas, which sometimes got absorbed into the ACLs of schemas created by concurrent test scripts. This should have resulted in failures when privileges.sql tried to drop the role holding such privileges; but thanks to the bug fixed here, it instead led to dangling ACLs in the final state of the regression database. We'd managed not to notice that, but it became obvious in the wake of commit da90676, which allowed the race condition to occur in pg_upgrade tests. To fix, add a function recordDependencyOnNewAcl to encapsulate what callers of get_user_default_acl need to do; while the original call sites got that right via ad-hoc code, none of the later-added ones have. Also change GenerateTypeDependencies to generate these dependencies, which requires adding the typacl to its parameter list. (That might be annoying if there are any extensions calling that function directly; but if there are, they're most likely buggy in the same way as the core callers were, so they need work anyway.) While I was at it, I changed GenerateTypeDependencies to accept most of its parameters in the form of a Form_pg_type pointer, making its parameter list a bit less unwieldy and mistake-prone. The test race condition is fixed just by wrapping the addition and removal of default privileges into a single transaction, so that that state is never visible externally. We might eventually prefer to separate out tests of default privileges into a script that runs by itself, but that would be a bigger change and would make the tests run slower overall. Back-patch relevant parts to all supported branches. Discussion: https://postgr.es/m/15719.1541725287@sss.pgh.pa.us
1 parent 2407d48 commit 277602d

File tree

7 files changed

+142
-138
lines changed

7 files changed

+142
-138
lines changed

‎src/backend/catalog/aclchk.c

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5120,7 +5120,10 @@ get_default_acl_internal(Oid roleId, Oid nsp_oid, char objtype)
51205120
/*
51215121
* Get default permissions for newly created object within given schema
51225122
*
5123-
* Returns NULL if built-in system defaults should be used
5123+
* Returns NULL if built-in system defaults should be used.
5124+
*
5125+
* If the result is not NULL, caller must call recordDependencyOnNewAcl
5126+
* once the OID of the new object is known.
51245127
*/
51255128
Acl *
51265129
get_user_default_acl(GrantObjectType objtype, Oid ownerId, Oid nsp_oid)
@@ -5190,3 +5193,27 @@ get_user_default_acl(GrantObjectType objtype, Oid ownerId, Oid nsp_oid)
51905193

51915194
return result;
51925195
}
5196+
5197+
/*
5198+
* Record dependencies on roles mentioned in a new object's ACL.
5199+
*/
5200+
void
5201+
recordDependencyOnNewAcl(Oid classId, Oid objectId, int32 objsubId,
5202+
Oid ownerId, Acl *acl)
5203+
{
5204+
int nmembers;
5205+
Oid *members;
5206+
5207+
/* Nothing to do if ACL is defaulted */
5208+
if (acl == NULL)
5209+
return;
5210+
5211+
/* Extract roles mentioned in ACL */
5212+
nmembers = aclmembers(acl, &members);
5213+
5214+
/* Update the shared dependency ACL info */
5215+
updateAclDependencies(classId, objectId, objsubId,
5216+
ownerId,
5217+
0, NULL,
5218+
nmembers, members);
5219+
}

‎src/backend/catalog/heap.c

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1287,13 +1287,16 @@ heap_create_with_catalog(const char *relname,
12871287
myself.classId = RelationRelationId;
12881288
myself.objectId = relid;
12891289
myself.objectSubId = 0;
1290+
12901291
referenced.classId = NamespaceRelationId;
12911292
referenced.objectId = relnamespace;
12921293
referenced.objectSubId = 0;
12931294
recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
12941295

12951296
recordDependencyOnOwner(RelationRelationId, relid, ownerid);
12961297

1298+
recordDependencyOnNewAcl(RelationRelationId, relid, 0, ownerid, relacl);
1299+
12971300
if (relpersistence != RELPERSISTENCE_TEMP)
12981301
recordDependencyOnCurrentExtension(&myself, false);
12991302

@@ -1304,18 +1307,6 @@ heap_create_with_catalog(const char *relname,
13041307
referenced.objectSubId = 0;
13051308
recordDependencyOn(&myself, &referenced, DEPENDENCY_NORMAL);
13061309
}
1307-
1308-
if (relacl != NULL)
1309-
{
1310-
int nnewmembers;
1311-
Oid *newmembers;
1312-
1313-
nnewmembers = aclmembers(relacl, &newmembers);
1314-
updateAclDependencies(RelationRelationId, relid, 0,
1315-
ownerid,
1316-
0, NULL,
1317-
nnewmembers, newmembers);
1318-
}
13191310
}
13201311

13211312
/* Post creation hook for new relation */

‎src/backend/catalog/pg_proc.c

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -643,17 +643,9 @@ ProcedureCreate(const char *procedureName,
643643
recordDependencyOnOwner(ProcedureRelationId, retval, proowner);
644644

645645
/* dependency on any roles mentioned in ACL */
646-
if (!is_update && proacl != NULL)
647-
{
648-
int nnewmembers;
649-
Oid *newmembers;
650-
651-
nnewmembers = aclmembers(proacl, &newmembers);
652-
updateAclDependencies(ProcedureRelationId, retval, 0,
653-
proowner,
654-
0, NULL,
655-
nnewmembers, newmembers);
656-
}
646+
if (!is_update)
647+
recordDependencyOnNewAcl(ProcedureRelationId, retval, 0,
648+
proowner, proacl);
657649

658650
/* dependency on extension */
659651
recordDependencyOnCurrentExtension(&myself, is_update);

0 commit comments

Comments
 (0)