Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 98fc31d

Browse files
committed
Avoid race condition between "GRANT role" and "DROP ROLE".
Concurrently dropping either the granted role or the grantee does not stop GRANT from completing, instead resulting in a dangling role reference in pg_auth_members. That's relatively harmless in the short run, but inconsistent catalog entries are not a good thing. This patch solves the problem by adding the granted and grantee roles as explicit shared dependencies of the pg_auth_members entry. That's a bit indirect, but it works because the pg_shdepend code applies the necessary locking and rechecking. Commit 6566133 previously established similar handling for the grantor column of pg_auth_members; it's not clear why it didn't cover the other two role OID columns. A side-effect of this approach is that DROP OWNED BY will now drop pg_auth_members entries that mention the target role as either the granted or grantee role. That's clearly appropriate for the grantee, since we'll drop its other privileges too. It doesn't seem too far out of line for the granted role, since we're presumably about to drop it and besides we're removing all reasons why it'd matter to be a member of it. (One could argue that this makes DropRole's code to auto-drop pg_auth_members entries unnecessary, but I chose to leave it in place since perhaps some people's workflows expect that to work without a DROP OWNED BY.) Note to patch readers: CreateRole's first CommandCounterIncrement call is now unconditional, because this change creates another case in which it's needed, and it seemed to be more trouble than it's worth to preserve that micro-optimization. Arguably this is a bug fix, but the fact that it changes the expected contents of pg_shdepend seems like not a great thing to do in the stable branches, and perhaps we don't want the change in DROP OWNED BY semantics there either. On the other hand, I opted not to force a catversion bump in HEAD, because the presence or absence of these entries doesn't matter for most purposes. Reported-by: Virender Singla <virender.cse@gmail.com> Reviewed-by: Laurenz Albe <laurenz.albe@cybertec.at> Discussion: https://postgr.es/m/CAM6Zo8woa62ZFHtMKox6a4jb8qQ=w87R2L0K8347iE-juQL2EA@mail.gmail.com
1 parent ddb17e3 commit 98fc31d

File tree

4 files changed

+63
-7
lines changed

4 files changed

+63
-7
lines changed

doc/src/sgml/ref/drop_owned.sgml

+1-1
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ DROP OWNED BY { <replaceable class="parameter">name</replaceable> | CURRENT_ROLE
3333
database that are owned by one of the specified roles. Any
3434
privileges granted to the given roles on objects in the current
3535
database or on shared objects (databases, tablespaces, configuration
36-
parameters) will also be revoked.
36+
parameters, or other roles) will also be revoked.
3737
</para>
3838
</refsect1>
3939

src/backend/commands/user.c

+17-6
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
#include "commands/defrem.h"
3131
#include "commands/seclabel.h"
3232
#include "commands/user.h"
33+
#include "lib/qunique.h"
3334
#include "libpq/crypt.h"
3435
#include "miscadmin.h"
3536
#include "storage/lmgr.h"
@@ -489,8 +490,7 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
489490
* Advance command counter so we can see new record; else tests in
490491
* AddRoleMems may fail.
491492
*/
492-
if (addroleto || adminmembers || rolemembers)
493-
CommandCounterIncrement();
493+
CommandCounterIncrement();
494494

