Below is the list of changes that have just been committed into a local
5.1 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@stripped, 2007-06-15 19:33:23+04:00, kostja@bodhi.(none) +3 -0
Problem:
--------
Patch for Bug 25411 introduces a new buffer to contain SQL text
without version comments. The buffer is filled in during lexical
analysis.
This buffer is allocated in THD memory root for every statement and
contains a copy of statement text.
With this buffer, the server performs at least 3 copies of each
query byte:
From VIO layer to NET layer.
From NET layer to SQL layer in alloc_query, using statement memory
From thd->query to Lex_input_stream::m_cpp_buf, using statement memory
again.
The second copy now has become completely redundant.
Additionally, presense of two queries (m_cpp_buf and thd->query) in SQL
layer, one without and another with version comments is error-prone.
Solution
--------
The goal of this sequence of patches is to ensure that:
- we avoid an extra copying in alloc_query that now has become
obsolete.
- the SQL layer always works with the pre-processed query, and
the network packet is used only in the parser
and in the query cache.
In other words, the goal is to change current the existing sequence
of calls:
alloc_query(); // allocates thd->query
query_cache_send_result_to_client(); // uses thd->query
Lex_input_stream::Lex_input_stream; // allocates m_cpp_buf
MYSQLparse(); // copies thd->query to m_cpp_buf
mysql_execute_command(); // uses both, m_cpp_buf and thd->query
to
query_cache_send_result_to_client(); // uses NET buffer, and makes a copy
// if necessary
Lex_input_stream::Lex_input_stream(); // allocates m_cpp_buf
MYSQLparse(); // fills in m_cpp_buf reading data from the network packet
thd->query= lip->m_cpp_buf;
mysql_execute_command(); // uses thd->query, which now is the same as
m_cpp_buf
This is the first patch in a sequence of patches, the goal of
this patch is to fix bogus packet_length that is currently passed
to dispatch_command and subsequently to alloc_query.
sql/sql_parse.cc@stripped, 2007-06-15 19:33:20+04:00, kostja@bodhi.(none) +58 -45
Fix bogus packet_length passed to dispatch_command.
Remove bogus check in COM_CHANGE_USER based on an incorrect assumption.
sql/sql_prepare.cc@stripped, 2007-06-15 19:33:20+04:00, kostja@bodhi.(none) +5 -7
dispatch_command now is passed a correct value of packet_length
tests/mysql_client_test.c@stripped, 2007-06-15 19:33:20+04:00, kostja@bodhi.(none) +210 -10
Fix warnings. Add a test case for mysql_change_user() (was not covered
with tests).
# 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: kostja
# Host: bodhi.(none)
# Root: /opt/local/work/mysql-5.1-alloc_query-piece-by-piece
--- 1.679/sql/sql_parse.cc 2007-06-14 18:35:56 +04:00
+++ 1.680/sql/sql_parse.cc 2007-06-15 19:33:20 +04:00
@@ -273,8 +273,6 @@
values of init_command_var can't be changed
*/
rw_rdlock(var_mutex);
- thd->query= init_command_var->value;
- thd->query_length= init_command_var->value_length;
save_client_capabilities= thd->client_capabilities;
thd->client_capabilities|= CLIENT_MULTI_QUERIES;
/*
@@ -284,7 +282,9 @@
save_vio= thd->net.vio;
thd->net.vio= 0;
thd->net.no_send_error= 0;
- dispatch_command(COM_QUERY, thd, thd->query, thd->query_length+1);
+ dispatch_command(COM_QUERY, thd,
+ init_command_var->value,
+ init_command_var->value_length);
rw_unlock(var_mutex);
thd->client_capabilities= save_client_capabilities;
thd->net.vio= save_vio;
@@ -643,30 +643,39 @@
net->error= 0;
DBUG_RETURN(FALSE);
}
- else
+
+ packet= (char*) net->read_pos;
+ /*
+ 'packet_length' contains length of data, as it was stored in packet
+ header. In case of malformed header, my_net_read returns zero.
+ If packet_length is not zero, my_net_read ensures that the returned
+ number of bytes was actually read from network.
+ There is also an extra safety measure in my_net_read:
+ it sets packet[packet_length]= 0, but only for non-zero packets.
+ */
+ if (packet_length == 0) /* safety */
{
- packet=(char*) net->read_pos;
- command = (enum enum_server_command) (uchar) packet[0];
- if (command >= COM_END)
- command= COM_END; // Wrong command
- DBUG_PRINT("info",("Command on %s = %d (%s)",
- vio_description(net->vio), command,
- command_name[command].str));
+ /* Initialize with COM_SLEEP packet */
+ packet[0]= (uchar) COM_SLEEP;
+ packet_length= 1;
}
+ /* Do not rely on my_net_read, extra safety against programming errors. */
+ packet[packet_length]= '\0'; /* safety */
+
+ command= (enum enum_server_command) (uchar) packet[0];
+
+ if (command >= COM_END)
+ command= COM_END; // Wrong command
+
+ DBUG_PRINT("info",("Command on %s = %d (%s)",
+ vio_description(net->vio), command,
+ command_name[command].str));
/* Restore read timeout value */
my_net_set_read_timeout(net, thd->variables.net_read_timeout);
- /*
- packet_length contains length of data, as it was stored in packet
- header. In case of malformed header, packet_length can be zero.
- If packet_length is not zero, my_net_read ensures that this number
- of bytes was actually read from network. Additionally my_net_read
- sets packet[packet_length]= 0 (thus if packet_length == 0,
- command == packet[0] == COM_SLEEP).
- In dispatch_command packet[packet_length] points beyond the end of packet.
- */
- DBUG_RETURN(dispatch_command(command,thd, packet+1, (uint) packet_length));
+ DBUG_ASSERT(packet_length);
+ DBUG_RETURN(dispatch_command(command, thd, packet+1, (uint) (packet_length-1)));
}
#endif /* EMBEDDED_LIBRARY */
@@ -679,9 +688,7 @@
thd connection handle
command type of command to perform
packet data for the command, packet is always null-terminated
- packet_length length of packet + 1 (to show that data is
- null-terminated) except for COM_SLEEP, where it
- can be zero.
+ packet_length length of packet. Can be zero, e.g. in case of COM_SLEEP.
RETURN VALUE
0 ok
1 request of thread shutdown, i. e. if command is
@@ -725,7 +732,7 @@
LEX_STRING tmp;
status_var_increment(thd->status_var.com_stat[SQLCOM_CHANGE_DB]);
thd->convert_string(&tmp, system_charset_info,
- packet, packet_length-1, thd->charset());
+ packet, packet_length, thd->charset());
if (!mysql_change_db(thd, &tmp, FALSE))
{
general_log_print(thd, command, "%s",thd->db);
@@ -745,14 +752,16 @@
{
char *tbl_name;
LEX_STRING db;
+ /* Safe because there is always a trailing \0 at the end of the packet */
uint db_len= *(uchar*) packet;
- if (db_len >= packet_length || db_len > NAME_LEN)
+ if (db_len + 1 > packet_length || db_len > NAME_LEN)
{
my_message(ER_UNKNOWN_COM_ERROR, ER(ER_UNKNOWN_COM_ERROR), MYF(0));
break;
}
+ /* Safe because there is always a trailing \0 at the end of the packet */
uint tbl_len= *(uchar*) (packet + db_len + 1);
- if (db_len+tbl_len+2 > packet_length || tbl_len > NAME_LEN)
+ if (db_len + tbl_len + 2 > packet_length || tbl_len > NAME_LEN)
{
my_message(ER_UNKNOWN_COM_ERROR, ER(ER_UNKNOWN_COM_ERROR), MYF(0));
break;
@@ -775,7 +784,8 @@
case COM_CHANGE_USER:
{
status_var_increment(thd->status_var.com_other);
- char *user= (char*) packet, *packet_end= packet+ packet_length;
+ char *user= (char*) packet, *packet_end= packet + packet_length;
+ /* Safe because there is always a trailing \0 at the end of the packet */
char *passwd= strend(user)+1;
thd->change_user();
@@ -789,6 +799,15 @@
char db_buff[NAME_LEN+1]; // buffer to store db in utf8
char *db= passwd;
char *save_db;
+ /*
+ If there is no password supplied, the packet must contain '\0',
+ in any type of handshake (4.1 or pre-4.1).
+ */
+ if (passwd >= packet_end)
+ {
+ my_message(ER_UNKNOWN_COM_ERROR, ER(ER_UNKNOWN_COM_ERROR), MYF(0));
+ break;
+ }
uint passwd_len= (thd->client_capabilities & CLIENT_SECURE_CONNECTION ?
*passwd++ : strlen(passwd));
uint dummy_errors, save_db_length, db_length;
@@ -797,22 +816,17 @@
USER_CONN *save_user_connect;
db+= passwd_len + 1;
-#ifndef EMBEDDED_LIBRARY
- /* Small check for incoming packet */
- if ((uint) ((uchar*) db - net->read_pos) > packet_length)
+ /*
+ Database name is always NUL-terminated, so in case of empty database
+ the packet must contain at least the trailing '\0'.
+ */
+ if (db >= packet_end)
{
my_message(ER_UNKNOWN_COM_ERROR, ER(ER_UNKNOWN_COM_ERROR), MYF(0));
break;
}
-#endif
+ db_length= packet_end - db - 1; /* do not count the trailing '\0' */
/* Convert database name to utf8 */
- /*
- Handle problem with old bug in client protocol where db had an extra
- \0
- */
- db_length= (packet_end - db);
- if (db_length > 0 && db[db_length-1] == 0)
- db_length--;
db_buff[copy_and_convert(db_buff, sizeof(db_buff)-1,
system_charset_info, db, db_length,
thd->charset(), &dummy_errors)]= 0;
@@ -948,7 +962,7 @@
break;
#else
{
- char *fields, *packet_end= packet + packet_length - 1, *arg_end;
+ char *fields, *packet_end= packet + packet_length, *arg_end;
/* Locked closure of all tables */
TABLE_LIST table_list;
LEX_STRING conv_name;
@@ -1023,7 +1037,7 @@
HA_CREATE_INFO create_info;
status_var_increment(thd->status_var.com_stat[SQLCOM_CREATE_DB]);
- if (thd->LEX_STRING_make(&db, packet, packet_length -1) ||
+ if (thd->LEX_STRING_make(&db, packet, packet_length) ||
thd->LEX_STRING_make(&alias, db.str, db.length) ||
check_db_name(&db))
{
@@ -1044,7 +1058,7 @@
status_var_increment(thd->status_var.com_stat[SQLCOM_DROP_DB]);
LEX_STRING db;
- if (thd->LEX_STRING_make(&db, packet, packet_length - 1) ||
+ if (thd->LEX_STRING_make(&db, packet, packet_length) ||
check_db_name(&db))
{
my_error(ER_WRONG_DB_NAME, MYF(0), db.str ? db.str : "NULL");
@@ -1113,7 +1127,7 @@
break; /* purecov: inspected */
/*
If the client is < 4.1.3, it is going to send us no argument; then
- packet_length is 1, packet[0] is the end 0 of the packet. Note that
+ packet_length is 0, packet[0] is the end 0 of the packet. Note that
SHUTDOWN_DEFAULT is 0. If client is >= 4.1.3, the shutdown level is in
packet[0].
*/
@@ -1472,9 +1486,8 @@
bool alloc_query(THD *thd, const char *packet, uint packet_length)
{
- packet_length--; // Remove end null
/* Remove garbage at start and end of query */
- while (my_isspace(thd->charset(),packet[0]) && packet_length > 0)
+ while (packet_length > 0 && my_isspace(thd->charset(), packet[0]))
{
packet++;
packet_length--;
--- 1.222/sql/sql_prepare.cc 2007-06-14 18:35:56 +04:00
+++ 1.223/sql/sql_prepare.cc 2007-06-15 19:33:20 +04:00
@@ -2098,7 +2098,7 @@
DBUG_VOID_RETURN;
}
- if (stmt->prepare(query, query_len+1))
+ if (stmt->prepare(query, query_len))
{
/* Statement map deletes the statement on erase */
thd->stmt_map.erase(stmt);
@@ -2261,7 +2261,7 @@
/* Query text for binary, general or slow log, if any of them is open */
String expanded_query;
#ifndef EMBEDDED_LIBRARY
- uchar *packet_end= packet + packet_length - 1;
+ uchar *packet_end= packet + packet_length;
#endif
Prepared_statement *stmt;
bool error;
@@ -2582,14 +2582,14 @@
Prepared_statement *stmt;
Item_param *param;
#ifndef EMBEDDED_LIBRARY
- char *packet_end= packet + packet_length - 1;
+ char *packet_end= packet + packet_length;
#endif
DBUG_ENTER("mysql_stmt_get_longdata");
status_var_increment(thd->status_var.com_stmt_send_long_data);
#ifndef EMBEDDED_LIBRARY
/* Minimal size of long data packet is 6 bytes */
- if (packet_length <= MYSQL_LONG_DATA_HEADER)
+ if (packet_length < MYSQL_LONG_DATA_HEADER)
{
my_error(ER_WRONG_ARGUMENTS, MYF(0), "mysql_stmt_send_long_data");
DBUG_VOID_RETURN;
@@ -2860,9 +2860,7 @@
lip.stmt_prepare_mode= TRUE;
lex_start(thd);
- error= parse_sql(thd, &lip) ||
- thd->net.report_error ||
- init_param_array(this);
+ error= parse_sql(thd, &lip) || init_param_array(this);
/*
While doing context analysis of the query (in check_prepared_statement)
--- 1.240/tests/mysql_client_test.c 2007-06-12 01:20:42 +04:00
+++ 1.241/tests/mysql_client_test.c 2007-06-15 19:33:20 +04:00
@@ -12296,25 +12296,25 @@
int rc;
myheader("test_bug6081");
- rc= simple_command(mysql, COM_DROP_DB, current_db,
+ rc= simple_command(mysql, COM_DROP_DB, (uchar *) current_db,
(ulong)strlen(current_db), 0);
if (rc == 0 && mysql_errno(mysql) != ER_UNKNOWN_COM_ERROR)
{
myerror(NULL); /* purecov: inspected */
die(__FILE__, __LINE__, "COM_DROP_DB failed"); /* purecov: inspected */
}
- rc= simple_command(mysql, COM_DROP_DB, current_db,
- (ulong)strlen(current_db), 0);
+ rc= simple_command(mysql, COM_DROP_DB, (uchar*) current_db,
+ (ulong)strlen(current_db), 0U);
myquery_r(rc);
- rc= simple_command(mysql, COM_CREATE_DB, current_db,
- (ulong)strlen(current_db), 0);
+ rc= simple_command(mysql, COM_CREATE_DB, (uchar*) current_db,
+ (ulong)strlen(current_db), 0U);
if (rc == 0 && mysql_errno(mysql) != ER_UNKNOWN_COM_ERROR)
{
myerror(NULL); /* purecov: inspected */
die(__FILE__, __LINE__, "COM_CREATE_DB failed"); /* purecov: inspected */
}
- rc= simple_command(mysql, COM_CREATE_DB, current_db,
- (ulong)strlen(current_db), 0);
+ rc= simple_command(mysql, COM_CREATE_DB, (uchar*) current_db,
+ (ulong)strlen(current_db), 0U);
myquery_r(rc);
rc= mysql_select_db(mysql, current_db);
myquery(rc);
@@ -13540,7 +13540,8 @@
/* Fill in the fethc packet */
int4store(buff, stmt->stmt_id);
buff[4]= 1; /* prefetch rows */
- rc= ((*mysql->methods->advanced_command)(mysql, COM_STMT_FETCH, buff,
+ rc= ((*mysql->methods->advanced_command)(mysql, COM_STMT_FETCH,
+ (uchar*) buff,
sizeof(buff), 0,0,1,NULL) ||
(*mysql->methods->read_query_result)(mysql));
DIE_UNLESS(rc);
@@ -16106,7 +16107,7 @@
int rc;
MYSQL_RES *result;
- char utf8_func[] =
+ uchar utf8_func[] =
{
0xd1, 0x84, 0xd1, 0x83, 0xd0, 0xbd, 0xd0, 0xba,
0xd1, 0x86, 0xd0, 0xb8, 0xd0, 0xb9, 0xd0, 0xba,
@@ -16114,7 +16115,7 @@
0x00
};
- char utf8_param[] =
+ uchar utf8_param[] =
{
0xd0, 0xbf, 0xd0, 0xb0, 0xd1, 0x80, 0xd0, 0xb0,
0xd0, 0xbc, 0xd0, 0xb5, 0xd1, 0x82, 0xd1, 0x8a,
@@ -16213,6 +16214,204 @@
DBUG_VOID_RETURN;
}
+/*
+ Test mysql_change_user() C API and COM_CHANGE_USER
+*/
+
+static void test_change_user()
+{
+ char buff[256];
+ const char *user_pw= "mysqltest_pw";
+ const char *user_no_pw= "mysqltest_no_pw";
+ const char *pw= "password";
+ const char *db= "mysqltest_user_test_database";
+ int rc;
+
+ DBUG_ENTER("test_change_user");
+ myheader("test_change_user");
+
+ /* Prepare environment */
+ sprintf(buff, "drop database if exists %s", db);
+ rc= mysql_query(mysql, buff);
+ myquery(rc);
+
+ sprintf(buff, "create database %s", db);
+ rc= mysql_query(mysql, buff);
+ myquery(rc);
+
+ sprintf(buff,
+ "grant select on %s.* to %s@'%%' identified by '%s'",
+ db,
+ user_pw,
+ pw);
+ rc= mysql_query(mysql, buff);
+ myquery(rc);
+
+ sprintf(buff,
+ "grant select on %s.* to %s@'%%'",
+ db,
+ user_no_pw);
+ rc= mysql_query(mysql, buff);
+ myquery(rc);
+
+
+ /* Try some combinations */
+ rc= mysql_change_user(mysql, NULL, NULL, NULL);
+ DIE_UNLESS(rc);
+ if (! opt_silent)
+ printf("Got error (as expected): %s\n", mysql_error(mysql));
+
+
+ rc= mysql_change_user(mysql, "", NULL, NULL);
+ DIE_UNLESS(rc);
+ if (! opt_silent)
+ printf("Got error (as expected): %s\n", mysql_error(mysql));
+
+ rc= mysql_change_user(mysql, "", "", NULL);
+ DIE_UNLESS(rc);
+ if (! opt_silent)
+ printf("Got error (as expected): %s\n", mysql_error(mysql));
+
+ rc= mysql_change_user(mysql, "", "", "");
+ DIE_UNLESS(rc);
+ if (! opt_silent)
+ printf("Got error (as expected): %s\n", mysql_error(mysql));
+
+ rc= mysql_change_user(mysql, NULL, "", "");
+ DIE_UNLESS(rc);
+ if (! opt_silent)
+ printf("Got error (as expected): %s\n", mysql_error(mysql));
+
+
+ rc= mysql_change_user(mysql, NULL, NULL, "");
+ DIE_UNLESS(rc);
+ if (! opt_silent)
+ printf("Got error (as expected): %s\n", mysql_error(mysql));
+
+ rc= mysql_change_user(mysql, "", NULL, "");
+ DIE_UNLESS(rc);
+ if (! opt_silent)
+ printf("Got error (as expected): %s\n", mysql_error(mysql));
+
+ rc= mysql_change_user(mysql, user_pw, NULL, "");
+ DIE_UNLESS(rc);
+ if (! opt_silent)
+ printf("Got error (as expected): %s\n", mysql_error(mysql));
+
+ rc= mysql_change_user(mysql, user_pw, "", "");
+ DIE_UNLESS(rc);
+ if (! opt_silent)
+ printf("Got error (as expected): %s\n", mysql_error(mysql));
+
+ rc= mysql_change_user(mysql, user_pw, "", NULL);
+ DIE_UNLESS(rc);
+ if (! opt_silent)
+ printf("Got error (as expected): %s\n", mysql_error(mysql));
+
+ rc= mysql_change_user(mysql, user_pw, NULL, NULL);
+ DIE_UNLESS(rc);
+ if (! opt_silent)
+ printf("Got error (as expected): %s\n", mysql_error(mysql));
+
+ rc= mysql_change_user(mysql, user_pw, "", db);
+ DIE_UNLESS(rc);
+ if (! opt_silent)
+ printf("Got error (as expected): %s\n", mysql_error(mysql));
+
+ rc= mysql_change_user(mysql, user_pw, NULL, db);
+ DIE_UNLESS(rc);
+ if (! opt_silent)
+ printf("Got error (as expected): %s\n", mysql_error(mysql));
+
+ rc= mysql_change_user(mysql, user_pw, pw, db);
+ myquery(rc);
+
+ rc= mysql_change_user(mysql, user_pw, pw, NULL);
+ myquery(rc);
+
+ rc= mysql_change_user(mysql, user_pw, pw, "");
+ myquery(rc);
+
+ rc= mysql_change_user(mysql, user_no_pw, pw, db);
+ DIE_UNLESS(rc);
+ if (! opt_silent)
+ printf("Got error (as expected): %s\n", mysql_error(mysql));
+
+ rc= mysql_change_user(mysql, user_no_pw, pw, "");
+ DIE_UNLESS(rc);
+ if (! opt_silent)
+ printf("Got error (as expected): %s\n", mysql_error(mysql));
+
+ rc= mysql_change_user(mysql, user_no_pw, pw, NULL);
+ DIE_UNLESS(rc);
+ if (! opt_silent)
+ printf("Got error (as expected): %s\n", mysql_error(mysql));
+
+ rc= mysql_change_user(mysql, user_no_pw, "", NULL);
+ myquery(rc);
+
+ rc= mysql_change_user(mysql, user_no_pw, "", "");
+ myquery(rc);
+
+ rc= mysql_change_user(mysql, user_no_pw, "", db);
+ myquery(rc);
+
+ rc= mysql_change_user(mysql, user_no_pw, NULL, db);
+ myquery(rc);
+
+ rc= mysql_change_user(mysql, "", pw, db);
+ DIE_UNLESS(rc);
+ if (! opt_silent)
+ printf("Got error (as expected): %s\n", mysql_error(mysql));
+
+ rc= mysql_change_user(mysql, "", pw, "");
+ DIE_UNLESS(rc);
+ if (! opt_silent)
+ printf("Got error (as expected): %s\n", mysql_error(mysql));
+
+ rc= mysql_change_user(mysql, "", pw, NULL);
+ DIE_UNLESS(rc);
+ if (! opt_silent)
+ printf("Got error (as expected): %s\n", mysql_error(mysql));
+
+ rc= mysql_change_user(mysql, NULL, pw, NULL);
+ DIE_UNLESS(rc);
+ if (! opt_silent)
+ printf("Got error (as expected): %s\n", mysql_error(mysql));
+
+ rc= mysql_change_user(mysql, NULL, NULL, db);
+ DIE_UNLESS(rc);
+ if (! opt_silent)
+ printf("Got error (as expected): %s\n", mysql_error(mysql));
+
+ rc= mysql_change_user(mysql, NULL, "", db);
+ DIE_UNLESS(rc);
+ if (! opt_silent)
+ printf("Got error (as expected): %s\n", mysql_error(mysql));
+
+ rc= mysql_change_user(mysql, "", "", db);
+ DIE_UNLESS(rc);
+ if (! opt_silent)
+ printf("Got error (as expected): %s\n", mysql_error(mysql));
+
+ /* Cleanup the environment */
+
+ mysql_change_user(mysql, opt_user, opt_password, current_db);
+
+ sprintf(buff, "drop database %s", db);
+ rc= mysql_query(mysql, buff);
+ myquery(rc);
+
+ sprintf(buff, "drop user %s@'%%'", user_pw);
+ rc= mysql_query(mysql, buff);
+ myquery(rc);
+
+ sprintf(buff, "drop user %s@'%%'", user_no_pw);
+ rc= mysql_query(mysql, buff);
+ myquery(rc);
+
+ DBUG_VOID_RETURN;
+}
/*
Read and parse arguments and MySQL options from my.cnf
@@ -16503,6 +16702,7 @@
#endif
{ "test_bug27876", test_bug27876 },
{ "test_bug27592", test_bug27592 },
+ { "test_change_user", test_change_user },
{ 0, 0 }
};
| Thread |
|---|
| • bk commit into 5.1 tree (kostja:1.2556) | konstantin | 17 Jun |
