From: Alfranio Correia
Date: July 30 2010 3:26pm
Subject: Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:3423) Bug#54197
List-Archive: http://lists.mysql.com/commits/114768
Message-Id: <4C52EF19.80205@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 8bit

Hi Luis,

On 07/22/2010 12:52 AM, Luís Soares wrote:
> 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

I am ok with the current patch but please file a new bug report or put the
information you mentioned above somewhere because I think the design
should be changed.

Cheers.


>
>> 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 */
>>>
>>>
>>>
>>>
>>>
>>
>