Hi,
On Mon, Jun 15, 2026 at 06:19:19PM -0700, Jeff Davis wrote:
> On Wed, 2026-06-10 at 15:16 +0000, Bertrand Drouvot wrote:
> > PFA a new version of v26, it adds a new test as compared to the v26
> > previously
> > shared.
>
> I'd like to avoid adding lines to AbortTransaction(). Also, I think it
> might miss subtransaction aborts, which could be relevant in complex
> cases with SPI.
>
> Can you use a structure like:
>
> ProcessUtility()
> {
> TrackAclTable *prevTrackAclTable = CurrentTrackAclTable;
> /* allocates in CurrentMemoryContext */
> CurrentTrackAclTable = NewTrackAclTable();
>
> PG_TRY();
> {
> ... rest of ProcessUtility ...
> }
> PG_FINALLY();
> {
> FreeTrackAclTable(CurrentTrackAclTable);
> CurrentTrackAclTable = prevTrackAclTable;
> }
> PG_END_TRY();
> }
>
> That would avoid the need to create a special memory context; you could
> just repalloc() the chunk allocated for the table. It would also mean
> you don't have to track the stack frames manually with a counter, just
> use a local variable.
The advantage of v26 is that it also checked for the outer while checking the
nested (that way a revoke that impacted the outer would fail sooner). That said,
I like your idea: it's more readable and probably less error-prone (don't use
global variable in multiple places and a dedicated counter for nested checks).
So, the attached implements it that way.
It's in 0001. The reason for multiple sub-patches:
> Also, are you sure that the two call sites for aclcheck_track_record()
> are enough? Or do we need checks in e.g. pg_attribute_aclcheck() as
> well?
Currently, the only caller of pg_attribute_aclcheck_ext() that also records
dependencies on the checked object is checkFkeyPermissions(), which already
holds ShareRowExclusiveLock on the referenced table. While ShareRowExclusiveLock
prevents direct REVOKE on the table, it does not prevent role membership
revokes.
So 0003 adds pg_attribute_aclcheck_ext to ACL tracking for dependency recording,
which also protects against new DDL callers.
The remaining aclcheck functions (pg_parameter_aclcheck and
pg_largeobject_aclcheck_snapshot) do not need tracking: pg_parameter_aclcheck
checks GUC parameters which are not dependency targets, and
pg_largeobject_aclcheck_snapshot is only called from inv_open() and
has_largeobject_privilege(), neither of which records dependencies.
That said, while looking at checkFkeyPermissions(), I realized that we have a
corner case in 0001.
Indeed, checkFkeyPermissions() first tries pg_class_aclcheck() (which fails for
column-level grants), then falls through to pg_attribute_aclcheck(). But as
aclcheck_track_record() is called for both successful and failed checks, then
the tracked failed entry could trigger a 'permission denied' in recheckAcl() if
catalog invalidations arrived between the check and dependency recording.
0002: fixes it by moving aclcheck_track_record() to after the permission check
succeeds in object_aclcheck_ext() and pg_class_aclcheck_ext(). Indeed, there is
no need to track failed permission checks. It also adds a test that reproduces
the issue by injecting invalidations while the FK creation is paused after the
failed table-level check. The test would fail without the fix in 0002 applied.
0003: adds pg_attribute_aclcheck_ext to ACL tracking for dependency recording.
It adds an attnum field to AclCheckEntry so that recheckAcl() can distinguish
column-level checks from table-level checks and call the appropriate recheck
function. InvalidAttrNumber means whole-object check.
The sub-patches are to ease the review. They should probably be merged before
being pushed.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
>From 7db50de3c71c269a31bbee0650705f41b59fb0df Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email protected]>
Date: Sat, 30 May 2026 14:35:40 +0000
Subject: [PATCH v27 1/3] Recheck permissions after lock acquisition in
dependency recording
After dependencyLockAndCheckObject() acquires a lock on a referenced object
(which may have blocked on a concurrent DROP), re-verify any permission check
that was done earlier in the statement. This closes the TOCTOU window where a
REVOKE could land between the original permission check and the dependency
recording.
The approach: record SharedInvalidMessageCounter at the time of the original
aclcheck. After acquiring the lock and verifying the object still exists,
compare the saved counter with the current value. If it changed (meaning
catalog invalidations arrived since the original check), re-verify the
permission. Since the OID is fixed (no name re-resolution), a failure is
definitive and no retry loop is needed.
Each ProcessUtility() call allocates a TrackAclTable in CurrentMemoryContext
and saves the previous one in a local variable. A PG_TRY/PG_FINALLY block
ensures that the table is freed and the previous pointer restored on both
normal return and error. This forms an implicit stack: each nesting level
tracks only its own ACL checks, and the outer level's entries remain
undisturbed. No dedicated memory context or AbortTransaction cleanup is needed.
Recording is gated by checking CurrentTrackAclTable != NULL, so DML and queries
pay only a single pointer test.
Alternatives considered:
- To avoid allocating memory for each statement, keep the array in
TopMemoryContext and never free it (only resetting the count). But that left the
high-water mark allocated for the lifetime of the backend.
- Passing privilege info (roleId, mode) as extra arguments through the
dependency recording APIs (recordDependencyOn, recordMultipleDependencies,
etc.) was discarded because expression-based dependencies
(recordDependencyOnExpr, find_expr_references_walker) discover objects by
walking expression trees: the caller never sees individual objects and cannot
attach privilege info to them.
Author: Bertrand Drouvot <[email protected]>
Reviewed-by: Jeff Davis <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/catalog/aclchk.c | 32 ++++
src/backend/catalog/pg_depend.c | 104 ++++++++++++-
src/backend/tcop/utility.c | 41 +++--
src/include/catalog/aclcheck_track.h | 80 ++++++++++
.../expected/ddl-dependency-locking.out | 64 +++++++-
.../specs/ddl-dependency-locking.spec | 140 ++++++++++++++++++
src/tools/pgindent/typedefs.list | 2 +
7 files changed, 441 insertions(+), 22 deletions(-)
20.0% src/backend/catalog/
6.7% src/backend/tcop/
11.7% src/include/catalog/
20.5% src/test/isolation/expected/
40.7% src/test/isolation/specs/
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 007ede997c5..ab19db9d789 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"
@@ -84,6 +85,10 @@
#include "utils/rel.h"
#include "utils/syscache.h"
+#define ACLCHECK_TRACK_INITIAL_SIZE 64
+
+TrackAclTable *CurrentTrackAclTable = NULL;
+
/*
* Internal format used by ALTER DEFAULT PRIVILEGES.
*/
@@ -3891,6 +3896,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 +4099,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 +5043,28 @@ RemoveRoleFromInitPriv(Oid roleid, Oid classid, Oid objid, int32 objsubid)
table_close(rel, RowExclusiveLock);
}
+
+/*
+ * Allocate a new tracking table in CurrentMemoryContext.
+ */
+TrackAclTable *
+CreateTrackAclTable(void)
+{
+ TrackAclTable *acltable = (TrackAclTable *) palloc(sizeof(TrackAclTable));
+
+ acltable->entries = (AclCheckEntry *)
+ palloc(ACLCHECK_TRACK_INITIAL_SIZE * sizeof(AclCheckEntry));
+ acltable->max = ACLCHECK_TRACK_INITIAL_SIZE;
+ acltable->count = 0;
+ return acltable;
+}
+
+/*
+ * Free a tracking table.
+ */
+void
+FreeTrackAclTable(TrackAclTable *acltable)
+{
+ pfree(acltable->entries);
+ pfree(acltable);
+}
diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c
index 9a7a401aced..912c92e8fef 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 recheckAcl(Oid classId, Oid objectId);
/*
@@ -732,12 +736,76 @@ isObjectPinned(const ObjectAddress *object)
}
+/*
+ * recheckAcl()
+ *
+ * Re-verify all tracked permission checks on the given object.
+ *
+ * Called after acquiring the lock and verifying the object still exists.
+ * If permission checks were tracked for this object and catalog
+ * invalidations arrived since the original checks, re-verify them all.
+ * This closes the TOCTOU window where a REVOKE could land between the
+ * original permission check and the dependency recording.
+ *
+ * Multiple checks may have been done on the same object with different
+ * modes or roles, so we iterate through all matching entries.
+ *
+ * Since we don't re-resolve names (the OID is fixed), there is no need for
+ * a retry loop here. If a recheck fails, it's a definitive failure.
+ *
+ * The linear scan over the tracking array is O(n) in the number of tracked
+ * entries, but that's fine since a typical DDL statement tracks only a few
+ * objects.
+ */
+static void
+recheckAcl(Oid classId, Oid objectId)
+{
+ TrackAclTable *acltable = CurrentTrackAclTable;
+
+ if (acltable == NULL)
+ return;
+
+ for (int i = acltable->count - 1; i >= 0; i--)
+ {
+ if (acltable->entries[i].classId == classId &&
+ acltable->entries[i].objectId == objectId &&
+ acltable->entries[i].inval_count != SharedInvalidMessageCounter)
+ {
+ AclResult aclresult;
+
+ if (classId == RelationRelationId)
+ aclresult = pg_class_aclcheck(objectId,
+ acltable->entries[i].roleId,
+ acltable->entries[i].mode);
+ else
+ aclresult = object_aclcheck(classId, objectId,
+ acltable->entries[i].roleId,
+ acltable->entries[i].mode);
+
+ if (aclresult != ACLCHECK_OK)
+ {
+ ObjectAddress object;
+
+ object.classId = classId;
+ object.objectId = objectId;
+ object.objectSubId = 0;
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ errmsg("permission denied for %s",
+ getObjectDescription(&object, false))));
+ }
+ }
+ }
+}
+
+
/*
* 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.
+ * 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 error. Then re-verify any tracked permission
+ * check 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
@@ -773,7 +841,11 @@ dependencyLockAndCheckObject(Oid classId, Oid objectId)
0);
if (LockHeldByMe(&tag, AccessShareLock, true))
+ {
+ /* Recheck ACL */
+ recheckAcl(classId, objectId);
return;
+ }
/* Assume we should lock the whole object not a sub-object */
LockDatabaseObject(classId, objectId, 0, AccessShareLock);
@@ -786,7 +858,11 @@ dependencyLockAndCheckObject(Oid classId, Oid objectId)
if (cache != SYSCACHEID_INVALID)
{
if (SearchSysCacheExists1(cache, ObjectIdGetDatum(objectId)))
+ {
+ /* Recheck ACL */
+ recheckAcl(classId, objectId);
return;
+ }
}
/*
@@ -814,6 +890,9 @@ dependencyLockAndCheckObject(Oid classId, Oid objectId)
systable_endscan(scan);
table_close(rel, AccessShareLock);
+
+ /* Recheck ACL */
+ recheckAcl(classId, objectId);
}
else
{
@@ -834,14 +913,23 @@ dependencyLockAndCheckObject(Oid classId, Oid objectId)
Assert(!IsSharedRelation(objectId));
if (CheckRelationOidLockedByMe(objectId, AccessShareLock, true))
+ {
+ /* Recheck ACL */
+ recheckAcl(classId, objectId);
return;
+ }
+
+ /* Acquire lock */
LockRelationOid(objectId, AccessShareLock);
- if (SearchSysCacheExists1(RELOID, ObjectIdGetDatum(objectId)))
- return;
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("referenced relation was concurrently dropped")));
+ /* Check that the object still exists */
+ if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(objectId)))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("referenced relation was concurrently dropped")));
+
+ /* Recheck ACL */
+ recheckAcl(classId, objectId);
}
}
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 73a56f1df1d..1fad5d9a78d 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"
@@ -510,24 +511,38 @@ ProcessUtility(PlannedStmt *pstmt,
DestReceiver *dest,
QueryCompletion *qc)
{
+ TrackAclTable *prevTrackAclTable = CurrentTrackAclTable;
+
Assert(IsA(pstmt, PlannedStmt));
Assert(pstmt->commandType == CMD_UTILITY);
Assert(queryString != NULL); /* required as of 8.4 */
Assert(qc == NULL || qc->commandTag == CMDTAG_UNKNOWN);
- /*
- * We provide a function hook variable that lets loadable plugins get
- * control when ProcessUtility is called. Such a plugin would normally
- * call standard_ProcessUtility().
- */
- if (ProcessUtility_hook)
- (*ProcessUtility_hook) (pstmt, queryString, readOnlyTree,
- context, params, queryEnv,
- dest, qc);
- else
- standard_ProcessUtility(pstmt, queryString, readOnlyTree,
- context, params, queryEnv,
- dest, qc);
+ /* Allocate a fresh acl tracking table for this utility statement. */
+ CurrentTrackAclTable = CreateTrackAclTable();
+
+ PG_TRY();
+ {
+ /*
+ * We provide a function hook variable that lets loadable plugins get
+ * control when ProcessUtility is called. Such a plugin would
+ * normally call standard_ProcessUtility().
+ */
+ if (ProcessUtility_hook)
+ (*ProcessUtility_hook) (pstmt, queryString, readOnlyTree,
+ context, params, queryEnv,
+ dest, qc);
+ else
+ standard_ProcessUtility(pstmt, queryString, readOnlyTree,
+ context, params, queryEnv,
+ dest, qc);
+ }
+ PG_FINALLY();
+ {
+ FreeTrackAclTable(CurrentTrackAclTable);
+ CurrentTrackAclTable = prevTrackAclTable;
+ }
+ PG_END_TRY();
}
/*
diff --git a/src/include/catalog/aclcheck_track.h b/src/include/catalog/aclcheck_track.h
new file mode 100644
index 00000000000..d994cf7889b
--- /dev/null
+++ b/src/include/catalog/aclcheck_track.h
@@ -0,0 +1,80 @@
+/*-------------------------------------------------------------------------
+ *
+ * aclcheck_track.h
+ * Track permission checks for revalidation after lock acquisition
+ * in dependencyLockAndCheckObject().
+ *
+ * DDL may perform ACL checks on referenced objects without first holding a lock
+ * on them. In that case, the lock is acquired much later, when recording
+ * dependencies.
+ * Track the ACL checks, so that we can re-check them after acquiring the
+ * lock while recording dependencies.
+ *
+ * XXX: consider refactoring so that we perform the name lookup, acquire the
+ * lock, and check ACLs all in unison, like RangeVarGetRelidExtended().
+ *
+ * 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;
+
+typedef struct TrackAclTable
+{
+ AclCheckEntry *entries;
+ int count;
+ int max;
+} TrackAclTable;
+
+extern TrackAclTable *CurrentTrackAclTable;
+
+extern TrackAclTable *CreateTrackAclTable(void);
+extern void FreeTrackAclTable(TrackAclTable *acltable);
+
+/*
+ * 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)
+{
+ TrackAclTable *acltable = CurrentTrackAclTable;
+ AclCheckEntry *entry;
+
+ if (acltable == NULL)
+ return;
+
+ if (acltable->count >= acltable->max)
+ {
+ acltable->max *= 2;
+ acltable->entries = (AclCheckEntry *)
+ repalloc(acltable->entries, acltable->max * sizeof(AclCheckEntry));
+ }
+
+ entry = &acltable->entries[acltable->count++];
+ entry->classId = classId;
+ entry->objectId = objectId;
+ entry->roleId = roleId;
+ entry->mode = mode;
+ entry->inval_count = SharedInvalidMessageCounter;
+}
+
+#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..77cc7489170 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,65 @@ step s2_drop_role: DROP ROLE regress_dependency; <waiting ...>
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; <waiting ...>
+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
+
+starting permutation: s2_begin s2_drop_fdw_spi_func s1_create_server_spi_func s3_revoke_role s2_rollback
+step s2_begin: BEGIN;
+step s2_drop_fdw_spi_func: DROP FOREIGN DATA WRAPPER fdw_spi_func RESTRICT;
+step s1_create_server_spi_func: SET ROLE role_user; CREATE SERVER srv_spi_func FOREIGN DATA WRAPPER fdw_spi_func; RESET ROLE; <waiting ...>
+step s3_revoke_role: REVOKE role_fdw FROM role_user;
+step s2_rollback: ROLLBACK;
+step s1_create_server_spi_func: <... completed>
+ERROR: permission denied for foreign-data wrapper fdw_spi_func
+
+starting permutation: s2_begin s2_drop_fdw_spi_rollback s1_create_server_spi_rollback s3_revoke_role s2_rollback
+step s2_begin: BEGIN;
+step s2_drop_fdw_spi_rollback: DROP FOREIGN DATA WRAPPER fdw_spi_rollback RESTRICT;
+step s1_create_server_spi_rollback: SET ROLE role_user; CREATE SERVER srv_spi_rollback FOREIGN DATA WRAPPER fdw_spi_rollback; RESET ROLE; <waiting ...>
+step s3_revoke_role: REVOKE role_fdw FROM role_user;
+step s2_rollback: ROLLBACK;
+step s1_create_server_spi_rollback: <... completed>
+ERROR: permission denied for foreign-data wrapper fdw_spi_rollback
+
+starting permutation: s2_begin s2_drop_fdw_trigger s1_insert_trigger_ddl s3_revoke_role s2_rollback
+step s2_begin: BEGIN;
+step s2_drop_fdw_trigger: DROP FOREIGN DATA WRAPPER fdw_trigger RESTRICT;
+step s1_insert_trigger_ddl: SET ROLE role_user; INSERT INTO trigger_tbl VALUES (1); RESET ROLE; <waiting ...>
+step s3_revoke_role: REVOKE role_fdw FROM role_user;
+step s2_rollback: ROLLBACK;
+step s1_insert_trigger_ddl: <... completed>
+ERROR: permission denied for foreign-data wrapper fdw_trigger
+
+starting permutation: s2_begin s2_drop_fdw_trigger_spi s1_insert_trigger_spi_ddl s3_revoke_role s2_rollback
+step s2_begin: BEGIN;
+step s2_drop_fdw_trigger_spi: DROP FOREIGN DATA WRAPPER fdw_trigger_spi_func RESTRICT;
+step s1_insert_trigger_spi_ddl: SET ROLE role_user; INSERT INTO trigger_spi_tbl VALUES (1); RESET ROLE; <waiting ...>
+step s3_revoke_role: REVOKE role_fdw FROM role_user;
+step s2_rollback: ROLLBACK;
+step s1_insert_trigger_spi_ddl: <... completed>
+ERROR: permission denied for foreign-data wrapper fdw_trigger_spi_func
+
+starting permutation: s2_begin s2_drop_fdw_inner s1_create_server_nested_toctou s3_revoke_both_roles s2_rollback
+step s2_begin: BEGIN;
+step s2_drop_fdw_inner: DROP FOREIGN DATA WRAPPER fdw_inner RESTRICT;
+step s1_create_server_nested_toctou: SET ROLE role_user; CREATE SERVER srv_outer FOREIGN DATA WRAPPER fdw_outer; RESET ROLE; <waiting ...>
+step s3_revoke_both_roles: REVOKE role_fdw, role_fdw_inner FROM role_user;
+step s2_rollback: ROLLBACK;
+step s1_create_server_nested_toctou: <... completed>
+ERROR: permission denied for foreign-data wrapper fdw_inner
+
+starting permutation: s2_begin s2_drop_fdw_inner s1_create_server_nested_rollback s3_revoke_role_inner s2_rollback
+step s2_begin: BEGIN;
+step s2_drop_fdw_inner: DROP FOREIGN DATA WRAPPER fdw_inner RESTRICT;
+step s1_create_server_nested_rollback: SET ROLE role_user; CREATE SERVER srv_outer_rollback FOREIGN DATA WRAPPER fdw_outer_rollback; RESET ROLE; <waiting ...>
+step s3_revoke_role_inner: REVOKE role_fdw_inner FROM role_user;
+step s2_rollback: ROLLBACK;
+step s1_create_server_nested_rollback: <... completed>
diff --git a/src/test/isolation/specs/ddl-dependency-locking.spec b/src/test/isolation/specs/ddl-dependency-locking.spec
index de5bd88d35e..72aa97c6dfd 100644
--- a/src/test/isolation/specs/ddl-dependency-locking.spec
+++ b/src/test/isolation/specs/ddl-dependency-locking.spec
@@ -11,7 +11,71 @@ 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;
+ CREATE FUNCTION spi_func(text[], oid) RETURNS void
+ LANGUAGE plpgsql AS $$ BEGIN EXECUTE 'CREATE TEMP TABLE IF NOT EXISTS validator_side_effect(x int)'; END; $$;
+ CREATE FUNCTION spi_func_rollback(text[], oid) RETURNS void
+ LANGUAGE plpgsql AS $$
+ BEGIN
+ BEGIN
+ EXECUTE 'CREATE TEMP TABLE validator_will_rollback(x int)';
+ RAISE EXCEPTION 'force rollback';
+ EXCEPTION WHEN OTHERS THEN
+ NULL;
+ END;
+ END; $$;
+ CREATE FOREIGN DATA WRAPPER fdw_spi_func VALIDATOR spi_func;
+ CREATE FOREIGN DATA WRAPPER fdw_spi_rollback VALIDATOR spi_func_rollback;
+ GRANT USAGE ON FOREIGN DATA WRAPPER fdw_spi_func TO role_fdw;
+ GRANT USAGE ON FOREIGN DATA WRAPPER fdw_spi_rollback TO role_fdw;
+ CREATE FOREIGN DATA WRAPPER fdw_trigger;
+ GRANT USAGE ON FOREIGN DATA WRAPPER fdw_trigger TO role_fdw;
+ CREATE FUNCTION trg_create_server() RETURNS trigger
+ LANGUAGE plpgsql AS $$ BEGIN EXECUTE 'CREATE SERVER srv_from_trigger FOREIGN DATA WRAPPER fdw_trigger'; RETURN NEW; END; $$;
+ CREATE TABLE trigger_tbl(a int);
+ GRANT INSERT ON trigger_tbl TO role_user;
+ CREATE TRIGGER trg BEFORE INSERT ON trigger_tbl
+ FOR EACH ROW EXECUTE FUNCTION trg_create_server();
+ CREATE FOREIGN DATA WRAPPER fdw_trigger_spi_func VALIDATOR spi_func;
+ GRANT USAGE ON FOREIGN DATA WRAPPER fdw_trigger_spi_func TO role_fdw;
+ CREATE FUNCTION trg_create_server_spi_func() RETURNS trigger
+ LANGUAGE plpgsql AS $$ BEGIN EXECUTE 'CREATE SERVER srv_from_trigger_spi FOREIGN DATA WRAPPER fdw_trigger_spi_func'; RETURN NEW; END; $$;
+ CREATE TABLE trigger_spi_tbl(a int);
+ GRANT INSERT ON trigger_spi_tbl TO role_user;
+ CREATE TRIGGER trg BEFORE INSERT ON trigger_spi_tbl
+ FOR EACH ROW EXECUTE FUNCTION trg_create_server_spi_func();
+ CREATE FOREIGN DATA WRAPPER fdw_inner;
+ GRANT USAGE ON FOREIGN DATA WRAPPER fdw_inner TO role_fdw;
+ CREATE FUNCTION spi_func_create_server(text[], oid) RETURNS void
+ LANGUAGE plpgsql AS $$
+ BEGIN
+ IF $2 = 'pg_catalog.pg_foreign_server'::regclass::oid THEN
+ EXECUTE 'CREATE SERVER srv_nested_inner FOREIGN DATA WRAPPER fdw_inner';
+ END IF;
+ END; $$;
+ CREATE FOREIGN DATA WRAPPER fdw_outer VALIDATOR spi_func_create_server;
+ GRANT USAGE ON FOREIGN DATA WRAPPER fdw_outer TO role_fdw;
+ CREATE ROLE role_fdw_inner;
+ GRANT USAGE ON FOREIGN DATA WRAPPER fdw_inner TO role_fdw_inner;
+ GRANT role_fdw_inner TO role_user;
+ CREATE FUNCTION spi_func_create_server_rollback(text[], oid) RETURNS void
+ LANGUAGE plpgsql AS $$
+ BEGIN
+ IF $2 = 'pg_catalog.pg_foreign_server'::regclass::oid THEN
+ BEGIN
+ EXECUTE 'CREATE SERVER srv_nested_rollback FOREIGN DATA WRAPPER fdw_inner';
+ RAISE EXCEPTION 'force rollback';
+ EXCEPTION WHEN OTHERS THEN
+ NULL;
+ END;
+ END IF;
+ END; $$;
+ CREATE FOREIGN DATA WRAPPER fdw_outer_rollback VALIDATOR spi_func_create_server_rollback;
+ GRANT USAGE ON FOREIGN DATA WRAPPER fdw_outer_rollback TO role_fdw;
}
teardown
@@ -24,7 +88,19 @@ 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 SERVER IF EXISTS srv_spi_func;
+ DROP SERVER IF EXISTS srv_spi_rollback;
+ DROP SERVER IF EXISTS srv_from_trigger;
+ DROP SERVER IF EXISTS srv_from_trigger_spi;
+ DROP SERVER IF EXISTS srv_from_spi;
+ DROP SERVER IF EXISTS srv_nested_inner;
+ DROP SERVER IF EXISTS srv_nested_rollback;
+ DROP SERVER IF EXISTS srv_outer;
+ DROP SERVER IF EXISTS srv_outer_rollback;
DROP TABLE IF EXISTS tabtype;
+ DROP TABLE IF EXISTS trigger_tbl;
+ DROP TABLE IF EXISTS trigger_spi_tbl;
DROP SCHEMA IF EXISTS testschema;
DROP SCHEMA IF EXISTS alterschema;
DROP TYPE IF EXISTS public.foo;
@@ -32,6 +108,22 @@ teardown
DROP DOMAIN IF EXISTS id;
DROP FUNCTION IF EXISTS f();
DROP FOREIGN DATA WRAPPER IF EXISTS fdw_wrapper;
+ DROP FOREIGN DATA WRAPPER IF EXISTS fdw_spi_func;
+ DROP FOREIGN DATA WRAPPER IF EXISTS fdw_spi_rollback;
+ DROP FOREIGN DATA WRAPPER IF EXISTS fdw_trigger;
+ DROP FOREIGN DATA WRAPPER IF EXISTS fdw_trigger_spi_func;
+ DROP FOREIGN DATA WRAPPER IF EXISTS fdw_inner;
+ DROP FOREIGN DATA WRAPPER IF EXISTS fdw_outer;
+ DROP FOREIGN DATA WRAPPER IF EXISTS fdw_outer_rollback;
+ DROP FUNCTION IF EXISTS spi_func(text[], oid);
+ DROP FUNCTION IF EXISTS spi_func_rollback(text[], oid);
+ DROP FUNCTION IF EXISTS spi_func_create_server(text[], oid);
+ DROP FUNCTION IF EXISTS spi_func_create_server_rollback(text[], oid);
+ DROP FUNCTION IF EXISTS trg_create_server();
+ DROP FUNCTION IF EXISTS trg_create_server_spi_func();
+ DROP ROLE IF EXISTS role_user;
+ DROP ROLE IF EXISTS role_fdw;
+ DROP ROLE IF EXISTS role_fdw_inner;
DROP ROLE regress_dependency;
}
@@ -47,6 +139,13 @@ 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_create_server_spi_func" { SET ROLE role_user; CREATE SERVER srv_spi_func FOREIGN DATA WRAPPER fdw_spi_func; RESET ROLE; }
+step "s1_create_server_spi_rollback" { SET ROLE role_user; CREATE SERVER srv_spi_rollback FOREIGN DATA WRAPPER fdw_spi_rollback; RESET ROLE; }
+step "s1_insert_trigger_ddl" { SET ROLE role_user; INSERT INTO trigger_tbl VALUES (1); RESET ROLE; }
+step "s1_insert_trigger_spi_ddl" { SET ROLE role_user; INSERT INTO trigger_spi_tbl VALUES (1); RESET ROLE; }
+step "s1_create_server_nested_toctou" { SET ROLE role_user; CREATE SERVER srv_outer FOREIGN DATA WRAPPER fdw_outer; RESET ROLE; }
+step "s1_create_server_nested_rollback" { SET ROLE role_user; CREATE SERVER srv_outer_rollback FOREIGN DATA WRAPPER fdw_outer_rollback; RESET ROLE; }
step "s1_commit" { COMMIT; }
session "s2"
@@ -60,8 +159,20 @@ step "s2_drop_footab_type" { DROP TYPE public.footab; }
step "s2_drop_function_f" { DROP FUNCTION f(); }
step "s2_drop_domain_id" { DROP DOMAIN id; }
step "s2_drop_fdw_wrapper" { DROP FOREIGN DATA WRAPPER fdw_wrapper RESTRICT; }
+step "s2_drop_fdw_spi_func" { DROP FOREIGN DATA WRAPPER fdw_spi_func RESTRICT; }
+step "s2_drop_fdw_spi_rollback" { DROP FOREIGN DATA WRAPPER fdw_spi_rollback RESTRICT; }
+step "s2_drop_fdw_trigger" { DROP FOREIGN DATA WRAPPER fdw_trigger RESTRICT; }
+step "s2_drop_fdw_trigger_spi" { DROP FOREIGN DATA WRAPPER fdw_trigger_spi_func RESTRICT; }
+step "s2_drop_fdw_inner" { DROP FOREIGN DATA WRAPPER fdw_inner 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; }
+step "s3_revoke_role_inner" { REVOKE role_fdw_inner FROM role_user; }
+step "s3_revoke_both_roles" { REVOKE role_fdw, role_fdw_inner FROM role_user; }
# create function - drop schema
permutation "s1_begin" "s1_create_function_in_schema" "s2_drop_schema" "s1_commit"
@@ -102,3 +213,32 @@ permutation "s1_begin" "s1_alter_function_owner" "s2_drop_role" "s1_commit"
# <OID> 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"
+
+# Role membership TOCTOU with SPI DDL in FDW validator.
+permutation "s2_begin" "s2_drop_fdw_spi_func" "s1_create_server_spi_func" "s3_revoke_role" "s2_rollback"
+
+# Same as above but the validator's SPI DDL fails and rolls back its
+# subtransaction.
+permutation "s2_begin" "s2_drop_fdw_spi_rollback" "s1_create_server_spi_rollback" "s3_revoke_role" "s2_rollback"
+
+# Role membership TOCTOU with DDL triggered from DML: a trigger function does
+# DDL via SPI during INSERT.
+permutation "s2_begin" "s2_drop_fdw_trigger" "s1_insert_trigger_ddl" "s3_revoke_role" "s2_rollback"
+
+# Same as above but the DDL inside the trigger uses an FDW with a SPI validator,
+# combining trigger-initiated DDL with nested SPI.
+permutation "s2_begin" "s2_drop_fdw_trigger_spi" "s1_insert_trigger_spi_ddl" "s3_revoke_role" "s2_rollback"
+
+# TOCTOU on the nested SPI DDL itself: the validator creates a server using
+# fdw_inner, session 2 blocks that by dropping fdw_inner, and the REVOKE
+# removes access to fdw_inner.
+permutation "s2_begin" "s2_drop_fdw_inner" "s1_create_server_nested_toctou" "s3_revoke_both_roles" "s2_rollback"
+
+# Rolled-back nested DDL should not cause the outer to fail: the validator
+# tries to create a server using fdw_inner (via role_fdw_inner) but rolls back.
+# The REVOKE removes role_fdw_inner, but the outer CREATE SERVER only needs
+# role_fdw (for fdw_outer_rollback), so it should succeed.
+permutation "s2_begin" "s2_drop_fdw_inner" "s1_create_server_nested_rollback" "s3_revoke_role_inner" "s2_rollback"
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index f9eb23e52c9..637e15b38b3 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
@@ -3226,6 +3227,7 @@ ToastedAttribute
TocEntry
TokenAuxData
TokenizedAuthLine
+TrackAclTable
TrackItem
TransApplyAction
TransInvalidationInfo
--
2.34.1
>From 3a823115cc9756650fc46c42d3182ac707f771f1 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email protected]>
Date: Tue, 16 Jun 2026 07:31:28 +0000
Subject: [PATCH v27 2/3] Only track successful ACL checks in
aclcheck_track_record()
Previously, aclcheck_track_record() was called before the actual permission
check, so both successful and failed checks were recorded. This caused a
problem with column-level REFERENCES: checkFkeyPermissions() first tries
pg_class_aclcheck() (which fails for column-level grants), then falls through
to pg_attribute_aclcheck() (which succeeds). The tracked failed entry could
trigger a spurious 'permission denied' in recheckAcl() if catalog
invalidations arrived between the check and dependency recording.
Fix by moving aclcheck_track_record() to after the permission check succeeds
in object_aclcheck_ext() and pg_class_aclcheck_ext().
Add an injection point in checkFkeyPermissions() and a test that reproduces
the issue by injecting invalidations while the FK creation is paused after
the failed table-level check.
Author: Bertrand Drouvot <[email protected]>
Reviewed-by: Jeff Davis <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/catalog/aclchk.c | 8 ++-
src/backend/commands/tablecmds.c | 4 ++
src/test/modules/injection_points/Makefile | 3 +-
.../expected/fk_column_ref.out | 33 +++++++++++
src/test/modules/injection_points/meson.build | 1 +
.../injection_points/specs/fk_column_ref.spec | 56 +++++++++++++++++++
6 files changed, 102 insertions(+), 3 deletions(-)
8.0% src/backend/catalog/
3.2% src/backend/commands/
27.0% src/test/modules/injection_points/expected/
58.9% src/test/modules/injection_points/specs/
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index ab19db9d789..02c4489f0c4 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -3896,10 +3896,12 @@ 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)
+ {
+ aclcheck_track_record(classid, objectid, roleid, mode);
return ACLCHECK_OK;
+ }
else
return ACLCHECK_NO_PRIV;
}
@@ -4099,10 +4101,12 @@ 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)
+ {
+ aclcheck_track_record(RelationRelationId, table_oid, roleid, mode);
return ACLCHECK_OK;
+ }
else
return ACLCHECK_NO_PRIV;
}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 38f9ffcd04f..2b456907575 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -101,6 +101,7 @@
#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/fmgroids.h"
+#include "utils/injection_point.h"
#include "utils/inval.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
@@ -13929,6 +13930,9 @@ checkFkeyPermissions(Relation rel, int16 *attnums, int natts)
ACL_REFERENCES);
if (aclresult == ACLCHECK_OK)
return;
+
+ INJECTION_POINT("checkFkeyPermissions-after-table-acl-fail", NULL);
+
/* Else we must have REFERENCES on each column */
for (i = 0; i < natts; i++)
{
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index c01d2fb095c..2af9d045555 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -19,7 +19,8 @@ ISOLATION = basic \
repack_temporal_multirange \
repack_toast \
syscache-update-pruned \
- heap_lock_update
+ heap_lock_update \
+ fk_column_ref
# some isolation tests require wal_level=replica
ISOLATION_OPTS = --temp-config $(top_srcdir)/src/test/modules/injection_points/extra.conf
diff --git a/src/test/modules/injection_points/expected/fk_column_ref.out b/src/test/modules/injection_points/expected/fk_column_ref.out
new file mode 100644
index 00000000000..5f33cc1d1a2
--- /dev/null
+++ b/src/test/modules/injection_points/expected/fk_column_ref.out
@@ -0,0 +1,33 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1_attach s1_add_fk s2_invalidate_and_wakeup
+step s1_attach:
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('checkFkeyPermissions-after-table-acl-fail', 'wait');
+
+injection_points_set_local
+--------------------------
+
+(1 row)
+
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step s1_add_fk:
+ SET ROLE role_fk_tester;
+ ALTER TABLE fk_source ADD FOREIGN KEY (ref_id) REFERENCES fk_ref_target(id);
+ RESET ROLE;
+ <waiting ...>
+step s2_invalidate_and_wakeup:
+ GRANT SELECT ON produce_inval TO role_fk_tester;
+ REVOKE SELECT ON produce_inval FROM role_fk_tester;
+ SELECT injection_points_wakeup('checkFkeyPermissions-after-table-acl-fail');
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step s1_add_fk: <... completed>
diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build
index 59dba1cb023..6c7a48edf77 100644
--- a/src/test/modules/injection_points/meson.build
+++ b/src/test/modules/injection_points/meson.build
@@ -51,6 +51,7 @@ tests += {
'repack_toast',
'syscache-update-pruned',
'heap_lock_update',
+ 'fk_column_ref',
],
'runningcheck': false, # see syscache-update-pruned
# Some tests wait for all snapshots, so avoid parallel execution
diff --git a/src/test/modules/injection_points/specs/fk_column_ref.spec b/src/test/modules/injection_points/specs/fk_column_ref.spec
new file mode 100644
index 00000000000..4e9307c913b
--- /dev/null
+++ b/src/test/modules/injection_points/specs/fk_column_ref.spec
@@ -0,0 +1,56 @@
+# Test that column-level REFERENCES does not trigger a false recheck failure.
+#
+# When a user has only column-level REFERENCES (not table-level),
+# checkFkeyPermissions() calls pg_class_aclcheck() which fails then falls
+# through to pg_attribute_aclcheck() which succeeds. If catalog invalidations
+# arrive before dependency recording, recheckAcl() must not spuriously fail on
+# the tracked failed check (it should not be tracked).
+
+setup
+{
+ CREATE EXTENSION injection_points;
+ CREATE TABLE fk_ref_target(id int PRIMARY KEY);
+ INSERT INTO fk_ref_target VALUES (1);
+ CREATE TABLE produce_inval(x int);
+ CREATE ROLE role_fk_tester;
+ GRANT REFERENCES(id) ON fk_ref_target TO role_fk_tester;
+ GRANT CREATE ON SCHEMA public TO role_fk_tester;
+ GRANT SELECT ON fk_ref_target TO role_fk_tester;
+ SET ROLE role_fk_tester;
+ CREATE TABLE fk_source(ref_id int);
+ INSERT INTO fk_source VALUES (1);
+ RESET ROLE;
+}
+
+teardown
+{
+ DROP TABLE IF EXISTS fk_source;
+ DROP TABLE IF EXISTS fk_ref_target;
+ DROP TABLE IF EXISTS produce_inval;
+ REVOKE CREATE ON SCHEMA public FROM role_fk_tester;
+ DROP ROLE role_fk_tester;
+ DROP EXTENSION injection_points;
+}
+
+session "s1"
+step "s1_attach" {
+ SELECT injection_points_set_local();
+ SELECT injection_points_attach('checkFkeyPermissions-after-table-acl-fail', 'wait');
+}
+step "s1_add_fk" {
+ SET ROLE role_fk_tester;
+ ALTER TABLE fk_source ADD FOREIGN KEY (ref_id) REFERENCES fk_ref_target(id);
+ RESET ROLE;
+}
+
+session "s2"
+step "s2_invalidate_and_wakeup" {
+ GRANT SELECT ON produce_inval TO role_fk_tester;
+ REVOKE SELECT ON produce_inval FROM role_fk_tester;
+ SELECT injection_points_wakeup('checkFkeyPermissions-after-table-acl-fail');
+}
+
+# s1 attaches the wait point, then starts the FK creation which blocks
+# after the failed pg_class_aclcheck. s2 generates invalidations and
+# wakes s1. The FK creation must succeed despite the invalidations.
+permutation "s1_attach" "s1_add_fk" "s2_invalidate_and_wakeup"
--
2.34.1
>From 8665017b0bbf42d867ebebe90a80724dfdb84847 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email protected]>
Date: Tue, 16 Jun 2026 07:33:39 +0000
Subject: [PATCH v27 3/3] Add pg_attribute_aclcheck_ext to ACL tracking for
dependency recording
Add an attnum field to AclCheckEntry so that recheckAcl() can distinguish
column-level checks from table-level checks and call the appropriate recheck
function. InvalidAttrNumber means whole-object check.
This also adds tracking to pg_attribute_aclcheck_ext(), so that future DDL
paths using column-level privileges will automatically get TOCTOU protection
without needing a pre-existing lock on the table.
Currently, the only caller of pg_attribute_aclcheck_ext() that also records
dependencies on the checked object is checkFkeyPermissions(), which holds
ShareRowExclusiveLock on the referenced table. While this prevents concurrent
REVOKE ON the table itself, it does not prevent concurrent role membership
revokes. Adding tracking here provides protection against that case and future
DDL callers.
The remaining aclcheck functions (pg_parameter_aclcheck and
pg_largeobject_aclcheck_snapshot) do not need tracking: pg_parameter_aclcheck
checks GUC parameters which are not dependency targets, and
pg_largeobject_aclcheck_snapshot is only called from inv_open() and
has_largeobject_privilege(), neither of which records dependencies.
Author: Bertrand Drouvot <[email protected]>
Reviewed-by: Jeff Davis <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/catalog/aclchk.c | 7 +++++--
src/backend/catalog/pg_depend.c | 7 ++++++-
src/include/catalog/aclcheck_track.h | 8 ++++++--
3 files changed, 17 insertions(+), 5 deletions(-)
64.1% src/backend/catalog/
35.8% src/include/catalog/
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 02c4489f0c4..fb51ea2e869 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -3899,7 +3899,7 @@ object_aclcheck_ext(Oid classid, Oid objectid,
if (object_aclmask_ext(classid, objectid, roleid, mode, ACLMASK_ANY,
is_missing) != 0)
{
- aclcheck_track_record(classid, objectid, roleid, mode);
+ aclcheck_track_record(classid, objectid, InvalidAttrNumber, roleid, mode);
return ACLCHECK_OK;
}
else
@@ -3934,7 +3934,10 @@ pg_attribute_aclcheck_ext(Oid table_oid, AttrNumber attnum,
{
if (pg_attribute_aclmask_ext(table_oid, attnum, roleid, mode,
ACLMASK_ANY, is_missing) != 0)
+ {
+ aclcheck_track_record(RelationRelationId, table_oid, attnum, roleid, mode);
return ACLCHECK_OK;
+ }
else
return ACLCHECK_NO_PRIV;
}
@@ -4104,7 +4107,7 @@ pg_class_aclcheck_ext(Oid table_oid, Oid roleid,
if (pg_class_aclmask_ext(table_oid, roleid, mode,
ACLMASK_ANY, is_missing) != 0)
{
- aclcheck_track_record(RelationRelationId, table_oid, roleid, mode);
+ aclcheck_track_record(RelationRelationId, table_oid, InvalidAttrNumber, roleid, mode);
return ACLCHECK_OK;
}
else
diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c
index 912c92e8fef..88a27fdc802 100644
--- a/src/backend/catalog/pg_depend.c
+++ b/src/backend/catalog/pg_depend.c
@@ -773,7 +773,12 @@ recheckAcl(Oid classId, Oid objectId)
{
AclResult aclresult;
- if (classId == RelationRelationId)
+ if (acltable->entries[i].attnum != InvalidAttrNumber)
+ aclresult = pg_attribute_aclcheck(objectId,
+ acltable->entries[i].attnum,
+ acltable->entries[i].roleId,
+ acltable->entries[i].mode);
+ else if (classId == RelationRelationId)
aclresult = pg_class_aclcheck(objectId,
acltable->entries[i].roleId,
acltable->entries[i].mode);
diff --git a/src/include/catalog/aclcheck_track.h b/src/include/catalog/aclcheck_track.h
index d994cf7889b..4ac65e4de5c 100644
--- a/src/include/catalog/aclcheck_track.h
+++ b/src/include/catalog/aclcheck_track.h
@@ -30,6 +30,7 @@ typedef struct AclCheckEntry
{
Oid classId;
Oid objectId;
+ AttrNumber attnum;
Oid roleId;
AclMode mode;
uint64 inval_count;
@@ -50,11 +51,13 @@ extern void FreeTrackAclTable(TrackAclTable *acltable);
/*
* Record an aclcheck for later revalidation.
*
- * Called from object_aclcheck_ext() and pg_class_aclcheck_ext().
+ * Called from object_aclcheck_ext(), pg_class_aclcheck_ext(), and
+ * pg_attribute_aclcheck_ext().
* Only records when inside an utility statement.
*/
static inline void
-aclcheck_track_record(Oid classId, Oid objectId, Oid roleId, AclMode mode)
+aclcheck_track_record(Oid classId, Oid objectId, AttrNumber attnum,
+ Oid roleId, AclMode mode)
{
TrackAclTable *acltable = CurrentTrackAclTable;
AclCheckEntry *entry;
@@ -72,6 +75,7 @@ aclcheck_track_record(Oid classId, Oid objectId, Oid roleId, AclMode mode)
entry = &acltable->entries[acltable->count++];
entry->classId = classId;
entry->objectId = objectId;
+ entry->attnum = attnum;
entry->roleId = roleId;
entry->mode = mode;
entry->inval_count = SharedInvalidMessageCounter;
--
2.34.1
