Re: Fix races conditions in DropRole() and GrantRole() - Mailing list pgsql-hackers
| From | surya poondla |
|---|---|
| Subject | Re: Fix races conditions in DropRole() and GrantRole() |
| Date | |
| Msg-id | CAOVWO5prVyU4-LtN2VF=hqMVrg4BmXSG=S4khhwpp-AwRFqQ-w@mail.gmail.com Whole thread |
| In response to | Fix races conditions in DropRole() and GrantRole() (Bertrand Drouvot <bertranddrouvot.pg@gmail.com>) |
| Responses |
Re: Fix races conditions in DropRole() and GrantRole()
|
| List | pgsql-hackers |
Hi Bertrand,
LockSharedObject(AuthIdRelationId, roleid, 0, AccessShareLock);
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").
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
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).
(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
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
Surya Poondla
pgsql-hackers by date: