From 3a823115cc9756650fc46c42d3182ac707f771f1 Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot 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 Reviewed-by: Jeff Davis Discussion: https://postgr.es/m/ZiYjn0eVc7pxVY45@ip-10-97-1-34.eu-west-3.compute.internal --- 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; + +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