From: Date: May 6 2006 9:26am
Subject: bk commit into 5.0 tree (kroki:1.2116) BUG#16372
List-Archive: http://lists.mysql.com/commits/6040
X-Bug: 16372
Message-Id: <200605060726.k467Q6ie013783@moonlight.intranet>

Below is the list of changes that have just been committed into a local
5.0 repository of tomash. When tomash does a push these changes will
be propagated to the main repository and, within 24 hours after the
push, to the public repository.
For information on how to access the public repository
see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html

ChangeSet
  1.2116 06/05/06 11:25:59 kroki@stripped +3 -0
  Reapply fix for bug#16372 (Server crashes when test 'conc_sys' is running)
  after merge.
  
  Concurrent read and update of privilege structures (like simultaneous
  run of SHOW GRANTS and ADD USER) could result in server crash.
  
  Ensure that proper locking of ACL structures is done.
  
  No test case is provided because this bug can't be reproduced
  deterministically.

  sql/sql_parse.cc
    1.543 06/05/06 11:25:53 kroki@stripped +2 -2
    Use is_acl_user() instead of check_acl_user().

  sql/sql_acl.h
    1.50 06/05/06 11:25:53 kroki@stripped +0 -1
    Remove check_acl_user() declaration.

  sql/sql_acl.cc
    1.192 06/05/06 11:25:53 kroki@stripped +61 -47
    Ensure that access to ACL data is protected by acl_cache->lock mutex.
    Use system_charset_info for host names consistently.
    Remove check_acl_user().  Use find_acl_user() instead.

# This is a BitKeeper patch.  What follows are the unified diffs for the
# set of deltas contained in the patch.  The rest of the patch, the part
# that BitKeeper cares about, is below these diffs.
# User:	kroki
# Host:	moonlight.intranet
# Root:	/home/tomash/src/mysql_ab/mysql-5.0-merge

