BUG#13864642: DROP/CREATE USER BEHAVING ODDLY

BACKGROUND:
In certain situations DROP USER fails to remove all privileges
belonging to user being dropped from in-memory structures.
Current workaround is to do DROP USER twice in scenario below
OR doing FLUSH PRIVILEGES after doing DROP USER.

ANALYSIS:
In MySQL, When we grant some stored routines privileges to a
user they are stored in their respective hash.
When doing DROP USER all the stored routine privilege entries
associated with that user has to be deleted from its respective 
hash.
The root cause for this bug is some entries from the hash
are not getting deleted. 
The problem is that code that deletes entries from the hash tries
to do so while iterating over it, without taking enough measures
to address the fact that such deletion can reshuffle elements in 
the hash. If the user/administrator creates the same user again 
he is thrown an  error 'Error 1396 ER_CANNOT_USER' from MySQL.
This prompts the user to either do FLUSH PRIVILEGES or do DROP USER 
again. This behaviour is not desirable as it is a workaround and
does not solves the problem mentioned above.

FIX:
This bug is fixed by introducing a dynamic array to store the 
pointersto all stored routine privilege objects that either have
to be deleted or updated. This is done in 3 steps.
Step 1: Fetching the element from the hash and checking whether 
it is to be deleted or updated.
Step 2: Storing the pointer to that privilege object in dynamic array.
Step 3: Traversing the dynamic array to perform the appropriate action 
either delete or update.
This is a much cleaner way to delete or update the privilege entries 
associated with some user and solves the problem mentioned above.
Also the code has been refactored a bit by introducing an enum
instead of hard coded numbers used for respective dynamic arrays 
and hashes in handle_grant_struct() function.

sql/sql_acl.cc

