MDEV-12366: FLUSH PRIVILEGES can break hierarchy of roles

Whenever we call merge_role_privileges on a role, we make use of
the role->counter variable to check if all it's children have had their
privileges merged. Only if all children have had their privileges merged,
do we update the privileges on parent. This is done to prevent extra work.
The same idea is employed during flush privileges. You only begin merging
from "leaf" roles. The recursive calls will merge their parents at some point.
A problem arises when we try to "re-merge" a parent. Take the following graph:

{noformat}
     A (0)  ----  C (2) ---- D (2)  ---- USER
                 /          /
     B (0)  ----/          /
                          /
     E (0) --------------/
{noformat}

In parentheses we have the "counter" value right before we start to iterate
through the roles hash and propagate values. It represents the number of roles
granted to the current role. The order in which we iterate through the roles
hash is alphabetical.

* First merge A, which leads to decreasing the counter for C to 1. Since C is
not 0, we don't proceed with merging into C.

* Second we merge B, which leads to decreasing the counter for C to 0. Now
we proceed with merging into C. This leads to reducing the counter for D to 1
as part of C merge process.

* Third as we iterate through the hash, we see that C has counter 0, thus we
start the merge process *again*. This leads to reducing the counter for
D to 0! We then attempt to merge D.

* Fourth we start merging E. When E sees D as it's parent (according to the code)
it attempts to reduce D's counter, which leads to overflow. Now D's counter is
a very large number, thus E's privileges are not forwarded to D yet.

To correct this behavior we must make sure to only start merging from initial
leaf nodes.

1 files changed, 18 insertions, 12 deletions

diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc
index 1b25871b664..27292105dde 100644
--- a/sql/sql_acl.cc
+++ b/sql/sql_acl.cc
@@ -5362,6 +5362,7 @@ static int merge_role_privileges(ACL_ROLE *role __attribute__((unused)),
 {
   PRIVS_TO_MERGE *data= (PRIVS_TO_MERGE *)context;
 
+  DBUG_ASSERT(grantee->counter > 0);
   if (--grantee->counter)
     return 1; // don't recurse into grantee just yet
 
@@ -6554,16 +6555,14 @@ end_index_init:
   DBUG_RETURN(return_val);
 }
 
-
-my_bool role_propagate_grants_action(void *ptr, void *unused __attribute__((unused)))
+static my_bool collect_leaf_roles(void *role_ptr,
+                                  void *roles_array)
 {
-  ACL_ROLE *role= (ACL_ROLE *)ptr;
-  if (role->counter)
-    return 0;
-
-  mysql_mutex_assert_owner(&acl_cache->lock);
-  PRIVS_TO_MERGE data= { PRIVS_TO_MERGE::ALL, 0, 0 };
-  traverse_role_graph_up(role, &data, NULL, merge_role_privileges);
+  ACL_ROLE *role= static_cast<ACL_ROLE *>(role_ptr);
+  Dynamic_array<ACL_ROLE *> *array=
+    static_cast<Dynamic_array<ACL_ROLE *> *>(roles_array);
+  if (!role->counter)
+    array->push(role);
   return 0;
 }
 
@@ -6614,7 +6613,7 @@ my_bool grant_reload(THD *thd)
     obtaining LOCK_grant rwlock.
   */
   if (open_and_lock_tables(thd, tables, FALSE, MYSQL_LOCK_IGNORE_TIMEOUT))
-    goto end;
+    DBUG_RETURN(1);
 
   mysql_rwlock_wrlock(&LOCK_grant);
   grant_version++;
@@ -6646,14 +6645,21 @@ my_bool grant_reload(THD *thd)
   }
 
   mysql_mutex_lock(&acl_cache->lock);
-  my_hash_iterate(&acl_roles, role_propagate_grants_action, NULL);
+  Dynamic_array<ACL_ROLE *> leaf_roles;
+  my_hash_iterate(&acl_roles, collect_leaf_roles, &leaf_roles);
+  PRIVS_TO_MERGE data= { PRIVS_TO_MERGE::ALL, 0, 0 };
+  for (size_t i= 0; i < leaf_roles.elements(); i++)
+  {
+    traverse_role_graph_up(leaf_roles.at(i), &data, NULL,
+                           merge_role_privileges);
+  }
+
   mysql_mutex_unlock(&acl_cache->lock);
 
   mysql_rwlock_unlock(&LOCK_grant);
 
   close_mysql_tables(thd);
 
-end:
   DBUG_RETURN(return_val);
 }