public inbox for [email protected]
help / color / mirror / Atom feedFix races conditions in DropRole() and GrantRole()
4+ messages / 2 participants
[nested] [flat]
* Fix races conditions in DropRole() and GrantRole()
@ 2026-07-04 07:47 Bertrand Drouvot <[email protected]>
0 siblings, 2 replies; 4+ messages in thread
From: Bertrand Drouvot @ 2026-07-04 07:47 UTC (permalink / raw)
To: [email protected]
Hi hackers,
While working on [1], I observed that DropRole() and GrantRole() have the same
"use stale data after the lock is acquired" issues.
Indeed, DropRole() and GrantRole() resolve the role name to an OID before acquiring
LockSharedObject() on the role. A concurrent session that commits a DROP ROLE
between the read and the lock acquisition leaves the first session acting on a
stale OID.
Examples:
1/ DROP ROLE + concurrent DROP ROLE
CREATE ROLE testrole;
gdb breakpoint at user.c:1198 (before LockSharedObject) on session 1
session 1: DROP ROLE testrole;
session 1 is paused by the breakpoint
session 2: DROP ROLE testrole;
continue session 1 produces:
ERROR: could not find tuple for role 24662
2/ GRANT ROLE + concurrent DROP ROLE
CREATE ROLE testrole;
CREATE ROLE testmember;
gdb breakpoint at user.c:1716 (before LockSharedObject) on session 1
session 1: GRANT testrole TO testmember;
session is paused by the breakpoint
session 2: DROP ROLE testrole;
continue session 1: GRANT ROLE succeeds
It produces an orphaned pg_auth_members entry:
postgres=# SELECT m.member::regrole, m.roleid, r.rolname
FROM pg_auth_members m
LEFT JOIN pg_roles r ON m.roleid = r.oid
WHERE r.oid IS NULL;
member | roleid | rolname
------------+--------+---------
testmember | 16386 |
The patch attached fixes the races by using the same approach as
RangeVarGetRelidExtended(): It encapsulates name resolution, permission checking
(via a caller-supplied callback), and lock acquisition inside a retry loop driven
by SharedInvalidMessageCounter. If invalidation messages arrive between name
resolution and locking, indicating concurrent DDL, the function retries.
The lock is kept across retries and only released if the name resolves to a
different OID on the next iteration.
Two callbacks are provided:
- RoleNameCallbackForDropRole(): checks current/session user, superuser attribute,
and ADMIN OPTION privilege before locking. This is similar to what DropRole() is
currently doing before LockSharedObject().
- RoleNameCallbackForGrantRole(): calls check_role_membership_authorization() to
verify the current user can grant/revoke membership. This is similar to what GrantRole()
is currently doing before calling AddRoleMems()/DelRoleMems().
DropRole() and GrantRole() now call RoleNameGetOid() with appropriate lock
levels.
Remark:
AlterRole() does not need the fix because it calls CatalogTupleUpdate() on the
pg_authid tuple before AddRoleMems(), which blocks a concurrent DROP ROLE.
[1]: https://postgr.es/m/akZUpiDa1UfmzYxL%40bdtpg
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Fix races conditions in DropRole() and GrantRole()
@ 2026-07-06 13:22 Bertrand Drouvot <[email protected]>
parent: Bertrand Drouvot <[email protected]>
1 sibling, 0 replies; 4+ messages in thread
From: Bertrand Drouvot @ 2026-07-06 13:22 UTC (permalink / raw)
To: [email protected]; +Cc: Tom Lane <[email protected]>
Hi,
On Sat, Jul 04, 2026 at 07:47:08AM +0000, Bertrand Drouvot wrote:
> The patch attached fixes the races by using the same approach as
> RangeVarGetRelidExtended(): It encapsulates name resolution, permission checking
> (via a caller-supplied callback), and lock acquisition inside a retry loop driven
> by SharedInvalidMessageCounter. If invalidation messages arrive between name
> resolution and locking, indicating concurrent DDL, the function retries.
Also I think we could make use of the same approach to fix one of the issue that
was discussed in [1], means:
"
create role my_group;
create role dropped_member;
Session 1: begin;grant my_group to dropped_member;
Session 2: drop role dropped_member;
Session 1: commit;
"
producing an orphaned pg_auth_members entry:
postgres=# SELECT m.member, m.roleid, r.rolname
FROM pg_auth_members m
LEFT JOIN pg_roles r ON m.member = r.oid
WHERE r.oid IS NULL;
member | roleid | rolname
--------+--------+---------
16385 | 16384 |
(1 row)
0002 fixes this by acquiring AccessShareLock on each resolved role within
roleSpecsToIds(), ensuring the role cannot be dropped while any caller is using
its OID.
It also fixes other cases that would produce orphaned pg_auth_members entries,
like "ALTER GROUP my_group ADD USER dropped_member" or
"CREATE ROLE new_group ROLE dropped_member" with a concurrent DROP of
dropped_member.
Note that DropOwnedObjects() and ReassignOwnedObjects() check has_privs_of_role()
after the lock is acquired. Moving those into a callback could be done if we feel
the need.
[1]: https://postgr.es/m/CAM6Zo8woa62ZFHtMKox6a4jb8qQ%3Dw87R2L0K8347iE-juQL2EA%40mail.gmail.com
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Fix races conditions in DropRole() and GrantRole()
@ 2026-07-09 04:45 surya poondla <[email protected]>
parent: Bertrand Drouvot <[email protected]>
1 sibling, 1 reply; 4+ messages in thread
From: surya poondla @ 2026-07-09 04:45 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: [email protected]
Hi Bertrand,
Thanks for the patch set. I agree the races are real, and reusing the
RangeVarGetRelidExtended() invalidation-retry idiom is a good fit, the
retry loop looks correct and closes the window for the covered paths.
I have a few comments
1. The 0002 patch creates a new deadlock. GrantRole() now locks the member
via roleSpecsToIds() (AccessShareLock, before the granted-role loop) and
then the granted role via RoleNameGetOid()
(ShareUpdateExclusiveLock).
DROP ROLE takes AccessExclusiveLock per role in list order.
Because AccessExclusiveLock conflicts with everything, this creates a
lock-order cycle that did not exist before.
For example say we have 2 concurrent sessions:
S1: GRANT g TO m; S2: DROP ROLE g, m;
1. S1 acquires AccessShareLock(m)
2. S2 acquires AccessExclusiveLock(g)
3. S1 waits ShareUpdateExclusiveLock(g) -- blocked by S2
4. S2 waits AccessExclusiveLock(m) -- blocked by S1
-> deadlock
Pre-patch, GRANT didn't lock the member, so S2 never blocked S1.
The same expanded locking applies to CREATE ROLE ... ROLE and the ALTER
ROLE ... ADD/DROP USER (ALTER GROUP) path,
which also goes through roleSpecsToIds().
Swapping a silent orphan for a detected deadlock is arguably fine, but it's
a behavior change, could you document the lock ordering, and maybe have
DROP ROLE lock in a canonical (OID-sorted) order?
2. DROP ROLE now blocks on unrelated long transactions. The
AccessShareLock from
roleSpecsToIds() is held to commit,
so an open txn that ran GRANT/CREATE ROLE ... ROLE/REASSIGN OWNED touching
X, blocks a concurrent DROP ROLE X. This is intended behavior, but worth a
note in the commit message/docs.
3. For non-cstring role specs, the else-branch (CURRENT_USER/SESSION_USER)
does:
roleid = get_rolespec_oid(rolespec, false);
LockSharedObject(AuthIdRelationId, roleid, 0, AccessShareLock);
get_rolespec_oid() returns the backend's cached session OID (GetUserId())
rather than re-resolving a name, so there is no retry mechanism and
no post-lock existence check.
Since DropRole() only blocks dropping the *dropping* session's own user,
nothing stops another session from dropping this session's login role, so
"GRANT g TO CURRENT_USER" can still orphan.
RoleNameCallbackForDropRole() already does this by doing a re-check (a
SearchSysCache1(AUTHOID) after resolving, erroring if the tuple is gone),
so the else-branch could do the same after locking.
4. In role-membership-drop-member.spec only checks that the concurrent DROP
waits; it never asserts the outcome, so it would still pass if the locking
left an orphan. A final step selecting for orphaned rows would make it
detect outcome regressions, e.g.
SELECT m.roleid, m.member, m.grantor
FROM pg_auth_members m
LEFT JOIN pg_authid ra ON m.roleid = ra.oid
LEFT JOIN pg_authid me ON m.member = me.oid
LEFT JOIN pg_authid gr ON m.grantor = gr.oid
WHERE ra.oid IS NULL OR me.oid IS NULL OR gr.oid IS NULL;
Nit: the spec header comment has a stray '#' ("orphaned # pg_auth_members").
Regards,
Surya Poondla
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Fix races conditions in DropRole() and GrantRole()
@ 2026-07-09 07:05 Bertrand Drouvot <[email protected]>
parent: surya poondla <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Bertrand Drouvot @ 2026-07-09 07:05 UTC (permalink / raw)
To: surya poondla <[email protected]>; +Cc: [email protected]
Hi Surya,
On Wed, Jul 08, 2026 at 09:45:34PM -0700, surya poondla wrote:
> Hi Bertrand,
>
>
> Thanks for the patch set. I agree the races are real, and reusing the
> RangeVarGetRelidExtended() invalidation-retry idiom is a good fit, the
> retry loop looks correct and closes the window for the covered paths.
Thanks for looking at it!
> Swapping a silent orphan for a detected deadlock is arguably fine, but it's
> a behavior change, could you document the lock ordering
Nice catch! I think that a rare deadlock is better than a rare orphaned entry,
so I added a note in the commit message.
>, and maybe have
> DROP ROLE lock in a canonical (OID-sorted) order?
That would add extra complexity and I'm not sure that sorting only in DROP ROLE
would fully solve it.
Also, I don't think there is precedent in the code tree. So I think we should keep
it simple and just mention it in the commit message.
> 2. DROP ROLE now blocks on unrelated long transactions. The
> AccessShareLock from
> roleSpecsToIds() is held to commit,
> so an open txn that ran GRANT/CREATE ROLE ... ROLE/REASSIGN OWNED touching
> X, blocks a concurrent DROP ROLE X. This is intended behavior, but worth a
> note in the commit message/docs.
Right, added in the commit message. Not sure it's worth an addition in the doc
given that existing locking behavior for role commands is not documented there
either.
> 3. For non-cstring role specs, the else-branch (CURRENT_USER/SESSION_USER)
> does:
> roleid = get_rolespec_oid(rolespec, false);
> LockSharedObject(AuthIdRelationId, roleid, 0, AccessShareLock);
> get_rolespec_oid() returns the backend's cached session OID (GetUserId())
> rather than re-resolving a name, so there is no retry mechanism and
> no post-lock existence check.
> Since DropRole() only blocks dropping the *dropping* session's own user,
> nothing stops another session from dropping this session's login role, so
> "GRANT g TO CURRENT_USER" can still orphan.
> RoleNameCallbackForDropRole() already does this by doing a re-check (a
> SearchSysCache1(AUTHOID) after resolving, erroring if the tuple is gone),
> so the else-branch could do the same after locking.
Good point, done in the attached.
>
> 4. In role-membership-drop-member.spec only checks that the concurrent DROP
> waits; it never asserts the outcome, so it would still pass if the locking
> left an orphan.
I'm not sure how an orphan could be created if we ensure proper locking. That
said this extra check does not hurt, so added for the permutations that would
produce orphans without the patch.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2026-07-09 07:05 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-07-04 07:47 Fix races conditions in DropRole() and GrantRole() Bertrand Drouvot <[email protected]>
2026-07-06 13:22 ` Bertrand Drouvot <[email protected]>
2026-07-09 04:45 ` surya poondla <[email protected]>
2026-07-09 07:05 ` Bertrand Drouvot <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox