List:Commits« Previous MessageNext Message »
From:Alfranio Correia Date:July 14 2010 10:14am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (luis.soares:3423)
Bug#54197
View as plain text  
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?

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