495495
/* Default grant. */
496496
InitGrantRoleOptions(&popt);
@@ -1904,7 +1904,8 @@ AddRoleMems(Oid currentUserId, const char *rolename, Oid roleid,
19041904
else
19051905
{
19061906
Oid objectId;
1907-
Oid *newmembers = palloc(sizeof(Oid));
1907+
Oid *newmembers = (Oid *) palloc(3 * sizeof(Oid));
1908+
int nnewmembers;
19081909

19091910
/*
19101911
* The values for these options can be taken directly from 'popt'.
@@ -1946,12 +1947,22 @@ AddRoleMems(Oid currentUserId, const char *rolename, Oid roleid,
19461947
new_record, new_record_nulls);
19471948
CatalogTupleInsert(pg_authmem_rel, tuple);
19481949

1949-
/* updateAclDependencies wants to pfree array inputs */
1950-
newmembers[0] = grantorId;
1950+
/*
1951+
* Record dependencies on the roleid, member, and grantor, as if a
1952+
* pg_auth_members entry were an object ACL.
1953+
* updateAclDependencies() requires an input array that is
1954+
* palloc'd (it will free it), sorted, and de-duped.
1955+
*/
1956+
newmembers[0] = roleid;
1957+
newmembers[1] = memberid;
1958+
newmembers[2] = grantorId;
1959+
qsort(newmembers, 3, sizeof(Oid), oid_cmp);
1960+
nnewmembers = qunique(newmembers, 3, sizeof(Oid), oid_cmp);
1961+
19511962
updateAclDependencies(AuthMemRelationId, objectId,
19521963
0, InvalidOid,
19531964
0, NULL,
1954-
1, newmembers);
1965+
nnewmembers, newmembers);
19551966
}
19561967

19571968
/* CCI after each change, in case there are duplicates in list */

src/test/regress/expected/privileges.out

+30
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,36 @@ CREATE USER regress_priv_user2;
113113
CREATE USER regress_priv_user3;
114114
CREATE USER regress_priv_user4;
115115
CREATE USER regress_priv_user5;
116+
-- DROP OWNED should also act on granted and granted-to roles
117+
GRANT regress_priv_user1 TO regress_priv_user2;
118+
GRANT regress_priv_user2 TO regress_priv_user3;
119+
SELECT roleid::regrole, member::regrole FROM pg_auth_members
120+
WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole)
121+
ORDER BY roleid::regrole::text;
122+
roleid | member
123+
--------------------+--------------------
124+
regress_priv_user1 | regress_priv_user2
125+
regress_priv_user2 | regress_priv_user3
126+
(2 rows)
127+
128+
REASSIGN OWNED BY regress_priv_user2 TO regress_priv_user4; -- no effect
129+
SELECT roleid::regrole, member::regrole FROM pg_auth_members
130+
WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole)
131+
ORDER BY roleid::regrole::text;
132+
roleid | member
133+
--------------------+--------------------
134+
regress_priv_user1 | regress_priv_user2
135+
regress_priv_user2 | regress_priv_user3
136+
(2 rows)
137+
138+
DROP OWNED BY regress_priv_user2; -- removes both grants
139+
SELECT roleid::regrole, member::regrole FROM pg_auth_members
140+
WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole)
141+
ORDER BY roleid::regrole::text;
142+
roleid | member
143+
--------+--------
144+
(0 rows)
145+
116146
GRANT pg_read_all_data TO regress_priv_user6;
117147
GRANT pg_write_all_data TO regress_priv_user7;
118148
GRANT pg_read_all_settings TO regress_priv_user8 WITH ADMIN OPTION;

src/test/regress/sql/privileges.sql

+15
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,21 @@ CREATE USER regress_priv_user3;
9090
CREATE USER regress_priv_user4;
9191
CREATE USER regress_priv_user5;
9292

93+
-- DROP OWNED should also act on granted and granted-to roles
94+
GRANT regress_priv_user1 TO regress_priv_user2;
95+
GRANT regress_priv_user2 TO regress_priv_user3;
96+
SELECT roleid::regrole, member::regrole FROM pg_auth_members
97+
WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole)
98+
ORDER BY roleid::regrole::text;
99+
REASSIGN OWNED BY regress_priv_user2 TO regress_priv_user4; -- no effect
100+
SELECT roleid::regrole, member::regrole FROM pg_auth_members
101+
WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole)
102+
ORDER BY roleid::regrole::text;
103+
DROP OWNED BY regress_priv_user2; -- removes both grants
104+
SELECT roleid::regrole, member::regrole FROM pg_auth_members
105+
WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole)
106+
ORDER BY roleid::regrole::text;
107+
93108
GRANT pg_read_all_data TO regress_priv_user6;
94109
GRANT pg_write_all_data TO regress_priv_user7;
95110
GRANT pg_read_all_settings TO regress_priv_user8 WITH ADMIN OPTION;

0 commit comments

Comments
 (0)