List:Commits« Previous MessageNext Message »
From:Luís Soares Date:July 21 2010 11:52pm
Subject:Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:3423) Bug#54197
View as plain text  
Hi Alfranio,



On 07/14/2010 11:14 AM, Alfranio Correia wrote:
>
> Hi Luis,
>
> Patch conditionally approved.
>
> The patch looks good. However, I was wondering if the fact that some
> commands call my_ok and others do not is not a bug.
> Could you explain why this happens?

Some events may call my_ok as part of apply_event, eg,
query_log_event calls mysql_parse:

   https://intranet.mysql.com/secure/paste/displaypaste.php?codeid=11767

while others don't, eg, format_description_log_event->apply_event:

   https://intranet.mysql.com/secure/paste/displaypaste.php?codeid=11766

And that's just design issue, as query_log_event, for instance, relies
on mysql_parse execution path to execute the statement.

Regards,
Luís

> Cheers.
>
>
> On 06/21/2010 01:05 AM, Luis Soares wrote:
>> #At
>> file:///home/lsoares/Workspace/bzr/work/bugfixing/54197/mysql-5.1-bugteam/
>> based on revid:georgi.kodinov@stripped
>>
>> 3423 Luis Soares 2010-06-21
>> BUG#54197: 'BINLOG ' statements crash the server
>>
>> Some BINLOG statements would make the server hit an assertion,
>> because they would lead to setting thd->main_da.m_status
>> twice. This was caused because the execution of some BINLOG
>> statements (in base 64 output) make my_ok to be called before
>> returning from ev->apply_event. Then, my_ok would be called
>> again, right before returning from mysql_client_binlog_statement,
>> triggering the assertion.
>>
>> We fix this by:
>>
>> 1. moving the call to my_ok from inside
>> mysql_client_binlog_statement function, to
>> mysql_execute_command
>>
>> 2. changing the signature of mysql_client_binlog_statement so
>> that it returns non-zero on error and zero otherwise.
>>
>> 3. calling my_ok in mysql_execute_command conditionally, ie, if
>> the status hasn't been set yet.
>>
>> modified:
>> mysql-test/r/mysqlbinlog_base64.result
>> mysql-test/t/mysqlbinlog_base64.test
>> sql/mysql_priv.h
>> sql/sql_binlog.cc
>> sql/sql_parse.cc
>> === modified file 'mysql-test/r/mysqlbinlog_base64.result'
>> --- a/mysql-test/r/mysqlbinlog_base64.result 2008-07-28 07:15:20 +0000
>> +++ b/mysql-test/r/mysqlbinlog_base64.result 2010-06-21 00:05:45 +0000
>> @@ -109,3 +109,9 @@ count(*)
>> 35840
>> drop table t1;
>> drop table t2;
>> +RESET MASTER;
>> +CREATE TABLE t1(c1 INT) ENGINE=myisam;
>> +FLUSH LOGS;
>> +INSERT INTO t1 VALUES(1),(2);
>> +DROP TABLE t1;
>> +DROP TABLE t1;
>>
>> === modified file 'mysql-test/t/mysqlbinlog_base64.test'
>> --- a/mysql-test/t/mysqlbinlog_base64.test 2008-07-28 07:15:20 +0000
>> +++ b/mysql-test/t/mysqlbinlog_base64.test 2010-06-21 00:05:45 +0000
>> @@ -71,3 +71,23 @@ select count(*) from t2;
>> --remove_file $MYSQLTEST_VARDIR/tmp/mysqlbinlog_base64.sql
>> drop table t1;
>> drop table t2;
>> +
>> +#
>> +# BUG#54197 'BINLOG ' statements crash the server
>> +#
>> +
>> +RESET MASTER;
>> +
>> +CREATE TABLE t1(c1 INT) ENGINE=myisam;
>> +FLUSH LOGS;
>> +INSERT INTO t1 VALUES(1),(2);
>> +
>> +-- let $fname_prefix= `SELECT UUID()`;
>> +-- let $outfile= $MYSQL_TEST_DIR/var/tmp/$fname_prefix.sql
>> +
>> +--let $MYSQLD_DATADIR=`select @@datadir`
>> +--exec $MYSQL_BINLOG --base64-output
>> $MYSQLD_DATADIR/master-bin.000001> $outfile
>> +DROP TABLE t1;
>> +--exec $MYSQL test< $outfile
>> +-- remove_file $outfile
>> +DROP TABLE t1;
>>
>> === modified file 'sql/mysql_priv.h'
>> --- a/sql/mysql_priv.h 2010-05-27 20:07:40 +0000
>> +++ b/sql/mysql_priv.h 2010-06-21 00:05:45 +0000
>> @@ -1045,7 +1045,7 @@ bool mysql_alter_db(THD *thd, const char
>> bool mysql_rm_db(THD *thd,char *db,bool if_exists, bool silent);
>> bool mysql_upgrade_db(THD *thd, LEX_STRING *old_db);
>> void mysql_binlog_send(THD* thd, char* log_ident, my_off_t pos, ushort
>> flags);
>> -void mysql_client_binlog_statement(THD *thd);
>> +int mysql_client_binlog_statement(THD *thd);
>> bool mysql_rm_table(THD *thd,TABLE_LIST *tables, my_bool if_exists,
>> my_bool drop_temporary);
>> int mysql_rm_table_part2(THD *thd, TABLE_LIST *tables, bool if_exists,
>>
>> === modified file 'sql/sql_binlog.cc'
>> --- a/sql/sql_binlog.cc 2009-10-14 01:39:05 +0000
>> +++ b/sql/sql_binlog.cc 2010-06-21 00:05:45 +0000
>> @@ -31,7 +31,7 @@
>> statement.
>> */
>>
>> -void mysql_client_binlog_statement(THD* thd)
>> +int mysql_client_binlog_statement(THD* thd)
>> {
>> DBUG_ENTER("mysql_client_binlog_statement");
>> DBUG_PRINT("info",("binlog base64: '%*s'",
>> @@ -40,7 +40,7 @@ void mysql_client_binlog_statement(THD*
>> thd->lex->comment.str));
>>
>> if (check_global_access(thd, SUPER_ACL))
>> - DBUG_VOID_RETURN;
>> + DBUG_RETURN(1);
>>
>> size_t coded_len= thd->lex->comment.length + 1;
>> size_t decoded_len= base64_needed_decoded_length(coded_len);
>> @@ -56,7 +56,7 @@ void mysql_client_binlog_statement(THD*
>> Format_description_event.
>> */
>> my_bool have_fd_event= TRUE;
>> - int err;
>> + int err= 0;
>> Relay_log_info *rli;
>> rli= thd->rli_fake;
>> if (!rli)
>> @@ -85,6 +85,7 @@ void mysql_client_binlog_statement(THD*
>> rli->relay_log.description_event_for_exec&&
>> buf))
>> {
>> + err= 1;
>> my_error(ER_OUTOFMEMORY, MYF(0), 1); /* needed 1 bytes */
>> goto end;
>> }
>> @@ -111,6 +112,7 @@ void mysql_client_binlog_statement(THD*
>>
>> if (bytes_decoded< 0)
>> {
>> + err= 1;
>> my_error(ER_BASE64_DECODE_ERROR, MYF(0));
>> goto end;
>> }
>> @@ -150,6 +152,7 @@ void mysql_client_binlog_statement(THD*
>> event_len, bytes_decoded));
>> if (bytes_decoded< EVENT_LEN_OFFSET || (uint) bytes_decoded< event_len)
>> {
>> + err= 1;
>> my_error(ER_SYNTAX_ERROR, MYF(0));
>> goto end;
>> }
>> @@ -166,6 +169,7 @@ void mysql_client_binlog_statement(THD*
>> have_fd_event= TRUE;
>> else
>> {
>> + err= 1;
>> my_error(ER_NO_FORMAT_DESCRIPTION_EVENT_BEFORE_BINLOG_STATEMENT,
>> MYF(0), Log_event::get_type_str((Log_event_type)type));
>> goto end;
>> @@ -183,6 +187,7 @@ void mysql_client_binlog_statement(THD*
>> causes by a bad statement
>> */
>> my_error(ER_SYNTAX_ERROR, MYF(0));
>> + err= 1;
>> goto end;
>> }
>>
>> @@ -238,10 +243,9 @@ void mysql_client_binlog_statement(THD*
>>
>>
>> DBUG_PRINT("info",("binlog base64 execution finished successfully"));
>> - my_ok(thd);
>>
>> end:
>> rli->clear_tables_to_lock();
>> my_free(buf, MYF(MY_ALLOW_ZERO_PTR));
>> - DBUG_VOID_RETURN;
>> + DBUG_RETURN(err);
>> }
>>
>> === modified file 'sql/sql_parse.cc'
>> --- a/sql/sql_parse.cc 2010-06-07 10:01:54 +0000
>> +++ b/sql/sql_parse.cc 2010-06-21 00:05:45 +0000
>> @@ -4922,7 +4922,15 @@ create_sp_error:
>> case SQLCOM_BINLOG_BASE64_EVENT:
>> {
>> #ifndef EMBEDDED_LIBRARY
>> - mysql_client_binlog_statement(thd);
>> + if (!mysql_client_binlog_statement(thd)&& !thd->main_da.is_set())
>> + /*
>> + Applying some 'BINLOG' statements makes my_ok to be called
>> + right after applying the event (ev->apply_event). So we
>> + need to check if the thd status hasn't yet been set, before
>> + we set it here, thence not risking setting OK twice.
>> + See BUG#54197.
>> + */
>> + my_ok(thd);
>> #else /* EMBEDDED_LIBRARY */
>> my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0), "embedded");
>> #endif /* EMBEDDED_LIBRARY */
>>
>>
>>
>>
>>
>

Thread
bzr commit into mysql-5.1-bugteam branch (luis.soares:3423) Bug#54197Luis Soares21 Jun
  • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:3423)Bug#54197Libing Song4 Jul
  • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:3423)Bug#54197Alfranio Correia14 Jul
    • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:3423) Bug#54197Luís Soares22 Jul
      • Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:3423) Bug#54197Alfranio Correia30 Jul