diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 5586fbe5b07..1a6088bf2f7 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -32,6 +32,7 @@ #include "access/xlogrecovery.h" #include "access/xlogutils.h" #include "access/xlogwait.h" +#include "catalog/aclcheck_track.h" #include "catalog/index.h" #include "catalog/namespace.h" #include "catalog/pg_enum.h" @@ -2961,6 +2962,9 @@ AbortTransaction(void) s->parallelModeLevel = 0; s->parallelChildXact = false; /* should be false already */ + /* Reset aclcheck tracking state */ + aclcheck_tracking_active = false; + /* * do abort processing */ diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index 007ede997c5..07d1a9f0a0f 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -44,6 +44,7 @@ #include "access/sysattr.h" #include "access/tableam.h" #include "access/xact.h" +#include "catalog/aclcheck_track.h" #include "catalog/binary_upgrade.h" #include "catalog/catalog.h" #include "catalog/dependency.h" @@ -81,9 +82,19 @@ #include "utils/fmgroids.h" #include "utils/guc.h" #include "utils/lsyscache.h" +#include "utils/memutils.h" #include "utils/rel.h" #include "utils/syscache.h" +#define ACLCHECK_TRACK_INITIAL_SIZE 64 + +static MemoryContext AclCheckTrackContext = NULL; + +AclCheckEntry *aclcheck_tracked = NULL; +int aclcheck_tracked_count = 0; +int aclcheck_tracked_max = 0; +bool aclcheck_tracking_active = false; + /* * Internal format used by ALTER DEFAULT PRIVILEGES. */ @@ -3891,6 +3902,7 @@ object_aclcheck_ext(Oid classid, Oid objectid, Oid roleid, AclMode mode, bool *is_missing) { + aclcheck_track_record(classid, objectid, roleid, mode); if (object_aclmask_ext(classid, objectid, roleid, mode, ACLMASK_ANY, is_missing) != 0) return ACLCHECK_OK; @@ -4093,6 +4105,7 @@ AclResult pg_class_aclcheck_ext(Oid table_oid, Oid roleid, AclMode mode, bool *is_missing) { + aclcheck_track_record(RelationRelationId, table_oid, roleid, mode); if (pg_class_aclmask_ext(table_oid, roleid, mode, ACLMASK_ANY, is_missing) != 0) return ACLCHECK_OK; @@ -5036,3 +5049,69 @@ RemoveRoleFromInitPriv(Oid roleid, Oid classid, Oid objid, int32 objsubid) table_close(rel, RowExclusiveLock); } + +/* + * Reset the tracking array. Called at each top-level ProcessUtility() + * to avoid carrying stale entries from a previous statement. + * Resets the memory context, freeing all prior allocations. + */ +void +aclcheck_track_reset(void) +{ + if (AclCheckTrackContext == NULL) + { + AclCheckTrackContext = AllocSetContextCreate(TopMemoryContext, + "AclCheckTrackContext", + ALLOCSET_DEFAULT_SIZES); + } + else + { + MemoryContextReset(AclCheckTrackContext); + } + + aclcheck_tracked = (AclCheckEntry *) + MemoryContextAlloc(AclCheckTrackContext, + ACLCHECK_TRACK_INITIAL_SIZE * sizeof(AclCheckEntry)); + aclcheck_tracked_max = ACLCHECK_TRACK_INITIAL_SIZE; + aclcheck_tracked_count = 0; + aclcheck_tracking_active = true; +} + +/* + * Grow the tracking array when full. Doubles the size each time. + */ +void +aclcheck_track_grow(void) +{ + aclcheck_tracked_max *= 2; + aclcheck_tracked = (AclCheckEntry *) + repalloc(aclcheck_tracked, + aclcheck_tracked_max * sizeof(AclCheckEntry)); +} + +/* + * Look up a tracked aclcheck entry for the given object. + * Returns true if found, filling in roleId, mode, and inval_count. + * Searches from the end to find the most recent check (the one with the + * freshest inval_count). + * + * This is O(n) in the number of tracked entries, but that's fine since a typical + * DDL statement should track only a few objects. + */ +bool +aclcheck_track_find(Oid classId, Oid objectId, Oid *roleId, AclMode *mode, + uint64 *inval_count) +{ + for (int i = aclcheck_tracked_count - 1; i >= 0; i--) + { + if (aclcheck_tracked[i].classId == classId && + aclcheck_tracked[i].objectId == objectId) + { + *roleId = aclcheck_tracked[i].roleId; + *mode = aclcheck_tracked[i].mode; + *inval_count = aclcheck_tracked[i].inval_count; + return true; + } + } + return false; +} diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c index 9a7a401aced..6b13515bbbc 100644 --- a/src/backend/catalog/pg_depend.c +++ b/src/backend/catalog/pg_depend.c @@ -17,6 +17,7 @@ #include "access/genam.h" #include "access/htup_details.h" #include "access/table.h" +#include "catalog/aclcheck_track.h" #include "catalog/catalog.h" #include "catalog/dependency.h" #include "catalog/indexing.h" @@ -29,6 +30,8 @@ #include "miscadmin.h" #include "storage/lmgr.h" #include "storage/lock.h" +#include "storage/sinval.h" +#include "utils/acl.h" #include "utils/fmgroids.h" #include "utils/lsyscache.h" #include "utils/rel.h" @@ -38,6 +41,7 @@ static bool isObjectPinned(const ObjectAddress *object); static void dependencyLockAndCheckObject(Oid classId, Oid objectId); +static void recheckAclAndLock(Oid classId, Oid objectId); /* @@ -732,21 +736,104 @@ isObjectPinned(const ObjectAddress *object) } +/* + * recheckAclAndLock() + * + * Verify permission and acquire lock. + * + * If a permission check was tracked for this object, compare the saved + * SharedInvalidMessageCounter with the current value. If it changed + * (meaning catalog invalidations arrived since the original check), + * re-verify the permission before locking. After acquiring the lock, + * if more invalidations arrived during the wait, release and retry. + * + * Same approach as RangeVarGetRelidExtended(). + * + * If no permission check was tracked, just acquire the lock. + */ +static void +recheckAclAndLock(Oid classId, Oid objectId) +{ + Oid roleId; + AclMode mode; + uint64 saved_inval_count; + ObjectAddress object; + bool had_aclcheck; + + had_aclcheck = aclcheck_track_find(classId, objectId, + &roleId, &mode, &saved_inval_count); + + /* No permission check was tracked, just acquire the lock */ + if (!had_aclcheck) + { + if (classId == RelationRelationId) + LockRelationOid(objectId, AccessShareLock); + else + LockDatabaseObject(classId, objectId, 0, AccessShareLock); + return; + } + + object.classId = classId; + object.objectId = objectId; + object.objectSubId = 0; + + for (;;) + { + uint64 inval_count; + + inval_count = SharedInvalidMessageCounter; + + /* Recheck only if invalidations arrived since the original check */ + if (saved_inval_count != inval_count) + { + AclResult aclresult; + + if (classId == RelationRelationId) + aclresult = pg_class_aclcheck(objectId, roleId, mode); + else + aclresult = object_aclcheck(classId, objectId, roleId, mode); + + if (aclresult != ACLCHECK_OK) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("permission denied for %s", + getObjectDescription(&object, false)))); + } + + /* Acquire lock */ + if (classId == RelationRelationId) + LockRelationOid(objectId, AccessShareLock); + else + LockDatabaseObject(classId, objectId, 0, AccessShareLock); + + /* If no invalidations during lock wait, we're done */ + if (inval_count == SharedInvalidMessageCounter) + break; + + /* Something changed, release lock and retry */ + if (classId == RelationRelationId) + UnlockRelationOid(objectId, AccessShareLock); + else + UnlockDatabaseObject(classId, objectId, 0, AccessShareLock); + + saved_inval_count = 0; /* force recheck on next iteration */ + } +} + + /* * dependencyLockAndCheckObject * * Lock the object that we are about to record a dependency on. After it's * locked, verify that it hasn't been dropped while we weren't looking. If it - * has been dropped, throw an an error. + * has been dropped, throw an error. + * + * Lock acquisition and permission verification are handled by + * recheckAclAndLock(), which uses the same retry pattern as + * RangeVarGetRelidExtended() to close the TOCTOU window. * * If the caller already holds a lock that conflicts with DROP - * (AccessShareLock or stronger), this does nothing. Callers should acquire - * locks already when they look up the referenced objects, but many callers - * currently do not. This is a backstop to make sure that we don't record a - * bogus reference permanently in the catalogs in that case. In the future, - * after we have tightened up all the callers to acquire locks earlier, this - * could just verify that the object is already locked and throw an error if - * not. + * (AccessShareLock or stronger), this skips lock acquisition entirely. */ static void dependencyLockAndCheckObject(Oid classId, Oid objectId) @@ -775,8 +862,8 @@ dependencyLockAndCheckObject(Oid classId, Oid objectId) if (LockHeldByMe(&tag, AccessShareLock, true)) return; - /* Assume we should lock the whole object not a sub-object */ - LockDatabaseObject(classId, objectId, 0, AccessShareLock); + /* Check permission and acquire lock using retry pattern */ + recheckAclAndLock(classId, objectId); /* * Check that the object still exists. If the catalog has a suitable @@ -835,7 +922,9 @@ dependencyLockAndCheckObject(Oid classId, Oid objectId) if (CheckRelationOidLockedByMe(objectId, AccessShareLock, true)) return; - LockRelationOid(objectId, AccessShareLock); + + /* Check permission and acquire lock using retry pattern */ + recheckAclAndLock(classId, objectId); if (SearchSysCacheExists1(RELOID, ObjectIdGetDatum(objectId))) return; diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index 73a56f1df1d..cc72c616657 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -20,6 +20,7 @@ #include "access/twophase.h" #include "access/xact.h" #include "access/xlog.h" +#include "catalog/aclcheck_track.h" #include "catalog/namespace.h" #include "catalog/pg_authid.h" #include "catalog/pg_inherits.h" @@ -515,6 +516,13 @@ ProcessUtility(PlannedStmt *pstmt, Assert(queryString != NULL); /* required as of 8.4 */ Assert(qc == NULL || qc->commandTag == CMDTAG_UNKNOWN); + /* + * Reset the aclcheck tracking for each top-level utility statement and + * enable tracking for the duration of the statement. + */ + if (context == PROCESS_UTILITY_TOPLEVEL) + aclcheck_track_reset(); + /* * We provide a function hook variable that lets loadable plugins get * control when ProcessUtility is called. Such a plugin would normally @@ -528,6 +536,9 @@ ProcessUtility(PlannedStmt *pstmt, standard_ProcessUtility(pstmt, queryString, readOnlyTree, context, params, queryEnv, dest, qc); + + if (context == PROCESS_UTILITY_TOPLEVEL) + aclcheck_tracking_active = false; } /* diff --git a/src/include/catalog/aclcheck_track.h b/src/include/catalog/aclcheck_track.h new file mode 100644 index 00000000000..86880f7b56f --- /dev/null +++ b/src/include/catalog/aclcheck_track.h @@ -0,0 +1,63 @@ +/*------------------------------------------------------------------------- + * + * aclcheck_track.h + * Track permission checks for revalidation after lock acquisition + * in dependencyLockAndCheckObject(). + * + * Portions Copyright (c) 1996-2026, PostgreSQL Global Development Group + * Portions Copyright (c) 1994, Regents of the University of California + * + * src/include/catalog/aclcheck_track.h + * + *------------------------------------------------------------------------- + */ +#ifndef ACLCHECK_TRACK_H +#define ACLCHECK_TRACK_H + +#include "storage/sinval.h" +#include "utils/acl.h" + +typedef struct AclCheckEntry +{ + Oid classId; + Oid objectId; + Oid roleId; + AclMode mode; + uint64 inval_count; +} AclCheckEntry; + +extern AclCheckEntry *aclcheck_tracked; +extern int aclcheck_tracked_count; +extern int aclcheck_tracked_max; +extern bool aclcheck_tracking_active; + +extern void aclcheck_track_reset(void); +extern void aclcheck_track_grow(void); +extern bool aclcheck_track_find(Oid classId, Oid objectId, + Oid *roleId, AclMode *mode, + uint64 *inval_count); + +/* + * Record an aclcheck for later revalidation. + * + * Called from object_aclcheck_ext() and pg_class_aclcheck_ext(). + * Only records when inside an utility statement. + */ +static inline void +aclcheck_track_record(Oid classId, Oid objectId, Oid roleId, AclMode mode) +{ + if (!aclcheck_tracking_active) + return; + + if (aclcheck_tracked_count >= aclcheck_tracked_max) + aclcheck_track_grow(); + + aclcheck_tracked[aclcheck_tracked_count].classId = classId; + aclcheck_tracked[aclcheck_tracked_count].objectId = objectId; + aclcheck_tracked[aclcheck_tracked_count].roleId = roleId; + aclcheck_tracked[aclcheck_tracked_count].mode = mode; + aclcheck_tracked[aclcheck_tracked_count].inval_count = SharedInvalidMessageCounter; + aclcheck_tracked_count++; +} + +#endif /* ACLCHECK_TRACK_H */ diff --git a/src/test/isolation/expected/ddl-dependency-locking.out b/src/test/isolation/expected/ddl-dependency-locking.out index 636de281022..002d537efb7 100644 --- a/src/test/isolation/expected/ddl-dependency-locking.out +++ b/src/test/isolation/expected/ddl-dependency-locking.out @@ -1,4 +1,4 @@ -Parsed test spec with 2 sessions +Parsed test spec with 3 sessions starting permutation: s1_begin s1_create_function_in_schema s2_drop_schema s1_commit step s1_begin: BEGIN; @@ -135,3 +135,12 @@ step s2_drop_role: DROP ROLE regress_dependency; step s1_commit: COMMIT; step s2_drop_role: <... completed> ERROR: role "regress_dependency" cannot be dropped because some objects depend on it + +starting permutation: s2_begin s2_drop_fdw_wrapper s1_create_server_as_role_user s3_revoke_role s2_rollback +step s2_begin: BEGIN; +step s2_drop_fdw_wrapper: DROP FOREIGN DATA WRAPPER fdw_wrapper RESTRICT; +step s1_create_server_as_role_user: SET ROLE role_user; CREATE SERVER srv_role_revoked FOREIGN DATA WRAPPER fdw_wrapper; RESET ROLE; +step s3_revoke_role: REVOKE role_fdw FROM role_user; +step s2_rollback: ROLLBACK; +step s1_create_server_as_role_user: <... completed> +ERROR: permission denied for foreign-data wrapper fdw_wrapper diff --git a/src/test/isolation/specs/ddl-dependency-locking.spec b/src/test/isolation/specs/ddl-dependency-locking.spec index de5bd88d35e..69e6fedf9f5 100644 --- a/src/test/isolation/specs/ddl-dependency-locking.spec +++ b/src/test/isolation/specs/ddl-dependency-locking.spec @@ -11,6 +11,10 @@ setup CREATE FUNCTION f() RETURNS int LANGUAGE SQL RETURN 1; CREATE FUNCTION public.falter() RETURNS int LANGUAGE SQL RETURN 1; CREATE FOREIGN DATA WRAPPER fdw_wrapper; + CREATE ROLE role_fdw; + CREATE ROLE role_user LOGIN; + GRANT USAGE ON FOREIGN DATA WRAPPER fdw_wrapper TO role_fdw; + GRANT role_fdw TO role_user; CREATE ROLE regress_dependency; } @@ -24,6 +28,7 @@ teardown DROP FUNCTION IF EXISTS alterschema.falter(); DROP DOMAIN IF EXISTS idid; DROP SERVER IF EXISTS srv_fdw_wrapper; + DROP SERVER IF EXISTS srv_role_revoked; DROP TABLE IF EXISTS tabtype; DROP SCHEMA IF EXISTS testschema; DROP SCHEMA IF EXISTS alterschema; @@ -32,6 +37,8 @@ teardown DROP DOMAIN IF EXISTS id; DROP FUNCTION IF EXISTS f(); DROP FOREIGN DATA WRAPPER IF EXISTS fdw_wrapper; + DROP ROLE IF EXISTS role_user; + DROP ROLE IF EXISTS role_fdw; DROP ROLE regress_dependency; } @@ -47,6 +54,7 @@ step "s1_alter_function_schema" { ALTER FUNCTION public.falter() SET SCHEMA alte step "s1_create_domain_with_domain" { CREATE DOMAIN idid as id; } step "s1_create_table_with_type" { CREATE TABLE tabtype(a footab); } step "s1_create_server_with_fdw_wrapper" { CREATE SERVER srv_fdw_wrapper FOREIGN DATA WRAPPER fdw_wrapper; } +step "s1_create_server_as_role_user" { SET ROLE role_user; CREATE SERVER srv_role_revoked FOREIGN DATA WRAPPER fdw_wrapper; RESET ROLE; } step "s1_commit" { COMMIT; } session "s2" @@ -62,6 +70,11 @@ step "s2_drop_domain_id" { DROP DOMAIN id; } step "s2_drop_fdw_wrapper" { DROP FOREIGN DATA WRAPPER fdw_wrapper RESTRICT; } step "s2_drop_role" { DROP ROLE regress_dependency; } step "s2_commit" { COMMIT; } +step "s2_rollback" { ROLLBACK; } + +session "s3" + +step "s3_revoke_role" { REVOKE role_fdw FROM role_user; } # create function - drop schema permutation "s1_begin" "s1_create_function_in_schema" "s2_drop_schema" "s1_commit" @@ -102,3 +115,6 @@ permutation "s1_begin" "s1_alter_function_owner" "s2_drop_role" "s1_commit" # was concurrently dropped", contains an OID that is not stable. # # permutation "s2_begin" "s2_drop_role" "s1_alter_function_owner" "s2_commit" + +# Role membership TOCTOU: permission via role revoked during lock wait. +permutation "s2_begin" "s2_drop_fdw_wrapper" "s1_create_server_as_role_user" "s3_revoke_role" "s2_rollback" diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 8cf40c87043..cb0cc25bda8 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -19,6 +19,7 @@ AbsoluteTime AccessMethodInfo AccessPriv Acl +AclCheckEntry AclItem AclMaskHow AclMode -- 2.34.1