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 6566133c5 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 <[email protected]>
Reviewed-by: Laurenz Albe <[email protected]>
Discussion: https://postgr.es/m/CAM6Zo8woa62ZFHtMKox6a4jb8qQ=w87R2L0K8347iE-juQL2EA@mail.gmail.com

diff --git a/doc/src/sgml/ref/drop_owned.sgml b/doc/src/sgml/ref/drop_owned.sgml
index efda01a39e88bc7a008d4f0df7e826f377b6a91f..46e1c229ec0fb762686f0c36b6c4522e135248f3 100644 (file)
--- a/doc/src/sgml/ref/drop_owned.sgml
+++ b/doc/src/sgml/ref/drop_owned.sgml
@@ -33,7 +33,7 @@ DROP OWNED BY { <replaceable class="parameter">name</replaceable> | CURRENT_ROLE
    database that are owned by one of the specified roles. Any
    privileges granted to the given roles on objects in the current
    database or on shared objects (databases, tablespaces, configuration
-   parameters) will also be revoked.
+   parameters, or other roles) will also be revoked.
   </para>
  </refsect1>
 

diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 0db174e6f1065dd96a7d90dd28ccd46988479589..8ae510c623b9d2c8387cb0060ba74717c5dc1933 100644 (file)
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -30,6 +30,7 @@
 #include "commands/defrem.h"
 #include "commands/seclabel.h"
 #include "commands/user.h"
+#include "lib/qunique.h"
 #include "libpq/crypt.h"
 #include "miscadmin.h"
 #include "storage/lmgr.h"
@@ -489,8 +490,7 @@ CreateRole(ParseState *pstate, CreateRoleStmt *stmt)
     * Advance command counter so we can see new record; else tests in
     * AddRoleMems may fail.
     */
-   if (addroleto || adminmembers || rolemembers)
-       CommandCounterIncrement();
+   CommandCounterIncrement();
 
    /* Default grant. */
    InitGrantRoleOptions(&popt);
@@ -1904,7 +1904,8 @@ AddRoleMems(Oid currentUserId, const char *rolename, Oid roleid,
        else
        {
            Oid         objectId;
-           Oid        *newmembers = palloc(sizeof(Oid));
+           Oid        *newmembers = (Oid *) palloc(3 * sizeof(Oid));
+           int         nnewmembers;
 
            /*
             * The values for these options can be taken directly from 'popt'.
@@ -1946,12 +1947,22 @@ AddRoleMems(Oid currentUserId, const char *rolename, Oid roleid,
                                    new_record, new_record_nulls);
            CatalogTupleInsert(pg_authmem_rel, tuple);
 
-           /* updateAclDependencies wants to pfree array inputs */
-           newmembers[0] = grantorId;
+           /*
+            * Record dependencies on the roleid, member, and grantor, as if a
+            * pg_auth_members entry were an object ACL.
+            * updateAclDependencies() requires an input array that is
+            * palloc'd (it will free it), sorted, and de-duped.
+            */
+           newmembers[0] = roleid;
+           newmembers[1] = memberid;
+           newmembers[2] = grantorId;
+           qsort(newmembers, 3, sizeof(Oid), oid_cmp);
+           nnewmembers = qunique(newmembers, 3, sizeof(Oid), oid_cmp);
+
            updateAclDependencies(AuthMemRelationId, objectId,
                                  0, InvalidOid,
                                  0, NULL,
-                                 1, newmembers);
+                                 nnewmembers, newmembers);
        }
 
        /* CCI after each change, in case there are duplicates in list */

diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 6b01313101b69b154d8583566167fd5339d6266c..a76256405fe27c73924fbd14315670f41df7f122 100644 (file)
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -113,6 +113,36 @@ CREATE USER regress_priv_user2;
 CREATE USER regress_priv_user3;
 CREATE USER regress_priv_user4;
 CREATE USER regress_priv_user5;
+-- DROP OWNED should also act on granted and granted-to roles
+GRANT regress_priv_user1 TO regress_priv_user2;
+GRANT regress_priv_user2 TO regress_priv_user3;
+SELECT roleid::regrole, member::regrole FROM pg_auth_members
+  WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole)
+  ORDER BY roleid::regrole::text;
+       roleid       |       member       
+--------------------+--------------------
+ regress_priv_user1 | regress_priv_user2
+ regress_priv_user2 | regress_priv_user3
+(2 rows)
+
+REASSIGN OWNED BY regress_priv_user2 TO regress_priv_user4;  -- no effect
+SELECT roleid::regrole, member::regrole FROM pg_auth_members
+  WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole)
+  ORDER BY roleid::regrole::text;
+       roleid       |       member       
+--------------------+--------------------
+ regress_priv_user1 | regress_priv_user2
+ regress_priv_user2 | regress_priv_user3
+(2 rows)
+
+DROP OWNED BY regress_priv_user2;  -- removes both grants
+SELECT roleid::regrole, member::regrole FROM pg_auth_members
+  WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole)
+  ORDER BY roleid::regrole::text;
+ roleid | member 
+--------+--------
+(0 rows)
+
 GRANT pg_read_all_data TO regress_priv_user6;
 GRANT pg_write_all_data TO regress_priv_user7;
 GRANT pg_read_all_settings TO regress_priv_user8 WITH ADMIN OPTION;

diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 60e7443bf5927db08d2fe9c34aa7f5329e782b04..d195aaf137741ff26503741f3e752e37e06aa728 100644 (file)
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -90,6 +90,21 @@ CREATE USER regress_priv_user3;
 CREATE USER regress_priv_user4;
 CREATE USER regress_priv_user5;
 
+-- DROP OWNED should also act on granted and granted-to roles
+GRANT regress_priv_user1 TO regress_priv_user2;
+GRANT regress_priv_user2 TO regress_priv_user3;
+SELECT roleid::regrole, member::regrole FROM pg_auth_members
+  WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole)
+  ORDER BY roleid::regrole::text;
+REASSIGN OWNED BY regress_priv_user2 TO regress_priv_user4;  -- no effect
+SELECT roleid::regrole, member::regrole FROM pg_auth_members
+  WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole)
+  ORDER BY roleid::regrole::text;
+DROP OWNED BY regress_priv_user2;  -- removes both grants
+SELECT roleid::regrole, member::regrole FROM pg_auth_members
+  WHERE roleid IN ('regress_priv_user1'::regrole,'regress_priv_user2'::regrole)
+  ORDER BY roleid::regrole::text;
+
 GRANT pg_read_all_data TO regress_priv_user6;
 GRANT pg_write_all_data TO regress_priv_user7;
 GRANT pg_read_all_settings TO regress_priv_user8 WITH ADMIN OPTION;