Below is the list of changes that have just been committed into a local
5.0 repository of kostja. When kostja 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.2207 06/06/28 21:48:36 konstantin@stripped +5 -0
A fix for Bug#19022 "Memory bug when switching db during trigger execution".
No test case as the bug is in an existing test case (rpl_trigger.test
when it is run under valgrind).
The warning was caused by memory corruption in replication slave: thd->db
was pointing at a stack address that was previously used by
sp_head::execute()::old_db. This happened because mysql_change_db
behaved differently in replication slave and did not make a copy of the
argument to assign to thd->db.
The solution is to always free the old value of thd->db and allocate a new
copy, regardless whether we're running in a replication slave or not.
sql/sql_db.cc
1.130 06/06/28 21:48:32 konstantin@stripped +60 -76
Always make a deep copy of the argument in mysql_change_db, even
if running in a replication slave. This is necessary because
sp_use_new_db (stored procedures) assumes that mysql_change_db always makes a
deep copy of the argument, and thus passes a pointer to stack into it.
This assumption was true for all cases except the replication slave thread.
sql/sql_class.h
1.292 06/06/28 21:48:32 konstantin@stripped +13 -11
Fix set_db to always free the old db, even if the argument is NULL.
Add a comment.
sql/slave.h
1.95 06/06/28 21:48:32 konstantin@stripped +0 -1
Remove an unneeded declaration.
sql/slave.cc
1.273 06/06/28 21:48:32 konstantin@stripped +0 -18
Move rewrite_db to log_event.cc
sql/log_event.cc
1.208 06/06/28 21:48:32 konstantin@stripped +25 -7
Move rewrite_db to log_event.cc, the only place where it is used.
# 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: konstantin
# Host: bodhi.netgear
# Root: /opt/local/work/mysql-5.0-19022
--- 1.207/sql/log_event.cc 2006-06-27 14:56:13 +04:00
+++ 1.208/sql/log_event.cc 2006-06-28 21:48:32 +04:00
@@ -1635,14 +1635,33 @@
*/
#if defined(HAVE_REPLICATION) && !defined(MYSQL_CLIENT)
+
+static const char *rewrite_db(const char *db)
+{
+ if (replicate_rewrite_db.is_empty() || db == NULL)
+ return db;
+ I_List_iterator<i_string_pair> it(replicate_rewrite_db);
+ i_string_pair* tmp;
+
+ while ((tmp=it++))
+ {
+ if (strcmp(tmp->key, db) == 0)
+ return tmp->val;
+ }
+ return db;
+}
+
+
int Query_log_event::exec_event(struct st_relay_log_info* rli)
{
return exec_event(rli, query, q_len);
}
-int Query_log_event::exec_event(struct st_relay_log_info* rli, const char *query_arg, uint32 q_len_arg)
+int Query_log_event::exec_event(struct st_relay_log_info* rli,
+ const char *query_arg, uint32 q_len_arg)
{
+ const char *new_db= rewrite_db(db);
int expected_error,actual_error= 0;
/*
Colleagues: please never free(thd->catalog) in MySQL. This would lead to
@@ -1651,8 +1670,7 @@
Thank you.
*/
thd->catalog= catalog_len ? (char *) catalog : (char *)"";
- thd->db_length= db_len;
- thd->db= (char*) rewrite_db(db, &thd->db_length);
+ thd->set_db(new_db, strlen(new_db)); /* allocates a copy of 'db' */
thd->variables.auto_increment_increment= auto_increment_increment;
thd->variables.auto_increment_offset= auto_increment_offset;
@@ -1869,7 +1887,7 @@
TABLE uses the db.table syntax.
*/
thd->catalog= 0;
- thd->reset_db(NULL, 0); // prevent db from being freed
+ thd->set_db(NULL, 0); /* will free the current database */
thd->query= 0; // just to be sure
thd->query_length= 0;
VOID(pthread_mutex_unlock(&LOCK_thread_count));
@@ -2817,8 +2835,8 @@
int Load_log_event::exec_event(NET* net, struct st_relay_log_info* rli,
bool use_rli_only_for_errors)
{
- thd->db_length= db_len;
- thd->db= (char*) rewrite_db(db, &thd->db_length);
+ const char *new_db= rewrite_db(db);
+ thd->set_db(new_db, strlen(new_db));
DBUG_ASSERT(thd->query == 0);
thd->query_length= 0; // Should not be needed
thd->query_error= 0;
@@ -3018,7 +3036,7 @@
const char *remember_db= thd->db;
VOID(pthread_mutex_lock(&LOCK_thread_count));
thd->catalog= 0;
- thd->reset_db(NULL, 0);
+ thd->set_db(NULL, 0); /* will free the current database */
thd->query= 0;
thd->query_length= 0;
VOID(pthread_mutex_unlock(&LOCK_thread_count));
--- 1.272/sql/slave.cc 2006-06-27 17:34:08 +04:00
+++ 1.273/sql/slave.cc 2006-06-28 21:48:32 +04:00
@@ -1177,24 +1177,6 @@
}
-const char *rewrite_db(const char* db, uint *new_len)
-{
- if (replicate_rewrite_db.is_empty() || !db)
- return db;
- I_List_iterator<i_string_pair> it(replicate_rewrite_db);
- i_string_pair* tmp;
-
- while ((tmp=it++))
- {
- if (!strcmp(tmp->key, db))
- {
- *new_len= (uint32)strlen(tmp->val);
- return tmp->val;
- }
- }
- return db;
-}
-
/*
From other comments and tests in code, it looks like
sometimes Query_log_event and Load_log_event can have db == 0
--- 1.291/sql/sql_class.h 2006-06-27 14:56:13 +04:00
+++ 1.292/sql/sql_class.h 2006-06-28 21:48:32 +04:00
@@ -1574,21 +1574,23 @@
/*
Initialize the current database from a NULL-terminated string with length
+ If we run out of memory, we free the current database and return TRUE.
+ This way the user will notice the error as there will be no current
+ database selected (in addition to the error message set by malloc).
*/
- void set_db(const char *new_db, uint new_db_len)
+ bool set_db(const char *new_db, uint new_db_len)
{
- if (new_db)
+ /* Do not reallocate memory if current chunk is big enough. */
+ if (db && new_db && db_length >= new_db_len)
+ memcpy(db, new_db, new_db_len+1);
+ else
{
- /* Do not reallocate memory if current chunk is big enough. */
- if (db && db_length >= new_db_len)
- memcpy(db, new_db, new_db_len+1);
- else
- {
- safeFree(db);
- db= my_strdup_with_length(new_db, new_db_len, MYF(MY_WME));
- }
- db_length= db ? new_db_len: 0;
+ x_free(db);
+ db= new_db ? my_strdup_with_length(new_db, new_db_len, MYF(MY_WME)) :
+ NULL;
}
+ db_length= db ? new_db_len : 0;
+ return new_db && !db;
}
void reset_db(char *new_db, uint new_db_len)
{
--- 1.129/sql/sql_db.cc 2006-06-27 00:50:50 +04:00
+++ 1.130/sql/sql_db.cc 2006-06-28 21:48:32 +04:00
@@ -781,32 +781,13 @@
exit:
(void)sp_drop_db_routines(thd, db); /* QQ Ignore errors for now */
/*
- If this database was the client's selected database, we silently change the
- client's selected database to nothing (to have an empty SELECT DATABASE()
- in the future). For this we free() thd->db and set it to 0. But we don't do
- free() for the slave thread. Indeed, doing a x_free() on it leads to nasty
- problems (i.e. long painful debugging) because in this thread, thd->db is
- the same as data_buf and db of the Query_log_event which is dropping the
- database. So if you free() thd->db, you're freeing data_buf. You set
- thd->db to 0 but not data_buf (thd->db and data_buf are two distinct
- pointers which point to the same place). Then in ~Query_log_event(), we
- have 'if (data_buf) free(data_buf)' data_buf is !=0 so this makes a
- DOUBLE free().
- Side effects of this double free() are, randomly (depends on the machine),
- when the slave is replicating a DROP DATABASE:
- - garbage characters in the error message:
- "Error 'Can't drop database 'test2'; database doesn't exist' on query
- - segfault
- - hang in "free(vio)" (yes!) in the I/O or SQL slave threads (so slave
- server hangs at shutdown etc).
+ If this database was the client's selected database, we silently
+ change the client's selected database to nothing (to have an empty
+ SELECT DATABASE() in the future). For this we free() thd->db and set
+ it to 0.
*/
if (thd->db && !strcmp(thd->db, db))
- {
- if (!(thd->slave_thread)) /* a slave thread will free it itself */
- x_free(thd->db);
- thd->reset_db(NULL, 0);
- }
+ thd->set_db(NULL, 0);
VOID(pthread_mutex_unlock(&LOCK_mysql_create_db));
start_waiting_global_read_lock(thd);
exit2:
@@ -1099,24 +1080,34 @@
/*
- Change default database.
+ Change the current database.
SYNOPSIS
mysql_change_db()
- thd Thread handler
- name Databasename
+ thd thread handle
+ name database name
no_access_check True don't do access check. In this case name may be ""
DESCRIPTION
- Becasue the database name may have been given directly from the
- communication packet (in case of 'connect' or 'COM_INIT_DB')
- we have to do end space removal in this function.
+ Check that the database name corresponds to a valid and existent
+ database, check access rights (unless called with no_access_check),
+ and set the current database. This function is called to change the
+ current database upon user request (COM_CHANGE_DB command) or
+ temporarily, to execute a stored procedure or function.
NOTES
Do as little as possible in this function, as it is not called for the
replication slave SQL thread (for that thread, setting of thd->db is done
in ::exec_event() methods of log_event.cc).
+ This function ensures that the name of the database is always allocated
+ on the system heap: this is necessary to be able to uniformly change the
+ database from any module of the server.
+ Up to 5.0, different modules used different memory to store the name of
+ the database, and this led to memory corruption: a stack pointer set by
+ Stored Procedures was used by replication after the stack address was
+ long gone.
+
This function does not send anything, including error messages to the
client, if that should be sent to the client, call net_send_error after
this function.
@@ -1128,9 +1119,8 @@
bool mysql_change_db(THD *thd, const char *name, bool no_access_check)
{
- int length, db_length;
- char *dbname= thd->slave_thread ? (char *) name :
- my_strdup((char *) name, MYF(MY_WME));
+ int path_length, db_length;
+ char *db_name;
char path[FN_REFLEN];
HA_CREATE_INFO create;
bool system_db= 0;
@@ -1142,32 +1132,35 @@
DBUG_ENTER("mysql_change_db");
DBUG_PRINT("enter",("name: '%s'",name));
- LINT_INIT(db_length);
-
- /* dbname can only be NULL if malloc failed */
- if (!dbname || !(db_length= strlen(dbname)))
+ if (name == NULL || name[0] == '\0' && no_access_check == FALSE)
{
- if (no_access_check && dbname)
- {
- /* Called from SP when orignal database was not set */
- system_db= 1;
- goto end;
- }
- if (!(thd->slave_thread))
- x_free(dbname); /* purecov: inspected */
- my_message(ER_NO_DB_ERROR, ER(ER_NO_DB_ERROR),
- MYF(0)); /* purecov: inspected */
+ my_message(ER_NO_DB_ERROR, ER(ER_NO_DB_ERROR), MYF(0));
DBUG_RETURN(1); /* purecov: inspected */
}
- if (check_db_name(dbname))
+ else if (name[0] == '\0')
+ {
+ /* Called from SP to restore the original database, which was NULL */
+ DBUG_ASSERT(no_access_check);
+ system_db= 1;
+ db_name= NULL;
+ db_length= 0;
+ goto end;
+ }
+ /*
+ Now we need to make a copy because check_db_name requires a
+ non-constant argument. TODO: fix check_db_name.
+ */
+ if ((db_name= my_strdup(name, MYF(MY_WME))) == NULL)
+ DBUG_RETURN(1); /* the error is set */
+ db_length= strlen(db_name);
+ if (check_db_name(db_name))
{
- my_error(ER_WRONG_DB_NAME, MYF(0), dbname);
- if (!(thd->slave_thread))
- my_free(dbname, MYF(0));
+ my_error(ER_WRONG_DB_NAME, MYF(0), db_name);
+ my_free(db_name, MYF(0));
DBUG_RETURN(1);
}
- DBUG_PRINT("info",("Use database: %s", dbname));
- if (!my_strcasecmp(system_charset_info, dbname, information_schema_name.str))
+ DBUG_PRINT("info",("Use database: %s", db_name));
+ if (!my_strcasecmp(system_charset_info, db_name, information_schema_name.str))
{
system_db= 1;
#ifndef NO_EMBEDDED_ACCESS_CHECKS
@@ -1182,45 +1175,36 @@
if (test_all_bits(sctx->master_access, DB_ACLS))
db_access=DB_ACLS;
else
- db_access= (acl_get(sctx->host, sctx->ip, sctx->priv_user, dbname, 0) |
+ db_access= (acl_get(sctx->host, sctx->ip, sctx->priv_user, db_name, 0) |
sctx->master_access);
if (!(db_access & DB_ACLS) && (!grant_option ||
- check_grant_db(thd,dbname)))
+ check_grant_db(thd,db_name)))
{
my_error(ER_DBACCESS_DENIED_ERROR, MYF(0),
sctx->priv_user,
sctx->priv_host,
- dbname);
+ db_name);
mysql_log.write(thd, COM_INIT_DB, ER(ER_DBACCESS_DENIED_ERROR),
- sctx->priv_user, sctx->priv_host, dbname);
- if (!(thd->slave_thread))
- my_free(dbname,MYF(0));
+ sctx->priv_user, sctx->priv_host, db_name);
+ my_free(db_name, MYF(0));
DBUG_RETURN(1);
}
}
#endif
- (void) sprintf(path,"%s/%s",mysql_data_home,dbname);
- length=unpack_dirname(path,path); // Convert if not unix
- if (length && path[length-1] == FN_LIBCHAR)
- path[length-1]=0; // remove ending '\'
+ (void) sprintf(path,"%s/%s", mysql_data_home, db_name);
+ path_length= unpack_dirname(path, path); // Convert if not UNIX
+ if (path_length && path[path_length-1] == FN_LIBCHAR)
+ path[path_length-1]= '\0'; // remove ending '\'
if (my_access(path,F_OK))
{
- my_error(ER_BAD_DB_ERROR, MYF(0), dbname);
- if (!(thd->slave_thread))
- my_free(dbname,MYF(0));
+ my_error(ER_BAD_DB_ERROR, MYF(0), db_name);
+ my_free(db_name, MYF(0));
DBUG_RETURN(1);
}
end:
- if (!(thd->slave_thread))
- x_free(thd->db);
- if (dbname && dbname[0] == 0)
- {
- if (!(thd->slave_thread))
- my_free(dbname, MYF(0));
- thd->reset_db(NULL, 0);
- }
- else
- thd->reset_db(dbname, db_length); // THD::~THD will free this
+ x_free(thd->db);
+ DBUG_ASSERT(db_name == NULL || db_name[0] != '\0');
+ thd->reset_db(db_name, db_length); // THD::~THD will free this
#ifndef NO_EMBEDDED_ACCESS_CHECKS
if (!no_access_check)
sctx->db_access= db_access;
--- 1.94/sql/slave.h 2006-06-27 17:34:08 +04:00
+++ 1.95/sql/slave.h 2006-06-28 21:48:32 +04:00
@@ -550,7 +550,6 @@
int add_wild_table_rule(DYNAMIC_ARRAY* a, const char* table_spec);
void init_table_rule_hash(HASH* h, bool* h_inited);
void init_table_rule_array(DYNAMIC_ARRAY* a, bool* a_inited);
-const char *rewrite_db(const char* db, uint *new_db_len);
const char *print_slave_db_safe(const char *db);
int check_expected_error(THD* thd, RELAY_LOG_INFO* rli, int error_code);
void skip_load_data_infile(NET* net);
| Thread |
|---|
| • bk commit into 5.0 tree (konstantin:1.2207) BUG#19022 | konstantin | 28 Jun |