--- 1.191/sql/sql_acl.cc	2006-05-06 11:18:37 +04:00
+++ 1.192/sql/sql_acl.cc	2006-05-06 11:25:53 +04:00
@@ -1027,6 +1027,8 @@
 			    USER_RESOURCES  *mqh,
 			    ulong privileges)
 {
+  safe_mutex_assert_owner(&acl_cache->lock);
+
   for (uint i=0 ; i < acl_users.elements ; i++)
   {
     ACL_USER *acl_user=dynamic_element(&acl_users,i,ACL_USER*);
@@ -1077,6 +1079,9 @@
 			    ulong privileges)
 {
   ACL_USER acl_user;
+
+  safe_mutex_assert_owner(&acl_cache->lock);
+
   acl_user.user=*user ? strdup_root(&mem,user) : 0;
   update_hostname(&acl_user.host, *host ? strdup_root(&mem, host): 0);
   acl_user.access=privileges;
@@ -1106,6 +1111,8 @@
 static void acl_update_db(const char *user, const char *host, const char *db,
 			  ulong privileges)
 {
+  safe_mutex_assert_owner(&acl_cache->lock);
+
   for (uint i=0 ; i < acl_dbs.elements ; i++)
   {
     ACL_DB *acl_db=dynamic_element(&acl_dbs,i,ACL_DB*);
@@ -1532,6 +1539,9 @@
 {
   DBUG_ENTER("find_acl_user");
   DBUG_PRINT("enter",("host: '%s'  user: '%s'",host,user));
+
+  safe_mutex_assert_owner(&acl_cache->lock);
+
   for (uint i=0 ; i < acl_users.elements ; i++)
   {
     ACL_USER *acl_user=dynamic_element(&acl_users,i,ACL_USER*);
@@ -1544,7 +1554,7 @@
     if (!acl_user->user && !user[0] ||
 	acl_user->user && !strcmp(user,acl_user->user))
     {
-      if (exact ? !my_strcasecmp(&my_charset_latin1, host,
+      if (exact ? !my_strcasecmp(system_charset_info, host,
                                  acl_user->host.hostname ?
 				 acl_user->host.hostname : "") :
           compare_hostname(&acl_user->host,host,host))
@@ -2868,6 +2878,7 @@
     create_new_users= test_if_create_new_users(thd);
   bool result= FALSE;
   rw_wrlock(&LOCK_grant);
+  pthread_mutex_lock(&acl_cache->lock);
   MEM_ROOT *old_root= thd->mem_root;
   thd->mem_root= &memex;
   grant_version++;
@@ -2885,12 +2896,10 @@
       continue;
     }
     /* Create user if needed */
-    pthread_mutex_lock(&acl_cache->lock);
     error=replace_user_table(thd, tables[0].table, *Str,
 			     0, revoke_grant, create_new_users,
                              test(thd->variables.sql_mode &
                                   MODE_NO_AUTO_CREATE_USER));
-    pthread_mutex_unlock(&acl_cache->lock);
     if (error)
     {
       result= TRUE;				// Remember error
@@ -2982,6 +2991,7 @@
   }
   grant_option=TRUE;
   thd->mem_root= old_root;
+  pthread_mutex_unlock(&acl_cache->lock);
   rw_unlock(&LOCK_grant);
   if (!result)
     send_ok(thd);
@@ -3074,6 +3084,7 @@
   if (!revoke_grant)
     create_new_users= test_if_create_new_users(thd);
   rw_wrlock(&LOCK_grant);
+  pthread_mutex_lock(&acl_cache->lock);
   MEM_ROOT *old_root= thd->mem_root;
   thd->mem_root= &memex;
 
@@ -3093,12 +3104,10 @@
       continue;
     }
     /* Create user if needed */
-    pthread_mutex_lock(&acl_cache->lock);
     error=replace_user_table(thd, tables[0].table, *Str,
 			     0, revoke_grant, create_new_users,
                              test(thd->variables.sql_mode &
                                   MODE_NO_AUTO_CREATE_USER));
-    pthread_mutex_unlock(&acl_cache->lock);
     if (error)
     {
       result= TRUE;				// Remember error
@@ -3140,6 +3149,7 @@
   }
   grant_option=TRUE;
   thd->mem_root= old_root;
+  pthread_mutex_unlock(&acl_cache->lock);
   rw_unlock(&LOCK_grant);
   if (!result && !no_error)
     send_ok(thd);
@@ -4116,20 +4126,15 @@
     DBUG_RETURN(TRUE);
   }
 
-  for (counter=0 ; counter < acl_users.elements ; counter++)
-  {
-    const char *user,*host;
-    acl_user=dynamic_element(&acl_users,counter,ACL_USER*);
-    if (!(user=acl_user->user))
-      user= "";
-    if (!(host=acl_user->host.hostname))
-      host= "";
-    if (!strcmp(lex_user->user.str,user) &&
-	!my_strcasecmp(system_charset_info, lex_user->host.str, host))
-      break;
-  }
-  if (counter == acl_users.elements)
+  rw_rdlock(&LOCK_grant);
+  VOID(pthread_mutex_lock(&acl_cache->lock));
+
+  acl_user= find_acl_user(lex_user->host.str, lex_user->user.str, TRUE);
+  if (!acl_user)
   {
+    VOID(pthread_mutex_unlock(&acl_cache->lock));
+    rw_unlock(&LOCK_grant);
+
     my_error(ER_NONEXISTING_GRANT, MYF(0),
              lex_user->user.str, lex_user->host.str);
     DBUG_RETURN(TRUE);
@@ -4144,10 +4149,12 @@
   field_list.push_back(field);
   if (protocol->send_fields(&field_list,
                             Protocol::SEND_NUM_ROWS | Protocol::SEND_EOF))
-    DBUG_RETURN(TRUE);
+  {
+    VOID(pthread_mutex_unlock(&acl_cache->lock));
+    rw_unlock(&LOCK_grant);
 
-  rw_wrlock(&LOCK_grant);
-  VOID(pthread_mutex_lock(&acl_cache->lock));
+    DBUG_RETURN(TRUE);
+  }
 
   /* Add first global access grants */
   {
@@ -4555,10 +4562,15 @@
 void get_mqh(const char *user, const char *host, USER_CONN *uc)
 {
   ACL_USER *acl_user;
+
+  pthread_mutex_lock(&acl_cache->lock);
+
   if (initialized && (acl_user= find_acl_user(host,user, FALSE)))
     uc->user_resources= acl_user->user_resource;
   else
     bzero((char*) &uc->user_resources, sizeof(uc->user_resources));
+
+  pthread_mutex_unlock(&acl_cache->lock);
 }
 
 /*
@@ -4638,31 +4650,6 @@
   DBUG_RETURN(0);
 }
 
-ACL_USER *check_acl_user(LEX_USER *user_name,
-			 uint *acl_acl_userdx)
-{
-  ACL_USER *acl_user= 0;
-  uint counter;
-
-  for (counter= 0 ; counter < acl_users.elements ; counter++)
-  {
-    const char *user,*host;
-    acl_user= dynamic_element(&acl_users, counter, ACL_USER*);
-    if (!(user=acl_user->user))
-      user= "";
-    if (!(host=acl_user->host.hostname))
-      host= "";
-    if (!strcmp(user_name->user.str,user) &&
-	!my_strcasecmp(system_charset_info, user_name->host.str, host))
-      break;
-  }
-  if (counter == acl_users.elements)
-    return 0;
-
-  *acl_acl_userdx= counter;
-  return acl_user;
-}
-
 
 /*
   Modify a privilege table.
@@ -4907,6 +4894,8 @@
   LINT_INIT(acl_db);
   LINT_INIT(grant_name);
 
+  safe_mutex_assert_owner(&acl_cache->lock);
+
   /* Get the number of elements in the in-memory structure. */
   switch (struct_no) {
   case 0:
@@ -5370,7 +5359,7 @@
   List_iterator <LEX_USER> user_list(list);
   while ((lex_user=user_list++))
   {
-    if (!check_acl_user(lex_user, &counter))
+    if (!find_acl_user(lex_user->host.str, lex_user->user.str, TRUE))
     {
       sql_print_error("REVOKE ALL PRIVILEGES, GRANT: User '%s'@'%s' does not "
                       "exists", lex_user->user.str, lex_user->host.str);
@@ -5606,6 +5595,7 @@
 
   combo->user.str= sctx->user;
   
+  VOID(pthread_mutex_lock(&acl_cache->lock));
   if (!find_acl_user(combo->host.str=(char*)sctx->host_or_ip, combo->user.str,
                      FALSE) &&
       !find_acl_user(combo->host.str=(char*)sctx->host, combo->user.str,
@@ -5613,7 +5603,11 @@
       !find_acl_user(combo->host.str=(char*)sctx->ip, combo->user.str,
                      FALSE) &&
       !find_acl_user(combo->host.str=(char*)"%", combo->user.str, FALSE))
+  {
+    VOID(pthread_mutex_unlock(&acl_cache->lock));
     DBUG_RETURN(TRUE);
+  }
+  VOID(pthread_mutex_unlock(&acl_cache->lock));
 
   bzero((char*)tables, sizeof(TABLE_LIST));
   user_list.empty();
@@ -5731,6 +5725,8 @@
   char *curr_host= thd->security_ctx->priv_host_name();
   DBUG_ENTER("fill_schema_user_privileges");
 
+  pthread_mutex_lock(&acl_cache->lock);
+
   for (counter=0 ; counter < acl_users.elements ; counter++)
   {
     const char *user,*host, *is_grantable="YES";
@@ -5766,6 +5762,9 @@
       }
     }
   }
+
+  pthread_mutex_unlock(&acl_cache->lock);
+
   DBUG_RETURN(0);
 #else
   return(0);
@@ -5785,6 +5784,8 @@
   char *curr_host= thd->security_ctx->priv_host_name();
   DBUG_ENTER("fill_schema_schema_privileges");
 
+  pthread_mutex_lock(&acl_cache->lock);
+
   for (counter=0 ; counter < acl_dbs.elements ; counter++)
   {
     const char *user, *host, *is_grantable="YES";
@@ -5823,6 +5824,9 @@
       }
     }
   }
+
+  pthread_mutex_unlock(&acl_cache->lock);
+
   DBUG_RETURN(0);
 #else
   return (0);
@@ -5840,6 +5844,8 @@
   char *curr_host= thd->security_ctx->priv_host_name();
   DBUG_ENTER("fill_schema_table_privileges");
 
+  rw_rdlock(&LOCK_grant);
+
   for (index=0 ; index < column_priv_hash.records ; index++)
   {
     const char *user, *is_grantable= "YES";
@@ -5885,6 +5891,9 @@
       }
     }
   }
+
+  rw_unlock(&LOCK_grant);
+
   DBUG_RETURN(0);
 #else
   return (0);
@@ -5902,6 +5911,8 @@
   char *curr_host= thd->security_ctx->priv_host_name();
   DBUG_ENTER("fill_schema_table_privileges");
 
+  rw_rdlock(&LOCK_grant);
+
   for (index=0 ; index < column_priv_hash.records ; index++)
   {
     const char *user, *is_grantable= "YES";
@@ -5953,6 +5964,9 @@
       }
     }
   }
+
+  rw_unlock(&LOCK_grant);
+
   DBUG_RETURN(0);
 #else
   return (0);

--- 1.49/sql/sql_acl.h	2005-11-30 22:27:08 +03:00
+++ 1.50/sql/sql_acl.h	2006-05-06 11:25:53 +04:00
@@ -196,7 +196,6 @@
 bool mysql_routine_grant(THD *thd, TABLE_LIST *table, bool is_proc,
 			 List <LEX_USER> &user_list, ulong rights,
 			 bool revoke, bool no_error);
-ACL_USER *check_acl_user(LEX_USER *user_name, uint *acl_acl_userdx);
 my_bool grant_init();
 void grant_free(void);
 my_bool grant_reload(THD *thd);

--- 1.542/sql/sql_parse.cc	2006-05-02 06:50:28 +04:00
+++ 1.543/sql/sql_parse.cc	2006-05-06 11:25:53 +04:00
@@ -3835,7 +3835,6 @@
     if (thd->security_ctx->user)              // If not replication
     {
       LEX_USER *user;
-      uint counter;
 
       List_iterator <LEX_USER> user_list(lex->users_list);
       while ((user= user_list++))
@@ -3853,7 +3852,8 @@
                           user->host.str, thd->security_ctx->host_or_ip))
         {
           // TODO: use check_change_password()
-          if (check_acl_user(user, &counter) && user->password.str &&
+          if (is_acl_user(user->host.str, user->user.str) &&
+              user->password.str &&
               check_access(thd, UPDATE_ACL,"mysql",0,1,1,0))
           {
             my_message(ER_PASSWORD_NOT_ALLOWED,