Skip to content

M
MariaDB
Commit f2f358c5 authored by Raghav Kapoor's avatar Raghav Kapoor
Browse files
Options

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.
parents c750516a d82962d5
Show whitespace changes
Inline Side-by-side
Showing with 159 additions and 96 deletions
sql/sql_acl.cc
View file @ f2f358c5
......@@ -49,6 +49,7 @@
#include "sql_connect.h"
#include "hostname.h"
#include "sql_db.h"
#include "sql_array.h"
bool mysql_user_table_is_in_short_password_format= false;
......@@ -545,6 +546,18 @@ static bool update_user_table(THD *thd, TABLE *table,
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,
PROXY_USERS_ACL
};
/*
Convert scrambled password to binary form, according to scramble type,
......@@ -5982,20 +5995,20 @@ 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
5 acl_proxy_users
Structures are enumerated as follows:
0 ACL_USER
1 ACL_DB
2 COLUMN_PRIVILIGES_HASH
3 PROC_PRIVILEGES_HASH
4 FUNC_PRIVILEGES_HASH
5 ACL_PROXY_USERS
@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;
......@@ -6007,6 +6020,11 @@ static int handle_grant_struct(uint struct_no, bool drop,
ACL_DB *acl_db= NULL;
ACL_PROXY_USER *acl_proxy_user= 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'",
......@@ -6019,25 +6037,25 @@ 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;
case 5:
case PROXY_USERS_ACL:
elements= acl_proxy_users.elements;
break;
default:
......@@ -6055,27 +6073,27 @@ 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*) my_hash_element(grant_name_hash, idx);
user= grant_name->user;
host= grant_name->host.hostname;
break;
case 5:
case PROXY_USERS_ACL:
acl_proxy_user= dynamic_element(&acl_proxy_users, idx, ACL_PROXY_USER*);
user= acl_proxy_user->get_user();
host= acl_proxy_user->get_host();
......@@ -6101,57 +6119,96 @@ 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);
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.
- This is valid for USER_ACL, DB_ACL and PROXY_USERS_ACL.
*/
idx--;
break;
case 1:
case DB_ACL:
delete_dynamic_element(&acl_dbs, idx);
elements--;
idx--;
break;
case 2:
case 3:
case 4:
my_hash_delete(grant_name_hash, (uchar*) grant_name);
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;
case 5:
case PROXY_USERS_ACL:
delete_dynamic_element(&acl_proxy_users, idx);
elements--;
idx--;
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.
*/
idx--;
}
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;
case PROXY_USERS_ACL:
acl_proxy_user->set_user (&mem, user_to->user.str);
acl_proxy_user->set_host (&mem, user_to->host.str);
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
......@@ -6174,32 +6231,10 @@ static int handle_grant_struct(uint struct_no, bool drop,
*/
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;
}
case 5:
acl_proxy_user->set_user (&mem, user_to->user.str);
acl_proxy_user->set_host (&mem, user_to->host.str);
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
......@@ -6237,6 +6272,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. */
......@@ -6248,14 +6284,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. */
......@@ -6267,14 +6308,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. */
......@@ -6286,23 +6332,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. */
......@@ -6330,9 +6388,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;
}
}
......@@ -6347,9 +6408,11 @@ static int handle_grant_data(TABLE_LIST *tables, bool drop,
else
{
/* Handle proxies_priv array. */
if ((handle_grant_struct(5, drop, user_from, user_to) && !result) ||
found)
if (((ret= handle_grant_struct(PROXY_USERS_ACL, drop, user_from, user_to) > 0)
&& !result) || found)
result= 1; /* At least one record/element found. */
else if (ret < 0)
result= -1;
}
}
end:
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7