From 87963d50b0416be397d0da9789c0c8efce733fe3 Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Wed, 22 Jan 2020 15:43:41 +0900 Subject: [PATCH 2/2] Don't check child's LOCK privilege when locked recursively --- src/backend/commands/lockcmds.c | 33 +++++++++++--------------------- src/test/regress/expected/lock.out | 8 ++------ src/test/regress/expected/privileges.out | 7 +++++++ src/test/regress/sql/lock.sql | 7 ++----- src/test/regress/sql/privileges.sql | 6 ++++++ 5 files changed, 28 insertions(+), 33 deletions(-) diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c index 329ab849c0..d8cafc42bb 100644 --- a/src/backend/commands/lockcmds.c +++ b/src/backend/commands/lockcmds.c @@ -28,7 +28,7 @@ #include "utils/lsyscache.h" #include "utils/syscache.h" -static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid); +static void LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait); static AclResult LockTableAclCheck(Oid relid, LOCKMODE lockmode, Oid userid); static void RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid, void *arg); @@ -59,7 +59,7 @@ LockTableCommand(LockStmt *lockstmt) if (get_rel_relkind(reloid) == RELKIND_VIEW) LockViewRecurse(reloid, lockstmt->mode, lockstmt->nowait, NIL); else if (recurse) - LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait, GetUserId()); + LockTableRecurse(reloid, lockstmt->mode, lockstmt->nowait); } } @@ -108,35 +108,26 @@ RangeVarCallbackForLockTable(const RangeVar *rv, Oid relid, Oid oldrelid, /* * Apply LOCK TABLE recursively over an inheritance tree * - * We use find_inheritance_children not find_all_inheritors to avoid taking - * locks far in advance of checking privileges. This means we'll visit - * multiply-inheriting children more than once, but that's no problem. + * This doesn't check permission to perform LOCK TABLE on the child tables, + * because getting here means that the user has permission to lock the + * parent which is enough. */ static void -LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid) +LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait) { List *children; ListCell *lc; - children = find_inheritance_children(reloid, NoLock); + children = find_all_inheritors(reloid, NoLock, NULL); foreach(lc, children) { Oid childreloid = lfirst_oid(lc); - AclResult aclresult; - /* Check permissions before acquiring the lock. */ - aclresult = LockTableAclCheck(childreloid, lockmode, userid); - if (aclresult != ACLCHECK_OK) - { - char *relname = get_rel_name(childreloid); - - if (!relname) - continue; /* child concurrently dropped, just skip it */ - aclcheck_error(aclresult, get_relkind_objtype(get_rel_relkind(childreloid)), relname); - } + /* Parent already locked. */ + if (childreloid == reloid) + continue; - /* We have enough rights to lock the relation; do so. */ if (!nowait) LockRelationOid(childreloid, lockmode); else if (!ConditionalLockRelationOid(childreloid, lockmode)) @@ -162,8 +153,6 @@ LockTableRecurse(Oid reloid, LOCKMODE lockmode, bool nowait, Oid userid) UnlockRelationOid(childreloid, lockmode); continue; } - - LockTableRecurse(childreloid, lockmode, nowait, userid); } } @@ -241,7 +230,7 @@ LockViewRecurse_walker(Node *node, LockViewRecurse_context *context) if (relkind == RELKIND_VIEW) LockViewRecurse(relid, context->lockmode, context->nowait, context->ancestor_views); else if (rte->inh) - LockTableRecurse(relid, context->lockmode, context->nowait, context->viewowner); + LockTableRecurse(relid, context->lockmode, context->nowait); } return query_tree_walker(query, diff --git a/src/test/regress/expected/lock.out b/src/test/regress/expected/lock.out index 185fd2f879..5f20b846c9 100644 --- a/src/test/regress/expected/lock.out +++ b/src/test/regress/expected/lock.out @@ -138,16 +138,12 @@ CREATE TABLE lock_tbl3 () INHERITS (lock_tbl2); BEGIN TRANSACTION; LOCK TABLE lock_tbl1 * IN ACCESS EXCLUSIVE MODE; ROLLBACK; --- Verify that we can't lock a child table just because we have permission --- on the parent, but that we can lock the parent only. +-- Verify that we can lock children too as long as we have permission +-- on the parent. GRANT UPDATE ON TABLE lock_tbl1 TO regress_rol_lock1; SET ROLE regress_rol_lock1; BEGIN; LOCK TABLE lock_tbl1 * IN ACCESS EXCLUSIVE MODE; -ERROR: permission denied for table lock_tbl2 -ROLLBACK; -BEGIN; -LOCK TABLE ONLY lock_tbl1; ROLLBACK; RESET ROLE; -- diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out index bc8e198097..43d50d0c54 100644 --- a/src/test/regress/expected/privileges.out +++ b/src/test/regress/expected/privileges.out @@ -716,6 +716,13 @@ ERROR: permission denied for table atestc TRUNCATE atestp1; -- ok TRUNCATE atestc; -- fail ERROR: permission denied for table atestc +BEGIN; +LOCK atestp1; +END; +BEGIN; +LOCK atestc; +ERROR: permission denied for table atestc +END; -- privileges on functions, languages -- switch to superuser \c - diff --git a/src/test/regress/sql/lock.sql b/src/test/regress/sql/lock.sql index 26a7e59a13..22db0d3690 100644 --- a/src/test/regress/sql/lock.sql +++ b/src/test/regress/sql/lock.sql @@ -101,16 +101,13 @@ BEGIN TRANSACTION; LOCK TABLE lock_tbl1 * IN ACCESS EXCLUSIVE MODE; ROLLBACK; --- Verify that we can't lock a child table just because we have permission --- on the parent, but that we can lock the parent only. +-- Verify that we can lock children too as long as we have permission +-- on the parent. GRANT UPDATE ON TABLE lock_tbl1 TO regress_rol_lock1; SET ROLE regress_rol_lock1; BEGIN; LOCK TABLE lock_tbl1 * IN ACCESS EXCLUSIVE MODE; ROLLBACK; -BEGIN; -LOCK TABLE ONLY lock_tbl1; -ROLLBACK; RESET ROLE; -- diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql index dfe2603fe2..2ba69617dc 100644 --- a/src/test/regress/sql/privileges.sql +++ b/src/test/regress/sql/privileges.sql @@ -459,6 +459,12 @@ UPDATE atestp1 SET f1 = 1; -- ok UPDATE atestc SET f1 = 1; -- fail TRUNCATE atestp1; -- ok TRUNCATE atestc; -- fail +BEGIN; +LOCK atestp1; +END; +BEGIN; +LOCK atestc; +END; -- privileges on functions, languages -- 2.16.5