@@ -195,7 +195,17 @@ static bool compare_hostname(const acl_host_and_ip *host,const char *hostname,
 static my_bool acl_load(THD *thd, TABLE_LIST *tables);
 static my_bool grant_load(THD *thd, TABLE_LIST *tables);
 static inline void get_grantor(THD *thd, char* grantor);
-
+/*
+ Enumeration of various ACL's and Hashes used in handle_grant_struct()
+*/
+enum enum_acl_lists
+{
+  USER_ACL= 0,
+  DB_ACL,
+  COLUMN_PRIVILEGES_HASH,
+  PROC_PRIVILEGES_HASH,
+  FUNC_PRIVILEGES_HASH
+};
 /*
   Convert scrambled password to binary form, according to scramble type, 
   Binary form is stored in user.salt.
@@ -5390,19 +5400,19 @@ static int handle_grant_table(TABLE_LIST *tables, uint table_no, bool drop,
     Delete from grant structure if drop is true.
     Update in grant structure if drop is false and user_to is not NULL.
     Search in grant structure if drop is false and user_to is NULL.
-    Structures are numbered as follows:
-    0 acl_users
-    1 acl_dbs
-    2 column_priv_hash
-    3 proc_priv_hash
-    4 func_priv_hash
+    Structures are enumerated as follows:
+    0 ACL_USER
+    1 ACL_DB
+    2 COLUMN_PRIVILEGES_HASH
+    3 PROC_PRIVILEGES_HASH
+    4 FUNC_PRIVILEGES_HASH
 
   @retval > 0  At least one element matched.
   @retval 0    OK, but no element matched.
-  @retval -1   Wrong arguments to function.
+  @retval -1   Wrong arguments to function or Out of Memory
 */
 
-static int handle_grant_struct(uint struct_no, bool drop,
+static int handle_grant_struct(enum enum_acl_lists struct_no, bool drop,
                                LEX_USER *user_from, LEX_USER *user_to)
 {
   int result= 0;
@@ -5413,6 +5423,11 @@ static int handle_grant_struct(uint struct_no, bool drop,
   ACL_USER *acl_user= NULL;
   ACL_DB *acl_db= NULL;
   GRANT_NAME *grant_name= NULL;
+  /*
+    Dynamic array acl_grant_name used to store pointers to all
+    GRANT_NAME objects
+  */
+  Dynamic_array<GRANT_NAME *> acl_grant_name;
   HASH *grant_name_hash= NULL;
   DBUG_ENTER("handle_grant_struct");
   DBUG_PRINT("info",("scan struct: %u  search: '%s'@'%s'",
@@ -5425,21 +5440,21 @@ static int handle_grant_struct(uint struct_no, bool drop,
 
   /* Get the number of elements in the in-memory structure. */
   switch (struct_no) {
-  case 0:
+  case USER_ACL:
     elements= acl_users.elements;
     break;
-  case 1:
+  case DB_ACL:
     elements= acl_dbs.elements;
     break;
-  case 2:
+  case COLUMN_PRIVILEGES_HASH:
     elements= column_priv_hash.records;
     grant_name_hash= &column_priv_hash;
     break;
-  case 3:
+  case PROC_PRIVILEGES_HASH:
     elements= proc_priv_hash.records;
     grant_name_hash= &proc_priv_hash;
     break;
-  case 4:
+  case FUNC_PRIVILEGES_HASH:
     elements= func_priv_hash.records;
     grant_name_hash= &func_priv_hash;
     break;
@@ -5458,21 +5473,21 @@ static int handle_grant_struct(uint struct_no, bool drop,
       Get a pointer to the element.
     */
     switch (struct_no) {
-    case 0:
+    case USER_ACL:
       acl_user= dynamic_element(&acl_users, idx, ACL_USER*);
       user= acl_user->user;
       host= acl_user->host.hostname;
     break;
 
-    case 1:
+    case DB_ACL:
       acl_db= dynamic_element(&acl_dbs, idx, ACL_DB*);
       user= acl_db->user;
       host= acl_db->host.hostname;
       break;
 
-    case 2:
-    case 3:
-    case 4:
+    case COLUMN_PRIVILEGES_HASH:
+    case PROC_PRIVILEGES_HASH:
+    case FUNC_PRIVILEGES_HASH:
       grant_name= (GRANT_NAME*) hash_element(grant_name_hash, idx);
       user= grant_name->user;
       host= grant_name->host.hostname;
@@ -5498,52 +5513,84 @@ static int handle_grant_struct(uint struct_no, bool drop,
     if ( drop )
     {
       switch ( struct_no ) {
-      case 0:
+      case USER_ACL:
         delete_dynamic_element(&acl_users, idx);
-        break;
-
-      case 1:
-        delete_dynamic_element(&acl_dbs, idx);
-        break;
-
-      case 2:
-      case 3:
-      case 4:
-        hash_delete(grant_name_hash, (uchar*) grant_name);
-	break;
-      }
         elements--;
         /*
         - If we are iterating through an array then we just have moved all
           elements after the current element one position closer to its head.
           This means that we have to take another look at the element at
           current position as it is a new element from the array's tail.
-        - If we are iterating through a hash the current element was replaced
-          with one of elements from the tail. So we also have to take a look
-          at the new element in current position.
-          Note that in our HASH implementation hash_delete() won't move any
-          elements with position after current one to position before the
-          current (i.e. from the tail to the head), so it is safe to continue
-          iteration without re-starting.
+        - This is valid for USER_ACL, DB_ACL.
         */
         idx--;
+        break;
+
+      case DB_ACL:
+        delete_dynamic_element(&acl_dbs, idx);
+        elements--;
+        idx--;
+        break;
+
+      case COLUMN_PRIVILEGES_HASH:
+      case PROC_PRIVILEGES_HASH:
+      case FUNC_PRIVILEGES_HASH:
+        /*
+          Deleting while traversing a hash table is not valid procedure and
+          hence we save pointers to GRANT_NAME objects for later processing.
+        */
+        if (acl_grant_name.append(grant_name))
+          DBUG_RETURN(-1);
+	break;
+      }
     }
     else if ( user_to )
     {
       switch ( struct_no ) {
-      case 0:
+      case USER_ACL:
         acl_user->user= strdup_root(&mem, user_to->user.str);
         acl_user->host.hostname= strdup_root(&mem, user_to->host.str);
         break;
 
-      case 1:
+      case DB_ACL:
         acl_db->user= strdup_root(&mem, user_to->user.str);
         acl_db->host.hostname= strdup_root(&mem, user_to->host.str);
         break;
 
-      case 2:
-      case 3:
-      case 4:
+      case COLUMN_PRIVILEGES_HASH:
+      case PROC_PRIVILEGES_HASH:
+      case FUNC_PRIVILEGES_HASH:
+        /*
+          Updating while traversing a hash table is not valid procedure and
+          hence we save pointers to GRANT_NAME objects for later processing.
+        */
+        if (acl_grant_name.append(grant_name))
+          DBUG_RETURN(-1);
+        break;
+      }
+    }
+    else
+    {
+      /* If search is requested, we do not need to search further. */
+      break;
+    }
+  }
+
+  if (drop || user_to)
+  {
+    /*
+      Traversing the elements stored in acl_grant_name dynamic array
+      to either delete or update them.
+    */
+    for (int i= 0; i < acl_grant_name.elements(); ++i)
+    {
+      grant_name= acl_grant_name.at(i);
+
+      if (drop)
+      {
+        my_hash_delete(grant_name_hash, (uchar *) grant_name);
+      }
+      else
       {
         /*
           Save old hash key and its length to be able properly update
@@ -5564,28 +5611,12 @@ static int handle_grant_struct(uint struct_no, bool drop,
           is renamed, the hash key is changed. Update the hash to
           ensure that the position matches the new hash key value
         */
-          hash_update(grant_name_hash, (uchar*) grant_name, (uchar*) old_key,
+        my_hash_update(grant_name_hash, (uchar*) grant_name, (uchar*) old_key,
                        old_key_length);
-          /*
-            hash_update() operation could have moved element from the tail
-            of the hash to the current position. So we need to take a look
-            at the element in current position once again.
-            Thanks to the fact that hash_update() for our HASH implementation
-            won't move any elements from the tail of the hash to the positions
-            before the current one (a.k.a. head) it is safe to continue
-            iteration without restarting.
-          */
-          idx--;
-          break;
-        }
-      }
       }
-    else
-    {
-      /* If search is requested, we do not need to search further. */
-      break;
     }
   }
+
 #ifdef EXTRA_DEBUG
   DBUG_PRINT("loop",("scan struct: %u  result %d", struct_no, result));
 #endif
@@ -5623,6 +5654,7 @@ static int handle_grant_data(TABLE_LIST *tables, bool drop,
 {
   int result= 0;
   int found;
+  int ret;
   DBUG_ENTER("handle_grant_data");
 
   /* Handle user table. */
@@ -5634,14 +5666,19 @@ static int handle_grant_data(TABLE_LIST *tables, bool drop,
   else
   {
     /* Handle user array. */
-    if ((handle_grant_struct(0, drop, user_from, user_to) && ! result) ||
-        found)
+    if (((ret= handle_grant_struct(USER_ACL, drop, user_from, user_to) > 0) &&
+         ! result) || found)
     {
       result= 1; /* At least one record/element found. */
       /* If search is requested, we do not need to search further. */
       if (! drop && ! user_to)
         goto end;
     }
+    else if (ret < 0)
+    {
+      result= -1;
+      goto end;
+    }
   }
 
   /* Handle db table. */
@@ -5653,14 +5690,19 @@ static int handle_grant_data(TABLE_LIST *tables, bool drop,
   else
   {
     /* Handle db array. */
-    if (((handle_grant_struct(1, drop, user_from, user_to) && ! result) ||
-         found) && ! result)
+    if ((((ret= handle_grant_struct(DB_ACL, drop, user_from, user_to) > 0) &&
+          ! result) || found) && ! result)
     {
       result= 1; /* At least one record/element found. */
       /* If search is requested, we do not need to search further. */
       if (! drop && ! user_to)
         goto end;
     }
+    else if (ret < 0)
+    {
+      result= -1;
+      goto end;
+    }
   }
 
   /* Handle stored routines table. */
@@ -5672,23 +5714,35 @@ static int handle_grant_data(TABLE_LIST *tables, bool drop,
   else
   {
     /* Handle procs array. */
-    if (((handle_grant_struct(3, drop, user_from, user_to) && ! result) ||
-         found) && ! result)
+    if ((((ret= handle_grant_struct(PROC_PRIVILEGES_HASH, drop, user_from,
+                                    user_to) > 0) && ! result) || found) &&
+        ! result)
     {
       result= 1; /* At least one record/element found. */
       /* If search is requested, we do not need to search further. */
       if (! drop && ! user_to)
         goto end;
     }
+    else if (ret < 0)
+    {
+      result= -1;
+      goto end;
+    }
     /* Handle funcs array. */
-    if (((handle_grant_struct(4, drop, user_from, user_to) && ! result) ||
-         found) && ! result)
+    if ((((ret= handle_grant_struct(FUNC_PRIVILEGES_HASH, drop, user_from,
+                                    user_to) > 0) && ! result) || found) &&
+        ! result)
     {
       result= 1; /* At least one record/element found. */
       /* If search is requested, we do not need to search further. */
       if (! drop && ! user_to)
         goto end;
     }
+    else if (ret < 0)
+    {
+      result= -1;
+      goto end;
+    }
   }
 
   /* Handle tables table. */
@@ -5716,9 +5770,12 @@ static int handle_grant_data(TABLE_LIST *tables, bool drop,
     else
     {
       /* Handle columns hash. */
-      if (((handle_grant_struct(2, drop, user_from, user_to) && ! result) ||
-           found) && ! result)
+      if ((((ret= handle_grant_struct(COLUMN_PRIVILEGES_HASH, drop, user_from,
+                                      user_to) > 0) && ! result) || found) &&
+          ! result)
         result= 1; /* At least one record/element found. */
+      else if (ret < 0)
+        result= -1;
     }
   }
  end: